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

mountiny · 2022-02-28T00:51:39Z

Related to https://github.com/Expensify/Web-Expensify/pull/33132

And related to small backend change where I emphasize the error messages need to be updated in frontend too if changed in backend https://github.com/Expensify/Auth/pull/6477

Details

We have only been showing a generic error when setting a default fund for Expensify Wallet has failed. This change handles all the possible error and gives the user better feedback when it fails. Unfortunately, for most of these errors the users cannot do much on their own, but knowing the specific error message can help the support to identify the next steps to fix it.

Fixed Issues

$ #7521

Tests

Verify that no errors appear in the JS console

To test all the specific errors would be hard. We can leave this for Applause as they reported this problem and we are only making the error messages a bit more helpful.

Prerequisite: Set the VBA in testing account (But make sure it is not in OPEN state)

Launch the app
Log in expensifail account (when locally testing any account which has Wallet)
Go to Setting - Payment
Tap on some card
Tap Set a default payment
Enter correct password
Tap Make Default payment method
Make sure there is more specific like this.

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Verify that no errors appear in the JS console

Prerequisite: Set the VBA in testing account

Launch the app
Log in expensifail account
Go to Setting - Payment
Tap on Verified Account
Tap Set a default payment
Enter correct password
Tap Make Default payment method
One of these errors should show:

'There was an error setting this bank account as your default payment method.',
'This bank account is temporarily suspended.',
'There was an error setting this card as your default payment method.',
'Something went wrong. Please chat with Concierge for further assistance.',

Tested On

Screenshots

Web

since this is only API error handling change, I have not tested it on all platforms as there is not expectation of change.

…nglish error messages

mountiny · 2022-02-28T00:52:43Z

src/languages/es.js

+            noAccountToLink: 'Something went wrong. Please, contact Concierge.',
+            invalidWallet: 'Your Wallet is currently suspended. Reach out to Concierge for help.',
+            notOwnerOfBankAccount: 'We couldn\'t link this bank account to your Wallet. Please, reach out to Concierge for help.',
+            invalidBankAccount: 'This bank account is temporarily suspended. Reach out to Concierge for help.',
+            notOwnerOfFund: 'We couldn\'t link this fund to your Wallet. Reach out to Concierge for help.',
+            invalidFund: 'This fund is temporarily suspended. Reach out to Concierge for help.',


Waiting for translations.

kevinksullivan · 2022-03-02T01:24:21Z

Sorry @mountiny I just caught up to this issue after OOO. We do not mention the word "Wallet" in product anywhere, so can we update the error message copy to avoid doing so?

Also, what is a "fund"? I don't know what that means or how to fix it, so I think that will be confusing to users.

Here are my suggestions for the first 4 (will propose something for the last two once I know what "fund" means):

error	message	Explanation
405 No account to link to wallet	Something went wrong. Please chat with concierge for further assistance.	If this happens something is wrong in the App so we should definitely report a bug, this is not a mistake on user's side.
405 Invalid wallet account	We were unable to set this bank account as your default payment method. Please reach out to concierge for help.	User's wallet is not in Open state and most likely that means the state is Suspended. They would reach out to Concierge to have this sorted out.
401 Wallet owner does not own linked bank account	We were unable to set this bank account as your default payment method. Please reach out to concierge for help.	This should not happen very often and it would indicate some problem in users account or some bad actor trying to link someone else's bank account to their wallet.
405 Attempting to link an invalid bank account to a wallet	This bank account is temporarily suspended. Please reach out to concierge for help.	The bank account they trying to use is most likely in suspended state and Concierge should look into why.
401 Wallet owner does not own linked fund	We were unable to set this card as your default payment method. Please reach out to concierge for help.	This should not happen very often and it would indicate some problem in users account or some bad actor trying to link someone else's card to their wallet.
405 Attempting to link an invalid fund to a wallet	We were unable to set this card as your default payment method. Please reach out to concierge for help.	The fund they trying to use is most likely in suspended state and Concierge should look into why.

mountiny · 2022-03-02T01:28:30Z

@kevinksullivan Oh, thank you for heads up, I think these should still be in beta so users should not see it, but better safe than sorry. cc @rosegrech as you helped with the translations.

I think funds are related to cards in this case, but I am not sure if it can be more things. There are VBAs and cards which can be used to top up the Wallet, right? @madmax330 should be able to shed more light on this too, thank you 🙇

kevinksullivan · 2022-03-02T01:33:49Z

