OBPIH-6331 Inferring bin locations in pick import #4675
Conversation
…efault error messages
…uring local testing
…e virtual + actual bin locations
… final version of the branch
… default allocation algorithm would suffice
…cklist because it has been working as expected
| String lotNumberName = data.lotNumber ?: g.message(code: "default.noLotNumber.label", default: Constants.DEFAULT_LOT_NUMBER) | ||
| String binLocationName = data.binLocation ?: g.message(code: "default.noBinLocation.label", default: Constants.DEFAULT_BIN_LOCATION_NAME) |
There was a problem hiding this comment.
This was probably an issue that came after rebase but it looks like g is not defined in this scope so we need to add
ApplicationTagLib g = grailsApplication.mainContext.getBean(ApplicationTagLib.class)in the scope or move this same code from the previous if (!availableItem) in a higher block/scope (move it out of the if block)
There was a problem hiding this comment.
yeah this was my fault. I think I moved it into the wrong block. Fixed!
There was a problem hiding this comment.
I don't expect you to change this but I'd love for us to have a dto (or whatever) package that we can put our request objects in instead of sticking them alongside the controllers. Something for us to think about in the future
| } | ||
|
|
||
| // FIXME If location.name is not unique then we need to do some error handling | ||
| return locations.find { it.name.equalsIgnoreCase(name) } |
There was a problem hiding this comment.
if this is an internal location, you don't need to do an isInternalLocation() check?
| @@ -38,6 +39,10 @@ class ImportPickCommand implements Validateable { | |||
| }) | |||
| } | |||
|
|
|||
| boolean getIsDefaultBinLocation() { | |||
| return !binLocation || binLocation?.equalsIgnoreCase(Constants.DEFAULT_BIN_LOCATION_NAME) | |||
There was a problem hiding this comment.
You don't need the ? if you're doing the null check first.
| return !binLocation || binLocation?.equalsIgnoreCase(Constants.DEFAULT_BIN_LOCATION_NAME) | |
| return !binLocation || binLocation.equalsIgnoreCase(Constants.DEFAULT_BIN_LOCATION_NAME) |
|
|
||
| stockMovementService.validatePicklistListImport(stockMovement, pickPageItems, importedLines) |
There was a problem hiding this comment.
This might be a larger discussion but I'll note it here.
What's the advantage of using these command objects vs just passing the objects that a method needs like we had before? What problem is it solving for us?
We used something similar at my previous job (we called them Context objects) but were encouraged to only use them in really complex flows that need to maintain a certain state across classes.
I get that it simplifies the args down to a single object, but if we're not careful I can see a world where we're stuck with dozens of command objects that only do one specific thing.
| void validatePicklistListImport(StockMovement stockMovement, List<PickPageItem> pickPageItems, List<ImportPickCommand> picklistItems) { | ||
| Map<String, List> picklistItemsGroupedByRequisitionItem = picklistItems.groupBy { it.id } | ||
| /** | ||
| * This method needs to be broken down into multiple smaller methods. |
There was a problem hiding this comment.
I think you have a comment in the controller class that is alluding to this, but you could have a PicklistImportValidator (that extends some generic ImportValidator) that wraps all this logic and returns a PicklistImportValidationResult object (that extends some generic ValidationResult).
This is a really complex validation step so I don't know exactly what that would look like, but you could break it down further into smaller validators if you really wanted to and just bubble up the ValidationResult.
PicklistImportValidationResult result = picklistImportValidator.validate()
if (!result.isValid()) {
result.errors()....
}
And in the PicklistImportValidator:
PicklistImportValidationResult validate() {
PicklistImportValidationResult result = new()
...
ValidationResult xResult = someMoreSpecificValidator.validate()
result.addAll(xResult.errors())
}
Again, this is super complex so I don't know how well that would work, but as is, this feels like it really needs to be broken down somehow. The method is pretty unwieldy.
… for method PickPageItem#getAvailableItem)
…s StockMovementService)
…here's enough quantity for any single available item rather than the sum of all available items
…n when only other available item is in a hold location
… available item in a receiving location
| return InventoryItem.createCriteria().get { | ||
| and { | ||
| eq("product", this) | ||
| if (lotNumber) { | ||
| eq("lotNumber", lotNumber) | ||
| } else { | ||
| or { | ||
| isNull("lotNumber") | ||
| eq("lotNumber", "") | ||
| } | ||
| } | ||
| } | ||
| } as InventoryItem | ||
| } |
There was a problem hiding this comment.
This is an issue I mentioned on the tech-huddle channel but I guess it will be best if I left a comment here under the PR to have some code reference
There are some cases that this chunck on code will result in an Exception.
createCriteria().get will throw an exception when it encounters more than one result found by given criteria, which is what I have encountered.
running the following query will return a list of products that will cause this issue.
select ii.product_id, count(ii.id) from inventory_item ii
where ii.lot_number is NULL or ii.lot_number = ''
group by ii.product_id
HAVING count(ii.id) > 1
ORDER BY count(*) desc;basically after inspecting these inventory items I have noticed that there are some items that have lotNumber: NULL and some that lotNumber: ""
InventoryItem has a following GORM validation set on lotNumber
lotNumber(nullable: true, unique: ['product'], maxSize: 255)
so there should be unique lotNumber per product.
As for DB constraints I don't see any fro inventory_item table lot_number column
| PicklistItem picklistItem = picklist.picklistItems.find { | ||
| it.inventoryItem?.id == availableItem.inventoryItem?.id && | ||
| it.binLocation?.id == availableItem.binLocation?.id | ||
| } |
There was a problem hiding this comment.
This picklistItem lookup might not work as you expect it to.
I encountered simillar case for picklist item update.
The issue is that before this lookup we call clearPicklist(pickPageItem.requisitionItem)
which will delete PicklistItems so in the ned we are always looking for Picklistitems that do not exist anymore.
So we either can remove this lookup line and ignore "updating picklist Items" or rethink the clearPicklist.
Currently we don't have a case anywhere where we update existing picklistItem (at least I don't think so).
The way we handle update for picklistItems is we remove them and recreate as a new one.
Rebased version.