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

Jump to bottom

Use more user friendly errors when choosing default fund for Wallet #7929

Merged
merged 14 commits into from
Mar 10, 2022
Merged

Use more user friendly errors when choosing default fund for Wallet #7929

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7f81c79
Add the error messages to CONST
mountiny Feb 27, 2022
7596e3c
Handle the specific errors in setWalletLinkedAccount action and add e…
mountiny Feb 27, 2022
c1fcfe3
Move the error handling to catch
mountiny Feb 28, 2022
e468caf
Remove the translation key from es.js
mountiny Feb 28, 2022
6fde654
Fix style errors
mountiny Feb 28, 2022
b31a27d
Merge branch 'main' into vit-improveWalletErrors
mountiny Mar 3, 2022
5bb9ffd
Update the copies
mountiny Mar 3, 2022
eb83cb6
Remove the ambiguous error message and use the default one
mountiny Mar 3, 2022
4d14ac6
Resolve merge conflict with the translations
mountiny Mar 8, 2022
b8a61bc
Update the copy again and reduce the number of error messages
mountiny Mar 8, 2022
4340851
Merge branch 'main' into vit-improveWalletErrors
mountiny Mar 9, 2022
42c161f
Add the Spanish translations
mountiny Mar 9, 2022
fe66a08
Fix lint issues
mountiny Mar 9, 2022
d1fdfa6
Use String literals
mountiny Mar 9, 2022
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
6 changes: 6 additions & 0 deletions src/CONST.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,12 @@ const CONST = {
UNEXPECTED: 'Unexpected error',
MISSING_FIELD: 'Missing required additional details fields',
UNABLE_TO_VERIFY: 'Unable to verify identity',
NO_ACCOUNT_TO_LINK: '405 No account to link to wallet',
INVALID_WALLET: '405 Invalid wallet account',
NOT_OWNER_OF_BANK_ACCOUNT: '401 Wallet owner does not own linked bank account',
INVALID_BANK_ACCOUNT: '405 Attempting to link an invalid bank account to a wallet',
NOT_OWNER_OF_FUND: '401 Wallet owner does not own linked fund',
INVALID_FUND: '405 Attempting to link an invalid fund to a wallet',
},
STEP: {
ONFIDO: 'OnfidoStep',
Expand Down
8 changes: 7 additions & 1 deletion src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export default {
enterManually: 'Enter it manually',
message: 'Message ',
leaveRoom: 'Leave room',
conciergeHelp: 'Please reach out to Concierge for help.',
},
attachmentPicker: {
cameraPermissionRequired: 'Camera permission required',
Expand Down Expand Up @@ -357,7 +358,6 @@ export default {
paymentMethodsTitle: 'Payment methods',
setDefaultConfirmation: 'Make default payment method',
setDefaultSuccess: 'Default payment method set!',
setDefaultFailure: 'Failed to set default payment method.',
deleteAccount: 'Delete Account',
deleteConfirmation: 'Are you sure that you want to delete this account?',
deleteBankAccountSuccess: 'Bank account successfully deleted',
Expand All @@ -366,6 +366,12 @@ export default {
allSet: 'All Set!',
transferConfirmText: ({amount}) => `${amount} will hit your account shortly!`,
gotIt: 'Got it, Thanks!',
error: {
notOwnerOfBankAccount: 'There was an error setting this bank account as your default payment method.',
invalidBankAccount: 'This bank account is temporarily suspended.',
notOwnerOfFund: 'There was an error setting this card as your default payment method.',
setDefaultFailure: 'Something went wrong. Please chat with Concierge for further assistance.',
},
},
transferAmountPage: {
transfer: ({amount}) => `Transfer${amount ? ` ${amount}` : ''}`,
Expand Down
8 changes: 7 additions & 1 deletion src/languages/es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ export default {
enterManually: 'Ingresar manualmente',
message: 'Chatear con ',
leaveRoom: 'Salir de la sala de chat',
conciergeHelp: 'Por favor contacta con Concierge para obtener ayuda.',
},
attachmentPicker: {
cameraPermissionRequired: 'Se necesita permiso para usar la cámara',
Expand Down Expand Up @@ -357,7 +358,6 @@ export default {
paymentMethodsTitle: 'Métodos de pago',
setDefaultConfirmation: 'Marcar como método de pago predeterminado',
setDefaultSuccess: 'Método de pago configurado',
setDefaultFailure: 'No se ha podido configurar el método de pago.',
deleteAccount: 'Eliminar cuenta',
deleteConfirmation: '¿Estás seguro de que quieres eliminar esta cuenta?',
deleteBankAccountSuccess: 'Cuenta bancaria eliminada correctamente',
Expand All @@ -366,6 +366,12 @@ export default {
allSet: 'Todo listo!',
transferConfirmText: ({amount}) => `${amount} llegará a tu cuenta en breve!`,
gotIt: 'Gracias!',
error: {
notOwnerOfBankAccount: 'Ha ocurrido un error al establecer esta cuenta bancaria como tu método de pago predeterminado.',
invalidBankAccount: 'Esta cuenta bancaria está temporalmente suspendida.',
notOwnerOfFund: 'Ha ocurrido un error al establecer esta tarjeta de crédito como tu método de pago predeterminado.',
setDefaultFailure: 'No se ha podido configurar el método de pago.',
},
},
transferAmountPage: {
transfer: ({amount}) => `Transferir${amount ? ` ${amount}` : ''}`,
Expand Down
24 changes: 19 additions & 5 deletions src/libs/actions/PaymentMethods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,26 @@ function setWalletLinkedAccount(password, bankAccountID, fundID) {
walletLinkedAccountID: bankAccountID || fundID, walletLinkedAccountType: bankAccountID ? CONST.PAYMENT_METHODS.BANK_ACCOUNT : CONST.PAYMENT_METHODS.DEBIT_CARD,
});
Growl.show(Localize.translateLocal('paymentsPage.setDefaultSuccess'), CONST.GROWL.SUCCESS, 5000);
} else {
Growl.show(Localize.translateLocal('paymentsPage.setDefaultFailure'), CONST.GROWL.ERROR, 5000);
return;
}
Growl.show(Localize.translateLocal('paymentsPage.error.setDefaultFailure'), CONST.GROWL.ERROR, 5000);
}).catch((error) => {
// Make sure to show user more specific errors which will help support identify the problem faster.
switch (error.message) {
case CONST.WALLET.ERROR.INVALID_WALLET:
case CONST.WALLET.ERROR.NOT_OWNER_OF_BANK_ACCOUNT:
Growl.show(Localize.translateLocal('paymentsPage.error.notOwnerOfBankAccount') + " " + Localize.translateLocal('common.conciergeHelp'), CONST.GROWL.ERROR, 5000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per Ionatan suggestion, I have put the general Please contact Concierge message to separate translation so it can be reused. I have concatenated the two messages like this but I am not sure if there is a preference over doing this or using the `${foo} ${bar}` way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, linter has answered this for me by throwing an error :)

return;
case CONST.WALLET.ERROR.NOT_OWNER_OF_FUND:
case CONST.WALLET.ERROR.INVALID_FUND:
Growl.show(Localize.translateLocal('paymentsPage.error.notOwnerOfFund') + " " + Localize.translateLocal('common.conciergeHelp'), CONST.GROWL.ERROR, 5000);
return;
case CONST.WALLET.ERROR.INVALID_BANK_ACCOUNT:
Growl.show(Localize.translateLocal('paymentsPage.error.invalidBankAccount') + " " + Localize.translateLocal('common.conciergeHelp'), CONST.GROWL.ERROR, 5000);
return;
default:
Growl.show(Localize.translateLocal('paymentsPage.error.setDefaultFailure'), CONST.GROWL.ERROR, 5000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone know if it's possible for this API command promise method to land in this .catch() block?

Copy link
Contributor

Choose a reason for hiding this comment

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

What exception is thrown and where?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I think it's this weird code here that I am removing 😄

App/src/libs/API.js

Lines 182 to 187 in c26e415

if (response.jsonCode === 405 || response.jsonCode === 404) {
// IOU Split & Request money transactions failed due to invalid amount(405) or unable to split(404)
// It's a failure, so reject the queued request
queuedRequest.reject(response);
return;
}

I think most errors do not work this way and catch() should never do anything. No worries though just in the process of cleaning some stuff up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is this one which we decided not to catch specifically as it should not happen but if it happens, the error message would need to be very general as the default already was.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes thanks I would like to know where the exception gets thrown in the JavaScript layer.

This issue has more details: https://github.com/Expensify/Expensify/issues/201109

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting, I have first tried to handle it in the then case but it did not work for me, catch did. Not sure now why that is happening. Catch worked locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for the context! 🙌

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's because the promise is getting rejected which will trigger the .catch() block and would be similar to throwing an exception inside the .then() block. Kind of weird IMO because who knows how many API can throw these codes (or which may throw them in the future) and any that are not handled like you did here will crash the app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that is weird indeed. We should standardize this for sure.

}
})
.catch(() => {
Growl.show(Localize.translateLocal('paymentsPage.setDefaultFailure'), CONST.GROWL.ERROR, 5000);
});
}

Expand Down