[ON HOLD FOR #40565 ][$250] Group chat - User mention is not copied when copying "made an admin" whisper to clipboard

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 1.4.63-0
Reproducible in staging?: y
Reproducible in production?: No
If this was caught during regression testing, add the test name, ID and link from TestRail: n/a
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause internal team
Slack conversation:

Action Performed:

1. Go to staging.new.expensify.com
2. Go to FAB > Start chat
3. Create a group chat with User B.
4. Send a message to the group chat.
5. Click on the chat header > Members.
6. Invite User B.
7. As User B, go to the group chat.
8. Right click on the whisper "made @user an admin" > Copy to clipboard.
9. Paste the content.

Expected Result:

The user mention in the whisper is copied.

Actual Result:

The user mention in the whisper is not copied.
The user mention is also missing in LHN.

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

Add any screenshot/video evidence

Bug6453668_1713448661144.20240418_215156.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c22e43e02e5a0415
• Upwork Job ID: 1785752223998607360
• Last Price Increase: 2024-06-05

[melvin-bot]

Triggered auto assignment to @francoisl (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[melvin-bot]

Triggered auto assignment to @isabelastisser (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.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[m-natarajan]

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[m-natarajan]

We think this bug might be related to #vip-vsb

[francoisl]

I feel like this has to do more with how we copy text from mentions specifically, because the message is in the format "<muted-text>made <mention-user accountID=NNN></mention-user> an admin</muted-text>".

Not sure it needs to be a blocker as it doesn't stop you from doing anything critical though.
In the meantime, cc @puneetlath if you have any idea off the top of your head what this could be from.

[ahmedGaber93]

Proposal

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

Group chat - User mention is not copied when copying "made an admin" whisper to clipboard

What is the root cause of that problem?

Mentioned user come from backend with different format unlike what we handle it now when get message text to copy it to clipboard.

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

Lines 395 to 398 in 9b839f4

	content.replace(/(<mention-user>)(.*?)(<\/mention-user>)/gi, (match, openTag, innerContent, closeTag): string => {
	const modifiedContent = Str.removeSMSDomain(innerContent) || '';
	return openTag + modifiedContent + closeTag || '';
	}),

<mention-user accountID=123></mention-user> // formate in this issue 
<mention-user>@login</mention-user> // current formate we handle it in copy to clipboard now

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

We need to handle this format by edit this replace or add extra replace to handle it here

// content.replace(/(<mention-user>)(.*?)(<\/mention-user>)/gi, (match, openTag, innerContent, closeTag): string => {
content.replace(/(<mention-user(?:\s+accountID=(\d*))?>)(.*?)(<\/mention-user>)/gi, (match, openTag, accountID, innerContent, closeTag): string => {
    // const modifiedContent = Str.removeSMSDomain(innerContent) || '';
    let modifiedContent = Str.removeSMSDomain(innerContent) || '';
    if (accountID && !modifiedContent){
        // get mentionDisplayText by accountID
        // I explained below how to get mentionDisplayText
        const modifiedContent = `@${mentionDisplayText};
    }

    return openTag + modifiedContent + closeTag || '';
})

• edit regex to optionally looking for accountID (?:\s+accountID=(\d*))?
• if accountID found, get mentionDisplayText by this accountID and use it rather than innerContent

how to get mentionDisplayText ?
After we catch accountID We will follow the way in MentionUserRenderer.tsx to get mentionDisplayText by accountID to handle all cases like short mention and hidden, etc.

App/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx

Lines 62 to 63 in 9b839f4

	mentionDisplayText = LocalePhoneNumber.formatPhoneNumber(user?.login ?? '') || PersonalDetailsUtils.getDisplayNameOrDefault(user);
	mentionDisplayText = getShortMentionIfFound(mentionDisplayText, htmlAttributeAccountID, user?.login ?? '');

const currentUserAccountID = getCurrentUserAccountID();
const [user, currentUser] = getPersonalDetailsByIDs([accountID, currentUserAccountID]);
let mentionDisplayText = LocalePhoneNumber.formatPhoneNumber(user?.login ?? '') || PersonalDetailsUtils.getDisplayNameOrDefault(user);
if (LoginUtils.areEmailsFromSamePrivateDomain(mentionDisplayText, currentUser.login ?? '')) {
    mentionDisplayText = mentionDisplayText.split('@')[0];
}

We can improve the code logic in the PR, I just keep it simple for explanation.

What alternative solutions did you explore? (Optional)

we can pass the second parameter extra accountIDToName and also reportIdToName to htmlToMarkdown and htmlToText here

and ExpensiMark will replace them here

these are the full changes

1. in src/libs/PersonalDetailsUtils.ts add new method getAccountIDToName

function getAccountIDToName(currentUserAccountID: number): Record<string, string> | undefined {
    const accountIDToName: Record<string, string> = {};
    const [currentUser] = getPersonalDetailsByIDs([currentUserAccountID], currentUserAccountID);

    Object.values(personalDetails ?? {}).forEach((detail: OnyxEntry<PersonalDetails>) => {
        if (detail?.accountID){
            let mentionDisplayText = LocalePhoneNumber.formatPhoneNumber(detail?.login ?? '') || getDisplayNameOrDefault(detail);
            if (LoginUtils.areEmailsFromSamePrivateDomain(mentionDisplayText, currentUser.login ?? '')) {
                mentionDisplayText = mentionDisplayText.split('@')[0];
            }
            accountIDToName[detail?.accountID] = mentionDisplayText;
        }
    });

    return accountIDToName;
}

2. in setClipboardMessage pass accountIDToName to ExpensiMark to fix the copied text plainText (plainText will be pasted if no HTML(content) is copied, or if you try to paste the copied text out of app see here)

function setClipboardMessage(content: string) {
    const parser = new ExpensiMark();
    const accountIDToName = PersonalDetailsUtils.getAccountIDToName(getCurrentUserAccountID());
    if (!Clipboard.canSetHtml()) {
        Clipboard.setString(parser.htmlToMarkdown(content, {accountIDToName}));
    } else {
        const plainText = parser.htmlToText(content, {accountIDToName});
        Clipboard.setHtml(content, plainText);
    }
}

3. fix the pasted html (content) in composer in web, we need to pass accountIDToName in handlePastedHTML to be parser.htmlToMarkdown(html, {accountIDToName}).

4. edit userMention here to match both <closedTag /> and <openTag></closeTag>

// regex: /<mention-user accountID="(\d+)" *\/>/gi,
regex: /<mention-user accountID="(\d+)"(?: *\/>|><\/mention-user>)/gi,

also userMention replacement function need to use extras.accountIDToName not extras.accountIdToName

result

Screen.Recording.2024-05-06.at.7.28.41.AM.mov

[marcaaron]

I don't think this worth blocking deploy over so gonna demote. Also can we check to see if the behavior works for Rooms? If not, then it's more or less something we haven't built out yet.

@francoisl if you want me to grab this I can as it's related to Group Chats.

[francoisl]

@marcaaron if you have the bandwidth, yes feel free to grab this one 🙏 I'm still catching up on some overdue issues from OOO.

[puneetlath]

I'm guessing it's something we just haven't built yet for accountID-based mentions.

[isabelastisser]

@marcaaron, are you able to work on this?

[marcaaron]

Yeah, let's put an External label on it? C+ can review any proposals.

[eh2077]

@ahmedGaber93 's proposal looks good to me. I think we should go with the alternative solution to fix this issue in ExpensiMark.

Though the solution of #40403 is similar to this one, they don't share same code - they're handled case by case according to the pattern of ExpensiMark. That said, it also makes sense to me to fix them separately.

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignee @marcaaron is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[marcaaron]

Curious to get @puneetlath's input here on that proposal.

Though the solution of #40403 is similar to this one, they don't share same code - they're handled case by case according to the pattern of ExpensiMark. That said, it also makes sense to me to fix them separately.

I'm not sure if I agree with this statement. We should be trying to write solutions that are holistic wherever possible. Having two separate implementations doing very similar stuff feels like we can come up with one proposal.

We can still treat them like two separate bugs (and get paid for both) - but let's work on the solution a bit more. @ahmedGaber93 What do you think about that?

[ahmedGaber93]

@marcaaron Yes I agree, the holistic plan makes the development process easier, as the both fixes have the same fix pattern (but not the same code), so I agree to fixing them in one PR in expensify-common and expensify repo.

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[eh2077]

I also agree to fix them together in one PR.

Let me summarise a bit on what the similar fix pattern looks like.

1. Prepare two maps
   • function getReportIdToName(): Record<string, string> | undefined {
   • function getAccountIDToName(currentUserAccountID: number): Record<string, string> | undefined {
2. From App, pass them to parser
   • parser.htmlToText(content, {accountIDToName, reportIdToName})
   • parser.htmlToMarkdown(content, {accountIDToName, reportIdToName})
3. Improve the regex rule of ExpensiMark if needed
   • htmlToMarkdownRules
   • htmlToTextRules

Currently, I think it looks fine to handle the two mentions in two different tags, <mention-user> and <mention-report>.

@marcaaron What do you think?

[marcaaron]

Yeah that's the general approach I'd take. But I'd like to get @puneetlath's feedback before moving forward. They implemented this feature originally so I think they should have the best context and final say. I don't think this is particularly urgent so let's give it a bit of time.

[eh2077]

Not overdue, we're waiting for @puneetlath 's input.

[puneetlath]

That makes sense to me. @robertKozik @rlinoz have we done similar elsewhere?

[rlinoz]

I think we haven't used the extras param yet, but it was added for this kind of situation as far as I can tell Expensify/expensify-common#686

[robertKozik]

Yes, I've introduced extras arguemnt to pass such a data to parser. Currently on main we don't have any usages yet, but this PR will introduce it: #40565. I suggest that we can hold this task until #40565 would be merged as in that PR we will introduce OnyxAwareParser which would encapsulate creating id maps

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[eh2077]

@marcaaron Should we put this issue and #40403 on hold for #40565?

[melvin-bot]

@isabelastisser @marcaaron @eh2077 this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[isabelastisser]

@marcaaron, can I put this on hold based on the comments above?

[marcaaron]

cc @eh2077

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