App
App copied to clipboard
Expensify
MEDIUM: [$500] Drop domain names when searching for users and auto-filling mentions
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:
- Create a room with 4 members: 2 on the same private domain, 1 on another private domain, and 1 on a public domain
- 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.
https://github.com/Expensify/App/assets/16747822/860a332c-b2e2-44dc-ad00-8f5f8fc409bf
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?
- [x] Android: Native
- [x] Android: mWeb Chrome
- [x] iOS: Native
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Screenshots/Videos
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~01c84c193620b0a5db
- Upwork Job ID: 1754684823991881728
- Last Price Increase: 2024-02-06
Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@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
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.
https://github.com/Expensify/App/blob/c6758578fb85e9eff6f9ccf5a52f3dd34e6e6383/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L303
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 https://github.com/Expensify/App/blob/c6758578fb85e9eff6f9ccf5a52f3dd34e6e6383/src/pages/home/report/ReportActionCompose/SuggestionMention.js#L78 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.
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
@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
I think that screenshot looks good! We can get some design team feedback too if you'd like.
@NikkiWines - let me know if you want me to run this over to Design and then I can assign external. Thanks!
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
I asked for feedback here:
https://expensify.slack.com/archives/C03TME82F/p1707179243394239
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
Job added to Upwork: https://www.upwork.com/jobs/~01c84c193620b0a5db
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)
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!
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.
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.
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?
-
In here, define a method
formatLoginPrivateDomainthat 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 forformatLoginPrivateDomainsimilarly, just that there's no need for theuserAccountIDcheck. -
Use that method for the
textandalternateTexthere, we need to use for both, not justalternateTextbecause if the user doesn't have display name, the login will show intext. -
Prepend the
@to thealternateTexthere 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.
-
Use the method in
1to strip the shared private domain when inserting the login of mentioned user here as well -
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-commonhere 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 (justpuneet), 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-userhtml tag.We might want to check if that constructed login (with appended private domain) exists in the
personalDetailListas well because the user might type a random@testtext and we don't want to recognize it as a mention. If we decide to do this, we can pass thepersonalDetailstoreplace, or just the list of logins extracted frompersonalDetails.- If the constructed login is not a valid personal detail, consider it an invalid mention and do not make the
mention-userhtml tag. - If the constructed login is a valid personal detail, it's a valid mention and we can proceed to make the
mention-userhtml tag with that constructed login
- If the constructed login is not a valid personal detail, consider it an invalid mention and do not make the
-
If no, it's an invalid mention and we do not make the
mention-userhtml 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
Bringing you back @allroundexperts as the C+, please keep us posted if a proposal here will fix the one. Thanks!
Hi @tienifr. I don't understand why we need to touch the ExpensiMark library? We're matching @... without the email already. Since we will be dropping the domain part from suggestion text, the highlight should work as expected as well.
https://github.com/Expensify/App/assets/30054992/a206ada8-ee70-44aa-ac30-0c6746d48bc5
Let me know if I missed something.
📣 @allroundexperts! 📣 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:
- Make sure you've read and understood the contributing guidelines.
- 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.
- 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.
- 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>
Hi @tienifr. I don't understand why we need to touch the ExpensiMark library? We're matching @... without the email already. Since we will be dropping the domain part from suggestion text, the highlight should work as expected as well.
@allroundexperts Yes the part where we search the mention will work.
But after we select a mention (@puneet), then try to send it, as per the requirement it will only be a text like hey there, @puneet being sent in the Composer. Currently our app cannot process such a email-stripped text into the correct mention-user html tag, it needs the full email (like hey there, @[email protected]).
The ExpensiMark change I suggested is so that it can handle email-stripped mention (like @puneet)
That makes sense. Thanks for the detailed explanation. @tienifr's proposal should work well!
🎀 👀 🎀 C+ reviewed
Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
⚠️ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Sorry, actually hadn't quite finished reviewing the proposals. I agree @tienifr's proposal looks good - thanks for the clarification on the ExpensiMark changes needed.
📣 @tienifr 🎉 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 📖