search

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.03k stars 2.54k forks source link

[Xero][QBO] [Wave Collect] Add error message in case of sync failure #42250

Open lakchote opened 2 weeks ago

lakchote commented 2 weeks ago

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 commented 2 weeks ago

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

trjExpensify commented 2 weeks ago

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

image
SzymczakJ commented 2 weeks ago

I don't see anything like this in codebase, so probably we forgot about this šŸ¤·

lakchote commented 2 weeks ago

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 commented 2 weeks ago

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

hayata-suenaga commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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" image ,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 commented 1 week ago

There's also a Xero bug: https://github.com/Expensify/App/pull/42307#issuecomment-2120669220

francoisl commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

Perfect, let's do it!

melvin-bot[bot] commented 4 days ago

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

hayata-suenaga commented 4 days ago

@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 commented 4 days ago

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

trjExpensify commented 4 days ago

Nah, Lucien reviewed the PR.

lakchote commented 3 days ago

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