[OBPIH-6679] inferring bin in packing list import based on lot number#4840
Conversation
| } | ||
|
|
||
| Product product = Product.findByProductCode(source['product']) | ||
| List<ProductAvailability> items = ProductAvailability.findAllByProductAndLotNumber(product, source['lotNumber']) |
There was a problem hiding this comment.
shouldn't we include the origin location in this query?
| return new Location(name: source['binLocation']) | ||
| } | ||
|
|
||
| Product product = Product.findByProductCode(source['product']) |
There was a problem hiding this comment.
can't we use obj.product at this point? If it is null at this point, we would just have to change the order of the payload that is sent to the API,, so that the product appears before the binLocation.
There was a problem hiding this comment.
yes it is null at this point.
I would avoid resorting to this type of a solution, feels a little hacky that a client has to send a payload in a specific order
…location inferring feature
| return new Location(name: source['binLocation']) | ||
| } | ||
|
|
||
| Product product = Product.findByProductCode(source['product']) |
There was a problem hiding this comment.
I'm definitely not asking you to change anything. Just noting for myself.
I'm not a huge fan of doing all this logic in the binding phase. In my opinion, @BindUsing should only be used for quite simple operations. I generally prefer to avoid manipulating user input whenever possible, even if it's just adding another field to the object. Moving this kind of thing to the service layer (or a helper class that is called by the service) helps keep domains and request objects simple, and makes it more clear and deliberate when we're doing database lookups.
Anyways, not something to think about now.
There was a problem hiding this comment.
@ewaterman do you have any specific alternative for that? I think we should follow one pattern and if we decide to use a command object to bind the data for us, we should do everything in order for all properties to be bound within the automatic binding process, especially that the commands are validated while being bound, so having this type of code:
def validate(ImportPackingListCommand command) {
<at this point command is already validated and might contain errors>
}
makes the command already validated.
If we were to mix up automatic binding (that would validate the command via validator) and some service binding, there would be a mess around that object, because:
- on one hand, the object would already contain an error that e.g. binLocation is null.
- but on the other hand, it's fine for us, because we would love to bind it in a service method
- that would then require clearing the error on that field and validate that field again after the service binding
I think this already sounds complicated, hence I am not a fan of mixing a few approaches for one object.
What sucks is maybe that Grails immediately validates the bound command object and this complicates the thing for us, but on the other hand, the logic that you can see in this BindUsing either has to be here or would be moved to a service, which wouldn't decrease the complexity of the code at all.
To make it more readable though, I agree that this logic could be moved at least to a method to make the command class at least look simple.
Summing up I am a fan of being able to validate and construct command at the binding time, as this makes the controller/service logic simpler, as all the validation logic is placed in a validator, but why I say that is that I might not be objective about this topic, but subjective, so don't treat my comment as some kind of "you are not right, this is how we should do it".
P.S. I have also researched that topic many times, but have not found anything that would be the "proof of concept", so I feel like we've just spliced a few Grails' functionalities and have tried to become the "proof of concept" for others in that binding/validate topic 😆
✨ Description of Change
Link to GitHub issue or Jira ticket: OBPIH-6679
Description:
The use case describes a scenario where user did not provide a bin location in the packing list document but provided a lot number, so based on the provided lot nuymber we want to infer the bin location only in cases where there is a single possible bin location for provided lot number
📷 Screenshots & Recordings (optional)