From 7594d372539b9061c4dd935780ab84907b0d8207 Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Thu, 25 Mar 2021 15:03:55 -0700 Subject: [PATCH 1/4] Move tests for creating a customer return into a new context In a subsequent change we are going to be adding a parallel context for when we want to reference existing return items, so this is just the first step to that change. There should be no change in functionality just spacing here. Co-authored-by: Mike Conlin --- .../api/customer_returns_controller_spec.rb | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/api/spec/requests/spree/api/customer_returns_controller_spec.rb b/api/spec/requests/spree/api/customer_returns_controller_spec.rb index 3f0eba8f90c..8830d3210b5 100644 --- a/api/spec/requests/spree/api/customer_returns_controller_spec.rb +++ b/api/spec/requests/spree/api/customer_returns_controller_spec.rb @@ -112,23 +112,27 @@ module Spree expect(json_response["stock_location_id"]).to eq final_stock_location.id end - it "can create a new customer return" do - stock_location = FactoryBot.create(:stock_location) - unit = FactoryBot.create(:inventory_unit, state: "shipped") - cr_params = { stock_location_id: stock_location.id, - return_items_attributes: [{ - inventory_unit_id: unit.id, - reception_status_event: "receive", - }] } - - post spree.api_order_customer_returns_path(order), params: { order_id: order.number, customer_return: cr_params } - - expect(response.status).to eq(201) - expect(json_response).to have_attributes(attributes) + context "when creating new return items" do + it "can create a new customer return" do + stock_location = FactoryBot.create(:stock_location) + unit = FactoryBot.create(:inventory_unit, state: "shipped") + cr_params = { stock_location_id: stock_location.id, + return_items_attributes: [{ + inventory_unit_id: unit.id, + reception_status_event: "receive", + }] } + + post spree.api_order_customer_returns_path(order), params: { order_id: order.number, customer_return: cr_params } + + expect(response.status).to eq(201) + expect(json_response).to have_attributes(attributes) - customer_return = Spree::CustomerReturn.last + customer_return = Spree::CustomerReturn.last + + expect(customer_return.return_items.first.reception_status).to eql "received" + end + end - expect(customer_return.return_items.first.reception_status).to eql "received" end end end From 6bd1e40b1f50b9786ac8ae14e0928d81b804be67 Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Thu, 25 Mar 2021 15:05:07 -0700 Subject: [PATCH 2/4] Spec for customer return referencing existing items This change adds a pending test which demonstrates an issue with the `create` action when attempting to pass a reference to existing return items from a previously created return authorization. Rails does not allow us to update the association on the return items to the customer return while creating the return. Co-authored-by: Mike Conlin --- .../api/customer_returns_controller_spec.rb | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/api/spec/requests/spree/api/customer_returns_controller_spec.rb b/api/spec/requests/spree/api/customer_returns_controller_spec.rb index 8830d3210b5..68ce76694fa 100644 --- a/api/spec/requests/spree/api/customer_returns_controller_spec.rb +++ b/api/spec/requests/spree/api/customer_returns_controller_spec.rb @@ -133,6 +133,65 @@ module Spree end end + context "when referencing existing return items" do + subject do + post( + spree.api_order_customer_returns_path(order), + params: { + order_id: order.number, + customer_return: customer_return_params + } + ) + end + + let(:stock_location) { create(:stock_location) } + let(:inventory_unit) { create(:inventory_unit, state: "shipped") } + let(:order) { inventory_unit.order } + let(:return_item) do + create(:return_item, inventory_unit: inventory_unit) + end + + let(:customer_return_params) do + { + stock_location_id: stock_location.id, + return_items_attributes: [return_item.attributes] + } + end + + it "can create a new customer return" do + pending("fix for referrencing existing return items") + expect { subject }.to change { Spree::CustomerReturn.count }. + from(0).to(1) + + expect(response).to have_http_status(:success) + expect(json_response).to have_attributes(attributes) + end + + it "does not change the reception status of the return item" do + expect { subject }. + to_not change { return_item.reload.reception_status }. + from("awaiting") + end + + context "and return items attributes passed in as a hash of hashes" do + let(:customer_return_params) do + { + stock_location_id: stock_location.id, + return_items_attributes: { + "0" => return_item.attributes + } + } + end + + it "can create a new customer return" do + pending("fix for referrencing existing return items") + expect { subject }.to change { Spree::CustomerReturn.count }. + from(0).to(1) + + expect(response).to have_http_status(:success) + expect(json_response).to have_attributes(attributes) + end + end end end end From c99c98b4a0b97ebd30c124626b4b519cc09b2724 Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Mon, 29 Mar 2021 11:22:14 -0700 Subject: [PATCH 3/4] Allow referencing of existing return items in create This change pulls the `before_action` from the backend customer returns controller for parsing `return_items_attributes` in order to handle creating a new customer return which references existing return items from a return authorization. This change works around a limitation in Rails when trying to update the association on existing nested resource while creating the related record. Co-authored-by: Mike Conlin --- .../spree/api/customer_returns_controller.rb | 27 ++++++++++++++++++- .../api/customer_returns_controller_spec.rb | 19 +++++++++++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/api/app/controllers/spree/api/customer_returns_controller.rb b/api/app/controllers/spree/api/customer_returns_controller.rb index 6bc4b769896..ebcbc9f3814 100644 --- a/api/app/controllers/spree/api/customer_returns_controller.rb +++ b/api/app/controllers/spree/api/customer_returns_controller.rb @@ -4,13 +4,14 @@ module Spree module Api class CustomerReturnsController < Spree::Api::BaseController before_action :load_order + before_action :build_customer_return, only: [:create] around_action :lock_order, only: [:create, :update, :destroy, :cancel] rescue_from Spree::Order::InsufficientStock, with: :insufficient_stock_error def create authorize! :create, CustomerReturn - @customer_return = CustomerReturn.create(customer_return_params) + if @customer_return.save respond_with(@customer_return, status: 201, default_template: :show) else @@ -62,6 +63,30 @@ def load_order def customer_return_params params.require(:customer_return).permit(permitted_customer_return_attributes) end + + def build_customer_return + customer_return_attributes = customer_return_params + return_items_params = customer_return_attributes. + delete(:return_items_attributes) + + if return_items_params.is_a? ActionController::Parameters + return_items_params = return_items_params.values + end + + @customer_return = CustomerReturn.new(customer_return_attributes) + + @customer_return.return_items = return_items_params.map do |item_params| + return_item = if item_params[:id] + Spree::ReturnItem.find(item_params[:id]) + else + Spree::ReturnItem.new + end + + return_item.assign_attributes(item_params) + + return_item + end + end end end end diff --git a/api/spec/requests/spree/api/customer_returns_controller_spec.rb b/api/spec/requests/spree/api/customer_returns_controller_spec.rb index 68ce76694fa..6b8bd31b41c 100644 --- a/api/spec/requests/spree/api/customer_returns_controller_spec.rb +++ b/api/spec/requests/spree/api/customer_returns_controller_spec.rb @@ -159,7 +159,6 @@ module Spree end it "can create a new customer return" do - pending("fix for referrencing existing return items") expect { subject }.to change { Spree::CustomerReturn.count }. from(0).to(1) @@ -184,7 +183,6 @@ module Spree end it "can create a new customer return" do - pending("fix for referrencing existing return items") expect { subject }.to change { Spree::CustomerReturn.count }. from(0).to(1) @@ -192,6 +190,23 @@ module Spree expect(json_response).to have_attributes(attributes) end end + + context "with reception_status_event provided for return item" do + let(:customer_return_params) do + { + stock_location_id: stock_location.id, + return_items_attributes: [ + return_item.attributes.merge(reception_status_event: "receive") + ] + } + end + + it "updates the reception status of the return item" do + expect { subject }. + to change { return_item.reload.reception_status }. + from("awaiting").to("received") + end + end end end end From 00ffaf2d174ba0a21301c6f87d87db402020cfac Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Mon, 29 Mar 2021 13:37:58 -0700 Subject: [PATCH 4/4] Deprecate passing `return_items_attributes` in API as hash of hashes This is a behaviour which was previously undocumented worked because of the native Rails parameter parsing for nested attributes. This is not something we want to support through the API going forward so we are adding a deprecation warning for anyone using this behaviour currently. --- .../spree/api/customer_returns_controller.rb | 4 ++++ .../spree/api/customer_returns_controller_spec.rb | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/api/app/controllers/spree/api/customer_returns_controller.rb b/api/app/controllers/spree/api/customer_returns_controller.rb index ebcbc9f3814..9bbaeaf73d3 100644 --- a/api/app/controllers/spree/api/customer_returns_controller.rb +++ b/api/app/controllers/spree/api/customer_returns_controller.rb @@ -71,6 +71,10 @@ def build_customer_return if return_items_params.is_a? ActionController::Parameters return_items_params = return_items_params.values + Spree::Deprecation.warn( + "Passing `return_items_attributes` as a hash of hashes is \ +deprecated and will be removed in future versions of Solidus." + ) end @customer_return = CustomerReturn.new(customer_return_attributes) diff --git a/api/spec/requests/spree/api/customer_returns_controller_spec.rb b/api/spec/requests/spree/api/customer_returns_controller_spec.rb index 6b8bd31b41c..1f4706dbc40 100644 --- a/api/spec/requests/spree/api/customer_returns_controller_spec.rb +++ b/api/spec/requests/spree/api/customer_returns_controller_spec.rb @@ -183,12 +183,22 @@ module Spree end it "can create a new customer return" do + expect(Spree::Deprecation).to receive(:warn) expect { subject }.to change { Spree::CustomerReturn.count }. from(0).to(1) expect(response).to have_http_status(:success) expect(json_response).to have_attributes(attributes) end + + it "logs a deprecation warning" do + expect(Spree::Deprecation). + to receive(:warn). + with( + /Passing `return_items_attributes` as a hash of hashes is deprecated/ + ) + subject + end end context "with reception_status_event provided for return item" do