Ahh got it ok, yeah those are likely debit cards then. I updated the table to incorporate suggestions for those as well. Basically I suggest we:

Drop mention of Wallet
Not explain that a card or account is Suspended if the user cannot do anything about it. If the best thing they can do is reach out to us for help, and they're either a bad actor or someone that has to reach out to us, then we just instruct them to talk to concierge.

mountiny · 2022-03-02T01:40:47Z

Sounds good to me, I will just wait for @rosegrech if she has anything to add here as she has helped with the original copy (but I suggested the Wallet and Suspended terms 😅 )

Gonna update the Spanish translation to HOLD

rosegrech · 2022-03-02T20:36:05Z

Thanks @kevinksullivan for jumping in. I did not realize about avoiding "wallet" so I updated our SO on copy.

Two quick overall things to note @vitHoracek

We should cap the "C' for "Concierge"
We try to use contractions when possible (not a big deal if we don't as it's proper ENG but just to be more casual speak. For example: use "We're" vs" We were", " I've" vs "I have", "Don't" vs "do not" etc/

mountiny · 2022-03-03T19:41:13Z

Asked for translations in Slack. Thank you very much for your help on this!

mountiny · 2022-03-08T14:36:01Z

I have updated the copy based on the discussion led here. For errors and notifications, we are trying to have a passive form. I have also reduced the number of messages as they have overlapped so now there are 3 messages and one deafult.

stitesExpensify · 2022-03-08T21:18:58Z

Is this still WIP?

mountiny · 2022-03-08T21:59:41Z

Still WIP, waiting for the Spanish translations as they were discussing more broadly what patterns to use in NewDot.

mountiny · 2022-03-09T20:20:18Z

src/libs/actions/PaymentMethods.js

+            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);


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.

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

mountiny · 2022-03-09T20:23:01Z

@stitesExpensify @madmax330 This is now finally ready for a review! Thank you very much 🙇

stitesExpensify

LGTM!

madmax330

LGTM, do we have to do all the steps mentioned in the OP or is that for contributor PRs?

mountiny · 2022-03-10T11:43:25Z

No, I don't think that is necessary in this case. Also, it does not influence UI really so we should be good. Thank you very much for your reviews!

OSBotify · 2022-03-10T11:53:12Z

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

OSBotify · 2022-03-11T00:59:20Z

🚀 Deployed to staging by @mountiny in version: 1.1.42-0 🚀

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

marcaaron · 2022-03-11T03:25:50Z

src/libs/actions/PaymentMethods.js

+                    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);


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

What exception is thrown and where?

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.

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.

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

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.

Ah, thanks for the context! 🙌

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.

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

kbecciv · 2022-03-11T19:12:33Z

@mountiny We also experienced with App freezes when tried to set payment as default.

Image.from.iOS.6.MP4

mountiny · 2022-03-11T19:34:41Z

@kbecciv hmm, that is odd. There was this issue #7869 which is exactly what you describe but it says it is fixed.

Can you confirm if this issue does not appear in production? If it happens in production too, we ould reopen the linked issue.

kbecciv · 2022-03-14T03:10:16Z

@mountiny Issue appears in production as well.

Image.from.iOS.10.MP4

mountiny · 2022-03-14T10:28:48Z

Thank you, I have reopened the linked issue!

OSBotify · 2022-03-15T20:52:03Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

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

mountiny added 3 commits February 27, 2022 23:32

Add the error messages to CONST

7f81c79

Handle the specific errors in setWalletLinkedAccount action and add e…

7596e3c

…nglish error messages

Move the error handling to catch

c1fcfe3

mountiny requested a review from stitesExpensify February 28, 2022 00:51

mountiny requested a review from a team as a code owner February 28, 2022 00:51

mountiny self-assigned this Feb 28, 2022

MelvinBot requested review from madmax330 and removed request for a team February 28, 2022 00:52

mountiny commented Feb 28, 2022

View reviewed changes

Remove the translation key from es.js

e468caf

mountiny changed the title ~~Use more user friendly errors when choosing default fund for Wallet~~ [WIP translations] Use more user friendly errors when choosing default fund for Wallet Feb 28, 2022

Fix style errors

6fde654

mountiny added 2 commits March 3, 2022 19:33

Merge branch 'main' into vit-improveWalletErrors

b31a27d

Update the copies

5bb9ffd

mountiny added 3 commits March 3, 2022 19:42

