OBPIH-6365 ability to create edit a pick via import with incorrect stock cases#4650
Conversation
drodzewicz
requested review from
alannadolny,
awalkowiak,
ewaterman,
jmiranda and
kchelstowski
drodzewicz
force-pushed
the
OBPIH-6365-ability-to-create-edit-a-pick-via-import-incorrect-stock-cases
branch
from
3b6cc6b to
08649b9
Compare
| picklistsToImport.each { params -> | ||
| AvailableItem availableItem = pickPageItem.getAvailableItem(params.binLocation, params.lotNumber) | ||
| Picklist picklist = stockMovement.requisition?.picklist | ||
| // find existing picklist to update | ||
| PicklistItem picklistItem = picklist.picklistItems.find { | ||
| it.inventoryItem?.id == availableItem.inventoryItem?.id && | ||
| it.binLocation?.id == availableItem.binLocation?.id | ||
| } | ||
| createOrUpdatePicklistItem( | ||
| pickPageItem.requisitionItem, | ||
| picklistItem, | ||
| availableItem.inventoryItem, | ||
| availableItem.binLocation, | ||
| params.quantity, | ||
| null, | ||
| null | ||
| ) | ||
| } | ||
| pickPageItem.picklistItems.add(new PicklistItem( | ||
| requisitionItem: requisitionItem, | ||
| inventoryItem: availableItem.inventoryItem, | ||
| binLocation: availableItem.binLocation, | ||
| quantity: params.quantity, | ||
| sortOrder: pickPageItem.sortOrder | ||
| )) | ||
| createMissingShipmentItem(pickPageItem.requisitionItem) |
There was a problem hiding this comment.
changes implemented here meant to follow the example of updating picklist like on
StockMovementItemApiController > updatePicklist where we update individual requisition item.
One problem that I have noticed is that previous implementation of importing picklist would not update the shipmentItems where we need to additionally do this: createMissingShipmentItem(requisitionitem)
There was a problem hiding this comment.
Oh shit, good catch. That's very important.
There was a problem hiding this comment.
We probably need to document this somewhere or centralize it i.e. any time we muck with the picklist it automatically updates the shipment.
One problem that I have noticed is that previous implementation of importing picklist would not update the shipmentItems where we need to additionally do this: createMissingShipmentItem(requisitionitem)
Additionally, are you saying this is a current bug in production? If you're not sure we should definitely add this as a discussion point to the ticket with the advise that we probably want to investigate this as an OBS data issue i.e. comparing picklist items to shipment items to ensure they include the same quantity. If not, this is going to be a pain to clean up.
There was a problem hiding this comment.
Most likely yes, since my previous implementation for the pick import was based on the old import I would assume that the same issue occurred before.
grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy
Outdated
Show resolved
Hide resolved
drodzewicz
force-pushed
the
OBPIH-6365-ability-to-create-edit-a-pick-via-import-incorrect-stock-cases
branch
from
08649b9 to
f211034
Compare
jmiranda
left a comment
jmiranda
left a comment
There was a problem hiding this comment.
You don't need to incorporate the requested changes before merging, but I would like to explore the question about validating quantity as a discussion in the ticket. I will make the other requested changes in my OBPIH-6331 PR later today.
| void validatePicklistListImport(List<PickPageItem> pickPageItems, Boolean supportsOverPick, List<ImportPickCommand> picklistItems) { | ||
| Map<String, List> groupedItems = picklistItems.groupBy { it.id } | ||
| void validatePicklistListImport(StockMovement stockMovement, List<PickPageItem> pickPageItems, List<ImportPickCommand> picklistItems) { | ||
| Map<String, List> picklistItemsGroupedByRequisitionItem = picklistItems.groupBy { it.id } |
There was a problem hiding this comment.
Thank you! This is way more clear.
| expirationDate: it.expirationDate ? new Date(it.expirationDate) : null, | ||
| binLocation: it.binLocation, | ||
| quantity: Integer.parseInt(it.quantity) | ||
| quantity: it.quantity.isNumber() ? Integer.parseInt(it.quantity) : null |
There was a problem hiding this comment.
Do we want to return a validation error if it's not a number? Or is it ok to handle the quantity as null? I think the distinction is that if null eventually becomes 0 and 0 is a valid quantity, then it might be confusing to the user who entered accidentially entered "100 ea" to have no picklist items for that line.
I'm looking at this in a vacuum so I don't have a good sense of the expected behavior or next steps but I would look downstream to see what behavior we've implemented (understanding that the behavior might change in the future) and make sure that we handle it as explicitly as possible, rather than coercsing input to something the user may not have intended.
| String lotNumberErrorMessage = data.lotNumber ?: "empty" | ||
| String binLocationErrorMessage = data.binLocation ?: "empty" | ||
|
|
||
| String lotNumberName = data.lotNumber ?: g.message(code: "default.label", default: "Default") |
There was a problem hiding this comment.
I used Constants.DEFAULT_LOT_NUMBER and Constants.DEFAULT_BIN_LOCATION, but this is much better.
However, let's do this to be consistent.
String lotNumberName = data.lotNumber ?: g.message(code: "default.noLotNumber.label", default: Constants.DEFAULT_LOT_NUMBER)
There was a problem hiding this comment.
String binLocationName = data.binLocation ?: g.message(code: "default.noBinLocation.label", default: Constants.DEFAULT_BIN_LOCATION)
| picklistsToImport.each { params -> | ||
| AvailableItem availableItem = pickPageItem.getAvailableItem(params.binLocation, params.lotNumber) | ||
| Picklist picklist = stockMovement.requisition?.picklist | ||
| // find existing picklist to update | ||
| PicklistItem picklistItem = picklist.picklistItems.find { | ||
| it.inventoryItem?.id == availableItem.inventoryItem?.id && | ||
| it.binLocation?.id == availableItem.binLocation?.id | ||
| } | ||
| createOrUpdatePicklistItem( | ||
| pickPageItem.requisitionItem, | ||
| picklistItem, | ||
| availableItem.inventoryItem, | ||
| availableItem.binLocation, | ||
| params.quantity, | ||
| null, | ||
| null | ||
| ) | ||
| } | ||
| pickPageItem.picklistItems.add(new PicklistItem( | ||
| requisitionItem: requisitionItem, | ||
| inventoryItem: availableItem.inventoryItem, | ||
| binLocation: availableItem.binLocation, | ||
| quantity: params.quantity, | ||
| sortOrder: pickPageItem.sortOrder | ||
| )) | ||
| createMissingShipmentItem(pickPageItem.requisitionItem) |
There was a problem hiding this comment.
Oh shit, good catch. That's very important.
| enum AvailableItemStatus { | ||
| AVAILABLE, PICKED, RECALLED, HOLD, NOT_AVAILABLE | ||
|
|
||
| static getList() { |
There was a problem hiding this comment.
Follow the existing convention for listing enum values and rename the second method to be more readable.
static list() { ... }
static listUnavailable() { ... }
awalkowiak
deleted the
OBPIH-6365-ability-to-create-edit-a-pick-via-import-incorrect-stock-cases
branch
3 participants
No description provided.