Yay. Just out of curiosity, do we know what happens when we hit this endpoint with a POST or PUT. I'm expecting a 405 Method Not Allowed.

See if you can add these as args to the action.

def revertPick(String id, String itemId) {
    picklistService.revertPick(id, itemId)
}

The itemId argument must map to PicklistItem.id in order for the API to make sense. If we want to revert ALL picklist items for a requisition item, we'll need to think about this differently.

For now, we could have a few different methods to revert picks.

void revertPick(PicklistItem picklis) {  ... }
void revertPick(PicklistItem picklistItem) {  ... }
void revertPick(List<PicklistItem> picklistItems) {  
    picklistItems.each { PicklistItem picklistItem ->
        revertPick(picklistItem)    
    }
}
void revertPick(RequisitionItem requisitionItem) { 
    revertPick(requisitionItem.picklistItems)
}

And this method would look like this ..

Picklist picklist = Picklist.get(id)
PicklistItem picklistItem = PicklistItem.get(itemId)
RequisitionItem requisitionItem = picklistItem.requisitionItem)

// Revert all picks for the given requisition item, 
// disable refresh product availability on hibernate event
requisitionItem.disableRefresh = true
revertPick(requisitionItem)

// Or if we can't pass the disableRefresh from the parent, we could do something like this
//requisitionItem.picklistItems.each { it.disableRefresh = true } 
//revertPick(requisitionItem)

// fire refresh event for the product (ignore bin locations)
productAvailabilityService.refreshProductAvailability(
    picklist?.requisition?.origin?.id,
    [requisitionItem?.product?.id],
    [],
    true)  // can't remember if this should be 
}

Or perhaps the API endpoint should look like this for now.

DELETE /api/stockMovementItems/:id/picklistItems

Key points above

• Approach 1
  • itemId should map to PicklistItem
  • how you deal with getting from a picklist item to all picklist items for a requisition item is up to you
  • when deleting picklist items we probably want to refresh product availability for the entire product, not just the bin locations that are effected.
• Approach 2
  • Move the action to the StockMovementItemApiController to ensure a consistent / coherent URL scheme
  • The other points are likely the same.

@jmiranda Yup, I agree that the current solution with DELETE /api/picklists/$id/items/$itemId and passing requisition item id as itemId is not perfect and might be misleading or cause some confusion. When I was looking at the code I was thinking about moving the current revertPick (with some minor adjustments if needed) under DELETE /api/requisitionItems/:id/picklistItems or DELETE /api/stockMovementItems/:id/picklistItems (the first one was my first thought, but considering it is stock movement related, then I think the latter one is more suiting)

Hopefully the requisitionItem already has an association to its picklistItems. There's probably going to be a lazy query here, but better to go through the model to avoid unnecessary queries.

Should be just requisitionItem.picklistItems

This will probably trigger a product availability refresh so we should try to disableRefresh before deleting.

Yup, it has to be checked and disabled if needed.

As discussed in Approach 1 above we might want to refresh at the product level, rather than at the bin location level to avoid any weird edge cases.

As mentioned above, we probably want to force refresh on the product level.

// fire refresh event for the product (ignore bin locations)
productAvailabilityService.refreshProductAvailability(
    picklist?.requisition?.origin?.id,
    [requisitionItem?.product?.id],
    [],
    true)  // can't remember if we should force refresh or not
}

@awalkowiak What do you think about that?

@jmiranda Force refresh deletes the line from the table first, I think we don't have to do it here and just doing the line update is enough.

We should be more obvious here to avoid confusion / regression later. If there's a baseUrl shared by multiple endpoints, let's define the base URL. Also

export const PICKLIST_API = `${API}/picklists`;
export const PICKLIST_ITEM_API = `${API}/picklists/${id}/items`;
export const PICKLIST_ITEM_CLEAR = id => `${PICKLIST_ITEMS_API}`;
export const PICKLIST_ITEM_REVERT = (id, itemId) => `${PICKLIST_ITEMS_API(id)}/${itemId}`;

👍 this could be improved

I would call this just after attempting to get a picklist, to avoid some unnecessary queries (e.g. the one from 331th), if they are not gonna be used either way.

@jmiranda Yup, I agree that the current solution with DELETE /api/picklists/$id/items/$itemId and passing requisition item id as itemId is not perfect and might be misleading or cause some confusion. When I was looking at the code I was thinking about moving the current revertPick (with some minor adjustments if needed) under DELETE /api/requisitionItems/:id/picklistItems or DELETE /api/stockMovementItems/:id/picklistItems (the first one was my first thought, but considering it is stock movement related, then I think the latter one is more suiting)

Should be just requisitionItem.picklistItems

Yup, it has to be checked and disabled if needed.

If you have requisitionItem, then having [requisitionItem?.product?.id] is enough, because there is no way there will be different products for picklist items for the same requisition item.

@jmiranda Force refresh deletes the line from the table first, I think we don't have to do it here and just doing the line update is enough.

👍 this could be improved

This endpoint needs to be improved changed as it was discussed in the comment in service and on slack

One minor thing. I think this should already take RequsitionItem as a param (or at least change the name of this param to requisitionItemId, so it is clear what it expects).