Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Web - Expensify.org Notification Does Not Update Display Name When Changed #26913

Closed
1 of 6 tasks
kbecciv opened this issue Sep 6, 2023 · 23 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Reviewing Has a PR in review

Comments

@kbecciv
Copy link

kbecciv commented Sep 6, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Click on the plus sign and select "Save the World".
  2. Choose "I know a teacher" and fill in the details.
  3. When the notification appears, go to Settings > Profile > Update Display Name. Notice that the notification does not update the display name dynamically

Expected Result:

The Expensify.org notification should update the display name when it is changed

Actual Result:

The Expensify.org notification does not update the display name when it is changed.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.3.64.2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

screen-capture.-.2023-09-02T010035.987.1.mp4
20230907_011030.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tewodrosGirmaA
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693642140451149

View all open jobs on GitHub

@kbecciv kbecciv added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 6, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@rmm-fl
Copy link
Contributor

rmm-fl commented Sep 6, 2023

Seems like a server issue, server should write @[email] instead of @[displayName].

@trjExpensify
Copy link
Contributor

I think the logic is to include the displayName or fallback on primaryLogin if one isn't set right now.

@StefStavri what do you think about this one?

We're going for a "@mention" style in the whispered welcome message, but it doesn't appear to be an actual mention (the displayName portion isn't highlighted). Should it be a real mention? If so, it would use primaryLogin as we don't use displayNames in mentions just yet until this issue is done.

After which, I'm not even 100% sure if we'd update all historical mentions to the updated displayName, but maybe @puneetlath can fill us in on if we'll get this bug report "for free" if we make it a real mention.

@StefStavri
Copy link

I don't really understand the problem here. In this example the member is called Apple and they are referring teachers they know to the campaign. When they do that, each time we send a whisper thank you in the TU public room with @[displayName] . I don't think there is anything to change here.

@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 7, 2023

Cool, so to expand on the bug report:

  1. User signs-up, sets their displayName as Tom Rhys Jones.
  2. Refers a teacher, a welcome message is sent which includes @displayName i.e @TomRhysJones
  3. User changes their displayName to Tom Jones-Shaw i.e @TomJones-Shaw
  4. the @displayName reference in the welcome message from (2) does not update to @TomJones-Shaw it remains as @TomRhysJones.

It's not a massively egregious bug, but it does highlight that we're basically "faux" styling a mention by hardcoding an @ before the user's firstName in this particular welcome message. So any changes to that displayName thereafter are not reflected. "Real" @displayName mentions don't exist yet, they are always @primaryLogin at the moment.

So my suggestions are either:

  1. Drop the @ so it's not being confused with mentions, it would just print the user's first and last name.
  2. Make it a real mention, so it would use the @primaryLogin for the time being until this is built which will properly display it as a @displayName mention when introduced.

@puneetlath
Copy link
Contributor

I agree with doing either 1 or 2. 2 is probably the best solution, but will require waiting until mentions have been updated to be accountID-based

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

@trjExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Sep 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

@trjExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@trjExpensify
Copy link
Contributor

I tend to agree with option #2. So @puneetlath if we made it a real mention now, it'll be shown as @primaryLogin in the welcome message. Then once mentions accommodate display names, it'll automatically start using @displayName if the conditions are met, right?

@StefStavri, what do you think?

@melvin-bot melvin-bot bot removed the Overdue label Sep 11, 2023
@puneetlath
Copy link
Contributor

that's correct

@trjExpensify
Copy link
Contributor

@marcochavezf @youssef-lr tagging you both in this one. We're leaning in the direction of making this a proper mention, are you cool with that before I move it forward with the expected results?

image

@melvin-bot melvin-bot bot added the Overdue label Sep 18, 2023
@trjExpensify
Copy link
Contributor

Bumped @marcochavezf to take a look!

@melvin-bot melvin-bot bot removed the Overdue label Sep 18, 2023
@marcochavezf
Copy link
Contributor

Yeah, converting it to proper mention sounds good. I will assign myself to update it tomorrow

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

Current assignee @marcochavezf is eligible for the Engineering assigner, not assigning anyone new.

@trjExpensify
Copy link
Contributor

Cool, thanks!

@marcochavezf
Copy link
Contributor

PR up

@marcochavezf marcochavezf added the Reviewing Has a PR in review label Sep 19, 2023
@marcochavezf
Copy link
Contributor

PR was deployed to production, I tested and received the welcome messages with the correct mentioning:

Screenshot 2023-09-22 at 12 36 04 Screenshot 2023-09-22 at 12 36 32

@marcochavezf
Copy link
Contributor

Closing it out

@tewodrosGirmaA
Copy link

Hey @trjExpensify, am I eligible for payment now that the issue has been resolved and deployed in production? Thank you

@trjExpensify
Copy link
Contributor

Yup! I've sent you an offer for $50.

@marcochavezf check the reportedBy before closing these. :)

@trjExpensify trjExpensify reopened this Sep 25, 2023
@tewodrosGirmaA
Copy link

@trjExpensify , I accept it thank you

@trjExpensify
Copy link
Contributor

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

7 participants