Add move, cancel, close actions FulfillmentOrder #635

karmakaze · 2019-11-08T16:03:51Z

This PR adds the following POST requests:

POST fulfillment_orders/:fulfillment_order_id/move
POST fulfillment_orders/:fulfillment_order_id/cancel
POST fulfillment_orders/:fulfillment_order_id/close

GET requests are in PR #633
Subsequent PRs will add other fulfillment order related write requests to relevant resources.

The approach I took was to add the move, cancel, and close actions on the FulfillmentOrder resource.

Because there's already a legacy Fulfillment resource, I named the fulfillment orders fulfillment as FulfillmentOrderFulfillment.

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

  closed_fulfillment_order = FulfillmentOrder.new(id: <the-fo-id>).close

froot · 2019-11-13T20:54:56Z

A few questions:

Given that move and cancel both return the original fulfillment order somewhere in the payload, shouldn’t we call load_attributes_from_response to populate the current object with the updated attributes?
According to https://help.shopify.com/en/api/reference/shipping-and-fulfillment/unstable/fulfillmentorder#close should we enable the app the provide an option message in the body on close?

test/fulfillment_order_test.rb

glen-j-nkali

Couple of comments to make sure I am following the logic here. Thank you again for handling this 🙏

lib/shopify_api/resources/fulfillment_order.rb

glen-j-nkali · 2019-11-19T21:47:49Z

lib/shopify_api/resources/fulfillment_order.rb

+      body = { fulfillment_order: { new_location_id: new_location_id } }.to_json
+      keyed_fos = load_keyed_attributes_from_response(post(:move, {}, body))
+      if keyed_fos&.fetch('original_fulfillment_order', nil)&.attributes
+        load(keyed_fos['original_fulfillment_order'].attributes, false, true)


just for my personal comprehension here but what are you trying to achieve here ? Specifically why does this check only apply to the original_fulfillment_order ?

Are you using it to determine wether or not we got here with the error message payload ?

I was a bit uncertain about this one. The request is made using an instance of a FulfillmentOrder, e.g. fulfillment_order.move(...) where the server responds with multiple keyed FOs. I took the one named original_fulfillment_order to represent the mutation of the fulfillment_order that was the subject of the .move and so load the attributes of original_fulfillment_order into itself.

This is following the pattern where responses that return a single FO and calling load_attributes_from_response will load the calling object's attrributes from the only keyed value.

glen-j-nkali · 2019-11-19T21:51:04Z

lib/shopify_api/resources/fulfillment_order.rb

+
+    def cancel
+      keyed_fos = load_keyed_attributes_from_response(post(:cancel, {}, only_id))
+      if keyed_fos&.fetch('fulfillment_order', nil)&.attributes


same question as above about why specifically this key ?

This key represents the mutation of the subject FO resource issuing the .cancel.

lib/shopify_api/resources/fulfillment_order.rb

froot · 2019-11-28T03:30:49Z

lib/shopify_api/resources/fulfillment_order.rb

+        else
+          [key, FulfillmentOrder.new(fo_attributes)]
+        end
+      end.to_h


nit: a simpler way to do this would be to use transform_values method:

keyed_fulfillment_orders.transform_values do |fulfillment_order| fulfillment_order.nil? nil : FulfillmentOrder.new(fulfillment_order) end

froot · 2019-11-29T16:48:27Z

lib/shopify_api/resources/fulfillment_order.rb

+    def move(new_location_id:)
+      body = { fulfillment_order: { new_location_id: new_location_id } }.to_json
+      keyed_fos = load_keyed_fulfillment_orders_from_response(post(:move, {}, body))
+      if keyed_fos&.fetch('original_fulfillment_order', nil)&.attributes


Maybe consider extracting this into a private method def load_attributes_from_response_for(fulfillment_order_key:) since it's used both in this PR and in https://github.com/Shopify/shopify_api/pull/637/files

froot · 2019-11-29T17:01:32Z

test/fulfillment_order_test.rb

+        fulfillment_order = ShopifyAPI::FulfillmentOrder.find(519788021)
+        new_location_id = 5
+
+        original = fulfillment_order.clone


I was confused reading this at first, would be easier to understand if we rename this to original_fulfillment_order_response an moved_fulfillment_order_response so that it's more clear the intention of declaring these variables.

froot · 2019-11-29T17:06:59Z

test/fulfillment_order_test.rb

+        assert_equal 3, response_fos.count
+        original_fulfillment_order = response_fos['original_fulfillment_order']
+        refute_nil original_fulfillment_order
+        assert_equal 'ShopifyAPI::FulfillmentOrder', original_fulfillment_order.class.name


nit: assert original_fulfillment_order.is_a?(ShopifyApi::FulfillmentOrder)

froot · 2019-11-29T17:48:20Z

test/fulfillment_order_test.rb

+        assert_equal new_location_id, moved_fulfillment_order.assigned_location_id
+
+        remaining_fulfillment_order = response_fos['remaining_fulfillment_order']
+        assert_nil remaining_fulfillment_order


