OBGM-471 Fix revising items on outbound stock movements#4117
OBGM-471 Fix revising items on outbound stock movements#4117kchelstowski merged 1 commit intofeature/upgrade-to-grails-3.3.10from
Conversation
awalkowiak
commented
Jun 20, 2023
awalkowiak
commented
- bring back checking if there is need to undo previous changes
- fix requisition refresh
- minor NPE fix
- bring back checking if there is need to undo previous changes - fix requisition refresh - minor NPE fix
|
I am looking forward to hearing more about this one. |
|
|
||
| Set<RequisitionItem> getSubstitutionItems() { | ||
| return requisitionItems.findAll { | ||
| return requisitionItems?.findAll { RequisitionItem it -> |
There was a problem hiding this comment.
I'm ok with these changes but can you explain why both of these were necessary?
There was a problem hiding this comment.
I think the latter was required in the end. I was getting NullPointerException here locally, and this was a fix to it.
| removeShipmentAndPicklistItemsForModifiedRequisitionItem(requisitionItem) | ||
| requisitionItem.undoChanges() | ||
| if (requisitionItem.isChanged() || requisitionItem.isCanceled()) { | ||
| requisitionItem.undoChanges() |
|
|
||
| void createMissingShipmentItems(StockMovement stockMovement) { | ||
| Requisition requisition = stockMovement.requisition.refresh() | ||
| Requisition requisition = Requisition.get(stockMovement.id) |
There was a problem hiding this comment.
I assume we're getting this requisition from the session so we should be getting the same object. Curious why we used refresh in the first place (https://pihemr.atlassian.net/browse/OBPIH-1257).
As such, I wonder if this
Requisition requisition = stockMovement.requisition
would suffice.
One worry I have is with stockMovement.id which could potentially represent another object like order, shipment, etc. Would that ever be the case? And if not, we should be explicit about handling these cases i.e. throw an exception.
There was a problem hiding this comment.
But that could be done as a post-migration refactoring.
I'm not a big fan of these methods, in general. For one, they are too generically named and should be part of a more specific to the work we're trying to accomplish i.e.
void refreshPicklist(RequisitionItem requisitionItem)
There was a problem hiding this comment.
Currently, this one is only used for Requisition-based movements (outbound), but true since stock movement could represent various things, this could be indicated better. I definitely agree with you, on your second comment. This definitely could be something for post-migration refactoring.
I'll check locally how looks:
Requisition requisition = stockMovement.requisitionafter revising, and let you know.
There was a problem hiding this comment.
I checked locally the stockMovement.requisition and looks like I had weird glitch or whatever, because all versions:
stockmovement.requisition
stockmovement.requisition.refresh()
Requisition.get(stockmovement.id)
now all return the same state of requisitionItems. 🤔
I'll change it to the stockmovement.requisition and push within my next PR.
jmiranda
left a comment
jmiranda
left a comment
There was a problem hiding this comment.
Btw, I'm ok with these changes so feel free to merge if you get an approval.
|
|
||
| void createMissingShipmentItems(StockMovement stockMovement) { | ||
| Requisition requisition = stockMovement.requisition.refresh() | ||
| Requisition requisition = Requisition.get(stockMovement.id) |
There was a problem hiding this comment.
@jmiranda I assume the refresh() (now the fetch) is done to "undo" any changes introduced to requisition object, but it needs to be confirmed by @awalkowiak
Since code LGTM, I will merge this one, so that it could be tested before the end of the sprint, and we can discuss your doubts later if it sounds OK.
3 participants
