[HOLD for payment][$250] Mention-Numbers are displayed when editing an email mention of a user that is not a contact

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: v9.0.6-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause-Internal team

Action Performed:

1. Go to staging.new.expensify.com
2. Go to any chat
3. Send a message containing "@" mentioning an existing user but is not in your contact list
4. Right-click on the message > Edit comment.
5. Observe the edit field

Expected Result:

The mentioned user's full email address is displayed in the edit box

Actual Result:

User ID number is displayed in the edit box instead of the email address

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6538648_1720680873449.2024-07-11_09_51_38.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021831462503801260039
• Upwork Job ID: 1831462503801260039
• Last Price Increase: 2024-09-04

Issue Owner

Current Issue Owner: @kadiealexander

Issue Owner

Current Issue Owner: @kadiealexander

[melvin-bot]

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[izarutskaya]

We think this issue might be related to the #vip-vsb

[Tony-MK]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Mention-Numbers are displayed when editing an email mention of a user that is not a contact

What is the root cause of that problem?

The root cause of the problem is the Parser.htmlTomakdown function needs an accountIDToName map to convert the mentioned accountIDs to the respective name.

This issue occurs in the Report.saveReportActionDraft function in the ContextMenuActions.tsx file

App/src/pages/home/report/ContextMenu/ContextMenuActions.tsx