Remove the ambiguous error message and use the default one

eb83cb6

Resolve merge conflict with the translations

4d14ac6

Update the copy again and reduce the number of error messages

b8a61bc

stitesExpensify previously approved these changes Mar 8, 2022

View reviewed changes

stitesExpensify mentioned this pull request Mar 8, 2022

Payments - Error message appears when making a bank account as a default payment #7868

Closed

mountiny added 2 commits March 9, 2022 19:56

Merge branch 'main' into vit-improveWalletErrors

4340851

Add the Spanish translations

42c161f

mountiny dismissed stitesExpensify’s stale review via 42c161f March 9, 2022 20:16

mountiny changed the title ~~[WIP translations] Use more user friendly errors when choosing default fund for Wallet~~ Use more user friendly errors when choosing default fund for Wallet Mar 9, 2022

mountiny commented Mar 9, 2022

View reviewed changes

mountiny requested a review from stitesExpensify March 9, 2022 20:20

Fix lint issues

fe66a08

Use String literals

d1fdfa6

stitesExpensify approved these changes Mar 10, 2022

View reviewed changes

madmax330 approved these changes Mar 10, 2022

View reviewed changes

mountiny merged commit 41bd789 into main Mar 10, 2022

mountiny deleted the vit-improveWalletErrors branch March 10, 2022 11:43

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

marcaaron reviewed Mar 11, 2022

View reviewed changes

mountiny mentioned this pull request Mar 11, 2022

[HOLD for payment 2022-04-12] [$500] iOS - App freezes when try to delete PayPal payment #7768

Closed

mountiny mentioned this pull request Mar 14, 2022

IOS - App is freezing after tap on "Make default payment method" #7869

Closed

marcaaron mentioned this pull request Mar 29, 2022

Remove catch in SetWalletLinkedAccount #8338

Merged

1 task

stitesExpensify mentioned this pull request Mar 29, 2022

Payments - Generic error message appears when deleting bank card #8325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mountiny commented Feb 28, 2022 •

edited

Loading

mountiny Feb 28, 2022

kevinksullivan commented Mar 2, 2022 •

edited

Loading

mountiny commented Mar 2, 2022

kevinksullivan commented Mar 2, 2022

mountiny commented Mar 2, 2022

rosegrech commented Mar 2, 2022

mountiny commented Mar 3, 2022

mountiny commented Mar 8, 2022

stitesExpensify commented Mar 8, 2022

mountiny commented Mar 8, 2022

mountiny Mar 9, 2022

mountiny Mar 9, 2022

mountiny commented Mar 9, 2022

stitesExpensify left a comment

madmax330 left a comment

mountiny commented Mar 10, 2022

OSBotify commented Mar 10, 2022

OSBotify commented Mar 11, 2022

marcaaron Mar 11, 2022

marcaaron Mar 11, 2022

marcaaron Mar 11, 2022

mountiny Mar 11, 2022

marcaaron Mar 11, 2022

mountiny Mar 11, 2022

mountiny Mar 11, 2022

marcaaron Mar 11, 2022

mountiny Mar 11, 2022

kbecciv commented Mar 11, 2022

mountiny commented Mar 11, 2022

kbecciv commented Mar 14, 2022

mountiny commented Mar 14, 2022

OSBotify commented Mar 15, 2022

	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;
	}

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

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

Conversation

mountiny commented Feb 28, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Tested On

Screenshots

Web

Choose a reason for hiding this comment

kevinksullivan commented Mar 2, 2022 • edited Loading

mountiny commented Mar 2, 2022

kevinksullivan commented Mar 2, 2022

mountiny commented Mar 2, 2022

rosegrech commented Mar 2, 2022

mountiny commented Mar 3, 2022

mountiny commented Mar 8, 2022

stitesExpensify commented Mar 8, 2022

mountiny commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mountiny commented Mar 9, 2022

stitesExpensify left a comment

Choose a reason for hiding this comment

madmax330 left a comment

Choose a reason for hiding this comment

mountiny commented Mar 10, 2022

OSBotify commented Mar 10, 2022

OSBotify commented Mar 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbecciv commented Mar 11, 2022

mountiny commented Mar 11, 2022

kbecciv commented Mar 14, 2022

mountiny commented Mar 14, 2022

OSBotify commented Mar 15, 2022

mountiny commented Feb 28, 2022 •

edited

Loading

kevinksullivan commented Mar 2, 2022 •

edited

Loading