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

New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount #9423

Merged
merged 14 commits into from
Jul 19, 2022
Merged

New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount #9423

merged 14 commits into from
Jul 19, 2022

Conversation

nkuoch
Copy link
Contributor

@nkuoch nkuoch commented Jun 13, 2022 •
edited by ctkochan22
Loading

Details

Refactoring AddPersonalBankAccount flow

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/214576
$ https://github.com/Expensify/Expensify/issues/214738

Tests

Make sure you can add a Personal Bank Account. See screenshots.

QA Steps

Make sure you can add a Personal Bank Account

Screenshots

Web

image

If you're offline:
image

If you're online:
image
image
image
image
image

If you're offline:
image

If you're online:
image
image
image
image

@nkuoch nkuoch self-assigned this Jun 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jun 13, 2022

Looks like you modified deprecatedAPI.js! To be clear, you should not be adding any code to this file.

Instead, all new API commands should use API.js, and follow our guidelines for writing new API commands.

Unsure if your change is okay? Drop a note in #expensify-open-source!

@nkuoch nkuoch changed the title New APIs for Plaid connection [WIP] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector Jun 14, 2022
@nkuoch nkuoch force-pushed the nat-plaidNewAPI branch 2 times, most recently from 52bee65 to 2997d1d Compare June 20, 2022 19:00
@nkuoch nkuoch changed the title [WIP] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector [WIP] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount Jun 20, 2022
@nkuoch nkuoch force-pushed the nat-plaidNewAPI branch 9 times, most recently from fec42e6 to eea36fb Compare June 29, 2022 15:35
@nkuoch nkuoch changed the title [WIP] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount [HOLD] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount Jun 29, 2022
@nkuoch nkuoch changed the title [HOLD] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount [HOLD - 33995] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount Jun 30, 2022
@nkuoch nkuoch force-pushed the nat-plaidNewAPI branch 4 times, most recently from 684dd4a to d27bc3e Compare July 3, 2022 20:14
@nkuoch nkuoch changed the title [HOLD - 33995] New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount New APIs OpenPlaidBankLogin and OpenPlaidBankAccountSelector and AddPersonalBankAccount Jul 5, 2022
@marcaaron marcaaron removed their request for review July 5, 2022 18:35
@nkuoch nkuoch marked this pull request as ready for review July 6, 2022 13:24
@nkuoch nkuoch requested a review from a team as a code owner July 6, 2022 13:24
NikkiWines
NikkiWines previously approved these changes Jul 8, 2022
View reviewed changes
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.

Couple of NABs but this looks good otherwise

src/components/AddPlaidBankAccount.js Outdated Show resolved Hide resolved
src/pages/AddPersonalBankAccountPage.js Outdated Show resolved Hide resolved
@ctkochan22
Copy link
Contributor

Updated. Going to test again tomorrow.

NikkiWines
NikkiWines approved these changes Jul 15, 2022
View reviewed changes
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.

👍

@ctkochan22
Copy link
Contributor

@MariaHCD @amyevans Please give it one more review

MariaHCD
MariaHCD requested changes Jul 15, 2022
View reviewed changes
Copy link
Contributor

@MariaHCD MariaHCD left a comment

Choose a reason for hiding this comment

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

New.Expensify.5.mp4

I noticed that the PBA isn't added immediately to the list in the Payments page after the success page is dismissed.

@ctkochan22
Copy link
Contributor

@MariaHCD huh! I'm unable to reproduce.

MariaHCD
MariaHCD approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@MariaHCD MariaHCD left a comment

Choose a reason for hiding this comment

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

Weird, I'm unable to reproduce either! LGTM and works as expected.

@ctkochan22 ctkochan22 merged commit 031341c into main Jul 19, 2022
@ctkochan22 ctkochan22 deleted the nat-plaidNewAPI branch July 19, 2022 17:29
@melvin-bot
Copy link

melvin-bot bot commented Jul 19, 2022

@ctkochan22 looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@melvin-bot melvin-bot bot added the Emergency label Jul 19, 2022
@ctkochan22
Copy link
Contributor

As we continue forward with our refactors, lets all keep a look out for each other and our changes!

@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.

@MariaHCD MariaHCD mentioned this pull request Jul 25, 2022
Merged
91 tasks
@OSBotify OSBotify mentioned this pull request Jul 25, 2022
Closed
92 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @ctkochan22 in version: 1.1.86-0 🚀

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

@neil-marcellini neil-marcellini mentioned this pull request Jul 25, 2022
Closed
@danieldoglas danieldoglas mentioned this pull request Jul 26, 2022
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Aug 1, 2022

🚀 Deployed to production by @yuwenmemon in version: 1.1.86-5 🚀

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

@alexxxwork alexxxwork mentioned this pull request Apr 21, 2023
Closed
@mananjadhav
Copy link
Collaborator

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.

@mateuuszzzzz mateuuszzzzz mentioned this pull request Mar 6, 2024
Merged
55 tasks
@nkuoch
Copy link
Contributor Author

nkuoch commented Apr 1, 2024

Removing Emergency label. It's old

@nkuoch nkuoch removed the Emergency label Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NikkiWines NikkiWines NikkiWines approved these changes

@ctkochan22 ctkochan22 ctkochan22 approved these changes

@MariaHCD MariaHCD MariaHCD approved these changes

@amyevans amyevans amyevans approved these changes

Assignees

@nkuoch nkuoch

@ctkochan22 ctkochan22

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nkuoch @MariaHCD @ctkochan22 @OSBotify @mananjadhav @amyevans @NikkiWines