OBPIH-7184 fix1. only remove 0 quantity lines on SMs when proceeding#5601
OBPIH-7184 fix1. only remove 0 quantity lines on SMs when proceeding#5601alannadolny merged 2 commits intodevelopfrom
Conversation
ewaterman
requested review from
SebastianLib,
alannadolny,
awalkowiak and
kchelstowski
ewaterman
added
the
warn: do not merge
github-actions
bot
added
type: bug
| // TODO: This is pre-existing code but why are we using 'VERIFYING' here for non-suppliers? | ||
| // It's not a valid StockMovementStatusCode value. Should this be 'VALIDATED'?? | ||
| const status = (formValues.origin.type === 'SUPPLIER' || !formValues.hasManageInventory) | ||
| ? 'CHECKING' : 'VERIFYING'; |
There was a problem hiding this comment.
?? I don't get how this is working when we set status to 'VERIFYING'
There was a problem hiding this comment.
@ewaterman I briefly checked that and you are right this shouldn't work. But from what I can see this code is never executed. Probably after adding approval workflow, we now rely on the submitRequest method that executes saveRequisitionItems and transitionToNextStep, while the saveAndTransitionToNextStep is not used now.
| requisitionItem.lotNumber = stockMovementItem.lotNumber | ||
| requisitionItem.expirationDate = stockMovementItem.expirationDate | ||
| requisitionItem.comment = stockMovementItem.comments | ||
| } |
There was a problem hiding this comment.
this whole code change is just adding back in what I removed in https://github.com/openboxes/openboxes/pull/5570/files.
The only difference is the removeEmptyItems boolean
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5601 +/- ##
============================================
- Coverage 9.12% 8.49% -0.63%
+ Complexity 1170 1116 -54
============================================
Files 701 711 +10
Lines 45281 45591 +310
Branches 10851 10909 +58
============================================
- Hits 4131 3875 -256
- Misses 40497 41142 +645
+ Partials 653 574 -79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kchelstowski
left a comment
kchelstowski
left a comment
There was a problem hiding this comment.
From the code perspective it looks good to me.
| // TODO: This is pre-existing code but why are we using 'VERIFYING' here for non-suppliers? | ||
| // It's not a valid StockMovementStatusCode value. Should this be 'VALIDATED'?? | ||
| const status = (formValues.origin.type === 'SUPPLIER' || !formValues.hasManageInventory) | ||
| ? 'CHECKING' : 'VERIFYING'; |
There was a problem hiding this comment.
@ewaterman I briefly checked that and you are right this shouldn't work. But from what I can see this code is never executed. Probably after adding approval workflow, we now rely on the submitRequest method that executes saveRequisitionItems and transitionToNextStep, while the saveAndTransitionToNextStep is not used now.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
3 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: https://pihemr.atlassian.net/browse/OBPIH-7184
Description: Original PR: #5570
When you click the "next" or "submit request" buttons on the add items step of request/inbound/outbound two things happen:
/stockMovements/${id}/updateItemsto save the items. This is just a generic save items endpoint so there's no way to know the context in which we're saving (ex: we don't know when we're saving a draft via the "save" / "save and exit" button vs saving before proceeding to the next step via "next" button)./stockMovements/${id}/updateStatusto change the status. This is also a generic status change endpoint and has a bunch of "if status is changing to X, do Y" logic.I like that the save is generic (and don't especially like that the status change isn't), however the "delete requisition item if quantity == 0" logic was existing there (before I removed it in my last PR), so in this PR I needed to either:
I initially tried to implement 1 because that felt more proper to me. The frontend should just say "I want to go to state X" and the backend should be able to handle that (which would involve removing empty items). This would save us from needing to depend on the frontend to decide when a row is removed or not, which can get us into bad states if we're not careful. However it got too complex for me since the update logic itself is really complex and it seemed like we already rely quite heavily on the frontend knowing when to update items. A refactor of that was looking to be quite unfun.
So I went for solution 2, which felt easier...