nit: can be compressed to assert_nil response_fos['remaining_fulfillment_order']

froot · 2019-11-29T18:56:01Z

test/fulfillment_order_test.rb

+
+        assert_equal 'cancelled', fulfillment_order.status
+        assert_equal 2, response_fos.count
+        fulfillment_order = response_fos['fulfillment_order']


I would rename this to something other than fulfillment_order, maybe response_fulfillment_order.

froot

One small necessary change (allowing apps to provide a message when closing a fulfillment order) and some nits. Other than that LGTM!

lib/shopify_api/resources/fulfillment_order.rb

test/assigned_fulfillment_order_test.rb

linzhao125

This looks great after reading all the conversations from previous reviews. It is interesting that the caller fulfillment_order needs to be loaded manually. I think the fact that we want to support both order/#{order_id}/fulfillment_orders and fulfillment_orders/#{fulfillment_order_id} made things a bit tricky. Also we return multiple keyed resources.

test/fulfillment_order_test.rb

karmakaze requested a review from a team as a code owner November 8, 2019 16:03

karmakaze self-assigned this Nov 8, 2019

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

karmakaze mentioned this pull request Nov 13, 2019

Fulfillment orders fulfillment cancellation request #637

Merged

justinkrol reviewed Nov 13, 2019

View reviewed changes

test/fulfillment_order_test.rb Outdated Show resolved Hide resolved

test/fulfillment_order_test.rb Outdated Show resolved Hide resolved

test/fulfillment_order_test.rb Outdated Show resolved Hide resolved

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch 2 times, most recently from 1d26023 to 6dae6dd Compare November 13, 2019 23:21

justinkrol reviewed Nov 14, 2019

View reviewed changes

test/fulfillment_order_test.rb Outdated Show resolved Hide resolved

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 2 times, most recently from d4f26b4 to cfb265f Compare November 14, 2019 21:45

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from 6dae6dd to e66255e Compare November 14, 2019 22:09

glen-j-nkali reviewed Nov 19, 2019

View reviewed changes

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from e66255e to 634d2b3 Compare November 21, 2019 21:08

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

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from 634d2b3 to a47cdba Compare November 26, 2019 18:21

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

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from a47cdba to 1a48c0e Compare December 2, 2019 20:07

froot reviewed Dec 4, 2019

View reviewed changes

lib/shopify_api/resources/fulfillment_order.rb Outdated Show resolved Hide resolved

test/assigned_fulfillment_order_test.rb Outdated Show resolved Hide resolved

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from 1a48c0e to 2bbefa9 Compare December 10, 2019 20:13

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

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from 2bbefa9 to 741c27e Compare December 16, 2019 18:21

froot approved these changes Dec 16, 2019

View reviewed changes

karmakaze requested review from justinkrol and glen-j-nkali January 8, 2020 15:43

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

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch 2 times, most recently from 621057d to 563d01d Compare January 9, 2020 21:06

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 approved these changes Jan 10, 2020

View reviewed changes

linzhao125 reviewed Jan 10, 2020

View reviewed changes

test/fulfillment_order_test.rb Outdated Show resolved Hide resolved

FulfillmentOrder: move/cancel/close

eb38d9a

karmakaze force-pushed the fulfillment-orders-move-cancel-close branch from 563d01d to eb38d9a Compare January 10, 2020 15:46

tanema approved these changes Jan 10, 2020

View reviewed changes

karmakaze merged commit 001f427 into fulfillment-orders-reads Jan 13, 2020

This was referenced Jan 13, 2020

Add move, cancel, close actions FulfillmentOrder #664

Merged

Fulfillment orders fulfillment cancellation request #665

Merged

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

Merged

karmakaze deleted the fulfillment-orders-move-cancel-close 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 move, cancel, close actions FulfillmentOrder #635

Add move, cancel, close actions FulfillmentOrder #635

karmakaze commented Nov 8, 2019 •

edited

Loading

froot commented Nov 13, 2019 •

edited

Loading

glen-j-nkali left a comment

glen-j-nkali Nov 19, 2019

karmakaze Nov 19, 2019

glen-j-nkali Nov 19, 2019

karmakaze Nov 19, 2019

froot Nov 28, 2019

froot Nov 29, 2019

froot Nov 29, 2019

froot Nov 29, 2019

froot Nov 29, 2019

froot Nov 29, 2019

froot left a comment

linzhao125 left a comment •

edited

Loading

Add move, cancel, close actions FulfillmentOrder #635

Add move, cancel, close actions FulfillmentOrder #635

Conversation

karmakaze commented Nov 8, 2019 • edited Loading

froot commented Nov 13, 2019 • edited Loading

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

froot left a comment

Choose a reason for hiding this comment

linzhao125 left a comment • edited Loading

Choose a reason for hiding this comment

karmakaze commented Nov 8, 2019 •

edited

Loading

froot commented Nov 13, 2019 •

edited

Loading

linzhao125 left a comment •

edited

Loading