Routing number error does not drop you back into connect manually flow

[aldo-expensify]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Navigate to /bank-account/
2. Click "Connect manually"
3. Input an invalid routing number, i.e. 12312312344
4. Input any bank number, i.e. 123
5. Click "Save & continue" button

Note: The incorrect behaviour only happens the first time we get the error, to replicate it again, reload the page.

Expected Result:

See the loading transition, show again the same form highlighting the error in the routing number

Actual Result:

After the backend validation fails, the user is taken back to the step before choosing "Connect manually"

Screen.Recording.2021-09-22.at.11.57.57.AM.mov

Workaround:

The user can just click again "Connect manually" and proceed.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

View all open jobs on GitHub

[MelvinBot]

Triggered auto assignment to @CortneyOfstad (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[MelvinBot]

Triggered auto assignment to @Luke9389 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[Luke9389]

I agree with the Daily label here. I'm working on a separate N6 feature (design doc) so I'm gonna drop this so someone else can pick it up.

[MelvinBot]

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

[MelvinBot]

Huh... This is 4 days overdue. Who can take care of this?

[aldo-expensify]

I'll work on this.

[marcaaron]

This worked fine before. Was there a regression somewhere?

[aldo-expensify]

This worked fine before. Was there a regression somewhere?

investigating...

[aldo-expensify]

@marcaaron I think you are right, I tested version 1.0.75-11 and it seems to work fine:

Screen.Recording.2021-09-10.at.12.58.37.PM.mov

NOTE: 1.0.75-11 is not the most recent version where this was working!

[aldo-expensify]

It was a regression caused by this code:

https://github.com/Expensify/App/blob/main/src/libs/actions/BankAccounts.js#L648-L657

This was introduced in this PR

Basically it causes the BankAccountStep component to be created before we update the withdrawal account data here. At this point achData is equal to {subStep: 'manual'}, which in the past made it work.

When we create the component BankAccountStep before that last update, subStep in Onyx is null when it gets copied in the constructor.

Reverting this change to:

    API.BankAccount_SetupWithdrawal(newACHData)
        .then((response) => {
            Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {loading: false, achData: {...newACHData}});

Fixes the problem with the bug bouncing the user back to the previous view, and the bug described here doesn't seem to be happening anymore. Maybe it got fix some other way? fixed in Onyx? fixed by constantly saving the draft values? 🤷

How would we prefer to proceed?

1. Revert as I'm pointing out here
2. Use my PR about reading the subStep directly form Onyx always?
3. Both

I would choose both, because I don't see any value in the complexity introduced here anymore, so I would revert that. I also still suggest reading the subStep directly from Onyx and avoid double fetching.

But if we wanted to go with the simplest solution, it would be just reverting!

[marcaaron]

Thanks for looking into that 🙇

I agree we should still make subStep the single source of truth as you've called out. That seems like a better solution long term to me.

Looked at the code you mention, but I'm not too familiar with why those changes were made. Maybe we can get @Jag96 to weigh in before proceeding?

[Jag96]

@aldo-expensify I think you hit the nail on the head w/ the investigation.

The reason for that change was to fix an issue where Onyx.merge was overwriting data unexpectedly, more info here. The changes in that PR were to enable us to save previously entered account/routing numbers (issue), but most of that functionality was replaced by #5114 recently.

If you revert the code in question and you can successfully pass the tests from #5114 and #4477 then I think it's ok to revert.

[aldo-expensify]

Thanks @Jag96 , I'll revert that part and test that everything is working fine then!

[aldo-expensify]

I'll pick up this later today (no overdue)

[aldo-expensify]

No overdue, PR waiting for review

[aldo-expensify]

Updated PR with main - resolve conflicts

[MelvinBot]

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