second review #4

andrewjtech123 · 2016-11-07T21:30:53Z

use this branch for the second round of reviews

dominiquesr · 2016-11-07T21:35:43Z

reviews two/swagger.yaml

+      -----|-----------|-------------------------
+      `id`| `id` of the draft order |`string`
+      `order_id`  | The id of the order associated to the draft order, once created. | `string`        
+      `name` | Name of the draft order, format #D<number>, where number, is an sequential identifier unique to the shop, starting at 1. For example #D133 | `integer`


a sequential identifier

dominiquesr · 2016-11-07T21:47:26Z

reviews two/swagger.yaml

+      `id`| `id` of the draft order |`string`
+      `order_id`  | The id of the order associated to the draft order, once created. | `string`        
+      `name` | Name of the draft order, format #D<number>, where number, is an sequential identifier unique to the shop, starting at 1. For example #D133 | `integer`
+      `customer` | Customer object will be serialized with only the default address, however only the ID can be set in order to associate the customer to the draft order. Setting the value to null removes the customer from the draft order. | `object`


This is an internal description. Change to something like (my english isn't perfect so rewrite in any way!):

The customer associated with this draft order. The customer is read only, however a new customer can be specified by replacing the ID. Setting the value to null removes the customer from the draft order.

dominiquesr · 2016-11-07T21:49:34Z

reviews two/swagger.yaml

+      `shipping_address` | The mailing address to where the draft order will be shipped. | `string`
+      `billing_address` | The mailing address associated with the payment method. | `string`
+      `note` | The text of an optional note that a shop owner can attach to the draft order. | `string`
+      `email` | The email address used for sending notifications. |`string`


Can you add that is default's t the customer's email address, when not specified and a customer is present? Or is that too much info?

dominiquesr · 2016-11-07T21:52:18Z

reviews two/swagger.yaml

+      `currency` | The three letter code for the currency to be used for the payment. | `string`
+      `invoice_sent_at` | DateTime when the invoice was emailed to the customer by Shopify. | `dateTime`
+      `invoice_url` | The url to send to the customer so that they can complete the checkout.  When using `send_invoice`, this url is emailed to the customer. This field can be used so that an API client can use another method of communication to provide the url to the customer. | `string`
+      `line_item`[ ] | | array of `line_item` objects


Is it possible to expand these here, like we do below in the parameters for create, update?

You're also missing a description for these?

A list of line item objects, each one containing information about an item in the draft order.

dominiquesr · 2016-11-07T21:54:03Z

reviews two/swagger.yaml

+      `tax_exempt` | Sets whether taxes are exempt for this draft order. If this value is `false`, Shopify will honor the `tax_exempt` value for each `line_item`. | `boolean`
+      `tax_lines` | Tax lines describing the sum of each type of tax line for line items and shipping line. | array of `tax_line` objects
+      `discount` | Order level discount. | `string`
+      `taxes_included` | Shop settings taxes are included in the price | `boolean`


There's a word missing here or something. The shop settings state, or have or something that taxes are included in the price.

dominiquesr · 2016-11-07T21:54:38Z

reviews two/swagger.yaml

+      `billing_address` | The mailing address associated with the payment method. | `string`
+      `note` | The text of an optional note that a shop owner can attach to the draft order. | `string`
+      `email` | The email address used for sending notifications. |`string`
+      `currency` | The three letter code for the currency to be used for the payment. | `string`


This field can't be set and is also a shop setting, provided here for convenience. Might be helpful to add that.

dominiquesr · 2016-11-07T21:55:07Z

reviews two/swagger.yaml

+      `email` | The email address used for sending notifications. |`string`
+      `currency` | The three letter code for the currency to be used for the payment. | `string`
+      `invoice_sent_at` | DateTime when the invoice was emailed to the customer by Shopify. | `dateTime`
+      `invoice_url` | The url to send to the customer so that they can complete the checkout.  When using `send_invoice`, this url is emailed to the customer. This field can be used so that an API client can use another method of communication to provide the url to the customer. | `string`


Do you we use API client or app to refer to apps?

dominiquesr · 2016-11-07T21:58:29Z

reviews two/swagger.yaml

+      `invoice_sent_at` | DateTime when the invoice was emailed to the customer by Shopify. | `dateTime`
+      `invoice_url` | The url to send to the customer so that they can complete the checkout.  When using `send_invoice`, this url is emailed to the customer. This field can be used so that an API client can use another method of communication to provide the url to the customer. | `string`
+      `line_item`[ ] | | array of `line_item` objects
+      `metafields`[ ] | | array of `metafield` objects


You're missing a description. Perhaps:
Attaches additional information to a shop's resources.

dominiquesr · 2016-11-07T22:01:42Z

reviews two/swagger.yaml

+      `invoice_url` | The url to send to the customer so that they can complete the checkout.  When using `send_invoice`, this url is emailed to the customer. This field can be used so that an API client can use another method of communication to provide the url to the customer. | `string`
+      `line_item`[ ] | | array of `line_item` objects
+      `metafields`[ ] | | array of `metafield` objects
+      `shipping_line` | | `object`


Also missing description.
An object which details the shipping rate which has been selected for the draft order. The shipping line is automatically reset to null when the applied shipping rate is no longer valid.

dominiquesr · 2016-11-07T22:02:30Z

reviews two/swagger.yaml

+      `total_tax` | Total tax amount | `integer`
+      `completed_at` | Date at which an order was created and the draft order moved to “completed” status. | `DateTime`
+      `created_at` | By default, this auto-generated property is the date and time when the order was created in Shopify, in ISO 8601 format. If you are importing orders to the Shopify platform from another system, the writable `processed_at` property will override the `created_at` date. | `dateTime`
+      `updated_at` | | `dateTime`


The date and time when the draft order was last modified. The API returns this value in ISO 8601 format.

dominiquesr · 2016-11-07T22:02:55Z

reviews two/swagger.yaml

+      `taxes_included` | Shop settings taxes are included in the price | `boolean`
+      `total_tax` | Total tax amount | `integer`
+      `completed_at` | Date at which an order was created and the draft order moved to “completed” status. | `DateTime`
+      `created_at` | By default, this auto-generated property is the date and time when the order was created in Shopify, in ISO 8601 format. If you are importing orders to the Shopify platform from another system, the writable `processed_at` property will override the `created_at` date. | `dateTime`


This processed_at info isn't valid, there is no processed_at field on draft orders.

dominiquesr · 2016-11-07T22:05:16Z

reviews two/swagger.yaml

+          ### About line items
+          There are two ways to add a line item:
+
+          * Supply a `variant_id`, `quantity` and (if applicable) `discount`


Maybe start this off by saying add a line item from a product variant by...

dominiquesr · 2016-11-07T22:06:24Z

reviews two/swagger.yaml

+          There are two ways to add a line item:
+
+          * Supply a `variant_id`, `quantity` and (if applicable) `discount`
+          * Add a custom line item by supplying `title`, `price`, `taxable` and `quantity` (at a minimum). The array of line items on a draft order must not contain a `variant_id` more than once. The data copied over from a variant upon creation of a line item is never subsequently updated.


This text should be on the overall line item:

The array of line items on a draft order must not contain a variant_id more than once.

And this text should be on the line above since it talks about the variant line items, not custom line items:

The data copied over from a variant upon creation of a line item is never subsequently updated.

dominiquesr · 2016-11-07T22:07:39Z

reviews two/swagger.yaml

+paths:
+  /admin/draft_orders.json:
+    get:
+      description: Retrieve draft orders


Sometimes you have periods and sometimes you don't. Can you please be consistent?

dominiquesr · 2016-11-08T14:29:09Z

reviews two/swagger.yaml

+        description: array of staff account e-mail addresses.
+      subject:
+        type: string
+        description: subject of custom message.


I previously gave feedback that this was incorrect. can you go back and apply this feedback? this is not the subject of the custom message.

feedback that was provided was for draft_order_invoice and I will apply the same here.

dominiquesr · 2016-11-08T14:29:31Z

reviews two/swagger.yaml

+        description: subject of custom message.
+      custom_message:
+        type: string
+        description: Custom message.


I think I provided a longer description for this too on your previous PR.

feedback that was provided was for draft_order_invoice and I will apply the same here

feedback that was provided was for draft_order_invoice and I will apply the same here.

dominiquesr · 2016-11-08T14:30:23Z

reviews two/swagger.yaml

+      title:
+        type: string
+        description: The shipping method name.
+      phone_required:


Remove phone_required from the docs as we aren't exposing it for now

dominiquesr · 2016-11-08T14:30:53Z

reviews two/swagger.yaml

+    properties:
+      handle:
+        type: string
+        description: The handle of the shipping rate which was selected and applied, if applicable


Inconsistency with periods... Pick one or the other please.

second review

8fdbe14

andrewjtech123 mentioned this pull request Nov 7, 2016

secondary review #5

Closed

dominiquesr reviewed Nov 7, 2016

View reviewed changes

dominiquesr reviewed Nov 8, 2016

View reviewed changes

second review #4

Are you sure you want to change the base?

second review #4

Uh oh!

Conversation

andrewjtech123 commented Nov 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominiquesr Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominiquesr Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dominiquesr Nov 7, 2016 •

edited

Loading

dominiquesr Nov 8, 2016 •

edited

Loading