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 new fields to lastResponse: apiVersion, stripeAccount, idempotencyKey #952

Merged
merged 2 commits into from
Jul 16, 2020

Conversation

polybuildr
Copy link
Contributor

r? @jlomas-stripe
cc @stripe/api-libraries @brandur-stripe

If any of these three fields are set in the headers, copy them over to lastResponse as suggested in #406.
Also add tests for the new fields and refactor an existing test for lastResponse.

They are set from the headers only if the relevant header is present.
Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

Hi @polybuildr -- thanks for the contribution!

This looks good. One request: can we avoid adding the new dev dependency on uuid in the test? Hardcoding an identifier would be better.

@polybuildr
Copy link
Contributor Author

Hi @richardm-stripe! I'm glad you brought that up! I started with hardcoding the key but then came across a problem. The test isn't hermetic and actually communicates with the Stripe API, so if I hardcode an idempotency key, running the test twice gives me an error saying: No such customer: cus_HdkOjCApDMTkzx.

I'm not sure why it's that error specifically -- I didn't investigate. I can look into it later if you don't already know off the top of your head.

@richardm-stripe
Copy link
Contributor

Ah thanks @polybuildr, I forgot about that. Can we roll our own random identifier instead of introducing a dependency?
(Math.random() * Number.MAX_SAFE_INTEGER).toString(16) or something?

@polybuildr
Copy link
Contributor Author

Ah yes, that's a reasonably good solution. I'll update to include a random identifier without using a dependency. :)

Also refactor existing test for some lastResponse fields.
@polybuildr
Copy link
Contributor Author

Done! I know it doesn't matter, but I was still uncomfortable using Math.random so I used the Node crypto module's randomBytes. No additional dependencies, so that should still be okay. :)

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

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

Thank you, this looks great. We are very excited about and appreciative of the contributions you are making to this repo & stripe-cli!

@richardm-stripe richardm-stripe merged commit 1b84a26 into stripe:master Jul 16, 2020
@richardm-stripe
Copy link
Contributor

Released in 8.74.0

@polybuildr
Copy link
Contributor Author

Thanks for the review! Glad to be of help. :)

gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
…yKey (stripe#952)

* Add stripeAccount, apiVersion and idempotencyKey to lastResponse
gurus00 pushed a commit to gurus00/stripe-node that referenced this pull request Feb 11, 2025
…yKey (stripe#952)

* Add stripeAccount, apiVersion and idempotencyKey to lastResponse
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.

3 participants