[HOLD for payment 2024-06-06] [$500] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account #39947

lanitochka17 · 2024-04-09T15:04:33Z

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

Version Number: 1.4.61.1
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4482904
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: user must be logged in

Create a new workspace
Go to Workspace > More features and enable Workflows
Go to Workspace > Workflows and tap on "Connect bank account"
Go back
Disable "Make or track payments" and enable it again
Verify "Bank account" and "Authorized payer" buttons are displayed

Expected Result:

"Connect bank account" button should be displayed

Actual Result:

"Bank account" and "Authorized payer" buttons are displayed instead of "Connect bank account"

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

Bug6443564_1712674706894.Lslh2115_1_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01ad1e7b394f8ced40
Upwork Job ID: 1778052154469085184
Last Price Increase: 2024-05-31
Automatic offers:

ishpaul777 | Reviewer | 0
ikevin127 | Contributor | 0

Issue Owner

Current Issue Owner: @isabelastisser

melvin-bot · 2024-04-09T15:04:43Z

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

lanitochka17 · 2024-04-09T15:04:57Z

@isabelastisser FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

allgandalf · 2024-04-09T15:32:43Z

Proposal

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

We see bank account and authorized payer options even when we haven't added bank account

What is the root cause of that problem?

We show bank account & authorized payer based on the condition hasVBA:

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Line 87 in 5b1dbea

const hasVBA = !!policy?.achAccount;

If this is false then we show connect bank account page.

Now when we click on connect bank account for the first time, we set a field inside of ACHaccount reimburser to the admin's email:

App/src/types/onyx/Policy.ts

Lines 187 to 194 in 5b1dbea

    
           type ACHAccount = { 
        
               bankAccountID: number; 
        
               accountNumber: string; 
        
               routingNumber: string; 
        
               addressName: string; 
        
               bankName: string; 
        
               reimburser: string; 
        
           };

So even when we do not complete the add bank account flow, we still set the value of reimburser:

And hence when we toggle the option, it takes data from Onyx as we have stored the value of reimburser temporarily in it and !!policy?.achAccount is set to true then and hence we see the approver and bank account field.

Note : If we refresh the page then hasVBA again becomes flase, as we have not stored the value of reimburser on the BE.

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

We should only set hasVBA to true only when all the fields of ACHAccount are defined.

What alternative solutions did you explore? (Optional)

N/A

ahmedGaber93 · 2024-04-10T02:12:59Z

Proposal

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

BA - Bank account and Authorized payer buttons displayed instead of Connect bank account

What is the root cause of that problem?

We display Bank account or Connect bank account based on this condition.

App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx

Line 87 in 5b1dbea

const hasVBA = !!policy?.achAccount;

but here when we disable Make or track payments we set optimisticData with {reimburser: reimburserEmail} then after API response, report come with "achAccount": null

App/src/libs/actions/Policy.ts

Line 732 in aed0bf0

achAccount: {reimburser: reimburserEmail},

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

We can define hasVBA based on bankAccountID existing not achAccount object

// const hasVBA = !!policy?.achAccount; 
const hasVBA = !!policy?.achAccount?.bankAccountID;

What alternative solutions did you explore? (Optional)

We can also return the correct optimisticData like API response, by set "achAccount": null when disabling Make or track payments

// achAccount: {reimburser: reimburserEmail},
achAccount: reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO ? null : {reimburser: reimburserEmail},

or just set the reimburser not whole achAccount to null
{reimburser: CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_NO ? null : reimburserEmail}
and check it in hasVBA condition hasVBA = !!policy?.achAccount && !isEmptyObject(policy?.achAccount)

melvin-bot · 2024-04-10T13:27:24Z

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

melvin-bot · 2024-04-10T13:27:29Z

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

ikevin127 · 2024-04-10T16:35:38Z

Proposal

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

"Bank account" and "Authorized payer" buttons are displayed instead of "Connect bank account" after clicking the "Connect bank account" button and re-toggling "Make or track payments".

What is the root cause of that problem?

