Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 of 6 tasks
lanitochka17 opened this issue Apr 9, 2024 · 55 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Apr 9, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com 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): applausetester+vd_ios040824@applause.expensifail.com
Issue reported by: Applause - Internal Team

Action Performed:

Pre-requisite: user must be logged in

  1. Create a new workspace
  2. Go to Workspace > More features and enable Workflows
  3. Go to Workspace > Workflows and tap on "Connect bank account"
  4. Go back
  5. Disable "Make or track payments" and enable it again
  6. 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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

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 OwnerCurrent Issue Owner: @isabelastisser
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 9, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

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

@lanitochka17
Copy link
Author

@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
Copy link
Contributor

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:

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:

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:
image

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
Copy link
Contributor

ahmedGaber93 commented Apr 10, 2024

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.

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

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)

@isabelastisser 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 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
Copy link

melvin-bot bot commented Apr 10, 2024

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

Copy link

melvin-bot bot commented Apr 10, 2024

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

@ikevin127
Copy link
Contributor

ikevin127 commented Apr 10, 2024

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:

  1. 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.

  2. 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.

  1. 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.

  1. 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
Copy link
Contributor

@ishpaul777 please review the proposals above. Thanks!

@ishpaul777
Copy link
Contributor

ishpaul777 commented Apr 11, 2024

reviewing now 👀

@ishpaul777
Copy link
Contributor

ishpaul777 commented Apr 11, 2024

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

Copy link

melvin-bot bot commented Apr 11, 2024

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

@allgandalf
Copy link
Contributor

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
Copy link
Contributor

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

@allgandalf
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

ikevin127 commented Apr 11, 2024

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

  1. 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 ?
  2. 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
Copy link
Contributor

does that sound good?

Alright @ishpaul777 , thanks for considering

@ishpaul777
Copy link
Contributor

ishpaul777 commented Apr 11, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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

@trjExpensify 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
@lakchote
Copy link
Contributor

Thank you all for your proposals and @ishpaul777 for your review.

After careful consideration, I'm leaning towards @ikevin127's proposal, a split doesn't make sense here.

Why @ikevin127's proposal? While @ahmedGaber93's proposal can work, it doesn't address the root cause of the problem.

It doesn't cover setting the reimburserEmail optimistically only if the user has a VBBA account. @ikevin127 address the root cause of the issue with his proposal.

@ishpaul777
Copy link
Contributor

ishpaul777 commented May 8, 2024

Given that this take more just the proposal solution i dont this bounty split makes sense anymore, And if we dont have a viable solution yet i think we should consider reapplying help wanted open this for more proposals again. Does that sound good? @trjExpensify @ikevin127 @lakchote

@ahmedGaber93
Copy link
Contributor

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
Copy link
Contributor

lakchote commented May 9, 2024

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

  1. 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.

  2. 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.

  3. 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
Copy link
Contributor

  1. 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.

  1. 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
Copy link
Contributor

ishpaul777 commented May 9, 2024

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
Copy link
Contributor

@ikevin127 I will review updated proposal over the weekend

@lakchote
Copy link
Contributor

@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 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 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 melvin-bot bot removed the Reviewing Has a PR in review label May 30, 2024
Copy link

melvin-bot bot commented May 30, 2024

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

Copy link

melvin-bot bot commented May 30, 2024

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:

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

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

Copy link

melvin-bot bot commented May 30, 2024

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
Copy link
Contributor

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

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

Do we agree 👍 or 👎.

@ikevin127
Copy link
Contributor

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

Screenshot 2024-05-30 at 14 51 33

cc @isabelastisser @lakchote

@lakchote lakchote changed the title [HOLD for payment 2024-06-06] [$250] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account [HOLD for payment 2024-06-06] [$500] [Workflows] Bank account and Authorized payer buttons displayed instead of Connect bank account May 31, 2024
Copy link

melvin-bot bot commented May 31, 2024

Upwork job price has been updated to $500

@lakchote
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jun 6, 2024
@isabelastisser
Copy link
Contributor

@isabelastisser
Copy link
Contributor

The offers were sent in Upwork. All set!

@ishpaul777
Copy link
Contributor

Offer accepted! Thanks!

@ikevin127
Copy link
Contributor

Offer accepted, thank you!

@ishpaul777

This comment was marked as resolved.

@ishpaul777
Copy link
Contributor

nvm my last comment i got it confused with another issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

10 participants