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

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

Closed
2 of 6 tasks
m-natarajan opened this issue Jul 31, 2024 · 26 comments
Closed
2 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff

Comments

@m-natarajan
Copy link

m-natarajan commented Jul 31, 2024

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

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

Screenshots/Videos

Bug6558584_1722458559343.20240801_034943.mp4

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
@m-natarajan m-natarajan added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jul 31, 2024
Copy link

melvin-bot bot commented Jul 31, 2024

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.

Copy link
Contributor

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

Copy link

melvin-bot bot commented Jul 31, 2024

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

@marcaaron marcaaron added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 DeployBlocker Indicates it should block deploying the API labels Jul 31, 2024
@marcaaron
Copy link
Contributor

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

@arosiclair
Copy link
Contributor

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

NJ-2020 commented Aug 1, 2024

@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

@marcaaron marcaaron added the External Added to denote the issue can be worked on by a contributor label Aug 2, 2024
@melvin-bot melvin-bot bot changed the title Accounting - Not here page opens after disconnecting QBO when it is connected via Old Dot [$250] Accounting - Not here page opens after disconnecting QBO when it is connected via Old Dot Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 2, 2024

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

@daledah
Copy link
Contributor

daledah commented Aug 2, 2024

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

const waitForNavigate = useWaitForNavigation();

@daledah
Copy link
Contributor

daledah commented Aug 2, 2024

My proposal modified to add an alternative solution part

Copy link

melvin-bot bot commented Aug 5, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 5, 2024
@melvin-bot melvin-bot bot removed the Overdue label Aug 7, 2024
@daledah
Copy link
Contributor

daledah commented Aug 8, 2024

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

Copy link

melvin-bot bot commented Aug 9, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 9, 2024
@eVoloshchak
Copy link
Contributor

@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?

Copy link

melvin-bot bot commented Aug 14, 2024

@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!

@melvin-bot melvin-bot bot added the Overdue label Aug 14, 2024
@marcaaron
Copy link
Contributor

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

@eVoloshchak
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Aug 15, 2024
@daledah
Copy link
Contributor

daledah commented Aug 15, 2024

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.

@Expensify Expensify deleted a comment from github-actions bot Aug 15, 2024
@marcaaron
Copy link
Contributor

marcaaron commented Aug 15, 2024

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.

@marcaaron marcaaron added Internal Requires API changes or must be handled by Expensify staff and removed 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 Aug 15, 2024
@sonialiap sonialiap added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

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

Copy link

melvin-bot bot commented Aug 19, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
Copy link

melvin-bot bot commented Aug 21, 2024

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

@marcaaron
Copy link
Contributor

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

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. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff
Projects
No open projects
Status: Done
Development

No branches or pull requests

8 participants