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

Revert "Conditionally require password for adding a debit card" #15217

Conversation

francoisl
Copy link
Contributor

@francoisl francoisl commented Feb 16, 2023

Reverts #15117 since it's not passing QA, and will require additional backend changes.

Internal discussion

Tests

  1. Sign into a passwordless account
  2. Go to Settings > Payments > Add Payment method > Add a debit card
  3. Make sure the page shows the password input field

image

  • Verify that no errors appear in the JS console

Offline tests

N/A behavior is the same as online tests

QA Steps

Will be QAed internally after it's CP'ed to staging.

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:
    -[x] 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.

@francoisl francoisl requested a review from NikkiWines February 16, 2023 19:18
@francoisl francoisl self-assigned this Feb 16, 2023
@github-actions
Copy link
Contributor

⚠️ ⚠️ 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.

@francoisl francoisl marked this pull request as ready for review February 16, 2023 19:25
@francoisl francoisl requested a review from a team as a code owner February 16, 2023 19:25
@melvin-bot melvin-bot bot requested review from thienlnam and removed request for a team February 16, 2023 19:25
@MelvinBot
Copy link

@thienlnam 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]

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.

@NikkiWines NikkiWines merged commit 3eff889 into main Feb 16, 2023
@NikkiWines NikkiWines deleted the revert-15117-nikki-conditionally-require-password-for-debit-card branch February 16, 2023 19:38
OSBotify pushed a commit that referenced this pull request Feb 16, 2023
…ally-require-password-for-debit-card

Revert "Conditionally require password for adding a debit card"

(cherry picked from commit 3eff889)
OSBotify added a commit that referenced this pull request Feb 16, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 727.032 ms → 743.144 ms (+16.112 ms, +2.2%)
App start runJsBundle 200.250 ms → 203.094 ms (+2.844 ms, +1.4%)
App start regularAppStart 0.015 ms → 0.024 ms (+0.009 ms, +60.2%) 🟡
App start nativeLaunch 20.032 ms → 19.467 ms (-0.566 ms, -2.8%)
Open Search Page TTI 621.789 ms → 613.879 ms (-7.910 ms, -1.3%)
Show details
Name Duration
App start TTI Baseline
Mean: 727.032 ms
Stdev: 29.907 ms (4.1%)
Runs: 668.7335890000686 691.6738379998133 691.8340299995616 692.2313449997455 692.7742980001494 695.7009290000424 699.2453859997913 699.8332820003852 701.5843500001356 705.2525490000844 706.2929670000449 711.6928939996287 712.9380230000243 714.1363469995558 715.0509320003912 718.824593000114 726.7325400002301 729.9494780004025 730.7778930002823 731.4341120002791 740.880366999656 744.2882329998538 744.5195040004328 751.7079849997535 755.6232620002702 762.0023050000891 765.890293000266 768.2045250004157 768.8490319997072 772.1440340001136 776.3607900002971 777.8640569997951

Current
Mean: 743.144 ms
Stdev: 28.165 ms (3.8%)
Runs: 701.5401200000197 702.1885519996285 704.5104229999706 704.76186100021 709.6680859997869 715.8783670002595 717.2889590002596 720.5307729998603 720.8128629997373 722.2781800003722 724.4936370002106 726.3650599997491 734.5446359999478 737.0981409996748 741.1127220001072 742.4347329996526 742.6075109997764 744.0772449998185 746.4439340000972 747.9482460003346 752.8624750003219 752.9687630003318 753.6043309997767 756.9981329999864 763.0510929999873 765.5487709995359 773.2252230001613 781.6010100003332 782.454990000464 788.7282729996368 796.6800450002775 806.30102299992
App start runJsBundle Baseline
Mean: 200.250 ms
Stdev: 21.033 ms (10.5%)
Runs: 167 171 175 177 178 181 182 182 185 186 187 188 191 191 191 194 196 198 202 203 204 204 214 214 218 222 225 230 237 238 238 239

Current
Mean: 203.094 ms
Stdev: 19.659 ms (9.7%)
Runs: 174 182 182 184 186 186 187 188 189 189 190 191 191 193 194 195 197 198 199 202 204 209 212 216 220 221 226 232 234 235 244 249
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (5.8%)
Runs: 0.013469000346958637 0.01355000026524067 0.013712999410927296 0.013753000646829605 0.013753999955952168 0.01387499924749136 0.01403799932450056 0.014118999242782593 0.014161000028252602 0.014200999401509762 0.014241999946534634 0.014242000877857208 0.014281999319791794 0.014282000251114368 0.01444500032812357 0.014526000246405602 0.014566999860107899 0.014648999087512493 0.014810999855399132 0.014810999855399132 0.014973999932408333 0.015056000091135502 0.015259000472724438 0.01537999976426363 0.015542999841272831 0.01566499937325716 0.015665000304579735 0.015666000545024872 0.015705999918282032 0.015991000458598137 0.01664199959486723 0.016805000603199005

Current
Mean: 0.024 ms
Stdev: 0.002 ms (8.3%)
Runs: 0.02026400063186884 0.0206300001591444 0.021402999758720398 0.021403000690042973 0.021769000217318535 0.021932000294327736 0.022053999826312065 0.022135999985039234 0.022216999903321266 0.022379999980330467 0.02254199981689453 0.02254199981689453 0.022664999589323997 0.022664999589323997 0.022990000434219837 0.02347799949347973 0.023641000501811504 0.023721999488770962 0.023763000033795834 0.02404800057411194 0.024129999801516533 0.024740000255405903 0.024740000255405903 0.024860999546945095 0.025716000236570835 0.025756999850273132 0.025798000395298004 0.026041999459266663 0.02612300030887127 0.0274660000577569 0.028524000197649002
App start nativeLaunch Baseline
Mean: 20.032 ms
Stdev: 2.293 ms (11.4%)
Runs: 17 18 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 20 20 21 21 21 21 22 22 22 23 23 23 24 27

Current
Mean: 19.467 ms
Stdev: 1.746 ms (9.0%)
Runs: 17 17 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 21 21 21 21 22 22 22 25
Open Search Page TTI Baseline
Mean: 621.789 ms
Stdev: 19.714 ms (3.2%)
Runs: 581.9359130002558 589.9441330004483 601.3370770001784 604.134766000323 604.9264740003273 606.8692220002413 607.7821450000629 608.7342940000817 608.9139409996569 611.4643970001489 613.2213949998841 613.2393399998546 613.8960380004719 615.5134680001065 615.6069750003517 618.0638020001352 620.4436039999127 622.0893149999902 622.2699790000916 624.5948080001399 627.9025060003623 628.9979250002652 630.1017659995705 631.3216560008004 634.6319579994306 635.0417069997638 637.3600269993767 652.7125649992377 656.5310879992321 666.6532800002024 669.238810999319

Current
Mean: 613.879 ms
Stdev: 14.185 ms (2.3%)
Runs: 587.5829270007089 587.787434999831 594.8519689999521 596.665853000246 598.0374359991401 600.6080730007961 604.420451999642 604.9293219996616 606.4425870003179 609.8158770008013 610.0738120004535 610.3380539994687 612.6349280001596 613.4944669995457 614.5897629996762 615.6958010001108 616.1121020000428 616.9864509999752 618.3047289997339 618.6118170004338 618.6533200005069 618.8020830005407 619.5631920006126 619.8133140001446 620.2032480007038 625.1657720003277 627.576253999956 630.7274579992518 644.7367759998888 653.1513269999996

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/NikkiWines in version: 1.2.72-1 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/francoisl in version: 1.2.72-1 🚀

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

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀

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.

4 participants