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

Update Stripe version #483

Merged
merged 20 commits into from Jun 7, 2019
Merged

Conversation

snewcomer
Copy link
Collaborator

@snewcomer snewcomer commented Apr 27, 2019

@coveralls
Copy link

coveralls commented Apr 27, 2019

Coverage Status

Coverage increased (+0.2%) to 87.403% when pulling 9f81847 on snewcomer:sn/update-deps-2 into 41d68bf on code-corps:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.185% when pulling ffe75ce on snewcomer:sn/update-deps-2 into 6d51205 on code-corps:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.185% when pulling ffe75ce on snewcomer:sn/update-deps-2 into 6d51205 on code-corps:master.

@dnsbty dnsbty mentioned this pull request May 22, 2019
@@ -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,
Copy link
Collaborator Author

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,
Copy link
Collaborator Author

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,
Copy link
Collaborator Author

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 :: %{
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

major

@snewcomer
Copy link
Collaborator Author

@maartenvanvliet Mind providing a quick review here? Will release 2.3, then merge this, and release 2.4. Then begin #504

Copy link
Member

@maartenvanvliet maartenvanvliet left a 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,
Copy link
Member

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

Copy link
Collaborator Author

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

Copy link
Member

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.

Copy link
Member

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.

@@ -105,14 +112,14 @@ defmodule Stripe.Charge do
:amount,
Copy link
Member

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?

Copy link
Collaborator Author

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 [
Copy link
Member

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

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Member

@maartenvanvliet maartenvanvliet left a comment

Choose a reason for hiding this comment

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

Looks good!

@snewcomer
Copy link
Collaborator Author

@dnsbty Merging with your work!

@snewcomer snewcomer merged commit fa21dd8 into beam-community:master Jun 7, 2019
@snewcomer snewcomer deleted the sn/update-deps-2 branch June 7, 2019 14:09
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

4 participants