[Search v2] [App] Use new query syntax #45617

adamgrzybowski · 2024-07-17T16:24:12Z

Details

This PR introduces new query syntax. Now you can see that in the URL we have q or cq params when on the search page.

Fixed Issues

$ #45027
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

Changing status option

Open the search tab
Change status from expenses to shared
See if the app loads shared entries

Changing workspace

Open the search tab
Open workspace switcher
Select different workspace
See if the entries are filtered for selected workspace

Wide layout only (web/desktop)

Open the search tab
Make sure the expenses option is selected
Press the date column to change sorting options
See if entries are properly sorted

PR Author Checklist

Screenshots/Videos

Android: Native

android.mp4

Android: mWeb Chrome

androidWeb.mp4

iOS: Native

ios.mp4

iOS: mWeb Safari

iosWeb.mp4

MacOS: Chrome / Safari

web.mp4

MacOS: Desktop

desktop.mp4

luacmartins

I know this is a draft but I wanted to leave some early comments

src/CONST.ts

src/ROUTES.ts

melvin-bot · 2024-07-19T14:52:01Z

@situchan 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]

adamgrzybowski · 2024-07-19T14:52:19Z

@luacmartins Ready for the review! I will add videos and tests on Monday

Kicu

after initial review seems pretty nice, will take another look later

src/pages/Search/SearchPageBottomTab.tsx

src/components/Search/index.tsx

src/pages/Search/SearchPageBottomTab.tsx

src/pages/Search/SearchStatusMenu.tsx

src/CONST.ts

luacmartins · 2024-07-22T18:43:38Z

src/ROUTES.ts

-        },
+        route: 'search',
+        getRoute: ({query, queryKind = CONST.SEARCH.QUERY_KIND.CANNED_QUERY, policyIDs}: {query: SearchQueryString; queryKind?: QueryKind; policyIDs?: string}) =>
+            `search?${queryKind === CONST.SEARCH.QUERY_KIND.CANNED_QUERY ? 'q' : 'cq'}=${query}${policyIDs ? `&policyIDs=${policyIDs}` : ''}` as const,


I think we need to include the policyID in the query as well so that the AST includes it in the filters. Any reason to treat it differently?

I think this is a transitional step.

BTW will current grammar handle policyID? I don't see this keyword in the .peggy file

Ah makes sense that we do it like this for now. We should eventually move it to the AST and include it in the peggy grammar too

Do we need a separate issue to migrate the policyID param to the AST?

Do you think it should be in the root of QueryJSON? So something like

type JSONQuery = { input: string; hash: number; type: string; status: string; sortBy: string; sortOrder: string; offset: number; filters: ASTNode; policyID: string; };

Also currently it is called policyIDs, as the assumption was that we wanted to be able to use multiple policyIDs in the future. Is this still the case?

We'll add it to filters as policyID. So the AST will look like this:

