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 support for the Checkout Session resource #1440

Merged
merged 1 commit into from
Apr 17, 2019
Merged

Add support for the Checkout Session resource #1440

merged 1 commit into from
Apr 17, 2019

Conversation

remi-stripe
Copy link
Contributor

@remi-stripe remi-stripe commented Dec 20, 2018 •
edited
Loading

This resource is now ready to be released. The PR has been updated to match the latest spec.

r? @ob-stripe
cc @stripe/api-libraries

@ob-stripe ob-stripe mentioned this pull request Jan 3, 2019
Closed
@remi-stripe remi-stripe force-pushed the remi-add-checkout-sessions branch 2 times, most recently from 1209c51 to 38c0c25 Compare January 17, 2019 03:58
@remi-stripe
Copy link
Contributor Author

Updated most of the PR to match the new resource shape. Still a few things to fix in tests

@remi-stripe remi-stripe changed the title [WIP] Add support for the Checkout Session resource Add support for the Checkout Session resource Apr 15, 2019
@remi-stripe
Copy link
Contributor Author

cc @matt-stripe for awareness

@ob-stripe ob-stripe assigned remi-stripe and unassigned ob-stripe Apr 15, 2019
@remi-stripe
Copy link
Contributor Author

@ob-stripe Fixed what you found. PTAL

@remi-stripe remi-stripe assigned ob-stripe and unassigned remi-stripe Apr 15, 2019
ob-stripe
ob-stripe reviewed Apr 15, 2019
View reviewed changes
src/Stripe.net/Services/Checkout/SessionCreateOptions.cs Outdated
/// <c>LineItems</c>. Usage with <c>SubscriptionData</c> is not yet available.
/// </summary>
[JsonProperty("customer")]
public string CustomerId { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we're super consistent w/ regards to ordering the attributes alphabetically per attribute name (CustomerId) or per JSON name (customer), but in Session, CustomerId appears after CustomerEmail. Can you quickly check if there's a common style we use for this and update the PR to be consistent?

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's no style beyond "remi does not know his alphabet". Will fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha okay I know why. It's because if something is expandable then it ends up appearing first. But I'd rather re-alphabetize when it becomes expandable.

ob-stripe
ob-stripe suggested changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

Just one more minor nit.

@remi-stripe remi-stripe mentioned this pull request Apr 15, 2019
Merged
@remi-stripe
Copy link
Contributor Author

Okay re-assigning to @mickjermsurawong-stripe to triple check based on the work we will do on stripe-java

@remi-stripe
Copy link
Contributor Author

Okay so now customer, payment_intent and subscription are expandable. So I had to change the PR again.

Also we will need a new release of stripe-mock to test this properly.

@remi-stripe
Copy link
Contributor Author

@ob-stripe Mick released a new stripe-mock that allows for expanding the right resource and I updated tests.

Can you have one last deep look so that we can release?

@ob-stripe ob-stripe assigned remi-stripe and unassigned ob-stripe Apr 17, 2019
@remi-stripe remi-stripe merged commit d2b4d43 into master Apr 17, 2019
@remi-stripe remi-stripe deleted the remi-add-checkout-sessions branch April 17, 2019 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ob-stripe ob-stripe ob-stripe approved these changes

Assignees

@remi-stripe remi-stripe

Labels
approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@remi-stripe @ob-stripe @mickjermsurawong-stripe