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

Add reset bank account modal #5828

Merged
merged 15 commits into from
Oct 22, 2021
Merged

Add reset bank account modal #5828

merged 15 commits into from
Oct 22, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Oct 13, 2021 •
edited
Loading

Details

Adds the ability to delete a bank account in setup so users can restart the VBA flow.

Fixed Issues

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

Tests / QA Steps

  1. Create a bank account in setup on NewDot by creating a workspace and following the instructions to create an OPEN bank account here
  2. Stop at the Personal Information step and exit the flow
  3. Tap "Connect bank account" in the workspace page
  4. Tap "Start over"
  5. Verify a modal appears with the following content
    2021-10-13_11-20-02
  6. Select "Cancel" and verify the modal closes
  7. Press "Start over" again and this time select "Yes, start over"
  8. Verify you are brought back to the "Streamline payments" page
  9. Begin setting up a bank account again but use the VERIFIYING instructions
  10. Go all the way to the end and verify you see the "No, let's start over" button
  11. Tap and verify the modal works as before. Reset the account.
  12. Add an account in the OPEN state again going all the way to the end of the flow
  13. Verify on the /enable step that a "Disconnect bank account" option is available
    2021-10-13_11-44-30
  14. Tap this option and verify a modal open
    2021-10-13_11-50-40
  15. Select "Yes, disconnect my bank account" and everything is reset

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron self-assigned this Oct 13, 2021
@marcaaron marcaaron changed the title [WIP] Add reset bank account modal [HOLD Auth 6034] Add reset bank account modal Oct 14, 2021
@marcaaron marcaaron changed the title [HOLD Auth 6034] Add reset bank account modal Add reset bank account modal Oct 21, 2021
@marcaaron marcaaron marked this pull request as ready for review October 21, 2021 22:22
@marcaaron marcaaron requested a review from a team as a code owner October 21, 2021 22:22
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team October 21, 2021 22:23
@marcaaron marcaaron requested a review from a team October 21, 2021 22:49
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team October 21, 2021 22:50
luacmartins
luacmartins requested changes Oct 21, 2021
View reviewed changes
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

Left a few comments. Still need to test it.

src/libs/actions/BankAccounts.js Outdated
*
* @param {String} password
*/
function resetFreePlanBankAccount(password = 'Password1') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we are defaulting the password to Password1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This can be removed. Basically, we have removed the requirement for a password entirely. I set this before that change to make dev testing easier and left it here.

src/libs/actions/BankAccounts.js Outdated
if (!credentials || !credentials.login) {
throw new Error('Missing credentials when attempting to reset free plan bank account');
}
if (!password) {
Copy link
Contributor

@luacmartins luacmartins Oct 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can we reach this if we default the password to Password1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can also be removed.

@marcaaron
Copy link
Contributor Author

Thanks for the thorough review @luacmartins 🙇
Made requested changes.

luacmartins
luacmartins approved these changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

LGTM and tested well! Great job 🎉

@luacmartins luacmartins merged commit b3b9302 into main Oct 22, 2021
@luacmartins luacmartins deleted the marcaaron-disconnectBankAccount branch October 22, 2021 14:43
@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 OSBotify mentioned this pull request Oct 23, 2021
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @luacmartins in version: 1.1.8-10 🚀

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

@marcaaron
Copy link
Contributor Author

I'm in the process of getting a test bank account set up here to complete the verifying section that QA could not get to. Almost done!

@marcaaron
Copy link
Contributor Author

Ok this is passing for me let's 🚢

@OSBotify
Copy link
Contributor

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

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

@NikkiWines NikkiWines mentioned this pull request Nov 5, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luacmartins luacmartins luacmartins approved these changes

@jasperhuangg jasperhuangg Awaiting requested review from jasperhuangg

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marcaaron @OSBotify @luacmartins