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

[$1000] Email address is being displayed on the IOU preview instead of the last name #14228

Closed
6 tasks
kavimuru opened this issue Jan 11, 2023 · 26 comments
Closed
6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kavimuru
Copy link

kavimuru commented Jan 11, 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. Open chat of a user that has set only last name (empty first name)
  2. Click on the + icon
  3. Click on Send Money
  4. Enter any amount
  5. Click Next
  6. Click I'll settle up elsewhere

Expected Result:

Last name is displayed on the IOU preview like it is displayed everywhere else

Actual Result:

Email address is being displayed instead of the last name on IOU preview
In LHN it appears with Last name

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.2.52-4
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:

last.name.mov
Recording.1264.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e0d43c6a348fa48c
  • Upwork Job ID: 1613588503429910528
  • Last Price Increase: 2023-01-12
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 11, 2023

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

@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 11, 2023
@MitchExpensify
Copy link
Contributor

MitchExpensify commented Jan 12, 2023

Not a bug because the profile is incomplete so its reasonable to display the email of the user instead of surname - Discussion here https://expensify.slack.com/archives/C049HHMV9SM/p1673487138834609?thread_ts=1673426920.016829&cid=C049HHMV9SM

@MitchExpensify
Copy link
Contributor

We're actually going to fix this in favor of the consistent pattern that exists today. Namely, showing the last name regardless of not having a first name saved

@MitchExpensify MitchExpensify added the Needs Reproduction Reproducible steps needed label Jan 12, 2023
@MitchExpensify
Copy link
Contributor

My mistake! This is indeed reproducible (I was looking at LHN versus the preview, my bad):

Web:
image

Desktop app:
image

@MitchExpensify MitchExpensify added External Added to denote the issue can be worked on by a contributor and removed Needs Reproduction Reproducible steps needed labels Jan 12, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 12, 2023
@melvin-bot melvin-bot bot changed the title Email address is being displayed on the IOU preview instead of the last name [$1000] Email address is being displayed on the IOU preview instead of the last name Jan 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 12, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 12, 2023

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Jan 12, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 12, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 12, 2023

