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.
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.
Rebased version.