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.

Sign up for GitHub

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

Sort group member avatar icons correctly to avoid avatar rearranging upon receiving backend response #25416

Merged
merged 8 commits into from
Sep 22, 2023
Merged

Sort group member avatar icons correctly to avoid avatar rearranging upon receiving backend response #25416

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
990932d
sort group member avatar icons correctly
honnamkuan Aug 17, 2023
10bebee
Merge remote-tracking branch 'origin/main' into 23364
honnamkuan Aug 17, 2023
83d5baa
Merge branch 'main' into 23364
honnamkuan Aug 23, 2023
fd1f659
sort avatar icons by user displayName/login
honnamkuan Aug 23, 2023
e70c1e9
update comment
honnamkuan Aug 23, 2023
bcaa077
Merge branch 'main' into 23364
honnamkuan Aug 24, 2023
2996bf5
Merge remote-tracking branch 'origin/main' into 23364
honnamkuan Sep 20, 2023
4d93923
refactor lodashGet usages to const
honnamkuan Sep 20, 2023
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
25 changes: 16 additions & 9 deletions src/libs/ReportUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,23 +884,30 @@ function getIconsForParticipants(participants, personalDetails) {
for (let i = 0; i < participantsList.length; i++) {
const accountID = participantsList[i];
const avatarSource = UserUtils.getAvatar(lodashGet(personalDetails, [accountID, 'avatar'], ''), accountID);
participantDetails.push([
accountID,
lodashGet(personalDetails, [accountID, 'displayName']) || lodashGet(personalDetails, [accountID, 'login'], ''),
lodashGet(personalDetails, [accountID, 'firstName'], ''),
avatarSource,
]);
participantDetails.push([accountID, lodashGet(personalDetails, [accountID, 'displayName']) || lodashGet(personalDetails, [accountID, 'login'], ''), avatarSource]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon reviewing this again, I found that the original firstName element (index 2) is only used for sorting.

The thing is, when a user has firstName configured, his displayName/login is going to be a string that starts with his firstName.

e.g.

firstName: 'Lagertha',
displayName: 'Lagertha Lothbrok',
lastName: 'Lothbrok'

So it is actually redundant to first sort using firstName, then sub-sort using displayName/login.
Because we will get the exact same results if we just skip sorting using firstName, and start initial sorting using displayName/login.

With that reasoning, I have updated the code to remove firstName element from the array, because not using it for sorting means we do not need it at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this a little more readable by pulling lodashGet(personalDetails, [accountID, 'displayName']) || lodashGet(personalDetails, [accountID, 'login'], '') into its own variable like we're doing with avatarSource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedbacks, I am back :)
Have added the const refactoring and merge latest main to my branch, done some testing and verified codes work well.

Appreciate your review again.

}

// Sort all logins by first name (which is the second element in the array)
const sortedParticipantDetails = participantDetails.sort((a, b) => a[2] - b[2]);
const sortedParticipantDetails = _.chain(participantDetails)
.sort((first, second) => {
// First sort by displayName/login
const displayNameLoginOrder = first[1].localeCompare(second[1]);
if (displayNameLoginOrder !== 0) {
return displayNameLoginOrder;
}

// Then fallback on accountID as the final sorting criteria.
// This will ensure that the order of avatars with same login/displayName
// stay consistent across all users and devices
return first[0] > second[0];
})
.value();

// Now that things are sorted, gather only the avatars (third element in the array) and return those
const avatars = [];
for (let i = 0; i < sortedParticipantDetails.length; i++) {
const userIcon = {
id: sortedParticipantDetails[i][0],
source: sortedParticipantDetails[i][3],
source: sortedParticipantDetails[i][2],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated from index 3 to 2 for avatar source, because the original index 2 element (firstName) is deemed unnecessary and removed.

type: CONST.ICON_TYPE_AVATAR,
name: sortedParticipantDetails[i][1],
};
Expand Down
72 changes: 72 additions & 0 deletions tests/unit/ReportUtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,13 @@ const participantsPersonalDetails = {
login: '+18332403627@expensify.sms',
displayName: '(833) 240-3627',
},
5: {
accountID: 5,
displayName: 'Lagertha Lothbrok',
firstName: 'Lagertha',
login: 'lagertha2@vikings.net',
pronouns: 'She/her',
},
};
const policy = {
policyID: 1,
Expand All @@ -55,6 +62,53 @@ describe('ReportUtils', () => {
});
beforeEach(() => Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, CONST.LOCALES.DEFAULT).then(waitForPromisesToResolve));

describe('getIconsForParticipants', () => {
it('returns sorted avatar source by name, then accountID', () => {
expect(ReportUtils.getIconsForParticipants([1, 2, 3, 4, 5], participantsPersonalDetails)).toEqual([
{
id: 4,
name: '(833) 240-3627',
source: {
testUri: '../../../assets/images/avatars/user/default-avatar_5.svg',
},
type: 'avatar',
},
{
id: 2,
name: 'floki@vikings.net',
source: {
testUri: '../../../assets/images/avatars/user/default-avatar_3.svg',
},
type: 'avatar',
},
{
id: 3,
name: 'Lagertha Lothbrok',
source: {
testUri: '../../../assets/images/avatars/user/default-avatar_4.svg',
},
type: 'avatar',
},
{
id: 5,
name: 'Lagertha Lothbrok',
source: {
testUri: '../../../assets/images/avatars/user/default-avatar_6.svg',
},
type: 'avatar',
},
{
id: 1,
name: 'Ragnar Lothbrok',
source: {
testUri: '../../../assets/images/avatars/user/default-avatar_2.svg',
},
type: 'avatar',
},
]);
});
});

describe('getDisplayNamesWithTooltips', () => {
test('withSingleParticipantReport', () => {
expect(ReportUtils.getDisplayNamesWithTooltips(participantsPersonalDetails, false)).toStrictEqual([
Expand Down Expand Up @@ -94,6 +148,15 @@ describe('ReportUtils', () => {
accountID: 4,
pronouns: undefined,
},
{
displayName: 'Lagertha Lothbrok',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_6.svg',
},
login: 'lagertha2@vikings.net',
accountID: 5,
pronouns: 'She/her',
},
]);
});

Expand Down Expand Up @@ -135,6 +198,15 @@ describe('ReportUtils', () => {
accountID: 4,
pronouns: undefined,
},
{
displayName: 'Lagertha',
avatar: {
testUri: '../../../assets/images/avatars/user/default-avatar_6.svg',
},
login: 'lagertha2@vikings.net',
accountID: 5,
pronouns: 'She/her',
},
]);
});
});
Expand Down