LF-4966 implement delete product functionality #3896

kathyavini · 2025-10-09T15:57:58Z

Description

This adds product deletion functionality to the inventory. The useRemoveProduct hook was based on the the squeaky clean useSaveProduct, but with a need for a lot more state in this flow, I don't know if I can get it much cleaner... certainly no where near as tidy as onSaveProduct right now! @SayakaOno if you don't mind checking that I did not destroy your vision for <ProductForm /> here 🙏

We had talked briefly in tech daily on Wed Oct 8 about supplying a "Removing..." title to the drawer while the remove modal was open, but when I looked again at Figma I realized that the drawer is closed while the modal is open, so I went with that rather than adding a title.

This PR also moves the menu item for "Inventory" out of the isAdmin code block, restoring it to workers (sorry! 🙇) and hides the delete button conditionally based on isAdmin status.

Jira link: https://lite-farm.atlassian.net/browse/LF-4966

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Passes test case
UI components visually reviewed on desktop view
UI components visually reviewed on mobile view
Other (please explain)

Checklist:

I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
The precommit and linting ran successfully
I have added or updated language tags for text that's part of the UI
I have ordered translation keys alphabetically (optional: run pnpm i18n to help with this)
I have added the GNU General Public License to all new files

kathyavini · 2025-10-09T17:14:22Z

packages/webapp/src/store/api/apiSlice.ts

  'FarmAddon',
  'IrrigationPrescriptions',
  'IrrigationPrescriptionDetails',
-  'SoilAmendmentProduct',


This tag was added at the same time as the edit and add mutations, but since it was never provided (the GET products is of course a Saga), it was never used for anything.

I'm not sure if it was a placeholder, but for now I've removed it so it doesn't seem like something that can be meaningfully invalidated (it can't; since it's not provided!) Please let me know if I've missed something here!

I don't know why I did that, thank you for cleaning it up! On an unrelated note, I regret naming products “soil amendment products” everywhere 😞

kathyavini · 2025-10-09T17:20:19Z

packages/webapp/src/containers/ProductInventory/ProductForm/index.tsx

-    </Drawer>
+    <>
+      <Drawer
+        isOpen={


As always wrapping the return in a parent fragment has annoyingly made the whole return into a diff, but the actual changes within <Drawer /> are the two extra conditionals into isOpen, and the isAdmin argument into renderDrawerTitle

Duncan-Brain · 2025-10-09T22:22:02Z

I think it is looking great!
No code problems here per se, but I did find one bug for the delete functionality as a whole.

Heres a long video bug:
https://github.com/user-attachments/assets/9fe6f503-9b56-4da5-96d0-d038439e9e26

Essentially I create two inventory with the same name, the second deletion looks like it sticks, but not actually in the database. Purging state by logout doesn't help.

I think at its core we are using the deleted prop in db as both "archived" and deleted and I think most of the app uses deleted a specific way. Its definitely possible to continue trying to differentiate this way here on the frontend.. but maybe it would be easier to differentiate removed from deleted like animals and others.

kathyavini · 2025-10-09T22:47:40Z

@Duncan-Brain I love your video, it is so complete!! 😍

I got worried when you said bug, but I'm super relieved to see it's actually behaving 100% as designed! There is a flowchart here that would have been impossible to find on Jira, sorry (different ticket and collapsed by default), but you actually stepped through all the steps for a custom product in this one video:

Delete a custom product and create a new one with the same name --> new product table record created ✅
Try to delete while in active task --> prohibited ✅
Complete task and delete --> product table record is non-deleted but product_farm table removed is set to true ✅

I think the video didn't show the product_farm table with the removed boolean, but you actually kind of see it in the inventory view when the text goes to grey -- this is the styling for when a row has the removed property (I actually didn't set that up when I made this table... I think it's a holdover from removed animal styling 😂) Ultimately the removed products will be hidden from inventory, but that is covered in the adjust selectors ticket that Sayaka will be picking up next.

So it looks pretty good to me; I'm happy to see it behaving as we had planned!

Duncan-Brain

Oh my mistake! Thanks for the link to the flow chart. And so removing the deleted products from the react select options, that caused the task not to create will be on that other ticket then -- very good!

Cool approving but you wanted Sayakas approval here so you two can merge.

SayakaOno

Looks great!

As for the state, isRemoveModalOpen and isCannotRemoveModalOpen could be combined (eg. modalType?), but I think it's good as is!
I'm most interested in the selector decision I mentioned in the inline comment 🙂

SayakaOno · 2025-10-09T20:04:51Z

packages/webapp/src/store/api/apiSlice.ts

  'FarmAddon',
  'IrrigationPrescriptions',
  'IrrigationPrescriptionDetails',
-  'SoilAmendmentProduct',


I don't know why I did that, thank you for cleaning it up! On an unrelated note, I regret naming products “soil amendment products” everywhere 😞

SayakaOno · 2025-10-09T20:29:42Z

packages/webapp/src/containers/productSlice.js

  });
 };

