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

[$500] Room - @expensify.sms suffix appears after invitation & disappears after opening user avatar #35089

Closed
5 of 6 tasks
lanitochka17 opened this issue Jan 24, 2024 · 45 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 24, 2024

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: 1.4.31-2
Reproducible in staging?: Y
Reproducible in production?: N
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

  1. Go to a room
  2. Click on the room header > Members > Invite
  3. Invite via phone number
  4. Return to the main chat
  5. Click on the phone number in the invite notification
  6. Note that it shows @expensify.sms
  7. Click on the avatar
  8. Close the avatar

Expected Result:

@expensify.sms should remain (behavior on production)

Actual Result:

@expensify.sms suffix disappears after opening user avatar

Workaround:

Unknown

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

Add any screenshot/video evidence

Bug6353531_1706119428365.20240124_194713.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01bcbbe2d08d76f86e
  • Upwork Job ID: 1752445825076850688
  • Last Price Increase: 2024-03-19
  • Automatic offers:
    • dukenv0307 | Contributor | 0
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

Triggered auto assignment to @tylerkaraszewski (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@thienlnam
Copy link
Contributor

Not sure if this was an intentional update or not. But doesn't seem to need to be a blocker

@thienlnam thienlnam added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jan 25, 2024
@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 30, 2024

@tylerkaraszewski Huh... This is 4 days overdue. Who can take care of this?

@tylerkaraszewski tylerkaraszewski added Help Wanted Apply this label when an issue is open to proposals by contributors External Added to denote the issue can be worked on by a contributor labels Jan 30, 2024
Copy link

melvin-bot bot commented Jan 30, 2024

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

@melvin-bot melvin-bot bot changed the title Room - @expensify.sms suffix appears after invitation & disappears after opening user avatar [$500] Room - @expensify.sms suffix appears after invitation & disappears after opening user avatar Jan 30, 2024
Copy link

melvin-bot bot commented Jan 30, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jan 30, 2024
@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Jan 30, 2024

Proposal

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

Room - @expensify.sms suffix appears after invitation & disappears after opening user avatar

What is the root cause of that problem?

Problem related with this PR #28999

The main problem with the issue is that we have the wrong condition here

if (!ValidationUtils.isValidAccountRoute(Number(accountID)) ?? !!avatarURL) {

Since we are using a new operator ??, the second part of the condition is never checked as result we call openPublicProfilePage

useEffect(() => {
if (!ValidationUtils.isValidAccountRoute(Number(accountID)) ?? !!avatarURL) {
return;
}
PersonalDetails.openPublicProfilePage(accountID);
}, [accountID, avatarURL]);

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

To fix this issue we can use || instead of ??

As result :

if (!ValidationUtils.isValidAccountRoute(Number(accountID)) || !!avatarURL) {

Or something similar

What alternative solutions did you explore? (Optional)

NA

@tienifr
Copy link
Contributor

tienifr commented Jan 31, 2024

Proposal

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

@expensify.sms suffix disappears after opening user avatar

I think the Expected Result doesn't sound correct, the @expensify.sms is known internally only, the user doesn't know about it and it should never be displayed for the user to see. We have a removeSMSDomain method just for this purpose, and it's used everywhere.

Also in this issue if we refresh the page, we'll see that there'll be no @expensify.sms suffix, which is correct. So the expectation here should be that @expensify.sms should never show, before or after.

What is the root cause of that problem?

  1. When we invite a new member by phone number, the displayName in the back-end response (of InviteToRoom) is incorrect. Let's say the phone number is +1 401-659-7742, the displayName in response's personalDetailList will be [email protected].

Then when opening the Avatar, it will trigger this (due to a bug in this line where ?? was used in correctly, it should be ||). And this will call the OpenPublicProfilePage endpoint, the back-end will return the correct displayName now (as the formatted phone number) and the @expensify.sms will disappear

  1. In here, we're not passing the accountID as data of the icons, so even though the back-end returns correct data now, in the Tooltip when hovering over member avatar in Members list, it will still show @expensify.sms
Screenshot 2024-01-31 at 10 50 06 AM

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

  1. Fix the back-end so that the displayName in the back-end response of InviteToRoom is correct (just like the displayName when calling OpenPublicProfilePage, same logic)

Although this should fix the issue already, we should also change ?? to || here, because with ?? usage currently, the second part (!!avatarURL) will never be evaluated because the first part is always a boolean (not null/undefined)

  1. Pass the accountID to the icons data here so this UserDetailsTooltip logic will work correctly.
id: accountID,

What alternative solutions did you explore? (Optional)

NA

@melvin-bot melvin-bot bot added the Overdue label Feb 2, 2024
@aimane-chnaif
Copy link
Contributor

@tylerkaraszewski should we show @expensify.sms everywhere or not?

Screenshot 2024-02-05 at 11 08 30 AM Screenshot 2024-02-05 at 11 08 57 AM

@melvin-bot melvin-bot bot removed the Overdue label Feb 5, 2024
Copy link

melvin-bot bot commented Feb 6, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Feb 7, 2024
Copy link

melvin-bot bot commented Feb 8, 2024

@tylerkaraszewski, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@tylerkaraszewski
Copy link
Contributor

I think we should show it nowhere, these should always look like phone numbers.

@melvin-bot melvin-bot bot removed the Overdue label Feb 9, 2024
Copy link

melvin-bot bot commented Mar 18, 2024

@tylerkaraszewski, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Mar 19, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@aimane-chnaif
Copy link
Contributor

I am going to be OOO soon. Please assign new C+

@melvin-bot melvin-bot bot removed the Overdue label Mar 20, 2024
@aimane-chnaif aimane-chnaif removed their assignment Mar 20, 2024
@dukenv0307
Copy link
Contributor

@tylerkaraszewski I can take over here as C+

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 20, 2024
Copy link

melvin-bot bot commented Mar 20, 2024

📣 @dukenv0307 🎉 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 📖

@dukenv0307
Copy link
Contributor

@tylerkaraszewski This issue is not reproducible, but I found the related bug, it seems to be a BE bug

Screen.Recording.2024-03-21.at.15.00.27.mov

API InviteToRoom returns the wrong displayName (within country code). After going to profile page, API OpenPublicProfilePage is triggered and it returns the correct displayName

Screen.Recording.2024-03-21.at.15.02.10.mov

@melvin-bot melvin-bot bot added the Overdue label Mar 25, 2024
@tylerkaraszewski
Copy link
Contributor

Discussion here about how to fix the backend issue: https://expensify.slack.com/archives/C03TQ48KC/p1711403191153639

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 29, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

@tylerkaraszewski, @dukenv0307 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@tylerkaraszewski
Copy link
Contributor

I'm making this weekly because it's like my 3rd priority right now.

@melvin-bot melvin-bot bot removed the Overdue label Apr 8, 2024
@tylerkaraszewski tylerkaraszewski added Weekly KSv2 and removed Daily KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot added the Overdue label Apr 17, 2024
@tylerkaraszewski
Copy link
Contributor

I should be able to look into this this week.

@melvin-bot melvin-bot bot removed the Overdue label Apr 22, 2024
@melvin-bot melvin-bot bot added the Overdue label May 1, 2024
@tylerkaraszewski
Copy link
Contributor

I did not. Maybe next week.

@melvin-bot melvin-bot bot removed the Overdue label May 2, 2024
@melvin-bot melvin-bot bot added the Overdue label May 13, 2024
@tylerkaraszewski
Copy link
Contributor

This has been sitting for months. I am closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants