[$250] QBD - QBD is displayed like an established connection without error after dismissing the RHP

[vincdargento]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 9.0.71-1 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5297104 Email or phone of affected tester (no customers): N/A Issue reported by: Applause Internal Team

Action Performed:

1. Go to ND and log in with a new expensifail account
2. Create a new collect workspace
3. Go to "More Features" in workspace settings
4. Enable the Accounting toggle
5. Navigate to Accounting
6. Click on the connect button next to QBD
7. Upgrade your workspace to Control plan
8. Dismiss the RHP before the setup link appears
9. Navigate to any other page in workspace settings
10. Go back to Accounting

Expected Result:

QuickBooks Desktop should not be displayed as if a successful connection was made due to the RHP being dismissed even before the setup link appears. The behavior should be similar to the other integrations.

Actual Result:

QBD is displayed like an established connection without error after dismissing the RHP before the setup link appears.

Workaround:

Unknown

Platforms:

• [ ] Android: Standalone
• [ ] Android: HybridApp
• [x] Android: mWeb Chrome
• [ ] iOS: Standalone
• [ ] iOS: HybridApp
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/1e07d67c-0a1b-467f-901f-f1582d49770b

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021865081766703229432
• Upwork Job ID: 1865081766703229432
• Last Price Increase: 2024-12-06

Issue Owner

Current Issue Owner: @getusha

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[ikevin127]

Proposal

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

QBD is displayed like an established connection without error after dismissing the RHP before the setup link appears.

What is the root cause of that problem?

https://github.com/Expensify/App/blob/b595f5d24141a403fcc3f44c73d00cd6951dca7d/src/pages/workspace/accounting/qbd/QuickBooksDesktopSetupPage.tsx#L62-L69

The code block above calls ConnectPolicyToQuickbooksDesktop when the RHP modal mounts, the modal starting with isLoading state variable as true which leads to the QBD connection to be established even when the RHP modal is closed while still loading because the call is initialized on mount.

The issue is that this component was designed such that the setup link appears only when the loading state becomes false and hasResultOfFetchingSetupLink becomes true as a result of the ConnectPolicyToQuickbooksDesktop call being completed, therefore the appearance of the setup link depends on the loading state which depends on the call being made.

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

Given that the setup link being shown depends on the loading state becoming false and hasResultOfFetchingSetupLink being true, which depends on the ConnectPolicyToQuickbooksDesktop call being completed, and the fact that the Expected result requires that the call should only be made once the setup link is visible, this is my suggested approach:

• remove the isLoading state variable completely from the RequireQuickBooksDesktopModal component
• remove the shouldShowLoading variable as well as the FullScreenLoadingIndicator which comes with it

this way the setup link page content is visible right away except for the copy to clipboard link where we will show a small ActivityIndicator until ConnectPolicyToQuickbooksDesktop call is completed - this way there's no issue anymore since we eliminated the full screen loading loading indicator which led to this issue.

This would pose another issue when it comes to the setup link content page because the link only becomes available once ConnectPolicyToQuickbooksDesktop call is completed and hasResultOfFetchingSetupLink becomes true.

For this we have to conditionally show the CopyTextToClipboard component only when hasResultOfFetchingSetupLink is true, if it's false we display a small ActivityIndicator:

{!hasResultOfFetchingSetupLink ? (
    <ActivityIndicator
        color={theme.spinner}
        size="small"
    />
) : (
    <CopyTextToClipboard
        text={codatSetupLink}
        textStyles={[styles.textSupporting]}
    />
)}

Here's the complete DIFF for clarity

diff --git a/src/pages/workspace/accounting/qbd/QuickBooksDesktopSetupPage.tsx b/src/pages/workspace/accounting/qbd/QuickBooksDesktopSetupPage.tsx
index 5e90c3db4ed..02a0744e356 100644
--- a/src/pages/workspace/accounting/qbd/QuickBooksDesktopSetupPage.tsx
+++ b/src/pages/workspace/accounting/qbd/QuickBooksDesktopSetupPage.tsx
@@ -1,11 +1,11 @@
+import {useTheme} from '@react-navigation/native';
 import React, {useCallback, useEffect, useState} from 'react';
-import {View} from 'react-native';
+import {ActivityIndicator, View} from 'react-native';
 import Computer from '@assets/images/laptop-with-second-screen-sync.svg';
 import FullPageOfflineBlockingView from '@components/BlockingViews/FullPageOfflineBlockingView';
 import Button from '@components/Button';
 import CopyTextToClipboard from '@components/CopyTextToClipboard';
 import FixedFooter from '@components/FixedFooter';
-import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
 import HeaderWithBackButton from '@components/HeaderWithBackButton';
 import Icon from '@components/Icon';
 import * as Illustrations from '@components/Icon/Illustrations';
@@ -31,10 +31,10 @@ type RequireQuickBooksDesktopModalProps = PlatformStackScreenProps<SettingsNavig
 
 function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalProps) {
     const {translate} = useLocalize();
+    const theme = useTheme();
     const styles = useThemeStyles();
     const {environmentURL} = useEnvironment();
     const policyID: string = route.params.policyID;
-    const [isLoading, setIsLoading] = useState(true);
     const [hasError, setHasError] = useState(false);
     const [codatSetupLink, setCodatSetupLink] = useState<string>('');
     const hasResultOfFetchingSetupLink = !!codatSetupLink || hasError;
@@ -42,20 +42,19 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro
     const ContentWrapper = hasResultOfFetchingSetupLink ? ({children}: React.PropsWithChildren) => children : FullPageOfflineBlockingView;
 
     const fetchSetupLink = useCallback(() => {
-        setIsLoading(true);
         setHasError(false);
         // eslint-disable-next-line rulesdir/no-thenable-actions-in-views
         QuickbooksDesktop.getQuickbooksDesktopCodatSetupLink(policyID).then((response) => {
-            if (response?.jsonCode) {
-                if (response.jsonCode === CONST.JSON_CODE.SUCCESS) {
-                    setCodatSetupLink(String(response?.setupUrl ?? ''));
-                } else {
-                    setConnectionError(policyID, CONST.POLICY.CONNECTIONS.NAME.QBD, translate('workspace.qbd.setupPage.setupErrorTitle'));
-                    setHasError(true);
-                }
+            if (!response?.jsonCode) {
+                return;
             }
 
-            setIsLoading(false);
+            if (response.jsonCode === CONST.JSON_CODE.SUCCESS) {
+                setCodatSetupLink(String(response?.setupUrl ?? ''));
+            } else {
+                setConnectionError(policyID, CONST.POLICY.CONNECTIONS.NAME.QBD, translate('workspace.qbd.setupPage.setupErrorTitle'));
+                setHasError(true);
+            }
         });
     }, [policyID, translate]);
 
@@ -77,8 +76,7 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro
         },
     });
 
-    const shouldShowLoading = isLoading || !hasResultOfFetchingSetupLink;
-    const shouldShowError = !shouldShowLoading && hasError;
+    const shouldShowError = hasError;
 
     return (
         <ScreenWrapper
@@ -92,7 +90,6 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro
                 onBackButtonPress={() => Navigation.dismissModal()}
             />
             <ContentWrapper>
-                {shouldShowLoading && <FullScreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />}
                 {shouldShowError && (
                     <View style={[styles.flex1, styles.justifyContentCenter, styles.alignItemsCenter, styles.ph5, styles.mb9]}>
                         <Icon
@@ -113,7 +110,7 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro
                         </Text>
                     </View>
                 )}
-                {!shouldShowLoading && !shouldShowError && (
+                {!shouldShowError && (
                     <View style={[styles.flex1, styles.ph5]}>
                         <View style={[styles.alignSelfCenter, styles.computerIllustrationContainer, styles.pv6]}>
                             <ImageSVG src={Computer} />
@@ -122,10 +119,17 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro
                         <Text style={[styles.textHeadlineH1, styles.pt5]}>{translate('workspace.qbd.setupPage.title')}</Text>
                         <Text style={[styles.textSupporting, styles.textNormal, styles.pt4]}>{translate('workspace.qbd.setupPage.body')}</Text>
                         <View style={[styles.qbdSetupLinkBox, styles.mt5]}>
-                            <CopyTextToClipboard
-                                text={codatSetupLink}
-                                textStyles={[styles.textSupporting]}
-                            />
+                            {!hasResultOfFetchingSetupLink ? (
+                                <ActivityIndicator
+                                    color={theme.spinner}
+                                    size="small"
+                                />
+                            ) : (
+                                <CopyTextToClipboard
+                                    text={codatSetupLink}
+                                    textStyles={[styles.textSupporting]}
+                                />
+                            )}
                         </View>
                         <FixedFooter style={[styles.mtAuto, styles.ph0]}>
                             <Button

This is how the UI would look with this solution:

MacOS: Web

https://github.com/user-attachments/assets/2ffc6cda-a783-4378-903b-65ff6c25ac48

cc @Expensify/design for their take on this approach UI / UX wise

[shawnborton]

That looks pretty good to me. cc @kadiealexander as you worked on QBD too.

[dannymcclain]

Yeah I think it looks good too.

[melvin-bot[bot]]

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

[abekkala]

@kadiealexander do you have any comments irt this one before proceeding?

[abekkala]

@dannymcclain @shawnborton I think we're good to proceed with the proposal from @ikevin127

@getusha do you want to review the proposal as well

[getusha]

Yes, reviewing.

[getusha]

@ikevin127 @abekkala I see the same behavior after applying the proposal.

QBD is displayed like an established connection without error after dismissing the RHP before the setup link appears.

https://github.com/user-attachments/assets/666eb218-6db6-4cf8-ba31-692a43e5cc4b

And does the link being displayed mean the connection was established successfully? don't we expect the user to complete the setup before determining that?

[ikevin127]

@getusha Te way QBD logic was designed is that the call is made as soon as the modal mounts and when this call is completed the connection will appear in Accounting. The issue states that the current implementation should not trigger the call if the RHP is closed while in the loading state, but that's impossible given that the current logic performs the call on mount and that loading state is basically linked to that API call.

I proposed not showing the loading indicator as a full page spinner initially when the RHP opens, but instead showing the setup link UI right away, and showing a small ActivityIndicator while the copy to clipboard link is being fetched by the API call which is still triggered on mount.

To clarify: I'm not changing the fact that the API call is triggered on RHP mount, but instead changing where and how we show the loading indicator such that it doesn't give the impression that the call still went through and the connection was made when the user closed the RHP while loading, making it more clear for the user that as soon as the setup link UI is shown (RHP is mounted), the connection was already made and is pending QBD setup via the link (once fetched) - and we're just waiting for the copy to clipboard link to appear when the API call is completed.

This was the only way that I could think of that would solve the issue in a way, except for the The behavior should be similar to the other integrations. part which is not possible in our case because QBD connection works differently than QBO and the other ones which are web based connections compared to QBD which requires a desktop app to be installed.

If I'm wrong in my assumptions and this is not a desirable solution, then the only solution left would be to reredesign the flow completely, starting with the BE.

[getusha]

Thanks for clarifying, @ikevin127. However, I assume we don't want to show the connection as established before displaying the link either way. Pulling in an internal engineer since I'm not sure if we'll actually be fixing the issue by changing where we display the loading indicator. 🎀 👀 🎀

[melvin-bot[bot]]

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

[francoisl]

Sounds good, I like the proposed solution.

When we generate the setup link in the backend, we have to store some details about it in the policy, so if you switch between different sections like in @getusha's video and one of them makes an API call to get updated policy details, eventually, one of them is going to include details that make it look like the policy is being connected to QBD. I don't think there's a straightforward way to avoid that and it sounds like a rare edge case, so let's go with the current proposal that addresses the main issue in the OP.

[melvin-bot[bot]]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[ikevin127]

PR https://github.com/Expensify/App/pull/54087 is ready for review!

[ikevin127]

♻️ Status update: PR has been open and ready for review for 4 days - awaiting review from @getusha.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

• https://github.com/Expensify/App/pull/54087

If no regressions arise, payment will be issued on 2024-12-30. :confetti_ball:

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

• @ikevin127 requires payment automatic offer (Contributor)
• @getusha requires payment through NewDot Manual Requests

[melvin-bot[bot]]

@ikevin127 / @getusha @abekkala @ikevin127 / @getusha The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[ikevin127]

BugZero Checklist:

• [x] [Contributor] Classify the bug:

Bug classification

Source of bug:

• [x] 1a. Result of the original design (eg. a case wasn't considered)
• [ ] 1b. Mistake during implementation
• [ ] 1c. Backend bug
• [ ] 1z. Other:

Where bug was reported:

• [x] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• [ ] 2b. Reported on staging (eg. found during regression or PR testing)
• [ ] 2d. Reported on a PR
• [ ] 2z. Other:

Who reported the bug:

• [ ] 3a. Expensify user
• [ ] 3b. Expensify employee
• [ ] 3c. Contributor
• [x] 3d. QA
• [ ] 3z. Other:

• [x] [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: https://github.com/Expensify/App/pull/51850/files#r1896283986.

• [x] [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A.

• [x] [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.

• [ ] [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

[!note] The QuickBooks Desktop setup RHP page is only available on Web and Desktop (wide layout), on Native and mWeb we're not allow to connect QBD.

1. Login with a test account and create a new workspace.
2. Go to workspace settings > "More Features" and enable the Accounting.
3. Navigate to Accounting and click on the Connect button for QuickBooks Desktop.
4. Upgrade your workspace to Control plan as suggested then click Got it, thanks.
5. Verify that the QuickBooks Desktop setup RHP page appears right away with a loading indicator under the Open this link to connect section.
6. Navigate to any other page in workspace settings.
7. Go back to Accounting and verify that QuickBooks Desktop is displayed like an established connection, but without other details since the setup must follow the QuickBooks Desktop setup steps in order for the connection to be complete.

Do we agree 👍 or 👎.

[bfitzexpensify]

Payment summary:

@ikevin127 paid $250 via Upwork @getusha to be paid $250 via NewDot manual request

[garrettmknight]

$250 approved for @getusha