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

Jump to bottom

Only consider personal details with logins set in OptionsListUtils.getOptions #21159

Merged
merged 5 commits into from
Jun 21, 2023
Merged

Only consider personal details with logins set in OptionsListUtils.getOptions #21159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9af9c96
only consider personal details with logins set
jasperhuangg Jun 20, 2023
6d3cea1
style
jasperhuangg Jun 20, 2023
fc0916a
remove unused
jasperhuangg Jun 20, 2023
1fcc839
add issue link
jasperhuangg Jun 20, 2023
116abf5
fix error with description being unset
jasperhuangg Jun 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
6 changes: 6 additions & 0 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,12 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any follow up issues to address the temp fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, we just recently merged the PR that handles when you try to chat with someone whose login you don't have in Onyx, so we should be able to undo this now. I'll create an issue for a contributor to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe actually I can just revert this.

personalDetails = _.pick(personalDetails, (detail) => Boolean(detail.login));
Comment on lines +606 to +610
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the effects of excluding personal details from this object? They don't appear in the option list?

Do you know where this was causing problems down the line? I imagine we where trying to use personalDetails.login which was missing, but in other fixes we have been adding personalDetails.login || personalDetails.displayName... just wondering why that approach was not good here.

Copy link
Contributor Author

@jasperhuangg jasperhuangg Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Because Tasks takes the login from the option and does a lot more stuff with it, so we can't just do that

Also, it's not guaranteed that personalDetails.displayName is equal to their login. If they have a display name set already then it won't be equal. That only applies to new users that haven't set their personal details yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that we actually needed the login (emails) for the flows that broke downstream, so using the displayName might not be enough

Copy link
Contributor

@aldo-expensify aldo-expensify Jun 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That only applies to new users

Then it can be that the personalDetails.login is missing for users that are not new? I mean, can it happen that personalDetails.login is missing and personalDetails.displayName is missing?

In the cases I have seen so far, when the login is missing, the displayName is the email address.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the cases I have seen so far, when the login is missing, the displayName is the email address.

Is this still true if the user sets their display name?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's definitely possible - maybe you can check with Puneet's updated query here? https://github.com/Expensify/Auth/pull/8153

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, this Auth fix works for 1:1 DM's

Copy link
Contributor

Choose a reason for hiding this comment

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

It's intended to also work for group DMs

Copy link
Contributor

Choose a reason for hiding this comment

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

Now to the last part of the issue, where if I have DM some random new users and send some messages. The chat appears in LHN. Again, when I go to chat switcher and type full email, and click on option, it creates a new report with same user

Copy link
Contributor

Choose a reason for hiding this comment

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

I reported here, in case it is already known

Copy link
Member

Choose a reason for hiding this comment

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

Hi, we should have placed this code just before creating an option

It would have prevented bug - Tooltip content only have comma in content when hovering group chat with non-register users

More details - #22593 (comment)


let recentReportOptions = [];
let personalDetailsOptions = [];
const reportMapForAccountIDs = {};
Expand Down
4 changes: 3 additions & 1 deletion src/libs/actions/Task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,9 @@ function editTaskAndNavigate(report, ownerEmail, ownerAccountID, {title, descrip

// Sometimes title or description is undefined, so we need to check for that, and we provide it to multiple functions
const reportName = (title || report.reportName).trim();
const reportDescription = (!_.isUndefined(description) ? description : report.description).trim();

// Description can be unset, so we default to an empty string if so
const reportDescription = (!_.isUndefined(description) ? description : lodashGet(report, 'description', '')).trim();

// If we make a change to the assignee, we want to add a comment to the assignee's chat
let optimisticAssigneeAddComment;
Expand Down