[HOLD for payment 2024-03-13] MEDIUM: [$500] Drop domain names when searching for users and auto-filling mentions

[NikkiWines]

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: v1.4.35-0
Reproducible in staging?: Y
Reproducible in production?: Y
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): N/A
Logs: N/A
Expensify/Expensify Issue URL: N/A
Issue reported by: N/A
Slack conversation: N/A

Action Performed:

1. Create a room with 4 members: 2 on the same private domain, 1 on another private domain, and 1 on a public domain
2. Draft a message that includes mentions for each user.

Expected Result:

When mentioning a user on the same private domain, the search result for the mention should not include the domain for the user's email. For example, given a message between two users - [email protected] and [email protected] - the search and auto-complete behavior should look like this:

When mentioning a user on a public domain or on a different private domain, the behavior should remain largely as it is, though we will now prepend an @ to the front of the login details (e.g. Nikki Wines [email protected] becomes Nikki Wines @[email protected]) when showing the mention options in search.

Actual Result:

The search result and mention auto-complete a same-private-domain mention contains the full email domain, even though the domain name gets removed after sending the message.

Screen.Recording.2024-01-31.at.17.31.02.mov

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01c84c193620b0a5db
• Upwork Job ID: 1754684823991881728
• Last Price Increase: 2024-02-06
• Automatic offers:
• tienifr | Contributor | 0

