Correctly display first name for IOU requests by Gonals · Pull Request #12184 · Expensify/App

Gonals · 2022-10-27T14:17:32Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/233534

Tests

Log into your account and start a chat with someone that has a name in place
Go offline
Request money from them.
Confirm that in the greyed-out request, their first name shows up:

5. Repeat steps 1-5 but requesting money from an account with no name. 6. Confirm the email displays instead:

Verify that no errors appear in the JS console

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Desktop

iOS

Android

Gonals · 2022-10-27T14:18:07Z

Having some trouble testing in mobile, but it should work fine across platforms. Trying again tomorrow, anyways

Santhosh-Sellavel · 2022-10-27T18:22:59Z

src/libs/actions/IOU.js

 * @param {String} comment
 */
-function requestMoney(report, amount, currency, recipientEmail, debtorEmail, comment) {
+function requestMoney(report, amount, currency, recipientEmail, debtorEmail, debtorName = '', comment = '') {


Not sure why we need default empty values here.

I don't think they'll always be available. Mostly a sanity check

This function is used only once and already state.comment initial value is '' and fallback value for debatorName is also '' i.e if firstName is missing then we passed '' as default value in lodash

That's fair. Removed.

youssef-lr

LGTM! Tested well.

youssef-lr · 2022-10-28T17:44:29Z

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Santhosh-Sellavel · 2022-10-30T20:07:21Z

Screen Recordings

~~Not unable to run android waiting for this issue~~
Able to run using workaround here

Details

Web & Desktop

With Name

DesktopWeb_WithName.mov

With Email

DesktopWebWithEmail.mov

Android & iOS mWeb

With Name

#### With Email

iOS Native

Both Cases

Simulator.Screen.Recording.-.iPhone.13.-.2022-10-31.at.00.18.15.mp4

###Android Native

With Name

With Email

Santhosh-Sellavel · 2022-10-30T20:10:49Z

PR Reviewer Checklist

Waiting to Test on android

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- [ x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Santhosh-Sellavel

Other wise looks good! Need to test on android #12184 (comment)

Santhosh-Sellavel

Screenshots: #12184 (comment)
Checklist: #12184 (comment)

LGTM!
C+ Reviewed
🎀 👀 🎀

Santhosh-Sellavel · 2022-11-02T20:11:56Z

src/pages/iou/IOUModal.js

selectedParticipants[0] or this.state.participants[0]

selectedParticipants[0] works :)

Gonals · 2022-11-03T09:30:13Z

Large changes after conflict resolution, but it is way simpler now.

Santhosh-Sellavel

Looks good, tests pass!
all you @youssef-lr

Gonals · 2022-11-04T12:14:18Z

Conflicts solved (again!).
@youssef-lr, let's do this!

youssef-lr · 2022-11-04T12:43:54Z

src/libs/ReportUtils.js

    const amount = NumberFormatUtils.format(preferredLocale, total / 100, {style: 'currency', currency});
-    const isMultipleParticipantReport = participants.length > 1;
-    const displayNames = _.map(participants, participant => getDisplayNameForParticipant(participant.login, isMultipleParticipantReport) || participant.login);
+    const displayNames = _.map(participants, participant => getDisplayNameForParticipant(allPersonalDetails[participant.login], true) || participant.login);


This is a throwing a text.replace is not function for me. Due to the change here, getDisplayNameForParticipant now expects a string instead of the participant object.

Oh, you are completely right!

Fixed and tested. Seems to be working!

youssef-lr · 2022-11-04T12:44:22Z

Left a comment @Gonals

Gonals · 2022-11-04T13:46:56Z

Left a comment @Gonals

Addressed! All yours!

youssef-lr

LGTM and tests well!

melvin-bot · 2022-11-04T14:14:00Z

@youssef-lr looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

OSBotify · 2022-11-04T14:20:40Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-11-04T17:51:50Z

🚀 Deployed to staging by @youssef-lr in version: 1.2.24-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-11-08T19:37:58Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.24-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-11-08T21:07:31Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.24-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-11-08T21:37:51Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.24-4 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Santhosh-Sellavel · 2022-11-08T21:45:49Z

@Gonals Can you tag a BZ here, to track payment for C+ Review?

For all PRs that are linked to E/E issues, C+ comments and tags any assigned Expensify engineers in the PR asking them to check the E/E issue.

If the E/E issue is a bug, a BZ is already assigned, internal engineer cc's the BZ in a comment on the PR so C+ knows who is paying.

If the E/E isn't a bug, internal engineers add the Bug label to assign one, then engineer comments on the PR tagging the BZ who's assigned to issue payment.

Gonals · 2022-11-09T09:48:41Z

@flaviadefaria 👆

youssef-lr · 2024-04-01T20:29:59Z

Not sure why the checks show that the workflow didn't run, but this wasn't an emergency.

Gonals added 3 commits October 27, 2022 15:21

Pass full participant array to retrieve the info as needed

540372e

pass the name separately

6b23844

remove unneeded import

bf18a9a

Gonals requested a review from a team as a code owner October 27, 2022 14:17

Gonals self-assigned this Oct 27, 2022

melvin-bot bot requested review from Santhosh-Sellavel and youssef-lr and removed request for a team October 27, 2022 14:18

Santhosh-Sellavel reviewed Oct 27, 2022

View reviewed changes

remove defaults

2fab9c1

youssef-lr previously approved these changes Oct 28, 2022

View reviewed changes

Santhosh-Sellavel reviewed Oct 30, 2022

View reviewed changes

Santhosh-Sellavel previously approved these changes Oct 31, 2022

View reviewed changes

Merge main and conflicts

d3eae9a

Gonals dismissed stale reviews from Santhosh-Sellavel and youssef-lr via d3eae9a November 2, 2022 11:47

Gonals added 2 commits November 2, 2022 16:50

always use shortName when available

5152034

keep parameter

56517f3

Santhosh-Sellavel reviewed Nov 2, 2022

View reviewed changes

Santhosh-Sellavel previously approved these changes Nov 3, 2022

View reviewed changes

conflicts

df2e7c9

youssef-lr reviewed Nov 4, 2022

View reviewed changes

pass the right param

04945cb

Gonals dismissed Santhosh-Sellavel’s stale review via 04945cb November 4, 2022 13:46

youssef-lr approved these changes Nov 4, 2022

View reviewed changes

youssef-lr merged commit a2eda4d into main Nov 4, 2022

youssef-lr deleted the alberto-name branch November 4, 2022 14:13

melvin-bot bot added the Emergency label Nov 4, 2022

OSBotify mentioned this pull request Nov 4, 2022

Deploy Checklist: New Expensify 2022-11-04 #12476

Closed

20 tasks

kbecciv mentioned this pull request Nov 4, 2022

[$250] New Room - Incorrect error message is displayed for the empty field under room name. #12482

Closed

youssef-lr removed the Emergency label Apr 1, 2024

Conversation

Gonals commented Oct 27, 2022

Details

Fixed Issues

Tests

QA Steps

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Desktop

iOS

Android

Uh oh!

Gonals commented Oct 27, 2022

Uh oh!

Santhosh-Sellavel Oct 27, 2022

Choose a reason for hiding this comment

Uh oh!

Gonals Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gonals Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

youssef-lr left a comment

Choose a reason for hiding this comment

Uh oh!

youssef-lr commented Oct 28, 2022

Uh oh!

Santhosh-Sellavel commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screen Recordings

Web & Desktop

With Name

With Email

Android & iOS mWeb

With Name

iOS Native

Both Cases

With Name

With Email

Uh oh!

Santhosh-Sellavel commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Checklist

Uh oh!

Santhosh-Sellavel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

Santhosh-Sellavel Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Gonals Nov 3, 2022

Choose a reason for hiding this comment

Uh oh!

Gonals commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

Uh oh!

Gonals commented Nov 4, 2022

Uh oh!

youssef-lr Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

Gonals Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

youssef-lr commented Nov 4, 2022

Santhosh-Sellavel Oct 28, 2022 •

edited

Loading

Santhosh-Sellavel commented Oct 30, 2022 •

edited

Loading

Santhosh-Sellavel commented Oct 30, 2022 •

edited

Loading

Santhosh-Sellavel left a comment •

edited

Loading

Gonals commented Nov 3, 2022 •

edited

Loading