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 miss-matched SMS email suffix on sign in page #15351

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

Julesssss
Copy link
Contributor

@Julesssss Julesssss commented Feb 22, 2023

Details

When using an SMS login, the *@expensify.sms suffix was appearing on the sign-up page when entering a phone number. There was a separate component beneath it that didn't have the same issue, so I simply aligned them.

Screenshot 2023-02-22 at 13 37 53 Screenshot 2023-02-22 at 13 36 53

Fixed Issues

$ #15291

Tests

1) Verify that no errors appear in the JS console

  • Self explanatory

2) Confirm SMS login is aligned and has no suffix

  • Sign out
  • Sign in using an SMS number (for example, +17474744747)
  • The welcome message should not include the @expensify.sms suffix
  • Both the welcome message and the 'Not XYZ, go back' messages should show the same login

Offline tests

QA Steps

  • 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web Screenshot 2023-02-22 at 17 37 23
Mobile Web - Chrome

and-web

Mobile Web - Safari

Simulator Screen Shot - iPhone 14 Pro - 2023-02-23 at 15 11 15

Desktop Screenshot 2023-02-22 at 18 07 32
iOS

Simulator Screen Shot - iPhone 14 Pro - 2023-02-23 at 15 19 38

Android

Screenshot_20230223_152846

@Julesssss Julesssss marked this pull request as ready for review February 23, 2023 15:30
@Julesssss Julesssss requested a review from a team as a code owner February 23, 2023 15:30
@melvin-bot melvin-bot bot requested review from aldo-expensify and mollfpr and removed request for a team February 23, 2023 15:31
@MelvinBot
Copy link

@mollfpr @aldo-expensify 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]

@mollfpr
Copy link
Contributor

mollfpr commented Feb 23, 2023

Redviewer Checklist (aldo: introduced a typo so the PR reviewer check skips this one)

  • 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@mollfpr
Copy link
Contributor

mollfpr commented Feb 23, 2023

@Julesssss I got an error when testing it with the same phone number as yours. Any idea?

Screen Shot 2023-02-23 at 23 51 41

@Julesssss
Copy link
Contributor Author

Julesssss commented Feb 23, 2023

@Julesssss I got an error when testing it with the same phone number as yours. Any idea?

Ah yeah, I had to switch country codes a few times. Can you try using a country code and valid phone number for your current country, and also the US? In my case, US phone numbers would show the same message you see, but not UK numbers. Except when I tested on an Emulator 🙄

UK: +447403666666
US: +12069575544

@aldo-expensify
Copy link
Contributor

@mollfpr if you are having problems with testing with phone numbers, I can take the screenshots. As Jules said, try with a phone from your country.

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Feb 23, 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.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

image

Mobile Web - Chrome

image

Mobile Web - Safari

image

Desktop image
iOS

Screenshot 2023-02-23 at 11 51 56 AM

Android

Screenshot 2023-02-23 at 11 41 15 AM

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mollfpr mollfpr left a comment

Choose a reason for hiding this comment

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

I can't validate my phone number because I never received the SMS for the validation, but I did the regression test where I logged in with an email and didn't find any issues. Such good work on this @Julesssss 👍

@Julesssss Julesssss merged commit a501c9c into main Feb 24, 2023
@Julesssss Julesssss deleted the jules-fixAndAlignSMSSuffix branch February 24, 2023 11:25
@OSBotify
Copy link
Contributor

