diff --git a/src/components/Search/SearchRouter/SearchRouter.tsx b/src/components/Search/SearchRouter/SearchRouter.tsx index 93d941907e8b..d2678c5a21ba 100644 --- a/src/components/Search/SearchRouter/SearchRouter.tsx +++ b/src/components/Search/SearchRouter/SearchRouter.tsx @@ -40,7 +40,8 @@ import {getReportAction} from '@libs/ReportActionsUtils'; import {isHiddenForCurrentUser, isOneOnOneChat} from '@libs/ReportUtils'; import type {OptionData} from '@libs/ReportUtils'; import {getAutocompleteQueryWithComma, getTrimmedUserSearchQueryPreservingComma} from '@libs/SearchAutocompleteUtils'; -import {buildSearchQueryJSON, buildUserReadableQueryString, getQueryWithUpdatedValues, sanitizeSearchValue} from '@libs/SearchQueryUtils'; +import {searchKeyToSavedSearchID} from '@libs/SearchKeyUtils'; +import {buildSearchQueryJSON, buildUserReadableQueryString, getQueryWithPreservedSort, getQueryWithUpdatedValues, sanitizeSearchValue} from '@libs/SearchQueryUtils'; import StringUtils from '@libs/StringUtils'; import { beginSearchRouterQuerySession, @@ -139,13 +140,14 @@ function SearchRouter({onRouterClose, shouldHideInputCaret, isSearchRouterDispla const reportAttributes = useReportAttributes(); // Seed the input on open. When the SearchRouter is opened from the search button on the search page - // and `shouldShowSearchQuery` is true, we build a user-readable query string from the current search - // query (showing names instead of IDs) along with the substitutions map needed to map those names + // and `shouldShowSearchQuery` is true or a saved search is active, we build a user-readable query + // from the current search (showing names instead of IDs) along with the substitutions map needed to map those names // back to IDs when submitting. Otherwise we fall back to the explicit pending query (e.g. from // ExpenseReportSearchHandler). Computed once via a lazy initializer so the query string and its // substitutions stay consistent. const [[initialQuery, initialSubstitutions]] = useState<[string, SubstitutionMap]>(() => { - if (!currentSearchQueryJSON || !isFromSearchPageSearchButton || !searchContext?.shouldShowSearchQuery) { + const isSavedSearch = !!searchKeyToSavedSearchID(currentSearchKey); + if (!currentSearchQueryJSON || !isFromSearchPageSearchButton || (!searchContext?.shouldShowSearchQuery && !isSavedSearch)) { return [pendingInitialQuery, {}]; } @@ -414,11 +416,17 @@ function SearchRouter({onRouterClose, shouldHideInputCaret, isSearchRouterDispla const submitSearch = (queryString: SearchQueryString, shouldSkipAmountConversion = false) => { const queryWithSubstitutions = getQueryWithSubstitutions(queryString, autocompleteSubstitutions, currentUserAccountID, getPersonalDetailsByLogin()); - const updatedQuery = getQueryWithUpdatedValues(queryWithSubstitutions, shouldSkipAmountConversion, policies); + let updatedQuery = getQueryWithUpdatedValues(queryWithSubstitutions, shouldSkipAmountConversion, policies); if (!updatedQuery) { return; } + // Readable input omits sorting. Restore it only for edits to the active saved search, + // not when selecting a separate search suggestion with its own query. + if (isFromSearchPageSearchButton && !shouldSkipAmountConversion && searchKeyToSavedSearchID(currentSearchKey) && currentSearchQueryJSON) { + updatedQuery = getQueryWithPreservedSort(updatedQuery, currentSearchQueryJSON, queryWithSubstitutions); + } + // Reset the search query flag when performing a new search setShouldResetSearchQuery(false); diff --git a/src/libs/SearchParser/searchParser.js b/src/libs/SearchParser/searchParser.js index 86ebf04b210e..a7ce5608ccf0 100644 --- a/src/libs/SearchParser/searchParser.js +++ b/src/libs/SearchParser/searchParser.js @@ -543,6 +543,10 @@ function peg$parse(input, options) { if (op === "contains") { return buildFilter("eq", "keyword", text().trim()); } + // Ignore unsupported groupings before they override a valid grouping or affect sort and view defaults. + if (key === "groupBy" && !Object.hasOwn(GROUP_BY_DEFAULT_SORT, value)) { + return null; + } updateDefaultValues(key, value); return {...buildFilter(op, key, value), isDefault: true}; }; diff --git a/src/libs/SearchParser/searchParser.peggy b/src/libs/SearchParser/searchParser.peggy index 60555c6d8df2..c7f8ce4ce61f 100644 --- a/src/libs/SearchParser/searchParser.peggy +++ b/src/libs/SearchParser/searchParser.peggy @@ -220,6 +220,10 @@ defaultFilter if (op === "contains") { return buildFilter("eq", "keyword", text().trim()); } + // Ignore unsupported groupings before they override a valid grouping or affect sort and view defaults. + if (key === "groupBy" && !Object.hasOwn(GROUP_BY_DEFAULT_SORT, value)) { + return null; + } updateDefaultValues(key, value); return {...buildFilter(op, key, value), isDefault: true}; } diff --git a/src/libs/SearchQueryUtils.ts b/src/libs/SearchQueryUtils.ts index 6cda42287bd8..130a03972326 100644 --- a/src/libs/SearchQueryUtils.ts +++ b/src/libs/SearchQueryUtils.ts @@ -3021,6 +3021,34 @@ function buildFilterQueryWithSortDefaults( return getQueryWithUpdatedValues(queryString, true, policies); } +/** + * Restores sorting omitted from readable search input unless the user specifies sorting + * or changes the search type, grouping, or view in a way that requires new defaults. + * rawQuery must be the submitted input before normalization adds default sort fields. + */ +function getQueryWithPreservedSort(query: SearchQueryString, previousQuery: Readonly, rawQuery: SearchQueryString): SearchQueryString { + // Clearing the input starts a fresh search rather than editing the saved filters. + if (!rawQuery.trim()) { + return query; + } + + const queryJSON = buildSearchQueryJSON(query, rawQuery); + if (!queryJSON || queryJSON.type !== previousQuery.type) { + return query; + } + + const hasExplicitSort = queryJSON.rawFilterList?.some((filter) => filter.key === CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_BY || filter.key === CONST.SEARCH.SYNTAX_ROOT_KEYS.SORT_ORDER); + if ( + hasExplicitSort || + shouldResetSort({newGroupBy: queryJSON.groupBy, oldGroupBy: previousQuery.groupBy}) || + shouldResetSortForViewChange({newView: queryJSON.view, oldView: previousQuery.view, groupBy: queryJSON.groupBy}) + ) { + return query; + } + + return buildSearchQueryString({...queryJSON, sortBy: previousQuery.sortBy, sortOrder: previousQuery.sortOrder}); +} + /** * Builds an optimistic Snapshot update to ensure offline data for Tasks and Chat messages appears in Search. */ @@ -3256,6 +3284,7 @@ export { sanitizeSearchValue, getKeywordQueryForSearchInput, getQueryWithUpdatedValues, + getQueryWithPreservedSort, getKeywordQueryWithCurrentSearchContext, getCurrentSearchQueryJSON, getSearchRootParamsFromRootState, diff --git a/tests/ui/SearchRouterSavedSearchSortTest.tsx b/tests/ui/SearchRouterSavedSearchSortTest.tsx new file mode 100644 index 000000000000..24470a0e65a7 --- /dev/null +++ b/tests/ui/SearchRouterSavedSearchSortTest.tsx @@ -0,0 +1,210 @@ +/** Verifies saved sorting survives the router's readable input and submission round-trip. */ +import {act, fireEvent, render, screen} from '@testing-library/react-native'; + +import {CurrentUserPersonalDetailsProvider} from '@components/CurrentUserPersonalDetailsProvider'; +import {LocaleContextProvider} from '@components/LocaleContextProvider'; +import OnyxListItemProvider from '@components/OnyxListItemProvider'; +import type {SearchQueryItem} from '@components/Search/SearchList/ListItem/SearchQueryListItem'; +import SearchRouter from '@components/Search/SearchRouter/SearchRouter'; +import type {SearchQueryJSON} from '@components/Search/types'; + +import Navigation from '@libs/Navigation/Navigation'; +import {savedSearchIDToSearchKey} from '@libs/SearchKeyUtils'; +import {buildSearchQueryJSON} from '@libs/SearchQueryUtils'; + +import ComposeProviders from '@src/components/ComposeProviders'; +import CONST from '@src/CONST'; +import ONYXKEYS from '@src/ONYXKEYS'; + +import type * as NativeNavigation from '@react-navigation/native'; + +import React from 'react'; +import Onyx from 'react-native-onyx'; + +import waitForBatchedUpdatesWithAct from '../utils/waitForBatchedUpdatesWithAct'; + +let mockCurrentQuery: Readonly | undefined; +let mockIsSavedSearch = true; +let mockIsFromSearchPage = true; +let mockOnListItemPress: ((item: SearchQueryItem) => void) | undefined; +const mockGetSearchKeyForQuery = jest.fn(() => undefined); + +jest.mock('@components/Search/SearchContext', () => ({ + ...jest.requireActual('@components/Search/SearchContext'), + useSearchQueryContext: () => ({ + currentSearchQueryJSON: mockCurrentQuery, + currentSearchHash: mockCurrentQuery?.hash, + currentSearchKey: mockIsSavedSearch ? savedSearchIDToSearchKey('saved-sort') : CONST.SEARCH.SEARCH_KEYS.EXPENSES, + }), + useSearchQueryActions: () => ({setShouldResetSearchQuery: jest.fn(), getSearchKeyForQuery: mockGetSearchKeyForQuery}), +})); + +jest.mock('@components/Search/SearchRouter/SearchRouterContext', () => ({ + peekPendingRouterState: () => ({query: '', isFromSearchPageSearchButton: mockIsFromSearchPage}), + clearPendingRouterState: jest.fn(), +})); + +jest.mock('@components/Search/DeferredSearchAutocompleteList', () => ({ + __esModule: true, + default: ({onListItemPress}: {onListItemPress: (item: SearchQueryItem) => void}) => { + mockOnListItemPress = onListItemPress; + return null; + }, +})); + +jest.mock('@src/libs/Log'); + +jest.mock('@src/libs/API', () => ({ + write: jest.fn(), + makeRequestWithSideEffects: jest.fn(), + read: jest.fn(), +})); + +jest.mock('@src/libs/Navigation/Navigation', () => ({ + navigate: jest.fn(), + dismissModalWithReport: jest.fn(), + getTopmostReportId: jest.fn(), + isNavigationReady: jest.fn(() => Promise.resolve()), + isDisplayedInModal: jest.fn(() => false), + getActiveRouteWithoutParams: jest.fn(() => ''), +})); + +jest.mock('@src/hooks/useRootNavigationState', () => ({ + __esModule: true, + default: () => ({ + contextualReportID: undefined, + isSearchRouterScreen: false, + }), +})); + +jest.mock('@react-navigation/native', () => { + const actualNav = jest.requireActual('@react-navigation/native'); + return { + ...actualNav, + useFocusEffect: jest.fn(), + useIsFocused: () => true, + useRoute: () => jest.fn(), + usePreventRemove: () => jest.fn(), + useNavigation: () => ({ + navigate: jest.fn(), + addListener: () => jest.fn(), + }), + createNavigationContainerRef: () => ({ + addListener: () => jest.fn(), + removeListener: () => jest.fn(), + isReady: () => jest.fn(), + getCurrentRoute: () => jest.fn(), + getState: () => jest.fn(), + getRootState: () => undefined, + }), + useNavigationState: () => ({ + routes: [], + }), + }; +}); + +jest.mock('@src/components/ConfirmedRoute.tsx'); + +async function renderSearchRouter() { + render( + + + , + ); + await waitForBatchedUpdatesWithAct(); +} + +function submitQuery(query: string, isSeparateSuggestion = false) { + fireEvent.changeText(screen.getByTestId('search-autocomplete-text-input'), query); + act(() => { + mockOnListItemPress?.({ + text: query, + searchQuery: query, + searchItemType: CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.SEARCH, + keyForList: isSeparateSuggestion ? 'separate-search' : CONST.SEARCH.SEARCH_ROUTER_ITEM_TYPE.FIND_ITEM, + }); + }); +} + +function getNavigatedQuery() { + expect(Navigation.navigate).toHaveBeenCalledTimes(1); + const route = jest.mocked(Navigation.navigate).mock.calls.at(0)?.at(0); + if (typeof route !== 'string') { + throw new Error('Expected a search route'); + } + const params = new URLSearchParams(route.slice(route.indexOf('?') + 1)); + return {query: buildSearchQueryJSON(params.get('q') ?? ''), params}; +} + +describe('SearchRouter saved search sorting', () => { + beforeAll(() => { + Onyx.init({keys: ONYXKEYS}); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + mockIsSavedSearch = true; + mockIsFromSearchPage = true; + mockOnListItemPress = undefined; + mockCurrentQuery = buildSearchQueryJSON('type:expense from:123 sortBy:amount sortOrder:asc'); + await Onyx.merge(ONYXKEYS.SESSION, {accountID: 123, email: 'saved-search@example.com'}); + }); + + afterEach(async () => { + await act(async () => { + await Onyx.clear(); + }); + }); + + it.each(['asc', 'desc'])('retains Amount %s and From after unsupported grouping is appended', async (sortOrder) => { + // Given a saved search whose readable input hides its Amount sorting + mockCurrentQuery = buildSearchQueryJSON(`type:expense from:123 sortBy:amount sortOrder:${sortOrder}`); + await renderSearchRouter(); + const input = screen.getByTestId('search-autocomplete-text-input'); + const readableQuery = input.props.value as string; + expect(readableQuery).not.toMatch(/sort-?by|sort-?order/i); + expect(readableQuery).toContain('from:me'); + + // When the edited query is submitted through the router's Find item + submitQuery(`${readableQuery} group-by:reports`); + + // Then navigation retains the saved sort, resolves me to its account ID, and keeps the saved identity + const {query, params} = getNavigatedQuery(); + expect(query).toMatchObject({sortBy: 'amount', sortOrder}); + expect(query?.flatFilters).toEqual(mockCurrentQuery?.flatFilters); + expect(params.get('searchKey')).toBe(savedSearchIDToSearchKey('saved-sort')); + expect(params.get('rawQuery')).not.toMatch(/sort-?by|sort-?order/i); + }); + + it.each([ + ['supported grouping', 'type:expense from:me group-by:category', 'groupCategory', 'asc'], + ['explicit sorting', 'type:expense from:me sort-by:date sort-order:asc', 'date', 'asc'], + ])('respects %s when submitting a saved search edit', async (_scenario, input, sortBy, sortOrder) => { + // Given a saved Amount sort that must not override the edited query's sorting rules + await renderSearchRouter(); + + // When the user changes grouping or explicitly specifies sorting + submitQuery(input); + + // Then the new query and search-key lookup both receive the resulting sort + const {query} = getNavigatedQuery(); + expect(query).toMatchObject({sortBy, sortOrder}); + expect(mockGetSearchKeyForQuery).toHaveBeenCalledWith(expect.objectContaining({sortBy, sortOrder})); + }); + + it.each(['unsaved search', 'global search', 'separate suggestion'])('does not inherit sorting for a %s', async (scenario) => { + // Given a search submission outside editing the active saved search + mockIsSavedSearch = scenario !== 'unsaved search'; + mockIsFromSearchPage = scenario !== 'global search'; + await renderSearchRouter(); + + // When a new query is submitted without sort fields + submitQuery('type:expense from:123', scenario === 'separate suggestion'); + + // Then the new search uses its default Date descending sort + expect(getNavigatedQuery().query).toMatchObject({sortBy: 'date', sortOrder: 'desc'}); + }); +}); diff --git a/tests/unit/Search/SearchQueryUtilsTest.ts b/tests/unit/Search/SearchQueryUtilsTest.ts index 5eadb963e422..1e29fdd056b9 100644 --- a/tests/unit/Search/SearchQueryUtilsTest.ts +++ b/tests/unit/Search/SearchQueryUtilsTest.ts @@ -39,6 +39,7 @@ import { buildQueryStringWithResetFilters, getQueryHashWithoutFilters, getQueryWithUpdatedValues, + getQueryWithPreservedSort, getRangeBoundariesFromFormValue, getRoutes, getSearchRootParamsFromRootState, @@ -516,12 +517,26 @@ describe('SearchQueryUtils', () => { expect(result).toEqual(`${defaultQuery} from:9876,87654 to:78901 amount:15000 hello test`); }); - test('returns query with updated groupBy', () => { - const userQuery = 'from:12345 groupBy:reports'; + test('ignores unsupported groupBy and preserves other filters', () => { + // Given a saved expense query with a From filter and an unsupported grouping + const userQuery = 'type:expense from:12345 group-by: reports'; + // When the router normalizes the submitted query const result = getQueryWithUpdatedValues(userQuery); - expect(result).toEqual(`${defaultQuery} groupBy:reports from:12345`); + // Then the unsupported grouping is omitted and the From filter remains + expect(result).toEqual(`${defaultQuery} from:12345`); + }); + + test('keeps a supported groupBy when followed by an unsupported groupBy', () => { + // Given a query with a valid grouping followed by an unsupported grouping + const userQuery = 'type:expense from:12345 group-by:merchant group-by:reports'; + + // When the router normalizes the submitted query + const result = getQueryWithUpdatedValues(userQuery); + + // Then the valid grouping and From filter remain applied + expect(result).toEqual('type:expense sortBy:groupMerchant sortOrder:asc groupBy:merchant from:12345'); }); test('returns query with updated view', () => { @@ -3041,7 +3056,7 @@ describe('SearchQueryUtils', () => { expect(isDefaultExpenses('type:expense footerTotal:billable footerCurrency:EUR')).toBe(true); expect(isDefaultExpenses('type:expense footerTotal:billable category:Travel')).toBe(false); expect(isDefaultExpenseReports('type:expense-report footerCount:expenses')).toBe(true); - expect(isDefaultExpenseReports('type:expense-report groupBy:report')).toBe(false); + expect(isDefaultExpenseReports('type:expense-report groupBy:merchant')).toBe(false); }); it('keeps every footer selection out of the recent and similar search hashes, since none of them changes a result', () => { @@ -3858,6 +3873,82 @@ describe('SearchQueryUtils', () => { }); }); + describe('getQueryWithPreservedSort', () => { + test.each([ + ['amount ascending', 'sortBy:amount sortOrder:asc', 'type:expense from:123 merchant:Coffee', 'amount', 'asc'], + ['amount descending', 'sortBy:amount sortOrder:desc', 'type:expense from:123 merchant:Coffee', 'amount', 'desc'], + ['unsupported grouping ascending', 'sortBy:amount sortOrder:asc', 'type:expense from:123 group-by:reports', 'amount', 'asc'], + ['unsupported grouping descending', 'sortBy:amount sortOrder:desc', 'type:expense from:123 group-by:reports', 'amount', 'desc'], + ['default sorting', 'sortBy:date sortOrder:desc', 'type:expense from:123 merchant:Coffee', 'date', 'desc'], + ['explicit sort column', 'sortBy:amount sortOrder:asc', 'type:expense from:123 sort-by:date', 'date', 'desc'], + ['explicit sort direction', 'sortBy:amount sortOrder:desc', 'type:expense from:123 sort-order:asc', 'date', 'asc'], + ['both explicit sort fields', 'sortBy:amount sortOrder:desc', 'type:expense from:123 sort-by:date sort-order:asc', 'date', 'asc'], + ['changed search type', 'sortBy:amount sortOrder:asc', 'type:expense-report from:123', 'date', 'desc'], + ['added grouping', 'sortBy:amount sortOrder:desc', 'type:expense from:123 group-by:category', 'groupCategory', 'asc'], + ['removed grouping', 'groupBy:category sortBy:groupCategory sortOrder:asc', 'type:expense from:123', 'date', 'desc'], + ['changed grouping', 'groupBy:category sortBy:groupCategory sortOrder:asc', 'type:expense from:123 group-by:month', 'groupmonth', 'desc'], + ['line view', 'groupBy:month view:table sortBy:groupmonth sortOrder:desc', 'type:expense from:123 group-by:month view:line', 'groupmonth', 'asc'], + ['table view', 'groupBy:month view:line sortBy:groupmonth sortOrder:asc', 'type:expense from:123 group-by:month view:table', 'groupmonth', 'desc'], + ['bar view', 'groupBy:month view:table sortBy:amount sortOrder:asc', 'type:expense from:123 group-by:month view:bar', 'amount', 'asc'], + ['unchanged grouping', 'groupBy:category sortBy:amount sortOrder:desc', 'type:expense from:123 group-by:category merchant:Coffee', 'amount', 'desc'], + ])('handles %s when editing a saved query', (_scenario, previousSort, rawQuery, sortBy, sortOrder) => { + // Given a saved query and edited input whose normalization supplies default sort fields + const previousQuery = buildSearchQueryJSON(`type:expense from:123 ${previousSort}`); + const normalizedQuery = getQueryWithUpdatedValues(rawQuery); + if (!previousQuery || !normalizedQuery) { + throw new Error('Invalid test query'); + } + + // When the saved sorting is considered after normalizing the edited input + const result = getQueryWithPreservedSort(normalizedQuery, previousQuery, rawQuery); + const parsedResult = buildSearchQueryJSON(result); + + // Then sorting follows explicit input and grouping/view rules without losing the From filter + expect(parsedResult).toMatchObject({sortBy, sortOrder}); + expect(parsedResult?.flatFilters).toEqual(buildSearchQueryJSON(normalizedQuery)?.flatFilters); + expect(parsedResult?.flatFilters).toEqual(expect.arrayContaining([expect.objectContaining({key: CONST.SEARCH.SYNTAX_FILTER_KEYS.FROM})])); + }); + + test('leaves the submitted query unchanged when parsing fails', () => { + // Given a valid saved query and a parser failure for the replacement query + const previousQuery = buildSearchQueryJSON('type:expense sortBy:amount sortOrder:asc'); + if (!previousQuery) { + throw new Error('Invalid test query'); + } + const parser = jest.requireMock<{parse: jest.Mock}>('@libs/SearchParser/searchParser'); + const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); + parser.parse.mockImplementationOnce(() => { + throw new Error('Unable to parse edited query'); + }); + + try { + // When sort preservation cannot interpret the replacement query + const query = 'type:expense merchant:unparseable-sort-preservation'; + const result = getQueryWithPreservedSort(query, previousQuery, query); + + // Then it leaves error handling to the caller without replacing the submitted query + expect(result).toBe(query); + expect(consoleError).toHaveBeenCalled(); + } finally { + consoleError.mockRestore(); + } + }); + + test.each(['', ' '])('uses defaults when the input is cleared to %j', (rawQuery) => { + // Given a saved Amount sort and an empty replacement search + const previousQuery = buildSearchQueryJSON('type:expense sortBy:amount sortOrder:asc'); + if (!previousQuery) { + throw new Error('Invalid test query'); + } + + // When the input is cleared instead of editing its filters + const result = getQueryWithPreservedSort(defaultQuery, previousQuery, rawQuery); + + // Then the fresh search keeps its defaults rather than inheriting saved sorting + expect(result).toBe(defaultQuery); + }); + }); + describe('buildFilterQueryWithSortDefaults', () => { describe('Merchant predicates', () => { function updateQuery(input: string, updates: Partial) {