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] QBD - Auto-sync toggle does not work correctly #52132

Open
2 of 8 tasks
lanitochka17 opened this issue Nov 6, 2024 · 50 comments
Open
2 of 8 tasks

[$250] QBD - Auto-sync toggle does not work correctly #52132

lanitochka17 opened this issue Nov 6, 2024 · 50 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Overdue Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Nov 6, 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.58-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5184320
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause - Internal Team

Action Performed:

Precondition: QBD connection is established in the workspace.

  1. Navigate to Accounting
  2. Click on Advanced
  3. disable Auto-sync toggle
  4. Go back to Accounting and wait for the sync to finish return to Advanced

Expected Result:

Auto-sync toggle is disabled

Actual Result:

Auto-sync toggle returns to enabled after sync finishes

Workaround:

Unknown

Platforms:

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

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence
Bug6656487_1730891945304.Screen_Recording_2024-11-06_at_2.08.24_in_the_afternoon.1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021856102760605829790
  • Upwork Job ID: 1856102760605829790
  • Last Price Increase: 2024-11-11
  • Automatic offers:
    • suneox | Reviewer | 104920810
    • ikevin127 | Contributor | 104999271
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 6, 2024
Copy link

melvin-bot bot commented Nov 6, 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.

@melvin-bot melvin-bot bot added the Overdue label Nov 11, 2024
@johncschuster johncschuster added the External Added to denote the issue can be worked on by a contributor label Nov 11, 2024
@melvin-bot melvin-bot bot changed the title QBD - Auto-sync toggle does not work correctly [$250] QBD - Auto-sync toggle does not work correctly Nov 11, 2024
Copy link

melvin-bot bot commented Nov 11, 2024

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

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

melvin-bot bot commented Nov 11, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 11, 2024
@johncschuster
Copy link
Contributor

Triaged

@narefyev91
Copy link
Contributor

Hi, I'm Nicolay from Callstack - expert contributor group - and I would like to work on this issue.

Copy link

melvin-bot bot commented Nov 15, 2024

@johncschuster, @suneox Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Nov 15, 2024
@ikevin127
Copy link
Contributor

I was able to reproduce / the issue happens on QBO as well. I will take over the issue from @suneox as C+ reviewer, coming from this Slack thread.

Let's go with @narefyev91 from expert contributor group as they've shown interest in fixing the issue!

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Nov 15, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Nov 15, 2024
@aldo-expensify
Copy link
Contributor

Have we confirmed this a frontend issue?

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 15, 2024
Copy link

melvin-bot bot commented Nov 15, 2024

📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@ikevin127
Copy link
Contributor

ikevin127 commented Nov 15, 2024

Have we confirmed this a frontend issue?

@aldo-expensify Looked into it and here's what happens:

When we turn Auto-sync off UpdateQuickbooksDesktopAutoSync is called -> returns response:

JSON
{
    "jsonCode": 200,
    "requestID": "8e31e2695c68cf1b-SJC",
    "onyxData": [
        {
            "key": "policy_E12DF2A9245F1530",
            "onyxMethod": "merge",
            "value": {
                "connections": {
                    "quickbooksDesktop": {
                        "config": {
                            "autoSync": {
                                "enabled": false,
                                "jobID": null
                            }
                        }
                    }
                }
            }
        }
    ],
    "previousUpdateID": 2960739932,
    "lastUpdateID": 2960747654
}

After QBD / QBO syncs, FE calls GetMissingOnyxMessages automatically -> which returns the following response that re-enables Auto-sync:

JSON
{
    "onyxData": [
        {
            "key": "policy_E12DF2A9245F1530",
            "onyxMethod": "merge",
            "value": {
                "connections": {
                    "quickbooksDesktop": {
                        "config": {
                            "autoSync": {
                                "enabled": true
                            }
                        }
                    }
                }
            }
        },
        {
            "key": "policy_E12DF2A9245F1530",
            "onyxMethod": "merge",
            "value": {
                "connections": {
                    "quickbooksDesktop": {
                        "config": {
                            "autoSync": {
                                "jobID": "9086846025541357"
                            }
                        }
                    }
                }
            }
        },
        {
            "key": "userMetadata",
            "onyxMethod": "set",
            "value": {
                "accountID": 15752938,
                "email": "[email protected]",
                "freeTrial": false,
                "planType": "corporate",
                "role": "admin",
                "tryNewDotDismissed": false
            }
        }
    ],
    "lastUpdateID": "2960783676",
    "previousUpdateID": "2960782355",
    "jsonCode": 200,
    "requestID": "8e31e7992b2f2536-SJC"
}

Looks like the first API call does not seem to persist the autoSync on the BE side which might have something to do with the fact that jobID is null for some reason which might mean that we don't pass it from FE when we make the call. What do you think ?

♻️ More context

