-
Notifications
You must be signed in to change notification settings - Fork 93
LF-4980: Show valid soil amendment products #3899
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
LF-4980: Show valid soil amendment products #3899
Conversation
eb88d82 to
8568d8f
Compare
b2c4d07 to
b46644c
Compare
filter products in PureSoilAmendmentProductForm
ef7a84e to
e4c8c34
Compare
e4c8c34 to
d63722f
Compare
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.
Hey Sayaka, everything is looking really good!
I think there is just one small bug about the initialProductId. The removed product is no longer available in the options when going forward and back, and when changing mind and clicking 'No changes made` the data does not reset (does not affect db). Here is a video:
Screen.Recording.2025-10-22.at.9.53.35.AM.mov
Other not important things:
Currently unused libraryProductsOutsideInventory wondering where it is going to be used.
Selectors - Maybe I have selector vision after our chat the other day but I was wondering if many of the functions that distill products should be selectors also or just put in the util file for reuse?
Happy to approve if any of these should be addressed elsewhere
| }, [mode]); | ||
|
|
||
| const productNames: SoilAmendmentProduct['name'][] = products.map(({ name }) => name); | ||
| const { customProductsInInventory } = useMemo(() => { |
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.
libraryProductsOutsideInventory is it defined somewhere? I imagine that a user could duplicate a library product multiple times to tweak values. Is this in preparation for showing a library without the already duplicated library products?
Also is there a chance this will be reused and should this be a createSelector?
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 don't think users would be able to duplicate a library product directly. They would first add it to the inventory, then duplicate it.
I expect libraryProductsOutsideInventory to be used as the options to add to the inventory:
I can’t think of a use case where we’d want to reuse it right now... can you?
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.
Maybe I am wrong but I think what I expect is that we always show allLibraryProducts instead of removing already added libraryProductsOutsideInventory.
How I understood library products was that the user can checkout their own copy and make edits to it as they choose? Adding a library product multiple times makes sense if the user is allowed to edit the values of the local copy. Maybe I misunderstood how library products work. At least currently, a library product is editable and so if I wanted the original unedited values I could not find it in libraryProductsOutsideInventory if it is removed.
Reuse: Maybe I was wishful thinking but I thought that this function looked generic enough for pest control and cleaning products too. Hopefully we don't have multiple product architecture for a long time!
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.
At least currently, a library product is editable
We haven’t handled library products yet, but eventually we’ll need to prevent them from being modified!
I removed libraryProductsOutsideInventory for now since it's hard to envision without the actual UI/UX. I could have created a function to generate customProductNames, but I'm leaving that for later too!
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.
Oh we might need to align on that! I thought for sure the library products ARE modifiable .. as a copy of the core library product. Anyways thanks for removing for now as we discuss!
packages/webapp/src/components/Task/AddSoilAmendmentProducts/ProductCard/index.tsx
Show resolved
Hide resolved
packages/webapp/src/components/Task/AddSoilAmendmentProducts/ProductCard/index.tsx
Outdated
Show resolved
Hide resolved
| const productId = getValues(PRODUCT_ID); | ||
| const purposes = watch(PURPOSES); | ||
|
|
||
| const initialProductId = useRef(productId); |
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 show it in the video but this changes on render.
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.
Thank you so much @Duncan-Brain for reviewing!
The removed product is no longer available in the options when going forward and back
True...😭 I'll try to come back with a better solution.
when changing mind and clicking 'No changes made` the data does not reset
I think this happens for all task types; I'll check with Denis if there's a ticket!
I believe I've covered all your comments, but please let me know if I missed anything!
| }, [mode]); | ||
|
|
||
| const productNames: SoilAmendmentProduct['name'][] = products.map(({ name }) => name); | ||
| const { customProductsInInventory } = useMemo(() => { |
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 don't think users would be able to duplicate a library product directly. They would first add it to the inventory, then duplicate it.
I expect libraryProductsOutsideInventory to be used as the options to add to the inventory:
I can’t think of a use case where we’d want to reuse it right now... can you?
packages/webapp/src/components/Task/AddSoilAmendmentProducts/ProductCard/index.tsx
Show resolved
Hide resolved
packages/webapp/src/components/Task/AddSoilAmendmentProducts/ProductCard/index.tsx
Outdated
Show resolved
Hide resolved
update productOptions generation in ProductCard
I resolved this by filtering unused products in an upper component. (abc5e21) Other changes:
Thank you for re-reviewing! 🙏 |
Are there downsides? it seems preferred approach to me! Looking great and thanks for removing that small library piece as we discuss. |
Description
isLibraryProductfunction andproductInventorySelectorproductInventorySelectorTaskDetails)TaskReadOnly) and (re-)completion (StepOne)productsForTaskTypeSelectorfrom a selector factory to a global selector (as discussed in the tech daily on Oct 20, 2025)useEffect)Note:
I initially created a selector factory for products so that the inventory table and product form could maintain separate caches. After the tech daily discussion, I created
productInventorySelectorfor the inventory table and reused the existing selector in the product form. I like this approach much better. Thank you for your input! @Duncan-Brain @kathyaviniJira link: https://lite-farm.atlassian.net/browse/LF-4980
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)