From 75d6ff5d037c7645fa319942aa7ccf72b4510f96 Mon Sep 17 00:00:00 2001 From: Alberto Vena Date: Mon, 16 May 2022 12:43:52 +0200 Subject: [PATCH 1/3] Handle empty values as quantity adding to cart via API Right now, when empty values are passed as quantity in the API call that creates line items, the line item is populated with quantity 0. This is not the expected behavior, as it makes no sense to have a line item with 0 as quantity. Still, if someone wants to force the value to 0, this is still possible but 0 needs to be explicitly passed as quantity value. A spec has been added for this scenario as well. --- .../spree/api/line_items_controller.rb | 2 +- api/spec/requests/spree/api/line_items_spec.rb | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/api/app/controllers/spree/api/line_items_controller.rb b/api/app/controllers/spree/api/line_items_controller.rb index e8ee1f0f01b..bd6e5c51528 100644 --- a/api/app/controllers/spree/api/line_items_controller.rb +++ b/api/app/controllers/spree/api/line_items_controller.rb @@ -13,7 +13,7 @@ def create variant = Spree::Variant.find(params[:line_item][:variant_id]) @line_item = @order.contents.add( variant, - params[:line_item][:quantity] || 1, + params[:line_item][:quantity].presence || 1, options: line_item_params[:options].to_h ) diff --git a/api/spec/requests/spree/api/line_items_spec.rb b/api/spec/requests/spree/api/line_items_spec.rb index bb092fa1171..85eab021747 100644 --- a/api/spec/requests/spree/api/line_items_spec.rb +++ b/api/spec/requests/spree/api/line_items_spec.rb @@ -95,13 +95,27 @@ module Spree::Api expect(response.status).to eq(201) end - it "default quantity to 1 if none is given" do + it "sets default quantity to 1 if none is given" do post spree.api_order_line_items_path(order), params: { line_item: { variant_id: product.master.to_param } } expect(response.status).to eq(201) expect(json_response).to have_attributes(attributes) expect(json_response[:quantity]).to eq 1 end + it "sets default quantity to 1 if empty values are given" do + post spree.api_order_line_items_path(order), params: { line_item: { variant_id: product.master.to_param, quantity: '' } } + expect(response.status).to eq(201) + expect(json_response).to have_attributes(attributes) + expect(json_response[:quantity]).to eq 1 + end + + it "allows to explicitly set quantity to 0" do + post spree.api_order_line_items_path(order), params: { line_item: { variant_id: product.master.to_param, quantity: 0 } } + expect(response.status).to eq(201) + expect(json_response).to have_attributes(attributes) + expect(json_response[:quantity]).to eq 0 + end + it "increases a line item's quantity if it exists already" do order.line_items.create(variant_id: product.master.id, quantity: 10) post spree.api_order_line_items_path(order), params: { line_item: { variant_id: product.master.to_param, quantity: 1 } } From cb2b878c21e970f31cb54978322e7215baf7c35f Mon Sep 17 00:00:00 2001 From: Ryan Woods Date: Mon, 24 Jul 2023 08:01:41 +0200 Subject: [PATCH 2/3] Improve OrderContents#update_cart testing for nested_attributes Its spec was not testing single-level nested_attributes, which in the codebase occurs in the Api::LineItemsController [1]. [1] https://github.com/solidusio/solidus/blob/df68c0c748ded6a595d28c37a9460126c6d12ecb/api/app/controllers/spree/api/line_items_controller.rb#L56-L62 --- core/spec/models/spree/order_contents_spec.rb | 70 +++++++++++++------ 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/core/spec/models/spree/order_contents_spec.rb b/core/spec/models/spree/order_contents_spec.rb index eb34512f870..ad6c193a32b 100644 --- a/core/spec/models/spree/order_contents_spec.rb +++ b/core/spec/models/spree/order_contents_spec.rb @@ -263,40 +263,66 @@ context "update cart" do let!(:shirt) { subject.add variant, 1 } - let(:params) do - { line_items_attributes: { - "0" => { id: shirt.id, quantity: 3 } - } } - end + context "with a multi-level nested params hash" do + let(:params) do + { line_items_attributes: { + "0" => { id: shirt.id, quantity: 3 } + } } + end - it "changes item quantity" do - subject.update_cart params - expect(shirt.reload.quantity).to eq 3 - end + it "changes item quantity" do + subject.update_cart params + expect(shirt.reload.quantity).to eq 3 + end - it "updates order totals" do - expect { + it "updates order totals" do + expect { + subject.update_cart params + }.to change { subject.order.total } + end + + context "submits item quantity 0" do + let(:params) do + { line_items_attributes: { + "0" => { id: shirt.id, quantity: 0 } + } } + end + + it "removes item from order" do + expect { + subject.update_cart params + }.to change { subject.order.line_items.count }.from(1).to(0) + end + end + + it "ensures updated shipments" do + expect(subject.order).to receive(:check_shipments_and_restart_checkout) subject.update_cart params - }.to change { subject.order.total } + end end - context "submits item quantity 0" do + context "with a single-level nested params hash" do let(:params) do { line_items_attributes: { - "0" => { id: shirt.id, quantity: 0 } + id: shirt.id, quantity: new_quantity } } end + let(:new_quantity) { 3 } - it "removes item from order" do - expect { - subject.update_cart params - }.to change { subject.order.line_items.count } + it "changes item quantity" do + subject.update_cart params + expect(shirt.reload.quantity).to eq 3 end - end - it "ensures updated shipments" do - expect(subject.order).to receive(:check_shipments_and_restart_checkout) - subject.update_cart params + context "submits item quantity 0" do + let(:new_quantity) { 0 } + + it "removes item from order" do + expect { + subject.update_cart params + }.to change { subject.order.line_items.count }.from(1).to(0) + end + end end end From 0d88daf966fb21b60ee0cd3a579649820e348866 Mon Sep 17 00:00:00 2001 From: Ryan Woods Date: Fri, 23 Jun 2023 14:56:00 +0200 Subject: [PATCH 3/3] Update OrderContents#update_cart This method allows the deletion of LineItems where the quantity is set to 0 or less. This is possible because LineItem allows a quantity of 0 on save, but soon this will be disallowed, so we have to manually destroy these LineItems and remove them from the params before saving. --- core/app/models/spree/order_contents.rb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/order_contents.rb b/core/app/models/spree/order_contents.rb index 5151df63030..bdaed7f9475 100644 --- a/core/app/models/spree/order_contents.rb +++ b/core/app/models/spree/order_contents.rb @@ -35,9 +35,10 @@ def remove_line_item(line_item, options = {}) end def update_cart(params) + params = destroy_non_positive_line_items(params) + if order.update(params) unless order.completed? - order.line_items = order.line_items.select { |li| li.quantity > 0 } # Update totals, then check if the order is eligible for any cart promotions. # If we do not update first, then the item total will be wrong and ItemTotal # promotion rules would not be triggered. @@ -83,6 +84,20 @@ def reload_totals @order.recalculate end + def destroy_non_positive_line_items(params) + line_items_to_destroy_ids = [] + # Could be a single LineItem or multiple via nesting + if id = params[:line_items_attributes][:id].present? + line_items_to_destroy_ids << id if params[:line_items_attributes][:quantity].to_i <= 0 + else + params[:line_items_attributes].reject! do |_index, attributes| + line_items_to_destroy_ids << attributes[:id] if attributes[:quantity].to_i <= 0 + end + end + order.line_items.where(id: line_items_to_destroy_ids).destroy_all + params + end + def add_to_line_item(variant, quantity, options = {}) line_item = grab_line_item_by_variant(variant, false, options)