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

API Blueprint Documentation for Stripe Subscription #402

Merged
merged 1 commit into from
Oct 27, 2016

Conversation

mackenziehicks
Copy link
Contributor

What's in this PR?

Added endpoints( index, show, create, edit ) and data structures for the Stripe Subscriptions to the API Blueprint.

References

Fixes #391

Progress on: #245

@mackenziehicks mackenziehicks changed the title 391 apiblueprint stripesubscription API Blueprint Documentation for Stripe Subscription Oct 27, 2016
+ Response 200 (application/vnd.api+json; charset=utf-8)

+ Attributes (Stripe Subscription Response)

Copy link
Contributor

Choose a reason for hiding this comment

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

If user should be authorized, do you need 401/403 repsonses?

+ Attributes (Record Not Found Response)

### Update a Stripe subscription [PATCH/subscriptions/{id}]

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be a space between PATCH and /subscriptions.

+ `current-period-start`: `2016-07-08T03:03:51.967Z` (string)
+ `customer-id-from-stripe`: 'cus_9RnUvtsb41mD1g' (string)
+ `ended-at`: null(string)
+ `id-from-stripe`: `sub_9RnVc9Cd03wV8X` (string, required)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment that this is the subscription id assigned by Stripe.

+ `cancelled-at`: null (string)
+ created: `2016-07-08T03:03:51.967Z` (string)
+ `current-period-end`: `2016-07-08T03:03:51.967Z` (string)
+ `current-period-start`: `2016-07-08T03:03:51.967Z` (string)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this different from start below?

+ type: `stripe-subscription` (string, required)

##Stripe Subscription Resource (object)
+ include Stripe Subscription Resource Identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

Spacing after ## here and at 2721


+ Response 200 (application/vnd.api+json; charset=utf-8)

+ Attributes (Stripe Subscription Response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Plural Stripe Subscriptions?


+ Response 200 (application/vnd.api+json; charset=utf-8)

+ Attributes (Stripe Subscription Response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be plural Stripe Subscriptions?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.082% when pulling 7693d63 on 391-apiblueprint-stripesubscription into 98b3bbd on develop.

Copy link
Contributor

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

Lots of minor comments/suggestions here:

@@ -1407,7 +1407,15 @@ Stripe Subscriptions allow us to charge a customer's card on a recurring basis.

+ Response 200 (application/vnd.api+json; charset=utf-8)

+ Attributes (Stripe Subscription Response)
+ Attributes (Stripe Subscriptions Response)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this got unindented somehow.


+ Response 403 (application/vnd.api+json; charset=utf-8)

+ Attributes (Forbidden Response)


Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove one line here.


This endpoint is for creating, editing and returning Stripe Subscriptions.

Stripe Subscriptions allow us to charge a customer's card on a recurring basis. A subscription ties a customer to a particular plan.
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer lowercase "subscriptions" in the descriptions here.

I would also state that our default here is monthly, that we're using $0.01 cent plans, and doing some X multiplier on top of that to set the actual amount.

I would also link to the Stripe docs from here.


+ Response 403 (application/vnd.api+json; charset=utf-8)

+ Attributes (Forbidden Response)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the list all per some recent conversation about just having filters instead.

Accept: application/vnd.api+json
Authorization: Bearer {token}


Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line in here.

@@ -2565,6 +2712,47 @@ This endpoint allows you to check whether a username is valid (by running a vali
+ data(array[Stripe Plan Resource], required)
+ include JSON API Version

## Stripe Subscription Attributes (object)
+ `cancelled-at`: null (string) - If the subscription has been canceled, the date of that cancellation.
+ created: `2016-07-08T03:03:51.967Z` (string)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add the description we have for created elsewhere, specifying this is the date on Stripe's system.

+ `cancelled-at`: null (string) - If the subscription has been canceled, the date of that cancellation.
+ created: `2016-07-08T03:03:51.967Z` (string)
+ `current-period-end`: `2016-07-08T03:03:51.967Z` (string) - End of the current period that the subscription has been invoiced for. At the end of this period, a new invoice will be created.
+ `current-period-start`: `2016-07-08T03:03:51.967Z` (string) - Start of the current period that the subscription has been invoiced for
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this an actual month distance for clarity's sake.

+ `active`
+ `past_due`
+ `canceled`
+ `unpaid`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think prefer alpha ordering here. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the order listed on the Stripe API, I'm thinking because of the order that the status is likely to have. But could do Alpha instead.

+ `stripe-plan`
+ data(Stripe Plan Resource Identifier)
+ `stripe-customer`
+ data(Stripe Customer Resource Identifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

Alpha ordering.

+ include JSON API Version

## Stripe Subscriptions Response
+ data(array[Stripe Subscription Resource], required)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the required isn't necessary.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 95.082% when pulling 0c3bd59 on 391-apiblueprint-stripesubscription into 98b3bbd on develop.

@joshsmith
Copy link
Contributor

Rebase and squash!

added filter to 'list all Stripe subscriptions'

added list all subscriptions, changed title of filter

changes from 1st code review

fixes after 2nd review
@mackenziehicks mackenziehicks force-pushed the 391-apiblueprint-stripesubscription branch from 0c3bd59 to e32c002 Compare October 27, 2016 23:23
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.082% when pulling e32c002 on 391-apiblueprint-stripesubscription into 3581955 on develop.

@mackenziehicks mackenziehicks merged commit 9e3302f into develop Oct 27, 2016
@mackenziehicks mackenziehicks deleted the 391-apiblueprint-stripesubscription branch October 27, 2016 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants