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

Adds the FailureReason property to StripeRedirect #1119

Merged
merged 1 commit into from
Mar 1, 2018

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Feb 28, 2018

We're currently missing the FailureReason property which is documented
here [1]. This patch adds it.

Fixes #1118.

[1] https://stripe.com/docs/api#source_object-redirect-failure_reason

r? @ob-stripe

@ob-stripe
Copy link
Contributor

@brandur-stripe @AFallad asked in #1121 if we could declare constants with the possible values for this property. I think we can do one better and create an enum type -- we currently have only one in the entire library (StripeBilling), but ideally we should have one for each of these attributes where the value is constrained like this.

We're currently missing the `FailureReason` property which is documented
here [1]. This patch adds it.

We do something slightly unusual (so far) here by adding reasons as a
new new called `StripeRedirectFailureReason`. There is only one other
documented instance of this in the library in `StripeBilling`, but we
might want to further pursue this pattern elsewhere in the future.

Fixes #1118.

[1] https://stripe.com/docs/api#source_object-redirect-failure_reason
@brandur-stripe
Copy link
Contributor

@ob-stripe Yeah, great point. There's a slight disadvantage in the enum in that there's no way for users to access a new value without first reporting the problem to us (given our current machinery, it's unlikely we'll notice ourselves), but we have this problem already for properties on classes anyway, so I wouldn't have a problem with trying this out.

I've updated the pull request. Can you take a look?

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

LGTM!

@ob-stripe ob-stripe merged commit 05b2663 into master Mar 1, 2018
@ob-stripe ob-stripe deleted the brandur-failure-reason branch March 1, 2018 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants