Authenticate with QBO by hayata-suenaga · Pull Request #39219 · Expensify/App

hayata-suenaga · 2024-03-28T23:45:15Z

Details

This PR is part of the QBO project where we supports integration with QuickBooks Online, an accounting software, on New Expensify.

This PR adds a button that the user can click to start the authentication flow with the QBO. On both web and native platforms, this button opens up a new web page where the user can authenticate with the QBO. After the authentication is complete, the user's authentication token is stored in the backend and the web page closes.

On a web browser, the web page is opened on a separate browser window. On iOS and Android, a temporary browser window is opened as a modal within the app to display the web page.

Fixed Issues

$ #39094
PROPOSAL: N/A

Tests

Important Notes for testing: When testing the QBO authentication flow:

Make sure you are not logged in already with another account in OldDot, otherwise it will use the incorrect authToken in some parts causing failure
If you are an Expensify employee, you will have to disable WARP

Create a NewDot account
Create a Workspace
Go to the Workspace's settings page
Go to the "More features" and enable "Accounting" at the bottom
Go to the new "Accounting" menu entry
Click "Set up"
Sign in with a QBO account
The authentication flows opens a new tab. Confirm that after finishing the authentication the tab automatically closes.
Verify that you see an loading spinner while the import is happening
Verify that after the import is done, some new menu entries appear:
Click in the vertical 3 dots and choose "Disconnect"
Click "Disconnect" in the confirmation modal
Verify that there is only the initial menu entry with the "Set up" button

Test on iOS or Android

Create a NewDot account
Create a Workspace
Go to the Workspace's settings page
Go to the "More features" and enable "Accounting" at the bottom
Go to the new "Accounting" menu entry
Click "Set up"
Sign in with a QBO account
The authentication flows opens in a web view inside the App. Confirm that after finishing the authentication the web view automatically closes.
Verify that you see an loading spinner while the import is happening
Verify that after the import is done, some new menu entries appear:
Click in the vertical 3 dots and choose "Disconnect"
Click "Disconnect" in the confirmation modal
Verify that there is only the initial menu entry with the "Set up" button

QA Steps

No QA. The new button is not used yet. There is no way for the QA team to test this.

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

Screen.Recording.2024-04-16.at.5.58.01.PM.mov

MacOS: Chrome / Safari

Screen.Recording.2024-04-10.at.6.13.30.PM.mov

MacOS: Desktop

aldo-expensify · 2024-04-02T19:36:27Z

@@ -0,0 +1,7 @@
+type WorkspaceIntegrationImportStatus = {
+    status: 'starting' | 'finished' | 'progress';


I think you can remove the status and just use stageInProgress

github-actions · 2024-04-02T19:37:50Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/39219/index.html	https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/39219/index.html

Desktop 💻	Web 🕸️
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/39219/NewExpensify.dmg	https://39219.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

…ate-with-qbo

aldo-expensify · 2024-04-05T20:11:46Z

@hayata-suenaga I see that the PolicyAccountingPage is not even implemented yet, I don't think it makes a lot of sense to work on this yet. Is there a PR implementing PolicyAccountingPage that is on the work? if not I think we should start with that.

hayata-suenaga · 2024-04-08T16:35:21Z

@aldo-expensify, this is the PR that is implementing the PolicyAccountingPage. That PR will be merged without the code to actually trigger the authentication flow.

After we finish this PP to open the authentication flow, we will create another PR to add to trigger the authentication flow when the button is clicked.

aldo-expensify · 2024-04-08T19:09:00Z

@aldo-expensify, this is the PR that is implementing the PolicyAccountingPage.

@hayata-suenaga Did you forget to add the link? :P

hayata-suenaga · 2024-04-08T23:52:34Z

oops 😓 🙇 here is the link 😄

hayata-suenaga · 2024-04-09T16:15:16Z

@aldo-expensify, just a heads up. Some codes related to the QBO authentication flow were added in this PR that implements the WorksapceAccountingPage.

ex. getQuickBooksOnlineSetupLink and type PolicyConnectionSyncStage

aldo-expensify · 2024-04-17T17:49:59Z

Added a constant for the command ConnectPolicyToQuickbooksOnline and its corresponding ConnectPolicyToQuickbooksOnlineParams types

aldo-expensify · 2024-04-17T17:50:52Z

Created a follow up issue #40365 for the Desktop bug

marcaaron

Changes LGTM. Did not have any hard blockers, but left some questions and small improvements. Noted that there are mistakes left in the code, but sounds like @aldo-expensify has plans to follow up and address them.

marcaaron · 2024-04-17T17:45:24Z

+                                Cookie: `authToken=${session?.authToken}`,
+                            },
+                        }}
+                        incognito // 'incognito' prop required for Android, issue here https://github.com/react-native-webview/react-native-webview/issues/1352


NAB, but this comment suggests we should have a platform implementation where the incognito prop is only passed for Android if we don't need it on other platforms.

I see what you are saying, but to me the code duplication of adding the index.android.ts is worse. This prop doesn't affect in a negative way for IOS.

Could we maybe remove the comment about Android?

One downside of leaving platform specific logic in the code is that the many contributors who work on this code will then think it's OK to bend one of our rules.

marcaaron · 2024-04-17T17:51:56Z

    [WRITE_COMMANDS.SET_POLICY_DISTANCE_RATES_UNIT]: Parameters.SetPolicyDistanceRatesUnitParams;
    [WRITE_COMMANDS.SET_POLICY_DISTANCE_RATES_DEFAULT_CATEGORY]: Parameters.SetPolicyDistanceRatesDefaultCategoryParams;
+
    // eslint-disable-next-line @typescript-eslint/no-explicit-any


Doesn't look like this was added in this PR but looks related to this project. Why do we need to have any here?

Ahh this was a problem cause by the generic of the param type here #39219 (comment). We are "forced" to choose something here and any seemed to work good enough.

An alternative would be to define different constants for each combination of <connectionName, settingName>, but that sounds worse I think.

marcaaron · 2024-04-17T17:55:27Z


    if (shouldUseStagingServer) {
-        if (CONFIG.IS_USING_WEB_PROXY) {
+        if (CONFIG.IS_USING_WEB_PROXY && !request?.shouldSkipWebProxy) {


suggestion: since we mainly use the proxy to avoid CORS errors and (I presume) there is no problem with CORS errors when using a WebView this could be updated to something like request.isForWebView. That way we are not exposing the underlying "proxy" or "no proxy" to other areas of the code that don't really need to know about it.

and a question - Is it a problem to use the proxy for this? 🤔 If so, can we please add a comment here about why the proxy does not work for webviews?

since we mainly use the proxy to avoid CORS errors and (I presume) there is no problem with CORS errors when using a WebView this could be updated to something like request.isForWebView.

This problem doesn't only apply to WebViews, it is also there in "web" when the URL opens in a new tab. The URL created when we use the proxy is /CommandName?xxx, it misses the domain, so then the function Link.openLink misses that it needs to get a short lived auth token for it.

isForWebViewOrNewTab 🙃

or maybe... requiresShortLivedAuthToken 😬

yeah, I like that better.

marcaaron · 2024-04-17T17:58:54Z

+        // {
+        //     onyxMethod: Onyx.METHOD.MERGE,
+        //     key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
+        //     value: {
+        //         errorFields: {
+        //             avatar: ErrorUtils.getMicroSecondOnyxError('avatarWithImagePicker.deleteWorkspaceError'),
+        //         },
+        //     },
+        // },
+    ];


Is it going to cause conflicts to delete this? 🤔

marcaaron · 2024-04-17T18:00:16Z

+    };
+    API.write(WRITE_COMMANDS.REMOVE_POLICY_CONNECTION, parameters, {optimisticData, failureData});
+}
+function updatePolicyConnectionConfig<TConnectionName extends ConnectionName, TSettingName extends keyof Connections[TConnectionName]['config']>(


Did we do too much TypeScript here? 😄

haha, I though it could cause problems when using the function updatePolicyConnectionConfig, but it didn't and works pretty well on matching the settingName with the possible settingValues for a specific connection name (find some usages of updatePolicyConnectionConfig).

marcaaron · 2024-04-17T18:03:14Z

+                            [settingName]: settingValue,
+                            pendingFields: {
+                                [settingName]: null,
+                            },
+                            errorFields: {
+                                [settingName]: null,
+                            },


Co-authored-by: Marc Glasser <marc@expensify.com>

…ate-with-qbo

aldo-expensify · 2024-04-17T19:42:03Z

Merged main and resolved conflicts.

OSBotify · 2024-04-17T19:58:36Z

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

aldo-expensify · 2024-04-17T20:00:05Z

Created another follow up issue for the flashing "Set up" button:

#40377

OSBotify · 2024-04-18T04:55:35Z

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.4.63-0 🚀

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

lakchote · 2024-04-19T13:08:00Z

@hayata-suenaga @marcaaron @s77rt I think this is ready for another review.

I think we can deal with the WebView crash in another PR to avoid holding this one. The migration to RN 0.74 is on the works here: #37374 and that will most likely solve the issue.

We bumped our head too with Xero on this one... Glad you've posted about it here!

aldo-expensify · 2024-04-19T17:17:13Z

@lakchote there are two issues that may impact your work:

WebView IOS crash: [HOLD for payment 2024-05-09] [Wave Collect][QBO] iOS - Accounting - App crashes after completing QBO setup flow #40517
The "Set up" flashes briefly after the connection finishes the first import: "Set up" button flashes after QBO authentication finishes #40377

OSBotify · 2024-04-22T14:56:34Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.63-21 🚀

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

Hayata Suenaga added 4 commits March 28, 2024 14:41

feat: add a function to get the URL to start the QBO authentication flow

eb07cd3

chore: add new text

ea46f0e

fix: the command name

364e45a

feat: implement the button to start QBO auth flow

db079f8

hayata-suenaga self-assigned this Mar 28, 2024

hayata-suenaga added the Ready To Build label Mar 29, 2024

This comment has been minimized.

Sign in to view

Hayata Suenaga added 2 commits April 2, 2024 12:07

chore: add ts type for integration import status

5b0adbc

Merge branch 'main' into hayata-authenticate-with-qbo

d579eee

aldo-expensify reviewed Apr 2, 2024

View reviewed changes

aldo-expensify self-assigned this Apr 4, 2024

aldo-expensify added 2 commits April 4, 2024 16:12

Merge branch 'main' of github.com:Expensify/App into hayata-authentic…

60ef11f

…ate-with-qbo

Merge branch 'main' of github.com:Expensify/App into hayata-authentic…

58b771d

…ate-with-qbo

Update type name

6a6a6fa

hayata-suenaga mentioned this pull request Apr 9, 2024

Feat: implement accounting page #39652

Merged

50 tasks

aldo-expensify added 8 commits April 9, 2024 20:51

Merge main / resolve conflicts

120270c

Rename Workspace to Policy

89ed46c

Update QBO Setup button

2d8185e

WIP: Connect accounting page to sync progress / update types

3c5896d

Refactor connection actions. Add remove connection code.

78bd974

Fix remove connection

613bde4

Style

9b7bb84

Finish Authentication flow

1cee264

aldo-expensify marked this pull request as ready for review April 11, 2024 01:07

aldo-expensify requested a review from mountiny April 17, 2024 17:19

aldo-expensify mentioned this pull request Apr 17, 2024

[HOLD for payment 2024-05-09] [Desktop] Create a page to show when we fail to close the window during the QBO Authentication flow #40365

Closed

6 tasks

Prettier

11ebabe

marcaaron requested review from a team and removed request for a team April 17, 2024 17:47

Add ConnectPolicyToQuickbooksOnlineParams

db78384

marcaaron previously approved these changes Apr 17, 2024

View reviewed changes

Use const

6463530

Co-authored-by: Marc Glasser <marc@expensify.com>

aldo-expensify dismissed marcaaron’s stale review via 6463530 April 17, 2024 18:30

aldo-expensify and others added 3 commits April 17, 2024 11:31

Remove comment

d1f6212

Co-authored-by: Marc Glasser <marc@expensify.com>

style

9dab752

Merge branch 'main' of github.com:Expensify/App into hayata-authentic…

bdf2d18

…ate-with-qbo

marcaaron self-requested a review April 17, 2024 19:50

marcaaron approved these changes Apr 17, 2024

View reviewed changes

marcaaron merged commit 51bebd5 into main Apr 17, 2024

marcaaron deleted the hayata-authenticate-with-qbo branch April 17, 2024 19:51

aldo-expensify mentioned this pull request Apr 17, 2024

"Set up" button flashes after QBO authentication finishes #40377

Closed

6 tasks

aldo-expensify mentioned this pull request Apr 17, 2024

[NO QA] Address comments on QBO Authentication implementation #40381

Closed

50 tasks

github-actions Bot mentioned this pull request Apr 18, 2024

Deploy Checklist: New Expensify 2024-04-18 #40392

Closed

hayata-suenaga mentioned this pull request May 16, 2024

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

Closed

thesahindia mentioned this pull request Jun 7, 2024

[HOLD for payment 2024-06-13] [$250] QBO - Default vendor toggle switch activation is delayed #42534

Closed

6 tasks

		@@ -0,0 +1,7 @@
		type WorkspaceIntegrationImportStatus = {
		status: 'starting' \| 'finished' \| 'progress';

Uh oh!

Conversation

hayata-suenaga commented Mar 28, 2024 • edited by aldo-expensify Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 2, 2024

Uh oh!

aldo-expensify commented Apr 5, 2024

Uh oh!

hayata-suenaga commented Apr 8, 2024

Uh oh!

aldo-expensify commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hayata-suenaga commented Apr 8, 2024

Uh oh!

hayata-suenaga commented Apr 9, 2024

Uh oh!

aldo-expensify commented Apr 17, 2024

Uh oh!

aldo-expensify commented Apr 17, 2024

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aldo-expensify commented Apr 17, 2024

Uh oh!

OSBotify commented Apr 17, 2024

Uh oh!

aldo-expensify commented Apr 17, 2024

Uh oh!

OSBotify commented Apr 18, 2024

Uh oh!

lakchote commented Apr 19, 2024

Uh oh!

aldo-expensify commented Apr 19, 2024

Uh oh!

OSBotify commented Apr 22, 2024

hayata-suenaga commented Mar 28, 2024 •

edited by aldo-expensify

Loading

aldo-expensify commented Apr 8, 2024 •

edited

Loading

aldo-expensify Apr 17, 2024 •

edited

Loading