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 double spaces in IOU confirmation message #14768

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

youssef-lr
Copy link
Contributor

Details

Fixed Issues

$ #14737

Tests

  1. Logged in from Account A, request money from Account B.
  2. Pay the money request using either elsewhere, PayPal, or Wallet.
  3. Verify there are no double spaces before the payment method, e.g. Settled up elsewhere
  • Verify that no errors appear in the JS console

Offline tests

Same as above.

QA Steps

  1. Logged in from Account A, request money from Account B.
  2. Pay the money request using either elsewhere, PayPal, or Wallet.
  3. Verify there are no double spaces before the payment method, e.g. Settled up elsewhere
  • 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

Web
Screen.Recording.2023-02-02.at.20.08.02.mov
Mobile Web - Chrome

See recording above, this should work on all platform as we're just changing text.

Mobile Web - Safari

See recording above, this should work on all platform as we're just changing text.

Desktop

See recording above, this should work on all platform as we're just changing text.

iOS

See recording above, this should work on all platform as we're just changing text.

Android

See recording above, this should work on all platform as we're just changing text.

@youssef-lr youssef-lr requested a review from a team as a code owner February 2, 2023 19:13
@youssef-lr youssef-lr self-assigned this Feb 2, 2023
@melvin-bot melvin-bot bot requested review from dangrous and thesahindia and removed request for a team February 2, 2023 19:14
@melvin-bot
Copy link

melvin-bot bot commented Feb 2, 2023

@thesahindia @dangrous 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]

@youssef-lr youssef-lr removed the request for review from thesahindia February 2, 2023 19:15
@youssef-lr
Copy link
Contributor Author

Removing C+ as this is a pretty simple change 😅

Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

This works, but do we want to take this opportunity to translate paymentMethodMessage which isn't currently translated?

Separately, on main when I do this, it appears with the double space first, but then if I move to another view and back (e.g. clicking on the payment details) it then fixes itself and removes the double space. It doesn't seem to mess up this PR, so it's probably fine, but did you experience that and/or do you know why it's happening?

@youssef-lr
Copy link
Contributor Author

This works, but do we want to take this opportunity to translate paymentMethodMessage which isn't currently translated?

Good point, but, the final IOU confirmation message actually comes from the backend, and replaces the optimistic one. Hence if we translate the optimistic message, it will be overwritten by the message coming from the backend. And this also answers your question below:

Separately, on main when I do this, it appears with the double space first, but then if I move to another view and back (e.g. clicking on the payment details) it then fixes itself and removes the double space. It doesn't seem to mess up this PR, so it's probably fine, but did you experience that and/or do you know why it's happening?

The message coming from the backend doesn't have double spaces. Check this function here responsible for creating the IOU confirmation message.

@dangrous
Copy link
Contributor

dangrous commented Feb 2, 2023

Oh, okay - that makes sense. I didn't realize it was coming from two sources and this was just the optimistic one. All good then!

