store applied filters in onyx #48312

289Adam289 · 2024-08-30T08:43:13Z

Details

Fixed Issues

$ #48156
$ #48407
PROPOSAL:

Tests

Open the app
Go to Search
From the available options, select both a Type (Expense, Invoice, Trip, or Chat) and a Status (from the status bar).
Click "Filters" button
Verify that the available filters are relevant to the selected type
Apply filters as needed
Close the filters page
Click "Filters" button
Verify that the previous filters values are selected
Click on "View results"
Verify that the displayed results match the previously selected Type and Status
Click "Filters" button
Verify that the previously used filters are selected

Verify that no errors appear in the JS console

Offline tests

QA Steps

Open the app
Go to Search
From the available options, select both a Type (Expense, Invoice, Trip, or Chat) and a Status (from the status bar).
Click "Filters" button
Verify that the available filters are relevant to the selected type
Apply filters as needed
Close the filters page
Click "Filters" button
Verify that the previous filters values are selected
Click on "View results"
Verify that the displayed results match the previously selected Type and Status
Click "Filters" button
Verify that the previously used filters are selected

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2024-09-03.at.10.58.07.mov

Android: mWeb Chrome

Screen.Recording.2024-09-03.at.09.28.39.mp4

iOS: Native

Screen.Recording.2024-09-02.at.15.06.02.mp4

iOS: mWeb Safari

Screen.Recording.2024-09-02.at.15.06.02.mov

MacOS: Chrome / Safari

Screen.Recording.2024-09-02.at.14.04.41.mov

MacOS: Desktop

Guccio163

LGTM when it comes to strictly this PR, but I found bug in Category filter and it's probably linked to parser, maybe you can take a look at it @289Adam289 :

If you create Category with & written inside, select this category in Filters, save and display results, the whole page crashed:

Screen.Recording.2024-08-30.at.15.42.57.mov

Additionally, it doesn't happen if I create tag with & alone in it, which encoded version in &

Kicu

In general code looks good, I have left mostly minor comments about naming, and 1 comment to simplify code.
Good job 👍

src/libs/SearchUtils.ts

src/components/Search/SearchPageHeader.tsx

src/libs/SearchUtils.ts

src/pages/Search/SearchTypeMenuNarrow.tsx

melvin-bot · 2024-09-03T09:13:09Z

@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

289Adam289 · 2024-09-03T09:13:30Z

@luacmartins ready for review

Kicu

LGTM, left some more small comments

src/pages/Search/AdvancedSearchFilters.tsx

src/libs/SearchUtils.ts

src/components/Search/types.ts

luacmartins

Looks great. Left a small comment.

src/components/Search/SearchPageHeader.tsx

src/pages/Search/SearchTypeMenuNarrow.tsx

rayane-djouah · 2024-09-04T15:53:44Z

src/libs/SearchUtils.ts

-            if (keyInCorrectForm) {
-                return `${CONST.SEARCH.SYNTAX_FILTER_KEYS[keyInCorrectForm]}:${filterValue as string}`;
-            }
+            return `${filterValue as string}`;


Do we need to sanitize the string for the keyword filter?

At first I didn't think it was necessary but after some thought if we add operator symbols to sanitizeString function and sanitize every word of an input it should prevent some bugs.

While working on keyword filter sanitization i came across another problem. Quotation marks are hard to parse. For example category with a single quote mark breaks the parser and even sanitizing it wont help. I suggest we discuss this problem in another issue cause it may need some fundamental changes in grammar itself. cc @luacmartins @Kicu

I agree that it'd require changes to the grammar and we can handle that as a separate issue.

src/libs/SearchUtils.ts

rayane-djouah · 2024-09-04T19:04:34Z

src/libs/SearchUtils.ts