+export const isProductUsedInPlannedTasksSelector = (product_id) => {


I don't think I've seen this kind of selector before, what made you go with a selector instead of a function or hook? (A selector wouldn’t have been my first choice, so I’m just curious to learn different approaches!)

Huh! That's interesting. I immediately thought of a selector here. It probably is because I just wrote this query on the backend, and (is this even reasonable??!) in my mind the Redux selector is the closest frontend approximation to a model query?

I do like getting the memoization automatically by putting it in a selector, but I'm definitely not married to it if you think a function would be better!

I've been paying more attention to selector memoization recently (since you asked about shallowEqual 😂), so I'm still learning, and I'll borrow ChatGPT's explanation...

if we call useSelector(isProductUsedInPlannedTasksSelector(product_id)) directly in the render, a new selector is created every time, so createSelector won’t actually memoize across renders. It works perfectly if the selector instance is stable (e.g., with useMemo).

So, to benefit from the selector’s memoization, we’d need useMemo; without it, I think it works just like a plain function! (but we've been doing it, and I'm fine with the selector without useMemo :))

I'd be interested too if you have some articles that helped you understand!

I found a thread where a Redux and Reselect maintainer explained this, and the official Redux doc was also very helpful. I think these will be great guidance to follow going forward!

Ahhhh I'm not sure I find that maintainers' comments very easy to understand 😓 As as the original poster said, the selector factory pattern is right from those very docs you linked so it's a bit rough that they followed the docs and got a "you're using createSelector wrong by creating a factory function"!

Well if it's truly an anti-pattern, it's kind of horrifying because we have so many selector factories in app. I think I'm still fuzzy on what would be the correct time to use one -- do you have good sense @SayakaOno?

Interestingly, removing the factory pattern here did cause a crash on the import of the task selector into the product slice, and made me realize this selector shouldn't have been in product in the first place... that's a plus I think! 👍

I did completely lose my desire to see this as a selector in the process of looking into this, but I kept it as such only because I wanted to see the memoization working for my own understanding.

Thanks for the articles @SayakaOno ! I have not thought this hard about this stuff before now!

So how I am understanding the guidance is:

Use a selector function for simple lookups on state:

// slice const selectC = state => state.c // app const useC = useSelector(selectC)

It seems like createSelector is itself a essentially a useMemo wrapping a selector, it has input args that it watches for changes for and the output function using those args so for "expensive" logic use createSelector:

// slice const selectCPrime = createSelector( (selectC), (C) => {return C.map(C*100000).filter().etc();} ) // app const useC = useSelector(CPrime);

Create a factory pattern if we plan on purposefully creating many unique memoized functions. For example a function with a state that should not be shared (in my head an example is state.isLoading on a bunch of separate items -- but its not a great example).

//slice const selectCPrimeFactory = () => createSelector((selectC, isLoading), (C, isLoading) => {return isLoading || C.map(C*100000).filter().etc();}) > // app const item1 = useSelector(selectCPrimeFactory()(isLoading1)) const item2 = useSelector(selectCPrimeFactory()(isLoading2))

Using a hook:
It seemed like we were using hooks when we have RTKquery set up, so that was why it seemed right to me to use a selector. In the future if we modularize our rtkQuery files (similar to Gursimrans weather API) using the reducerPath arg in createAPI() we could store and populate some slices so that we have more integration with core toolkit where valuable as well as access to createSelectors for memoization without creating as many custom hooks? I added an AI convo in Tech Daily note with some examples I thought looked good.

So for this function! Haha I think you did the right thing refactoring and it looks correct!

Duncan's interpretation is aligned with mine. I created a quick demo to see how each option behaves in practice, so we could take a look at it on Monday if you guys haven’t tested it yet. I couldn’t think of good examples where a selector factory would be preferable either, but the products selector might be a potential case. I’ll talk about it on Monday as well!
I’ll also try to familiarize myself with RTK Query’s reducerPath by then. A lot to learn...

The updated selector looks great, thank you Joyce!

packages/webapp/src/containers/ProductInventory/ProductForm/useRemoveProduct.ts

kathyavini · 2025-10-10T18:49:25Z

@SayakaOno I would like to try the modal state refactor! 🙏

Just wanted to check -- you mean something like this?

export enum ModalType {
  NONE = 'none',
  CONFIRM = 'confirm',
  CANNOT_REMOVE = 'cannotRemove',
}

const [modalType, setModalType] = useState<ModalType>(ModalType.NONE)

Edit: I'll push a change like this but if you could please double-check!

SayakaOno

I think modalType could be undefined instead of none, but it's probably a preference?
Looks good! ❤️

SayakaOno · 2025-10-10T19:29:52Z

