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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump Stripe API to 2018-08-23 #415

Merged
merged 8 commits into from Sep 10, 2018

Conversation

mcrumm
Copy link
Contributor

@mcrumm mcrumm commented Sep 7, 2018

  • Bump Stripe.API version to 2018-08-23
  • Support tax_info on Stripe.Customer (already exists! 馃帀)
  • Support float values for percent_off in Stripe.Coupon
  • Remove business_vat_id from Stripe.Customer
  • Remove source from Stripe.Subscription.create/2
  • Remove source from Stripe.Subscription.update/2
  • Deprecate Stripe.Subscription.delete/2 with at_period_end: true
  • Deprecate Stripe.Subscription.delete/3

@snewcomer Pretty sure all of the above are 馃憤, but if you wouldn't mind checking them off when you feel the same way, that would be great!

@coveralls
Copy link

coveralls commented Sep 7, 2018

Coverage Status

Coverage increased (+1.5%) to 83.586% when pulling bc214c4 on mcrumm:stripe-api-2018-08-23 into 212c8ee on code-corps:master.

Copy link
Collaborator

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

@mcrumm 馃挴 perfecto! Just a few comments/potential small changes. Really like what you did with the deprecation stuff.

@@ -28,7 +28,7 @@ defmodule Stripe.Coupon do
max_redemptions: pos_integer | nil,
metadata: Stripe.Types.metadata(),
name: String.t() | nil,
percent_off: pos_integer | nil,
percent_off: number | nil,
Copy link
Collaborator

Choose a reason for hiding this comment

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

馃憤

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind changing tax_percent in invoice as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem!

@@ -7,7 +7,8 @@ defmodule Stripe.CustomerTest do
end

test "is retrievable" do
assert {:ok, %Stripe.Customer{}} = Stripe.Customer.retrieve("cus_123")
assert {:ok, %Stripe.Customer{} = cus} = Stripe.Customer.retrieve("cus_123")
Copy link
Collaborator

Choose a reason for hiding this comment

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

cus - is this variable unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that's a hold-over from some manual verification. I will remove.

is deprecated. Use `Subscription.update/2` with
`cancel_at_period_end: true` instead.
"""
@deprecated "Use Stripe.Subscription.update/2 with `cancel_at_period_end: true`"
Copy link
Collaborator

Choose a reason for hiding this comment

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

馃帀

@deprecated "Use Stripe.Subscription.update/2 with `cancel_at_period_end: true`"
@spec delete(Stripe.id() | t, %{at_period_end: true}) :: {:ok, t} | {:error, Stripe.Error.t()}
def delete(id, %{at_period_end: true}),
do: update(id, %{cancel_at_period_end: true}, [])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could leave off the [] if you wanted. Up to you though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Good point.

@mcrumm
Copy link
Contributor Author

mcrumm commented Sep 7, 2018

@snewcomer Okay, ready for another look!

Copy link
Collaborator

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

One last spot.

@@ -137,7 +136,6 @@ defmodule Stripe.Subscription do
optional(:metadata) => Stripe.Types.metadata(),
optional(:prorate) => boolean,
optional(:proration_date) => Stripe.timestamp(),
optional(:source) => Stripe.id() | Stripe.Source.t(),
optional(:tax_percent) => float,
optional(:trial_end) => Stripe.timestamp(),
optional(:trial_from_plan) => boolean
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mcrumm A couple more spots to remove :source, specifically in the cast_to_id helper function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Drat, thanks for catching that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snewcomer Should be good to go now

Copy link
Collaborator

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

This looks great!

@snewcomer snewcomer merged commit 75158e2 into beam-community:master Sep 10, 2018
@mcrumm mcrumm deleted the stripe-api-2018-08-23 branch September 11, 2018 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants