Skip to content
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.

Sign up for GitHub

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

Add error message in case of integration sync failure #42307

Merged
merged 14 commits into from
May 28, 2024
Merged

Add error message in case of integration sync failure #42307

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
68b7551
add error message in case of sync failure
SzymczakJ May 16, 2024
08b321a
Merge branch 'main' into @szymczak/error-message-sync-failure
SzymczakJ May 20, 2024
ecdeaf4
add translations
SzymczakJ May 20, 2024
e87d711
delete unrelated changes
SzymczakJ May 20, 2024
27972bf
fix PR comments
SzymczakJ May 20, 2024
de93763
Merge remote-tracking branch 'expensify-real/xero-merge-freeze' into …
SzymczakJ May 20, 2024
ef5d777
fix connections lastSync unsafe access
SzymczakJ May 20, 2024
5979a35
change translations
SzymczakJ May 23, 2024
52216fb
Merge branch 'xero-merge-freeze' into @szymczak/error-message-sync-fa…
SzymczakJ May 27, 2024
25db9b7
fix style
SzymczakJ May 27, 2024
b779dff
fix styles
SzymczakJ May 27, 2024
86f509f
fix lint errors
SzymczakJ May 27, 2024
adddbcd
fix type error
SzymczakJ May 27, 2024
4ce01d2
fix PR comments
SzymczakJ May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/components/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,15 @@ type MenuItemBaseProps = {
/** Error to display at the bottom of the component */
errorText?: MaybePhraseKey;

/** Any additional styles to pass to error text. */
errorTextStyle?: StyleProp<ViewStyle>;

/** Hint to display at the bottom of the component */
hintText?: MaybePhraseKey;

/** Should the error text red dot indicator be shown */
shouldShowErrorTextRedDot?: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not name this as shouldShowRedDotIndicator? This is consistent across and red dot is generally associated with errors.


/** A boolean flag that gives the icon a green fill if true */
success?: boolean;

Expand Down Expand Up @@ -308,6 +314,8 @@ function MenuItem(
helperText,
helperTextStyle,
errorText,
errorTextStyle,
shouldShowErrorTextRedDot,
hintText,
success = false,
focused = false,
Expand Down Expand Up @@ -683,9 +691,9 @@ function MenuItem(
{!!errorText && (
<FormHelpMessage
isError
shouldShowRedDotIndicator={false}
shouldShowRedDotIndicator={!!shouldShowErrorTextRedDot}
message={errorText}
style={styles.menuItemError}
style={[styles.menuItemError, errorTextStyle]}
/>
)}
{!!hintText && (
Expand Down
11 changes: 11 additions & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2340,6 +2340,17 @@ export default {
}
}
},
syncError: (integration?: ConnectionName): string => {
switch (integration) {
case CONST.POLICY.CONNECTIONS.NAME.QBO:
return "Couldn't connect to QuickBooks Online due to incorrect credentials.";
case CONST.POLICY.CONNECTIONS.NAME.XERO:
return "Couldn't connect to Xero due to incorrect credentials.";
default: {
return "Couldn't connect to integration due to incorrect credentials.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if "due to incorrect credentials" is the correct thing to say here. The lastSync.isSuccessful flag is used for any sync error, not just invalid credentials. It could be something like third-party was down, or the user is missing permissions in the third-party for what we're trying to do, etc.

Copy link
Contributor

@lakchote lakchote May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you François for answering quickly my previous question and raising this point!

cc @trjExpensify @zanyrenney in the Xero DD, and in the Figma we would show this Couldn't connect to Xero due to incorrect credentials. message.
image

But since it can be a lot of completely different things, maybe we need to change it?

Before having this discussion, I suggested to @SzymczakJ using as copy something like Couldn't connect to {integrationName}.. Generic yes, but it fits all the possible use cases. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zany might have more context, but I think this was just an example where incorrect credentials are at fault, but the intention is to pass in the most relevant error we have. Do we not have all of these in OldDot already?

Copy link
Contributor

@lakchote lakchote May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so after investigating, when sync fails, we have error codes (401 and 407) we can use to display a meaningful error message to the user.

See:
image

Currently on OD, the Xero error message when session has expired is:

xeroConnectFailedMessage: 'Your connection to Xero seems to have expired. Do you want to reconnect now?',

It appears in a modal like below:
image

We also have dynamic error messages in place (we could reuse the same thought process for QBO):

genericConnectFailedTitle: 'Couldn\'t connect to <%- connectionName %>',
genericConnectFailedMessage: 'Your connection to <%- connectionName %> has expired. You can correct this by clicking <b>Reconnect</b> and reauthorizing Expensify with <%- connectionName %>.',

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay cool, so in this case for NewDot, would the button inside the three dot overflow menu read Enter credentials in this scenario? So our direction would be to point to that instead of a "Reconnect" button?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we might want to keep it, at least for errors when connecting (not autoSync/export/manual export errors that are out of scope). Especially to demo it at XeroCon.
Thoughts @trjExpensify @arosiclair?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think we can keep it though the error we show here should just a generic message like Francois mentioned

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when connecting I agree. 👍

Copy link
Contributor Author

@SzymczakJ SzymczakJ May 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I should proceed with generic error message like "Couldn't connect to {integration}.", right? @trjExpensify

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! This one I believe:

genericConnectFailedTitle: 'Couldn't connect to <%- connectionName %>',

}
}
},
accounts: 'Chart of accounts',
taxes: 'Taxes',
imported: 'Imported',
Expand Down
11 changes: 11 additions & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2344,6 +2344,17 @@ export default {
}
}
},
syncError: (integration?: ConnectionName): string => {
switch (integration) {
case CONST.POLICY.CONNECTIONS.NAME.QBO:
return 'No se pudo conectar a QuickBooks Online debido a credenciales incorrectas.';
case CONST.POLICY.CONNECTIONS.NAME.XERO:
return 'No se pudo conectar a Xero debido a credenciales incorrectas.';
default: {
return 'No se pudo conectar a la integración debido a credenciales incorrectas.';
}
}
},
accounts: 'Plan de cuentas',
taxes: 'Impuestos',
imported: 'Importado',
Expand Down
9 changes: 7 additions & 2 deletions src/libs/actions/connections/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type {OnyxUpdate} from 'react-native-onyx';
import type {OnyxEntry, OnyxUpdate} from 'react-native-onyx';
import Onyx from 'react-native-onyx';
import * as API from '@libs/API';
import type {
Expand All @@ -13,6 +13,7 @@ import * as ErrorUtils from '@libs/ErrorUtils';
import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import type {ConnectionName, Connections, PolicyConnectionName} from '@src/types/onyx/Policy';
import type Policy from '@src/types/onyx/Policy';

function removePolicyConnection(policyID: string, connectionName: PolicyConnectionName) {
const optimisticData: OnyxUpdate[] = [
Expand Down Expand Up @@ -231,4 +232,8 @@ function updateManyPolicyConnectionConfigs<TConnectionName extends ConnectionNam
API.write(WRITE_COMMANDS.UPDATE_MANY_POLICY_CONNECTION_CONFIGS, parameters, {optimisticData, failureData, successData});
}

export {removePolicyConnection, updatePolicyConnectionConfig, updateManyPolicyConnectionConfigs, syncConnection};
function hasSynchronizationError(policy: OnyxEntry<Policy>, connectionName: PolicyConnectionName): boolean {
return policy?.connections?.[connectionName].lastSync?.isSuccessful === false;
}

export {removePolicyConnection, updatePolicyConnectionConfig, updateManyPolicyConnectionConfigs, hasSynchronizationError, syncConnection};
30 changes: 17 additions & 13 deletions src/pages/workspace/accounting/PolicyAccountingPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import usePermissions from '@hooks/usePermissions';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import {removePolicyConnection, syncConnection} from '@libs/actions/connections';
import {hasSynchronizationError, removePolicyConnection, syncConnection} from '@libs/actions/connections';
import {findCurrentXeroOrganization, getCurrentXeroOrganizationName, getXeroTenants} from '@libs/PolicyUtils';
import Navigation from '@navigation/Navigation';
import AccessOrNotFoundWrapper from '@pages/workspace/AccessOrNotFoundWrapper';
Expand All @@ -41,7 +41,7 @@ import type {PolicyConnectionName} from '@src/types/onyx/Policy';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type IconAsset from '@src/types/utils/IconAsset';

type MenuItemData = MenuItemProps & {pendingAction?: OfflineWithFeedbackProps['pendingAction']};
type MenuItemData = MenuItemProps & {pendingAction?: OfflineWithFeedbackProps['pendingAction']; errors?: OfflineWithFeedbackProps['errors']};

type PolicyAccountingPageOnyxProps = {
connectionSyncProgress: OnyxEntry<PolicyConnectionSyncProgress>;
Expand Down Expand Up @@ -172,7 +172,9 @@ function PolicyAccountingPage({policy, connectionSyncProgress}: PolicyAccounting
wrapperStyle: [styles.sectionMenuItemTopDescription],
shouldShowRightComponent: true,
title: integrationData?.title,

errorText: hasSynchronizationError(policy, connectedIntegration) ? translate('workspace.accounting.syncError', connectedIntegration) : undefined,
errorTextStyle: {marginTop: 8},
shouldShowErrorTextRedDot: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the majority of cases we use this error text without a red dot, so I decided to use shouldShowErrorTextRedDot this way.

description: isSyncInProgress
? translate('workspace.accounting.connections.syncStageName', connectionSyncProgress.stageInProgress)
: translate('workspace.accounting.lastSync'),
Expand Down Expand Up @@ -250,21 +252,23 @@ function PolicyAccountingPage({policy, connectionSyncProgress}: PolicyAccounting
]),
];
}, [
connectedIntegration,
connectionSyncProgress?.stageInProgress,
currentXeroOrganization,
currentXeroOrganizationName,
tenants,
policy,
isSyncInProgress,
overflowMenu,
policy?.connections,
policyConnectedToXero,
connectedIntegration,
policyID,
styles,
translate,
styles.sectionMenuItemTopDescription,
styles.popoverMenuIcon,
styles.fontWeightNormal,
connectionSyncProgress?.stageInProgress,
theme.spinner,
overflowMenu,
threeDotsMenuPosition,
translate,
policyConnectedToXero,
currentXeroOrganizationName,
tenants.length,
accountingIntegrations,
currentXeroOrganization?.id,
]);

const otherIntegrationsItems = useMemo(() => {
Expand Down
Loading