OBPIH-7542 Fix reorder report not to remove products affected by expi…#5603
OBPIH-7542 Fix reorder report not to remove products affected by expi…#5603alannadolny merged 3 commits intodevelopfrom
Conversation
kchelstowski
requested review from
SebastianLib,
alannadolny,
awalkowiak and
ewaterman
github-actions
bot
added
type: feature
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5603 +/- ##
============================================
- Coverage 9.12% 8.50% -0.63%
+ Complexity 1170 1118 -52
============================================
Files 701 711 +10
Lines 45281 45594 +313
Branches 10851 10911 +60
============================================
- Hits 4131 3876 -255
- Misses 40497 41144 +647
+ Partials 653 574 -79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| inList("location", locations) | ||
|
|
||
| // The additional if check is needed to avoid joining product twice in two separate ifs for categories/tags, as it could throw an error | ||
| // if both categories and tags were provided. Another possible solution would be to store "usedAliases" and check if categories already joined the product. |
There was a problem hiding this comment.
you can probably remove this comment now
| // Final quantity available to promise - quantity available to promise subtracted by expired stock based on expiration filter | ||
| Integer finalQuantityAvailableToPromise = productAvailabilityRecords.sum { ProductAvailability paRecord -> | ||
| // We can return immedietaly if we do not want to subtract expired stock | ||
| if (expirationFilter == ExpirationFilter.DO_NOT_SUBTRACT_EXPIRED_STOCK) { |
There was a problem hiding this comment.
why do we need both SUBTRACT_EXPIRED_STOCK and DO_NOT_SUBTRACT_EXPIRED_STOCK? Can't we just check if expirationFilter != ExpirationFilter.SUBTRACT_EXPIRED_STOCK?
There was a problem hiding this comment.
For DO_NOT_SUBTRACT_EXPIRED_STOCK we return immediately, so this is what I added an additional if for that case.
As for SUBTRACT_EXPIRED_STOCK - it's mostly needed to calculate the maxDate and to know what sign to use > vs >= as for SUBTRACT_EXPIRED_STOCK we want to keep items that have the expiration day set for today.
SUBTRACT_EXPIRED_STOCK: item expires at 6th November - keep it (>=)
SUBTRACT_EXPIRING_IN_MONTH: item expires at 6th December - remove it (>)
| private static Map<String, Integer> calculateQuantitiesByExpirationFilter(List<ProductAvailability> productAvailabilityRecords, ExpirationFilter expirationFilter) { | ||
| boolean removeExpiredStock = expirationFilter == ExpirationFilter.SUBTRACT_EXPIRED_STOCK | ||
| Date today = new Date() | ||
| Date maxDate = removeExpiredStock ? today : today + expirationFilter.days |
There was a problem hiding this comment.
won't this break for DO_NOT_SUBTRACT_EXPIRED_STOCK since it doesn't have a value for days?
There was a problem hiding this comment.
it won't since as I explained above, for the DO_NOT_SUBTRACT_EXPIRED_STOCK we return at the top of the logic without doing any calculations, we just return QATP.
| } | ||
| // We want to include inventory items that either don't have expiration date or have expiration date after maxDate (or >= maxDate if removing expired stock) | ||
| InventoryItem inventoryItem = paRecord.inventoryItem | ||
| if (!inventoryItem.expirationDate || (removeExpiredStock ? inventoryItem.expirationDate >= maxDate : inventoryItem.expirationDate > maxDate)) { |
There was a problem hiding this comment.
Why do we need to distinguish here betwen the removeExpiredStock true/false case?
If our filter is "expiring within 30 days (or already expired)" then we exclude anything that is expiring in exactly 30 days.
Why not treat the already expired case the same? If our filter is "expiring within 0 days (or already expired)" then we exclude anything that is expiring today.
Maybe it's a nitpick but I don't see why we need a special case for the "expires within 0 days" scenario.
There was a problem hiding this comment.
I talked with Kuba before about this specific case and we probably want to INCLUDE an item that expires today (assuming you chose SUBTRACT_EXPIRED_STOCK, as it expires tomorrow, not today.
This is why we need to distinguish between removeExpiredStock, as the sign changes from >= to > or vice versa
awalkowiak
left a comment
awalkowiak
left a comment
There was a problem hiding this comment.
I am not sure about the final... naming, but since it aligns with the column name, I won't be nitpicky
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
4 participants
…ration filter
✨ Description of Change
Link to GitHub issue or Jira ticket:
Description:
📷 Screenshots & Recordings (optional)