-
Notifications
You must be signed in to change notification settings - Fork 356
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
Update Stripe version #483
Conversation
1 similar comment
@@ -63,14 +70,14 @@ defmodule Stripe.Charge do | |||
amount: non_neg_integer, | |||
amount_refunded: non_neg_integer, | |||
application: Stripe.id() | nil, | |||
application_fee: Stripe.id() | Stripe.ApplicationFee.t() | nil, | |||
application_fee_amount: Stripe.id() | Stripe.ApplicationFee.t() | nil, |
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.
minor
@@ -56,6 +57,7 @@ defmodule Stripe.Invoice do | |||
starting_balance: integer, | |||
statement_descriptor: String.t() | nil, | |||
status: String.t() | nil, | |||
status_transitions: status_transitions() | nil, |
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.
minor
optional(:support_email) => String.t(), | ||
optional(:support_url) => String.t(), | ||
optional(:requested_capabilities) => capabilities, | ||
optional(:settings) => settings, |
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.
major
@@ -37,6 +37,13 @@ defmodule Stripe.Charge do | |||
predicate: String.t() | |||
} | |||
|
|||
@type billing_details :: %{ |
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.
major
@maartenvanvliet Mind providing a quick review here? Will release 2.3, then merge this, and release 2.4. Then begin #504 |
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 good!
Couple of remarks.
country: String.t(), | ||
debit_negative_balances: boolean, | ||
decline_charge_on: decline_charge_on, | ||
created: Stripe.timestamp() | nil, | ||
default_currency: String.t(), | ||
details_submitted: 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.
The deleted
key seems to be missing here
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.
Where do you see this? (just making sure we are both looking at the same documentation)
https://stripe.com/docs/api/accounts/object
https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml
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.
Hmm, I ran the tests against the stripe-mock and got this warning
[warn] Extra keys were received but ignored when converting Stripe.Account: [:deleted]
But it seems the deleted
is only added to the response in the test for the Account.delete/2
function.
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.
Seems Account.delete/2
returns the deleted account but that is no more than this in JSON
{
"id": "acct_1032D82eZvKYlo2C",
"object": "account",
"deleted": true
}
We may need a different pattern to deal with deletes of this kind.
lib/stripe/core_resources/charge.ex
Outdated
@@ -105,14 +112,14 @@ defmodule Stripe.Charge do | |||
:amount, |
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.
When I run against stripe-mock and turn on warnings I get
[warn] Extra keys were received but ignored when converting Stripe.Charge: [:application_fee, :payment_method, :payment_method_details]
The first one was changed here, but the api still returns it. Should it not still be included?
The other two are also missing, but might be for later PR?
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.
Ready for your re-review!
paid_at: Stripe.timestamp() | nil, | ||
voided_at: Stripe.timestamp() | nil | ||
}) | ||
|
||
defstruct [ |
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.
A lot of fields missing here: [warn] Extra keys were received but ignored when converting Stripe.Invoice: [:account_country, :account_name, :customer_address, :customer_email, :customer_name, :customer_phone, :customer_shipping, :customer_tax_exempt, :customer_tax_ids, :default_tax_rates, :payment_intent, :post_payment_credit_notes_amount, :pre_payment_credit_notes_amount, :total_tax_amounts]
But can be left for another PR
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.
Fixed! How did you generate these warning messages. Not Logger.warn
?
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.
Changed this line https://github.com/code-corps/stripity_stripe/blob/master/lib/stripe/converter.ex#L143 from Logger.debug()
to Logger.warn()
and run the tests.
When you do, it'll show a lot more warnings but those were unrelated to this PR.
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 good!
@dnsbty Merging with your work! |
https://stripe.com/docs/upgrades#api-changelog