[HOLD for payment 2023-10-05] [$1000] Add support for accountIDs in mentions on the client

[puneetlath]

Currently, mentions are purely login based. They take the form of <mention-user>@blah@test.com</mention-user>. Let's update mentions to support accountIDs. That means:

• If an accountID is present as an attribute (e.g. <mention-user accountID=1234>@blah@test.com</mention-user> or <mention-user accountID=1234 />) then we should use the accountID to render the mention. We should look at the personalDetailsList in Onyx and render the mention using the information for that accountID.
  • For example, if the email address for accountID=1234 is hi@test.com then we should render the mention as @hi@test.com instead of @blah@test.com.
  • If the accountID is present in the personalDetailsList, but there is no login available for that user, we should use the display name. For example, if the display name for accountID=1234 was "Bob Smith" we should render the mention as @Bob Smith
  • If the accountID is not present in the personalDetailsList then we should render the mention as @Hidden
  • When rendering a mention with an accountID, clicking on it should lead to the accountID-based profile page at the path /a/1234
• If there is no accountID present (e.g. <mention-user>@blah@test.com</mention-user>) then we should continue to render the mention just as we do today.

Currently the API does not send the accountID as part of the mention, so you will need to use mock data to test and build this functionality.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01544767e1c8edfd0d
• Upwork Job ID: 1696915578109919232
• Last Price Increase: 2023-08-30
• Automatic offers:
• samh-nl | Contributor | 26456828
• situchan | Contributor | 26658771

[melvin-bot]

Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[neonbhai]

Can contributors work on this feature? I would love to work on this

[puneetlath]

Yes, feel free to submit a proposal for how you'll do it.

[samh-nl]

Proposal

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

Add support for accountIDs in mentions

What is the root cause of that problem?

Functionality is not implemented.

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

The mention tag is processed in the MentionUserRenderer component. Here, we should add the specified behavior if the accountID is added. We can extract the accountID from props.tnode.

1. Connect with Onyx (instead of using PersonalDetailsUtils.getPersonalDetailsByIDs, this would result in stale data from being shown if the data changes):

withOnyx({
    personalDetails: {
        key: ONYXKEYS.PERSONAL_DETAILS_LIST,
    },
});

2. Get the data we want to display:

const htmlAttribAccountID = lodashGet(props.tnode.attributes, 'accountID');

