[$500] - Chat Room - Handle API response inVBA flow when tapping finish Set up

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to https://staging.new.expensify.com
2. Log in with expensifail account
3. Find any room in LHN
4. Tap on plus button and tap Send Money
5. Select amount and user
6. Tap Pay with Expensify
7. Tap Add Bank Account
8. Put Wells Fargo credentials
9. Select Saving Account
10. Put Password and Tap Finish Set up

Expected Result:

If the API request fails, we handle that and show the error to the user.

Actual Result:

Unable procced with VBA flow after put password and tap finish Set up. No error shown.

Workaround:

Unknown

Platform:

Where is this issue occurring?

• Mobile Web

Version Number: 1.1.29.1

Reproducible in staging?: Yes

Reproducible in production?: No

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL:

Issue reported by: Applause

https://user-images.githubusercontent.com/93399543/149238337-c6759873-2ae1-4df7-ae90-a17a81a0f866.mp4

Slack conversation:

View all open jobs on GitHub

[chiragsalian]

Oh hey, so yeah Rajat brought the discussion on slack and i believe he found a solution to test on staging using sandbox creds. So i believe @parasharrajat is working on this issue. Let me know if i'm mistaken.

[parasharrajat]

Yeah, I got the solution to test the flow yesterday. I will share the updates asap.

[dylanexpensify]

not overdue

[dylanexpensify]

Also, I'm heading on parental leave so reassigning! Thank you to whoever gets assigned this! ❤️

[melvin-bot[bot]]

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[parasharrajat]

Ok, I am looking into this. But I found few inconsistencies.

1. When a user enters the wrong password. Response does not contain the title for error.

   {
   "code": 666,
   "jsonCode": 666,
   "type": "Expensify\\Libs\\Error\\ExpError",
   "UUID": "xxxxxx",
   "message": "Incorrect Expensify password entered",
   "title": "",
   "data": [],
   "htmlMessage": "",
   "requestID": "xxxxx"
   }

---

I need a copy and translation.

• For error when the user selects an account that already exists.

But I think we should fix where we check for the duplicate accounts so that it does not show duplicate accounts for selection instead of showing the error. This logic should successfully filter the existing accounts. But currently, it is not. It may be due to testing accounts. https://github.com/Expensify/App/blob/572f6cb1b161fbb90fad17569f10c4af4926e773/src/libs/actions/Plaid.js#L64

I have added the PR for https://github.com/Expensify/App/issues/7185#issuecomment-1057336885

[parasharrajat]

But I think we should fix where we check for the duplicate accounts so that it does not show duplicate accounts for selection instead of showing the error. This logic should successfully filter the existing accounts. But currently, it is not. It may be due to testing accounts.

I will update the PR based on the decision.

[parasharrajat]

@chiragsalian Any thoughts on the above comment?

[chiragsalian]

Answering in the same order of the comment,

1. Would it help if response had a title? We can easily add that if it would help. Let me know.
2. Before we get the copy and translation yeah i think we should fix the logic that you mentioned, I would think it would exclude accounts already added. I don't see immediately see any logic in the backend that hard codes account.alreadyExists for test accounts so i would expect it to give us the actual values even for test account for i can test it more today.

[parasharrajat]

Would it help if the response had a title? We can easily add that if it would help. Let me know.

Does not affect anything technically but I was thinking that checking the title is the way to go for our codebase. Currently, the check is applied to the message string. To me, it feels like the message can change very often than the title. So this is totally optional.

Thanks. @techievivek found something here

[chiragsalian]

Nice, thats a good find. As for the title vs message. In the backend title is optional so it may not always be present but message is a required param. Additionally we can also perhaps make it return a specific error code (something other than 666) and then the front end can handle that error code as needed.

[Christinadobrzyn]

Not overdue - looks like we're still working on the PR

[parasharrajat]

Currently waiting for reply from @chiragsalian on the backend changes.

[chiragsalian]

Oh oops i didn't realize we were ready to make the backend changes. Normally if we are ready to make backend changes you'll see a new issue created for us for tracking purposes. So if you dont see one in 2 days that means we're most likely not working on it. Okay so can you remind me whats required here? Do you want us to just add a title or is any other backend change required? Would the title "Incorrect Expensify password entered" (which is same as message) be okay or would you prefer something else?

[parasharrajat]

I think we have to fix the code for point 2 on https://github.com/Expensify/App/issues/7185#issuecomment-1127516268.

account.alreadyExists is not true for half added bank accounts and try to re-add then gives the duplicate error. More details https://github.com/Expensify/App/issues/9085#issuecomment-1131489962

[chiragsalian]

Ah okay, i was looking at BankAccount_Create all this time which just had an error response like so,

{
    "code": 666,
    "jsonCode": 666,
    "type": "Expensify\\Libs\\Error\\ExpError",
    "UUID": "15fc249e-1f89-49be-aa78-1d87132bf9c5",
    "message": "Bank account can't be created, it looks like a bank account with the same information already exists in your account.",
    "title": "Bank Account Already Exists",
    "data": [],
    "htmlMessage": "",
    "requestID": "wUb69V"
}

But i believe you guys are talking about the API command before that which is BankAccount_Get having a response like,

{
    "accounts": [
        {
            "plaidAccountID": "MQGxKWAK87urmzvBDZp5FK1QGyJG8Buegl3Ly",
            "routingNumber": "011401533",
            "accountNumber": "1111222233330000",
            "addressName": "Plaid Checking",
            "alreadyExists": false,
            "ownershipType": "",
            "isSavings": false,
            "mask": "0000"
        },
        {
            "plaidAccountID": "1g3x9XG9Rlso96y8kvQqidQRZAmZgqtZdK4pr",
            "routingNumber": "011401533",
            "accountNumber": "1111222233331111",
            "addressName": "Plaid Saving",
            "alreadyExists": false,
            "ownershipType": "",
            "isSavings": true,
            "mask": "1111"
        }
    ],
    "plaidAccessToken": "access-sandbox-0a26517a-1429-47df-85cc-293983a97fd6",
    "jsonCode": 200,
    "requestID": "iFvBq8"
}

and you guys would like alreadyExists to return the correct value which should have been true. Did i get that correct?

Test steps so i don't forget,

1. In NewDot add a bank account. Use wells fargo test account details "user_good".
2. Open network tab once we see the success screen,
3. Click Continue, check BankAccount_Get response.
4. atm its response shows incorrect values for alreadyExists but we need to correct this.

Can you verify @parasharrajat then we'll create an issue to investigate and tackle?

[parasharrajat]

Yeah, that is correct. If alreadyExists can't be used for partial set accounts. Then we can add another key.

[chiragsalian]

Sorry for the delay, swamped with a couple of other tasks. Anyway i just created a backend issue for this - https://github.com/Expensify/Expensify/issues/214332. Shall we place this issue on HOLD until the backend issue has been resolved?

[ctkochan22]

I'm trying to reproduce, but I think i'm doing something wrong.

1. Add a Personal Bank Account. Pick wells fargo. Log in with "user_good". Pick the 0000 (or any account).
2. Do it again, and log into wells fargo with "user_good". I don't see the 0000 option anymore

[Christinadobrzyn]

Still working on this - not overdue.

@parasharrajat would you be able to provide your insight into what @ctkochan22 wrote above ^?

[parasharrajat]

@ctkochan22 there is an issue only for partially added accounts. As per https://github.com/Expensify/App/issues/9085#issuecomment-1131489962, If you leave the bank account addition flow in the middle, the bank account is still shown in the dropdown, and adding it back will throw an error that this bank account already exists.

Take a look at the analysis on https://github.com/Expensify/App/issues/9085.

[chiragsalian]

An Update - the backend fix is in review and should be merged soon hopefully.

[parasharrajat]

I think PR must have been merged by now. Any update @chiragsalian?

[chiragsalian]

Yes the PR is merged. It went to staging today and should be on production most likely by tomorrow. Once its on production the issue shown above,

will automatically change its state from open to closed.

[parasharrajat]

Unfortunately, I can't see that link due to permissions. Please let me know when it does so that I can move ahead with my PR.

But it should have been moved to PROD so I will update the PR.

[chiragsalian]

Yes the PR is in production

[parasharrajat]

Ok, I am looking at the PR now and it seems after we fixed the backend to correctly identify the existed accounts, we don't need to throw the 'Bank Account Already Exists.' error.

But I added the logic so that the user is taken back to the previous screen when there is no account to add.

---

At this step solution for this issue is complete.

But, I am curious to know "will it look good to show the user a message No bank account is available when there are accounts on Plaid but nothing left to be added on Expensify."

I can check if all the returned accounts are alreadyExists= True, then throw a different error. If no accounts are returned, then I will show `No bank account is available.

https://user-images.githubusercontent.com/24370807/179300470-91615900-ff84-4673-95b8-a71f621bc590.mp4

What do you think @chiragsalian @techievivek ? PR is https://github.com/Expensify/App/pull/9023. It currently has old changes for throwing 'Bank Account Already Exists.' error. I will remove it if not needed.

[parasharrajat]

Bump :arrow_up:

[parasharrajat]

Bump :arrow_up:

[parasharrajat]

@chiragsalian Awaiting your response to complete the PR.

[chiragsalian]

Sorry this slipped my radar.

it seems after we fixed the backend to correctly identify the existed accounts, we don't need to throw the 'Bank Account Already Exists.' error.

agree

But I added the logic so that the user is taken back to the previous screen when there is no account to add.

I'm okay with this but bear in mind that currently they see this error message,

"will it look good to show the user a message No bank account is available when there are accounts on Plaid but nothing left to be added on Expensify."

Hmm i think no. I think the current error message is more verbose i.e., - "Could not find any bank accounts that you haven't already added".

I can check if all the returned accounts are alreadyExists= True, then throw a different error. If no accounts are returned, then I will show `No bank account is available.

