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

Update feedback message when the link to get a magic code is resent #15573

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

pecanoro
Copy link
Contributor

@pecanoro pecanoro commented Feb 28, 2023

Details

Fixed Issues

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

Tests

  1. Go to the login page and enter an Expensify email (since passwordless is not enabled for other domains)
  2. When the prompt to enter the magic code shows up, click on "Didn't receive a magic code? right"
  3. Once the email is sent, the link message should get updated to the image below and it should be non-clickable:

Monosnap New Expensify 2023-02-28 15-52-33

  1. Repeat the process with an account that is not under the beta, instead of asking for magic code, it should ask for the password and clicking Forgot? should take you to the following screen:

Monosnap New Expensify 2023-02-28 16-06-50

  • Verify that no errors appear in the JS console

Offline tests

N/A, you can't log in without internet.

QA Steps

  1. Go to the login page and enter an Expensify email (since passwordless is not enabled for other domains)
  2. When the prompt to enter the magic code shows up, click on "Didn't receive a magic code? right"
  3. Once the email is sent, the link message should get updated to the image below and it should be non-clickable:

Monosnap New Expensify 2023-02-28 15-52-33

  1. Repeat the process with an account that is not under the beta, instead of asking for magic code, it should ask for the password and clicking Forgot? should take you to the following screen:

Monosnap New Expensify 2023-02-28 16-06-50

  • 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 Monosnap New Expensify 2023-02-28 15-52-33
Mobile Web - Chrome

image

Mobile Web - Safari

image

Desktop Monosnap New Expensify 2023-02-28 15-52-33
iOS

image

Android

image

@pecanoro pecanoro requested a review from a team as a code owner February 28, 2023 21:00
@pecanoro pecanoro self-assigned this Feb 28, 2023
@pecanoro pecanoro changed the title Update feedback message wen the link is sent Update feedback message when the link to get a magic code is resent Feb 28, 2023
@melvin-bot melvin-bot bot requested review from Santhosh-Sellavel and stitesExpensify and removed request for a team February 28, 2023 21:00
@MelvinBot
Copy link

@Santhosh-Sellavel @stitesExpensify 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]

@pecanoro
Copy link
Contributor Author

@Santhosh-Sellavel removing you from the review as this feature is under a beta and can't be tested without an Expensify domain.

@pecanoro pecanoro removed the request for review from Santhosh-Sellavel February 28, 2023 21:01
@Santhosh-Sellavel
Copy link
Collaborator

I have the beta access to test this got it while testing other PR, let me know if need to test it thanks!

@pecanoro
Copy link
Contributor Author

Ahh perfect, then I can assign you back, but since the original issue is in our private repo, I think we need to find a person from our BugZero team to create an Upwork job for you so we can pay you.

@pecanoro pecanoro requested review from Santhosh-Sellavel and removed request for stitesExpensify February 28, 2023 21:33
@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Feb 28, 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
Screen.Recording.2023-03-01.at.4.43.15.AM.mov
Desktop
Screen.Recording.2023-03-01.at.4.35.24.AM.mov
Desktop
Screen.Recording.2023-03-01.at.4.35.24.AM.mov
Mobile Web - Safari & iOS
Simulator.Screen.Recording.-.iPhone.14.-.2023-03-01.at.04.45.43.mp4
Mweb & Android
Screen_Recording_20230301_045323_New.Expensify.mp4

@Santhosh-Sellavel
Copy link
Collaborator

@pecanoro I'm just confused with the test steps here.

Is test step 2 incomplete?

Click on Didn't receive a magic code? right?

@Santhosh-Sellavel
Copy link
Collaborator

@pecanoro I'm just confused with the test steps here.

Is test step 2 incomplete?

Click on Didn't receive a magic code? right?

@NikkiWines After seeing the changes I get it we should just update the test step 2 For both the Tests & QA Steps as

  1. When the prompt to enter the magic code shows up, click on Didn't receive a magic code?

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

Need to update the test steps as said here, otherwise looks good, Tests well. So approving! Over to you @NikkiWines

@MelvinBot
Copy link

🎯 @Santhosh-Sellavel, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #15577.

@pecanoro
Copy link
Contributor Author

pecanoro commented Mar 1, 2023

Ah yes, sorry about that, I updated the description

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

Looks good! Sorry for the delay here

@NikkiWines NikkiWines merged commit 7f74d55 into main Mar 1, 2023
@NikkiWines NikkiWines deleted the rocio-CodeSent branch March 1, 2023 22:47
@OSBotify
Copy link
Contributor