if (!_.isEmpty(htmlAttribAccountID)) {
    const user = lodashGet(props.personalDetails, htmlAttribAccountID);
    displayName = lodashGet(user, 'login', '') || lodashGet(user, 'displayName', '') || Localize.translateLocal('common.hidden');
    ...
} else {
    // Existing logic
    ...

We could use the existing util function PersonalDetailsUtils.getDisplayNameOrDefault and add the email as the default value. However, this function uses displayName as the primary source and falls back to the email, which is the opposite as what is specified in the issue description.

3. Replace loginWithoutLeadingAt with the new display value
4. Update TextLink:

• If no email is present, onPress and href must navigate based on the accountID instead.
• If the name shows Hidden, there should be no navigation when clicking and we can use Text instead of TextLink. Or we maintain using TextLink but add styles.cursorDisabled.

+ const navigationRoute = _.isEmpty(htmlAttribAccountID) ? ROUTES.getDetailsRoute(displayName) : ROUTES.getProfileRoute(htmlAttribAccountID);
...
return (
...
<TextLink
-    href={ROUTES.getDetailsRoute(loginWithoutLeadingAt)}
-    onPress={() => showUserDetails(loginWithoutLeadingAt)}
+    href={navigationRoute}
+    onPress={() => Navigation.navigate(navigationRoute)}

What alternative solutions did you explore? (Optional)

N/A

[b4s36t4]

Proposal

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

Add support for accountIDs in mentions

What is the root cause of that problem?

No RC - Feature Request

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

We need to do multiple changes here.

1. Getting Attribute accountID of the <mention> tag which we can do by using the following method.

   const attributes = props.tnode.attributes
   const _accountIDAttribute = lodashGet(attributes,"accountID",0)

2. Checking the personaldetailslist by following method.

       PersonalDetails.getPersonalDetailsByIDs([_accountIDAttribute])

   Which would give the personal details list.

Based on the above two changes we need to figure out which have to follow the things.

If personal details found we need to update the text content of tnode.

props.tnode.set("hi@text.com")

This needs to be tested (98%) confirm this should work

hi@text.com is example user found by personaldetaillist

If no user found we should update content by Hidden by following same method as above.

After all some proper logic changes needed to figure out which should render when.

All these changes are being done inside MentionUserRenderer.js component.

What alternative solutions did you explore? (Optional)

NA

[Benjamin0427]

Hi. So you want to show differenct Page by accountID right?

[Benjamin0427]

Then Where can we fetch the accountID info from? API, Local Vairable or Cookie.

[joh42]

Proposal

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

Implement support for accountIDs in mentions.

What is the root cause of that problem?

Mentions currently do not support accountIDs

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

To implement this behavior, we first need to check if there is an accountID present in props.tnode. If not, we should leave the email that was included in the mention.

If there is an accountID, we should check if there is a login for that account in the personalDetailsList. If there is, we show the email of that login, if there is not, we show the display name. If that accoundID is not in the personalDetailsList at all, we show the mention as @hidden.

We also need to update the link - if there is an accountID present, we should replace ROUTES.getDetailsRoute below with ROUTES.getProfileRoute(accountID) to make sure we use the accountID-based path (a/1234).

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

Lines 49 to 57 in 03d13f7

	<TextLink
	// eslint-disable-next-line react/jsx-props-no-spreading
	{...defaultRendererProps}
	href={ROUTES.getDetailsRoute(loginWithoutLeadingAt)}
	style={[_.omit(props.style, 'color'), StyleUtils.getMentionStyle(isOurMention), {color: StyleUtils.getMentionTextColor(isOurMention)}]}
	onPress={() => showUserDetails(loginWithoutLeadingAt)}
	// Add testID so it is NOT selected as an anchor tag by SelectionScraper
	testID="span"
	>

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

📣 @Benjamin0427! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[b4s36t4]

#26306 (comment)

Demo for the my proposal.

Kapture.2023-08-31.at.00.02.17.mp4

PS: That delay is because of the API request and response override delay.

Maybe I forgot to do something but altogether the main objective is completed.

[neonbhai]

Proposal

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

Add support for accountIDs in mentions

What is the root cause of that problem?

New Feature.

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

To accomplish this we would:

• Add accountID to the component's propTypes here.

  accountID: propTypes.number,

  This accountID will be optional.
• We will be checking if we received accountID as prop.

  • if we did. we will be fetching personal details by accountID using:

    const fetchedPersonalDetails = ReportUtils.getPersonalDetailsForAccountID(props.accountID);

    Alternatively we can use PersonalDetails.getPersonalDetailsByIDs to fetch the accountID

  • We will be using displayText to set text of the mention.

  • The logic chain we will be following will resemble this:

      if(_.contains(fetchedPersonalDetails, 'accountID')) {
          //  we check for 'accountID' again to make sure a default object isn't returned
          
          if(_.contains(fetchedPersonalDetails, 'login')) {
              displayText = fetchedPersonalDetails.login;
          } else {
              displayText = fetchedPersonalDetails.displayName;
          }
      } else {
          // accountID is not present in ONYX
          displayText = 'Hidden';
      }

• We will then set props.tnode.data as displayText or render the displayText in TextLink using

    props.accountID 
      ? {displayText} 
      : <TNodeChildrenRenderer tnode={props.tnode} />

What alternative solutions did you explore? (Optional)

xx

[puneetlath]

@thesahindia any thoughts on the proposals so far?

[thesahindia]

@samh-nl's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[samh-nl]

@puneetlath @thesahindia I wanted to verify the expected behavior for @hidden mentions: should they also have a tooltip and link to /a/(account id)? Or would it be more fitting if the tooltip was removed and the cursor is changed to default (rather than a pointer)? Thanks.

[thesahindia]

Or would it be more fitting if the tooltip was removed and the cursor is changed to default (rather than a pointer)? Thanks.

It makes sense to me!

[b4s36t4]

@puneetlath I'm sorry, doesn't we have the rule first proposal (good proposal) gets assigned the bug? Just like did in this issue? #26706

[puneetlath]

Hey @b4s36t4. I think in this case, it's too tough to see a clear history around whose proposal said what at which time. Given that this is a feature request and the requirements were mostly laid out in the original issue, I'm comfortable going forward with @samh-nl.

@samh-nl in the future, as a best practice, it's best to post a new comment indicating that you've updated your proposal whenever updating it. That will help reduce this type of conflict on the issue.

[b4s36t4]

I can understand but It's being happened in multiple issues and I have seen people selecting the issues which are posted first without checking the version history nor asking for any detailing if the proposals are same. Maybe some rules with C+ would solve this. I have been a victim of these type of situations 4 times now and it's so hurting not to get equal opportunity.

Anyways thanks for responding, hoping you'd make some rules to overcome this issues.

[b4s36t4]

Also we can see the time of edited history when hover sometime on the selection.

Hope I'm not rude :)

[samh-nl]

@thesahindia Thanks for confirming.
@puneetlath Agreed!

[puneetlath]

@b4s36t4 I'll raise a discussion internally about comment editing and whether we should update our process in some way to account for it.

[melvin-bot]

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[puneetlath]

Re-assigning @situchan as C+

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @samh-nl got assigned: 2023-09-01 18:22:32 Z
• when the PR got merged: 2023-09-24 04:47:20 UTC
• days elapsed: 15

On to the next one 🚀

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.74-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• feat: support accountid attribute in user mentions #26877

If no regressions arise, payment will be issued on 2023-10-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @situchan requires payment offer (Contributor)
• @samh-nl requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[puneetlath]

All paid. Thanks y'all!