updated API Blueprint with stripecard #386

amyschools · 2016-10-24T18:04:15Z

Closes #385

Added endpoint and data structures for StripeCard.

joshsmith

Below are some questions and some suggested changes.

Did you try previewing the generated API Blueprint (at localhost:8081? I'm guessing it didn't render correctly due to syntax errors.

joshsmith · 2016-10-24T19:29:19Z

blueprint/api.apib

@@ -1533,7 +1533,6 @@ This resource identifies a relationship between a User and a Skill. For example,
    + Attributes (Record Not Found Response)

 # Group Users
-


Looks like accidental removal of this line.

joshsmith · 2016-10-24T19:29:36Z

blueprint/api.apib

@@ -1690,6 +1689,50 @@ This endpoint allows you to check whether a username is valid (by running a vali

    + Attributes (Unprocessable Entity Response)

+#StripeCard


You should have a space between # and StripeCard.

Also, it looks like this breaks the alpha ordering convention.

You also need to prefix with Group.

joshsmith · 2016-10-24T19:29:54Z

blueprint/api.apib

@@ -1690,6 +1689,50 @@ This endpoint allows you to check whether a username is valid (by running a vali

    + Attributes (Unprocessable Entity Response)

+#StripeCard
+
+This resource identifies the relationship between a StripeCustomer and a StripeCard. A StripeCustomer has many StripeCards and StripeCard belongs to a StripeCustomer.


I wouldn't call this a relationship. This on its own is a resource.

joshsmith · 2016-10-24T19:31:26Z

blueprint/api.apib

+
+This resource identifies the relationship between a StripeCustomer and a StripeCard. A StripeCustomer has many StripeCards and StripeCard belongs to a StripeCustomer.
+
+## StripeCards [/stripe_cards]


This should be dasherized.

joshsmith · 2016-10-24T19:32:00Z

blueprint/api.apib

+    + Parameters
+
+        + id (number) - Id of StripeCard.
+        +stripe_customer_id(number) - ID of StripeCustomer.


You don't need the stripe_customer_id to make a request to this endpoint.

joshsmith · 2016-10-24T19:32:24Z

blueprint/api.apib

+
+    + Response 404 (application/vnd.api+json; charset=utf-8)
+
+        + Attributes (Record Not Found Response)


I would also add in the 401 and 403 response since we know that this is authenticated and authorized.

joshsmith · 2016-10-24T19:33:09Z

blueprint/api.apib

+##StripeCards (object)
+ data(StripeCards Resource)
+ include JSON API Version
+


Where is the StripeCard Resource defined?

joshsmith · 2016-10-24T19:34:10Z

blueprint/api.apib

+ include JSON API Version
+
+##StripeCards (object)
+ data(StripeCards Resource)


I think you need a StripeCard Response and a StripeCards Response. Look at Categories as an example here.

joshsmith · 2016-10-24T19:34:33Z

blueprint/api.apib

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


Where is StripeCards Response defined?

coveralls · 2016-10-24T23:42:58Z

Coverage increased (+0.2%) to 95.517% when pulling d438f21 on 385-apiblueprint-stripecard into da5867b on develop.

joshsmith

Some of these changes should be implemented inside of this PR.

Other comments, though, are simply suggestions for refactorings that could be placed in future issue(s).

joshsmith · 2016-10-24T23:55:35Z

blueprint/api.apib

+
+## Stripe Cards [/stripe-cards]
+
+### List all Stripe Cards [GET]


Would change Cards to cards here.

joshsmith · 2016-10-24T23:56:03Z

blueprint/api.apib

+
+## Stripe Card [/stripe-cards/{id}]
+
+### Retrieve a Stripe Card [GET]


Also would change to lowercase card.

joshsmith · 2016-10-24T23:57:00Z

blueprint/api.apib

+
+ Parameters
+
+    + id (number) - Id of a Stripe card.


It seems like we're inconsistent in other endpoints about whether or not we have an id parameter on GET requests for the show actions. I'm not sure if we should open another issue for this, but we might want to consider it to make everything consistent across our entire documentation.

joshsmith · 2016-10-24T23:58:23Z

blueprint/api.apib

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


We might want to consider opening an issue about the security implications of returning a 401 or 403 vs just a 404 to say that the object doesn't exist, so people don't sneak around trying to find extra information about cards in our API somehow.

Added an issue for this: #392

I wasn't sure how to reference this comment specifically.

joshsmith · 2016-10-24T23:59:36Z

blueprint/api.apib

+        + `pass`
+        + `fail`
+        + `unavailable`
+        + `unchecked`


It looks like there's no default value here. Not sure we should specify, but I guess if we don't the assumption should be that there is no default.

joshsmith · 2016-10-24T23:59:54Z

blueprint/api.apib

+        + `unavailable`
+        + `unchecked`
+ exp_month: `12` (number)
+ exp_year: `2017` (number)


Is this an integer in #377 inside the database schema?

I also don't think this value needs backticks around it if it's just an integer. Not sure, but we should check some of the other examples.

joshsmith · 2016-10-25T00:02:09Z

blueprint/api.apib

+ attributes(Stripe Card Attributes)
+ relationships(Stripe Card Relationships)
+    + `project-categories`
+        + data(array[Project Category Resource Identifier])


We definitely missed this project-categories inside of here.

joshsmith · 2016-10-25T00:04:09Z

blueprint/api.apib

+
+## Stripe Customer Resource Identifier (object)
+ id: `1` (string, required)
+ type: `stripe-customer`


I think there's some inconsistency across our responses here. id and type should both be required.

Given some of the inconsistencies we're identifying here that stem from this blueprint not being significantly refactored after the changeover from Rails, I think we should open a parent journey issue that contains all of these suggestions for refactoring/fixes. Then we can break off into individual issues where needed.

Added issue for this: #393

coveralls · 2016-10-25T16:52:49Z

Coverage increased (+0.3%) to 95.517% when pulling 743147d on 385-apiblueprint-stripecard into 1b42ff1 on develop.

coveralls · 2016-10-25T16:54:12Z

Coverage remained the same at 95.205% when pulling 743147d on 385-apiblueprint-stripecard into 1b42ff1 on develop.

coveralls · 2016-10-26T19:28:32Z

Coverage decreased (-0.5%) to 95.017% when pulling 6cdf336 on 385-apiblueprint-stripecard into f7f764f on develop.

joshsmith · 2016-10-26T19:30:01Z

Ignore coveralls here. I don't know why it's saying test coverage went down. Nonsense.

joshsmith · 2016-10-26T19:31:47Z

I haven't checked if this renders. Does this still render correctly for you?

Good work! 🎉

joshsmith · 2016-10-26T19:32:58Z

Before you can merge, you'll need to rebase and squash your commits into one.

Your reworded commit message should probably read something like:

Add API Blueprint documentation for StripeCard

joshsmith

Just noticed something I'd missed here previously.

joshsmith · 2016-10-26T19:38:00Z

blueprint/api.apib

+ exp_year: 2017 (number)
+ id_from_stripe: `card_18Emd52eZvKYlo2C1s9DYdv8` (required, string)
+ last4: `4242` (string)
+ name: `John Doe` (string) - Cardholder name


I think this is missing inserted-at and updated-at timestamps, which actually all objects in our database will have.

amyschools · 2016-10-26T21:27:50Z

It was rendering earlier when i tried to push it, but won't anymore - I think it might be because of the stripe auth stuff that was added when we rebased?

joshsmith · 2016-10-26T21:32:06Z

blueprint/api.apib

@@ -2293,6 +2293,8 @@ This endpoint allows you to check whether a username is valid (by running a vali
 + id_from_stripe: `card_18Emd52eZvKYlo2C1s9DYdv8` (required, string)
 + last4: `4242` (string)
 + name: `John Doe` (string) - Cardholder name
+ `inserted-at`: `2014-07-07T18:01:26.000Z` (string)
+ `updated-at`: `2014-07-07T18:01:26.000Z` (string)


Watch the alpha ordering here.

joshsmith · 2016-10-26T21:32:33Z

blueprint/api.apib

@@ -1961,7 +2021,6 @@ This endpoint allows you to check whether a username is valid (by running a vali
 ## Preview Resource Identifier (object)
 + id: `1` (string, required)
 + type: `preview` (string, required)
-


Looks like accidental deletion here

joshsmith · 2016-10-26T21:32:57Z

blueprint/api.apib

@@ -2209,6 +2268,59 @@ This endpoint allows you to check whether a username is valid (by running a vali
 + data(Stripe Auth Resource)
 + include JSON API Version

+## StripeCard (object)
+ data(StripeCard Resource)
+ include JSON API Version


I think this is why rendering is failing. Probably should be Stripe Card and not StripeCard.

Actually, no. This shouldn't even exist. Not sure what this is doing here.

This is still why it's failing. Trying to hit a StripeCard Resource that doesn't exist.

joshsmith · 2016-10-26T21:34:51Z

blueprint/api.apib

+        + `unchecked`
+ exp_month: 12 (number)
+ exp_year: 2017 (number)
+ id_from_stripe: `card_18Emd52eZvKYlo2C1s9DYdv8` (required, string)


I actually think these attributes should be dasherized, and surrounded by ` marks.

coveralls · 2016-10-26T21:34:56Z

Coverage remained the same at 95.515% when pulling 16ad395 on 385-apiblueprint-stripecard into f7f764f on develop.

joshsmith · 2016-10-26T21:35:32Z

blueprint/api.apib

+
+This resource identifies a card created on the [Stripe API](https://stripe.com/docs/api#cards).
+
+A Stripe card belongs to a Stripe customer.


I think we changed this now in our system to say that it belongs to a user. Belonging to a customer (for us) is some unnecessary complexity.

coveralls · 2016-10-26T22:10:40Z

Coverage decreased (-0.5%) to 95.017% when pulling 23848c7 on 385-apiblueprint-stripecard into f7f764f on develop.

joshsmith · 2016-10-26T22:11:41Z

Squash and rebase.

coveralls · 2016-10-26T23:32:15Z

Coverage remained the same at 95.017% when pulling ab53c82 on 385-apiblueprint-stripecard into 36236be on develop.

joshsmith · 2016-10-27T00:06:40Z

blueprint/api.apib

@@ -2511,6 +2571,59 @@ 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 Card (object)


We'll need to put these above Stripe plans.

is it not showing up in alpha order from the most recent commit?

It didn't show up for me randomly.

coveralls · 2016-10-27T00:09:37Z

Coverage increased (+0.5%) to 95.515% when pulling 4d70a80 on 385-apiblueprint-stripecard into 36236be on develop.

joshsmith

Looks pretty good, a couple notes here.

joshsmith · 2016-10-27T00:07:28Z

blueprint/api.apib

@@ -2714,6 +2714,8 @@ This endpoint allows you to check whether a username is valid (by running a vali
    + data(array[User Role Resource Identifier])
 + `user-skills` (object)
    + data(array[User Skill Resource Identifier])
+ `stripe-cards` (object)
+    + data(array[Stripe Card Resource Identifier])


Watch alpha order here.

joshsmith · 2016-10-27T00:08:12Z

blueprint/api.apib

@@ -2624,6 +2597,33 @@ This endpoint allows you to check whether a username is valid (by running a vali
 + id: `1` (string, required)
 + type: `stripe-customer`

+## Stripe Plan Attributes (object)
+ amount: 999 (number) - Describes the amount of a plan in cents.
+ created: `2016-07-08T03:03:51.967Z` (string)


I would write that this is the created time from Stripe. Did we do this in #383?

From 383:
+ *created- A timestamp, indicating when the plan was created by Stripe
I'll add the comment into this also.

joshsmith · 2016-10-27T00:10:42Z

Didn't mean to approve that one. It should have been requested changes.

joshsmith · 2016-10-27T00:11:48Z

And that one comment should not be showing as outdated, so make sure you expand that.

coveralls · 2016-10-27T00:38:37Z

Coverage remained the same at 95.017% when pulling cf91a38 on 385-apiblueprint-stripecard into 36236be on develop.

joshsmith · 2016-10-27T04:13:25Z

@amyschools looks great, go ahead and squash again!

coveralls · 2016-10-27T16:54:27Z

Coverage remained the same at 95.082% when pulling c4204c0 on 385-apiblueprint-stripecard into a98bf75 on develop.

amyschools added the in progress label Oct 24, 2016

joshsmith reviewed Oct 24, 2016

View reviewed changes

joshsmith requested changes Oct 25, 2016

View reviewed changes

amyschools changed the title ~~updated api.apib with stripecard~~ updated API Blueprint with stripecard Oct 25, 2016

amyschools self-assigned this Oct 25, 2016

This was referenced Oct 25, 2016

Consider security implications of rendering 404 versus 401 or 403 error #392

Closed

Refactor API Blueprint documentation #393

Closed

amyschools force-pushed the 385-apiblueprint-stripecard branch from 743147d to 6cdf336 Compare October 26, 2016 19:24

joshsmith added the ready to merge label Oct 26, 2016

joshsmith approved these changes Oct 26, 2016

View reviewed changes

joshsmith requested changes Oct 26, 2016

View reviewed changes

joshsmith reviewed Oct 26, 2016

View reviewed changes

joshsmith approved these changes Oct 26, 2016

View reviewed changes

amyschools force-pushed the 385-apiblueprint-stripecard branch from 23848c7 to ab53c82 Compare October 26, 2016 23:28

joshsmith reviewed Oct 27, 2016

View reviewed changes

joshsmith removed the ready to merge label Oct 27, 2016

joshsmith approved these changes Oct 27, 2016

View reviewed changes

joshsmith added the needs work label Oct 27, 2016

joshsmith added ready to merge and removed needs work labels Oct 27, 2016

Add API Blueprint documentation for Stripe Card

c4204c0

amyschools force-pushed the 385-apiblueprint-stripecard branch from cf91a38 to c4204c0 Compare October 27, 2016 16:51

amyschools merged commit 98b3bbd into develop Oct 27, 2016

amyschools deleted the 385-apiblueprint-stripecard branch October 27, 2016 16:55

amyschools removed in progress ready to merge labels Oct 27, 2016

		@@ -1533,7 +1533,6 @@ This resource identifies a relationship between a User and a Skill. For example,
		+ Attributes (Record Not Found Response)

		# Group Users

		@@ -1690,6 +1689,50 @@ This endpoint allows you to check whether a username is valid (by running a vali

		+ Attributes (Unprocessable Entity Response)

		#StripeCard


		This resource identifies the relationship between a StripeCustomer and a StripeCard. A StripeCustomer has many StripeCards and StripeCard belongs to a StripeCustomer.

		## StripeCards [/stripe_cards]


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

		+ Attributes (Record Not Found Response)


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

		+ Attributes (StripeCards Response)


		## Stripe Cards [/stripe-cards]

		### List all Stripe Cards [GET]


		## Stripe Card [/stripe-cards/{id}]

		### Retrieve a Stripe Card [GET]


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

		+ Attributes (Forbidden Response)


		This resource identifies a card created on the [Stripe API](https://stripe.com/docs/api#cards).

		A Stripe card belongs to a Stripe customer.

updated API Blueprint with stripecard #386

updated API Blueprint with stripecard #386

Conversation

amyschools commented Oct 24, 2016 • edited by joshsmith Loading

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 24, 2016

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 25, 2016

coveralls commented Oct 25, 2016

coveralls commented Oct 26, 2016

joshsmith commented Oct 26, 2016

joshsmith commented Oct 26, 2016

joshsmith commented Oct 26, 2016

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyschools commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 26, 2016

Choose a reason for hiding this comment

coveralls commented Oct 26, 2016

joshsmith commented Oct 26, 2016

coveralls commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 27, 2016

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Oct 27, 2016

joshsmith commented Oct 27, 2016

coveralls commented Oct 27, 2016

joshsmith commented Oct 27, 2016

coveralls commented Oct 27, 2016

amyschools commented Oct 24, 2016 •

edited by joshsmith

Loading