OBPIH-6679 Fix inferring bin location when providing a lot number#4848
Conversation
…ins and lots that are in stock
github-actions
bot
added
the
domain: backend
drodzewicz
changed the title
| Product product = Product.findByProductCode(source['product']) | ||
| List<ProductAvailability> items = ProductAvailability.findAllByProductAndBinLocationAndLocation(product, internalLocation, obj.origin) | ||
| List<ProductAvailability> items = productAvailabilityService.getAvailableBinLocations(obj.origin, product?.productCode) | ||
| .findAll { it?.binLocation?.name == source['binLocation'] } |
There was a problem hiding this comment.
since there is a lot of those methods in the product availability service, I won't force that, but I would prefer to have this filtering in the query itself, instead of calling .findAll just after doing the fetch.
Please take a look if there is a method that would return product availability for pair: origin, product, binLocation and use it if possible.
Otherwise, if the performance is OK for an item, that has stock in a lot of bins, I'm fine with that, not to implement another comprehensive method in that service.
There was a problem hiding this comment.
I don't think we have a one right now that does a search for specific bin locaitons
There was a problem hiding this comment.
yeah, we don't and I would need to implement a new service method for that.
I don't think we are dealing with too much data at this point and maybe a simple findAll search should be enough
There was a problem hiding this comment.
ok, I guess we can just monitor and live with it for now, but I wanted to make a point, that this usually should not be the way to go.
| if (items.size() > 0 && items.any { it.binLocation == null }) { | ||
| // null value assumes that this is a default bin location | ||
| return null | ||
| } |
There was a problem hiding this comment.
is this check needed, if you return null below anyway?
There was a problem hiding this comment.
yeah, you are right, I had a little different approach before and this was necessary there.
| binLocation(nullable: true, validator: { Location binLocation, ImportPackingListItem item -> | ||
| if (!item.binLocationFound) { | ||
| return ['binLocationNotFound', binLocation.name] | ||
| return ['binLocationNotFound', binLocation?.name] |
There was a problem hiding this comment.
I don't mind it, but I'm wondering if you had run into the nullpointer here? I think it's not possible, because the only possible option to execute this code is if you return the dummy location
There was a problem hiding this comment.
you are right, I had a NPE but I had used a different approach before where I returned a a null value with setting this flag as false which resulted in a NPE.
I can remove this nullsafe operator now
|
|
||
| Product product = Product.findByProductCode(source['product']) | ||
| List<ProductAvailability> items = productAvailabilityService.getAvailableBinLocations(obj.origin, product?.productCode) | ||
| .findAll { it?.inventoryItem?.lotNumber == source['lotNumber']} |
There was a problem hiding this comment.
a similar comment like above
awalkowiak
left a comment
awalkowiak
left a comment
There was a problem hiding this comment.
Beside the one unnecessary if with return null that Kacper already mentioned it LGTM.
…inding binLocation
awalkowiak
deleted the
OBPIH-6679-fix-inferring-bin-location-when-lotnumber-provided
branch
4 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: OBPIH-6679
Description: Had to modify the inferring bin location logic a bit due to following problem.
📷 Screenshots & Recordings (optional)