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

m-natarajan commented 1 month ago

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:

Go to staging.new.expensify.com
Go to FAB > Start chat
Create a group chat with User B.
Send a message to the group chat.
Click on the chat header > Members.
Invite User B.
As User B, go to the group chat.
Right click on the whisper "made @user an admin" > Copy to clipboard.
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?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/38435837/800261ee-1d6f-4604-add9-c0d2b5918bc9

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-05-15

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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[bot] commented 1 month ago

:wave: 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:

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

m-natarajan commented 1 month ago

@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 commented 1 month ago

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

francoisl commented 1 month ago

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 commented 1 month ago

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. https://github.com/Expensify/App/blob/9b839f4f18d724dba1ed17ed5967f7e033781428/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L395-L398

<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. https://github.com/Expensify/App/blob/9b839f4f18d724dba1ed17ed5967f7e033781428/src/components/HTMLEngineProvider/HTMLRenderers/MentionUserRenderer.tsx#L62-L63
```
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

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;
}

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);
}
}

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

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

https://github.com/Expensify/App/assets/41129870/dc71d224-7386-4f72-b2f2-eb66679b5438

marcaaron commented 1 month ago

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 commented 1 month ago

@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 commented 4 weeks ago

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

isabelastisser commented 2 weeks ago

@marcaaron, are you able to work on this?

marcaaron commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

@isabelastisser @marcaaron @eh2077 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!

ahmedGaber93 commented 2 weeks ago

Proposal

Updated

Add alternative solution.

Also, I think we can fix this issue and #40403 in one PR by the alternative solution.

eh2077 commented 2 weeks ago

Also, I think we can fix this issue and https://github.com/Expensify/App/issues/40403 in one PR by the alternative solution.

@Santhosh-Sellavel Should we put this issue on hold? I saw you have already started reviewing https://github.com/Expensify/App/issues/40403

isabelastisser commented 2 weeks ago

@Santhosh-Sellavel and @marcaaron, should we put this on hold?

marcaaron commented 2 weeks ago

@eh2077 can you answer the above please?

I'll jump back in here when have a proposal that's ready to review.

eh2077 commented 2 weeks ago

@Santhosh-Sellavel and @marcaaron, should we put this on hold?

@isabelastisser I think we should put this on hold because the other older issue is likely to be ended up a holistic approach that might help with this issue as well.

Santhosh-Sellavel commented 2 weeks ago

@eh2077 Yes, the fix should be addressing both issues. So feel free to review proposal if you have bandwidth as I running with less bandwidth. Let's put the other one on hold.

marcaaron commented 2 weeks ago

Ok let's actually then just assign whoever is handling that and we can remove the Help Wanted label.

marcaaron commented 2 weeks ago

Oh, it doesn't look like anyone is assigned. Anyways - @eh2077 let us know what we need to do when this is ready and I can update the assignments. You got this 😉

eh2077 commented 2 weeks ago

Reviewing proposals

ahmedGaber93 commented 1 week ago

Proposal

Updated

Add the full changes we need it in the alternative solution for accountIDToName following the changes here https://github.com/Expensify/App/issues/40403#issuecomment-2091898840 for reportIdToName.

eh2077 commented 1 week ago

@ahmedGaber93 Thanks for the update! I'm reviewing the proposal. Will update later

eh2077 commented 1 week ago

@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 https://github.com/Expensify/App/issues/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[bot] commented 1 week ago

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

marcaaron commented 1 week ago

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

Though the solution of https://github.com/Expensify/App/issues/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 commented 1 week ago

@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[bot] commented 1 week ago

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

eh2077 commented 1 week ago

I also agree to fix them together in one PR.

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

Prepare two maps
- function getReportIdToName(): Record<string, string> | undefined {
- function getAccountIDToName(currentUserAccountID: number): Record<string, string> | undefined {
From App, pass them to parser
- parser.htmlToText(content, {accountIDToName, reportIdToName})
- parser.htmlToMarkdown(content, {accountIDToName, reportIdToName})
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 commented 1 week ago

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 commented 5 days ago

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

puneetlath commented 5 days ago

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

rlinoz commented 5 days ago

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 https://github.com/Expensify/expensify-common/pull/686

robertKozik commented 5 days ago

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: https://github.com/Expensify/App/pull/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[bot] commented 3 days ago

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

eh2077 commented 3 days ago

@marcaaron Should we put this issue and https://github.com/Expensify/App/issues/40403 on hold for https://github.com/Expensify/App/pull/40565?

melvin-bot[bot] commented 2 days ago

@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 commented 1 day ago

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

Expensify / App