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

karmakaze · 2019-11-14T16:27:58Z

This PR adds the following POST requests:

POST fulfillments (via FulfillmentOrderFulfillment.save)
POST fulfillments/:fulfillment_id/update_tracking
POST fulfillments/:fulfillment_id/cancel

GET requests are in PR #633
FulfillmentOrder actions move, cancel, close are in PR #635
FulfillmentOrder actions fulfillment_request (+ accept/reject), cancellation_request (+ accept/reject) are in PR #637
Subsequent PRs will add other fulfillment order related requests to relevant resources.

Since all that's really needed is the fulfillment_id, if it's known you can do (rather than querying it first):

  FulfillmentOrderFulfillment.new(id: <the-fulfillment-id>).update_tracking(<params>)

Notes to reviewer:

there's already a Fulfillment resource which I haven't yet determined is the same/compatible with FO fulfillments so created a new resource FulfillmentOrderFulfillment
the specific statuses used in the tests may not make logical sense in terms of fulfillment order/fulfillment lifecycle. the tests do verify proper request dispatching and serialization

lib/shopify_api/resources/fulfillment_order_fulfillment.rb

test/fulfillment_order_fulfillment_test.rb

test/fulfillment_v2_test.rb

froot

Not finished reviewing this yet, will finish up tomorrow. Here are some of my existing thoughts.

froot · 2019-12-16T20:59:31Z

lib/shopify_api/resources/fulfillment.rb

@@ -6,8 +6,32 @@ def order_id
      @prefix_options[:order_id]
    end

-    def cancel; load_attributes_from_response(post(:cancel, {}, only_id)); end
+    def cancel(using_fulfillment_orders: false)


The new cancel endpoint does functionally the same thing as the old one. So why don't we just leave the original cancel method as is?

Yes, we had decided on this, everyone can use the existing endpoint.

froot · 2019-12-16T21:05:15Z

lib/shopify_api/resources/fulfillment.rb

    def complete; load_attributes_from_response(post(:complete, {}, only_id)); end
    def open; load_attributes_from_response(post(:open, {}, only_id)); end
+
+    def create


I'm unsure what's provided by ActiveResource::Base but is this intended to override self.create the class method instead of the instance method? What about .save? We still need to support both POST /admin/api/2020-01/orders/#{order_id}/fulfillments.json and POST /admin/api/2020-01/fulfillments.json no?

Yup, that makes sense, I forgot about save being used for create.

froot · 2019-12-20T20:01:22Z

lib/shopify_api/resources/fulfillment.rb

@@ -9,5 +9,26 @@ def order_id
    def cancel; load_attributes_from_response(post(:cancel, {}, only_id)); end
    def complete; load_attributes_from_response(post(:complete, {}, only_id)); end
    def open; load_attributes_from_response(post(:open, {}, only_id)); end
+
+    def self.create(attributes = {})


I think self.create is synonymous to calling new then save. https://api.rubyonrails.org/v3.2.6/classes/ActiveResource/Base.html#method-c-create so we should be good by just overriding save.

froot · 2019-12-20T20:07:07Z

lib/shopify_api/resources/fulfillment.rb

+      fulfillment
+    end
+
+    def save


We still need to support the ability to create fulfillments using the old endpoint, as well as the new fulfillment create:

We can do so by inspecting the passed in params and deciding if the client intended to use the legacy/new endpoint.

linzhao125

LGTM! I only have one question about `attributes.delete(:order_id).

linzhao125 · 2020-01-10T16:38:24Z

lib/shopify_api/resources/fulfillment.rb

@@ -9,5 +9,41 @@ def order_id
    def cancel; load_attributes_from_response(post(:cancel, {}, only_id)); end
    def complete; load_attributes_from_response(post(:complete, {}, only_id)); end
    def open; load_attributes_from_response(post(:open, {}, only_id)); end
+
+    def order_id=(order_id)
+      binding.pry


Need to be removed.

linzhao125 · 2020-01-10T16:43:12Z

lib/shopify_api/resources/fulfillment.rb

+
+    def order_id=(order_id)
+      binding.pry
+      attributes.delete('order_id') || attributes.delete(:order_id)


Why is it calling delete twice? I am a bit confused here 🤔

Normally the attributes contains string keys. When I was refactoring this, some versions ended up with symbol keys :order_id. Finally in the end, neither is needed as the prefix_options are set explicitly by client code and not inferred from attributes so removing this.

lib/shopify_api/resources/fulfillment_order.rb

…illment resource

tanema

Just wondering why you are doing one thing

tanema · 2020-01-10T20:26:27Z

lib/shopify_api/resources/fulfillment.rb

+        end
+
+        fulfillmentV2 = FulfillmentV2.new(attributes)
+        result = fulfillmentV2.save


Is this a way of migrating fulfillments? Why are you doing this?

We do this not for any sort of migration but because we have different endpoints with different supported operations. We have this situation because the backend has two REST paths for fulfillments, the existing pre-FulfillmentOrder one at /order/#/fulfillments and the new FulfillmentOrder based one at /fulfillments. To make them interoperable we use the Fulfillment resource which routes to the FulfillmentV2 resource when we need to use the /fulfillments path for an operation. Whenever reading a response, we always convert FulfillmentV2 back to Fulfillment.

karmakaze requested a review from a team as a code owner November 14, 2019 16:27

karmakaze requested review from caueguerra, stevelounsbury, justinkrol, jianghong and StefanSlehta November 14, 2019 16:28

karmakaze force-pushed the fulfillment-orders-fulfillment-cancellation-request branch from 7f5e63c to 90ab4b6 Compare November 14, 2019 22:34

karmakaze force-pushed the fulfillment-orders-fulfillment branch from 43505f9 to 797e348 Compare November 14, 2019 22:42

froot reviewed Nov 26, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-fulfillment-cancellation-request branch from 366a1e8 to 4ce57df Compare November 26, 2019 18:37

karmakaze force-pushed the fulfillment-orders-fulfillment branch from 797e348 to 130deca Compare November 26, 2019 20:16

tanema approved these changes Nov 26, 2019

View reviewed changes

froot reviewed Nov 28, 2019

View reviewed changes

test/fulfillment_v2_test.rb Outdated Show resolved Hide resolved

test/fulfillment_v2_test.rb Outdated Show resolved Hide resolved

test/fulfillment_v2_test.rb Outdated Show resolved Hide resolved

froot reviewed Nov 28, 2019

View reviewed changes

test/fulfillment_v2_test.rb Outdated Show resolved Hide resolved

test/fulfillment_v2_test.rb Outdated Show resolved Hide resolved

karmakaze force-pushed the fulfillment-orders-fulfillment branch from 130deca to d1fd0c8 Compare November 28, 2019 15:39

froot approved these changes Nov 29, 2019

View reviewed changes

caueguerra approved these changes Nov 29, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-fulfillment-cancellation-request branch 2 times, most recently from 8c5ddaf to 41b60d0 Compare December 2, 2019 20:07

karmakaze force-pushed the fulfillment-orders-fulfillment branch from d1fd0c8 to 0c557e7 Compare December 2, 2019 20:08

karmakaze force-pushed the fulfillment-orders-fulfillment branch from 0c557e7 to a82a554 Compare December 10, 2019 20:09

karmakaze force-pushed the fulfillment-orders-fulfillment-cancellation-request branch 2 times, most recently from f1da843 to 46935ea Compare December 16, 2019 18:21

karmakaze force-pushed the fulfillment-orders-fulfillment branch from a82a554 to 1c3b49d Compare December 16, 2019 18:22

froot reviewed Dec 16, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-fulfillment-cancellation-request branch from 46935ea to 225ad9f Compare December 16, 2019 21:36

karmakaze force-pushed the fulfillment-orders-fulfillment branch from 1c3b49d to 744ef60 Compare December 17, 2019 18:40

froot reviewed Dec 20, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-fulfillment-cancellation-request branch 2 times, most recently from fc57449 to c5e8b3b Compare January 9, 2020 21:06

karmakaze closed this Jan 9, 2020

karmakaze force-pushed the fulfillment-orders-fulfillment branch from 744ef60 to 6fc2312 Compare January 9, 2020 23:09

karmakaze reopened this Jan 10, 2020

linzhao125 reviewed Jan 10, 2020

View reviewed changes

karmakaze force-pushed the fulfillment-orders-fulfillment branch from e34a461 to a615ff8 Compare January 10, 2020 18:51

Add save, update_tracking, and cancel actions to FulfillmentOrderFulf…

4842030

…illment resource

karmakaze force-pushed the fulfillment-orders-fulfillment branch from a615ff8 to 4842030 Compare January 10, 2020 19:31

linzhao125 approved these changes Jan 10, 2020

View reviewed changes

tanema approved these changes Jan 10, 2020

View reviewed changes

karmakaze merged commit cc1307d into fulfillment-orders-fulfillment-cancellation-request Jan 13, 2020

karmakaze deleted the fulfillment-orders-fulfillment branch January 13, 2020 18:03

karmakaze restored the fulfillment-orders-fulfillment branch January 13, 2020 21:16

karmakaze mentioned this pull request Jan 13, 2020

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

Merged

karmakaze deleted the fulfillment-orders-fulfillment branch January 14, 2020 15:41

karmakaze mentioned this pull request Jan 20, 2020

[WIP] Adding FulfillmentOrder, FulfillmentRequest, CancellationRequest, AssignedFulfillmentOrder resources/filters #622

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

karmakaze commented Nov 14, 2019

froot left a comment

froot Dec 16, 2019

karmakaze Dec 16, 2019

froot Dec 16, 2019

karmakaze Dec 16, 2019

froot Dec 20, 2019

froot Dec 20, 2019

linzhao125 left a comment

linzhao125 Jan 10, 2020

linzhao125 Jan 10, 2020

karmakaze Jan 10, 2020

tanema left a comment

tanema Jan 10, 2020

karmakaze Jan 13, 2020 •

edited

Loading

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

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

Conversation

karmakaze commented Nov 14, 2019

froot 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

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

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

karmakaze Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

karmakaze Jan 13, 2020 •

edited

Loading