OBPIH-6042 Add filter by date range to expiration reports#4487
OBPIH-6042 Add filter by date range to expiration reports#4487awalkowiak merged 6 commits intofeature/upgrade-to-grails-3.3.10from
Conversation
alannadolny
requested review from
awalkowiak,
drodzewicz,
jmiranda and
kchelstowski
alannadolny
force-pushed
the
OBPIH-6042
branch
from
11849d5 to
316cd86
Compare
|
@alannadolny there are some conflicts here |
| List<InventoryItem> inventoryItems = dashboardService.getExpiredStock(command.category, command.location, command.startDate, command.endDate) | ||
| List<Category> categories = inventoryItems?.collect { it.product.category }?.unique() | ||
| Map<InventoryItem, Integer> quantityMap = productAvailabilityService.getQuantityOnHandByInventoryItem(command.location) | ||
| LinkedHashMap<InventoryItem, Integer> expiredStockMap = [:] |
There was a problem hiding this comment.
LinkedHashMap is an implementation of Map interface.
We should use interfaces as types if possible, hence Map should be the type here.
The same case as with Lists - you shouldn't define the type as ArrayList<String> list = [...], but as List<String> list = [...]
|
|
||
| if (endDate) { | ||
| expiredStock = expiredStock.findAll { item -> item?.expirationDate <= endDate } | ||
| } |
There was a problem hiding this comment.
this feels odd to me - this should be probably filtered out at the query level, but I can see that you've followed the convention, so I don't have an opinion if it should be handled now or if we should treat it as a tech debt.
| * @return a list of inventory items | ||
| */ | ||
| List getExpiringStock(Category category, Location location, String expirationStatus) { | ||
| List getExpiringStock(Category category, Location location, String expirationStatus, Date startDate, Date endDate) { |
There was a problem hiding this comment.
just a comment - 4 arguments feels a lot to me, I think in the near future, we should rely on command classes more, since they give us more flexibility and better dev experience in my opinion.
| if (endDate) { | ||
| expiringStock = expiringStock.findAll { item -> item?.expirationDate <= endDate } | ||
| } | ||
|
|
There was a problem hiding this comment.
same comment as above
alannadolny
force-pushed
the
OBPIH-6042
branch
from
316cd86 to
55ff995
Compare
| } | ||
|
|
||
| if (command.endDate) { | ||
| expiringStock = expiringStock.findAll { item -> item?.expirationDate <= command.endDate } |
There was a problem hiding this comment.
I'm approving this one, but this needs to be refactored asap, especially if the performance is bad there.
| expiredStock = expiredStock.findAll { item -> item?.product?.category == command.category } | ||
| } | ||
|
|
||
| if (command.startDate) { |
There was a problem hiding this comment.
I think this should not work like that. IMHO this should be properly used in the findAll or in criteria as a proper date range
findAllByExpirationDateGreaterOrEqualThanAndByExpirationDateLessOrEqualThan, but this (if works) is undreadable and should include category, so I think this needs to end up in the proper criteria including all filtering properties: category, startDate and endDate. And then these results can be filtered by quantity on hand.
| def today = new Date() | ||
| today.clearTime() | ||
| // Get all stock expiring ever (we'll filter later) | ||
| def expiringStock = InventoryItem.findAllByExpirationDateGreaterThanEquals(today + 1, [sort: 'expirationDate', order: 'asc']) |
3 participants
No description provided.