Add fulfillment order related read requests #633

karmakaze · 2019-11-06T21:57:52Z

This PR adds the following read requests:

GET orders/:order_id/fulfillment_orders
GET assigned_fulfillment_orders
GET fulfillment_orders/:fulfillment_order_id
GET fulfillment_orders/:fulfillment_order_id/fulfillments

Subsequent PRs will add fulfillment order related write requests to relevant resources.

The approach I took was pretty straight forward with the following notable choices:

the resources returned via AssignedFulfillmentOrder are converted to FulfillmentOrder type so that subsequent actions defined on FulfillmentOrder may be used
the fulfillments method on FulfillmentOrder will get the related fulfillments

froot · 2019-11-13T21:25:08Z

lib/shopify_api/resources/fulfillment_order_fulfillment.rb

@@ -0,0 +1,4 @@
+module ShopifyAPI
+  class FulfillmentOrderFulfillment < Base


Wondering if this is needed? I'm thinking that the fulfillments returned via GET fulfillment_orders/:fulfillment_order_id/fulfillments shouldn't be treated any differently than the existing fulfillment object.

It wasn't clear at the time whether it was ok to mix/match existing Fulfillment actions with those newly added, seemed like it might get confusing to remember which are legacy type requests vs new fulfillment order ones.

Should we restrict apps from calling legacy requests on fulfillments (https://help.shopify.com/en/api/reference/shipping-and-fulfillment/fulfillment#show-2019-10) associated to fulfillment orders? I don't think we should since non opted-in FS should be able to query for fulfillments from a fulfillment order and still run legacy requests.

There are a few conflicting overlaps that have to get resolved if we collapse them into a single Fulfullment resource. Queries will either route to under orders or fulfillment_orders. There's also the cancel action that's defined for both legacy and FO fulfullment.

We could collapse them together, and have to override a lot of the default ActiveResource behaviour to include both types of actions (legacy and new). It may not be necessary to do all of that extra work though, talked with Malaz and it should be fine to return a fulfillment resource that doesn't support legacy actions. Non opted-in FS shouldn't really even be querying fulfillments from fulfillment orders (even though we don't restrict them from doing so). That being said I like Glen's proposition of instead using FulfillmentV2 as the resource name.

There's no technical problem with returning Fulfillment in the shopify_api from a v2 operation. Only confusion would be then trying to perform a legacy operation on it. It really comes down to how the two types of Fulfillment resources get documented. If we distinguish the operations as legacy / FO we can use the same Fulfillment resource in shopify_api. If we document two different types of Fulfillment resources then we can call the new one FulfillmentV2.

Currently, the documentation exists like so:

https://help.shopify.com/en/api/reference/shipping-and-fulfillment/fulfillment#complete-2019-10

https://help.shopify.com/en/api/reference/shipping-and-fulfillment/unstable/fulfillmentunstable

Thanks this is very clear. We have two separate descriptions of distinct Fulfillment resources. We should go with FulfillmentV2

@karmakaze I'm working on documentation right now and realized that the shipping-and-fulfillment/unstable/fulfillmentunstable doc will be moved to under the shipping-and-fulfillment/fulfillment doc (under the RC version). The new Fulfillment resource under the 2020-01 version will have both the legacy operations and new operations that we are adding. With this, should we combine FulfillmentV2 and Fulfillment?

Yes, it makes sense to combine them to follow the documentation structure.

froot · 2019-11-13T21:25:57Z

lib/shopify_api/resources/fulfillment_order.rb

@@ -0,0 +1,14 @@
+module ShopifyAPI
+  class FulfillmentOrder < Base
+    def self.all(options = {})


With the fulfillment_orders method implemented on the order object, is this method necessary?

That's right it can be queried on the order object. Any activeresource type will attempt to make queries, i.e. FulfillmentOrder.all will attempt to make a request even if this override method isn't included. Adding this just does the expected thing by routing to the order's implementation rather than failing without good diagnostics.

justinkrol

I'm not very familiar with this repo, so I have a lot of questions!

lib/shopify_api/resources/assigned_fulfillment_order.rb

justinkrol · 2019-11-13T21:23:16Z

lib/shopify_api/resources/assigned_fulfillment_order.rb

+
+    def self.all(options = {})
+      assigned_fulfillment_orders = super(options)
+      assigned_fulfillment_orders.map { |afo| FulfillmentOrder.new(afo.as_json) }


If we do AssignedFulfillmentOrder < FulfillmentOrder, is this still necessary?

Seemed like that should work but it results in uninitialized constant ShopifyAPI::FulfillmentOrder (NameError)

lib/shopify_api/resources/fulfillment_order_fulfillment.rb

test/assigned_fulfillment_order_test.rb

lib/shopify_api/resources/fulfillment_order.rb

test/order_test.rb

glen-j-nkali

Looks great, thanks for pushing this through 🎉 !
I added a couple of nits but the code LGTM

glen-j-nkali · 2019-11-19T20:52:30Z

lib/shopify_api/resources/fulfillment_order_fulfillment.rb

@@ -0,0 +1,4 @@
+module ShopifyAPI
+  class FulfillmentOrderFulfillment < Base


Not familiar with this repo but ideally we would like to return a Fulfillment object instead here. If that's not possible what do you think of calling this FulfillmentV2 instead ?

Reason being that that's how we called our new mutation and inputs on the Shopify/Shopify side of things and I personally think that Fulfillment_V2 does a better job of indicating that this is related to the current Fulfillment object.

Naming as FulfillmentV2 since it will most closely match the documentation having separate sections describing each form of fulfillment resource.

glen-j-nkali · 2019-11-19T20:56:44Z

lib/shopify_api/resources/assigned_fulfillment_order.rb

+    FULFILLMENT_ACCEPTED = 'fulfillment_accepted'
+
+    def self.all(options = {})
+      assigned_fulfillment_orders = super(options)


should you enforce that if the options have a assigned_status param then the value should be part of the constants you defined above ?

There's a check on Shopify/Shopify side that's going to 💥 if they aren't

Why don't we allow Shopify/Shopify to 💥 and have it bubble up to shopify_api? If core is already checking this and returning that error, isn't this check redundant?

We could go either way. If we don't perform that check on this side there's not really any use for the constants described above.

From an API perspective I would argue that all checks that you can perform without making a request across the network should be performed locally.

One could argue that doing so kind of forces you to keep these constant values in sync with Core but with versioning and the 1 year deprecation cycle you should have plenty of time to update them if anything changes on Core.

That's fair, I'm fine with both implementations 👍

My original thought was to provide the constants to prevent typos from custom client code. Putting the constants/checks here means that it should be kept in sync with server side changes. The rest of the shopify_api doesn't seem to define many constants, so removing them is more consistent.

I think the assigned Fulfillment orders controller is probably the only one that accepts a custom type of filter param (assignment status) that's later converted into something else. I think that's the reason why you can't find another example on the shopify api.

I'm fine either way as I previously said but you should keep in mind that relying on Shopify Core to return you a bad request when you send in invalid assignment status means that you will have to parse that error message appropriately in order to return something meaningful to the user.

I ran a quick test and this is the error message Core will return :

"Assignment status is not included in the list"

This conveys what the issue is but isn't clear from an API standpoint IMO (what's the list). One way you could address this, could be through the documentation. Just something to keep in mind IMO.

That's not a great error message, but if the client specifies an assigned_status and got this Bad Request, it should be enough to point to documentation.

test/assigned_fulfillment_order_test.rb

lib/shopify_api/resources/assigned_fulfillment_order.rb

froot · 2019-11-25T15:54:24Z

lib/shopify_api/resources/assigned_fulfillment_order.rb

+    FULFILLMENT_ACCEPTED = 'fulfillment_accepted'
+
+    def self.all(options = {})
+      assigned_fulfillment_orders = super(options)


Why don't we allow Shopify/Shopify to 💥 and have it bubble up to shopify_api? If core is already checking this and returning that error, isn't this check redundant?

lib/shopify_api/resources/assigned_fulfillment_order.rb

froot · 2019-11-29T18:34:50Z

lib/shopify_api/resources/assigned_fulfillment_order.rb

+  class AssignedFulfillmentOrder < Base
+    def self.all(options = {})
+      assigned_fulfillment_orders = super(options)
+      assigned_fulfillment_orders.map { |afo| FulfillmentOrder.new(afo.as_json) }


Could we use attributes instead of as_json?

In this instance attributes works just fine.

froot · 2019-11-29T18:40:18Z

lib/shopify_api/resources/fulfillment_order.rb

+    end
+
+    def fulfillments(options = {})
+      fo_fulfillments = get(:fulfillments, options)


Anywhere you find yourself abbreviating fulfillment orders to fo, it's best to use the full phrase for clarity

- orders/:order_id/fulfillment_orders - assigned_fulfillment_orders - fulfillment_orders/:fulfillment_order_id - fulfillment_orders/:fulfillment_order_id/fulfillments Read FulfillmentOrder for an Order or with just an order_id

…illmentOrder

karmakaze · 2020-01-09T19:49:55Z

Fixed the find/all problem found in the tests. There are two usages (1) direct find by id should use the /fulfillment_orders/:id endpoint, and all should require and order_id and use the /orders/:order_id/fulfillment_orders endpoint.

linzhao125

Looks great overall!
I left a comment about .all for your consideration.

linzhao125 · 2020-01-10T14:48:15Z

lib/shopify_api/resources/fulfillment_order.rb

+        raise ShopifyAPI::ValidationException, "'order_id' is required" if order_id.blank?
+
+        order = ::ShopifyAPI::Order.new(id: order_id)
+        order.fulfillment_orders(args.first[:params].except(:order_id))


I don't think we need to support the case of all here. If a user wants to get all fulfillment_orders for an order, they can call order.fulfillment_orders. Support calling .all with a param order_id seems redundant IMO.

Given that FulfillmentOrder can be queried as a top-level resource with find it would be unexpected from an ActiveResource implementation to not support all in some meaningful way.

tanema

Looks good I have a few questions and if they have answers then this can go out.

tanema · 2020-01-10T19:30:31Z

lib/shopify_api/resources/order.rb

@@ -30,6 +30,11 @@ def capture(amount = "", currency: nil)
      Transaction.create(capture_transaction)
    end

+    def fulfillment_orders(options = {})


Could this not be done with a has_many?

That doesn't quite work for us because FulfillmentOrder is both a top-level resource (i.e. /fulfillment_orders/:id) as well as defined within an order (/orders/:order_id/fulfillments). We resolved this by creating the FulfillmentOrder ActiveResource as a top-level one (no :order_id prefix_option) then handling the order.fulfillment_orders with method implementation rather than association.

tanema · 2020-01-10T19:32:52Z

lib/shopify_api/resources/assigned_fulfillment_order.rb

+  class AssignedFulfillmentOrder < Base
+    def self.find(scope, *args)
+      assigned_fulfillment_orders = super(scope, *args)
+      assigned_fulfillment_orders.map { |afo| FulfillmentOrder.new(afo.attributes) }


Wont this cause complications? You find assigned fulfilments and then if you update that FulfillmentOrder it will hit the fulfillments endpoint. Why are you returning FulfillmentOrders from AssignedFulfillmentOrder. (this may just be me not understanding the work that you are doing)

AssignedFulfillmentOrders are just FulfillmentOrders that just happen to be assigned. We didn't want to create two different ActiveResource types for subsequent actions common to both AssignedFulfillmentOrders and FulfillmentOrders so we convert the first to the second.

karmakaze requested a review from a team as a code owner November 6, 2019 21:57

karmakaze self-assigned this Nov 7, 2019

karmakaze force-pushed the fulfillment-orders-reads branch 3 times, most recently from b0e31d0 to b941411 Compare November 7, 2019 21:27

karmakaze mentioned this pull request Nov 8, 2019

Add move, cancel, close actions FulfillmentOrder #635

Merged

karmakaze requested review from caueguerra, justinkrol, stevelounsbury, jianghong and StefanSlehta November 8, 2019 16:36

karmakaze mentioned this pull request Nov 13, 2019

Fulfillment orders fulfillment cancellation request #637

Merged

froot reviewed Nov 13, 2019

View reviewed changes

justinkrol reviewed Nov 13, 2019

View reviewed changes

karmakaze mentioned this pull request Nov 14, 2019

Add save, update_tracking, and cancel actions to FulfillmentOrderFulfillment resource #639

Merged

karmakaze force-pushed the fulfillment-orders-reads branch 3 times, most recently from d4f26b4 to cfb265f Compare November 14, 2019 21:45

glen-j-nkali approved these changes Nov 19, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-reads branch 2 times, most recently from 702b858 to c72bff7 Compare November 21, 2019 21:53

froot reviewed Nov 25, 2019

View reviewed changes

froot reviewed Nov 29, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-reads branch from fde9c1e to 738e870 Compare December 2, 2019 20:07

caueguerra approved these changes Dec 3, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-reads branch from d110aeb to 0870528 Compare December 16, 2019 18:21

Add fulfillment order related GET requests:

b0350cb

- orders/:order_id/fulfillment_orders - assigned_fulfillment_orders - fulfillment_orders/:fulfillment_order_id - fulfillment_orders/:fulfillment_order_id/fulfillments Read FulfillmentOrder for an Order or with just an order_id

karmakaze force-pushed the fulfillment-orders-reads branch from 0870528 to b0350cb Compare January 8, 2020 18:23

Define find rather than all for AssignedFulfillmentOrder and Fulf…

6fc2312

…illmentOrder

karmakaze force-pushed the fulfillment-orders-reads branch from e3eeb3b to 6fc2312 Compare January 9, 2020 19:08

linzhao125 force-pushed the fulfillment-orders-reads branch from 6fc2312 to 50bd958 Compare January 9, 2020 22:59

karmakaze force-pushed the fulfillment-orders-reads branch from 50bd958 to 6fc2312 Compare January 9, 2020 23:12

linzhao125 reviewed Jan 10, 2020

View reviewed changes

tanema approved these changes Jan 10, 2020

View reviewed changes

karmakaze merged commit e180de4 into master Jan 13, 2020

linzhao125 temporarily deployed to rubygems January 27, 2020 20:49 Inactive

		@@ -0,0 +1,4 @@
		module ShopifyAPI
		class FulfillmentOrderFulfillment < Base

Add fulfillment order related read requests #633

Add fulfillment order related read requests #633

Conversation

karmakaze commented Nov 6, 2019

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

froot Nov 25, 2019 • edited

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

justinkrol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glen-j-nkali 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmakaze commented Jan 9, 2020 • edited

linzhao125 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanema 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

froot Nov 25, 2019 •

edited

karmakaze commented Jan 9, 2020 •

edited