Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use localeCompare to sort in mention list #45324

Merged
merged 8 commits into from
Jul 31, 2024
38 changes: 35 additions & 3 deletions src/pages/home/report/ReportActionCompose/SuggestionMention.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -56,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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function getDisplayName(p: PersonalDetails) {
const displayNameFromAccountID = ReportUtils.getDisplayNameForParticipant(p.accountID);
if (!displayNameFromAccountID) {
return p.login?.length ? p.login : '';
}
return displayNameFromAccountID;
}
function getDisplayName(details: PersonalDetails) {
const displayNameFromAccountID = ReportUtils.getDisplayNameForParticipant(details.accountID);
if (!displayNameFromAccountID) {
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

The jsdoc already explains this, and this says what the code is doing, which is obvious from reading the code. Since these comments don't follow our style guide (should use capital letters on the first word), I suggest removing them

Suggested change
// 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.
if (first.weight !== second.weight) {
return first.weight - second.weight;
}
const displayNameLoginOrder = localeCompare(getDisplayName(first), getDisplayName(second));
if (displayNameLoginOrder !== 0) {
return displayNameLoginOrder;
}

return first.accountID - second.accountID;
}

function SuggestionMention(
{value, selection, setSelection, updateComment, isAutoSuggestionPickerLarge, measureParentContainerAndReportCursor, isComposerFocused, isGroupPolicyReport, policyID}: SuggestionProps,
ref: ForwardedRef<SuggestionsRef>,
Expand Down Expand Up @@ -270,9 +298,11 @@ function SuggestionMention(
}

return true;
});
}) as Array<PersonalDetails & {weight: number}>;

// at this point we are sure that the details are not null, since empty user details have been filtered in the previous step
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 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);

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),
Expand Down Expand Up @@ -443,3 +473,5 @@ function SuggestionMention(
SuggestionMention.displayName = 'SuggestionMention';

export default forwardRef(SuggestionMention);

export {compareUserInList};
24 changes: 24 additions & 0 deletions tests/unit/compareUserInListTest.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import {compareUserInList} from '@pages/home/report/ReportActionCompose/SuggestionMention';

describe('compareUserInList', () => {
it('Should compare the weight if the weight is different', () => {
const first = {login: 'John Doe', weight: 1, accountID: 1};
const second = {login: 'Jane Doe', weight: 2, accountID: 2};
expect(compareUserInList(first, second)).toBe(-1);
dominictb marked this conversation as resolved.
Show resolved Hide resolved
});

it('Should compare the displayName if the weight is the same', () => {
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);
dominictb marked this conversation as resolved.
Show resolved Hide resolved
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 = {login: 'águero', weight: 2, accountID: 6};
const second = {login: 'aguero', weight: 2, accountID: 7};
expect(compareUserInList(first, second)).toBe(-1);
dominictb marked this conversation as resolved.
Show resolved Hide resolved
});
});
Loading