OBPIH-6747 support empty lot number in import that matches inventory item defined as null#4866
Conversation
…e logic into standalone methods
…ulfillmentService
drodzewicz
added
warn: do not merge
github-actions
bot
added
type: feature
drodzewicz
requested review from
alannadolny,
awalkowiak,
ewaterman and
kchelstowski
| return debitTransaction | ||
| } | ||
|
|
||
| String bindOrInferLotNumber(ImportPackingListItem obj, Map source) { |
There was a problem hiding this comment.
I can see that the only property from obj we use is the origin. Let's change the arguments to be Location origin, Map source, since we do not need anything else from the obj.
There was a problem hiding this comment.
let's leave it as they are since the methods are basically the @UseBinding methods but moved to the service.
The other binding method requires this obj since we set obj.binLcoationNotFound flag on it and it would be weird where one has this argument and the other binding method doens't
grails-app/services/org/pih/warehouse/fulfillment/FulfillmentService.groovy
Outdated
Show resolved
Hide resolved
grails-app/services/org/pih/warehouse/fulfillment/FulfillmentService.groovy
Outdated
Show resolved
Hide resolved
| return availableBinLocations | ||
| } | ||
|
|
||
| AvailableItem getAvailableItemWithDefaultLots(Location origin, String productCode) { |
There was a problem hiding this comment.
I think the name of this method doesn't fit to what the method is actually doing. We should either think about the name again, or at least add a comment above the method.
My point is that if you find 3 items, in fact there is 3 available items found, but the method would return null, which is a bit misleading.
| return null | ||
| } | ||
|
|
||
| AvailableItem inferAvailableItemByBinLocation(Location origin, String productCode, String binLocationName) { |
There was a problem hiding this comment.
I think get fits more than infer - we should change it to get here or use infer in all methods.
grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy
Outdated
Show resolved
Hide resolved
| return new Location(name: source['binLocation']) | ||
| @BindUsing({ obj, source -> | ||
| FulfillmentService fulfillmentService = Holders.grailsApplication.mainContext.getBean(FulfillmentService) | ||
| return fulfillmentService.bindOrInferBinLocation(obj as ImportPackingListItem, source as Map) |
There was a problem hiding this comment.
instead of casting it here, I think you could assign the type in the obj, source, like:
@BindUsing({ ImportPackingListItem obj, Map source -> })There was a problem hiding this comment.
I am not sure why but for some reason this doesn't work.
When doing this it can't find the method which is why I resorted to the approach of casting the value
drodzewicz
removed
the
warn: do not merge
awalkowiak
deleted the
feature/OBPIH-6747-support-empty-lot-number-in-import-that-matches-inventory-item-defined-as-null
branch
…item defined as null (openboxes#4866)
…item defined as null (openboxes#4866)
3 participants
✨ Description of Change
Link to GitHub issue or Jira ticket: OBPIH-6747
Description:
The inferring bin Location and lot number required a bit of a refactor, I decided to move many parts into standalone service methods for better maintainability and readability because this feature has grown a bit.
Additionally since there are many different inferring/binding scenarios I am working on implementing tests for these methods
📷 Screenshots & Recordings (optional)