Everytime we toggle "Make or track payments", we call Policy.setWorkspaceReimbursement here we set achAccount: {reimburser: reimburserEmail} here optimistically (optimisticData).

This was introduced by PR #39017, specifically for the scenario where a bank account is already added, in order to be able to change the Authorized payer while offline (optimistically).

The problem with this is that in our case, we don't yet have a bank account added and because the achAccount: {reimburser: reimburserEmail} is set everytime we toggle "Make or track payments", the UI ends up looking like we have a bank account added because of this check, when we don't actually have a bank account added yet.

Note

When offline, the following buttons are always disabled:

"Connect bank account" (when bank is not added yet)
"Bank account" (when bank exists)

The "Authorized payer" button is always enabled because we want to be able to change the Authorized payer even if we're offline, according to PR #39017.

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

In order to maintain the functionality added by PR #39017 and also fix our issue, accounting for both flows (no BA added and with BA added) we have to perform the following changes:

Introduce a new loading variable isLoadingWorkspaceReimbursement for the SetWorkspaceReimbursement call because whenever we toggle "Make or track payments", the call takes a while and by doing this we're now able to show a spinner instead of the UI jumping from the "Connect bank account" button to "Bank account" and "Authorized payer" buttons. Already approved by design here, here and here.
The two repeating checks here and here will be replaced with the following check:

const shouldShowBankAccount = !!bankAccountID && policy?.reimbursementChoice === CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES;

which removes hasVBA variable and instead replaces it with !!bankAccountID which actually checks if a BA exists in order to show the proper UI and styling here.

The following two checks 1.here and 2.here will be replaced with:

// 1
} else if (!!policy?.achAccount && !Policy.isCurrencySupportedForDirectReimbursement(policy?.outputCurrency ?? '')) {

replacing hasVBA directly with !!policy?.achAccount check and:

// 2
newReimbursementChoice = CONST.POLICY.REIMBURSEMENT_CHOICES.REIMBURSEMENT_YES;

removing the hasVBA check as it breaks the flow when we first toggle "Make or track payments" and we have BA added, by not showing the BA.

When offline, I added a check with commit 61afb5b in order to not get infinite loading spinner and instead the optimistic UI.

Videos (without BA / with BA)

MacOS: Chrome / Safari

Without BA	With BA
withoutBA.mov	withBA.mov

isabelastisser · 2024-04-10T20:10:35Z

@ishpaul777 please review the proposals above. Thanks!

ishpaul777 · 2024-04-11T17:04:41Z

reviewing now 👀

ishpaul777 · 2024-04-11T17:27:07Z

Thanks for your proposals all! While all of the proposed solution works i lean of choosing solution from both @ahmedGaber93 and @ikevin127 because checking const hasVBA = !!policy?.achAccount?.bankAccountID in component will ensure we show the options on correct condition while we also want to set the reimburser optimistically only when there is a bank account added.

So i'd suggest we split up the contributor bounty 75% for contributor for implementation and 25% for the other contributor.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2024-04-11T17:27:21Z

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

allgandalf · 2024-04-11T17:30:30Z

But, shouldn’t we check for all the fields @ishpaul777 ? i guess for a bank account to be displayed we must validated that all the fields are present , my proposal mentions the same thing, what do you think ?

ishpaul777 · 2024-04-11T17:34:09Z

hmm, but i dont think there will ever be a case when we have a bankAccountId without other fields or is it? 🤔

allgandalf · 2024-04-11T17:36:10Z

fair point you got not saying no, but why to leave any loophole, suppose if we delete bank account offline, do we clear all the fields optimistically? 🤔

ishpaul777 · 2024-04-11T17:41:15Z

okay good point, for safety i'd agree we can include a check if bankDisplayName is not a empty string, does that sound good?

ikevin127 · 2024-04-11T17:53:16Z

So i'd suggest we split up the contributor bounty 75% for contributor for implementation and 25% for the other contributor.

I'm assuming I'd be the 75% since my proposed solution is also covering the setting of the reimburser optimistically only when there is a bank account added ?
How many splits are going to be in total ?

If nothing's set in stone yet, lemme go ask a few people to post a proposal here mentioning some other achAccount field that was not mentioned yet - this way maybe we can do a 4, maybe even 5 way split of $250 😂

hmm, but i dont think there will ever be a case when we have a bankAccountId without other fields or is it? 🤔

You're correct ✅

allgandalf · 2024-04-11T17:54:49Z

does that sound good?

Alright @ishpaul777 , thanks for considering

ishpaul777 · 2024-04-11T18:02:35Z

I'm assuming I'd be the 75% since my proposed solution is also covering the setting of the reimburser optimistically only when there is a bank account added ?

yes, becuase your RCA was most detailed and easy to follow

How many splits are going to be in total ?

i believe 2 only @ikevin127 and @ahmedGaber93 because the other fields are just to be on safe side, not required.

If nothing's set in stone yet, lemme go ask a few people to post a proposal here mentioning some other achAccount field that was not mentioned yet - this way maybe we can do a 4, maybe even 5 way split of $250 😂

I know this is being said satirically but such comments are not necessary while on a public platform

ikevin127 · 2024-04-11T18:15:25Z

I was joking, pardon me - I just found the split situation hilarious considering we're talking about a $250 bounty 😅

Give this one to @ahmedGaber93, you have full consent to use my side of the solution -> just make sure to test it well (including w/ existing BA scenario).

cc @tylerkaraszewski

ishpaul777 · 2024-04-11T18:19:02Z

Great Thanks for voluntering, i only suggested the split to be fair to everyone

ahmedGaber93 · 2024-05-08T19:48:07Z

Another issue for me in moving forward with this is the bounty split suggested by @lakchote in #39947 (comment), which I don't agree with considering the complexity of the issue (including BE changes), and considering the current bounty is only $250.

@lakchote's decision was fair at this time. @ikevin127 if you found other complex issue in PR which make split not fair for you, don't let this stop you, you can move forward to complete it, and take the full bounty.

lakchote · 2024-05-09T10:26:07Z

@ikevin127 I've made and explained my decision based on facts discussed here.

This was before you've mentioned the necessity for a more complex solution. I try my best to make it fair for everyone by allowing splits, when it's appropriate.
Let's not let make this a blocking point. If you didn't feel like to proceed with the issue considering the split, you could have directly tagged me, to save time and move forward. 3 weeks have passed already.
I've posted up in your PR here asking for design team input, and @trjExpensify posted a comment here.

With that being said, could you detail why a complex solution will need to be done? Is it because it'd require, according to you, extensive frontend changes?

Optimizing the API command SetWorkspaceReimbursement so it takes less time to respond would involve extensive changes and refactoring by porting the main logic from PHP to C++. And even then, I don't think the change would be dramatic regarding the API response time when I look at the existing logic in place. Maybe the frontend solution proposed needs to be adapted instead.
cc @nkuoch as you've worked on setACHDetails(), tell me if you'd agree or not with me.

I'll dig more into it if there needs to be, as I have to handle an important issue that's breaking our 1:1:1 philosophy.

And if we dont have a viable solution yet i think we should consider reapplying help wanted open this for more proposals again.

Let's wait for answer to @trjExpensify's comment here.

ikevin127 · 2024-05-09T22:33:18Z

This was before you've mentioned the necessity for a more complex solution. I try my best to make it fair for everyone by allowing splits, when it's appropriate.

Correct, and I did not have an issue with the split at that time.
However issue came when the PR reviewer mentioned in #40182 (comment) that they don't sit well with the loading spinner flow, which was already triple-approved by design here, here and here.

This was confusing to me and obviously required additional changes, outside of what was already agreed upon when the split reasoning went down. This is why I find it unfair to split the already small bounty, just because the other contributor had a 1-line contribution which in actuality was of little help to the final solution implemented (see updated proposal).

I take responsability here for not communicating well enough as I should've communicated this earlier and better.

Let's not let make this a blocking point. If you didn't feel like to proceed with the issue considering the split, you could have directly tagged me, to save time and move forward. 3 weeks have passed already.

Big part of why the issue stagnated, at least on my part is cause I was confused with the reviewer's comment regarding the already approved loading spinner flow and a final decision from the rest of the team after their comment. Additionally, I also mentioned some possible BE related issues since @trjExpensify pointed out that one of the endpoint responses is too slow and could be improved.

Besides this, while testing the PR I discovered other FE logic issues with the flow, especially when we have a BA added and we toggle "Make or track payments" which require additional changes to the current PR.

Updated proposal

adjusted FE logic to account for "Make or track payments" toggle flow of when BA is added

@lakchote @ishpaul777

ishpaul777 · 2024-05-09T22:45:37Z

I have put the option to move forward here even though i was not aligned with it fully but then this comment made me think we still have this not fully functional even with the loader

If we want something solid here, it requires BE changes. Otherwise, I think FE only changes will only break the current functionality instead of actually improving the flow.

ishpaul777 · 2024-05-09T23:02:43Z

@ikevin127 I will review updated proposal over the weekend

lakchote · 2024-05-10T09:19:42Z

@ikevin127 thank you for updating your proposal and taking the time to answer my questions. I didn't read your message that way. I totally understand your concern and frustration now, regarding the work involved in the loading spinner only to put it on hold at the final stage.

melvin-bot · 2024-05-30T21:26:03Z

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

melvin-bot · 2024-05-30T21:26:07Z

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

Bank account and Authorized payer buttons displayed instead of Connect bank account #40182

If no regressions arise, payment will be issued on 2024-06-06. 🎊

For reference, here are some details about the assignees on this issue:

@ikevin127 requires payment automatic offer (Contributor)
@ishpaul777 requires payment automatic offer (Reviewer)

melvin-bot · 2024-05-30T21:26:08Z

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:

[@ikevin127 / @ishpaul777] The PR that introduced the bug has been identified. Link to the PR:
[@ikevin127 / @ishpaul777] 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:
[@ikevin127 / @ishpaul777] 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:
[@ikevin127 / @ishpaul777] Determine if we should create a regression test for this bug.
[@ikevin127 / @ishpaul777] 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.
[@isabelastisser] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

ikevin127 · 2024-05-30T21:53:18Z

[@ikevin127] The PR that introduced the bug has been identified. Link to the PR: Use policy achAccount instead of reimbursementAccount in WorkspaceWorkflow #38395
[@ikevin127] 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: https://github.com/Expensify/App/pull/38395/files#r1621461968
[@ikevin127] 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: ❌

No need to open a discussion or update the PR review checklist as this was an issue which could've been mitigated by thoroughly testing the feature both while online and offline, which is already a checkbox on the PR author / reviewer checklist.

[@ikevin127] Determine if we should create a regression test for this bug.
[@ikevin127] 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.

Regression Test Proposal

Create a new workspace.
Go to Workspace > More features and enable Workflows.
Go to Workspace > Workflows and under Make or track payments tap on Connect bank account.
Go back.
Disable Make or track payments then enable it again.
Verify that only the Connect bank account button is visible.

Do we agree 👍 or 👎.

ikevin127 · 2024-05-30T22:01:02Z

Note: This issue's bounty should be increased to $500 as per this Slack post, since it was always HIGH priority.

cc @isabelastisser @lakchote

melvin-bot · 2024-05-31T07:47:59Z

Upwork job price has been updated to $500

lakchote · 2024-05-31T07:49:04Z

Note: This issue's bounty should be increased to $500 as per this Slack post, since it was always HIGH priority.

cc @isabelastisser @lakchote

I do agree. @isabelastisser I've increased the Upwork job price.

isabelastisser · 2024-06-06T19:38:06Z

https://github.com/Expensify/Expensify/issues/402811

isabelastisser · 2024-06-06T19:42:52Z

The offers were sent in Upwork. All set!

ishpaul777 · 2024-06-06T19:44:13Z

Offer accepted! Thanks!

ikevin127 · 2024-06-06T19:44:18Z

Offer accepted, thank you!

ishpaul777 · 2024-06-12T18:10:17Z

nvm my last comment i got it confused with another issue

lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 9, 2024

melvin-bot bot assigned isabelastisser Apr 9, 2024

isabelastisser added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Apr 10, 2024

melvin-bot bot changed the title ~~BA - Bank account and Authorized payer buttons displayed instead of Connect bank account~~ [$250] BA - Bank account and Authorized payer buttons displayed instead of Connect bank account Apr 10, 2024

melvin-bot bot assigned ishpaul777 Apr 10, 2024

melvin-bot bot assigned tylerkaraszewski Apr 11, 2024

trjExpensify changed the title ~~[$250] BA - Bank account and Authorized payer buttons displayed instead of Connect bank account~~ [$250] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account Apr 11, 2024

trjExpensify added this to [#whatsnext] #wave-collect Apr 11, 2024

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 30, 2024

melvin-bot bot changed the title ~~[$250] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account~~ [HOLD for payment 2024-06-06] [$250] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account May 30, 2024

melvin-bot bot removed the Reviewing Has a PR in review label May 30, 2024

ikevin127 mentioned this issue May 30, 2024

Use policy achAccount instead of reimbursementAccount in WorkspaceWorkflow #38395

Merged

49 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 6, 2024

isabelastisser closed this as completed Jun 6, 2024

github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Jun 6, 2024

This comment was marked as resolved.

Sign in to view

[HOLD for payment 2024-06-06] [$500] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account #39947

[HOLD for payment 2024-06-06] [$500] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account #39947

Comments

lanitochka17 commented Apr 9, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Apr 9, 2024

lanitochka17 commented Apr 9, 2024

allgandalf commented Apr 9, 2024

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

ahmedGaber93 commented Apr 10, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

melvin-bot bot commented Apr 10, 2024

melvin-bot bot commented Apr 10, 2024

ikevin127 commented Apr 10, 2024 • edited Loading

Proposal

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

What is the root cause of that problem?

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

Videos (without BA / with BA)

isabelastisser commented Apr 10, 2024

ishpaul777 commented Apr 11, 2024 • edited Loading

ishpaul777 commented Apr 11, 2024 • edited Loading

melvin-bot bot commented Apr 11, 2024

allgandalf commented Apr 11, 2024

ishpaul777 commented Apr 11, 2024

allgandalf commented Apr 11, 2024

ishpaul777 commented Apr 11, 2024

ikevin127 commented Apr 11, 2024 • edited Loading

allgandalf commented Apr 11, 2024

ishpaul777 commented Apr 11, 2024 • edited Loading

ikevin127 commented Apr 11, 2024

ishpaul777 commented Apr 11, 2024

ahmedGaber93 commented May 8, 2024

lakchote commented May 9, 2024 • edited Loading

ikevin127 commented May 9, 2024

Updated proposal

ishpaul777 commented May 9, 2024 • edited Loading

ishpaul777 commented May 9, 2024

lakchote commented May 10, 2024

melvin-bot bot commented May 30, 2024

melvin-bot bot commented May 30, 2024

melvin-bot bot commented May 30, 2024

ikevin127 commented May 30, 2024

Regression Test Proposal

ikevin127 commented May 30, 2024

melvin-bot bot commented May 31, 2024

lakchote commented May 31, 2024

isabelastisser commented Jun 6, 2024

isabelastisser commented Jun 6, 2024

ishpaul777 commented Jun 6, 2024

ikevin127 commented Jun 6, 2024

This comment was marked as resolved.

ishpaul777 commented Jun 12, 2024

lanitochka17 commented Apr 9, 2024 •

edited by melvin-bot bot

Loading

ahmedGaber93 commented Apr 10, 2024 •

edited

Loading

ikevin127 commented Apr 10, 2024 •

edited

Loading

ishpaul777 commented Apr 11, 2024 •

edited

Loading

ishpaul777 commented Apr 11, 2024 •

edited

Loading

ikevin127 commented Apr 11, 2024 •

edited

Loading

ishpaul777 commented Apr 11, 2024 •

edited

Loading

lakchote commented May 9, 2024 •

edited

Loading

ishpaul777 commented May 9, 2024 •

edited

Loading