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.34k stars 2.77k forks source link

[HOLD for payment 2024-06-20] [HOLD for payment 2024-06-18] [$250] [Wave Control - NetSuite] Have Accounting Connection Sync Spinner Timeout After 20 minutes #42356

Closed yuwenmemon closed 3 months ago

yuwenmemon commented 4 months ago

Part of the #Wave-Control NetSuite project

Main issue: https://github.com/Expensify/Expensify/issues/377671 Doc section: https://docs.google.com/document/d/1WubNv_VAv78IxG4FKsi9aS0pWESfWvUYbqBAAy5b4bc/edit#heading=h.tb9yro1dmj3r Project: https://github.com/orgs/Expensify/projects/130

Feature Description

One issue with the Pusher ProgressManager updates for syncs that come from the IS to NewDot is that they are sometimes unreliable. If the jobDone sync update is not received by the client, then the sync spinner will hang forever, even though the sync has finished:

https://github.com/Expensify/App/assets/4741899/1360d0e9-cab0-49b6-a302-24a88f477e03

To fix this, let's have the spinner timeout to stop 20 min after a sync is started and no jobDone sync update is received. We landed one 20 minutes because, in recent measurements, our longest NetSuite jobs take around 14 minutes.

Manual Test Steps

Initiate a QBO sync from your workspace that's connected to QBO
Go offline
Wait for 10 min
Go back online
Make sure that after 10 more minutes, the spinner has stopped.

Automated Tests

N/A

cc @aldo-expensify @francoisl @lakchote @marcaaron @tgolen

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01c42a63242268a2f0
Upwork Job ID: 1792660966817673216
Last Price Increase: 2024-05-29

Issue Owner

Current Issue Owner: @twisterdotcom

melvin-bot[bot] commented 4 months ago

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

twisterdotcom commented 4 months ago

I think this is New Feature, not Bug right?

melvin-bot[bot] commented 4 months ago

Current assignee @twisterdotcom is eligible for the NewFeature assigner, not assigning anyone new.

yuwenmemon commented 4 months ago

Yeah that seems like a fair characterization as well.

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

nkdengineer commented 4 months ago

Proposal

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

Spinner hasn't stopped even though the sync has finished

What is the root cause of that problem?

This is a new feature

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

In QuickBooksOnline, create a function like syncConnectionAfterTimeout that will update stageInProgress to jobDone

function syncConnectionAfterTimeout(policyID: string) {
    Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY_CONNECTION_SYNC_PROGRESS}${policyID}`,  {
        stageInProgress: CONST.POLICY.CONNECTIONS.SYNC_STAGE_NAME.JOB_DONE,
        connectionName: CONST.POLICY.CONNECTIONS.NAME.QBO,
    })
}

In PolicyAccountingPage, we will create a useEffect that will be triggered when connectionSyncProgress?.stageInProgress is changed and in this we will have a setTimeout 20 minutes to update connectionSyncProgress?.stageInProgress to jobDone so if a sync is started and no jobDone sync update is received

useEffect(() => {
    if (!connectionSyncProgress?.stageInProgress || connectionSyncProgress?.stageInProgress === CONST.POLICY.CONNECTIONS.SYNC_STAGE_NAME.JOB_DONE) {
        return;
    }
    spinnerTimeoutRef.current = setTimeout(() => {
        syncConnectionAfterTimeout(policy.id);
    }, 20*60*1000);

    return () => {
        if (!spinnerTimeoutRef.current) {
            return;
        }

        clearTimeout(spinnerTimeoutRef.current)
    }
}, [connectionSyncProgress?.stageInProgress])

What alternative solutions did you explore? (Optional)

We can create a state to control whenever we should show spinner and then update it in the setTimeout of point 2

aldo-expensify commented 4 months ago

@yuwenmemon for this, instead of adding a setTimeout in the front end, I think it can be more reliably implemented like:

The IS server should include a timestamp in the progress data (PolicyConnectionSyncProgress)
isSyncInProgress should ignore the progress data if it is older than 20 minutes

What do you think?

francoisl commented 4 months ago

Does that solve the issue with missing the final notification though? Let's say you close your laptop while a sync is running, and so you never effectively receive the jobDone notification. When you get back online, you still have the wrong stageInProgress right?

aldo-expensify commented 4 months ago

When you get back online, you still have the wrong stageInProgress right?

yes, it would be still in Onyx, but ignored because it is older than 20 minutes

yuwenmemon commented 4 months ago

@aldo-expensify Yeah good idea, that should make things a lot simpler and reliable. In this case we'll need to take this internal.

melvin-bot[bot] commented 4 months ago

Current assignee @ahmedGaber93 is eligible for the Internal assigner, not assigning anyone new.

francoisl commented 4 months ago

Ah ok I originally misunderstood your comment, I thought you wanted to ignore new progress data received from IS if the previous one was older than 20 minutes. Makes more sense now 👍

twisterdotcom commented 3 months ago

Removed the /app job and automation hopefully.

twisterdotcom commented 3 months ago

Ah crap, wait I see @ahmedGaber93 reviewed https://github.com/Expensify/App/pull/42498. Damn, thought that was an internal PR. Okay, will pay that out when it's due.