[HOLD for payment 2024-06-11] [Xero][QBO] [Wave Collect] Add error message in case of sync failure

[lakchote]

Problem

Right now, if an error occurs during sync, we do not display it to the user.

Solution

Display an error to the user to indicate sync wasn't successful, to avoid confusion.

You should display as error text, Couldn't connect to {integrationName}.

Implement hasSynchronizationError() by doing so.

[SzymczakJ]

Hey! I’m Jakub Szymczak from Software Mansion, an expert agency, and I’d like to work on this issue!

[trjExpensify]

Interesting yeah, this was mocked up in QBO, did it not get implemented? CC: @hayata-suenaga

[SzymczakJ]

I don't see anything like this in codebase, so probably we forgot about this 🤷

[lakchote]

Side note

Thanks to @SzymczakJ for notifying me of this.

Actually I've created the issue for Xero, it was closed by mistake by the PR that fixed the disconnect button not working for Xero.

Maybe my issue's title was too vague: Handle errors in the connections
I should have written down: Handle sync errors for the accounting integrations
& also double checked the related issue.

[trjExpensify]

Ah cool, so we're just going to use this issue now?

[hayata-suenaga]

Interesting yeah, this was mocked up in QBO, did it not get implemented? CC: @hayata-suenaga

Yes seems like we missed it in this PR and this PR
Thank you Lucien for realizing this and handling this 🙇

[lakchote]

Ah cool, so we're just going to use this issue now?

I think yes to avoid confusion, what do you think? We could change the other's issue title/body to reflect it was to fix the disconnect option. Waiting on your answer on this, and I'll update if necessary.

Thank you Lucien for realizing this and handling this 🙇

To be fair, it's @yuwenmemon's message on Slack that got me the lead 😄

[trjExpensify]

I think yes to avoid confusion, what do you think? We could change the other's issue title/body to reflect it was to fix the disconnect option. Waiting on your answer on this, and I'll update if necessary.

Nah, all good. Let's use this one!

[SzymczakJ]

I've found bug connected to QBO sync:
when the user changes credentials on Quickbooks Online site(eg. he changes email or password), and then clicks on "sync now"
,then the sync is performed as if it was successful. There's no indication that sync failed but there's lot of integration data missing in onyx(so I guess it eventually failed but the backend didn't set the lastSync.isSuccessful = false value).

The expected behaviour(based on what's stated in QBO design doc): sync fails and value policy.connections.lastSync.isSuccessful is set to false

cc @lakchote

[SzymczakJ]

There's also a Xero bug: #42307 (comment)

[francoisl]

As far as I know, changing your QBO password should not invalidate your existing OAuth credentials, so syncing should still work. If some data is missing in Onyx, my guess would be that there's a different bug causing the sync to fail. If you have a rough timeframe and email address of the account you were using, we can check our logs to see what happened.

[arosiclair]

@lakchote @trjExpensify This issue might be a bit premature? We have the design doc for handling sync and export errors here which hasn't been reviewed/published yet.

[trjExpensify]

Oh hm, but surfacing errors when making the connection is in the QBO doc is scoped here for example: https://docs.google.com/document/d/1LLrOYRGEb_k_Z6dttcS99zlbCNNdpD1QlXAbFJYbR7g/edit#bookmark=id.9crbfxsoog1j

IIRC, the intention was to handle it here, and then adding manual export and surfacing auto-sync/export failures in the follow-up. CC: @zanyrenney to correct me if I'm wrong!

[arosiclair]

Oh hm, but surfacing errors when making the connection is in the QBO doc is scoped here for example: https://docs.google.com/document/d/1LLrOYRGEb_k_Z6dttcS99zlbCNNdpD1QlXAbFJYbR7g/edit#bookmark=id.9crbfxsoog1j

Yeah I noticed that but also noticed that it was not actually designed in the detailed section 😄 . From what I understand, it was a last minute thing to split error handling into its own doc.

Either way, the PR seems to be in the right direction, but we're not sending errorMessage or isCredentialsError info to the frontend yet (being handled in the errors doc), so it can't be fully implemented just yet.

[trjExpensify]

@zanyrenney @hayata-suenaga @aldo-expensify might be able to speak to what happened there some more, but there is this in the out of scope section of the QBO doc:

Export and Sync Errors
This design doc is focused on the workflows for setting up and configuring the accounting integration with QuickBooks Online. Whilst we do cover the initial connection error from invalid tokens (discussed here), the scope of this doc does not include the full suite of export and sync errors that can occur between Expensify and QuickBooks online. These are being tackled as part of a program here. The reason for this is that there are many errors and the scope of the doc is already large. We do cover export configuration settings and have made improvements to the design as it exists on OldDot, by blocking configurations for export that are incongruent with each other. This should help alleviate support requests that we have seen previously on Expensify Classic around missing data points in report types.

That lines up with looking at the doc for QBO Manual Export & Errors the scope doesn't outline error handling when connecting, but rather errors on export and when autoSync runs. So I do think we need to surface error(s) when connecting for XeroCon at a minimum.

FWIW, ideally we can get over the line on manual/export/autoSync errors as well if it's possible to complete the end-to-end on NewDot.

[arosiclair]

Gotcha that makes sense. So we can probably keep this PR with a generic error but make sure it's forward compatible with the other changes we're planning in the errors doc. I can review to make sure that happens.

[trjExpensify]

Perfect, let's do it!

[melvin-bot]

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

[hayata-suenaga]

@zanyrenney @hayata-suenaga @aldo-expensify might be able to speak to what happened there some more, but there is this in the out of scope section of the QBO doc:

I don't know how the decision was made as I joined the project in the later stage of the planning phase, but I agree that we should have handled the sync error handling in our QBO project.

@lakchote, just to make sure that we're on the same page, are you planning to handle the sync error on QBO and Xero or are you planning to handle Xero sync error only? 😄

[rlinoz]

I'm confused, I don't think I am needed here?

[trjExpensify]

Nah, Lucien reviewed the PR.

[lakchote]

@lakchote, just to make sure that we're on the same page, are you planning to handle the sync error on QBO and Xero or are you planning to handle Xero sync error only? 😄

It's been done by @SzymczakJ so it's dynamic and not tied to one specific integration (ex in the PR here).

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.78-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Add error message in case of integration sync failure #42307

If no regressions arise, payment will be issued on 2024-06-11. 🎊

For reference, here are some details about the assignees on this issue:

• @SzymczakJ does not require payment (Contractor)

[melvin-bot]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

[melvin-bot]

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

[hayata-suenaga]

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

Should we pay for @mananjadhav for the C+ review of the PR?

[melvin-bot]

@lakchote, @SzymczakJ 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[trjExpensify]

Nope, Manan was paid centrally!