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.

Sign up for GitHub

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

Jump to bottom

Add Stripe Issuing #493

Merged
merged 5 commits into from May 28, 2019
Merged

Add Stripe Issuing #493

merged 5 commits into from May 28, 2019

Conversation

jcartwright
Copy link
Contributor

@jcartwright jcartwright commented May 9, 2019 •
edited by snewcomer

Adding the Stripe Issuing objects:

  • Issuing.Authorization
  • Issuing.Card
  • Issuing.CardDetails
  • Issuing.Cardholder
  • Issuing.Dispute
  • Issuing.Transaction

https://stripe.com/docs/api/issuing/authorizations
https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml

@coveralls
Copy link

coveralls commented May 9, 2019 •
edited

Coverage Status

Coverage increased (+1.5%) to 87.055% when pulling 9e64940 on jcartwright:add-stripe-issuing into 7c54802 on code-corps:master.

@jcartwright
Copy link
Contributor Author

@snewcomer here's my WIP merge request. I'm attempting to exercise the changes against the live API before I feel fully confident and would appreciate your eyes on the code as well.

@jcartwright jcartwright changed the title WIP: Add Stripe Issuing Add Stripe Issuing May 9, 2019
@jcartwright
Copy link
Contributor Author

I've exercised these changes against Stripe's test environment and things appear to work as expected. I think it's good to go.

Add support for Stripe.Issuing
c4a9764 
- Issuing.Authorization
- Issuing.Card
- Issuing.CardDetails
- Issuing.Cardholder
- Issuing.Dispute
- Issuing.Transaction
@jcartwright jcartwright mentioned this pull request May 10, 2019
Closed
snewcomer
snewcomer reviewed May 12, 2019
View reviewed changes
lib/stripe/types.ex Outdated
@@ -12,6 +12,30 @@ defmodule Stripe.Types do
state: String.t() | nil
}

@type authorization_controls :: %{
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we inlined this in a Stripe.Issuing.Types module? Unless you see this type used outside the Issuing scope?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I noticed that pattern in some other instances. I'm happy to move the issuing-specific types into a separate module or inline them where appropriate.

Copy link
Collaborator

@snewcomer snewcomer May 14, 2019 •
edited

Choose a reason for hiding this comment

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

Yeah that might be good if you don't mind

@jcartwright
Copy link
Contributor Author

@snewcomer I've moved the issuing types around per your suggestion. Let me know if there's anything else you see that I can improve. Thanks.

@jcartwright
Copy link
Contributor Author

@snewcomer any other feedback on this merge request? I'd love to get it merged before it gets stale.

@snewcomer
Copy link
Collaborator

@jcartwright Feel free to merge master and CI should be g2g!

@jcartwright
Copy link
Contributor Author

@snewcomer CI seems to be failing CI to unformatted files outside of my merge request

stripity_stripe add-stripe-issuing % mix format --dry-run --check-formatted
** (Mix) mix format failed due to --check-formatted.
The following files were not formatted:

  * /Users/jcartwright/apps/stripity_stripe/lib/stripe/radar/review.ex
  * /Users/jcartwright/apps/stripity_stripe/test/stripe/converter_test.exs
  * /Users/jcartwright/apps/stripity_stripe/test/stripe/connect/transfer_reversal_test.exs
  * /Users/jcartwright/apps/stripity_stripe/test/stripe/subscriptions/usage_test.exs
  * /Users/jcartwright/apps/stripity_stripe/test/support/stripe_case.ex

Do you want me to format them in a separate MR?

snewcomer
snewcomer reviewed May 21, 2019
View reviewed changes
lib/stripe/issuing/card_details.ex
"""
@spec retrieve(Stripe.id() | Stripe.Issuing.Card.t(), Stripe.options()) ::
{:ok, t} | {:error, Stripe.Error.t()}
def retrieve(id, opts \\ []) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the same as the retrieve endpoint in Stripe.Issuing.Card? Is this module necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CardDetails is a nested resource used to retrieve the full sensitive details for an issued card. I treated it like a separate and distinct resource because the struct is very different from the Card resource used to manage issued cards.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. In their example response, it looks like the response object contains most of what is in the Issuing.Card struct. Am I missing something?

https://stripe.com/docs/api/issuing/cards/retrieve_details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is some overlap, but the CardDetails has a Card child object and then the sensitive attributes for :cvc and :number. I'm open to suggestions if it makes more sense to define the CardDetails struct and a retrieve_card_details function in the Card module. I see them as distinct structs but concede that it might be unnecessary to have a separate module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcartwright Well I guess I am curious - if we limit the attributes to just these few, then won't we be discarding all of them that may come in the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the common attributes are in the CardDetails :card child element, so they are not discarded but simply nested.

Add required params
9e64940 
- add required params on Card.create, Cardholder.create, and Dispute.create functions
- add atom enums to typespecs and params
snewcomer
snewcomer approved these changes May 28, 2019
View reviewed changes
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 really good! Lots of great work in here. 💯

@snewcomer snewcomer merged commit d61b4bf into beam-community:master May 28, 2019
@jcartwright jcartwright deleted the add-stripe-issuing branch May 28, 2019 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snewcomer snewcomer snewcomer approved these changes

Assignees

@jcartwright jcartwright

Labels
2.0 enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jcartwright @coveralls @snewcomer