That sounds good to me.

Let me know if you have any more questions.

[Christinadobrzyn]

Looks like the PR is in the works? @parasharrajat do you have an update on this one?

[parasharrajat]

I am trying to understand the new API structure by looking at the code. My last changes didn't work anymore as we refactor the whole API architecture of the app. I will have to redo those. I will it in few days.

[parasharrajat]

Looked at some docs and existing code to understand the open API architecture. I am still not sure of the way to do this.

[parasharrajat]

Looks like I need help, I will open a discussion on slack.

[Christinadobrzyn]

Hey @parasharrajat can you add a link to the slack convo here so we can follow? Thanks

[parasharrajat]

I will do that thanks.

[chiragsalian]

Any update here @parasharrajat and could you link the slack conversation too?

[parasharrajat]

So, I was checking again on the code to prepare a post for slack but I didn't find a good way to propose a solution. So I think it is better if continue discussing this here and reach a conclusion asap.

1. Now the backend error is being shown via https://github.com/Expensify/App/blob/01292ee0a9e53d0ee4724c541afb0e8629c17f96/src/components/AddPlaidBankAccount.js#L153.
2. There is no more logic to detect the existing accounts as you fixed the backend.
3. I don't think it is possible to handle the API error codes in https://github.com/Expensify/App/blob/01292ee0a9e53d0ee4724c541afb0e8629c17f96/src/libs/actions/Plaid.js#L24.

So Where should I handle the errors? How can check for the error codes that were present before? Is there any example where error handling is done via the new API?

I am also not able to test the VBA flow as it always goes to PROD in all three environments.

[parasharrajat]

Bump @chiragsalian. Thank you.

[chiragsalian]

discussing this here and reach a conclusion asap.

Hmm, personally I don't feel like discussing on GH is faster versus slack or newDot. But sure we can try it here if you prefer.

Firstly I'm unable to test on dev and staging too. Its also going to prod endpoint. Not sure what happened here but I'll ask others.

Good questions, so let's tackle them one at a time.

Where should I handle the errors?

So with the new API the first thing to check if the API returns them as errors or just sends a push notification with data. I think the way its setup it looks like backend sends an errors and its captured as noBankAccountAvailable here which is what is displayed in the link you posted.

If the error is not captured there then we've to update the backend to throw an error properly for our scenarios.

How can check for the error codes that were present before?

The json code is returned in the API response so you can check it in the network tab. But via code we're moving away from checking the json code. Instead, if more error messages are needed then we should remove this line and then the value should be set dynamically in the backend when throwing the error.

Is there any example where error handling is done via the new API?

Well if there is any error handled by the front end then its done in failureData like so. But that's only applicable when its a single case and we want to show one error message regardless of what the backend actually failed on. If we want more error cases then the backend is now responsible for setting the onyx data dynamically.

Does that make sense? Let me know.

As for the next steps, I think it is,

1. Test on staging when we can. Figure out if the API OpenPlaidBankAccountSelector is throwing an error properly when there are no bank accounts available.
2. If it's not throwing the error properly - we've to update the back end.
3. If it's throwing the error properly, then we've to check the failureData is setting the onyx error value properly and it's being displayed.

Let me know if that makes sense.

[Christinadobrzyn]

I think we're back to reviewing proposals? Is that correct @parasharrajat? Should this price be raised, have you been working a lot on this Rajat?

[parasharrajat]

No price increase is needed. We are not looking for proposals. I was trying to fix it but a lot has been changed and my last solution does not apply anymore. I think this issue is no valid but I am trying to adjust the solution based on the new architecture. I will try the above suggestion today and let you know what should be done next for it.

[parasharrajat]

No update for now. I will post one soon.

[chiragsalian]

Waiting for an update by rajat when he gets the chance.

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[parasharrajat]

Due to the implementation of Open API and the unavailability of documentation, I am not sure how should I handle this case.

Technically, already existing bank account detection was fixed on the backend after I pointed it out and error handling was changed on the front.

@chiragsalian what do you suggest I should do next? Could you please update the OP for new behaviour?

Otherwise, I feel like there is no action left for me here. I will close the PR.

[chiragsalian]

Hmm im not sure where we currently stand since so many APIs have changed lately. I'll retest this today and decide on the next steps.

[chiragsalian]

I just retested. Looks like the error is now received appropriately in the onyxData. This means there is a proper error message when the same back account is selected.

Hence I don't think there is anything to be done here. Maybe in the future, we can clear up how the error message is shown but the original issue mentioned in this GH is resolved so closing it out.

[parasharrajat]

Thanks, Chirag. @Christinadobrzyn Could you please take care of the payment for this? Original job posting where contract is live https://www.upwork.com/jobs/Handle-API-response-inVBA-flow-when-tapping-finish-Set-7185-Expensify_~01c21d7847d9c14366