Skip to content

[OBPIH-6679] inferring bin in packing list import based on lot number#4840

Merged
awalkowiak merged 2 commits intorelease/0.9.2-hotfix1from
bugfix/OBPIH-6679-inferring-bin-in-packing-list-import-for-full-outboub
Sep 16, 2024
Merged

[OBPIH-6679] inferring bin in packing list import based on lot number#4840
awalkowiak merged 2 commits intorelease/0.9.2-hotfix1from
bugfix/OBPIH-6679-inferring-bin-in-packing-list-import-for-full-outboub

Conversation

@drodzewicz
Copy link
Collaborator

@drodzewicz drodzewicz commented Sep 12, 2024

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

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)

If this PR contains a UI change, consider adding one or more screenshots here or link to a screen recording to help reviewers visualize the change. Otherwise, you can remove this section.

@drodzewicz drodzewicz self-assigned this Sep 12, 2024
@github-actions github-actions bot added type: bug Addresses unintended behaviours of the app domain: backend Changes or discussions relating to the backend server labels Sep 12, 2024
}

Product product = Product.findByProductCode(source['product'])
List<ProductAvailability> items = ProductAvailability.findAllByProductAndLotNumber(product, source['lotNumber'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we include the origin location in this query?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

return new Location(name: source['binLocation'])
}

Product product = Product.findByProductCode(source['product'])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return new Location(name: source['binLocation'])
}

Product product = Product.findByProductCode(source['product'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😆

@awalkowiak awalkowiak merged commit f564baf into release/0.9.2-hotfix1 Sep 16, 2024
@awalkowiak awalkowiak deleted the bugfix/OBPIH-6679-inferring-bin-in-packing-list-import-for-full-outboub branch September 16, 2024 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: backend Changes or discussions relating to the backend server type: bug Addresses unintended behaviours of the app

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants