Offline and errors pattern in Quickbooks Online integration #46511

zfurtak · 2024-07-30T15:51:39Z

Details

Fixed Issues

$#45406 - partially fixed
PROPOSAL:

Tests

Test errors:

Set Simulate failing network in Troubleshooting (you can set MAX_REQUEST_RETRIES to 0 to get errors faster).
Go to Settings -> Workspaces -> your QuickbooksOnline workspace -> Accounting.
Go through every page and try changing values.
See that there are RBRs and error messages when request fails.

Verify that no errors appear in the JS console

Offline tests

Go to Settings -> Workspaces -> your QuickbooksOnline workspace -> Accounting.
Go through every page and try changing values.
See that changed values are on opacity 50%.

QA Steps

Test errors:

Set Simulate failing network in Troubleshooting (you can set MAX_REQUEST_RETRIES to 0 to get errors faster).
Go to Settings -> Workspaces -> your QuickbooksOnline workspace -> Accounting.
Go through every page and try changing values.
See that there are RBRs and error messages when request fails.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

#### Offline https://github.com/user-attachments/assets/8bf46e3d-8f15-4a88-a2cf-592bd7ac8df2

Errors

android-error-1.mov

Android: mWeb Chrome

Offline

mweb-android-offline-1.mov

Errors

mweb-android-errors-1.mov

iOS: Native

Offline

ios-offline-1.mov

Errors

ios-error-1.mov

iOS: mWeb Safari

Offline

mweb-ios-offline-1.mov

Errors

mweb-ios-error-1.mov

MacOS: Chrome / Safari

Offline

web-offline-1.mov

Errors

web-error-1.mov

MacOS: Desktop

#### Offline https://github.com/user-attachments/assets/beeb1c13-53bc-4be2-8208-28f6dda6febb

Errors

desktop-error-1.mov

war-in

You've performed a titanic work here 🤯 Please address the comments and then we're ready for C+ review 🔥

src/pages/workspace/accounting/qbo/advanced/QuickbooksAccountSelectPage.tsx

src/pages/workspace/accounting/qbo/advanced/QuickbooksAdvancedPage.tsx

src/pages/workspace/accounting/qbo/advanced/QuickbooksInvoiceAccountSelectPage.tsx

src/pages/workspace/accounting/qbo/import/QuickbooksTaxesPage.tsx

src/pages/workspace/accounting/qbo/export/QuickbooksCompanyCardExpenseAccountPage.tsx

src/pages/workspace/accounting/qbo/import/QuickbooksChartOfAccountsPage.tsx

src/pages/workspace/accounting/PolicyAccountingPage.tsx

src/pages/workspace/accounting/qbo/export/QuickbooksPreferredExporterConfigurationPage.tsx

zfurtak · 2024-08-08T08:00:27Z

hi @kirillzyusko, since the new React Compiler check was introduced, the flow is failing for me with components that my PR hasn't touched at all.
Could you take a look? 😊
EDIT:
I found errors related to changed files:

Reason: Invalid nesting in program blocks or scopes
Reason: Unexpected terminal kind optional for ternary test block
Reason: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved...

kirillzyusko · 2024-08-08T08:38:10Z

Hey @zfurtak

Which errors QuickbooksOutOfPocketExpenseConfigurationPage.tsx file throws? If it's only React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved then it could because of the fact that you've changed useMemo code - maybe it happens because we duplicate dependency and re-use the same expression inside useMemo (maybe worth to create a separate variable and re-use it).

I believe if you run npm run react-compiler-healthcheck then it should print all files with all problematic code and line numbers. Can you go through all these errors and figure out which code causes problems and try to re-work it so that compiler doesn't complain anymore?

zfurtak · 2024-08-08T08:58:56Z

I believe if you run npm run react-compiler-healthcheck then it should print all files with all problematic code and line numbers. Can you go through all these errors and figure out which code causes problems and try to re-work it so that compiler doesn't complain anymore?

This is exactly what I did, here is the output I got:

...QuickbooksAdvancedPage.tsx Reason: Invalid nesting in program blocks or scopes
...QuickbooksExportConfigurationPage.tsx:97:111. Reason: Unexpected terminal kind optional for ternary test block
...QuickbooksExportDateSelectPage.tsx:36:8. Reason: React Compiler has skipped optimizing this component because the existing manual ...
...QuickbooksOutOfPocketExpenseConfigurationPage.tsx Reason: Invalid nesting in program blocks or scopes

EDIT:
@kirillzyusko
I managed to solve 2 of these issues, I am left with two files with the same error: Reason: Invalid nesting in program blocks or scopes
Here are links to React Compiler Playground which shows the errors (however they are different)
QuickbooksAdvancedPage.tsx
QuickbooksOutOfPocketExpenseConfigurationPage.tsx

At this point I don't know how to solve them 🧐

zfurtak · 2024-08-14T12:56:00Z

hi 👋
I'm still not sure what should I do with the error on the empty page from the first point 🤔
About the second point (offline):
So far I've changed the grey empty list component, but it was with opacity 50% because when we change the value of Export as, we also change the value of the account, sometimes it changes to an empty list and then we see the component. However now it has lower opacity when options exist and 100% on empty list. 😊

About the error messages, we were changing the flow of showing them, however we didn't update the messages and sticked with previous ones (same as on staging).

However I discovered a problem related to this part (video below). 🐛

The title should be Not configured but the value of reimbursableExpensesAccount.name (from QBO config) remains from the previous option, as we pass undefined to onyx. I tried to change it in the code to clear the value with null but the backend doesn't allow it. Could you consider this as a possible issue?

Screen.Recording.2024-08-14.at.14.42.36.mov

cc @ikevin127 @dubielzyk-expensify @shawnborton @trjExpensify

ikevin127

~~2. "Accounting > Export > Export company card expenses as" empty Bank account page is grayed-out after changing Export as while offline~~ | 🟢 FIXED

I'm still not sure what should I do with the error on the empty page from the first point 🤔

Which leaves the issue number 1 with the error placed at the bottom of the empty page:

I think given that the design team doesn't feel strongly about it (ref1, ref2) I think we can leave it like that.

The title should be Not configured but the value of reimbursableExpensesAccount.name (from QBO config) remains from the previous option, as we pass undefined to onyx. I tried to change it in the code to clear the value with null but the backend doesn't allow it. Could you consider this as a possible issue?

Not sure regarding this issue, sounds like we might want to handle it here but it might require BE changes so I'll defer this to assigned CME to decide whether to handle this here or in a new issue.

cc @deetergp on the sentence ^ above!

Otherwise, PR tests well, LGTM 🚀

deetergp · 2024-08-15T21:14:46Z

Can we elaborate on HOW the backend is preventing us from clearing this out? Which API command is returning the undefined — is it UpdateQuickbooksOnlineReimbursableExpensesAccount?

war-in · 2024-08-19T10:49:44Z

@deetergp We use the UpdateManyPolicyConnectionConfigurations command here and we want to set nonReimbursableExpensesAccount field to null (or some other value that tells us it's empty).

Now it's done this way

[CONST.QUICKBOOKS_CONFIG.NON_REIMBURSABLE_EXPENSES_ACCOUNT]: row.accounts[0],

but if the row.accounts is empty we get undefined and send nothing to the backend.

We tried to pass null when the row.accounts[0] is undefined but the backend returned an error:

That's why we think this issue is BE related but maybe there is a way of clearing the value. Any help would be appreciated 🙏

zfurtak · 2024-08-26T09:50:55Z

hi @deetergp, could you take a look at this issue? 😊

zfurtak · 2024-08-27T09:22:45Z

@deetergp
Can we get on with this issue? because it's been in progress for quite a long time so far 😊

deetergp · 2024-08-27T16:05:59Z

My apologies @zfurtak, I will investigate those BE changes today.

zfurtak · 2024-08-29T07:59:28Z

Hello @deetergp, how is it going? 😃

deetergp · 2024-08-29T19:43:35Z

@zfurtak @war-in Can you give me the policyID for the workspace you are testing with? I found the block where we are throwing it (link, for future me's benefit) but I want to see what's currently stored in the DB for that property. Thanks!

zfurtak · 2024-09-01T21:46:41Z

@deetergp sure thing!
Here's the ID of my policy that is connected to QBO: A99CBAAB0B1E1AB3

zfurtak · 2024-09-04T11:46:35Z

Hi @deetergp, at the moment the code that connects back and front is being changed, maybe this refactor will fix the problem at the same time. 🤔.
However this bug had existed before this refactor, so I think it could be good to leave it as it is for now and merge this PR as it's been waiting enough already.
What do you think? 😊
cc: @war-in

ZhenjaHorbach · 2024-09-04T15:08:12Z

src/libs/actions/connections/QuickbooksOnline.ts

    };
    API.write(WRITE_COMMANDS.UPDATE_QUICKBOOKS_ONLINE_REIMBURSABLE_EXPENSES_ACCOUNT, parameters, onyxData);
 }

-function updateQuickbooksOnlineSyncLocations(policyID: string, settingValue: IntegrationEntityMap) {
-    const onyxData = buildOnyxDataForQuickbooksConfiguration(policyID, CONST.QUICK_BOOKS_CONFIG.SYNC_LOCATIONS, settingValue);
+function updateQuickbooksOnlineSyncLocations(policyID: string, settingValue: IntegrationEntityMap, oldValue?: IntegrationEntityMap) {


I have a small proposal
Maybe you will like it 😅

I also started making a similar PR today by mistake
And how do you like this format of typing?

https://github.com/Expensify/App/pull/48525/files#diff-7b811795926d7001296df148e05cadd46ac9cf78bdb6418d2787d1172b8e237bR210-R214

We can be sure that the parameters passed will have correct types
Plus we can reuse TSettingValue for settingValue and oldValue

I like it! Should I do it right away here or do you prefer to merge your PR? 😊

I think yours is better
Since my PR is in draft and requires filling out checklists 😅
Plus it's better to close it since there is your PR

OSBotify · 2024-09-05T01:09:43Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2024-09-05T19:40:02Z

🚀 Deployed to staging by https://github.com/deetergp in version: 9.0.30-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

aldo-expensify · 2024-09-16T17:27:12Z

src/libs/actions/connections/QuickbooksOnline.ts

@@ -92,6 +90,7 @@ function buildOnyxDataForQuickbooksConfiguration<TSettingName extends keyof Conn
    policyID: string,
    settingName: TSettingName,
    settingValue: Partial<Connections['quickbooksOnline']['config'][TSettingName]>,
+    oldSettingValue?: Partial<Connections['quickbooksOnline']['config'][TSettingName]>,


Why did we make this value optional with ?? I think it is not optional and it looks like we are passing it in every call

It's because we pass values from QuickbooksOnline config that can be undefined
e.g. here

ohh, I see, thanks!

ikevin127 · 2024-11-05T00:19:38Z

src/pages/workspace/accounting/qbo/export/QuickbooksExportConfigurationPage.tsx

+            contentContainerStyle={styles.pb2}
+            titleStyle={styles.ph5}
+            connectionName={CONST.POLICY.CONNECTIONS.NAME.QBO}
+            onBackButtonPress={() => Navigation.goBack(ROUTES.POLICY_ACCOUNTING.getRoute(policyID))}


Looks like having a fallback to the goBack function here led to the following issue:

[HOLD for payment 2024-11-11] [$250] Distance - Receipt disappears when dismissing distance editor after receipt is generated #48630

which was fixed by removing the fallback, more details in the proposed solution.

zfurtak added 12 commits July 30, 2024 13:04

Initial refactor of import pages

263743b

Merge branch 'main' into @zfurtak/qbo-error-messages

362d521

Adjusted import pages and refactored advanced page

ab278cb

Further refactor

2f73caa

Export pages refactor

a98872e

Merge branch 'main' into @zfurtak/qbo-error-messages

cabe722

Add error/offline patterns and refactor advanced pages

4a2c570

Adding missing patterns

5de6c69

Merge branch 'main' into @zfurtak/qbo-error-messages

ee86a23

Revert max_request_retry value

93c42d1

Adjustments

def4201

Adjustments

0cff3d9

war-in suggested changes Aug 6, 2024

View reviewed changes

war-in reviewed Aug 7, 2024

View reviewed changes

src/pages/workspace/accounting/qbo/import/QuickbooksChartOfAccountsPage.tsx Outdated Show resolved Hide resolved

zfurtak added 5 commits August 7, 2024 12:34

Adjusted to review

c109a1d

Added needed conditions

e609478

Merge branch 'main' into @zfurtak/qbo-error-messages

0b181b3

Adjust error patterns

26a9448

Fix console errors

a5e667a

zfurtak requested a review from war-in August 7, 2024 13:06

war-in approved these changes Aug 7, 2024

View reviewed changes

src/pages/workspace/accounting/PolicyAccountingPage.tsx Outdated Show resolved Hide resolved

src/pages/workspace/accounting/qbo/export/QuickbooksPreferredExporterConfigurationPage.tsx Outdated Show resolved Hide resolved

zfurtak added 3 commits August 7, 2024 16:30

Adjusted to review

0ca45ed

Style fix

1600fe6

Merge branch 'main' into @zfurtak/qbo-error-messages

f95c601

Rerun the checks

21d34d0

zfurtak marked this pull request as ready for review August 8, 2024 12:49

zfurtak requested a review from a team as a code owner August 8, 2024 12:49

zfurtak added 3 commits August 14, 2024 13:27

Remove opacity from empty list

ce308ed

Merge branch 'main' into @zfurtak/qbo-error-messages

ee8a820

Remove opacity from empty list

650192a

zfurtak mentioned this pull request Aug 14, 2024

Refactor UpdatePolicyConnectionConfiguration to be 1:1:1 (Part 1) #46234

Merged

50 tasks

ikevin127 approved these changes Aug 14, 2024

View reviewed changes

melvin-bot bot requested a review from deetergp August 14, 2024 20:48

Merge branch 'main' into @zfurtak/qbo-error-messages

1449d8a

Merge branch 'main' into @zfurtak/qbo-error-messages

e5faeb4

Merge main

9664819

war-in mentioned this pull request Sep 4, 2024

Update UpdatePolicyConnectionConfiguration to be 1:1:1 Part-3 #47628

Merged

50 tasks

ZhenjaHorbach reviewed Sep 4, 2024

View reviewed changes

zfurtak added 2 commits September 4, 2024 18:40

Merge main

14875d2

Added generic types for oldSettingValue

5e556ff

deetergp approved these changes Sep 5, 2024

View reviewed changes

deetergp merged commit fd16fdd into Expensify:main Sep 5, 2024
16 checks passed

github-actions bot mentioned this pull request Sep 5, 2024

Deploy Checklist: New Expensify 2024-09-05 #48664

Closed

69 tasks

aldo-expensify reviewed Sep 16, 2024

View reviewed changes

ikevin127 reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offline and errors pattern in Quickbooks Online integration #46511

Offline and errors pattern in Quickbooks Online integration #46511

zfurtak commented Jul 30, 2024 •

edited

Loading

war-in left a comment

zfurtak commented Aug 8, 2024 •

edited

Loading

kirillzyusko commented Aug 8, 2024

zfurtak commented Aug 8, 2024 •

edited

Loading

zfurtak commented Aug 14, 2024 •

edited

Loading

ikevin127 left a comment •

edited

Loading

deetergp commented Aug 15, 2024

war-in commented Aug 19, 2024

zfurtak commented Aug 26, 2024

zfurtak commented Aug 27, 2024

deetergp commented Aug 27, 2024

zfurtak commented Aug 29, 2024

deetergp commented Aug 29, 2024

zfurtak commented Sep 1, 2024

zfurtak commented Sep 4, 2024

ZhenjaHorbach Sep 4, 2024 •

edited

Loading

zfurtak Sep 4, 2024

ZhenjaHorbach Sep 4, 2024

zfurtak Sep 4, 2024

OSBotify commented Sep 5, 2024

OSBotify commented Sep 5, 2024

aldo-expensify Sep 16, 2024

zfurtak Sep 16, 2024

aldo-expensify Sep 16, 2024

ikevin127 Nov 5, 2024

Offline and errors pattern in Quickbooks Online integration #46511

Offline and errors pattern in Quickbooks Online integration #46511

Conversation

zfurtak commented Jul 30, 2024 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Errors

Offline

Errors

Offline

Errors

Offline

Errors

Offline

Errors

Errors

war-in left a comment

Choose a reason for hiding this comment

zfurtak commented Aug 8, 2024 • edited Loading

kirillzyusko commented Aug 8, 2024

zfurtak commented Aug 8, 2024 • edited Loading

zfurtak commented Aug 14, 2024 • edited Loading

However I discovered a problem related to this part (video below). 🐛

ikevin127 left a comment • edited Loading

Choose a reason for hiding this comment

Otherwise, PR tests well, LGTM 🚀

deetergp commented Aug 15, 2024

war-in commented Aug 19, 2024

zfurtak commented Aug 26, 2024

zfurtak commented Aug 27, 2024

deetergp commented Aug 27, 2024

zfurtak commented Aug 29, 2024

deetergp commented Aug 29, 2024

zfurtak commented Sep 1, 2024

zfurtak commented Sep 4, 2024

ZhenjaHorbach Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

zfurtak Sep 4, 2024

Choose a reason for hiding this comment

ZhenjaHorbach Sep 4, 2024

Choose a reason for hiding this comment

zfurtak Sep 4, 2024

Choose a reason for hiding this comment

OSBotify commented Sep 5, 2024

OSBotify commented Sep 5, 2024

aldo-expensify Sep 16, 2024

Choose a reason for hiding this comment

zfurtak Sep 16, 2024

Choose a reason for hiding this comment

aldo-expensify Sep 16, 2024

Choose a reason for hiding this comment

ikevin127 Nov 5, 2024

Choose a reason for hiding this comment

zfurtak commented Jul 30, 2024 •

edited

Loading

zfurtak commented Aug 8, 2024 •

edited

Loading

zfurtak commented Aug 8, 2024 •

edited

Loading

zfurtak commented Aug 14, 2024 •

edited

Loading

ikevin127 left a comment •

edited

Loading

ZhenjaHorbach Sep 4, 2024 •

edited

Loading