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.

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 payment error messages #1728

Merged

Conversation

Projects
None yet
4 participants
@meandavejustice
Copy link
Member

commented Jul 10, 2019

error-dialog

@meandavejustice meandavejustice requested a review from lmorchard Jul 10, 2019

@meandavejustice meandavejustice force-pushed the meandavejustice:1589-payment-error-messages branch from d6ce03a to 90d914e Jul 10, 2019

@jackiemunroe
Copy link

left a comment

+1

@lmorchard
Copy link
Member

left a comment

Looks pretty good to me after a looking-over! Going to play with it locally for a brief bit before merging, but I don't expect issues. Might be nice to have some tests for errors.ts now that there's a bit of lookup logic in getErrorMessage(), but I wouldn't block merge on that.

@@ -1,9 +1,109 @@
// ref: fxa-auth-server/lib/error.js
export const AuthServerErrno = {

This comment has been minimized.

Copy link
@lmorchard

lmorchard Jul 10, 2019

Member

I usually prefer leaving the exports in-line, rather than listing them all at the end.

That said... there are a lot of intermediate variables here that aren't exported. Listing them at the end seems to keep them from getting lost in the rest of the file. So, 👍

@lmorchard

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Oh, but it looks like you do need to set up GPG commit signing here since we turned on enforcement of that for this project :/

@lmorchard

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

Oh yeah, would probably be nice to add some stories to maybe both the Product & Subscription routes to display & demo some of these new errors. But we could spin that off as a follow-up

@vbudhram
Copy link
Contributor

left a comment

@meandavejustice Nice work on getting this approved. Before you land, you should update the commit message so that the change gets picked up in our release script.

@meandavejustice

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

I've added some basic tests for errors.ts, updated the commit message to follow our guidelines, and enabled gpg signing.

@lmorchard

Oh yeah, would probably be nice to add some stories to maybe both the Product & Subscription routes to display & demo some of these new errors. But we could spin that off as a follow-up

I haven't figured out a good way to trigger this error, while developing I was manually adding some characters to the api key to invalidate it. 🤔

@meandavejustice meandavejustice force-pushed the meandavejustice:1589-payment-error-messages branch 2 times, most recently from abba789 to f802b14 Jul 10, 2019

@lmorchard

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

I haven't figured out a good way to trigger this error, while developing I was manually adding some characters to the api key to invalidate it.

Oh, actually, for storybook purposes, it might be enough to set up some React views with dialogs just to show how the messages render & appear in the layout. Doesn't have to be a part of this PR though

feat(fxa-payments-server): add payment error messages
- fixes #1589

add error messages to use instead of relying on stripe responses,
also set us up for L10N for post-mvp milestone.

@meandavejustice meandavejustice force-pushed the meandavejustice:1589-payment-error-messages branch from f802b14 to 53f03ce Jul 15, 2019

@meandavejustice meandavejustice merged commit 1441016 into mozilla:master Jul 15, 2019

1 check passed

test Workflow: test
Details

@meandavejustice meandavejustice deleted the meandavejustice:1589-payment-error-messages branch Jul 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.