Remove unnecessary password field from add account via plaid step #4944

Julesssss · 2021-08-31T11:42:44Z

Details

Removes an unnecessary password field from the 'Add Bank Account' via Plaid step.

Fixed Issues

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

Tests

A) Confirm that the password step is no longer seen

Create a New Workspace via the global menu, or navigate to an existing workspace
Tap 'Get Started' to launch the add bank account flow
Tap 'Log in to your bank' which launches the Plaid modal
Enter 'user_good' & 'pass_good'
You should NOT see a password input field
The button should be enabled after selecting an account from the dropdown

B) Confirm that removing the password does not block the flow

Continue from test A
You should be able to progress through the remaining modal steps (use these details if on dev env)

QA Steps

Run above test

Tested On

Screenshots

@trjExpensify

Web

Mobile Web

I have been unable to verify this due to a possible SSL issue

Desktop

iOS

Android

Note: Android is blocked by this issue, we should retest Android after this issue is resolved.

Julesssss · 2021-08-31T15:24:35Z

@luacmartins I'm still struggling to verify Android/mobile Web due to this (SSL?) issue. I'll keep trying to resolve this, but for now please test on Android & mWeb to ensure we have covered all platforms, thank you!

luacmartins

Code looks good! Test steps worked on all platforms, but Android. The Plaid modal does not open in my emulator. I tried several times, but can't tell what the issue is. Here's a video:

android-error.mov

Here are the console logs:

 DEBUG  [info] Making API request - {"command":"Plaid_GetLinkToken","type":"post","shouldUseSecure":true}
 DEBUG  [info] Flushing logs as app is going inactive - {}
 DEBUG  [info] Previous log requestID - {"requestID":"SW4vyV"}
 DEBUG  [info] [NetworkConnection] Firing reconnection callbacks because app became active - ""
 DEBUG  [info] Making API request - {"command":"Get","type":"post","shouldUseSecure":false,"rvl":"personalDetailsList"}
 DEBUG  [info] Making API request - {"command":"Get","type":"post","shouldUseSecure":false,"rvl":"chatList"}
 DEBUG  [info] Finished API request - {"command":"Plaid_GetLinkToken","type":"post","shouldUseSecure":true,"jsonCode":200,"requestID":"ZC1ROb"}
 DEBUG  [info] Finished API request - {"command":"Get","type":"post","shouldUseSecure":false,"jsonCode":200,"requestID":"STbVn6"}
 DEBUG  [info] Flushing logs as app is going inactive - {}
 DEBUG  [info] Finished API request - {"command":"Get","type":"post","shouldUseSecure":false,"jsonCode":200,"requestID":"cgneTa"}
 DEBUG  [info] Making API request - {"command":"Get","type":"post","shouldUseSecure":false,"rvl":"reportSummaryList"}
 DEBUG  [info] Previous log requestID - {"requestID":"wJmVxy"}
 DEBUG  [info] Finished API request - {"command":"Get","type":"post","shouldUseSecure":false,"jsonCode":200,"requestID":"Y3YPaq"}
 DEBUG  [info] [Report] successfully fetched report data - {"chatList":["648","649","650","651","652","653","654","655","656","657","658","659","660","661","662","663","664","665","666","667","668","671","672"]}
 DEBUG  [info] Making API request - {"command":"PersonalDetails_GetForEmails","type":"post","shouldUseSecure":false}
 DEBUG  [info] Finished API request - {"command":"PersonalDetails_GetForEmails","type":"post","shouldUseSecure":false,"jsonCode":200,"requestID":"x2nxNs"}
 DEBUG  [Report] Local reportActions up to date. Not fetching additional actions.

Am I missing something?

Julesssss · 2021-08-31T17:48:06Z

Nope! That's exactly my problem. I've been trying to resolve this here, with no luck so far. I think we should ask for testing from someone who is able to verify Android / mWeb to keep this moving forward.

Edit: I have asked for another reviewer to keep things moving forward.

marcaaron · 2021-08-31T18:39:27Z

