Skip to content

Autofill previously entered accounting and routing numbers#4477

Merged
Julesssss merged 5 commits into
mainfrom
joe-save-bank-details
Aug 10, 2021
Merged

Autofill previously entered accounting and routing numbers#4477
Julesssss merged 5 commits into
mainfrom
joe-save-bank-details

Conversation

@Jag96

@Jag96 Jag96 commented Aug 6, 2021

Copy link
Copy Markdown
Contributor

@roryabraham will you please review this?

Details

This PR updates the Add Bank Account flow to save the accountNumber and routingNumber if the user exits the flow after setting those values.

Fixed Issues

$ #4476

Tests/QA

  1. Log into an account w/ a workspace and go to Settings->Workspace->Expensify Card and hit Get Started
  2. Fill out values for the accounting number and routing number and hit Save & Continue
  3. Close the modal then hit Get Started again, confirm routing number and account number are saved
  4. Change the routing and account numbers, then hit Save & Continue
  5. Close the modal then hit Get Started again. Verify that the routing and account number have been updated.
  6. Hit Save & Continue again.
  7. Fill out the company information step and hit save and continue
  8. Close the modal and hit Get Started again, confirm the values are pre-populated
  9. Sign out of the account and sign back in, confirm that the values are pre-populated (this will only work if you save on the Company Information step)

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web/Desktop

Starting Flow Already saved at Company Info

Mobile

Starting Flow Already saved at Company Info

@Jag96 Jag96 requested a review from roryabraham August 6, 2021 22:23
@Jag96 Jag96 requested a review from a team as a code owner August 6, 2021 22:23
@Jag96 Jag96 self-assigned this Aug 6, 2021
@MelvinBot MelvinBot requested review from Julesssss and removed request for a team August 6, 2021 22:23
Julesssss
Julesssss previously approved these changes Aug 9, 2021

@Julesssss Julesssss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just noticed a case that I think might be a failure. It's a bit of an edge case, but we will definitely have customers who might not notice that the old numbers overwrote the newer details

  • Run test steps 1 & 2
  • Now, reopen the 'Add Bank Account' step but this time change the acc numbers
  • Hit save & continue, then close the modal
  • Reopen the 'Add Bank Account' step
  • Notice that the OLD numbers are used -- this should instead show the last entered numbers (666666666 instead of 555555555 in the screencast)
not-overriding-nums.mov

@Jag96

Jag96 commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

Looking into this, it seems like there is an Onyx.merge call that isn't working properly in this flow. In this code the newACHData is being passed with a routingNumber 123123123, but after the merge resolves the value in Onyx is still set to the old value 555555555. Going to have a look and see why this is happening.

image

@Jag96 Jag96 marked this pull request as draft August 9, 2021 17:46
@Jag96

Jag96 commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

Ok, looks like the fix here is to just wait until the first merge call finishes before letting the 2nd one happen since the 2nd one is overwriting the first with the previous values. Testing and updating.

@Jag96 Jag96 marked this pull request as ready for review August 9, 2021 20:51
@Jag96

Jag96 commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

@roryabraham @Julesssss updated to fix the edge case there, good find!

@roryabraham

roryabraham commented Aug 9, 2021

Copy link
Copy Markdown
Contributor

Updated the testing steps to include the edge case you found @Julesssss, good find 👏

Comment thread src/libs/actions/BankAccounts.js
@Julesssss

Copy link
Copy Markdown
Contributor

Confirmed the edge case is fixed 👍

@Julesssss Julesssss merged commit 1de3e10 into main Aug 10, 2021
@Julesssss Julesssss deleted the joe-save-bank-details branch August 10, 2021 12:34
@OSBotify

Copy link
Copy Markdown
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
Copy Markdown
Contributor

🚀 Deployed to staging by @Julesssss in version: 1.0.83-2 🚀

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

@isagoico

Copy link
Copy Markdown

@Jags96 Hello! We're currently unable to test this add bank account flow with our testing domains since all already have the Expensify card. Can we test this via navigating to staging.new.expensify.com/bank-account/new?

@Jag96

Jag96 commented Aug 11, 2021

Copy link
Copy Markdown
Contributor Author

@isagoico no worries, I'll do QA for this one and check it off on the spreadsheet!

@OSBotify

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants