Conversation
…earching from searchRouter
|
FYI I'll prepare videos as first thing in the morning |
|
Actually managed to do this today @luacmartins 😄 Maybe we'll manage to merge it today, if @rayane-djouah is still up. |
|
Is the PR ready for review? |
|
@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] |
Kicu
left a comment
There was a problem hiding this comment.
Found some small tweaks, please take a look
| if (currentSearchOptions.userToInvite) { | ||
| reports.push(currentSearchOptions.userToInvite); | ||
| if (debouncedInputValue === '') { | ||
| return searchOptions.recentReports.slice(0, 10); |
There was a problem hiding this comment.
I'm curious if that doesn't change previous logic.
Originally we had debouncedInputValue === '' ? searchOptions - so it meant that if the input is empty, we push in the searchOptions (so I guess contextual search?).
But now we only show recent reports? Is that the change we want?
There was a problem hiding this comment.
Contextual search is added independently earlier in the code. This block of code was only responsible for adding recent chats section(which consisted of recentReports, personalDetails and userToInvite). Right now in case when text input is empty, we are limiting the recent chats section to only recentReports.
src/libs/SearchUtils.ts
Outdated
| orderedQuery += `type ${query.type} `; | ||
| orderedQuery += ` status ${query.status}`; | ||
| orderedQuery += ` sortBy ${query.sortBy} `; | ||
| orderedQuery += ` sortOrder ${query.sortOrder} `; |
There was a problem hiding this comment.
please use consts here, eg. SYNTAX_ROOT_KEYS.TYPE etc
src/libs/SearchUtils.ts
Outdated
| function buildSearchQueryJSON(query: SearchQueryString) { | ||
| try { | ||
| const result = searchParser.parse(query) as SearchQueryJSON; | ||
| // console.log('%%%%%\n', 'result', result); |
There was a problem hiding this comment.
Haste makes waste 😄
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-18.at.7.06.01.PM.movAndroid: mWeb ChromeScreen.Recording.2024-10-18.at.7.08.00.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-18.at.19.02.57.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-10-18.at.19.04.54.mp4 |
src/libs/SearchUtils.ts
Outdated
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_BY}: ${query.sortBy} `; | ||
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_ORDER}: ${query.sortOrder} `; |
There was a problem hiding this comment.
Shouldn't we exclude contextual filters (sortBy, sortOrder, policyID) when we calculate the query hash?
EDIT: I think that including them is necessary here, but I suggested a solution for related issues here: #50921 (comment)
There was a problem hiding this comment.
I think this will cause duplicate recent searches options
There was a problem hiding this comment.
This is reproducible also on main. I think we can fix it in a separate PR
There was a problem hiding this comment.
I think a potential solution is to create two hashes for the query: one that includes sortBy, sortOrder, and policyID, and one that does not.
There was a problem hiding this comment.
Hmm I think this should be fixed on BE, because recentSearches are generated on BE. In case when we are searching for some query twice, then it should be renewed in history and not duplicated.
There was a problem hiding this comment.
I also gave one argument for having sortOrder etc. here #50921 (comment).
There was a problem hiding this comment.
@SzymczakJ The duplication of the query isn't due to it being searched twice, but because the queries differ in sortBy, sortOrder, or policyID. For instance, in this case: #51044 (comment), there are two queries: one with sortBy and sortOrder, and another without these filters, resulting in different hashes. I agree that we should always include these filters when sending the query to the backend and when calculating the hash. However, we need to filter out duplicate queries in saved searches and recent searches that differ only by sortBy, sortOrder, or policyID. This can be achieved by computing a secondary hash that excludes these filters, and using this secondary hash to determine whether to display or exclude the query in saved and recent searches and whether to highlight the saved search item.
There was a problem hiding this comment.
We have a separate issue for this, so let's treat it as NAB for this PR and keep the discussion in that issue.
| if (query.policyID) { | ||
| orderedQuery += `${CONST.SEARCH.SYNTAX_ROOT_KEYS.POLICY_ID} ${query.policyID} `; | ||
| } |
| filtersString.push(`${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_BY}:${CONST.SEARCH.TABLE_COLUMNS.DATE}`); | ||
| filtersString.push(`${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_ORDER}:${CONST.SEARCH.SORT_ORDER.DESC}`); |
There was a problem hiding this comment.
This will cause the saved query name to include sortBy and sortOrder, and the highlight on the saved search item will be lost when we change the sort order.
Screen.Recording.2024-10-18.at.11.06.40.AM.mov
There was a problem hiding this comment.
The highlight on the saved search item will be lost when we change the sort order bug is reproducible on main and can be fixed in a separate PR. But the saved query name include sortBy and sortOrder bug is specific to this PR
There was a problem hiding this comment.
Do you know why the saved query name includes sortBy and sortOrder but not policyID?
There was a problem hiding this comment.
highlight on the saved search item will be lost when we change the sort order
I tend to lean toward idea that this is not a bug. For me searches differing only with sortBy or sortOrder are two different queries. Reason:
- There might be a user which has about 1000 expenses with category 'Advertising'
- When he queries for category 'Advertising' he will get a paginated result with only 50 of newest expenses
- When we will change sortOrder then he will get 50 oldest expenses. These two queries are giving us different responses
There was a problem hiding this comment.
Good point. However, we are not including sortBy, sortOrder, and policyID in the displayed query (in the header input and recent searches) because we want users to rely on the contextual filters rather than manually typing these parameters. Therefore, it doesn't make sense to include them in the saved search item name.
Also, please see this comment: #50921 (comment)
There was a problem hiding this comment.
I agree that those are different queries. We should treat these params the same way we do with status, since to @rayane-djouah's point, we use the contextual filters to change these values instead of manually typing them in.
src/libs/SearchUtils.ts
Outdated
| function getQueryHash(query: SearchQueryJSON): number { | ||
| let orderedQuery = ''; | ||
| if (query.policyID) { | ||
| orderedQuery += `${CONST.SEARCH.SYNTAX_ROOT_KEYS.POLICY_ID} ${query.policyID} `; |
There was a problem hiding this comment.
| orderedQuery += `${CONST.SEARCH.SYNTAX_ROOT_KEYS.POLICY_ID} ${query.policyID} `; | |
| orderedQuery += `${CONST.SEARCH.SYNTAX_ROOT_KEYS.POLICY_ID}:${query.policyID}`; |
src/libs/SearchUtils.ts
Outdated
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.TYPE}: ${query.type} `; | ||
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.STATUS}: ${query.status}`; | ||
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_BY}: ${query.sortBy} `; | ||
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_ORDER}: ${query.sortOrder} `; |
There was a problem hiding this comment.
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.TYPE}: ${query.type} `; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.STATUS}: ${query.status}`; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_BY}: ${query.sortBy} `; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_ORDER}: ${query.sortOrder} `; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.TYPE}:${query.type}`; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.STATUS}:${query.status}`; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_BY}:${query.sortBy}`; | |
| orderedQuery += ` ${CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_ORDER}:${query.sortOrder}`; |
src/libs/SearchUtils.ts
Outdated
| orderedQuery += ` ${key}:`; | ||
| filterValues | ||
| ?.sort((queryFilter1, queryFilter2) => { | ||
| if (queryFilter1.value > queryFilter2.value) { | ||
| return 1; | ||
| } | ||
| return -1; | ||
| }) | ||
| ?.forEach((queryFilter) => { | ||
| orderedQuery += ` ${queryFilter.operator} ${queryFilter.value}`; | ||
| }); |
There was a problem hiding this comment.
| orderedQuery += ` ${key}:`; | |
| filterValues | |
| ?.sort((queryFilter1, queryFilter2) => { | |
| if (queryFilter1.value > queryFilter2.value) { | |
| return 1; | |
| } | |
| return -1; | |
| }) | |
| ?.forEach((queryFilter) => { | |
| orderedQuery += ` ${queryFilter.operator} ${queryFilter.value}`; | |
| }); | |
| const sortedFilterValues = filterValues | |
| ?.sort((queryFilter1, queryFilter2) => { | |
| if (queryFilter1.value > queryFilter2.value) { | |
| return 1; | |
| } | |
| return -1; | |
| }); | |
| orderedQuery += ` ${SearchUtils.buildFilterString(key, sortedFilterValues)}`; |
src/libs/SearchUtils.ts
Outdated
| } | ||
| return -1; | ||
| }); | ||
| orderedQuery += `${buildFilterString(key, sortedFilterValues ?? [])}`; |
There was a problem hiding this comment.
NAB
| orderedQuery += `${buildFilterString(key, sortedFilterValues ?? [])}`; | |
| orderedQuery += ` ${buildFilterString(key, sortedFilterValues ?? [])}`; |
|
@luacmartins What do you think about #50921 (comment) and #50921 (comment)? |
|
I left comments on both of those threads. I think we can handle both of those cases as part of #51044 since they'd likely have the same or similar solutions. |
luacmartins
left a comment
There was a problem hiding this comment.
We have a few pending comments to resolve
|
Just resolved them @luacmartins |
rayane-d
left a comment
There was a problem hiding this comment.
LGTM and tests well. @luacmartins all yours!
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.0.51-1 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.51-4 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.51-4 🚀
|
|
|
||
| function getQueryHashFromString(query: SearchQueryString): number { | ||
| return UserUtils.hashText(query, 2 ** 32); | ||
| function getQueryHash(query: SearchQueryJSON): number { |
There was a problem hiding this comment.
We're missing the group_by key here, causing this issue #62222



Details
This PR fixes following issues:
Fixed Issues
$ #50250
$ #50977
$ #50993
$ #50998
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
iosweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov