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

fix iou preview display name #14408

Merged
merged 2 commits into from
Jan 20, 2023
Merged

fix iou preview display name #14408

merged 2 commits into from
Jan 20, 2023

Conversation

bernhardoj
Copy link
Contributor

Details

IOU preview show email address when the user does not have first name. We should instead show the user last name if present.

Fixed Issues

$ #14228
PROPOSAL: #14228 (comment)

Tests

  1. Open chat of a user that has set only last name (empty first name)
  2. Click on the + icon
  3. Click on any IOU action
  4. Complete the process
  5. Verify the user name on the IOU preview is the last name
  • Verify that no errors appear in the JS console

Offline tests

  1. Open chat of a user that has set only last name (empty first name)
  2. Click on the + icon
  3. Click on any IOU action
  4. Complete the process
  5. Verify the user name on the IOU preview is the last name

QA Steps

  1. Open chat of a user that has set only last name (empty first name)
  2. Click on the + icon
  3. Click on any IOU action
  4. Complete the process
  5. Verify the user name on the IOU preview is the last name
  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (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 is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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 followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • 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
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

User with last name only

image
image

Web

image

Mobile Web - Chrome

328996

Mobile Web - Safari

image

Desktop

image

iOS

image

Android

328995

@bernhardoj bernhardoj requested a review from a team as a code owner January 19, 2023 03:49
@melvin-bot melvin-bot bot requested review from robertjchen and sobitneupane and removed request for a team January 19, 2023 03:49
@melvin-bot
Copy link

melvin-bot bot commented Jan 19, 2023

@robertjchen @sobitneupane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@sobitneupane
Copy link
Contributor

Changes are looking good. I will review it first hour tomorrow (NPT).

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshots/Videos

Web Screenshot 2023-01-20 at 13 16 03 Screenshot 2023-01-20 at 13 13 12
Mobile Web - Chrome
Mobile Web - Safari
Desktop Screenshot 2023-01-20 at 13 18 40
iOS
Android

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer Checklist

  • 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:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • 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 is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • 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 any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • 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.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Copy link
Contributor

@robertjchen robertjchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, thanks!!

@robertjchen robertjchen merged commit 8cd50c8 into Expensify:main Jan 20, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 710.032 ms → 733.810 ms (+23.778 ms, +3.3%)
App start runJsBundle 193.226 ms → 201.500 ms (+8.274 ms, +4.3%)
Open Search Page TTI 610.543 ms → 611.746 ms (+1.202 ms, ±0.0%)
App start regularAppStart 0.015 ms → 0.022 ms (+0.007 ms, +49.0%) 🟡
App start nativeLaunch 20.469 ms → 19.667 ms (-0.802 ms, -3.9%)
Show details
Name Duration
App start TTI Baseline
Mean: 710.032 ms
Stdev: 31.190 ms (4.4%)
Runs: 650.1919560004026 662.8488309998065 664.8849050002173 675.6261210003868 679.0567300003022 679.2504439996555 686.1590309999883 690.5506650004536 691.2651779996231 692.3122680000961 694.4116730000824 695.6261390000582 699.0486289998516 699.2236200002953 699.3787690000609 701.215933999978 713.7428139997646 714.2254590000957 714.8414089996368 715.4433899996802 717.0594819998369 725.4897699998692 727.9953229995444 732.9999590003863 741.1095740003511 744.4624420003965 745.4090099995956 749.3791190003976 765.1474930001423 769.749947999604 772.8738989997655

Current
Mean: 733.810 ms
Stdev: 32.210 ms (4.4%)
Runs: 683.6027410002425 691.014906000346 692.5458580004051 696.0847559999675 697.8608520003036 700.1788689997047 706.657518000342 706.9715369995683 708.0055510001257 712.7747139995918 712.7997639998794 715.6768260002136 719.3574860002846 719.4600029997528 722.1283670002595 723.4770360002294 727.2013429999352 728.367425000295 742.0801269998774 745.0568469995633 746.0127170002088 746.4581760000437 752.7818160001189 753.6878540003672 760.7938419999555 763.5423090001568 771.2177839996293 777.9876739997417 781.4160940004513 782.1922679999843 784.0611220002174 810.4665540000424
App start runJsBundle Baseline
Mean: 193.226 ms
Stdev: 17.836 ms (9.2%)
Runs: 168 168 171 172 173 176 176 177 181 181 182 185 188 190 191 192 193 195 195 195 195 202 202 204 205 209 219 219 222 230 234

Current
Mean: 201.500 ms
Stdev: 13.288 ms (6.6%)
Runs: 179 183 184 187 188 189 189 190 190 191 193 194 197 197 197 198 201 201 202 205 206 207 208 210 214 214 215 219 222 224 225 229
Open Search Page TTI Baseline
Mean: 610.543 ms
Stdev: 17.680 ms (2.9%)
Runs: 580.1081139994785 581.3071699999273 581.7581380000338 590.0399980004877 591.3382169995457 594.8717050002888 595.2813320001587 596.6526290001348 597.2690030001104 600.3828539997339 600.6301680002362 601.8978279996663 605.529826999642 606.1708169998601 606.7478029998019 608.5635989997536 609.6970629999414 612.2027190001681 613.4649660000578 613.5793059999123 616.8667399995029 617.0544030005112 618.6781409997493 622.9423830006272 624.2613519998267 625.2798259994015 628.5707609998062 630.5767830004916 632.4328209999949 639.2317300001159 645.9687909996137 648.0330809997395

Current
Mean: 611.746 ms
Stdev: 12.277 ms (2.0%)
Runs: 589.2377939997241 594.2174479998648 594.9984949994832 595.8747969996184 597.2078449996188 598.0217699995264 598.8612879998982 600.6404629992321 601.2833259999752 603.484864000231 604.9731860002503 605.8978280005977 606.8557540001348 608.4639490004629 608.6675619995221 608.9563400000334 610.0366620002314 610.8486740002409 613.0740149999037 614.2449950007722 617.2106119999662 618.7420250000432 618.9429940003902 619.4204919999465 621.5533039998263 622.6872560000047 623.6043299995363 626.5150560000911 626.6503090001643 626.9403480002657 630.4553629998118 632.0599370002747 636.9848629999906
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (6.5%)
Runs: 0.013184000737965107 0.013264999724924564 0.013590999878942966 0.0139979999512434 0.01403799932450056 0.014038000255823135 0.014201000332832336 0.014240999706089497 0.014403999783098698 0.01444500032812357 0.014566999860107899 0.01464799977838993 0.014649000018835068 0.014770999550819397 0.014771000482141972 0.014810999855399132 0.014812000095844269 0.0148930000141263 0.014973999932408333 0.014973999932408333 0.015339999459683895 0.015339999459683895 0.01534000039100647 0.015381000004708767 0.015583999454975128 0.01587000023573637 0.016315999440848827 0.01660200022161007 0.016927000135183334 0.017333999276161194

Current
Mean: 0.022 ms
Stdev: 0.002 ms (7.0%)
Runs: 0.019164999946951866 0.01948999986052513 0.019775000400841236 0.0204670000821352 0.02054900024086237 0.020832999609410763 0.0210359999909997 0.021076999604701996 0.02115900069475174 0.021239999681711197 0.021280999295413494 0.021606999449431896 0.022094999440014362 0.022135999985039234 0.022135999985039234 0.02250200044363737 0.02254300005733967 0.022583000361919403 0.0227870000526309 0.022909000515937805 0.023031000047922134 0.023275000043213367 0.02339700050652027 0.023559999652206898 0.02360100019723177 0.02364099957048893 0.02392599917948246 0.02396600041538477 0.024821000173687935 0.0258780000731349
App start nativeLaunch Baseline
Mean: 20.469 ms
Stdev: 2.436 ms (11.9%)
Runs: 17 17 18 18 18 18 18 18 18 19 19 19 20 20 20 20 20 20 20 20 21 21 22 22 23 23 23 23 24 25 25 26

Current
Mean: 19.667 ms
Stdev: 1.398 ms (7.1%)
Runs: 17 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 21 21 21 22 22 22 23

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/robertjchen in version: 1.2.57-1 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.57-3 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants