-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[QBD] Handle errors gracefully if the setup link cannot be obtained #51850
Changes from 3 commits
6cb975c
224781c
57f6848
f8ca901
720dcb4
955872a
1dc585e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,16 +8,22 @@ 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'; | ||
import ImageSVG from '@components/ImageSVG'; | ||
import ScreenWrapper from '@components/ScreenWrapper'; | ||
import Text from '@components/Text'; | ||
import TextLink from '@components/TextLink'; | ||
import useEnvironment from '@hooks/useEnvironment'; | ||
import useLocalize from '@hooks/useLocalize'; | ||
import useNetwork from '@hooks/useNetwork'; | ||
import useThemeStyles from '@hooks/useThemeStyles'; | ||
import {setConnectionError} from '@libs/actions/connections'; | ||
import * as QuickbooksDesktop from '@libs/actions/connections/QuickbooksDesktop'; | ||
import Navigation from '@libs/Navigation/Navigation'; | ||
import type {SettingsNavigatorParamList} from '@libs/Navigation/types'; | ||
import * as PolicyAction from '@userActions/Policy/Policy'; | ||
import CONST from '@src/CONST'; | ||
import ROUTES from '@src/ROUTES'; | ||
import type SCREENS from '@src/SCREENS'; | ||
|
||
|
@@ -26,20 +32,31 @@ type RequireQuickBooksDesktopModalProps = StackScreenProps<SettingsNavigatorPara | |
function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalProps) { | ||
const {translate} = useLocalize(); | ||
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 ContentWrapper = codatSetupLink ? ({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) => { | ||
setCodatSetupLink(String(response?.setupUrl ?? '')); | ||
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')); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @hoangzinh, we do not send via Pusher this error, it is returned instead in the response's body on the PHP side. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, I guess I have to set optimistic error here |
||
setHasError(true); | ||
} | ||
} | ||
|
||
setIsLoading(false); | ||
}); | ||
}, [policyID]); | ||
}, [policyID, translate]); | ||
|
||
useEffect(() => { | ||
// Since QBD doesn't support Taxes, we should disable them from the LHN when connecting to QBD | ||
|
@@ -59,6 +76,9 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro | |
}, | ||
}); | ||
|
||
const shouldShowLoading = isLoading || (!codatSetupLink && !hasError); | ||
const shouldShowError = hasError; | ||
|
||
return ( | ||
<ScreenWrapper | ||
shouldEnablePickerAvoiding={false} | ||
|
@@ -71,9 +91,28 @@ function RequireQuickBooksDesktopModal({route}: RequireQuickBooksDesktopModalPro | |
onBackButtonPress={() => Navigation.dismissModal()} | ||
/> | ||
<ContentWrapper> | ||
{isLoading || !codatSetupLink ? ( | ||
<FullScreenLoadingIndicator style={[styles.flex1, styles.pRelative]} /> | ||
) : ( | ||
{shouldShowLoading && <FullScreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />} | ||
{shouldShowError && ( | ||
<View style={[styles.flex1, styles.justifyContentCenter, styles.alignItemsCenter, styles.ph5, styles.mb9]}> | ||
<Icon | ||
src={Illustrations.BrokenMagnifyingGlass} | ||
width={116} | ||
height={168} | ||
/> | ||
<Text style={[styles.textHeadlineLineHeightXXL, styles.mt3]}>{translate('workspace.qbd.setupPage.setupErrorTitle')}</Text> | ||
<Text style={[styles.textSupporting, styles.ph5, styles.mv3, styles.textAlignCenter]}> | ||
{translate('workspace.qbd.setupPage.setupErrorBody1')}{' '} | ||
<TextLink | ||
href={`${environmentURL}/${ROUTES.CONCIERGE}`} | ||
style={styles.link} | ||
> | ||
{translate('workspace.qbd.setupPage.setupErrorBodyContactConcierge')} | ||
</TextLink>{' '} | ||
{translate('workspace.qbd.setupPage.setupErrorBody2')} | ||
</Text> | ||
</View> | ||
)} | ||
{!shouldShowLoading && !shouldShowError && ( | ||
<View style={[styles.flex1, styles.ph5]}> | ||
<View style={[styles.alignSelfCenter, styles.computerIllustrationContainer, styles.pv6]}> | ||
<ImageSVG src={Computer} /> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add
isConnected
: false or changeisSuccessful
toisConnected
here to not show optionsApp/src/libs/actions/connections/index.ts
Lines 314 to 328 in 955872a
App/src/pages/workspace/accounting/PolicyAccountingPage.tsx
Line 284 in 955872a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZhenjaHorbach do you mean BE (Pusher event) sent
isConnected=true
in your case?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
When we have an error
We don't receive anything
But in case an error we save in Onyx this using setConnectionError
As result we return false here
App/src/libs/actions/connections/index.ts
Lines 314 to 328 in 955872a
and show options
App/src/pages/workspace/accounting/PolicyAccountingPage.tsx
Line 284 in 955872a
App/src/pages/workspace/accounting/PolicyAccountingPage.tsx
Line 371 in 955872a
For example when we have correct response with setupLink we save "isConnected": false in Onyx and don't show options because shouldHideConfigurationOptions === true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see. Good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here 1dc585e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks !