-
Notifications
You must be signed in to change notification settings - Fork 352
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 active to plan struct #399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elijahkim Awesome find! Looks like this was updated. Just a few comments and should be g2g!
lib/stripe/subscriptions/plan.ex
Outdated
@@ -61,7 +61,8 @@ defmodule Stripe.Plan do | |||
tiers_mode: boolean | nil, | |||
transform_usage: map | nil, | |||
trial_period_days: non_neg_integer | nil, | |||
usage_type: String.t() | nil | |||
usage_type: String.t() | nil, | |||
active: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind updating the doc block above with this?
{
"id": "ivory-extended-580",
"object": "plan",
"active": true,
"aggregate_usage": null,
"amount": 999,
"billing_scheme": "per_unit",
"created": 1531234812,
"currency": "usd",
"interval": "month",
"interval_count": 1,
"livemode": false,
"metadata": {
},
"nickname": null,
"product": "prod_DCmtkptv7qHXGE",
"tiers": null,
"tiers_mode": null,
"transform_usage": null,
"trial_period_days": null,
"usage_type": "licensed"
}
lib/stripe/subscriptions/plan.ex
Outdated
@@ -138,6 +141,7 @@ defmodule Stripe.Plan do | |||
optional(:nickname) => String.t(), | |||
optional(:product) => Stripe.id() | Stripe.Product.t(), | |||
optional(:trial_period_days) => non_neg_integer, | |||
optional(:active) => boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like list
s can now be filtered by active
as well. Mind adding it there as well as an optional attribute?
@snewcomer I think I covered all the changes but coveralls is whining :P |
@elijahkim Also 👇
|
lib/stripe/subscriptions/plan.ex
Outdated
@@ -167,7 +173,8 @@ defmodule Stripe.Plan do | |||
optional(:ending_before) => t | Stripe.id(), | |||
optional(:limit) => 1..100, | |||
optional(:product) => Stripe.Product.t() | Stripe.id(), | |||
optional(:starting_after) => t | Stripe.id() | |||
optional(:starting_after) => t | Stripe.id(), | |||
optional(:active) => boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snewcomer isn't this the change that you requested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think github is having trouble displaying the diffs cleanly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are so right. Yep you added it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 note - hard to improve code coverage with stripe mock...
lib/stripe/subscriptions/plan.ex
Outdated
@@ -167,7 +173,8 @@ defmodule Stripe.Plan do | |||
optional(:ending_before) => t | Stripe.id(), | |||
optional(:limit) => 1..100, | |||
optional(:product) => Stripe.Product.t() | Stripe.id(), | |||
optional(:starting_after) => t | Stripe.id() | |||
optional(:starting_after) => t | Stripe.id(), | |||
optional(:active) => boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are so right. Yep you added it!
Great! Would love to see it merged soon. Thanks for being so responsive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last request that totally swooped right by me. Could you alpha order the properties?
I'll merge tomorrow if you aren't able to get to it as you have done a lot already 🙏
lib/stripe/subscriptions/plan.ex
Outdated
@@ -83,7 +86,8 @@ defmodule Stripe.Plan do | |||
:tiers_mode, | |||
:transform_usage, | |||
:trial_period_days, | |||
:usage_type | |||
:usage_type, | |||
:active |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing I just realized. alpha order the properties :( .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elijahkim Looks like trailing commas are failing the biuld?
Active is missing in plan https://stripe.com/docs/api#service_product_object-active