Can't you just do:

I do not see values in the code 🤔

yeah, you are right. Previously I used stockMovementId so I had to do it this way, and when changing it to picklist, I have not noticed I could shorten it. Thanks.

😄

We should delete this method, correct?

I mentioned before that I'd be ok with a DELETE action on an abstract nested resource off the stock movement API. Something like this.

DELETE /api/stockMovement/:id/allocations

But I'd probably wait on that until we have a chance to put it through a design review, since I'd probably propose something like this once we do the Order Everywhere refactor.

DELETE /api/orders/:id/allocations

The endpoints I think we need to create now should have very straightforward and obvious implementations.

• DELETE /api/picklists -> ignore or throw an error?
• DELETE /api/picklists/:id -> delete the picklist associated with the given id along with all of its items
• DELETE /api/picklists/:id/items -> delete all picklist items of the picklist associated with the given id

If this is not needed right now, and we are going to rely only on the picklist API, then let's remove this for now.

yeah, sorry for the confusion, I just forgot to remove it from here, after moving it to the PicklistApiController

I'm cool with returning 204 here, but what's the expected behavior for when there are no items to delete.

For a single resource instance, the convention seems to be 200 Ok, 204 No Content, or even 202 Accepted.

HTTP 1.1 Spec says

If a DELETE method is successfully applied, the origin server SHOULD
send a 202 (Accepted) status code if the action will likely succeed
but has not yet been enacted, a 204 (No Content) status code if the
action has been enacted and no further information is to be supplied,
or a 200 (OK) status code if the action has been enacted and the
response message includes a representation describing the status.

Source: https://www.rfc-editor.org/rfc/rfc7231#section-4.3.5

Here are some deeper discussions that HTTP spec and REST guidelines.

• https://stackoverflow.com/questions/6439416/status-code-when-deleting-a-resource-using-http-delete-for-the-second-time
• https://stackoverflow.com/questions/2342579/http-status-code-for-update-and-delete

But for deleting a collection, it's a little tough to research since there's no consensus that we should even allow deleting a resource collection.

Aside: I am making the argument that it is fine to delete a nested resource collection (/api/picklists/:id/items) but not a parent resource collection (/api/picklists).

Here's a stackoverflow about deleting "some" of a resource collection, but it doesn't discuss response codes.
https://stackoverflow.com/questions/37138720/restful-api-delete-some-of-a-resource-collection

If anyone finds good documentation on response codes for deleting a resource collection and in particular first vs subsequent requests with different results i.e. deleted X items vs. resource collection has no items vs deleted 0 items because there were filters applied to the nested resource collection

• /api/picklists/:id/items
• /api/picklists/:id/items
• /api/picklists/:id/items?filter=value

@jmiranda Definitely topic worth to extend, but from my understanding, since we do not return anything in the response, we should be fine with the 204, since this is "No Content", even though we delete a collection.
I've always said I was "pro" a silent delete - so that e.g. even if there is nothing to delete or a resource of given id is not found, we still return 204.
This is about idempotency of HTTP methods. Looking at many resources, it feels like DELETE is idempotent, hence it should return the same response for a single request even if called many times, so in our case - even if we attempt to delete an empty collection, we should still get 204.
https://stackoverflow.com/a/6440374/23546798

So regardless of whether you DELETE an existing resource, or attempt to DELETE a resource that does not exist, the server resource state is the same.

Make binLocations a Set and we probably don't need to deal with the unique issue.

It can't be a Set, because the product availability method requires a List, and it breaks, if the binLocations become the Set. I think refactoring it now (the PA method), or creating another overloaded method with Set instead of List does not make sense, unless you feel strong about it.

This is a bit misleading at first glance. I think it's doing this implicitly

picklist?.requisition?.requisitionItems?.collect { it?.product?.id } 

so let's make that explicit so it's slightly more readable and less confusing.

Set<String> productIds = picklist?.requisition?.requisitionItems?.collect { it?.product?.id } 
productAvailabilityService.refreshProductsAvailability(picklist?.requisition?.origin?.id,
                productIds, binLocations, false)

Note: The collect method might not allow the return to be a set, so if necessary turn it into a List and add .unique() at the end.

Yeah, this is doing the .collect behind the scenes. I've just used "groovy magic" :D but I'm fine with doing it implicitly

If this is not needed right now, and we are going to rely only on the picklist API, then let's remove this for now.

This is the second void clearPicklist(String id) in the code. The other one is in the StockMovementService. Please add a comment explaining that the parameter with id here is meant to be picklistItem picklist id, and in the other one that it expects stock movement id. I would even consider deprecating the other one, but we have not discussed it so it is just a thought.

I will add the comments, but imho it's kinda self-explonatory that if I use the PicklistService, we are rather referring to the picklist id (unless the argument was named differently), and the same with the StockMovementService - if I use id as an argument, it's rather the SM id.

Technically it is self-explanatory, but I feel like there was a lot of self-explanatory code in this repo, that is now not that self-explanatory 😅

To be honest I expected something more like a javadoc with [at]param, but that can be adjusted in the next pr from that epic.

yeah, you are right, I will try to remember to change it to the javadoc.

There is one unknown here. Initially I thought this should be changed to pick only when we trigger clear picklist, but let's leave it as is for now, and let's see what Manon will say about that part.

I got confirmation, that your current version is correct. You can disregard the previous comment here.

won't this add a null to the list if there's no binLocation? What's going to happen in the productAvailability if that happens?

actually we have a use case for the null binLocation. It's the Default bin location. It's quite difficult to understand on first sight, so maybe either Justin or Artur could explain it in more details if you were curious.

Essentially, looking at the stockcard of a product:

the first inventory item's binLocation is null.

gotcha. Thanks for the explanation!

To be honest I expected something more like a javadoc with [at]param, but that can be adjusted in the next pr from that epic.