✋ 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

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 610.710 ms → 652.087 ms (+41.377 ms, +6.8%)
App start TTI 703.482 ms → 708.597 ms (+5.115 ms, +0.7%)
App start nativeLaunch 18.714 ms → 19.656 ms (+0.942 ms, +5.0%)
App start regularAppStart 0.013 ms → 0.014 ms (+0.000 ms, +3.2%)
App start runJsBundle 196.625 ms → 194.531 ms (-2.094 ms, -1.1%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 610.710 ms
Stdev: 24.646 ms (4.0%)
Runs: 570.9280600007623 574.4701339993626 577.1420090002939 577.8308509998024 584.5204670000821 586.5147709995508 587.115559999831 588.806925999932 591.1467289999127 596.3693039994687 597.8120929999277 599.669719000347 602.0743009997532 602.2612310005352 607.4955649999902 608.6704099997878 610.522746999748 613.0483809998259 614.4308270001784 620.406780000776 620.4645589999855 624.2248949995264 624.5583089999855 625.3873700005934 626.3528650002554 629.1733399992809 632.6245120000094 633.399943000637 635.3522140001878 639.2304279999807 664.3197429999709 676.3966479999945

Current
Mean: 652.087 ms
Stdev: 26.235 ms (4.0%)
Runs: 617.5347090000287 618.1079510003328 618.2770589999855 618.7331140004098 621.2358400002122 621.8266599997878 621.9800619995221 626.4548340002075 626.9504399998114 627.2461759997532 627.2575280005112 632.8941250005737 636.5152589995414 646.0799970002845 649.2765309996903 649.5410159993917 651.3751220004633 654.2875169999897 654.4022629996762 655.5010580001399 667.9995940001681 669.3680419996381 672.2302249995992 672.2401529997587 673.3842780003324 673.9425870003179 674.972574999556 678.3753669997677 678.703246999532 685.9931240007281 686.3475750004873 703.2162689995021 706.6197920003906
App start TTI Baseline
Mean: 703.482 ms
Stdev: 19.645 ms (2.8%)
Runs: 667.8933049999177 671.1777119999751 676.2483630003408 677.7111010001972 684.4844810003415 685.1065159998834 686.9745950000361 688.6647969996557 689.8500640001148 691.2353339996189 692.2271490003914 693.4479270000011 698.5807410003617 700.7457659998909 701.3842979995534 704.2505980003625 704.9490759996697 708.9889869997278 710.180773999542 710.7399829998612 712.4902560003102 712.5003000004217 713.1146299997345 713.8452819995582 717.9280150001869 719.8587880004197 723.7982430001721 726.3455699998885 729.2429820001125 739.7507360000163 754.215991999954

Current
Mean: 708.597 ms
Stdev: 31.332 ms (4.4%)
Runs: 656.2123220004141 663.5877710003406 668.8467939998955 669.5979770002887 673.0774550000206 673.3731810003519 678.0251320004463 683.425406999886 683.9716630000621 689.0834379997104 691.6323039997369 693.4928299998865 696.3008970003575 697.6172960000113 701.9019830003381 707.542016999796 708.8009219998494 709.6829129997641 713.9180979998782 714.3672970002517 717.2890309998766 718.2175460001454 720.6055129999295 725.4137260001153 728.3801239999011 741.643504999578 742.1497790003195 748.9297580001876 753.4188510002568 763.9199059996754 764.5623120004311 776.1020649997517
App start nativeLaunch Baseline
Mean: 18.714 ms
Stdev: 1.508 ms (8.1%)
Runs: 17 17 17 17 17 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 21 22 23

Current
Mean: 19.656 ms
Stdev: 2.258 ms (11.5%)
Runs: 17 17 17 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 21 21 22 22 22 22 22 24 25 25
App start regularAppStart Baseline
Mean: 0.013 ms
Stdev: 0.001 ms (7.2%)
Runs: 0.01139300037175417 0.012329000048339367 0.012329000048339367 0.012450999580323696 0.012492000125348568 0.012532000429928303 0.012655000202357769 0.012655000202357769 0.012897999957203865 0.012980000115931034 0.013020999729633331 0.013061000034213066 0.013102000579237938 0.013143000192940235 0.013143000192940235 0.013143000192940235 0.013224000111222267 0.013224000111222267 0.0133050000295043 0.013346999883651733 0.0134680001065135 0.013509999960660934 0.013712000101804733 0.013794000260531902 0.014404000714421272 0.014405000023543835 0.014851999469101429 0.014852000400424004 0.014973999932408333 0.015217999927699566 0.015217999927699566 0.015217999927699566

Current
Mean: 0.014 ms
Stdev: 0.001 ms (5.1%)
Runs: 0.012206999585032463 0.012736000120639801 0.012938999570906162 0.012980000115931034 0.01314299926161766 0.013265000656247139 0.013387000188231468 0.013508999720215797 0.013508999720215797 0.013508999720215797 0.013509000651538372 0.01355000026524067 0.013631000183522701 0.013631999492645264 0.013711999170482159 0.013712000101804733 0.01371300034224987 0.01371300034224987 0.013834000565111637 0.014077999629080296 0.014077999629080296 0.014322999864816666 0.014364000409841537 0.014364000409841537 0.014566999860107899 0.014566999860107899 0.01464799977838993 0.014649000018835068 0.014810999855399132 0.014811999164521694 0.015096000395715237 0.015421000309288502
App start runJsBundle Baseline
Mean: 196.625 ms
Stdev: 21.056 ms (10.7%)
Runs: 166 168 169 173 174 175 176 178 180 181 182 185 188 189 190 196 197 197 201 203 204 204 205 207 212 213 214 221 225 232 236 251

Current
Mean: 194.531 ms
Stdev: 20.495 ms (10.5%)
Runs: 164 167 169 173 176 177 178 179 179 180 180 181 185 185 186 189 190 192 194 195 199 201 201 203 212 217 222 223 223 230 237 238

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/Julesssss in version: 1.2.77-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/Julesssss in version: 1.2.77-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.77-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 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.

5 participants