Closed NikkiWines closed 7 months ago
Triggered auto assignment to @alexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
@NikkiWines what would you think about including the @ symbol in the auto-selection dialog. So instead of having it be:
Puneet Expensicorp puneet@expensicorp.com Puneet Lath puneet
To have it be:
Puneet Expensicorp @puneet@expensicorp.com Puneet Lath @puneet
So that we really reinforce this idea of the handle
Users on the same private domain should have their domain hidden in the list of suggestions.
Currently the suggested mentions computed are displayed as is.
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.
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
Yup, what Jon said! I agree with that.
Nice, thanks, guys! I think this one is ready for proposals then π
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.
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.
This is a new feature
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.
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
.
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 nikkiwines@expensifail.com becomes Nikki Wines @nikkiwines@expensifail.com) when showing the mention options in search.
Use the method in 1
to 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-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. puneet@expensify.com
, 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
.
mention-user
html tag.mention-user
html tag with that constructed loginIf 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.
NA
Bringing you back @allroundexperts as the C+, please keep us posted if a proposal here will fix the one. Thanks!
@allroundexperts - will any of these proposals help here? 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:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
Please don't lie @MelvinBot. Are you having memory issues? π
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, @puneet@expensify.com
).
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.
Daily Update: It's been assigned and waiting for a PR to review.
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 π
@alexpensify @NikkiWines @allroundexperts @tienifr 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!
what's the latest here? Who is picking this up?
This got assigned to @tienifr yesterday and we are waiting for a PR here.
I'm creating the PR. Will raise in a hour
@tienifr it looks like your changes from this PR were accidentally excluded when MentionUserRenderer
was migrated to typescript, so shortened mentions aren't shown any longer.
Could you add your original changes from that PR as part of this issue, please?
@NikkiWines @allroundexperts Commented in https://github.com/Expensify/App/issues/36347#issuecomment-1959112012
Ok, @allroundexperts should we spin out a new GH for the other PR as @tienifr mentioned in the last comment? Thanks!
@alexpensify A PR was raised and merged for this already here
Perfect, thanks for the update @allroundexperts
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!
@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
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?
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.
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.47-10 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-03-13. :confetti_ball:
For reference, here are some details about the assignees on this issue:
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:
There's regression - https://github.com/Expensify/App/issues/38074#issuecomment-1991267250
Thanks for flagging.
I'm going to work on the payment process tomorrow-- I need to review the payment details here.
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: 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:
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 - nikkiwines@expensify.com and puneet@expensify.com - 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 nikkiwines@expensifail.com
becomesNikki Wines @nikkiwines@expensifail.com
) 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?
Screenshots/Videos
View all open jobs on GitHub
Upwork Automation - Do Not Edit