Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OBPIH-5739 Fix filtering for handling requirements in products list page #4931

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

kchelstowski
Copy link
Collaborator

✨ 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:

Description:


📷 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.

@kchelstowski kchelstowski self-assigned this Nov 8, 2024
@github-actions github-actions bot added the domain: backend Changes or discussions relating to the backend server label Nov 8, 2024
Comment on lines +365 to +377
or {
if (handlingRequirements.contains(ProductField.COLD_CHAIN)) {
eq("coldChain", true)
}
if (handlingRequirements.contains(ProductField.CONTROLLED_SUBSTANCE)) {
eq("controlledSubstance", true)
}
if (handlingRequirements.contains(ProductField.HAZARDOUS_MATERIAL)) {
eq("hazardousMaterial", true)
}
if (handlingRequirements.contains(ProductField.RECONDITIONED)) {
eq("reconditioned", true)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about doing it more generic? I mean that now when we are adding a new handling requirement we have to add it to the product domain, to the ProductField enum, and to this place to make the filtering work. So maybe we can add a function for listing all of the handling requirements - something similar to what we have in the RequisitionStatus enum (listOubtoundOptions, listOutboundOptionsWhenApprovalRequired, listRequestOptionsWhenApprovalRequired, etc.). Then we can do a loop over this list and if the current requirement from the loop is in handling requirement we can do eq(elemenetFromLoop, true), so doing it this way we don't have to handle adding new requirements in this place. (I believe we can do a loop here)

cc @awalkowiak

Copy link
Collaborator

Choose a reason for hiding this comment

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

(my thought - you have valid point, but imho this is fine for now, because I think there are no plans to add new handling requirements 🤔)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had the same feeling, initially I considered doing this with a generic method but then I realized that we wouldn’t achieve much by that, since those are only 4 lines of the code with a low risk to expand

@awalkowiak awalkowiak merged commit 4613003 into develop Nov 8, 2024
6 of 7 checks passed
@awalkowiak awalkowiak deleted the OBPIH-5739-fix branch November 8, 2024 13:34
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants