Updates from PR review

lib/shopify_api/resources/fulfillment_order.rb

@@ -14,12 +14,16 @@ def fulfillments(options = {})
     end
 
     def move(new_location_id:)
-      body = { fulfillment_order: { new_location_id: new_location_id } }
-      load_values(post(:move, body, only_id))
+      body = { fulfillment_order: { new_location_id: new_location_id } }.to_json
+      keyed_fulfillment_orders = keyed_fulfillment_orders_from_response(post(:move, {}, body))
+      load_keyed_fulfillment_order(keyed_fulfillment_orders, 'original_fulfillment_order')
+      keyed_fulfillment_orders
     end
 
     def cancel
-      load_values(post(:cancel, {}, only_id))
+      keyed_fulfillment_orders = keyed_fulfillment_orders_from_response(post(:cancel, {}, only_id))
+      load_keyed_fulfillment_order(keyed_fulfillment_orders, 'fulfillment_order')
+      keyed_fulfillment_orders
     end
 
     def close
@@ -28,17 +32,19 @@ def close
 
     private
 
-    def load_values(response)
+    def load_keyed_fulfillment_order(keyed_fulfillment_orders, key)
+      if keyed_fulfillment_orders[key]&.attributes
+        load(keyed_fulfillment_orders[key].attributes, false, true)
+      end
+    end
+
+    def keyed_fulfillment_orders_from_response(response)
       return load_attributes_from_response(response) if response.code != '200'
 
-      keyed_fulfillments = ActiveSupport::JSON.decode(response.body)
-      keyed_fulfillments.map do |key, fo_attributes|
-        if fo_attributes.nil?
-          [key, nil]
-        else
-          [key, FulfillmentOrder.new(fo_attributes)]
-        end
-      end.to_h
+      keyed_fulfillment_orders = ActiveSupport::JSON.decode(response.body)
+      keyed_fulfillment_orders.transform_values do |fulfillment_order_attributes|
+        FulfillmentOrder.new(fulfillment_order_attributes) if fulfillment_order_attributes
+      end
     end
   end
 end

test/fulfillment_order_test.rb

@@ -55,46 +55,48 @@ def setup
     end
 
     context "#move" do
-      should "be able to move fulfillment order to a new_location_id" do
+      should "move a fulfillment order to a new_location_id" do
         fulfillment_order = ShopifyAPI::FulfillmentOrder.find(519788021)
         new_location_id = 5
 
-        moved = ActiveSupport::JSON.decode(load_fixture('fulfillment_order'))
-        moved['assigned_location_id'] = new_location_id
-        fulfillment_order.status = 'closed'
+        fake_original_fulfillment_order = fulfillment_order.clone
+        fake_original_fulfillment_order.status = 'closed'
+        fake_moved_fulfillment_order = ActiveSupport::JSON.decode(load_fixture('fulfillment_order'))
+        fake_moved_fulfillment_order['assigned_location_id'] = new_location_id
 
+        request_body = { fulfillment_order: { new_location_id: 5 } }
         body = {
-            original_fulfillment_order: fulfillment_order,
-            moved_fulfillment_order: moved,
-            remaining_fulfillment_order: nil,
+          original_fulfillment_order: fake_original_fulfillment_order,
+          moved_fulfillment_order: fake_moved_fulfillment_order,
+          remaining_fulfillment_order: nil,
         }
-        api_version = ShopifyAPI::ApiVersion.find_version('2019-01')
-        endpoint = "fulfillment_orders/519788021/move"
-        extension = ".json"
-        url = "https://this-is-my-test-shop.myshopify.com#{api_version.construct_api_path("#{endpoint}#{extension}")}"
-        url = url + "?fulfillment_order%5Bnew_location_id%5D=5"
-        fake endpoint, :method => :post, :url => url, :body => ActiveSupport::JSON.encode(body)
-
-        response_fos = fulfillment_order.move(new_location_id: new_location_id)
-        assert_equal 3, response_fos.count
-        original_fulfillment_order = response_fos['original_fulfillment_order']
+        fake "fulfillment_orders/519788021/move", :method => :post,
+          :request_body => ActiveSupport::JSON.encode(request_body),
+          :body => ActiveSupport::JSON.encode(body)
+
+        response_fulfillment_orders = fulfillment_order.move(new_location_id: new_location_id)
+
+        assert_equal 'closed', fulfillment_order.status
+
+        assert_equal 3, response_fulfillment_orders.count
+        original_fulfillment_order = response_fulfillment_orders['original_fulfillment_order']
         refute_nil original_fulfillment_order
-        assert_equal 'ShopifyAPI::FulfillmentOrder', original_fulfillment_order.class.name
+        assert original_fulfillment_order.is_a?(ShopifyAPI::FulfillmentOrder)
         assert_equal 'closed', original_fulfillment_order.status
 
-        moved_fulfillment_order = response_fos['moved_fulfillment_order']
+        moved_fulfillment_order = response_fulfillment_orders['moved_fulfillment_order']
         refute_nil moved_fulfillment_order
-        assert_equal 'ShopifyAPI::FulfillmentOrder', moved_fulfillment_order.class.name
+        assert moved_fulfillment_order.is_a?(ShopifyAPI::FulfillmentOrder)
         assert_equal 'open', moved_fulfillment_order.status
         assert_equal new_location_id, moved_fulfillment_order.assigned_location_id
 
-        remaining_fulfillment_order = response_fos['remaining_fulfillment_order']
+        remaining_fulfillment_order = response_fulfillment_orders['remaining_fulfillment_order']
         assert_nil remaining_fulfillment_order
       end
     end
 
     context "#cancel" do
-      should "be able to cancel fulfillment order" do
+      should "cancel a fulfillment order" do
         fulfillment_order = ShopifyAPI::FulfillmentOrder.find(519788021)
         assert_equal 'open', fulfillment_order.status
 
@@ -106,11 +108,13 @@ def setup
         }
         fake "fulfillment_orders/519788021/cancel", :method => :post, :body => ActiveSupport::JSON.encode(body)
 
-        response_fos = fulfillment_order.cancel
-        assert_equal 2, response_fos.count
-        fulfillment_order = response_fos['fulfillment_order']
+        response_fulfillment_orders = fulfillment_order.cancel
+
+        assert_equal 'cancelled', fulfillment_order.status
+        assert_equal 2, response_fulfillment_orders.count
+        fulfillment_order = response_fulfillment_orders['fulfillment_order']
         assert_equal 'cancelled', fulfillment_order.status
-        replacement_fulfillment_order = response_fos['replacement_fulfillment_order']
+        replacement_fulfillment_order = response_fulfillment_orders['replacement_fulfillment_order']
         assert_equal 'open', replacement_fulfillment_order.status
       end
     end

test/test_helper.rb

@@ -92,6 +92,7 @@ def assert_request_body(expected)
       end
 
       def fake(endpoint, options={})
+        request_body = options.has_key?(:request_body) ? options.delete(:request_body) : nil
         body   = options.has_key?(:body) ? options.delete(:body) : load_fixture(endpoint)
         format = options.delete(:format) || :json
         method = options.delete(:method) || :get
@@ -104,7 +105,9 @@ def fake(endpoint, options={})
           "https://this-is-my-test-shop.myshopify.com#{api_version.construct_api_path("#{endpoint}#{extension}")}"
         end
 
-        WebMock.stub_request(method, url).to_return(
+        stubbing = WebMock.stub_request(method, url)
+        stubbing = stubbing.with(body: request_body) if request_body
+        stubbing.to_return(
           body: body, status: status, headers: { content_type: "text/#{format}", content_length: 1 }.merge(options)
         )
       end