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

Prevent errorAttemptsCount from being wiped out by JSON.stringify #4889

Merged
merged 1 commit into from
Aug 30, 2021

Conversation

NikkiWines
Copy link
Contributor

@NikkiWines NikkiWines commented Aug 27, 2021

Details

Fixes a bug that keeps the user stuck at the Requestor Step of the bank account setup process.

This was originally fixed by this PR but, for some reason, that fix (which worked for a solid month without issue) stopped working. .

Instead of rewriting how we save errorAttemptsCount in Web-Secure and potentially causing issues with E.com bank account setups, let's just convert this to an object that JSON.stringify can handle before calling API.BankAccount_SetupWithdrawal. We call JSON.stringify here We can also revert the changes made to Web-Secure the the afore mentioned PR since they're no longer necessary.

For reference as to why this is happening... JSON.stringify ignores any keys that aren't numeric.
Screen Shot 2021-08-27 at 12 01 03 PM

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/169717#issuecomment-907336665

Tests

  1. Set isPlaidTestRequestor() in lib/ACHData.php to return false so that we run the ownerIdentity checks
  2. Run through the testing steps

QA

  1. Log into an account on new.expensify.com that doesn't have a bank account and navigate to /bank-account
  2. Navigate to Settings > Account > Payments and click Add Verified Bank Account
  3. Click on Log Into Your Bank and then exist out of the Plaid modal that pops up
  4. Click on Connect Manually and enter the credentials for a PENDING bank account from this SO post
    • image
  5. Once you get to the Requestor step confirm that you get the Please verify your name and date of birth. If the information is correct, click "Save & Continue error once
  6. Resubmit your information and confirm you're routed on the OnFido verification screen
  7. Exit the modal, refresh the page, and delete the verified bank account

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2021-08-27.at.12.26.41.PM.mov

Mobile Web

Desktop

iOS

Android

@NikkiWines NikkiWines self-assigned this Aug 27, 2021
@NikkiWines NikkiWines requested a review from a team as a code owner August 27, 2021 19:22
@MelvinBot MelvinBot requested review from joelbettner and removed request for a team August 27, 2021 19:22
@NikkiWines
Copy link
Contributor Author

FWIW I'm not totally against updating Web-Secure so we don't send across an array here. We could use this as a temporary fix while I work on updating Web-S if that's the way we want to go.

@marcaaron
Copy link
Contributor

marcaaron commented Aug 27, 2021

I spent some time trying to figure out why the API is returning an array with non numeric keys instead of an object and I'm pretty thoroughly confused on how it's possible...

@NikkiWines has proposed this:

basically the way i see it is we have 2 options:

  1. Convert to an object in BankAccount.js (my proposed PR)
  2. Re-write how we store errorAttemptsCount in Web-S so it's stored in a way that doesn't confuse JSON.stringify
  3. Do both, step1 then step 2

Which seems reasonable to me 👍

@marcaaron
Copy link
Contributor

Actually, pretty sure this is a front end bug caused by PHP being unable to send an empty object and treating a default empty associative array as an empty array. If we merge a normal object into an empty array then we get this weird thing...

2021-08-27_10-29-24

@joelbettner joelbettner merged commit ea3643e into main Aug 30, 2021
@joelbettner joelbettner deleted the nikki-prevent-errorsAttemptCount-override branch August 30, 2021 16:08
@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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @joelbettner in version: 1.0.88-3 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Sep 1, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 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