[HOLD for payment 2023-04-24] [$2000] Connect Bank Account - Plaid triggered again after several clicks on selector page in offline mode

[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. Open NewDot Android app and login
2. Go to Setting > Payments
3. Click Add payment method
4. Click Bank account
5. Proceed with Plaid identification until selector page is displayed
6. Go offline
7. Select account 0000 and try to submit clicking the button
8. Select account 1111 and try to submit clicking the button
9. Go online

Expected Result:

User can proceed with submitting chosen account

Actual Result:

Plaid triggered again after several clicks on selector page in offline mode

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.2.82.3

Reproducible in staging?: Yes

Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5972180_video_51.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f856df63ae19dee5
• Upwork Job ID: 1637878648446828544
• Last Price Increase: 2023-04-04

[MelvinBot]

Triggered auto assignment to @slafortune (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[MelvinBot]

@slafortune Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MelvinBot]

@slafortune Eep! 4 days overdue now. Issues have feelings too...

[slafortune]

Looks like the external label should be used here 👍

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~01f856df63ae19dee5

[MelvinBot]

Current assignee @slafortune is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @0xmiroslav (External)

[MelvinBot]

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

[slafortune]

No proposals yet

[MelvinBot]

@slafortune @grgia @0xmiroslav this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[hoangzinh]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Plaid triggered again after go online again in the select bank accounts screen

What is the root cause of that problem?

The root cause is come from this PR https://github.com/Expensify/App/pull/14827/files#diff-2a35d342f7be1cc5c1a8f02937f635a5c7083ec66b64b400027d65087d400e09R90-R98

Which it always re-run Plaid again when we are coming back from offline

What changes do you think we should make in order to solve the problem

I think we can add more condition to skip re-run logic as metioned in the RCA to match with our new expectation. More details, in this case, we would like keep the page as it's if user already logged in with Plaid. There are 2 cases here:

• The login process is successfully => we can verify this case by checking if we have bankAccounts data in Onyx
• The login process is failed => we can verify this case by checking if we have errors data in Onyx

If either 2 above cases are satisfy, we can assume user has finished Plaid bank login step and don't re-run Plaid again we are coming back from offline.

Beside that, I found that those logics also used in the componentDidAmount so we can make a function to implement those logics, then reuse it in both componentDidAmount and componentDidUpdate places

Code reference:

const haveFinishedPlaidBankLogin = () => (
    !_.isEmpty(lodashGet(this.props.plaidData, 'bankAccounts'))
    || !_.isEmpty(lodashGet(this.props.plaidData, 'errors'))
);

if (!prevProps.network.isOffline || this.props.network.isOffline || haveFinishedPlaidBankLogin()) {
    return;
}

[0xmiros]

@hoangzinh thanks for your proposal. You explained root cause correctly but not about solution.
I don't think plaidBankAccounts.length > 0 check is enough.
Also, how will you test your changes in dev mode? For now, plaid points to production in dev/production. It points to sandbox only in staging.

[hoangzinh]

Also, how will you test your changes in dev mode? For now, plaid points to production in dev/production. It points to sandbox only in staging.

That's explain why I couldn't pass step authentication in Plaid. Last week, I tried on local dev but unsuccessfully so I read code and do some check on staging directly to come with current proposal.

I don't think plaidBankAccounts.length > 0 check is enough.

Might be you're right. I just looked at the code here and make a guest on that condition, may be I missed 'bankName' 🤔

[MelvinBot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[MelvinBot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.0-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix Plaid triggered again on selector bank accounts page after coming back from offline #17136

If no regressions arise, payment will be issued on 2023-04-24. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[MelvinBot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@0xmiroslav / @grgia] The PR that introduced the bug has been identified. Link to the PR:
• [@0xmiroslav / @grgia] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@0xmiroslav / @grgia] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@slafortune] Determine if we should create a regression test for this bug.
• [@0xmiroslav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@slafortune] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[hoangzinh]

Follow up with this comment #15861 (comment)

@situchan and I discussed internally and we're happy with this split:

• Share equal compensation 50/50 (1k for each) because 2 proposals made complete this GH issue together.
• Bonus/penalty speed rule applies to assigned contributor (PR author)

To be clear:

• In case don't have bonus speed: 1k for @situchan, 1k for me
• In case having bonus speed: 1k for @situchan, 2k for me.

Please help us on it after the payment hold date.
cc @grgia @0xmiroslav

[situchan]

https://ex-syt.glitch.me/?issue=15861&pr=17136

Here's the Issue timeline analysis:
🐛 Issue created at: Fri, 10 Mar 2023 21:42:59 GMT
🧯 Help Wanted at: Fri, 07 Apr 2023 10:03:28 GMT
🕵🏻 Contributor assigned at: Fri, 07 Apr 2023 10:03:51 GMT
🛸 PR created at: Fri, 07 Apr 2023 15:20:17 GMT
🎯 PR merged at: Wed, 12 Apr 2023 11:56:07 GMT
⌛ Business Days Elapsed between assignment and PR merge: 3

💰 Timeline Bonus/Penalty: 50% Bonus! 🎉

[hoangzinh]

Friendly bump for payment date.

[grgia]

cc @slafortune for payments

[slafortune]

Sorry for my delay - sent offers to you both @hoangzinh and @situchan

[allroundexperts]

@slafortune You sent an offer to me as well by mistake 😄

[slafortune]

Alrighty - all squared away! Thanks so much @allroundexperts !

NOW I've sent you an offer @situchan

[0xmiros]

@slafortune please send offer to me too 🙂

[slafortune]

Sorry about that @0xmiroslav ! What a struggle today 😂 - sent you an offer too!

[0xmiros]

@slafortune accepted, thanks!

[0xmiros]

@slafortune I am also eligible for 50% bonus in case you missed. Thanks

[slafortune]

OH my gosh @0xmiroslav ! No, I knew it, just totally forgot to update that in the original! - - - sorry for the extra run around on this fabulous Friday! Another invite sent!

[hoangzinh]

@slafortune accepted, thanks

[grgia]

@slafortune are we good to close this one out?

[slafortune]

@grgia and @0xmiroslav can you complete the check list here - then if there is no regression test needed, we can close this out - everyone is paid

[0xmiros]

@slafortune no PRs caused regression but missed this edge case while refactoring VBBA setup flow in #11162. As this is unusual scenario, no regression test needed.

[slafortune]

Great! Thanks so much @0xmiroslav !