-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add Stripe.Card #118
Add Stripe.Card #118
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.
Looks pretty good, just some comments.
:address_zip, :address_zip_check, :brand, :country, | ||
:customer, :cvc_check, :dynamic_last4, :exp_month, :exp_year, | ||
:fingerprint, :funding, :last4, :metadata, :name, :recipient, | ||
:tokenization_method, :number |
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.
Should fix alpha ordering here.
@valid_create_keys [ | ||
:exp_month, :exp_year, :number, :address_city, :address_country, | ||
:address_line1, :address_line2, :address_state, :address_zip, | ||
:cvc, :metadata, :name |
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.
Should fix alpha ordering here.
@valid_update_keys [ | ||
:exp_month, :exp_year, :address_city, :address_country, | ||
:address_line1, :address_line2, :address_state, :address_zip, | ||
:metadata, :name |
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.
Should fix alpha ordering here.
] | ||
|
||
@customer_endpoint "customers" | ||
@recipient_endpoint "recipients" |
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 we should remove these?
@joshsmith - all comments should be addressed now |
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.
Couple minor comments. I like changing the create to be token based to make sure no sensitive data is sent to a server.
:id, :address_city, :address_country, :address_line1, | ||
:address_line1_check, :address_line2, :address_state, | ||
:address_zip, :address_zip_check, :brand, :country, | ||
:customer, :cvc_check, :dynamic_last4, :exp_month, :exp_year, |
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.
Can we remove the cvc_check
since we don't have cvc anymore?
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.
No, that's returned from the API, but not created.
:address_line1_check, :address_line2, :address_state, | ||
:address_zip, :address_zip_check, :brand, :country, | ||
:customer, :cvc_check, :dynamic_last4, :exp_month, :exp_year, | ||
:fingerprint, :funding, :last4, :metadata, :name, :number, |
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.
We should get rid of number
@green-arrow updated to fix comments. |
ca693b1
to
3b796b2
Compare
Closes #109.
Adds create, retrieve, update, and delete capability for a Stripe.Card.
Also adds in a convenience method for translating a stripe API response to a given struct. Additional manipulation may need to be performed to get dates in the proper format as well as for nested structs, but this will take care of most of the heavy lifting.