OBS-1269 Remove dangling references when calling StockMovementService.reviseItems() #3530
Conversation
There was a problem hiding this comment.
I'm going to defer to @awalkowiak on this one. It seems reasonable but I understand why these changes would fix the issue.
|
|
||
| // Cannot cancel quantity if it has already been canceled | ||
| if (requisitionItem.quantityCanceled) { | ||
| requisitionItem.undoChanges() |
There was a problem hiding this comment.
I can't think of under what circumstances this would occur, but there might be cases where we aren't canceling the quantity so we don't want to undo the changes. But I'll defer to @awalkowiak because this code has changed a lot since it was first written.
There was a problem hiding this comment.
@jmiranda I tried to be as nit picky in the review as I could, I pulled it locally too, and it seems to be fine. The undo button basically under the hood is doing the:
removeShipmentAndPicklistItemsForModifiedRequisitionItem(requisitionItem)
requisitionItem.undoChanges()
so we are simply doing the revert item first here. I think Katarzyna will find all the edge cases with it (for me it looks good).
Btw @jmiranda I found NPE in your recent changes in the requisitionItem.undoChanges() here: https://github.com/openboxes/openboxes/blob/develop/grails-app/domain/org/pih/warehouse/requisition/RequisitionItem.groovy#L224. I am assuming, that it should be substitutionItem.disableRefresh = Boolean.TRUE, I'll push a fixing commit within this PR.
cc @mdpearson
|
|
||
| class RequisitionItem implements Comparable<RequisitionItem>, Serializable { | ||
|
|
||
| private static final transient Logger log = LoggerFactory.getLogger(RequisitionItem) |
There was a problem hiding this comment.
There should be an an implicit logger on all services, controller, and domain classes so you can probably remove this. Not sure why I used println here.
There was a problem hiding this comment.
There should be, but the implicit logger doesn't come up during unit tests, which is why the previous PR failed tests (cf. f40f82b). I have no idea why it doesn't under test (an implicit logger is created when the application runs), but being explicit here fixes tests and also works properly when the app runs. That may be why you put a println here (there's only one other log statement in the whole domain/ directory)
PS this is me looking forward to grails 3
| @@ -1890,20 +1889,14 @@ class StockMovementService { | |||
| } | |||
|
|
|||
| if (!requisitionItem) { | |||
| throw new IllegalArgumentException("Could not find stock movement item with ID ${stockMovementItem.id}") | |||
| throw new IllegalArgumentException("Could not find requisition item for stock movement item with ID ${stockMovementItem.id}") | |||
There was a problem hiding this comment.
This is unnecessary. A requisition item is for all intents and purposes a stock movement item. You cannot have a stock movement item without one.
There was a problem hiding this comment.
ok, good to know. Backed out in c4f4614
|
|
||
| // Cannot cancel quantity if it has already been canceled | ||
| if (requisitionItem.quantityCanceled) { | ||
| requisitionItem.undoChanges() |
There was a problem hiding this comment.
@jmiranda I tried to be as nit picky in the review as I could, I pulled it locally too, and it seems to be fine. The undo button basically under the hood is doing the:
removeShipmentAndPicklistItemsForModifiedRequisitionItem(requisitionItem)
requisitionItem.undoChanges()
so we are simply doing the revert item first here. I think Katarzyna will find all the edge cases with it (for me it looks good).
Btw @jmiranda I found NPE in your recent changes in the requisitionItem.undoChanges() here: https://github.com/openboxes/openboxes/blob/develop/grails-app/domain/org/pih/warehouse/requisition/RequisitionItem.groovy#L224. I am assuming, that it should be substitutionItem.disableRefresh = Boolean.TRUE, I'll push a fixing commit within this PR.
cc @mdpearson
Hi @jmiranda, I was able to reproduce OBS-1269 by moving back and forth a few times between Edit, Pick and Ship and editing the quantity field without ever clicking the big red Undo button. More details are on the ticket.
The one-line summary of this change is that it basically presses the undo button for you if you don't press it yourself before editing the quantity.
In more detail,
reviseItems(), as written, has guards aroundremoveShipmentAndPicklistItemsForModifiedRequisitionItem()andRequisitionItem::undoChanges()which strikes me as both unnecessary and, at least in the case described in OBS-1269, incorrect. (Contrast therevertItem()implementation which calls them indiscriminately.)In fact, nowhere else in this file do I see guards around either of these methods, while both of them have sufficient logic internally to prevent doing the wrong thing if they are called when there is in fact nothing to undo (I tested this assumption by making a separate build that called each method twice while testing this patch).
I was able to go back and forth cheerfully between states while making any number of edits with this patch in place. Is there anything else you'd like me to look into to make sure I didn't break something elsewhere?