I'm able to reproduce this issue on my Galaxy S8. Something is breaking the Plaid flow for sure.
I guess the next step would be to see if this is broken on main or introduced in this PR (doesn't look like it would be the latter but unsure).

It looks like there's no problem with getting the token to launch the modal (verified by using the Network plugin in Flipper). But seems like we get caught in an infinite loop or something and then the app crashes.

marcaaron · 2021-08-31T18:49:20Z

From the device logs (filtering errors and plaid):

Not sure if there is anything helpful here, but this looks like what ultimately causes the crash

Illegal callback invocation from native module. This callback type only permits a single invocation from native

marcaaron · 2021-08-31T18:52:13Z

There's an issue here in the Plaid repository: plaid/react-native-plaid-link-sdk#398

And the suggestion seems to be that we are opening the Plaid link multiple times. Looking at the code it doesn't seem like this is the case, but maybe we are not cleaning things up properly when the Plaid flow is exited.

FWIW when I tested this here's what I saw:

The Plaid modal did not open at all and I got a white screen
Close the modal
Try again
💥

marcaaron · 2021-08-31T19:57:16Z

FWIW this issue also exists on main I'm not sure if the issue exists in the latest play store release so I'll try there next.

marcaaron · 2021-08-31T20:11:38Z

~~Broken on play store release~~

marcaaron · 2021-08-31T20:12:06Z

Oh wait, it is actually NOT broken... it just takes... forever to load.

Edit: And by forever I mean like 30+ seconds where there is also a completely blank screen for some time and then finally the Plaid UI appears.

marcaaron · 2021-08-31T20:36:30Z

Ok, Going to create a new issue for this and make it a deploy blocker.
I don't see any reason why this PR should be blocked though.

marcaaron · 2021-08-31T22:19:45Z

src/components/AddPlaidBankAccount.js

@@ -93,7 +91,6 @@ class AddPlaidBankAccount extends React.Component {

        this.state = {
            selectedIndex: undefined,
-            password: '',


It looks like we are still referencing this on line 116

account, password: this.state.password, plaidLinkToken: this.props.plaidLinkToken,

I honestly can't remember, but is it safe to just remove this / is nothing expecting the password?
What are we expecting on the API side here?

Good spot, I thought I had removed this 😕

Yes, I believe it is safe to remove this. The password param is sent at each step, but it is only required at step: CompanyStep. I'll remove the param and add some additional test steps to prove this is the case.

Notice that we don't send password for the 'Connect manually' step either:

App/src/pages/ReimbursementAccount/BankAccountStep.js

Lines 79 to 91 in 993bc0b

addManualAccount() {

setupWithdrawalAccount({

acceptTerms: this.state.hasAcceptedTerms,

accountNumber: this.state.accountNumber,

routingNumber: this.state.routingNumber,

setupType: CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL,

// Note: These are hardcoded as we're not supporting AU bank accounts for the free plan

country: CONST.COUNTRY.US,

currency: CONST.CURRENCY.USD,

fieldsType: CONST.BANK_ACCOUNT.FIELDS_TYPE.LOCAL,

});

}

To make this even clearer, I could raise a PR in Web-Secure which would only pass password to the API here if parameters.password is not undefined (or only for those steps which require it).

However, as all it does is prevent an undefined param from being sent in the API, I'm not sure how valuable this is.

I guess I am mostly curious here if this was an oversight or if the Web-Secure API was ever expecting this parameter? Probably we just went off of mockups here and assumed the password is required.

I think it's necessary when calling BankAccount_Create (for the PBA flow) but perhaps is not required for the VBA flow. I'm not too sure. I do want to point out that SetupWithdrawalAccount does accept a password parameter and validates it under certain conditions.

https://github.com/Expensify/Auth/blob/3ed205bafa8c39de56a77183732d3d6d0ce4429c/auth/command/SetupWithdrawalAccount.cpp#L130-L133

But unsure if that's something we need to worry about here.

Oh yeah, that Auth line did make me question this too... But as my testing raised no issues I think it can be explained as an oversight or change.

luacmartins · 2021-09-01T16:24:01Z

I will retest this once #4959 is merged.

marcaaron · 2021-09-01T16:58:27Z

I will retest this once #4959 is merged.

Unsure if this will be fixed very soon 😬 - probably we should just merge this since the Plaid modal is already affected in staging and release builds.

OSBotify · 2021-09-01T17:00:25Z

✋ 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 · 2021-09-02T01:02:41Z

🚀 Deployed to staging by @marcaaron in version: 1.0.91-1 🚀

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

OSBotify · 2021-09-03T01:30:10Z

🚀 Deployed to production by @roryabraham in version: 1.0.92-0 🚀

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

Julesssss added 4 commits August 31, 2021 12:02

remove password input, test flow

1b71b2f

clear up removed input field

40c76b3

remove now unused string

f7f3cdb

remove now unused import

e6c61df

Julesssss self-assigned this Aug 31, 2021

remove unused translated string

8a9e650

Julesssss marked this pull request as ready for review August 31, 2021 15:22

Julesssss requested a review from a team as a code owner August 31, 2021 15:22

MelvinBot requested review from luacmartins and removed request for a team August 31, 2021 15:23

luacmartins reviewed Aug 31, 2021

View reviewed changes

marcaaron self-requested a review August 31, 2021 18:21

marcaaron mentioned this pull request Aug 31, 2021

Plaid modal is no longer opening on Android #4959

Closed

marcaaron requested changes Aug 31, 2021

View reviewed changes

remove password from AddPlaidBankAccount

5fd5d61

Julesssss requested a review from marcaaron September 1, 2021 14:42

luacmartins approved these changes Sep 1, 2021

View reviewed changes

marcaaron approved these changes Sep 1, 2021

View reviewed changes

marcaaron merged commit 0ca4c4d into main Sep 1, 2021

marcaaron deleted the jules-removePlaidPasswordField branch September 1, 2021 16:59

OSBotify mentioned this pull request Sep 1, 2021

Deploy Checklist: New Expensify 2021-09-01 #5004

Closed

55 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove unnecessary password field from add account via plaid step #4944

Remove unnecessary password field from add account via plaid step #4944

Julesssss commented Aug 31, 2021 •

edited

Loading

Julesssss commented Aug 31, 2021

luacmartins left a comment

Julesssss commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021

marcaaron commented Aug 31, 2021

marcaaron commented Aug 31, 2021

marcaaron commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021

marcaaron Aug 31, 2021

Julesssss Sep 1, 2021

Julesssss Sep 1, 2021

Julesssss Sep 1, 2021 •

edited

Loading

marcaaron Sep 1, 2021

Julesssss Sep 1, 2021

luacmartins commented Sep 1, 2021

marcaaron commented Sep 1, 2021

OSBotify commented Sep 1, 2021

OSBotify commented Sep 2, 2021

OSBotify commented Sep 3, 2021

	addManualAccount() {
	setupWithdrawalAccount({
	acceptTerms: this.state.hasAcceptedTerms,
	accountNumber: this.state.accountNumber,
	routingNumber: this.state.routingNumber,
	setupType: CONST.BANK_ACCOUNT.SETUP_TYPE.MANUAL,

	// Note: These are hardcoded as we're not supporting AU bank accounts for the free plan
	country: CONST.COUNTRY.US,
	currency: CONST.CURRENCY.USD,
	fieldsType: CONST.BANK_ACCOUNT.FIELDS_TYPE.LOCAL,
	});
	}

Remove unnecessary password field from add account via plaid step #4944

Remove unnecessary password field from add account via plaid step #4944

Conversation

Julesssss commented Aug 31, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Julesssss commented Aug 31, 2021

luacmartins left a comment

Choose a reason for hiding this comment

Julesssss commented Aug 31, 2021 • edited Loading

marcaaron commented Aug 31, 2021 • edited Loading

marcaaron commented Aug 31, 2021

marcaaron commented Aug 31, 2021

marcaaron commented Aug 31, 2021

marcaaron commented Aug 31, 2021 • edited Loading

marcaaron commented Aug 31, 2021 • edited Loading

marcaaron commented Aug 31, 2021

marcaaron Aug 31, 2021

Choose a reason for hiding this comment

Julesssss Sep 1, 2021

Choose a reason for hiding this comment

Julesssss Sep 1, 2021

Choose a reason for hiding this comment

Julesssss Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

marcaaron Sep 1, 2021

Choose a reason for hiding this comment

Julesssss Sep 1, 2021

Choose a reason for hiding this comment

luacmartins commented Sep 1, 2021

marcaaron commented Sep 1, 2021

OSBotify commented Sep 1, 2021

OSBotify commented Sep 2, 2021

OSBotify commented Sep 3, 2021

Julesssss commented Aug 31, 2021 •

edited

Loading

Julesssss commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021 •

edited

Loading

marcaaron commented Aug 31, 2021 •

edited

Loading

Julesssss Sep 1, 2021 •

edited

Loading