OSBotify commented Mar 1, 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 Mar 1, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 607.820 ms → 662.256 ms (+54.436 ms, +9.0%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 607.820 ms
Stdev: 28.591 ms (4.7%)
Runs: 571.3081059996039 576.8487560003996 579.0441490001976 581.1313480008394 583.3129470013082 584.3261309992522 585.0629879999906 585.7530519999564 588.7519940007478 589.3322760015726 590.6127119995654 591.2939050011337 592.7944339998066 592.9698489997536 593.4388839993626 597.4831949993968 601.0632729995996 603.1744790002704 604.5167240016162 604.8161619994789 606.3441160004586 606.6393240001053 607.0866710003465 612.4496260005981 613.0384929999709 630.9583339989185 635.984659999609 637.8573819994926 645.2267259992659 648.7012120001018 658.8964030016214 659.0296630002558 698.8029379993677

Current
Mean: 662.256 ms
Stdev: 25.909 ms (3.9%)
Runs: 616.8557130005211 619.6830649990588 621.1290689986199 622.0175379998982 622.419067999348 623.4106040000916 636.6656909994781 643.4554040003568 644.7600100003183 651.097657000646 654.9269210007042 655.0533450003713 658.6174730006605 659.5846359990537 663.5834149997681 664.9858810007572 665.4436039999127 666.93827399984 667.6026209983975 667.8243819996715 668.9921470005065 677.5302740000188 677.8547359984368 677.9281419999897 678.7632650006562 680.3579920008779 682.0402429997921 683.3365070000291 687.4279379993677 692.890298999846 698.5072840005159 699.3591309990734 723.4083669986576

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 716.190 ms → 721.554 ms (+5.364 ms, +0.7%)
App start regularAppStart 0.014 ms → 0.014 ms (+0.000 ms, +2.4%)
App start nativeLaunch 19.710 ms → 19.645 ms (-0.065 ms, ±0.0%)
App start runJsBundle 198.438 ms → 195.469 ms (-2.969 ms, -1.5%)
Show details
Name Duration
App start TTI Baseline
Mean: 716.190 ms
Stdev: 36.050 ms (5.0%)
Runs: 638.7017509993166 667.5332180000842 675.3425149992108 675.3548930007964 679.5998149998486 686.1282599996775 693.0071760006249 694.7122019995004 697.4678320009261 697.9144269991666 698.0105520002544 699.2235189992934 703.363838000223 705.7039199993014 705.8973500002176 707.6983610000461 707.7873700000346 710.1691689994186 711.1632780004293 711.2767130006105 722.4967369996011 726.2565579991788 733.0908790007234 738.7614050004631 739.201806999743 746.6799950003624 753.6709390003234 756.4051830004901 769.6926630008966 773.2085909992456 782.7215270008892 809.8324950002134

Current
Mean: 721.554 ms
Stdev: 30.798 ms (4.3%)
Runs: 664.199598999694 674.5661040004343 684.4783420003951 684.8241559993476 690.2540809996426 691.8498330004513 692.2417280003428 693.9849839992821 698.2604109998792 699.1182380001992 705.143982000649 705.5227269995958 711.448545999825 713.4090679995716 714.9707180000842 719.7655410002917 722.905523000285 723.568245999515 725.9859769996256 727.8534559998661 729.094808999449 738.5400869995356 739.9534949995577 740.3563189990819 743.8008880000561 746.5556499995291 754.2934399992228 756.5424240007997 760.2688560001552 763.7425040006638 767.0230880007148 805.2067489996552
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.4%)
Runs: 0.011962998658418655 0.012776000425219536 0.013020999729633331 0.013101998716592789 0.013305999338626862 0.013345999643206596 0.01342799887061119 0.01342799887061119 0.013509001582860947 0.01355000026524067 0.01355000026524067 0.013630999252200127 0.013630999252200127 0.013630999252200127 0.013794001191854477 0.013916000723838806 0.013996999710798264 0.014079000800848007 0.014159999787807465 0.014241000637412071 0.014242000877857208 0.014322999864816666 0.014323001727461815 0.014485999941825867 0.014485999941825867 0.01476999931037426 0.01477000117301941 0.014852000400424004 0.014973999932408333 0.015829000622034073

Current
Mean: 0.014 ms
Stdev: 0.001 ms (6.2%)
Runs: 0.012817000970244408 0.012938998639583588 0.012938998639583588 0.01314299926161766 0.013182999566197395 0.013183999806642532 0.013224000111222267 0.01342800073325634 0.01354999840259552 0.01355000026524067 0.013875000178813934 0.013876000419259071 0.013915998861193657 0.014079000800848007 0.0142000000923872 0.014242000877857208 0.01440499909222126 0.014485999941825867 0.014485999941825867 0.014649000018835068 0.014689000323414803 0.014689000323414803 0.014810999855399132 0.014932999387383461 0.014933999627828598 0.015095999464392662 0.015096001327037811 0.015461999922990799 0.015705998986959457 0.015706000849604607 0.01607299968600273
App start nativeLaunch Baseline
Mean: 19.710 ms
Stdev: 1.904 ms (9.7%)
Runs: 17 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 20 20 20 20 20 20 21 21 21 22 22 22 23 24 24

Current
Mean: 19.645 ms
Stdev: 1.944 ms (9.9%)
Runs: 17 17 17 17 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 21 21 21 21 21 22 22 23 24 24
App start runJsBundle Baseline
Mean: 198.438 ms
Stdev: 22.887 ms (11.5%)
Runs: 153 169 172 173 174 175 176 176 181 183 184 187 187 191 192 193 194 202 203 211 213 213 214 215 217 219 219 222 222 225 237 258

Current
Mean: 195.469 ms
Stdev: 15.867 ms (8.1%)
Runs: 167 172 175 177 178 179 181 182 182 184 186 188 188 190 191 193 193 194 195 199 201 205 207 211 211 215 215 215 217 218 221 225

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Mar 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@NikkiWines
Copy link
Contributor

Already in discussion here

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

🚀 Deployed to staging by https://github.com/NikkiWines in version: 1.2.78-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

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
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants