OBPIH-6331 Inferring bin locations in pick import #4658
Conversation
|
Although it's not done, y'all can start to review this one. |
jmiranda
requested review from
drodzewicz,
EWaterman,
awalkowiak,
alannadolny and
kchelstowski
grails-app/controllers/org/pih/warehouse/allocation/AllocationRequest.groovy
Outdated
Show resolved
Hide resolved
grails-app/controllers/org/pih/warehouse/picklist/PicklistController.groovy
Outdated
Show resolved
Hide resolved
| Location getDefaultLocation() { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Is there something missing? I can't see any comment here
There was a problem hiding this comment.
Yeah, this is awkward at best. I think this was just a quick hack to get it working and I was expecting to get back to it. I am trying to provide a representation of a default location. But the default location is just a Location that is null.
I would love to do this in a better way and open to ideas. A javadoc comment is necessary. And maybe this needs to be a static method, returning a constant?
static Location getDefaultLocation() {
return Constants.DEFAULT_LOCATION
}
or something like this?
final static Location DEFAULT_LOCATION = null;
static Location getDefaultLocation() {
return Location.DEFAULT_LOCATION;
}
There was a problem hiding this comment.
The first static method returning a constant looks great
| @@ -323,6 +324,10 @@ class AvailableItem { | |||
| quantityAvailable(nullable: true) | |||
| } | |||
|
|
|||
| String getBinLocationName() { | |||
| binLocation?.name?:Constants.DEFAULT_BIN_LOCATION_NAME | |||
There was a problem hiding this comment.
| binLocation?.name?:Constants.DEFAULT_BIN_LOCATION_NAME | |
| binLocation?.name ?: Constants.DEFAULT_BIN_LOCATION_NAME |
…uring local testing
jmiranda
added
the
status: ready for review
| String binLocationName = data.binLocation ?: g.message(code: "default.noBinLocation.label", default: Constants.DEFAULT_BIN_LOCATION_NAME) | ||
| // TODO Do we handle the case where ID is null? | ||
| // Group the picklist items from the CSV by requisition item ID | ||
| Map<String, List> picklistItemsGroupedByRequisitionItem = command.picklistItems.groupBy { it.id } |
There was a problem hiding this comment.
Is the id on the PicklistItemCommand a requisition item's id?
| picklistItemsGroupedByRequisitionItem.each { requisitionItemId, picklistItems -> | ||
|
|
||
| // Iterate over each separate pick | ||
| picklistItems.each { PicklistItemCommand data -> |
There was a problem hiding this comment.
Does PicklistItemCommand represent already picked item, item to be picked or both cases?
| // TODO What happens if ID is null. We should throw an exception if we don't have a good answer. | ||
| // skip validation if id is empty since most of the validation relies on an existing requisition id | ||
| // skip validation if id is empty since mos tof the validation relies on an existing requisition id | ||
| if (!data.id) { |
There was a problem hiding this comment.
looks like it cannot be empty because id on PicklistItemCommand has constraints: nullable: false, blank: false
| // FIXME this probably shouldn't be necessary, but i was getting | ||
| // a validation error on location (possibly when the command object | ||
| // was initially bound). | ||
| location = AuthService.currentLocation |
There was a problem hiding this comment.
I think it should be a StockMovement's origin location instead current location. What do you think?
| // OBPIH-6331 Resolve bin location, if a bin location value was provided. Otherwise, | ||
| // if the user-provided bin location is null or ambiguous then we need to apply | ||
| // allocation rules | ||
| Location internalLocation = command.location.getInternalLocation(data.binLocation) |
There was a problem hiding this comment.
Is it technically possible (now or in the future, through API) that someone will be able to import an outbound not from the "current location"? This is why I am wondering if we should not start using origin instead of current location as a command.location 🤔
| boolean isInDefaultLocation = availableItems.every { it.isDefaultLocation } | ||
| boolean isInReceivingLocations = availableItems.every { it.isReceivingLocation } | ||
| boolean isInBinLocations = availableItems.every { it.isBinLocation && !it.isDefaultLocation } |
There was a problem hiding this comment.
Are these three variables used anywhere
| Integer countInBinLocations2 = availableItems?.findAll { it?.isBinLocation }?.size() | ||
| log.info "countInBinLocations " + countInBinLocations2 | ||
| Integer countInVirtualLocations = availableItems?.findAll { it?.isVirtualLocation }?.size() | ||
| log.info "countInVirtualLocations " + countInVirtualLocations |
There was a problem hiding this comment.
Are these required too? Or the scenario above is still to be finished?
| // Scenario 4: Any other scenario (stock in “real” bin and virtual bin, stock in multiple real bins) | ||
| // TODO If there's not enough available quantity in default + receiving | ||
| // locations, then throw an exception | ||
| boolean isAllStockInReceivingOrDefault = availableItems.every { it.isReceivingLocation || it.isDefaultLocation } |
There was a problem hiding this comment.
Is there an order of precedence for this scenario which is picked first? Plus for this scenario shouldn't we also look at stock in “real” bin looks like you only look at receiving or default
| // TODO Need to test this thoroughly as it does not feel like a good idea. For example, | ||
| // we don't necessarily want to clear picklist if there's no picklist items to import | ||
| // so this might need to go inside a check on !picklistItemsToImport.empty | ||
| clearPicklist(pickPageItem.requisitionItem) |
There was a problem hiding this comment.
Yeah I think this might cause unnecessary product availability refresh if there are no existing picklist items at this stage
| "binLocation", | ||
| "importPickCommand.binLocation.multipleEmpty.error", | ||
| [data.lotNumber] as Object[], | ||
| "Bin location cannot be empty for multiple rows with the same lot number {0}" |
There was a problem hiding this comment.
I never knew we can treat default message just like the label 🙈
| // Bind user-provided location or current location | ||
| command.location = command.location ?: Location.get(session.warehouse.id) |
There was a problem hiding this comment.
(Just thinking out loud)
I wonder if in case of importing, we will ever need current logged in location 🤔
For example, case with supported activity Allow Overpick I belive we should check this activity not on current logged in location but on origin because the origin location should decide if it supports this activity.
This was just one example but I guess any future validation will involve either the origin or destination locations, the current logged in location will probably be a single case that will not allow importing items if current location != origin.
Do I have a point or am I going off the track?
There was a problem hiding this comment.
Yup, I agree with you. I also think we should base it on the origin in this case. I brought this up in the PicklistImportDataCommand too.
|
|
||
| // FIXME Remove this if it's no longer being used | ||
| // Configuration property that allows overpick for a given location | ||
| Boolean supportsOverpick = command.location.supports(ActivityCode.ALLOW_OVERPICK) |
There was a problem hiding this comment.
like I mentioned in the comment before, this should probably be checked for origin location and not current logged-in.
|
Closing this one because this is stale branch, and the feature itself was already merged |
No description provided.