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.

Sign up for GitHub

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

Jump to bottom

Only show passwordless flow if user is on passwordless or all betas #14730

Merged
merged 3 commits into from
Feb 1, 2023
Merged

Only show passwordless flow if user is on passwordless or all betas #14730

merged 3 commits into from
Feb 1, 2023

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Feb 1, 2023 •
edited
Loading

cc @NikkiWines @johnmlee101

Details

We currently default Environment.isDevelopment to the "dev" environment, which allows ALL contributors to access all betas. This is great, but we're not ready for that for passwordless flows yet, so we're removing that ability for now. The user must have specifically passwordless beta OR all betas

Fixed Issues

$ None yet, based on slack convo: https://expensify.slack.com/archives/C049HHMV9SM/p1675264680096279

Tests

  1. Try to sign in as a user on the passwordless beta
  2. Verify you see the "Magic code" signin flow
  3. Sign out
  4. Try to sign in as a user on all betas
  5. Verify you see the "Magic code" signin flow
  6. Sign out
  7. Try to sign in as a user on NEITHER the passwordless not all betas
  8. Verify you see the old "Password login" flow
  • Verify that no errors appear in the JS console

Offline tests

N/A, signin flows are all offline

QA Steps

Same as above

  • 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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android

@Beamanator Beamanator requested a review from a team as a code owner February 1, 2023 15:50
@Beamanator Beamanator self-assigned this Feb 1, 2023
@melvin-bot melvin-bot bot requested review from joelbettner and removed request for a team February 1, 2023 15:51
@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

@joelbettner Please 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]

@Beamanator Beamanator added the InternalQA This pull request required internal QA label Feb 1, 2023
mountiny
mountiny previously approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Looking good, does this need to be CP Staging?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

⚠️ ⚠️ Heads up! This pull request has the CP Staging label ⚠️ ⚠️
If you applied the CP Staging label before the PR was merged, the PR will be be immediately deployed to staging even if the open StagingDeployCash deploy checklist is locked.
However if you applied the CP Staging after the PR was merged it's possible it won't be CP'ed automatically. If you need it to be CP'ed to staging, tag a member of @Expensify/mobile-deployers to CP it manually, otherwise you can wait for it to go out with the next deploy.

@Beamanator Beamanator changed the title User must specifically have passwordless beta Only show passwordless flow if user is on passwordless or all betas Feb 1, 2023
@mountiny
Copy link
Contributor

mountiny commented Feb 1, 2023 •
edited
Loading

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

Not dding screenshots as this only influences the Passwordless beta logic which should be fine.

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

mountiny
mountiny approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

🚢

@Beamanator Beamanator merged commit dbc1764 into main Feb 1, 2023
@Beamanator Beamanator deleted the beaman-onlyAllowPasswordlessForThatBeta branch February 1, 2023 16:04
@Beamanator Beamanator mentioned this pull request Feb 1, 2023
Closed
OSBotify pushed a commit that referenced this pull request Feb 1, 2023
@Beamanator @OSBotify
Merge pull request #14730 from Expensify/beaman-onlyAllowPasswordless…
cea6c07 
…ForThatBeta

Only show `passwordless` flow if user is on `passwordless` or `all` betas

(cherry picked from commit dbc1764)
This was referenced Feb 1, 2023
Closed
Closed
@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 688.692 ms → 693.504 ms (+4.812 ms, +0.7%)
Open Search Page TTI 596.750 ms → 597.245 ms (+0.494 ms, ±0.0%)
App start regularAppStart 0.014 ms → 0.015 ms (+0.001 ms, +5.7%)
App start nativeLaunch 20.406 ms → 20.133 ms (-0.273 ms, -1.3%)
App start runJsBundle 196.094 ms → 189.516 ms (-6.578 ms, -3.4%)
Show details
Name Duration
App start TTI Baseline
Mean: 688.692 ms
Stdev: 36.409 ms (5.3%)
Runs: 596.9407299999148 625.8786120000295 647.7752800001763 648.7893869997934 654.0279009998776 656.630131999962 656.8894810001366 660.6488219997846 661.0179980001412 664.1195330000482 670.1836749999784 681.8276610001922 682.6465839999728 684.7634530002251 689.6793010002002 690.1419310001656 692.5995100000873 692.9201600002125 693.3442549998872 693.7242760001682 694.1904090000317 696.1030749999918 697.7589079998434 710.8694059997797 716.7705370001495 724.0102639999241 727.8611010001041 728.9172789999284 740.7815990000963 741.4922500001267 751.4045959999785 763.4320149999112