+        if (
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.CATEGORY ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.CARD_ID ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.TAX_RATE ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.EXPENSE_TYPE ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.TAG ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.CURRENCY ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.FROM ||
+            filterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.TO
+        ) {


Let's add In and Has filters keys

We can also consider defining a MULTIPLE_VALUES_FILTER_KEYS const and then check for Object.values(MULTIPLE_VALUES_FILTER_KEYS).includes(filterKey) instead. This will simplify the condition if we need it in multiple places

rayane-djouah · 2024-09-04T19:36:47Z

I've noticed that we're not excluding duplicate keyword filter values

289Adam289 · 2024-09-05T10:30:17Z

I don't think duplicate keyword filter values are a problem. If a user chooses to enter "test test" then I think we should leave it as it is.

luacmartins · 2024-09-05T17:23:16Z

I don't think duplicate keyword filter values are a problem. If a user chooses to enter "test test" then I think we should leave it as it is.

I agree this is not a problem. We're also updating the grammar here that will combine them into a single node

luacmartins

LGTM

luacmartins

LGTM

luacmartins · 2024-09-09T19:51:38Z

I'm gonna merge this PR as is and then work on a follow up to address this comment

luacmartins · 2024-09-09T23:44:41Z

@289Adam289 if you'd like you can remove the Has and Is filter from the advanced filters page, so we can move this PR forward. I'll work on adding them to the status bar, since that requires some backend changes.

289Adam289 · 2024-09-10T07:29:01Z

I can do that

rayane-djouah · 2024-09-10T12:16:10Z

@289Adam289 I've found a bug:

Open the expense type search.
Open filters and select values.
Navigate back from the filters page; note that the filter values remain saved.
Change the search type to "chat."
Re-open the filters page.
Observe that the previously saved filter values are still being applied for the common filters between expense and chat types.
Click "view results."

Expected Results: Only filters relevant to the "chat" type, from the previously saved filter values, should be applied.
Or, filters values should be cleared when we change the type.

Actual Results: Filters related to both the "chat" and the original "expense" type from the saved filter values are incorrectly applied together.

Screen.Recording.2024-09-10.at.1.08.42.PM.mov

rayane-djouah · 2024-09-10T12:31:46Z

Regarding the QA steps, steps 9 and 10 should be before "clicking view results button" because we clear the selected values when we click on the button, Could you please update the steps to:

Open the app
Go to Search
From the available options, select both a Type (Expense, Invoice, Trip, or Chat) and a Status (from the status bar).
Click "Filters" button
Verify that the available filters are relevant to the selected type
Apply filters as needed
Close the filters page
Click "Filters" button
Verify that the previous filters values are selected
Click on "View results"
Verify that the displayed results match the previously selected Type and Status
Click "Filters" button
Verify that the filters values are empty (cleared)

289Adam289 · 2024-09-10T12:36:16Z

I don't think last step is correct. Filter values should be the same as before clicking "ViewResults"

rayane-djouah · 2024-09-10T12:39:08Z

I don't think last step is correct. Filter values should be the same as before clicking "ViewResults"

@289Adam289 We're currently clearing the filters here when we click on "View Results" :

App/src/pages/Search/AdvancedSearchFilters.tsx

Line 265 in fe4d19e

SearchActions.clearAdvancedFilters();

Do we need to change it?

rayane-djouah · 2024-09-10T12:59:19Z

@289Adam289 We've encountered another bug:

The "Reset filters" button is visible on the filters page even when no values have been selected. When accessing the filters page from a different type (e.g., "Chat") and clicking the "Reset filters" button, the filters displayed switch to those related to the "Expense" type instead of remaining consistent with the currently selected type.

Screen.Recording.2024-09-10.at.1.54.55.PM.mov

289Adam289 · 2024-09-10T14:08:04Z

We're currently clearing the filters here when we click on "View Results" :

Onyx is cleared to prevent bugs and make url the only source of truth. So we shouldn't change it.
The goal od this PR is to get filters from url and set values in Onyx upon clicking filter button

luacmartins · 2024-09-10T15:55:17Z

It seems like we have a couple of minor bugs left. Let's squash them and get this one merged 😄

Kicu · 2024-09-11T07:10:23Z

To clarify about filter values in onyx.
Storing them in onyx was added only for the Advanced Filters Form to work.
In some of the filters we are using FromProvider which expects to get an onyx key with the form state - won't work otherwise. So it made the most sense to make the whole form use onyx as storage, both for single value filters and multi-picker filters.

However we store the values in onyx only when the form page (search/filters) is open. When you're on other Search pages we store nothing in onyx - URL (or params in navigation state) are the single source of truth.

289Adam289 · 2024-09-11T08:49:17Z

Now Onyx is cleared when changing type. Reset button is only visible when filters are set and it clears only advanced filters.

rayane-djouah · 2024-09-11T10:45:37Z

Screenshots/Videos

Android: Native

Screen.Recording.2024-09-11.at.11.40.56.AM.mov

Android: mWeb Chrome

Screen.Recording.2024-09-11.at.11.33.02.AM.mov

iOS: Native

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-11.at.11.29.47.mp4

iOS: mWeb Safari

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-09-11.at.11.25.31.mp4

MacOS: Chrome / Safari

Screen.Recording.2024-09-11.at.10.57.34.AM.mov

MacOS: Desktop

Screen.Recording.2024-09-11.at.11.22.55.AM.mov

rayane-djouah · 2024-09-11T10:48:14Z

@289Adam289, on both the Android native app and mobile Chrome web version, the filter values sometimes do not clear when changing the type. Could you please try to reproduce this issue on your end?

Android: Native

Screen.Recording.2024-09-11.at.11.40.56.AM.mov

Android: mWeb Chrome

Screen.Recording.2024-09-11.at.11.33.02.AM.mov

rayane-djouah · 2024-09-11T12:17:28Z

Fixed! thanks!

Screen.Recording.2024-09-11.at.1.14.00.PM.mov

rayane-djouah

LGTM and tests well

rayane-djouah · 2024-09-11T12:22:18Z

@luacmartins all yours!

luacmartins

LGTM

OSBotify · 2024-09-11T14:56:35Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-09-12T21:18:53Z

🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.33-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

289Adam289 added 2 commits August 30, 2024 10:35

add applied filters to onxy upen filters button press

e08f197

Merge branch 'main' into 289Adam289/48156-store-applied-filters

c265761

Guccio163 approved these changes Aug 30, 2024

View reviewed changes

289Adam289 added 4 commits September 2, 2024 13:57

change toString function

cebebc8

Merge branch 'main' into 289Adam289/48156-store-applied-filters

c55bb3e

add applied filters on narrow view

88e8219

improve function names

d0028ce

Kicu suggested changes Sep 2, 2024

View reviewed changes

289Adam289 added 2 commits September 2, 2024 16:11

review suggestions

1b2eecf

Merge branch 'main' into 289Adam289/48156-store-applied-filters

2616394

rayane-djouah mentioned this pull request Sep 2, 2024

[Search v2.1] Create In filter #46030 #47676

Merged

50 tasks

fix ts and ampersant error

3260c11

289Adam289 marked this pull request as ready for review September 3, 2024 09:13

289Adam289 requested a review from a team as a code owner September 3, 2024 09:13

melvin-bot bot requested review from rayane-djouah and removed request for a team September 3, 2024 09:13

Kicu approved these changes Sep 3, 2024

View reviewed changes

src/pages/Search/AdvancedSearchFilters.tsx Outdated Show resolved Hide resolved

src/libs/SearchUtils.ts Outdated Show resolved Hide resolved

src/components/Search/types.ts Show resolved Hide resolved

289Adam289 added 2 commits September 3, 2024 13:04

review suggestions

975349e

Merge branch 'main' into 289Adam289/48156-store-applied-filters

c1d4ca5

luacmartins self-requested a review September 4, 2024 17:16

luacmartins reviewed Sep 4, 2024

View reviewed changes

src/components/Search/SearchPageHeader.tsx Outdated Show resolved Hide resolved

src/pages/Search/SearchTypeMenuNarrow.tsx Outdated Show resolved Hide resolved

rayane-djouah suggested changes Sep 4, 2024

View reviewed changes

289Adam289 added 2 commits September 5, 2024 12:39

review

507fc44

fix function signature

e5fb785

luacmartins previously approved these changes Sep 5, 2024

View reviewed changes

luacmartins previously approved these changes Sep 9, 2024

View reviewed changes

remove is and has filters

ad05cf4

289Adam289 dismissed luacmartins’s stale review via ad05cf4 September 10, 2024 09:06

fix typecheck

b57bbcf

fix filters and reset button

e96f20d

fix on narrow layout

263c291

rayane-djouah approved these changes Sep 11, 2024

View reviewed changes

melvin-bot bot requested a review from luacmartins September 11, 2024 12:19

luacmartins approved these changes Sep 11, 2024

View reviewed changes

luacmartins merged commit 3e3388d into Expensify:main Sep 11, 2024
16 checks passed

github-actions bot mentioned this pull request Sep 11, 2024

Deploy Checklist: New Expensify 2024-09-11 #49025

Closed

49 tasks

rayane-djouah mentioned this pull request Sep 17, 2024

[HOLD for Payment 2024-09-19][Search v2.1] Set currently applied filters on Advanced Filters Page upon clicking Filters Button #48156

Closed

store applied filters in onyx #48312

store applied filters in onyx #48312

Conversation

289Adam289 commented Aug 30, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Guccio163 left a comment • edited Loading

Choose a reason for hiding this comment

Kicu left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Sep 3, 2024

289Adam289 commented Sep 3, 2024

Kicu left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

rayane-djouah Sep 4, 2024

Choose a reason for hiding this comment

289Adam289 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

289Adam289 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

luacmartins Sep 5, 2024

Choose a reason for hiding this comment

rayane-djouah Sep 4, 2024

Choose a reason for hiding this comment

rayane-djouah Sep 4, 2024

Choose a reason for hiding this comment

rayane-djouah commented Sep 4, 2024

289Adam289 commented Sep 5, 2024

luacmartins commented Sep 5, 2024

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins commented Sep 9, 2024

luacmartins commented Sep 9, 2024

289Adam289 commented Sep 10, 2024

rayane-djouah commented Sep 10, 2024 • edited Loading

rayane-djouah commented Sep 10, 2024

289Adam289 commented Sep 10, 2024

rayane-djouah commented Sep 10, 2024 • edited Loading

rayane-djouah commented Sep 10, 2024

289Adam289 commented Sep 10, 2024

luacmartins commented Sep 10, 2024

Kicu commented Sep 11, 2024 • edited Loading

289Adam289 commented Sep 11, 2024

rayane-djouah commented Sep 11, 2024

Screenshots/Videos

rayane-djouah commented Sep 11, 2024 • edited Loading

rayane-djouah commented Sep 11, 2024

rayane-djouah left a comment

Choose a reason for hiding this comment

rayane-djouah commented Sep 11, 2024

luacmartins left a comment

Choose a reason for hiding this comment

OSBotify commented Sep 11, 2024

OSBotify commented Sep 12, 2024

289Adam289 commented Aug 30, 2024 •

edited

Loading

Guccio163 left a comment •

edited

Loading

289Adam289 Sep 5, 2024 •

edited

Loading

289Adam289 Sep 5, 2024 •

edited

Loading

rayane-djouah commented Sep 10, 2024 •

edited

Loading

rayane-djouah commented Sep 10, 2024 •

edited

Loading

Kicu commented Sep 11, 2024 •

edited

Loading

rayane-djouah commented Sep 11, 2024 •

edited

Loading