[melvin-bot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[puneetlath]

@NikkiWines what would you think about including the @ symbol in the auto-selection dialog. So instead of having it be:

Puneet Expensicorp [email protected]
Puneet Lath puneet

To have it be:

Puneet Expensicorp @[email protected]
Puneet Lath @puneet

So that we really reinforce this idea of the handle

[ayoung19]

Proposal

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

Users on the same private domain should have their domain hidden in the list of suggestions.

What is the root cause of that problem?

Currently the suggested mentions computed are displayed as is.

App/src/pages/home/report/ReportActionCompose/SuggestionMention.js

Line 303 in c675857

mentions={suggestionValues.suggestedMentions}

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

Like already done in the CompanyStep component, the SuggestionMention component should get the user and session object from Onyx to figure out whether the user is a from a public domain. If not, the domain can be extracted with Str.extractEmailDomain from the session object and this value can be used to transform alternateText in suggestionValues.suggestedMentions to hide the private domain before it is passed into MentionSuggestions.

We can then modify

App/src/pages/home/report/ReportActionCompose/SuggestionMention.js

Line 78 in c675857

const mentionCode = mentionObject.text === CONST.AUTO_COMPLETE_SUGGESTER.HERE_TEXT ? CONST.AUTO_COMPLETE_SUGGESTER.HERE_TEXT : `@${mentionObject.login}`;

to also strip the private domain when a suggestion has been selected so that the autofilled value matches what is being displayed in the suggestion list.

What alternative solutions did you explore? (Optional)

The above proposal has a little bit of duplicate logic that's spread out in an attempt to lower the risk of regressions, as the underlying data (which may be used in many other places) remains the same. Only alternateText in suggestionValues.suggestedMentions and behavior of insertSelectedMention are changed, both of which are self contained changes that won't propagate too far.

An alternative solution is to modify how the suggestionValues state is set. getMentionOptions can be modified to correctly compute the correct alternateText and login values of a suggestion. This may have a higher chance of causing a regression.

[NikkiWines]

I'm fine with it! I based this issue on what we have currently in product and also what Slack does.

But, since the autocomplete mention in the message draft prepends @, and we see it in the actual mention, I think it'd be fine to display it in the search as well. I'll update the screenshots + details in the OP

[NikkiWines]

@puneetlath do you think we should also highlight the @ in the search or do you think it's fine as it is in this screenshot

[puneetlath]

I think that screenshot looks good! We can get some design team feedback too if you'd like.

[alexpensify]

@NikkiWines - let me know if you want me to run this over to Design and then I can assign external. Thanks!

[NikkiWines]

Always get to get a once over by the experts! Let's grab someone from design to give things a look just to be certain

[alexpensify]

I asked for feedback here:

https://expensify.slack.com/archives/C03TME82F/p1707179243394239

[dubielzyk-expensify]

I don't feel super strongly. Given the name in bold won't have a @ sign I err on the side of not highlighting it. If we highlighted the @ symbol we'd end up with different highlights.

I hope that made sense, but yeah, I'm mostly on not highlighting it. Keen on @dannymcclain and @shawnborton thoughts too though

[shawnborton]

Yup, what Jon said! I agree with that.

[NikkiWines]

Nice, thanks, guys! I think this one is ready for proposals then 🚀

[melvin-bot]

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

[melvin-bot]

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

[ayoung19]

Hey all! I'm a new contributor and am a bit unfamiliar with the proposal flow. Was this issue previously not ready for proposals? Do I have to resubmit mine? Thanks!

[NikkiWines]

You can post a proposal before the Help Wanted label is added, but it comes with some risk and won't be reviewed until the issue is marked as ready for pickup (via the aforementioned Help Wanted label).

There are some more details in the Propose a solution for the job section of the contributing readme here, which is a good resource to review in general.

[ayoung19]

Ah got it. Thank you so much for clarifying, I missed that part of the doc and was kind of just cargo cult'ing other proposals that I saw.

[tienifr]

Proposal

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

The search result and mention auto-complete a same-private-domain mention contains the full email domain, even though the domain name gets removed after sending the message.

What is the root cause of that problem?

This is a new feature

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

1. In here, define a method formatLoginPrivateDomain that will return the formatted login after taking into account the common private domain elimination rule, we already have a very similar method here which is used to format the mention text, we can use it for formatLoginPrivateDomain similarly, just that there's no need for the userAccountID check.

2. Use that method for the text and alternateText here, we need to use for both, not just alternateText because if the user doesn't have display name, the login will show in text.

3. Prepend the @ to the alternateText here to address the below requirement

When mentioning a user on a public domain or on a different private domain, the behavior should remain largely as it is, though we will now prepend an @ to the front of the login details (e.g. Nikki Wines [email protected] becomes Nikki Wines @[email protected]) when showing the mention options in search.

4. Use the method in 1 to strip the shared private domain when inserting the login of mentioned user here as well

5. Since now if the user and the mention shares a private email, when inserting the mention, the email domain will be stripped, we need to update in expensify-common here to address that use case. Basically we'll be matching the @... pattern without the email part as well. If the matched string is a valid email (eg. [email protected], we can proceed as usual, if it's not a valid email (just puneet), then continue to below logic:

Check if the current user's login has private domain (by eg pass a param currentUserPrivateDomain to replace method),

• If yes, append that private domain to the @... pattern before constructing the mention-user html tag.

  We might want to check if that constructed login (with appended private domain) exists in the personalDetailList as well because the user might type a random @test text and we don't want to recognize it as a mention. If we decide to do this, we can pass the personalDetails to replace, or just the list of logins extracted from personalDetails.

  • If the constructed login is not a valid personal detail, consider it an invalid mention and do not make the mention-user html tag.
  • If the constructed login is a valid personal detail, it's a valid mention and we can proceed to make the mention-user html tag with that constructed login

• If no, it's an invalid mention and we do not make the mention-user html tag.

We need to check the above cases for phone number mention as well to make sure it works correctly there, there might be some minor additional changes needed to make sure of that.

What alternative solutions did you explore? (Optional)

NA

[alexpensify]

Perfect, thanks for the update @allroundexperts

[alexpensify]

Heads up, I will be offline until Tuesday, March 5, 2024. I will not be actively watching over this GitHub during that period. If anything urgent is needed here, please ask for help in the Open Source Slack Room-- thanks!

[tienifr]

@NikkiWines In the OP we just need to fix email mention

When mentioning a user on the same private domain, the search result for the mention should not include the domain for the user's email

While implementing the PR, we also fix your case here, it's about phone number and I think it is not in the OP. Can we get the bonus for fixing this point? The same situation is in here. Thanks. cc @allroundexperts @alexpensify

[NikkiWines]

Technically the phone number logins are emails (in that they all use @expensify.sms) and would fall under the same category 😅 but I understand your meaning and it definitely wasn't clear in the issue description. Additionally, since updating the phone number logins required separate logic from the email ones, I think an argument can be made for applying a bonus here. What do you think @alexpensify?

[alexpensify]

Ok, after reviewing, this wouldn't be a reporting bonus but the amount wouldn't be the full payment amount for a PR. I'm suggesting to go with 25% of the PR amount and apply that as a payment for the extra update. In this case, it would be 500 for the PR and then 125 for the additional PR update.

[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.4.47-10 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Drop domain names when searching for users and auto-filling mentions #36697

If no regressions arise, payment will be issued on 2024-03-13. 🎊

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

• @allroundexperts requires payment through NewDot Manual Requests
• @tienifr requires payment automatic offer (Contributor)

[melvin-bot]

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:

• [@allroundexperts] The PR that introduced the bug has been identified. Link to the PR:
• [@allroundexperts] 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:
• [@allroundexperts] 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:
• [@allroundexperts] Determine if we should create a regression test for this bug.
• [@allroundexperts] 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.
• [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[situchan]

There's regression - #38074 (comment)

[alexpensify]

Thanks for flagging.

[alexpensify]

I'm going to work on the payment process tomorrow-- I need to review the payment details here.

[alexpensify]

Here is the payment summary:

• External issue reporter - N/A
• Contributor that fixed the issue @tienifr $500 + $125 = $625
• Contributor+ that helped on the issue and/or PR @allroundexperts $250 + $125 = $375

Upwork Job: https://www.upwork.com/jobs/~01c84c193620b0a5db

Extra Notes regarding payment: There is regression, so I've accounted for that in the payment summary. Closing since everyone who is paid in Upwork has been sent one there.

There is an additional amount for the extra work as discussed here: #35532 (comment)

[allroundexperts]

Hi @alexpensify!
I think you forgot to account for #35532 (comment) in the payment summary?

[alexpensify]

@allroundexperts - thanks for the reminder. I'll work on this update later today.

[alexpensify]

I've updated the payment summary:

#35532 (comment)

[alexpensify]

Alright, closing again - the summary and the payment in Upwork has been updated accordingly.

[JmillsExpensify]

$375 approved for @allroundexperts based on summary.