function updateQuickbooksDesktopAutoSync<TSettingValue extends Connections['quickbooksDesktop']['config']['autoSync']['enabled']>(policyID: string, settingValue: TSettingValue) {
const onyxData = buildOnyxDataForQuickbooksConfiguration(policyID, CONST.QUICKBOOKS_DESKTOP_CONFIG.AUTO_SYNC, {enabled: settingValue}, {enabled: !settingValue});
const parameters: UpdateQuickbooksDesktopGenericTypeParams = {
policyID,
settingValue: JSON.stringify(settingValue),
idempotencyKey: String(CONST.QUICKBOOKS_DESKTOP_CONFIG.AUTO_SYNC),
};
API.write(WRITE_COMMANDS.UPDATE_QUICKBOOKS_DESKTOP_AUTO_SYNC, parameters, onyxData);
}

When we call the function in:

onToggle: (isOn: boolean) => QuickbooksDesktop.updateQuickbooksDesktopAutoSync(policyID, isOn),

as 2nd param (settingValue) we pass isOn (boolean) so looks like we don't pass any jobID which might be the root cause because if we follow settingValue's type we can notice QBDConnectionConfig:

App/src/types/onyx/Policy.ts

Lines 1286 to 1298 in 992e5d4

type QBDConnectionConfig = OnyxCommon.OnyxValueWithOfflineFeedback<{
/** API provider */
apiProvider: string;
/** Configuration of automatic synchronization from QuickBooks Desktop to the app */
autoSync: {
/** Job ID of the synchronization */
jobID: string;
/** Whether changes made in QuickBooks Desktop should be reflected into the app automatically */
enabled: boolean;
};

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Nov 15, 2024

Looks like the first API call does not seem to persist the autoSync on the BE side which might have something to do with the fact that jobID is null for some reason which might mean that we don't pass it from FE when we make the call. What do you think ?

I don't think the frontend needs to send jobID: null, that shouldn't matter. There is some special code in the backend handling the job deleting and making that key null.

The steps described in the issue look incomplete

  1. Navigate to Accounting
  2. Click on Advanced
  3. disable Auto-sync toggle
  4. Go back to Accounting and wait for the sync to finish return to Advanced

I think first we should clear that up. Which case is it? What triggers the sync?

Case 1

  1. Navigate to Accounting
  2. Trigger the policy sync manually (NEW STEP!)
  3. Click on Advanced
  4. disable Auto-sync toggle
  5. Go back to Accounting and wait for the sync to finish return to Advanced

Case 2

  1. Navigate to Accounting
  2. Click on Advanced
  3. disable Auto-sync toggle
  4. Go back to Accounting
  5. Trigger the policy sync manually (NEW STEP!)
  6. Wait for the sync to finish return to Advanced

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2024
Copy link

melvin-bot bot commented Dec 6, 2024

@johncschuster, @narefyev91, @ikevin127, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@ikevin127
Copy link
Contributor

@aldo-expensify Just retested and the issue is still reproducible on both QBO and QBD, tested both on dev / staging API.

Once the Auto-sync is turned off and the connection sync is completed -> if we wait long enough GetMissingOnyxMessages call is triggered (not sure by what) the Auto-sync will be turned back on.

App/src/libs/actions/App.ts

Lines 326 to 330 in 5f3782b

* Fetches data when the client has discovered it missed some Onyx updates from the server
* @param [updateIDFrom] the ID of the Onyx update that we want to start fetching from
* @param [updateIDTo] the ID of the Onyx update that we want to fetch up to
*/
function getMissingOnyxUpdates(updateIDFrom = 0, updateIDTo: number | string = 0): Promise<void | OnyxTypes.Response> {

GetMissingOnyxMessages is called on FE side in two places related to OnyxUpdateManager:

There were instances where GetMissingOnyxMessages was triggered shortly after connection sync is completed, and I also had instances where I had to wait 4-5 minutes for it to trigger, but when it does, it always turns Auto-sync back on.

Here are the details you asked for from my side - UpdateQuickbooksOnlineAutoSync:

{
    policyID: 18920FC1A9030899,
    email: "[email protected]",

    requestID: "8edefd622fd89648-SJC",
    lastUpdateID: 3280895077,
    previousUpdateID: 3269474291,
}

then after the one above, GetMissingOnyxMessages was called with this data ~3 minutes later:

{
    requestID: "8edf0bccaec123a9-SJC",
    lastUpdateID: "3280898069",
    previousUpdateID: "3280895077",
}

which turned Auto-sync back on as a result.

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 6, 2024
@aldo-expensify
Copy link
Contributor

Thanks @ikevin127 , I'll investigate again later this week with the information you provided 🙇

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Dec 9, 2024
Copy link

melvin-bot bot commented Dec 13, 2024

@johncschuster, @narefyev91, @ikevin127, @aldo-expensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@johncschuster
Copy link
Contributor

How's this going, @aldo-expensify?

@melvin-bot melvin-bot bot removed the Overdue label Dec 13, 2024
@aldo-expensify
Copy link
Contributor

aldo-expensify commented Dec 13, 2024

Thanks for the ping.. investigating now:

I can confirm that the setting is being set to false in that request (disabling autoSync). I also see that since a setting was changed, we queue a job to run the policy integration sync. (Should we skip queueing a sync job if the setting that was change was autoSync=false?)

In those logs, I can see that the Integration Server is turning back on the autoSync setting. The only place that can happen for QBO I believe is here:

https://github.com/Expensify/Integration-Server/blob/c89f2fb0f140701543779a6069b309cf4f5af6b7/src/expensify/quickbooksonline/QuickbooksOnlineImporter.java#L217

So that narrows down the problem to figuring out why the Integration Server is considering that the policy has never been configured before.

@aldo-expensify
Copy link
Contributor

I'm trying to debug this further, but trying to connect a new QBO accounts redirects me to an invalid URL in dev, I don't know if there is a new bug or something else:

https://integrations.expensify.com.dev/Integration-Server/null?client_id=Q00XcQZaPUmMXcob
image

@ikevin127
Copy link
Contributor

@aldo-expensify This is a known issue, but you ca get around it by simply removing the .dev part from the link and proceed as usual.

@aldo-expensify
Copy link
Contributor

I'm pretty sure the issue is not the .dev, since I'm on purpose trying to work against the integration server running in my dev environment. The issue seems to be that /null which I'm not sure where is it coming from (the code has changed a lot since the last time I worked on this 🥲 ).

@aldo-expensify
Copy link
Contributor

There seems to be a problem with the Integration Server in my dev env, since the URL that ends up with /null is being constructed there

@ikevin127
Copy link
Contributor

I'm pretty sure the issue is not the .dev

Oh, yes - I confused it with what I'm doing when connecting QBO on DEV.

@melvin-bot melvin-bot bot added the Overdue label Dec 16, 2024
@aldo-expensify
Copy link
Contributor

I'm still trying to fix my dev environment to work on this, I'm asking here: https://expensify.slack.com/archives/C3BGN04K1/p1734356547490549

@melvin-bot melvin-bot bot removed the Overdue label Dec 16, 2024
@aldo-expensify
Copy link
Contributor

I was just able to fix my dev environment, so I'll be able to resume my investigation / debugging today

@aldo-expensify
Copy link
Contributor

@ikevin127 did you add the QBO connection from Old Dot? or New Dot? I didn't find in the logs a call from your account to ConnectPolicyToQuickbooksOnline so that makes me thing you connected in Old Dot and then switched to New Dot. Is that correct?

@ikevin127
Copy link
Contributor

No idea whether the very first time was from OD, but after that I disconnected and re-connected multiple times all from ND.

@aldo-expensify
Copy link
Contributor

No idea whether the very first time was from OD, but after that I disconnected and re-connected multiple times all from ND.

I'm referring to this specific time you tested: #52132 (comment) . If you connected to QBO that same day for testing, from looking at logs, I think it wasn't through New Dot.

@aldo-expensify
Copy link
Contributor

aldo-expensify commented Dec 16, 2024

So far the only possible way I see for this to happen is:

  1. User connects QBO in OldDOt
  2. OldDot shows the configuration modal but the user doesn't save (just close the modal or the tab). When this happens, a configuration is not saved and connections.quickbooksOnline.config.lastConfigurationTime is missing.
    image
  3. User goes to New Dot and changes a QBO setting
  4. The sync job runs because a setting was changed
  5. The IS finds a missing connections.quickbooksOnline.config.lastConfigurationTime, so it assumes you are just connecting QBO for the first time, so it sets all the default values plus autoSync: true.

This sounds unlike to happen in a real case, so lowering the priority. I'm not sure about what we should do about it.

@aldo-expensify aldo-expensify added Weekly KSv2 and removed Daily KSv2 labels Dec 16, 2024
@ikevin127
Copy link
Contributor

Makes sense, let me try this on a fresh account with connecting directly from ND to make sure I didn't go through OD flow initially, then and check whether the issue is still reproducible - will pass the details regarding policyID and all other from the fresh try.

@ikevin127
Copy link
Contributor

Did as mentioned in the comment above and the issue is not reproducible if the connection flow is done strictly on ND, here are the details from UpdateQuickbooksOnlineAutoSync:

{
    policyID: C6E8EC51B5025647,
    email: "[email protected]",
    requestID: "8f44e0e189a9cf1b-SJC",
    lastUpdateID: 3487737068,
    previousUpdateID: 3487736201,
}

Tried both with test emails (using +) and non-test and was not able to reproduce when connection was made from ND.

@melvin-bot melvin-bot bot added the Overdue label Dec 27, 2024
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. External Added to denote the issue can be worked on by a contributor Overdue Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

6 participants