OBPIH-7186 Change expiration date confirm modal in old inbound workflow#5591
OBPIH-7186 Change expiration date confirm modal in old inbound workflow#5591alannadolny merged 4 commits intodevelopfrom
Conversation
SebastianLib
requested review from
alannadolny,
awalkowiak,
ewaterman and
kchelstowski
github-actions
bot
added
the
domain: frontend
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5591 +/- ##
============================================
- Coverage 9.12% 8.50% -0.63%
+ Complexity 1170 1116 -54
============================================
Files 701 711 +10
Lines 45281 45583 +302
Branches 10851 10907 +56
============================================
- Hits 4131 3875 -256
- Misses 40497 41134 +637
+ Partials 653 574 -79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| item.inventoryItem && | ||
| item.inventoryItem.expirationDate !== item.expirationDate && | ||
| item.inventoryItem.quantity && | ||
| item.inventoryItem.quantity !== '0', |
There was a problem hiding this comment.
you seem to do two .some methods to check those, and here, the .filter kinda "combines" those two statements.
Instead of checking hasExpiryMismatch and do a nested hasNonZeroQuantity and then checking if itemsWithMismatchedExpiry size is > 0, can't you just do the itemsWithMismatchedExpiry length check immediately?
TL:DR: I think you can get rid of ifs from 648 and 653 lines and just keep the one from 672.
There was a problem hiding this comment.
I did it on purpose. We want to display this "warning" modal only when an item has a different inventoryItem.expirationDate and its inventoryItem.quantity is not equal to 0. We don't want to display the modal for items that only have a different inventoryItem.expirationDate but their inventoryItem.quantity is 0 although we still want to update inventory Item. That's why I use two .some
I know it's a bit weird, but that's how it was working before my changes. Maybe we should just display everything?
There was a problem hiding this comment.
but in the filter you don't check OR, but AND, so you filter items that align with what you wrote so:
We want to display this "warning" modal only when an item has a different inventoryItem.expirationDate and its inventoryItem.quantity is not equal to 0
There was a problem hiding this comment.
We check in the filter "AND" not "OR" because some items might only meet the first .some() (different inventoryItem.expirationDate). In that case, we still want to update them but not show them in the modal.
The modal should only display items that meet both conditions (different inventoryItem.expirationDate and inventoryItem.quantity not equal 0), so we can’t use an OR there.
There was a problem hiding this comment.
Ok @SebastianLib , after you sent me the whole method on Slack, I understand the case.
Then what I think is worth doing is negating the logic, so to check if (!hasExpiry...) immedietaly and eventually do the return transitionToNextStep at the top.
By doing so, you will get rid of one nested if.
|
|
||
| if (itemsWithMismatchedExpiry.length > 0) { | ||
| const shouldUpdateExpirationDate = await | ||
| this.confirmExpirationDateSave(itemsWithMismatchedExpiry); |
There was a problem hiding this comment.
the formatting looks weird here. don't break the await calls to another line. If you need to move it due to the eslint error as the line is too long, move it together with the await.
| variant="primary" | ||
| onClick={onConfirm} | ||
| /> | ||
| <div data-testid="modal-with-table"> |
There was a problem hiding this comment.
@alannadolny did I set the data-testid in the correct place?
3 participants
✨ Description of Change
Link to GitHub issue or Jira ticket:
https://pihemr.atlassian.net/browse/OBPIH-7186
Description:
There is a small chance that the new inbound will be released now, so we also need to add this new confirmation modal to the old inbound workflow
📷 Screenshots & Recordings (optional)