packages/webapp/src/containers/productSlice.js

  });
 };

+export const isProductUsedInPlannedTasksSelector = (product_id) => {


I've been paying more attention to selector memoization recently (since you asked about shallowEqual 😂), so I'm still learning, and I'll borrow ChatGPT's explanation...

if we call useSelector(isProductUsedInPlannedTasksSelector(product_id)) directly in the render, a new selector is created every time, so createSelector won’t actually memoize across renders. It works perfectly if the selector instance is stable (e.g., with useMemo).

So, to benefit from the selector’s memoization, we’d need useMemo; without it, I think it works just like a plain function! (but we've been doing it, and I'm fine with the selector without useMemo :))

kathyavini · 2025-10-10T19:52:36Z

Sorry to invalidate review @SayakaOno but I just spotted a bug! If the API error snackbar triggers the form mode re-opens but without the buttons since the reset to FormMode.READ_ONLY was missing in that logical branch 🙇

if we call useSelector(isProductUsedInPlannedTasksSelector(product_id)) directly in the render, a new selector is created every time, so createSelector won’t actually memoize across renders. It works perfectly if the selector instance is stable (e.g., with useMemo).

Is ChatGPT for sure correct about this????! For some reason I can't believe it... I can't imagine that every useSelector() call was meant to be wrapped in useMemo()! I really am clueless though so I'm going to have to hit the RTK docs and read into this now as well 🙏

Duncan-Brain

Still working for me!

kathyavini added 9 commits October 8, 2025 11:21

LF-4966 Add new strings

a032fbf

LF-4966 Add new modals

3931611

LF-4966 Add delete mutation and remove unused tag

6bb4fec

LF-4966 Add selector for product deletability check

7128204

LF-4966 Add useRemoveProduct.ts hook

c12f64e

LF-4966 Refactor hook props

5ccd82c

LF-4966 Move inventory menu item outside of isAdmin block

70dc8a0

LF-4966 Render delete button conditionally on admin status

62d341d

LF-4966 Merge integration and fix conflict in ProductForm imports

ed1c940

kathyavini self-assigned this Oct 9, 2025

kathyavini requested review from a team as code owners October 9, 2025 15:57

kathyavini added enhancement New feature or request new translations New translations to be sent to CrowdIn are present labels Oct 9, 2025

kathyavini requested review from Duncan-Brain and removed request for a team October 9, 2025 15:58

kathyavini commented Oct 9, 2025

View reviewed changes

Duncan-Brain previously approved these changes Oct 10, 2025

View reviewed changes

SayakaOno reviewed Oct 10, 2025

View reviewed changes

kathyavini added 2 commits October 10, 2025 11:14

LF-4966 Fix setter capitalization

08547fa

LF-4966 Call onProductsFetched() logic directly after removal

62e4d93

LF-4966 Combine modal state

4f68976

kathyavini dismissed Duncan-Brain’s stale review via 4f68976 October 10, 2025 18:57

LF-4966 Remove extra state setting line

b5725fc

SayakaOno previously approved these changes Oct 10, 2025

View reviewed changes

LF-4966 Restore FormMode.READ_ONLY in the case of API error

7d5e11b

kathyavini dismissed SayakaOno’s stale review via 7d5e11b October 10, 2025 19:52

kathyavini added 2 commits October 14, 2025 14:05

LF-4966 Fix PropType console error

934f678

LF-4966 Re-write selector without using a factory

b1f3725

Duncan-Brain approved these changes Oct 15, 2025

View reviewed changes

SayakaOno approved these changes Oct 15, 2025

View reviewed changes

SayakaOno added this pull request to the merge queue Oct 15, 2025

Merged via the queue into integration with commit a107a35 Oct 15, 2025
5 checks passed

SayakaOno deleted the LF-4966-implement-delete-product-functionality branch October 15, 2025 19:07

LF-4966 implement delete product functionality #3896

LF-4966 implement delete product functionality #3896

Uh oh!

Conversation

kathyavini commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kathyavini Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kathyavini Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Duncan-Brain commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kathyavini commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Duncan-Brain left a comment

Choose a reason for hiding this comment

Uh oh!

SayakaOno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kathyavini Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Duncan-Brain Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kathyavini commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SayakaOno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kathyavini commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Duncan-Brain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kathyavini commented Oct 9, 2025 •

edited

Loading

kathyavini Oct 9, 2025 •

edited

Loading

kathyavini Oct 9, 2025 •

edited

Loading

Duncan-Brain commented Oct 9, 2025 •

edited

Loading

kathyavini commented Oct 9, 2025 •

edited

Loading

kathyavini Oct 14, 2025 •

edited

Loading

Duncan-Brain Oct 15, 2025 •

edited

Loading

kathyavini commented Oct 10, 2025 •

edited

Loading

kathyavini commented Oct 10, 2025 •

edited

Loading