{filters: { "operator": "eq", "left": "policyID", "right": "<policyID>"

I think we should use policyID singular from now on since we have a bunch of other filters, e.g. category, tag, etc that accepts multiple values but are all singular too.

I hope I understand this correctly. I will add policyID to grammar and keep it inside AST, while removing it from URL

read my comment here: #45617 (comment)

src/components/Search/index.tsx

src/components/Search/SearchPageHeader.tsx

src/components/Search/index.tsx

src/libs/SearchUtils.ts

luacmartins · 2024-07-22T19:31:55Z

src/libs/SearchUtils.ts

+function normalizeQuery(query: string) {
+    const normalizedQueryJSON = buildSearchQueryJSON(query);
+    return buildSearchQueryString(normalizedQueryJSON);
+}


Why do we need this function? I think that it could inject params into the query string that were not defined by the user and that might mess up with how we display the filter UI, no?

This is something we could discuss more about. We have a couple of questions here:

Do we want to put defaults for sort etc in the URL? (I think yes, they were here in the previous version).

Do we want to organize the query for the user? So is the:
type:expenses sortBy:date same query as sortBy:date type:expenses I would say yes. In that case we should sort the keywords in the query to make it look the same.

And to address your concerns. It will inject defaults but nothing more. It shouldn't break UI filters.

Another argument for normalizing but it will be relevant in the future. If we ever want to change the defaults it could break the saved queries if we don't normalize them.

Do we want to put defaults for sort etc in the URL? (I think yes, they were here in the previous version).

Thinking a bit ahead, we'd only want to do that if the user explicitly types the defaults in their queries, otherwise it'd be weird for the user to type amount>200 and then see the search bar update to type:expense status:all amount>200 sortBy:date sortOrder:desc offset:0. I don't think we should modify the user input in that way and the URL is kinda the only thing tying everything up, e.g. the user input, the UI filters and the syntax itself. I think we'd only want to normalize it/build the AST when hashing and sending the API request

This is NAB for now, but something that we should align on before we get too deep in it

Yeah, we may need to brainstorm this topic a little. I feel like not normalizing the query may be a pain in the future. On the other hand, it's not a perfect solution either.

Let's look at this example:

User types simple query like type:expenses

Press the date column name to change the sorting order.

Now the query looks like type:expenses sortOrder:asc sortBy:date

The user presses the column again and in theory we have same result as in the step 1 but the query looks differently type:expenses sortOrder:desc sortBy:date

Maybe we could think about hiding the defaults in places we want to display the query to the user instead of removing them from the query and logic.

I agree with Adam and I feel the code will make much more sense if query is a single source of truth and thus always contains all the values that are actually used when sending query to api.
I don't think we should worry how the URL looks for the user - it looks ugly anyways since it's url-encoded 😅

That being said this PR is blocking some other work, so I'm a fan of leaving this conversation for later and moving forward with this.

I think the concern here is that we were gonna use the decoded URL to populate the search bar (once that's available) with the value the user input, but I see now that any interaction with the UI will also cause this issue. From the example above:

User types simple query like type:expenses

Press the date column name to change the sorting order.

Now the query looks like type:expenses sortOrder:asc sortBy:date

We'd display type:expenses sortOrder:asc sortBy:date in the Search bar, which is different than what the user typed

The user presses the column again and in theory we have same result as in the step 1 but the query looks differently type:expenses sortOrder:desc sortBy:date, which is also not what the user input was.

I think we might need another solution for this problem, so in all cases we only show type:expenses to the user since that's what they typed into the search bar. That's a problem for v2.3, but we should keep it in mind.

luacmartins · 2024-07-22T19:45:57Z

src/types/onyx/SearchResults.ts

+            // eslint-disable-next-line jsdoc/require-jsdoc
+            drafts: boolean;
+            // eslint-disable-next-line jsdoc/require-jsdoc
+            approved: boolean;


We actually return all statuses to the client, e.g. all, drafts, outstanding, approved, paid

So should I add all the fields? I am not sure if I understand this part of the code

Yea, I think we should add all of them for now since they are always returned by the server

Do you mean these statuses? I don't see any outstanding in the costs.

STATUS: { ALL: 'all', SHARED: 'shared', DRAFTS: 'drafts', FINISHED: 'finished', },

Not sure where to look for these values 😄

They are here. We should still support Shared, Finished for now, but we'll deprecate that one soon and add the other statuses mentioned in the doc, e.g. outstanding, approved, paid

why do we have to add this in this PR? I don't understand why this suddenly appeared here, when I don't see this being used anywhere.

Can I just remove it if it's unused and we can add it in the PR that this is actually being used?

src/pages/Search/SearchStatusMenuNarrow.tsx

luacmartins

It seems like loading more results is broken. We don't pass the correct offset param to the API call even thought it's updated in the URL

adamgrzybowski · 2024-07-23T16:33:57Z

Hi @luacmartins. @Kicu and I concluded that we should remove the offset property from the query.

Hash is generated from the query string. Offset is part of the query string. This means changing the offset will change the hash. This breaks caching and loading new elements.

Deep linking with URL that contains offset also won't load elements properly. We can't simply jump to offset 150. We need to go through 0, 50, 100, 150 to load all elements.

Instead, we could just keep it in the state in the Search component.

I would propose to do it as a follow up up though. This PR is necessary to move forward. It would be great to merge it soon. BTW I will be ooo Jul 25-29 and I don't want to block others.

src/pages/Search/SearchPage.tsx

luacmartins · 2024-07-23T22:36:35Z

src/libs/actions/Search.ts

+// TODO_SEARCH: use this function after backend changes.
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+function searchV2(queryString: SearchQueryString) {
+    const queryJSON = buildSearchQueryJSON(queryString);


It seems like we're already passing the AST here, so no need to try to build it again

Suggested change

const queryJSON = buildSearchQueryJSON(queryString);

Not sure if I understand. It looks like we pass queryString here not queryJSON.

Hmm in my tests I had to remove this to make it work with the backend.

luacmartins · 2024-07-23T22:36:59Z

src/libs/actions/Search.ts

+
+    // TODO_SEARCH: uncomment this line after backend changes
+    // @ts-expect-error waiting for backend changes
+    API.read(READ_COMMANDS.SEARCH, queryJSON, {optimisticData, finallyData});


Suggested change

API.read(READ_COMMANDS.SEARCH, queryJSON, {optimisticData, finallyData});

API.read(READ_COMMANDS.SEARCH, {hash, jsonQuery: JSON.stringify(jsonQuery)}, {optimisticData, finallyData});

luacmartins · 2024-07-23T22:37:19Z

src/libs/actions/Search.ts

@@ -53,6 +55,22 @@ function search({hash, query, policyIDs, offset, sortBy, sortOrder}: SearchParam
    API.read(READ_COMMANDS.SEARCH, {hash, query, offset, policyIDs, sortBy, sortOrder}, {optimisticData, finallyData});
 }

+// TODO_SEARCH: use this function after backend changes.
+// eslint-disable-next-line @typescript-eslint/no-unused-vars
+function searchV2(queryString: SearchQueryString) {


Suggested change

function searchV2(queryString: SearchQueryString) {

function searchV2(jsonQuery: SearchQueryJSON) {

rayane-djouah · 2024-07-25T13:05:42Z

@rayane-djouah is the Bug 2 related to changes in this PR? because HOLD and bulk is not something that should be modified here

@Kicu - Bug 2 is not reproducible on main. Maybe it's related to our changes concerning TRANSACTION_HOLD_REASON_RHP route?

Kicu · 2024-07-25T13:26:58Z

Ah yes might be, thanks I will test it.

Meanwhile for bug 1 I think the problem might be that policyIDs needs to be part of hash for a single search query, because hash becomes part of onyx key.
So for your reproduction what (I believe, not sure 100%) happens is:

we filter a specific query by policyIDs, we get correct data
we change the workspaces from A to B
we get new filtered workspaces (as I made sure the correct call is made to backend)
we get the same hash because policyIDs is not part of hash
we display all the same results which will be mixed results from 2 calls

Good catch.

But in general I now have na "existential" 😅 problem with this PR. I expected that I will help fix several comments and we can finally close this.
However @luacmartins the switch for policyIDs that you suggested - although it makes sense - is a really big change.
policyIDs is strictly connected with workspace Switcher and with all the other routes that use /w/<policyID>/.... it also is connected with export to CSV.

If we'd have to now remove it from params and move it to inside AST then that's editing 15 files and possibly breaking some functionality. IMO that will delay this PR for at least 2 days.
Also I actually have no idea how the new policyID param inside AST would work with existing workspace switcher? If it should just seamless work then we'd need to parse query in multiple places to be able extract this param, for flows like:

go to reports page
switch workspace
go to search
should it already have search?q=policyID:xxxxx in this scenario?

To sum up here are my questions about how to move forward @luacmartins
Q1 can we move forward with this PR keeping policyIDs as is? this can lead to some bugs but we can fix it in separate PRs
Q2 how will policyIDs interact with existing workspace switcher component
Q3 how are we switching to a new backend? will current Search simply stop working at some point and we have to switch sending search calls with jsonQuery? do I have to do it inside this PR?

Kicu · 2024-07-25T14:13:10Z

@rayane-djouah my latest commit fixes Bug 2 via optional params.
Bug 1 would be fixed automatically if we would put policyID to hash generation, but Im not sure if we want to do this in this PR (see my previous long comment).

luacmartins · 2024-07-25T14:54:32Z

Bug 1 would be fixed automatically if we would put policyID to hash generation, but Im not sure if we want to do this in this PR (see my previous long comment).

I think we should include the policyID in the hash so we don't break the existing functionality

Kicu · 2024-07-25T15:16:29Z

I tried to fix Bug 1 with my newest commit and upon quick check it should work, however i need to drop now so please double check if I didn't break anything else.

Temporarily we can add policyID to has generation which should make our hashes stable and reacting to changing policyID.

In future when we move policyID to inside filters as part of AST this bug will no longer exist

rayane-djouah · 2024-07-25T15:18:32Z

Thank you! @Kicu

rayane-djouah · 2024-07-25T15:18:41Z

@Kicu - Is there any reason we're not using the searchV2 function yet?

luacmartins

LGTM and tests well

rayane-djouah · 2024-07-25T15:29:01Z

src/pages/Search/SearchPage.tsx


-    const query = rawQuery as SearchQuery;
-    const isValidQuery = Object.values(CONST.SEARCH.TAB).includes(query);
+    const queryJSON = useMemo(() => buildSearchQueryJSON(route.params.q, policyIDs), [route.params]);


Suggested change

const queryJSON = useMemo(() => buildSearchQueryJSON(route.params.q, policyIDs), [route.params]);

const queryJSON = useMemo(() => buildSearchQueryJSON(route.params.q, policyIDs), [route.params, policyIDs]);

rayane-djouah · 2024-07-25T15:32:03Z

src/libs/SearchUtils.ts

-};
-
-function buildJSONQuery(query: string) {
+function buildSearchQueryJSON(query: SearchQueryString, policyID?: string) {


Suggested change

function buildSearchQueryJSON(query: SearchQueryString, policyID?: string) {

function buildSearchQueryJSON(query: SearchQueryString, policyIDs?: string) {

rayane-djouah · 2024-07-25T15:32:28Z

src/libs/SearchUtils.ts

+        // Temporary solution until we move policyID filter into the AST - then remove this line and keep only query
+        const policyIDPart = policyID ?? '';


Suggested change

// Temporary solution until we move policyID filter into the AST - then remove this line and keep only query

const policyIDPart = policyID ?? '';

// Temporary solution until we move policyIDs filter into the AST - then remove this line and keep only query

const policyIDsPart = policyIDs ?? '';

rayane-djouah · 2024-07-25T15:32:41Z

src/libs/SearchUtils.ts

+
+        // Temporary solution until we move policyID filter into the AST - then remove this line and keep only query
+        const policyIDPart = policyID ?? '';
+        result.hash = getQueryHashFromString(query + policyIDPart);


Suggested change

result.hash = getQueryHashFromString(query + policyIDPart);

result.hash = getQueryHashFromString(query + policyIDsPart);

rayane-djouah · 2024-07-25T15:35:00Z

We still need to address these comments: #45617 (comment) #45617 (comment)

rayane-djouah · 2024-07-25T15:41:59Z

Bug 1 and 2 are no longer reproducible, great work!

Kicu · 2024-07-25T15:47:18Z

@luacmartins do you mind sharing here that we decided to straighten out policyIDs in a separate PR?
Also the future name to be used will be policyID that is why I named it like that right now.

luacmartins · 2024-07-25T15:48:15Z

@rayane-djouah #45617 (comment) is NAB, we'll do that in a follow up since it can introduce several regressions and we want this PR merged soon.

rayane-djouah · 2024-07-25T15:50:06Z

Okay, I will retest on all platforms & upload videos shortly

luacmartins · 2024-07-25T15:51:39Z

#45617 (comment) this one doesn't seem to be a blocker either. Offset is working fine for now. We'll need to make changes to the parser in a follow up, so this will be addressed then.

rayane-djouah

LGTM and test well! Thanks all!
@luacmartins all yours!

OSBotify · 2024-07-25T17:52:00Z

✋ 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-07-26T05:36:56Z

🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.13-0 🚀

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

OSBotify · 2024-07-29T18:39:41Z

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

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

adamgrzybowski force-pushed the adamgrzybowski/use-new-query-syntax branch from 4925690 to cb9d619 Compare July 18, 2024 16:19

adamgrzybowski mentioned this pull request Jul 18, 2024

[HOLD for payment 2024-08-05] [Search v2.1] [App] Use new query syntax #45027

Closed

1 task

luacmartins self-requested a review July 18, 2024 16:22

luacmartins reviewed Jul 18, 2024

View reviewed changes

src/CONST.ts Outdated Show resolved Hide resolved

src/CONST.ts Outdated Show resolved Hide resolved

src/ROUTES.ts Outdated Show resolved Hide resolved

adamgrzybowski added 2 commits July 19, 2024 13:29

use new query syntax

25acc96

Merge branch 'main' into adamgrzybowski/use-new-query-syntax

fe0ec54

adamgrzybowski force-pushed the adamgrzybowski/use-new-query-syntax branch from cb9d619 to fe0ec54 Compare July 19, 2024 12:11

adamgrzybowski added 3 commits July 19, 2024 14:34

fix problems after merge

6f9ba6d

Merge branch 'main' into adamgrzybowski/use-new-query-syntax

0a8edf0

remove default query

9eceb03

adamgrzybowski marked this pull request as ready for review July 19, 2024 14:51

adamgrzybowski requested a review from a team as a code owner July 19, 2024 14:51

melvin-bot bot requested review from situchan and removed request for a team July 19, 2024 14:51

Kicu reviewed Jul 22, 2024

View reviewed changes

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

improve handling undefined queryJSON

1c7e908

adamgrzybowski commented Jul 22, 2024

View reviewed changes

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

WojtekBoman reviewed Jul 22, 2024

View reviewed changes

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

WojtekBoman reviewed Jul 22, 2024

View reviewed changes

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

fix imports for SearchStatusMenu

0987879

luacmartins requested changes Jul 22, 2024

View reviewed changes

luacmartins reviewed Jul 22, 2024

View reviewed changes

adamgrzybowski added 3 commits July 23, 2024 12:34

add review suggestions

a4d185a

remove unecessary comment

ddd1b91

remove cq and add isCustomQuery

fb83ab1

Kicu mentioned this pull request Jul 23, 2024

[Search v2] Add displaying advanced filter values and type/status #46022

Merged

48 tasks

luacmartins reviewed Jul 23, 2024

View reviewed changes

Merge branch 'main' into adamgrzybowski/use-new-query-syntax

f758cc4

Add fixes after review

84ee1f9

Fix search hash not reacting to policyIDs change

4741f7a

luacmartins previously approved these changes Jul 25, 2024

View reviewed changes

rayane-djouah suggested changes Jul 25, 2024

View reviewed changes

Fix lint

1fc7be1

Kicu dismissed luacmartins’s stale review via 1fc7be1 July 25, 2024 15:43

luacmartins approved these changes Jul 25, 2024

View reviewed changes

rayane-djouah approved these changes Jul 25, 2024

View reviewed changes

melvin-bot bot requested a review from luacmartins July 25, 2024 16:22

luacmartins approved these changes Jul 25, 2024

View reviewed changes

luacmartins merged commit 5df5630 into Expensify:main Jul 25, 2024
15 of 17 checks passed

github-actions bot mentioned this pull request Jul 26, 2024

Deploy Checklist: New Expensify 2024-07-26 #46264

Closed

48 tasks

Kicu mentioned this pull request Jul 31, 2024

Use new Search query syntax when calling api #46481

Merged

48 tasks

	API.read(READ_COMMANDS.SEARCH, queryJSON, {optimisticData, finallyData});
	API.read(READ_COMMANDS.SEARCH, {hash, jsonQuery: JSON.stringify(jsonQuery)}, {optimisticData, finallyData});

	function searchV2(queryString: SearchQueryString) {
	function searchV2(jsonQuery: SearchQueryJSON) {

	const queryJSON = useMemo(() => buildSearchQueryJSON(route.params.q, policyIDs), [route.params]);
	const queryJSON = useMemo(() => buildSearchQueryJSON(route.params.q, policyIDs), [route.params, policyIDs]);

	function buildSearchQueryJSON(query: SearchQueryString, policyID?: string) {
	function buildSearchQueryJSON(query: SearchQueryString, policyIDs?: string) {

		// Temporary solution until we move policyID filter into the AST - then remove this line and keep only query
		const policyIDPart = policyID ?? '';

	result.hash = getQueryHashFromString(query + policyIDPart);
	result.hash = getQueryHashFromString(query + policyIDsPart);

[Search v2] [App] Use new query syntax #45617

[Search v2] [App] Use new query syntax #45617

Conversation

adamgrzybowski commented Jul 17, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

luacmartins left a comment

Choose a reason for hiding this comment

melvin-bot bot commented Jul 19, 2024

adamgrzybowski commented Jul 19, 2024

Kicu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

adamgrzybowski commented Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayane-djouah commented Jul 25, 2024

Kicu commented Jul 25, 2024 • edited Loading

Kicu commented Jul 25, 2024

luacmartins commented Jul 25, 2024

Kicu commented Jul 25, 2024

rayane-djouah commented Jul 25, 2024

rayane-djouah commented Jul 25, 2024

luacmartins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayane-djouah commented Jul 25, 2024

rayane-djouah commented Jul 25, 2024

Kicu commented Jul 25, 2024

luacmartins commented Jul 25, 2024

rayane-djouah commented Jul 25, 2024

luacmartins commented Jul 25, 2024

rayane-djouah left a comment

Choose a reason for hiding this comment

OSBotify commented Jul 25, 2024

OSBotify commented Jul 26, 2024

OSBotify commented Jul 29, 2024

adamgrzybowski commented Jul 17, 2024 •

edited

Loading

luacmartins Jul 24, 2024 •

edited

Loading

adamgrzybowski commented Jul 23, 2024 •

edited

Loading

luacmartins Jul 23, 2024 •

edited

Loading

Kicu commented Jul 25, 2024 •

edited

Loading