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

Add Stripe.Token.create #200

Merged
merged 1 commit into from Feb 11, 2017

Conversation

sylver
Copy link
Contributor

@sylver sylver commented Jan 27, 2017

This function is meant to be only used in Unit Tests to simulate a token provided by the client.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 26.147% when pulling 5e85cb8 on sylv3r:feature/add-stripe-token-create into adb8603 on code-corps:2.0.

@joshsmith
Copy link
Contributor

We can perhaps handle this with some upcoming testing work we're planning on doing. I'm not sure it's a good idea to implement a public API for doing something that only helps with testing.

@sylver
Copy link
Contributor Author

sylver commented Jan 28, 2017

I was also wondering if this is something to actually do or not.

Here's my thoughts :
Stripe implements it in its API, all other languages libraries implement it too.
I think the goal of a library like this one is to reflect nearly perfectly the REST API it is based on, it doesn't have to make choice on what is best or not to the end user.
Since Stripe implements it, someone could want to use it, this is the responsibility of a library to be exhaustive and not to take position on whether implements this or that.

@sylver
Copy link
Contributor Author

sylver commented Jan 31, 2017

@joshsmith @begedin, any thoughts about this one ?

@joshsmith
Copy link
Contributor

I think if we do it, it should be done as a create and we note it in the documentation.

@sylver sylver force-pushed the feature/add-stripe-token-create branch from 5e85cb8 to 4f4b48f Compare February 1, 2017 17:56
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 26.027% when pulling 4f4b48f on sylv3r:feature/add-stripe-token-create into fab5b57 on code-corps:2.0.

@sylver
Copy link
Contributor Author

sylver commented Feb 1, 2017

Right @joshsmith, I just updated the PR accordingly.


You MUST pass in your Publishable Key in `opts` :

Stripe.Token.create(%{card: card_map}, api_key: "STRIPE_PUBLISHABLE_KEY")
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to pass in your publishable key for this to work?

Copy link
Contributor Author

@sylver sylver Feb 1, 2017

Choose a reason for hiding this comment

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

That was a false assumption of mine, since it has to be with the publishable key on the client side, I supposed we needed it too on direct API requests for creating a token (it works with both, though). In the API Docs they are giving an example with a secret key as a parameter.
https://stripe.com/docs/api#create_card_token

I will remove the requirement for the publishable key as an argument in the doc section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joshsmith just updated the doc section.

Mirroring Stripe Public Token API

This function is meant to be only used in Unit Tests to simulate
a token provided by the client.
@sylver sylver force-pushed the feature/add-stripe-token-create branch from 4f4b48f to 950ddda Compare February 1, 2017 22:07
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 26.027% when pulling 950ddda on sylv3r:feature/add-stripe-token-create into fab5b57 on code-corps:2.0.

@sylver sylver changed the title Add Stripe.Token.create_unsecure Add Stripe.Token.create Feb 1, 2017
@asummers
Copy link
Contributor

This is fine enough for me 👍

@asummers asummers merged commit 4501a25 into beam-community:2.0 Feb 11, 2017
asummers added a commit to asummers/stripity_stripe that referenced this pull request Feb 11, 2017
Fixed a compile error introduced by merging beam-community#197 and beam-community#200 in parallel.
@asummers asummers mentioned this pull request Feb 11, 2017
@sylver
Copy link
Contributor Author

sylver commented Feb 12, 2017

Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants