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

PAID [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially #11512

Closed
kbecciv opened this issue Oct 1, 2022 · 42 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@kbecciv
Copy link

kbecciv commented Oct 1, 2022

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 URL https://staging.new.expensify.com/
  2. Login with any account
  3. Create a Workspace (if it has not been created before)
  4. Go to Settings->Workspace->Connect bank account->Connect online with Plaid
  5. Complete the bank account creation process
  6. Verify that the workspace options are available
  7. Go to Settings->Workspace->Connect bank account->Disconnect bank account
  8. Ensure that the workspace options are no longer available
  9. Check the Workspace Setting, make sure all options are locked now

Expected Result:

Workspace options become unavailable

Actual Result:

Workspace options remain available initially after disconnect bank account

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • IOS
  • Android
  • mWeb
  • Desktop

Version Number: 1.11.2.0

Reproducible in staging?: Yes

Reproducible in production?: Yes

Email or phone of affected tester (no customers): any

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5758053_Recording__2136.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@melvin-bot
Copy link

melvin-bot bot commented Oct 1, 2022

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

@danieldoglas
Copy link
Contributor

This is indeed something that should be automatically updated. Checked and can be exported.

@danieldoglas danieldoglas added the External Added to denote the issue can be worked on by a contributor label Oct 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 3, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 3, 2022

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

@melvin-bot melvin-bot bot changed the title Web - Workspace - After deleting a bank account, workspace options remain available initially [$250] Web - Workspace - After deleting a bank account, workspace options remain available initially Oct 3, 2022
@danieldoglas danieldoglas removed their assignment Oct 3, 2022
@laurenreidexpensify
Copy link
Contributor

@ItsJustTomDev
Copy link

So far from what I can see is that when you disconnect your bank account, you still have access to the "issue cards" page.

function resetFreePlanBankAccount() {

This function gets called when you disconnect your bank, so what does it do? Well it tells the API (backend) to update that this user with this and this ID has disconnect there bank account, but then there is no check of that in this page:

const WorkspaceCardPage = props => (

There is no check that checks that there is a bank account connect to this user, so it will always show that the cards are ready.
So how would we fix this? There is actually already some kind of implementation that fixes this issue.

in the WorkspaceBankAccountPage is a fix, that checks if there is a bank account yes or the no.

Here on component mount, it checks if there is a bank account connected,
This is the function its calling:

and this function is basically just checking with the backend if this user has a bank account connected.
So the fix is very simple, we just implement getShouldShowPage function into the cards page, we check it on mount, and if there is no open bank, we either show a different component, or we disable everything.

@laurenreidexpensify laurenreidexpensify added Weekly KSv2 and removed Daily KSv2 labels Oct 5, 2022
@mananjadhav
Copy link
Collaborator

@ItsJustTomDev Thanks for proposal. I am not sure I agree with the getShouldShowPage implementation, because we already have WorkspacePageWithSections page with multiple checks.

@kbecciv Can you confirm if its happening only on the Cards page? or other pages too?

@kbecciv
Copy link
Author

kbecciv commented Oct 7, 2022

@mananjadhav Its happening on other pages as well.

@ItsJustTomDev
Copy link

@mananjadhav @kbecciv, alright guys, I have not seen the WorkspacePageWithSections 😅
I will setup a local dev environment up tomorrow so I can debug this issue, there should be something wrong with fetching the data.

@melvin-bot melvin-bot bot added the Overdue label Oct 14, 2022
@laurenreidexpensify
Copy link
Contributor

@ItsJustTomDev are you still following up on this? Thanks

@melvin-bot melvin-bot bot removed the Overdue label Oct 16, 2022
@puneetlath puneetlath added the Bug Something is broken. Auto assigns a BugZero manager. label Oct 19, 2022
@mananjadhav
Copy link
Collaborator

@laurenreidexpensify I think we should double this

@laurenreidexpensify laurenreidexpensify changed the title [$250] Web - Workspace - After deleting a bank account, workspace options remain available initially [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially Oct 21, 2022
@bernhardoj
Copy link
Contributor

Proposal

As @ItsJustTomDev mentioned, when we disconnect bank account, resetFreePlanBankAccount is called. Inside of it, we merge the REIMBURSEMENT_ACCOUNT with new achData. However, we forget to merge the "state" property (to reset it). So, we can just add state with value of BankAccount.STATE.DELETED.

function resetFreePlanBankAccount() {
    const bankAccountID = lodashGet(store.getReimbursementAccountInSetup(), 'bankAccountID');
    if (!bankAccountID) {
        throw new Error('Missing bankAccountID when attempting to reset free plan bank account');
    }
    if (!store.getCredentials() || !store.getCredentials().login) {
        throw new Error('Missing credentials when attempting to reset free plan bank account');
    }

    const achData = {
        useOnfido: true,
        policyID: '',
        isInSetup: true,
        domainLimit: 0,
        currentStep: CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT,
+        state: BankAccount.STATE.DELETED,
    };

    API.write('RestartBankAccountSetup',
        {
            bankAccountID,
            ownerEmail: store.getCredentials().login,
        },
        {
            optimisticData: [{
                onyxMethod: 'merge',
                key: ONYXKEYS.REIMBURSEMENT_ACCOUNT,
                value: {achData, shouldShowResetModal: false},
            },
            {
                onyxMethod: 'set',
                key: ONYXKEYS.REIMBURSEMENT_ACCOUNT_DRAFT,
                value: null,
            }],
        });

    Navigation.navigate(ROUTES.getBankAccountRoute());
}

@melvin-bot melvin-bot bot removed the Weekly KSv2 label Oct 25, 2022
@mananjadhav
Copy link
Collaborator

@kbecciv Can you help me with some form data for Bank Account creation?

I am stuck at this step:

image

@laurenreidexpensify laurenreidexpensify added the Reviewing Has a PR in review label Oct 31, 2022
@laurenreidexpensify
Copy link
Contributor

offers sent on Upwork

@mananjadhav
Copy link
Collaborator

@kbecciv Quick bump. Can you help me with the QA steps for this one?

@kbecciv
Copy link
Author

kbecciv commented Nov 2, 2022

@mananjadhav Sorry, I missed you message earlier.
Please use the steps for adding fully verified withdrawal account credentials:

  1. Navigate to the add bank account modal in Workspace settings
  2. Verify the Add bank account modal is displayed with 2 options to add bank accounts (Log in and Manual)
  3. Select plaid modal option
  4. Select Fidelity bank
  5. Enter the test credentials: Username: user_good | Password: pass_good
  6. Choose the savings account "Plaid Saving11122XXXXXX111"
  7. In the company information fill out the fields with the required test credentials
    CompanyName: Alberta Bobbeth Charleson
    CompanyTaxID: 123456789
    First Name: Alberta
    Last Name: Charleson
  8. Verify you're able to continue to the personal information step.
  9. Fill out the fields with the required test credentials above
  10. Verify you're able to continue.
  11. In the beneficial owners step toggle the last 2 checkboxes (terms and conditions and true information)
  12. Confirm you're able to continue and reach the validate step.
  13. Verify the bank account was added to the workspace

@mananjadhav
Copy link
Collaborator

Okay thanks for the comment @kbecciv. Will I need to upload files with onfido with the given credentials?

@puneetlath
Copy link
Contributor

@mananjadhav I don't think you should need to. Did you try it?

@mananjadhav
Copy link
Collaborator

@puneetlath I'll be up in 2 hours, will test and confirm.

@mananjadhav
Copy link
Collaborator

PR Review is done. For some reason the flow didn't work for my usual account. I logged in from another account and the flow with the test information worked. Thanks @kbecciv for the help here.

@melvin-bot
Copy link

melvin-bot bot commented Nov 4, 2022

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

  • A regression test has been added or updated so that the same bug will not reach production again. Link to the updated test here:
  • The PR that introduced the bug has been identified. Link to the PR:
  • 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:
  • A discussion in #contributor-plus 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:
  • Payment has been made to the issue reporter (if applicable)
  • Payment has been made to the contributor that fixed the issue (if applicable)
  • Payment has been made to the contributor+ that helped on the issue (if applicable)

2 similar comments
@melvin-bot

This comment was marked as duplicate.

@melvin-bot

This comment was marked as duplicate.

@puneetlath
Copy link
Contributor

@mananjadhav could you help me with identifying the PR that caused the bug?

@mananjadhav
Copy link
Collaborator

@puneetlath I tried checking the PR, but the file has gone through multiple refactors. I'll try to take another stab at it.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@laurenreidexpensify
Copy link
Contributor

Everyone is hired in Upwork

@mvtglobally
Copy link

Issue not reproducible during KI retests. (Second week)

@laurenreidexpensify
Copy link
Contributor

Oh this was deployed to prod 8 days ago but for some reason the automation failed to flag it #12279. Updating now.

@laurenreidexpensify laurenreidexpensify changed the title [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially Payment due 2022-10-16 [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially Nov 16, 2022
@laurenreidexpensify
Copy link
Contributor

laurenreidexpensify commented Nov 16, 2022

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

  • [@puneetlath @mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
  • [@puneetlath @mananjadhav] 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.
  • [@puneetlath @mananjadhav] 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:
  • [@laurenreidexpensify] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/245103
  • [@laurenreidexpensify] Applicable payments have been made. This potentially includes the issue reporter, the contributor that fixed the issue, and/or the contributor+ that helped on the issue.

@laurenreidexpensify
Copy link
Contributor

@bernhardoj @mananjadhav payments have been issued in Upwork

@puneetlath puneetlath changed the title Payment due 2022-10-16 [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially Payment due 2022-11-16 [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially Nov 16, 2022
@puneetlath
Copy link
Contributor

@mananjadhav any luck with the offending PR?

@mananjadhav
Copy link
Collaborator

Well @puneetlath not so much. I traced the changes to the following PRs:

  1. src/libs/actions/ReimbursementAccount/resetFreePlanBankAccount.js file to this Improve VBA logic #6230, where we added the file, but I am not sure if we had the same object passed as this PR is like a refactor.

  2. src/pages/workspace/WorkspacePageWithSections.js changes to PR Delay section display until ACH state has loaded #9138, where we changed the hasVBA check to BankAccount.STATE.OPEN

To further find the offending PR, I'll have to dig further to get the context of the changes. Do one of these PRs help?

@laurenreidexpensify laurenreidexpensify changed the title Payment due 2022-11-16 [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially PAID [$500] Web - Workspace - After deleting a bank account, workspace options remain available initially Nov 18, 2022
@mvtglobally
Copy link

Issue not reproducible during KI retests. (Third week) Can we close it?

@laurenreidexpensify
Copy link
Contributor

laurenreidexpensify commented Nov 23, 2022

I'm just having an internal convo here on whether we want to include this in regression testing. As soon as that is firmed I'll close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

8 participants