-
-
Notifications
You must be signed in to change notification settings - Fork 427
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
Conversation
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) | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 🤔)
There was a problem hiding this comment.
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
✨ Description of Change
Link to GitHub issue or Jira ticket:
Description:
📷 Screenshots & Recordings (optional)