Triggered auto assignment to @robertjchen (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 12, 2023

Proposal

We can add another or condition to get the last name in case the first name is empty.

diff --git a/src/components/ReportActionItem/IOUPreview.js b/src/components/ReportActionItem/IOUPreview.js
index 1c997119d..cb3115602 100644
--- a/src/components/ReportActionItem/IOUPreview.js
+++ b/src/components/ReportActionItem/IOUPreview.js
@@ -119,8 +119,11 @@ const IOUPreview = (props) => {
     const isCurrentUserManager = managerEmail === sessionEmail;
 
     const managerName = lodashGet(props.personalDetails, [managerEmail, 'firstName'], '')
+                        || lodashGet(props.personalDetails, [ownerEmail, 'lastName'], '') 
                         || Str.removeSMSDomain(managerEmail);
-    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') || Str.removeSMSDomain(ownerEmail);
+    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') 
+                        || lodashGet(props.personalDetails, [ownerEmail, 'lastName'], '') 
+                        || Str.removeSMSDomain(ownerEmail);
     const managerAvatar = lodashGet(props.personalDetails, [managerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(managerEmail);
     const ownerAvatar = lodashGet(props.personalDetails, [ownerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(ownerEmail);
     const cachedTotal = props.iouReport.total && props.iouReport.currency

@Ollyws
Copy link
Contributor

Ollyws commented Jan 12, 2023

Proposal

We just need to change firstName to displayName:

diff --git a/src/components/ReportActionItem/IOUPreview.js b/src/components/ReportActionItem/IOUPreview.js
index 1c997119d..d1ea9faad 100644
--- a/src/components/ReportActionItem/IOUPreview.js
+++ b/src/components/ReportActionItem/IOUPreview.js
@@ -118,9 +118,9 @@ const IOUPreview = (props) => {
     // Pay button should only be visible to the manager of the report.
     const isCurrentUserManager = managerEmail === sessionEmail;
 
-    const managerName = lodashGet(props.personalDetails, [managerEmail, 'firstName'], '')
+    const managerName = lodashGet(props.personalDetails, [managerEmail, 'displayName'], '')
                         || Str.removeSMSDomain(managerEmail);
-    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') || Str.removeSMSDomain(ownerEmail);
+    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'displayName'], '') || Str.removeSMSDomain(ownerEmail);
     const managerAvatar = lodashGet(props.personalDetails, [managerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(managerEmail);
     const ownerAvatar = lodashGet(props.personalDetails, [ownerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(ownerEmail);
     const cachedTotal = props.iouReport.total && props.iouReport.currency

@daraksha-dk
Copy link
Contributor

Proposal

Display name is enough it will also handle email/phone

const managerName = lodashGet(props.personalDetails, [managerEmail, 'firstName'], '')
|| Str.removeSMSDomain(managerEmail);
const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') || Str.removeSMSDomain(ownerEmail);

Make the suggested changes -

-   const managerName = lodashGet(props.personalDetails, [managerEmail, 'firstName'], '')
-                        || Str.removeSMSDomain(managerEmail);

+    const managerName = lodashGet(props.personalDetails, [managerEmail, 'displayName'], '')
-    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') || Str.removeSMSDomain(ownerEmail);
+    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'displayName'], '')

@bernhardoj
Copy link
Contributor

Using display name will show both first name and last name, but I think we only want to show either the first or the last name (or email if both empty).

@sobitneupane
Copy link
Contributor

Using display name will show both first name and last name, but I think we only want to show either the first or the last name (or email if both empty).

@bernhardoj I think so.

But IOU Quote must also be updated accordingly.
Screenshot 2023-01-13 at 00 12 26

I am not sure if it is possible to change IOUQuote message from front end.

@bernhardoj
Copy link
Contributor

bernhardoj commented Jan 13, 2023

Yeah, I think the IOU quote message is composed on the backend. Here is the params example when we send money through SendMoneyElsewhere api.

{"chatReportID": "1989942874938753", "clientID": 83, "iouReportID": "6320334089970635", "newIOUReportDetails": "{\"amount\":100,\"currency\":\"IDR\",\"requestorEmail\":\"bernhard.josephus+0000@gmail.com\",\"comment\":\"\",\"idempotencyKey\":\"2302bd1b-fb87-4bf9-9530-ef9fc36fe019\"}", "paymentMethodType": "Elsewhere", "reportActionID": "8939369918308593365", "transactionID": "2870947001874360238"}

Anyway, I have an updated proposal which I will post on the next comment.

@bernhardoj
Copy link
Contributor

Updated Proposal

We can simplify the condition with just firstName and displayName

diff --git a/src/components/ReportActionItem/IOUPreview.js b/src/components/ReportActionItem/IOUPreview.js
index 1c997119d..b90d9ce42 100644
--- a/src/components/ReportActionItem/IOUPreview.js
+++ b/src/components/ReportActionItem/IOUPreview.js
@@ -119,8 +119,9 @@ const IOUPreview = (props) => {
     const isCurrentUserManager = managerEmail === sessionEmail;
 
     const managerName = lodashGet(props.personalDetails, [managerEmail, 'firstName'], '')
-                        || Str.removeSMSDomain(managerEmail);
-    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') || Str.removeSMSDomain(ownerEmail);
+                        || lodashGet(props.personalDetails, [managerEmail, 'displayName'], '');
+    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') 
+                        || lodashGet(props.personalDetails, [ownerEmail, 'displayName'], '');
     const managerAvatar = lodashGet(props.personalDetails, [managerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(managerEmail);
     const ownerAvatar = lodashGet(props.personalDetails, [ownerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(ownerEmail);
     const cachedTotal = props.iouReport.total && props.iouReport.currency

If first name is empty, then display name will only contains either last name or email.

Or,

we can use an existing function to get the name which basically do the same as above.

diff --git a/src/components/ReportActionItem/IOUPreview.js b/src/components/ReportActionItem/IOUPreview.js
index 1c997119d..7f9844de7 100644
--- a/src/components/ReportActionItem/IOUPreview.js
+++ b/src/components/ReportActionItem/IOUPreview.js
@@ -118,9 +118,8 @@ const IOUPreview = (props) => {
     // Pay button should only be visible to the manager of the report.
     const isCurrentUserManager = managerEmail === sessionEmail;
 
-    const managerName = lodashGet(props.personalDetails, [managerEmail, 'firstName'], '')
-                        || Str.removeSMSDomain(managerEmail);
-    const ownerName = lodashGet(props.personalDetails, [ownerEmail, 'firstName'], '') || Str.removeSMSDomain(ownerEmail);
+    const managerName = ReportUtils.getDisplayNameForParticipant(managerEmail, true)
+    const ownerName = ReportUtils.getDisplayNameForParticipant(ownerEmail, true)
     const managerAvatar = lodashGet(props.personalDetails, [managerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(managerEmail);
     const ownerAvatar = lodashGet(props.personalDetails, [ownerEmail, 'avatar']) || ReportUtils.getDefaultAvatar(ownerEmail);
     const cachedTotal = props.iouReport.total && props.iouReport.currency

@sobitneupane
Copy link
Contributor

@robertjchen We might need to handle it backend (at least partially) since change in IOUQuote message might not be possible at the front.

@melvin-bot melvin-bot bot added the Overdue label Jan 16, 2023
@robertjchen
Copy link
Contributor

@sobitneupane Got it! That makes sense, I can look into the backend changes- appreciate the discussion here. @bernhardoj 's updated proposal looks good to me 👍

@melvin-bot melvin-bot bot removed the Overdue label Jan 16, 2023
@sobitneupane
Copy link
Contributor

Yes, for the frontend change we can go with @bernhardoj's proposal of using existing ReportUtils.getDisplayNameForParticipant() function.

@MitchExpensify
Copy link
Contributor

@bernhardoj
Copy link
Contributor

Should I open the PR now?

@sobitneupane @robertjchen

@MitchExpensify
Copy link
Contributor

Yes @bernhardoj

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jan 19, 2023
@bernhardoj
Copy link
Contributor

Okay. I thought I need to wait for the assignment first 😅. PR is ready.

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

melvin-bot bot commented Jan 20, 2023

📣 @bernhardoj You have been assigned to this job by @MitchExpensify!
Please apply to this job in Upwork 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 📖

@robertjchen
Copy link
Contributor

Looks like the change went out to prod last week, going to remove the Reviewing label to move this forward to the next step of the process.

@robertjchen robertjchen removed the Reviewing Has a PR in review label Feb 2, 2023
@robertjchen
Copy link
Contributor

robertjchen commented Feb 2, 2023

It seems like we may be past the 7-day regression period as I don't see the checklist? cc: @MitchExpensify for clarification on next steps 🙏

@MitchExpensify
Copy link
Contributor

All paid out! Thanks everyone

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 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

7 participants