Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/components/Search/SearchMultipleSelectionPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type SearchMultipleSelectionPickerProps = {
};

function SearchMultipleSelectionPicker({items, initiallySelectedItems, pickerTitle, onSaveSelection, shouldShowTextInput = true}: SearchMultipleSelectionPickerProps) {
const {translate} = useLocalize();
const {translate, localeCompare} = useLocalize();

const [searchTerm, debouncedSearchTerm, setSearchTerm] = useDebouncedState('');
const [selectedItems, setSelectedItems] = useState<SearchMultipleSelectionPickerItem[]>(initiallySelectedItems ?? []);
Expand All @@ -35,7 +35,7 @@ function SearchMultipleSelectionPicker({items, initiallySelectedItems, pickerTit
const {sections, noResultsFound} = useMemo(() => {
const selectedItemsSection = selectedItems
.filter((item) => item?.name.toLowerCase().includes(debouncedSearchTerm?.toLowerCase()))
.sort((a, b) => sortOptionsWithEmptyValue(a.value.toString(), b.value.toString()))
.sort((a, b) => sortOptionsWithEmptyValue(a.value.toString(), b.value.toString(), localeCompare))
.map((item) => ({
text: item.name,
keyForList: item.name,
Expand All @@ -47,7 +47,7 @@ function SearchMultipleSelectionPicker({items, initiallySelectedItems, pickerTit
(item) =>
!selectedItems.some((selectedItem) => selectedItem.value.toString() === item.value.toString()) && item?.name?.toLowerCase().includes(debouncedSearchTerm?.toLowerCase()),
)
.sort((a, b) => sortOptionsWithEmptyValue(a.value.toString(), b.value.toString()))
.sort((a, b) => sortOptionsWithEmptyValue(a.value.toString(), b.value.toString(), localeCompare))
.map((item) => ({
text: item.name,
keyForList: item.name,
Expand All @@ -72,7 +72,7 @@ function SearchMultipleSelectionPicker({items, initiallySelectedItems, pickerTit
],
noResultsFound: isEmpty,
};
}, [selectedItems, items, pickerTitle, debouncedSearchTerm]);
}, [selectedItems, items, pickerTitle, debouncedSearchTerm, localeCompare]);

const onSelectItem = useCallback(
(item: Partial<OptionData & SearchMultipleSelectionPickerItem>) => {
Expand Down
14 changes: 11 additions & 3 deletions src/libs/SearchQueryUtils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import cloneDeep from 'lodash/cloneDeep';
import type {OnyxCollection} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import type {LocaleContextProps} from '@components/LocaleContextProvider';
import type {
ASTNode,
QueryFilter,
Expand All @@ -26,7 +27,6 @@ import type {SearchDataTypes} from '@src/types/onyx/SearchResults';
import {getCardFeedsForDisplay} from './CardFeedUtils';
import {getCardDescription} from './CardUtils';
import {convertToBackendAmount, convertToFrontendAmountAsInteger} from './CurrencyUtils';
import localeCompare from './LocaleCompare';
import Log from './Log';
import {validateAmount} from './MoneyRequestUtils';
import navigationRef from './Navigation/navigationRef';
Expand Down Expand Up @@ -273,6 +273,14 @@ function getUpdatedFilterValue(filterName: ValueOf<typeof CONST.SEARCH.SYNTAX_FI
return filterValue;
}

/**
* @private
* This is a custom collator only for getQueryHashes function.
* The reason for this is that the computation of hashes should not depend on the locale.
* This is used to ensure that hashes stay consistent.
Comment thread
shubham1206agra marked this conversation as resolved.
*/
const customCollator = new Intl.Collator('en', {usage: 'sort', sensitivity: 'variant', numeric: true, caseFirst: 'upper'});

/**
* @private
* Computes and returns a numerical hash for a given queryJSON.
Expand All @@ -287,7 +295,7 @@ function getQueryHashes(query: SearchQueryJSON): {primaryHash: number; recentSea
query.flatFilters
.map((filter) => {
const filters = cloneDeep(filter.filters);
filters.sort((a, b) => localeCompare(a.value.toString(), b.value.toString()));
filters.sort((a, b) => customCollator.compare(a.value.toString(), b.value.toString()));
return buildFilterValuesString(filter.key, filters);
})
.sort()
Expand Down Expand Up @@ -883,7 +891,7 @@ function isDefaultExpensesQuery(queryJSON: SearchQueryJSON) {
/**
* Always show `No category` and `No tag` as the first option
*/
const sortOptionsWithEmptyValue = (a: string, b: string) => {
const sortOptionsWithEmptyValue = (a: string, b: string, localeCompare: LocaleContextProps['localeCompare']) => {
if (a === CONST.SEARCH.CATEGORY_EMPTY_VALUE || a === CONST.SEARCH.TAG_EMPTY_VALUE) {
return -1;
}
Expand Down
4 changes: 2 additions & 2 deletions src/pages/Search/AdvancedSearchFilters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -436,15 +436,15 @@ function getFilterDisplayTitle(
if (nonDateFilterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.CATEGORY && filters[nonDateFilterKey]) {
const filterArray = filters[nonDateFilterKey] ?? [];
return filterArray
.sort(sortOptionsWithEmptyValue)
.sort((a, b) => sortOptionsWithEmptyValue(a, b, localeCompare))
.map((value) => (value === CONST.SEARCH.CATEGORY_EMPTY_VALUE ? translate('search.noCategory') : value))
.join(', ');
}

if (nonDateFilterKey === CONST.SEARCH.SYNTAX_FILTER_KEYS.TAG && filters[nonDateFilterKey]) {
const filterArray = filters[nonDateFilterKey] ?? [];
return filterArray
.sort(sortOptionsWithEmptyValue)
.sort((a, b) => sortOptionsWithEmptyValue(a, b, localeCompare))
.map((value) => (value === CONST.SEARCH.TAG_EMPTY_VALUE ? translate('search.noTag') : getCleanedTagName(value)))
.join(', ');
}
Expand Down
26 changes: 25 additions & 1 deletion tests/unit/Search/SearchQueryUtilsTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,17 @@
// we need "dirty" object key names in these tests
import {generatePolicyID} from '@libs/actions/Policy/Policy';
import CONST from '@src/CONST';
import {buildFilterFormValuesFromQuery, buildQueryStringFromFilterFormValues, buildSearchQueryJSON, getQueryWithUpdatedValues, shouldHighlight} from '@src/libs/SearchQueryUtils';
import {
buildFilterFormValuesFromQuery,
buildQueryStringFromFilterFormValues,
buildSearchQueryJSON,
getQueryWithUpdatedValues,
shouldHighlight,
sortOptionsWithEmptyValue,
} from '@src/libs/SearchQueryUtils';
import ONYXKEYS from '@src/ONYXKEYS';
import type {SearchAdvancedFiltersForm} from '@src/types/form';
import {localeCompare} from '../../utils/TestHelper';

const personalDetailsFakeData = {
'johndoe@example.com': {
Expand Down Expand Up @@ -268,4 +276,20 @@ describe('SearchQueryUtils', () => {
expect(shouldHighlight('Take a 2-minute tour', 'tour 2-minute')).toBe(false);
});
});

describe('sortOptionsWithEmptyValue', () => {
it('should prioritize empty values at the start', () => {
const options = ['B', 'A', CONST.SEARCH.CATEGORY_EMPTY_VALUE, 'C'];
const sortedOptions = options.sort((a, b) => sortOptionsWithEmptyValue(a, b, localeCompare));

expect(sortedOptions).toEqual([CONST.SEARCH.CATEGORY_EMPTY_VALUE, 'A', 'B', 'C']);
});

it('should sort non-empty values properly', () => {
const options = ['B', 'A', 'C'];
const sortedOptions = options.sort((a, b) => sortOptionsWithEmptyValue(a, b, localeCompare));

expect(sortedOptions).toEqual(['A', 'B', 'C']);
});
});
});
11 changes: 11 additions & 0 deletions tests/utils/TestHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,16 @@ async function navigateToSidebarOption(index: number): Promise<void> {
await waitForBatchedUpdatesWithAct();
}

/**
* @private
* This is a custom collator only for testing purposes.
*/
const customCollator = new Intl.Collator('en', {usage: 'sort', sensitivity: 'variant', numeric: true, caseFirst: 'upper'});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is duplicated, I have a couple of suggestions:

  1. Define it in CONST (which isn't great because @private designation won't be as effective
  2. Export it from src/libs/SearchQueryUtils.ts with an explicit comment saying it's only being exported so that tests can access it and it shouldn't be used otherwise
  3. Define it in its own file somewhere and then import it into this test and SearchQueryUtils.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these declaration different. As otherwise, I would have to break private assumption of the variable. And I don't want to do that as I don't trust any contributor of not mishandling the variable given I saw way too many people doing anti-patterns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, sounds good. I agree with that argument.


function localeCompare(a: string, b: string): number {
return customCollator.compare(a, b);
}

export type {MockFetch, FormData};
export {
assertFormDataMatchesObject,
Expand All @@ -363,4 +373,5 @@ export {
navigateToSidebarOption,
getOnyxData,
getNavigateToChatHintRegex,
localeCompare,
};