From 8a982420b808ee78f7b2ad0e02381faec63c759d Mon Sep 17 00:00:00 2001 From: dominictb Date: Fri, 12 Jul 2024 14:56:42 +0700 Subject: [PATCH 1/5] fix: use localeCompare to sort in mention list Signed-off-by: dominictb --- .../ReportActionCompose/SuggestionMention.tsx | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx index 86a05bad1994..d36dd5a5af21 100644 --- a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx +++ b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx @@ -14,6 +14,7 @@ import useCurrentReportID from '@hooks/useCurrentReportID'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; import useDebounce from '@hooks/useDebounce'; import useLocalize from '@hooks/useLocalize'; +import localeCompare from '@libs/LocaleCompare'; import * as LoginUtils from '@libs/LoginUtils'; import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils'; import getPolicyEmployeeAccountIDs from '@libs/PolicyEmployeeListUtils'; @@ -270,9 +271,29 @@ function SuggestionMention( } return true; + }) as Array; // at this point we are sure that the details are not null + + const sortedPersonalDetails = filteredPersonalDetails.sort((first, second) => { + // first, we should sort by weight + if (first.weight !== second.weight) { + return first.weight - second.weight; + } + + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + const firstDisplayName = ReportUtils.getDisplayNameForParticipant(first.accountID) || first?.login || ''; + + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + const secondDisplayName = ReportUtils.getDisplayNameForParticipant(second.accountID) || second?.login || ''; + + const displayNameLoginOrder = localeCompare(firstDisplayName, secondDisplayName); + if (displayNameLoginOrder !== 0) { + return displayNameLoginOrder; + } + + // Then fallback on accountID as the final sorting criteria. + return first.accountID - second.accountID; }); - const sortedPersonalDetails = lodashSortBy(filteredPersonalDetails, ['weight', 'displayName', 'login']); sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => { suggestions.push({ text: formatLoginPrivateDomain(PersonalDetailsUtils.getDisplayNameOrDefault(detail), detail?.login), From 97ed6e1131281a8530101128c1378fb3e26aeb4c Mon Sep 17 00:00:00 2001 From: dominictb Date: Wed, 17 Jul 2024 22:31:17 +0700 Subject: [PATCH 2/5] chore: refactor and add unit tests --- src/libs/compareUserInList.ts | 29 ++++++++++++++++ .../ReportActionCompose/SuggestionMention.tsx | 34 +++++++------------ tests/unit/compareUserInListTest.ts | 24 +++++++++++++ 3 files changed, 65 insertions(+), 22 deletions(-) create mode 100644 src/libs/compareUserInList.ts create mode 100644 tests/unit/compareUserInListTest.ts diff --git a/src/libs/compareUserInList.ts b/src/libs/compareUserInList.ts new file mode 100644 index 000000000000..4f3bfcef27f3 --- /dev/null +++ b/src/libs/compareUserInList.ts @@ -0,0 +1,29 @@ +import localeCompare from './LocaleCompare'; + +/** + * the comparison function used to determine which user will come first in the sorted list + * generally, smaller weight means will come first, and if the weight is the same, we'll sort based on displayName/login and accountID + */ + +type UserDetailsWithWeight = { + displayName: string; + weight: number; + accountID: number; +}; + +function compareUserInList(first: UserDetailsWithWeight, second: UserDetailsWithWeight) { + // first, we should sort by weight + if (first.weight !== second.weight) { + return first.weight - second.weight; + } + + const displayNameLoginOrder = localeCompare(first.displayName, second.displayName); + if (displayNameLoginOrder !== 0) { + return displayNameLoginOrder; + } + + // Then fallback on accountID as the final sorting criteria. + return first.accountID - second.accountID; +} + +export default compareUserInList; diff --git a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx index d36dd5a5af21..5601684b39b1 100644 --- a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx +++ b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx @@ -3,8 +3,8 @@ import lodashMapValues from 'lodash/mapValues'; import lodashSortBy from 'lodash/sortBy'; import type {ForwardedRef} from 'react'; import React, {forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState} from 'react'; -import {useOnyx} from 'react-native-onyx'; import type {OnyxCollection} from 'react-native-onyx'; +import {useOnyx} from 'react-native-onyx'; import * as Expensicons from '@components/Icon/Expensicons'; import type {Mention} from '@components/MentionSuggestions'; import MentionSuggestions from '@components/MentionSuggestions'; @@ -14,7 +14,7 @@ import useCurrentReportID from '@hooks/useCurrentReportID'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; import useDebounce from '@hooks/useDebounce'; import useLocalize from '@hooks/useLocalize'; -import localeCompare from '@libs/LocaleCompare'; +import compareUserInList from '@libs/compareUserInList'; import * as LoginUtils from '@libs/LoginUtils'; import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils'; import getPolicyEmployeeAccountIDs from '@libs/PolicyEmployeeListUtils'; @@ -273,26 +273,16 @@ function SuggestionMention( return true; }) as Array; // at this point we are sure that the details are not null - const sortedPersonalDetails = filteredPersonalDetails.sort((first, second) => { - // first, we should sort by weight - if (first.weight !== second.weight) { - return first.weight - second.weight; - } - - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - const firstDisplayName = ReportUtils.getDisplayNameForParticipant(first.accountID) || first?.login || ''; - - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - const secondDisplayName = ReportUtils.getDisplayNameForParticipant(second.accountID) || second?.login || ''; - - const displayNameLoginOrder = localeCompare(firstDisplayName, secondDisplayName); - if (displayNameLoginOrder !== 0) { - return displayNameLoginOrder; - } - - // Then fallback on accountID as the final sorting criteria. - return first.accountID - second.accountID; - }); + const sortedPersonalDetails = filteredPersonalDetails + .map((user) => ({ + originalDetail: user, + weight: user.weight, + accountID: user.accountID, + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + displayName: ReportUtils.getDisplayNameForParticipant(user.accountID) || user.login || '', + })) + .sort((first, second) => compareUserInList(first, second)) + .map((userAfterCompare) => userAfterCompare.originalDetail); sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => { suggestions.push({ diff --git a/tests/unit/compareUserInListTest.ts b/tests/unit/compareUserInListTest.ts new file mode 100644 index 000000000000..40ee003ad536 --- /dev/null +++ b/tests/unit/compareUserInListTest.ts @@ -0,0 +1,24 @@ +import compareUserInList from '@libs/compareUserInList'; + +describe('compareUserInList', () => { + it('should compare the weight if the weight is difference', () => { + const first = {displayName: 'John Doe', weight: 1, accountID: 1}; + const second = {displayName: 'Jane Doe', weight: 2, accountID: 2}; + expect(compareUserInList(first, second)).toBe(-1); + }); + + it('Should compare the displayName if the weight is the same', () => { + const first = {displayName: 'águero', weight: 2, accountID: 1}; + const second = {displayName: 'Bronn', weight: 2, accountID: 1}; + const third = {displayName: 'Carol', weight: 2, accountID: 1}; + expect(compareUserInList(first, second)).toBe(-1); + expect(compareUserInList(first, third)).toBe(-1); + expect(compareUserInList(second, third)).toBe(-1); + }); + + it('Should compare the accountID if both the weight and displayName are the same', () => { + const first = {displayName: 'águero', weight: 2, accountID: 1}; + const second = {displayName: 'aguero', weight: 2, accountID: 2}; + expect(compareUserInList(first, second)).toBe(-1); + }); +}); From 07ff80e585027226b9004828184d4310545ea6dd Mon Sep 17 00:00:00 2001 From: dominictb Date: Thu, 18 Jul 2024 10:07:24 +0700 Subject: [PATCH 3/5] fix: update implementation --- .../ReportActionCompose/SuggestionMention.tsx | 26 ++++++++++++------- tests/unit/compareUserInListTest.ts | 2 +- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx index 5601684b39b1..1bedaa16a3fe 100644 --- a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx +++ b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx @@ -273,16 +273,22 @@ function SuggestionMention( return true; }) as Array; // at this point we are sure that the details are not null - const sortedPersonalDetails = filteredPersonalDetails - .map((user) => ({ - originalDetail: user, - weight: user.weight, - accountID: user.accountID, - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - displayName: ReportUtils.getDisplayNameForParticipant(user.accountID) || user.login || '', - })) - .sort((first, second) => compareUserInList(first, second)) - .map((userAfterCompare) => userAfterCompare.originalDetail); + const sortedPersonalDetails = filteredPersonalDetails.sort((first, second) => + compareUserInList( + { + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + displayName: ReportUtils.getDisplayNameForParticipant(first.accountID) || first.login || '', + weight: first.weight, + accountID: first.accountID, + }, + { + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing + displayName: ReportUtils.getDisplayNameForParticipant(second.accountID) || second.login || '', + weight: second.weight, + accountID: second.accountID, + }, + ), + ); sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => { suggestions.push({ diff --git a/tests/unit/compareUserInListTest.ts b/tests/unit/compareUserInListTest.ts index 40ee003ad536..e9a6b2c1fa6e 100644 --- a/tests/unit/compareUserInListTest.ts +++ b/tests/unit/compareUserInListTest.ts @@ -1,7 +1,7 @@ import compareUserInList from '@libs/compareUserInList'; describe('compareUserInList', () => { - it('should compare the weight if the weight is difference', () => { + it('Should compare the weight if the weight is different', () => { const first = {displayName: 'John Doe', weight: 1, accountID: 1}; const second = {displayName: 'Jane Doe', weight: 2, accountID: 2}; expect(compareUserInList(first, second)).toBe(-1); From a5bea00163fcac93d8431c6b73a39b5b2269f4de Mon Sep 17 00:00:00 2001 From: dominictb Date: Tue, 30 Jul 2024 10:15:40 +0700 Subject: [PATCH 4/5] fix: clean up implementation --- src/libs/compareUserInList.ts | 29 ---------- .../ReportActionCompose/SuggestionMention.tsx | 53 ++++++++++++------- tests/unit/compareUserInListTest.ts | 16 +++--- 3 files changed, 42 insertions(+), 56 deletions(-) delete mode 100644 src/libs/compareUserInList.ts diff --git a/src/libs/compareUserInList.ts b/src/libs/compareUserInList.ts deleted file mode 100644 index 4f3bfcef27f3..000000000000 --- a/src/libs/compareUserInList.ts +++ /dev/null @@ -1,29 +0,0 @@ -import localeCompare from './LocaleCompare'; - -/** - * the comparison function used to determine which user will come first in the sorted list - * generally, smaller weight means will come first, and if the weight is the same, we'll sort based on displayName/login and accountID - */ - -type UserDetailsWithWeight = { - displayName: string; - weight: number; - accountID: number; -}; - -function compareUserInList(first: UserDetailsWithWeight, second: UserDetailsWithWeight) { - // first, we should sort by weight - if (first.weight !== second.weight) { - return first.weight - second.weight; - } - - const displayNameLoginOrder = localeCompare(first.displayName, second.displayName); - if (displayNameLoginOrder !== 0) { - return displayNameLoginOrder; - } - - // Then fallback on accountID as the final sorting criteria. - return first.accountID - second.accountID; -} - -export default compareUserInList; diff --git a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx index 7c5832f9829f..6a59b6c0ae1c 100644 --- a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx +++ b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx @@ -14,7 +14,7 @@ import useCurrentReportID from '@hooks/useCurrentReportID'; import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; import useDebounce from '@hooks/useDebounce'; import useLocalize from '@hooks/useLocalize'; -import compareUserInList from '@libs/compareUserInList'; +import localeCompare from '@libs/LocaleCompare'; import * as LoginUtils from '@libs/LoginUtils'; import * as PersonalDetailsUtils from '@libs/PersonalDetailsUtils'; import getPolicyEmployeeAccountIDs from '@libs/PolicyEmployeeListUtils'; @@ -57,6 +57,33 @@ type SuggestionPersonalDetailsList = Record< | null >; +function getDisplayName(p: PersonalDetails) { + const displayNameFromAccountID = ReportUtils.getDisplayNameForParticipant(p.accountID); + if (!displayNameFromAccountID) { + return p.login?.length ? p.login : ''; + } + return displayNameFromAccountID; +} + +/** + * the comparison function used to determine which user will come first in the sorted list + * generally, smaller weight means will come first, and if the weight is the same, we'll sort based on displayName/login and accountID + */ +function compareUserInList(first: PersonalDetails & {weight: number}, second: PersonalDetails & {weight: number}) { + // first, we should sort by weight + if (first.weight !== second.weight) { + return first.weight - second.weight; + } + + // next we sort by display name + const displayNameLoginOrder = localeCompare(getDisplayName(first), getDisplayName(second)); + if (displayNameLoginOrder !== 0) { + return displayNameLoginOrder; + } + // then fallback on accountID as the final sorting criteria. + return first.accountID - second.accountID; +} + function SuggestionMention( {value, selection, setSelection, updateComment, isAutoSuggestionPickerLarge, measureParentContainerAndReportCursor, isComposerFocused, isGroupPolicyReport, policyID}: SuggestionProps, ref: ForwardedRef, @@ -271,24 +298,10 @@ function SuggestionMention( } return true; - }) as Array; // at this point we are sure that the details are not null - - const sortedPersonalDetails = filteredPersonalDetails.sort((first, second) => - compareUserInList( - { - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - displayName: ReportUtils.getDisplayNameForParticipant(first.accountID) || first.login || '', - weight: first.weight, - accountID: first.accountID, - }, - { - // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing - displayName: ReportUtils.getDisplayNameForParticipant(second.accountID) || second.login || '', - weight: second.weight, - accountID: second.accountID, - }, - ), - ); + }) as Array; + + // at this point we are sure that the details are not null, since empty user details have been filtered in the previous step + const sortedPersonalDetails = filteredPersonalDetails.sort(compareUserInList); sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => { suggestions.push({ @@ -460,3 +473,5 @@ function SuggestionMention( SuggestionMention.displayName = 'SuggestionMention'; export default forwardRef(SuggestionMention); + +export {compareUserInList}; diff --git a/tests/unit/compareUserInListTest.ts b/tests/unit/compareUserInListTest.ts index e9a6b2c1fa6e..10e3dfe60012 100644 --- a/tests/unit/compareUserInListTest.ts +++ b/tests/unit/compareUserInListTest.ts @@ -1,24 +1,24 @@ -import compareUserInList from '@libs/compareUserInList'; +import {compareUserInList} from '@pages/home/report/ReportActionCompose/SuggestionMention'; describe('compareUserInList', () => { it('Should compare the weight if the weight is different', () => { - const first = {displayName: 'John Doe', weight: 1, accountID: 1}; - const second = {displayName: 'Jane Doe', weight: 2, accountID: 2}; + const first = {login: 'John Doe', weight: 1, accountID: 1}; + const second = {login: 'Jane Doe', weight: 2, accountID: 2}; expect(compareUserInList(first, second)).toBe(-1); }); it('Should compare the displayName if the weight is the same', () => { - const first = {displayName: 'águero', weight: 2, accountID: 1}; - const second = {displayName: 'Bronn', weight: 2, accountID: 1}; - const third = {displayName: 'Carol', weight: 2, accountID: 1}; + const first = {login: 'águero', weight: 2, accountID: 3}; + const second = {login: 'Bronn', weight: 2, accountID: 4}; + const third = {login: 'Carol', weight: 2, accountID: 5}; expect(compareUserInList(first, second)).toBe(-1); expect(compareUserInList(first, third)).toBe(-1); expect(compareUserInList(second, third)).toBe(-1); }); it('Should compare the accountID if both the weight and displayName are the same', () => { - const first = {displayName: 'águero', weight: 2, accountID: 1}; - const second = {displayName: 'aguero', weight: 2, accountID: 2}; + const first = {login: 'águero', weight: 2, accountID: 6}; + const second = {login: 'aguero', weight: 2, accountID: 7}; expect(compareUserInList(first, second)).toBe(-1); }); }); From 3782e22d6ea75ea5521a53c0723cdfb943e3a684 Mon Sep 17 00:00:00 2001 From: dominictb Date: Wed, 31 Jul 2024 14:53:28 +0700 Subject: [PATCH 5/5] fix: clean up implementation --- .../ReportActionCompose/SuggestionMention.tsx | 15 ++++++--------- tests/unit/compareUserInListTest.ts | 6 ++++++ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx index 6a59b6c0ae1c..c4957f8b7fae 100644 --- a/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx +++ b/src/pages/home/report/ReportActionCompose/SuggestionMention.tsx @@ -57,30 +57,27 @@ type SuggestionPersonalDetailsList = Record< | null >; -function getDisplayName(p: PersonalDetails) { - const displayNameFromAccountID = ReportUtils.getDisplayNameForParticipant(p.accountID); +function getDisplayName(details: PersonalDetails) { + const displayNameFromAccountID = ReportUtils.getDisplayNameForParticipant(details.accountID); if (!displayNameFromAccountID) { - return p.login?.length ? p.login : ''; + return details.login?.length ? details.login : ''; } return displayNameFromAccountID; } /** - * the comparison function used to determine which user will come first in the sorted list - * generally, smaller weight means will come first, and if the weight is the same, we'll sort based on displayName/login and accountID + * Comparison function to sort users. It compares weights, display names, and accountIDs in that order */ function compareUserInList(first: PersonalDetails & {weight: number}, second: PersonalDetails & {weight: number}) { - // first, we should sort by weight if (first.weight !== second.weight) { return first.weight - second.weight; } - // next we sort by display name const displayNameLoginOrder = localeCompare(getDisplayName(first), getDisplayName(second)); if (displayNameLoginOrder !== 0) { return displayNameLoginOrder; } - // then fallback on accountID as the final sorting criteria. + return first.accountID - second.accountID; } @@ -300,7 +297,7 @@ function SuggestionMention( return true; }) as Array; - // at this point we are sure that the details are not null, since empty user details have been filtered in the previous step + // At this point we are sure that the details are not null, since empty user details have been filtered in the previous step const sortedPersonalDetails = filteredPersonalDetails.sort(compareUserInList); sortedPersonalDetails.slice(0, CONST.AUTO_COMPLETE_SUGGESTER.MAX_AMOUNT_OF_SUGGESTIONS - suggestions.length).forEach((detail) => { diff --git a/tests/unit/compareUserInListTest.ts b/tests/unit/compareUserInListTest.ts index 10e3dfe60012..ce8b27ddf226 100644 --- a/tests/unit/compareUserInListTest.ts +++ b/tests/unit/compareUserInListTest.ts @@ -5,6 +5,7 @@ describe('compareUserInList', () => { const first = {login: 'John Doe', weight: 1, accountID: 1}; const second = {login: 'Jane Doe', weight: 2, accountID: 2}; expect(compareUserInList(first, second)).toBe(-1); + expect(compareUserInList(second, first)).toBe(1); }); it('Should compare the displayName if the weight is the same', () => { @@ -14,11 +15,16 @@ describe('compareUserInList', () => { expect(compareUserInList(first, second)).toBe(-1); expect(compareUserInList(first, third)).toBe(-1); expect(compareUserInList(second, third)).toBe(-1); + + expect(compareUserInList(second, first)).toBe(1); + expect(compareUserInList(third, first)).toBe(1); + expect(compareUserInList(third, second)).toBe(1); }); it('Should compare the accountID if both the weight and displayName are the same', () => { const first = {login: 'águero', weight: 2, accountID: 6}; const second = {login: 'aguero', weight: 2, accountID: 7}; expect(compareUserInList(first, second)).toBe(-1); + expect(compareUserInList(second, first)).toBe(1); }); });