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

QBO – Unable to change preferred exporter in QBO connection #49797

Open
2 of 6 tasks
mvtglobally opened this issue Sep 26, 2024 · 25 comments
Open
2 of 6 tasks

QBO – Unable to change preferred exporter in QBO connection #49797

mvtglobally opened this issue Sep 26, 2024 · 25 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Engineering Weekly KSv2

Comments

@mvtglobally
Copy link

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: v9.0.40-1
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5006859
Email or phone of affected tester (no customers): applausetester+shsb22145@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause Internal Team

Slack conversation:

Action Performed:

Prerequisite
Workspace is connected to QBO
Another Admin is added to the workspace

  1. Navigate to Accounting
  2. Click on Export
  3. Click Preferred Exporter
  4. Select the unchecked workspace admin
  5. Wait for the connection to sync and check the preferred exporter

Expected Result:

The newly selected exporter is displayed with a checkmark

Actual Result:

The selected exporter does not stay selected and the exporter is reverted back to the previous one

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

Bug6615731_1727334878461.2024-09-26_10_05_27.mp4

View all open jobs on GitHub

@mvtglobally mvtglobally added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 26, 2024
Copy link

melvin-bot bot commented Sep 26, 2024

Triggered auto assignment to @johncschuster (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

melvin-bot bot commented Sep 26, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Sep 26, 2024
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.

@mvtglobally
Copy link
Author

We think this is related to #wave-collect - Release 2

@mvtglobally mvtglobally added DeployBlocker Indicates it should block deploying the API and removed DeployBlocker Indicates it should block deploying the API labels Sep 26, 2024
@mananjadhav
Copy link
Collaborator

mananjadhav commented Sep 26, 2024

Proposal

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

  • Unable to update the QBO Preferred exporter

What is the root cause of that problem?

  • In QuickbooksPreferredExporterConfigurationPage we are using qboConfig?.export.exporter as the fallback value for optimistic update. But qboConfig.export can be undefined when we first sync.

if (row.value !== qboConfig?.export?.exporter) {
QuickbooksOnline.updateQuickbooksOnlinePreferredExporter(policyID, {exporter: row.value}, {exporter: qboConfig?.export.exporter ?? ''});
}

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

  • In QuickbooksPreferredExporterConfigurationPage update code to qboConfig?.export?.exporter.

We should also check if there are any other such instances for the update.

What alternative solutions did you explore? (Optional)

@mananjadhav
Copy link
Collaborator

Proposal updated.

@daledah
Copy link
Contributor

daledah commented Sep 26, 2024

Proposal

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

The selected exporter does not stay selected and the exporter is reverted back to the previous one

What is the root cause of that problem?

This is a Pusher Backend bug, as the Onyx data is updated correctly when we select the second admin, but the Pusher data updates the preferred exporter to the original account, as shown below:

Screen.Recording.2024-09-26.at.23.44.40.mp4

When investigating the issue, I found another bug, in which we can't select the second admin as preferred exporter:

Screen.Recording.2024-09-26.at.23.36.23.mov

This bug happened because we don't use optional chaining in

QuickbooksOnline.updateQuickbooksOnlinePreferredExporter(policyID, {exporter: row.value}, {exporter: qboConfig?.export.exporter ?? ''});

And qboConfig?.export doesn't exist in the first sync yet.

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

For the original issue, we should update Backend to not reset the data to the original exporter.

For the other bug mentioned, we can add optional chaining to here

QuickbooksOnline.updateQuickbooksOnlinePreferredExporter(policyID, {exporter: row.value}, {exporter: qboConfig?.export?.exporter ?? ''});

What alternative solutions did you explore? (Optional)

@Beamanator
Copy link
Contributor

Oh dang, thanks for the proposals so far! Anyone know which PR the bug may have come from??

@Beamanator
Copy link
Contributor

Yaaa i was kinda thinking that too ^

@Beamanator
Copy link
Contributor

@shubham1206agra @dangrous @rushatgabhane @aldo-expensify can y'all look at this?

@aldo-expensify
Copy link
Contributor

Investigating...

@dangrous
Copy link
Contributor

I think this is probably due to https://github.com/Expensify/Expensify/issues/431392#issuecomment-2377490529 which I just noticed today - basically I think we're ending up trying to update the export object to {exporter: {exporter: [actual value]}} but not 100% sure

@aldo-expensify
Copy link
Contributor

The exporter is getting encoded and nested in a weird way:

image

This is a duplicate of https://github.com/Expensify/Expensify/issues/431392#issuecomment-2377490529.

@aldo-expensify
Copy link
Contributor

This seems to fix it: #49818

cc @dangrous

@mananjadhav
Copy link
Collaborator

@aldo-expensify This might fix the API call but when I did a fresh connection, I could see the JS error for qboConfig?.export.exporter as qboConfig.export was undefined. May be we'll need to fix both?

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Sep 26, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Hourly KSv2 labels Sep 26, 2024
@aldo-expensify
Copy link
Contributor

@aldo-expensify This might fix the API call but when I did a fresh connection, I could see the JS error for qboConfig?.export.exporter as qboConfig.export was undefined. May be we'll need to fix both?

That sounds unrelated. Can you reproduce it in production?

@aldo-expensify
Copy link
Contributor

About that second bug, I'm not sure what is proposed here is the "right" way to fix it: #49797 (comment)

If the request to update the exporter fails, the "current value" we revert to could be wrong. The real value may have just been delayed.

@mananjadhav
Copy link
Collaborator

Here's a recording, and it looks like reproducible on Prod too.

Screen.Recording.2024-09-27.at.2.46.03.AM.mov

@aldo-expensify
Copy link
Contributor

Here's a recording, and it looks like reproducible on Prod too.

nice, I think we should work on the right solution with more time, so I would avoid putting it together with the deploy blocker

@mananjadhav
Copy link
Collaborator

mananjadhav commented Sep 26, 2024

Is the API being called in your solution? If you see the recording the API wasn't called at all. Adding optional chaining works as mentioned in my proposal.

Screen.Recording.2024-09-27.at.2.47.37.AM.mov

On a side note, I can help you review the DeployBlocker PR.

@aldo-expensify
Copy link
Contributor

Adding optional chaining works as mentioned in my #49797 (comment).

I want to avoid adding optional chaining just to go around an issue that may require a better solution

@aldo-expensify
Copy link
Contributor

Is the API being called in your solution?

yes, you can see it in the video I attached to the PR

image

@mananjadhav
Copy link
Collaborator

Yeah I can see that. Thanks for clarifying.

@aldo-expensify aldo-expensify added Awaiting Payment Auto-added when associated PR is deployed to production and removed Reviewing Has a PR in review DeployBlockerCash This issue or pull request should block deployment labels Sep 27, 2024
@Beamanator
Copy link
Contributor

Thanks so much for taking care of this since i had to peace out early yesterday @aldo-expensify 🙏

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. Engineering Weekly KSv2
Projects
None yet
Development

No branches or pull requests

7 participants