Current
Mean: 693.504 ms
Stdev: 24.202 ms (3.5%)
Runs: 644.3337159999646 662.7269159997813 664.8478310001083 665.8250489998609 666.021788999904 666.2057329998352 666.6814779997803 671.2750140000135 671.610756999813 672.5428280001506 677.962096999865 684.374584000092 687.3823379999958 687.7833309997804 692.1730980002321 698.4332019998692 698.7333419998176 700.8476929999888 703.9054979998618 705.346226000227 705.4928720002063 706.2007749998011 709.0607360000722 709.901060000062 712.5250309999101 715.1834089998156 717.7355929999612 724.696026999969 731.0914389998652 732.2478700000793 745.4762599999085
Open Search Page TTI Baseline
Mean: 596.750 ms
Stdev: 18.355 ms (3.1%)
Runs: 564.568521999754 572.9056400000118 575.5078940000385 577.6230879998766 577.9996750000864 578.7065840000287 580.5815439997241 582.1989350002259 583.0582690001465 583.238362999633 583.7364100003615 585.1974289999343 587.6487630000338 589.0592860002071 590.8881840002723 592.3120530000888 597.4235439999029 597.7397059998475 601.1721199997701 603.5070810001343 603.8057050001808 603.904378999956 604.2768560000695 604.3570959996432 607.1134849996306 607.1754970001057 612.2219650000334 621.4672860000283 625.6724449996836 627.1188149997033 634.9405110003427 638.8800050001591

Current
Mean: 597.245 ms
Stdev: 17.198 ms (2.9%)
Runs: 566.300659999717 567.4014489999972 568.432618000079 570.866536999587 574.8135990002193 581.3914390001446 583.081991000101 583.2981370002963 584.4859209996648 585.1630859998986 585.4852709998377 590.3636880000122 593.2768959999084 597.3647469999269 598.0699060000479 600.3822840000503 600.5114339999855 600.6083580004051 600.8724370002747 604.3599049998447 604.3884279998019 604.513469000347 604.7695310004056 605.2522789998911 607.7542320000939 609.7889809999615 612.0148110003211 612.3940840000287 612.4738779999316 615.198406000156 617.2026780000888 625.5120439999737 641.2790530002676
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (3.8%)
Runs: 0.012614000122994184 0.01293900003656745 0.013143000192940235 0.01318400027230382 0.013265000190585852 0.0134680001065135 0.013549999799579382 0.01355000026524067 0.013631000183522701 0.013671999797224998 0.013671999797224998 0.013712999876588583 0.01383400009945035 0.01383400009945035 0.013835000339895487 0.013957000337541103 0.014038000255823135 0.014078999869525433 0.014159999787807465 0.014200999867171049 0.014240999706089497 0.014240999706089497 0.014241999946534634 0.014322999864816666 0.014403999783098698 0.014810999855399132 0.014933000318706036

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.1%)
Runs: 0.013264999724924564 0.01334700034931302 0.013508999720215797 0.013590999878942966 0.013915999792516232 0.0139979999512434 0.014159999787807465 0.014159999787807465 0.014241999946534634 0.014281999785453081 0.014282000251114368 0.014322999864816666 0.01436399994418025 0.014404000248759985 0.014566999860107899 0.014607999939471483 0.014648000244051218 0.014690000098198652 0.0147299999371171 0.014810999855399132 0.014810999855399132 0.014891999773681164 0.015015000011771917 0.015217999927699566 0.015218000393360853 0.015219000168144703 0.01525900000706315 0.015542999841272831 0.015746999997645617 0.01607299968600273 0.016480000223964453
App start nativeLaunch Baseline
Mean: 20.406 ms
Stdev: 2.234 ms (10.9%)
Runs: 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 22 22 22 23 23 24 25 25 26

Current
Mean: 20.133 ms
Stdev: 1.688 ms (8.4%)
Runs: 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 20 20 21 21 21 21 22 22 22 22 22 22 22 22 24
App start runJsBundle Baseline
Mean: 196.094 ms
Stdev: 21.551 ms (11.0%)
Runs: 159 161 166 168 169 175 177 178 181 182 185 186 186 187 193 194 195 196 199 200 205 209 214 214 215 216 217 223 227 229 231 238

Current
Mean: 189.516 ms
Stdev: 18.661 ms (9.8%)
Runs: 165 166 168 168 170 172 173 173 174 177 178 180 182 184 185 186 188 192 192 194 196 197 201 204 205 205 206 210 211 226 247

@mvtglobally
Copy link

@Beamanator Can you validate this internally as we can only use Expensifail account and it will not fit both scenarios

@johnmlee101
Copy link
Contributor

@mvtglobally this was mistakenly marked as CP'd and is actually not on staging, so this shouldn't be tested yet

@OSBotify
Copy link
Contributor

OSBotify commented Feb 2, 2023

🚀 Deployed to staging by https://github.com/Beamanator in version: 1.2.64-0 🚀

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

@thienlnam thienlnam mentioned this pull request Feb 2, 2023
Closed
30 tasks
@Beamanator
Copy link
Contributor Author

Same as tests for other PR, here: #13655 (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 🍎 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
Reviewers

@mountiny mountiny mountiny approved these changes

@johnmlee101 johnmlee101 johnmlee101 approved these changes

@joelbettner joelbettner Awaiting requested review from joelbettner

Assignees

@Beamanator Beamanator

Labels
InternalQA This pull request required internal QA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Beamanator @mountiny @mvtglobally @johnmlee101 @OSBotify