(I can make a separate issue/bug report to translate that text. I wonder if we did it only in English since the grammar might be different in Spanish and so the string replacement won't work.)

@dangrous
Copy link
Contributor

dangrous commented Feb 2, 2023

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.

Screenshots/Videos

Web Screenshot 2023-02-02 at 16 16 31
Mobile Web - Chrome Screenshot 2023-02-02 at 16 19 11
Mobile Web - Safari Screenshot 2023-02-02 at 16 17 53
Desktop Screenshot 2023-02-02 at 16 19 46
iOS
Android Screenshot 2023-02-02 at 16 23 04

@dangrous dangrous merged commit 0d95639 into main Feb 2, 2023
@dangrous dangrous deleted the youssef_fix_double_spaces branch February 2, 2023 21:30
@OSBotify
Copy link
Contributor

OSBotify commented Feb 2, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 695.192 ms → 721.770 ms (+26.578 ms, +3.8%)
Open Search Page TTI 602.734 ms → 606.503 ms (+3.770 ms, +0.6%)
App start runJsBundle 193.438 ms → 196.323 ms (+2.885 ms, +1.5%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +6.8%)
App start nativeLaunch 19.903 ms → 19.844 ms (-0.059 ms, ±0.0%)
Show details
Name Duration
App start TTI Baseline
Mean: 695.192 ms
Stdev: 32.629 ms (4.7%)
Runs: 645.642780999653 650.5323569998145 659.5715889995918 663.2252709995955 664.5063610002398 664.7982900002971 669.3087200000882 670.2573309997097 672.1830070000142 673.2007609996945 673.994857000187 674.4453809997067 676.4115359997377 676.8272679997608 677.2882690001279 680.0058369999751 685.9637359995395 688.3676370000467 699.092326999642 707.0732359997928 708.9524550000206 713.8994000004604 717.9403410004452 718.788034000434 719.342965000309 719.7533290004358 725.3337270002812 726.6470839995891 741.6590740000829 745.1458449997008 748.0061090001836 787.9717960003763

Current
Mean: 721.770 ms
Stdev: 24.553 ms (3.4%)
Runs: 682.7101539997384 693.4999080002308 693.7246970003471 697.6633879998699 698.5246090004221 700.6458740001544 701.446605999954 702.0140270004049 703.1584809999913 703.3141480004415 704.9643230000511 707.6780949998647 707.9596509998664 708.970079000108 709.2504879999906 716.2805599998683 717.4569730004296 720.4564049998298 722.7678610002622 723.4647270003334 731.2019159998745 732.4214369999245 733.615888999775 745.8115199999884 747.9034150000662 750.733264000155 755.9683819999918 756.817363999784 759.807025000453 766.2263960000128 778.4157670000568
Open Search Page TTI Baseline
Mean: 602.734 ms
Stdev: 21.049 ms (3.5%)
Runs: 550.2198489997536 570.7821859996766 581.4061690000817 581.7209479995072 581.9604089995846 581.9801019998267 582.591309000738 584.9629720002413 585.60021999944 585.740885999985 587.7745359996334 592.7450359994546 593.3022870002314 599.2206219993532 601.823283999227 601.8949390007183 601.9980480000377 603.7467860002071 608.5197350000963 609.8006599992514 610.0297849997878 612.2539060004056 613.6484380001202 614.8969329996035 615.0483400002122 619.5422769999132 619.6093350006267 620.1989339999855 620.6175950001925 632.8838300006464 633.5796300005168 639.0892740003765 651.0204269997776

Current
Mean: 606.503 ms
Stdev: 15.339 ms (2.5%)
Runs: 581.375530000776 585.4181320006028 586.0764570003375 587.1144620003179 589.1906329998747 589.8934329999611 592.1104739997536 596.1117759998888 596.9141440000385 597.1958419997245 597.4175220001489 597.6540120001882 598.1257729995996 600.0395109998062 601.7019450003281 602.4608150003478 604.1600750004873 605.734659999609 605.9571939995512 607.6360269999132 609.5814619995654 609.8584389993921 609.9003500007093 610.6536050001159 619.8417159998789 623.6282139997929 624.2006430001929 624.6942140003666 626.6889239996672 627.4463710002601 633.2886960003525 634.5365399997681 637.9981690002605
App start runJsBundle Baseline
Mean: 193.438 ms
Stdev: 23.701 ms (12.3%)
Runs: 162 164 165 169 172 172 175 176 177 177 179 179 179 182 183 188 189 191 191 195 197 198 199 204 209 211 218 222 233 238 244 252

Current
Mean: 196.323 ms
Stdev: 19.649 ms (10.0%)
Runs: 167 169 174 176 176 179 180 180 183 184 185 187 188 188 189 189 191 196 197 200 203 203 205 208 215 216 224 230 231 235 238
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (6.3%)
Runs: 0.012248000130057335 0.01285799965262413 0.013020999729633331 0.0131029998883605 0.013143000192940235 0.013306000269949436 0.013346999883651733 0.013549000024795532 0.013549000024795532 0.013549000024795532 0.013753000646829605 0.013794000260531902 0.013875000178813934 0.013956000097095966 0.014119000174105167 0.014161000028252602 0.014485000632703304 0.014525999315083027 0.014568000100553036 0.014607000164687634 0.01460800040513277 0.014649000018835068 0.014689000323414803 0.014689000323414803 0.014810999855399132 0.014933000318706036 0.014973999932408333 0.014974000863730907 0.015259000472724438 0.015502999536693096 0.0166830001398921

Current
Mean: 0.015 ms
Stdev: 0.001 ms (6.0%)
Runs: 0.013143000192940235 0.013794000260531902 0.0139979999512434 0.0139979999512434 0.014078999869525433 0.014200999401509762 0.014241999946534634 0.014525999315083027 0.014649000018835068 0.014728999696671963 0.0147299999371171 0.014770999550819397 0.014851999469101429 0.014933000318706036 0.015014000236988068 0.015054999850690365 0.01521800085902214 0.0152580002322793 0.015298999845981598 0.015420999377965927 0.015421999618411064 0.015461999922990799 0.015502999536693096 0.015625 0.015828000381588936 0.015868999995291233 0.01590999960899353 0.016114000231027603 0.01619499921798706 0.016560999676585197 0.016805000603199005 0.017292999662458897
App start nativeLaunch Baseline
Mean: 19.903 ms
Stdev: 1.748 ms (8.8%)
Runs: 17 17 18 18 18 18 18 18 18 19 19 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 22 22 23 23 24

Current
Mean: 19.844 ms
Stdev: 1.889 ms (9.5%)
Runs: 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 20 20 20 20 21 21 21 22 22 23 25 25

@OSBotify
Copy link
Contributor

OSBotify commented Feb 2, 2023

🚀 Deployed to staging by https://github.com/dangrous in version: 1.2.64-4 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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

1 similar comment
@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2023

🚀 Deployed to production by https://github.com/thienlnam in version: 1.2.64-7 🚀

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.

3 participants