Lines 243 to 245 in 28e5e45

	if (!draftMessage) {
	Report.saveReportActionDraft(reportID, reportAction, Parser.htmlToMarkdown(getActionHtml(reportAction)));
	} else {

Therefore, the Parser.htmlTomakdown function returns the raw accountID because the HTML looks like the below example.

"<mention-user accountID="17038670"/>"

It returns @17038670 instead of @Alice and saves the draft as @17038670 in the onyx.

What changes do you think we should make in order to solve the problem?

We should pass an accountIDToName object for the extras argument for the Parser.htmlTomakdown.

1. The First Solution which displays "@Alice"

We should create the accountIDToName record just before calling the Report.saveReportActionDraft function.

So, the code that will create the accountIDToName object should look similar to the one below.

const accountIDToName: Record<string, string> = {} as Record<string, string>;

if (reportAction.originalMessage?.mentionedAccountIDs) {
    reportAction.originalMessage.mentionedAccountIDs.forEach((accountID: number) => {
        accountIDToName[accountID] = ReportUtils.getDisplayNameForParticipant(accountID);
    });
}

Report.saveReportActionDraft(reportID, reportAction, Parser.htmlToMarkdown(getActionHtml(reportAction),  { accountIDToName }));

Screen.Recording.2024-07-11.at.18.47.51.mov

2. The Second Solution which displays "@Hidden"

Introduce accountIDToName by the following example code shown below.

accountIDToName: ReportUtils.getDisplayNameForParticipant as unknown as Record<string, string>

Hidden.mov

What alternative solutions did you explore? (Optional)

We could use or create a function to display "@alice" instead of "@hidden" like the second solution illustrated above for cleaner code in one line.

[nkdengineer]

Edited by proposal-police: This proposal was edited at 2024-08-07 10:06:41 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

User ID number is displayed in the edit box instead of hidden

What is the root cause of that problem?

After we mention a user that we haven't chatted with before, BE returns the personal details of this user without login. And then accountIDToNameMap of this user is fallback to accountID

App/src/libs/Parser.ts

Line 18 in 2c60fc9

accountIDToNameMap[personalDetails.accountID] = personalDetails.login ?? personalDetails.displayName ?? String(personalDetails.accountID);

What changes do you think we should make in order to solve the problem?

We should fallback as an empty string here then htmlToMarkdown function can display the user as Hidden by default here

App/src/libs/Parser.ts

Line 18 in 2c60fc9

accountIDToNameMap[personalDetails.accountID] = personalDetails.login ?? personalDetails.displayName ?? String(personalDetails.accountID);

What alternative solutions did you explore? (Optional)

Result

Screen.Recording.2024-08-07.at.17.05.04.mov

[melvin-bot]

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[aimane-chnaif]

@puneetlath what do you think is the expected behavior?
As no way to get login (email), should show @displayName or @Hidden?

[puneetlath]

Hmm, weird scenario. Interesting. I think either is fine. I guess display name is better.

[aimane-chnaif]

accountIDToNameMap[personalDetails.accountID] = personalDetails.login ?? String(personalDetails.accountID);

@nkdengineer thanks for the proposal. Can you please find PR that added this line and confirm that falling back to displayName instead of accountID doesn't cause any regression?

[nkdengineer]

@aimane-chnaif It comes from this PR #44732. It's simply something we display when we don't have a user login field so changing the fallback will not cause any regression.

[aimane-chnaif]

@aimane-chnaif It comes from this PR #44732. It's simply something we display when we don't have a user login field so changing the fallback will not cause any regression.

@nkdengineer No. That PR just moved this line from old file to new file

[Tony-MK]

@aimane-chnaif, Could you give feedback on my proposal?

I believe the best-excepted behavior is display @displayName.

[aimane-chnaif]

@Tony-MK Thanks for the proposal but I think your solution is unnecessarily complex. Have you found any regression with @nkdengineer's simple solution?

[Tony-MK]

Yeah.

According to my testing, the other proposal does not display @displayName but @hidden.

Therefore, editing a message, for example, @Alice would be @hidden after the edit.

Basically, like my second solution in my propsal

We should attempt the first solution.

[aimane-chnaif]

why not we simply update like this?
accountIDToNameMap[personalDetails.accountID] = personalDetails.login || personalDetails.displayName || String(personalDetails.accountID)

EDIT: we shouldn't do like that because it should show @Hidden instead of accountID when login, displayName don't exist

[nkdengineer]

@aimane-chnaif Is there any problem with my proposal?

[aimane-chnaif]

@nkdengineer your solution always shows @Hidden though @DisplayName exists. And #45259 (comment) was not confirmed yet.

[Tony-MK]

And #45259 (comment) was not confirmed yet.

@aimane-chnaif, Do you want the PR that introduced this line?

[nkdengineer]

@aimane-chnaif The change came from this PR #40565. And I see that in this PR we only test the case select the mention in suggestion list that already had the login field.

[nkdengineer]

your solution always shows @hidden though @DisplayName exists.

What does that mean? It always shows when open edit composer or after editing?

[melvin-bot]

@kadiealexander @aimane-chnaif this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

This issue has not been updated in over 15 days. @srikarparsi, @kadiealexander, @aimane-chnaif, @nkdengineer eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[nkdengineer]

@kadiealexander The PR was deployed to production 2 weeks ago.

[kadiealexander]

Ah sorry, the payment automation broke. Let me finish things now.

[kadiealexander]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR: [Mentions v2] Support mentions in editing comments #40565
• [@aimane-chnaif] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: [Mentions v2] Support mentions in editing comments #40565 (comment)
• [@aimane-chnaif] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: https://github.com/Expensify/Expensify/issues/427767

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~021831462503801260039

[melvin-bot]

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

[kadiealexander]

Payouts due:

• Contributor: $250 @nkdengineer (offer)
• Reviewer: $250 @aimane-chnaif (offer)

Upwork job is here.

[kadiealexander]

@nkdengineer please let me know when you accept the offer.
@aimane-chnaif bump on completing the checklist!

[aimane-chnaif]

• The PR that introduced the bug has been identified. Link to the PR: [Mentions v2] Support mentions in editing comments #40565
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: [Mentions v2] Support mentions in editing comments #40565 (comment)
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A

Regression Test Proposal

1. Go to any chat
2. Send a message containing "@" mentioning an existing user without display name and not in your contacts list
3. Verify that "@hidden" is shown
4. Right-click on the message > Edit comment
5. Observe the edit field
6. Verify that "@hidden" is displayed in the edit box

[nkdengineer]

@kadiealexander I accepted the offer

[aimane-chnaif]

@kadiealexander I'd like to use upwork

[kadiealexander]

@aimane-chnaif sent you an offer!

[kadiealexander]

@aimane-chnaif bump!

[kadiealexander]

@aimane-chnaif please reopen the issue once you've accepted the Upwork offer.

[aimane-chnaif]

@kadiealexander I accepted offer. Sorry for delay

[kadiealexander]

Paid, thanks @aimane-chnaif!