LF-4967 Implement product filter

[kathyavini]

Description

This adds the filter to Product Inventory, following Animal Inventory exactly (except the fallback in filterSlice -- see in-line comment below).

A common component has been refactored out of AnimalFilter and ProductInventoryFilter.

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

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?

• 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]

It's been a long time since we've had this kind of change to the shape of an existing Redux slice, so I don't 100% remember if we have an established way to handle it.

We could either force logout or supply a fallback as above; otherwise the selector (and setter) will error when trying to access the nonexistent tempStateReducer.filterReducer.inventory. I'm thinking logout is certainly more common as our pattern, but we have to then commit to bumping the app version when this goes live, and since I wasn't sure if we wanted to commit to that, I added fallback here.

[kathyavini]

Looks like i18n had not touched this file in a while, so the keys got ordered alphabetically.

[SayakaOno]

It works great!

Is it possible to create a reusable filter container that accepts the filter content as children? Encapsulating isFilterOpen/isDirty state management etc. would be nice...
At least, I’d prefer to import the SCSS file from the animal filter as you did before. Was there a reason you duplicated it instead of reusing it?

[SayakaOno]

Could we pass onChange directly?

[kathyavini]

Thank you!! I don't think I would have ever noticed that; callback prop drilling still really trips me up 😓

[kathyavini]

Is it possible to create a reusable filter container that accepts the filter content as children? Encapsulating isFilterOpen/isDirty state management etc. would be nice...
At least, I’d prefer to import the SCSS file from the animal filter as you did before. Was there a reason you duplicated it instead of reusing it

Maybe it wasn't very good reasoning, but I duplicated instead of referring for this one since we have so many filter drawers in app (as opposed to only 2 inventories) and I thought maybe they should stay independent from each other! I also thought perhaps the styling would be changed in some way as I finished the component, but it was definitely not so 😅

I see the old filters had all been grouped via the <PureFilterPage /> component, so I think these two as well are worthy of an extract + shared component... I'll give that a go 🙏

[kathyavini]

@SayakaOno thank you -- I think an extraction of a shared component was definitely necessary here! Could you please take a look and re-review? 🙏 The extraction should not have changed any logic.

[SayakaOno]

I love it!!!

I’m being a bit greedy and want to take it one step further…
The more I look at it, the more I feel like the new FilterDrawerContainer could maybe contain <FilterGroup />. Handling onChange would be needed, but now that ProductFilter and AnimalsFilter are super lean, filters could go in there...?

You’ve already done so much, so no pressure! I just thought it could be a nice improvement if you agree :)

[kathyavini]

The more I look at it, the more I feel like the new FilterDrawerContainer could maybe contain . Handling onChange would be needed, but now that ProductFilter and AnimalsFilter are super lean, filters could go in there...?

This is such an interesting suggestion, because in a certain way, it's very much going full circle back to our original implementation of filters! Like if you look at one of the older filters in app (e.g. DocumentsFilterPage), it connects to the Redux store, sets up the filters, and imports <PureFilterPage /> which is the layout component with the filter update logic that contains <FilterGroup />!

It makes me wonder if the use of the independent <...FilterContent /> pattern was uniquely necessary for transactions filters, since they had to be imported into two places?

That said, I'm not sure I want to move off the modularity of the <...FilterContent /> pattern in particular yet -- I think it's kind of nice and makes adding a new filter very simple/boilerplate. But I do agree that the ProductFilter and AnimalsFilter wrappers are lean to the point of being silly! I might still prefer that though, just to keep the flexibility and simplicity of a separate filter content.

It's probably not for now -- or at least I'm not super inspired to pick it up 😅 -- but I agree there is definitely scope for more filter refactor here. At the very least I think the main TransactionFilter should be using the same component as Animals and Inventory now, shouldn't it?

[SayakaOno]

At the very least I think the main TransactionFilter should be using the same component as Animals and Inventory now, shouldn't it?

That looks like it! I don’t fully remember how we ended up with this filter code, but I like PureFilterPage ...
It makes a lot of sense to have a TransactionFilterContent!

I initially got overwhelmed by the number of files I’d have to review for the filter, so I think I was unconsciously trying to reduce the diffs and my mental load 😅
I haven’t worked on a filter myself yet, but with the current setup, I feel more comfortable reviewing or creating a new filter. So I'll merge, thank you!