Skip to content

Fix Tooltip content only have comma in content when hovering group chat with non-register users #23807

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

Merged
jasperhuangg merged 3 commits into from
Aug 24, 2023
Merged

Fix Tooltip content only have comma in content when hovering group chat with non-register users #23807

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
89414f6
Only filter personal details having login for displaying in option list
hoangzinh Jul 28, 2023
ca33a26
Merge branch 'main' into df/22593
hoangzinh Aug 18, 2023
5828a1d
Add more comment to warning override personalDetails object
hoangzinh Aug 24, 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
13 changes: 6 additions & 7 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,12 +628,6 @@ function getOptions(
};
}

// We're only picking personal details that have logins set
// This is a temporary fix for all the logic that's been breaking because of the new privacy changes
// See https://github.com/Expensify/Expensify/issues/293465 for more context
// eslint-disable-next-line no-param-reassign
personalDetails = !includeP2P ? {} : _.pick(personalDetails, (detail) => Boolean(detail.login));

let recentReportOptions = [];
let personalDetailsOptions = [];
const reportMapForAccountIDs = {};
Expand Down Expand Up @@ -714,7 +708,12 @@ function getOptions(
);
});

let allPersonalDetailsOptions = _.map(personalDetails, (personalDetail) =>
// We're only picking personal details that have logins set
// This is a temporary fix for all the logic that's been breaking because of the new privacy changes
// See https://github.com/Expensify/Expensify/issues/293465 for more context
Comment on lines +711 to +713

@jasperhuangg jasperhuangg Aug 23, 2023

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.

Maybe add to this comment a bit explaining the placement of this block specifically–like why do we need to put it after the logic where we generate all the report options?

@hoangzinh hoangzinh Aug 24, 2023 •
edited
Loading

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.

Hi @jasperhuangg I'm unsure should we add more comment about placement of this block. Because the current comment is quite clear to me. It's saying we should only pick ... because ...

But I just added a comment about why we should not override personalDetails object. Could you help to review the comment again? Thanks

// Moreover, we should not override the personalDetails object, otherwise the createOption util won't work properly, it returns incorrect tooltipText
const havingLoginPersonalDetails = !includeP2P ? {} : _.pick(personalDetails, (detail) => Boolean(detail.login));
let allPersonalDetailsOptions = _.map(havingLoginPersonalDetails, (personalDetail) =>
createOption([personalDetail.accountID], personalDetails, reportMapForAccountIDs[personalDetail.accountID], reportActions, {
showChatPreviewLine,
forcePolicyNamePreview,
Expand Down