Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$250] Accounting - Not here page opens after disconnecting QBO when it is connected via Old Dot #46618

Closed m-natarajan closed 2 months ago

m-natarajan commented 3 months ago

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: 9.0.15-4 Reproducible in staging?: y Reproducible in production?: n 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 Expensify/Expensify Issue URL: Issue reported by: applause internal team Slack conversation:

Action Performed:

  1. Go to staging.new.expensify.com
  2. Create a new workspace.
  3. Go to workspace settings and do not enable accounting.
  4. Log in to Old Dot with the same account.
  5. Connect to QBO on Old Dot.
  6. Return to New Dot.
  7. Go to Accounting.
  8. Click 3-dot menu next to QBO > Disconnect.
  9. Disconnect QBO.

Expected Result:

App will remain on Accounting page.

Actual Result:

Not here page opens after disconnecting QBO when it is connected via Old Dot.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/2c9d14fa-8181-4329-b13f-0ea03475c756

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01141a4cb4a1c084c4
  • Upwork Job ID: 1819202515595068782
  • Last Price Increase: 2024-08-09
Issue OwnerCurrent Issue Owner: @eVoloshchak
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

github-actions[bot] commented 3 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @marcaaron (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

marcaaron commented 3 months ago

imo should not block the deploy. @arosiclair might have some insight into this one?

arosiclair commented 3 months ago

Haven't looked at the connect/disconnect functionality yet. Looks like this PR is closely related though. @carlosmiceli @NJ-2020 can you take a look?

NJ-2020 commented 3 months ago

@arosiclair This is because the areConnectionEnabled is false but there is a connection QB data, so when we disconnect the QB the connection data become empty and the checking condition become false, so it show 404 page not found

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01141a4cb4a1c084c4

melvin-bot[bot] commented 3 months ago

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

daledah commented 3 months ago

Proposal

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

Not here page opens after disconnecting QBO when it is connected via Old Dot.

What is the root cause of that problem?

When we connect an integration in OD, the areConnectionsEnabled still is false in ND because the user did not explicitly enable accounting on ND.

So when the connection is disconnected in ND, it shows 404 page because it's now an inaccessible page.

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

If areConnectionsEnabled is false, before removing the connection we need to navigate to the workspace profile/workspace list/other fallback page first

To do it we can update this to

if (!policy.areConnectionsEnabled) {
    Navigation.navigate(ROUTES.WORKSPACE_PROFILE.getRoute(policyID ?? '-1'));

    InteractionManager.runAfterInteractions(() => {
        removePolicyConnection(policyID, connectedIntegration);
    });

    return;
}
removePolicyConnection(policyID, connectedIntegration);

The route can be changed depending on which page we want to fallback to in this case.

What alternative solutions did you explore? (Optional)

From my testing InteractionManager.runAfterInteractions works reliably to make sure we only remove the Onyx data after navigation so the 404 does not show up briefly. But an even more sure way is to use useWaitForNavigation to wait for navigation to complete, like in https://github.com/Expensify/App/blob/5005f595661607d563d8ae3bd72e763971d9d873/src/pages/Search/AdvancedSearchFilters.tsx#L55

daledah commented 3 months ago

My proposal modified to add an alternative solution part

melvin-bot[bot] commented 3 months ago

@eVoloshchak, @sonialiap, @marcaaron Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

marcaaron commented 3 months ago

@eVoloshchak are you able to look at this soon?

eVoloshchak commented 3 months ago

When we connect an integration in OD, the areConnectionsEnabled still is false in ND because the user did not explicitly enable accounting on ND. So when the connection is disconnected in ND, it shows 404 page because it's now an inaccessible page.

@daledah, areConnectionsEnabled shouldn't be false if QBO is connected

Screenshot 2024-08-07 at 14 05 33

Here the QBO is connected, which means connectedIntegration is not empty, shouldn't areConnectionsEnabled be true? Can we use the withPolicyConnections hook to re-fetch this?

before removing the connection we need to navigate to the workspace profile/workspace list/other fallback page first

It should stay on the same page (Accounting), just with QBO disconnected

Screenshot 2024-08-07 at 14 10 07
daledah commented 3 months ago

@daledah, areConnectionsEnabled shouldn't be false if QBO is connected

@eVoloshchak But it is (feel free to check the Onyx data to validate this, even after logged out and logged in, areConnectionsEnabled is still false), areConnectionsEnabled is only true if the user explicitly enables it in More features. In this case, the user did not explicitly enable it in More features, but just connect directly via OldDot.

Looking at this condition, we can see this is a known case, because we're checking both areConnectionsEnabled and !isEmptyObject(policy?.connections). Even if areConnectionsEnabled is false but policy?.connections have value, we would still display the connection list, but it still doesn't mean that the user enabled Accounting, they didn't.

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

eVoloshchak commented 3 months ago

@daledah, thank you for the explanation The expected result is "App will remain on Accounting page", which would happen only if accounting is enabled in ND. I think the way to achieve this is if you connect a QBO, the Accounting should be automatically enabled for the workspace. Does that seem right to you?

melvin-bot[bot] commented 3 months ago

@eVoloshchak @sonialiap @marcaaron 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!

marcaaron commented 3 months ago

What's the latest on this one @eVoloshchak @daledah ?

eVoloshchak commented 3 months ago

According to a discussion on Slack: Accounting should be automatically enabled for the workspace if user is enabling QBO (or any other integration) in OldDot

Do you agree? If so, we should mark this Internal

daledah commented 3 months ago

The expected result is "App will remain on Accounting page", which would happen only if accounting is enabled in ND. I think the way to achieve this is if you connect a QBO, the Accounting should be automatically enabled for the workspace. Does that seem right to you?

@eVoloshchak IMO we should only enable Accounting if the user explicitly enables it, not inferring from OldDot's connection actions. We should navigate the user properly as suggested in my proposal to the Workspace Profile page so 404 page will not show up briefly.

I think this is a known use case, by looking at this condition. If we should enable accounting whenever we enable any connection in OldDot, that condition will be meaningless and we should only check areConnectionsEnabled.

marcaaron commented 3 months ago

If we should enable accounting whenever we enable any connection in OldDot, that condition will be meaningless and we should only check areConnectionsEnabled.

Hmm... I think I disagree, but could use @trjExpensify's thoughts here. I believe the toggle design is there to entice people to "enable" them (which triggers an upgrade flow).

So, I think as soon as you add a connection it is very logical for us to set areConnectionsEnabled: true.

But if you remove one, we don't necessarily need to toggle it off and probably shouldn't. It's still "enabled" because you showed some previous interest in setting up an accounting package. And, we should only toggle it on when you add one not when you remove the final one or something.

So, I agree with @eVoloshchak that this should be internal.

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

sonialiap commented 3 months ago

@bfitzexpensify I'm OOO Aug 19-30, adding leave buddy. Status: internal issue, waiting for solution and build

melvin-bot[bot] commented 3 months ago

@eVoloshchak, @sonialiap, @marcaaron, @bfitzexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 2 months ago

@eVoloshchak, @sonialiap, @marcaaron, @bfitzexpensify Huh... This is 4 days overdue. Who can take care of this?

marcaaron commented 2 months ago

This is on production now so I think we can close it out.