From 6f8ade52eb38bfc351da0e289d5512919b4bfc30 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 27 Mar 2021 14:22:24 +0000 Subject: [PATCH 01/10] Persist last_ip_address when loading current_order Leaving the object with unpersisted changes breaks order locking with this error (in various places): RuntimeError: Locking a record with unpersisted changes is not supported. Use `save` to persist the changes, or `reload` to discard them explicitly. --- lib/spree/core/controller_helpers/order.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index d3f093cfd2..e0f7423613 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -52,7 +52,7 @@ module Spree return unless @current_order - @current_order.last_ip_address = ip_address + @current_order.update_columns(last_ip_address: ip_address) session[:order_id] = @current_order.id @current_order end From 72c6935ff8ff4fa586cd0185e4b2f54b0975f26e Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 27 Mar 2021 14:54:27 +0000 Subject: [PATCH 02/10] Fix handling of empty order cycle params This guard clause was returning an instance of an unpermitted Params object (containing a blank hash) here, which was causing unexpected results in various places. Returning a blank hash explicitly resolved the issue. Fixes: 4) Admin::OrderCyclesController create as a manager of a shop when creation is successful returns success: true and a valid edit path Failure/Error: @order_cycle_params ||= PermittedAttributes::OrderCycle.new(params).call. to_h.with_indifferent_access ActionController::UnfilteredParameters: unable to convert unpermitted parameters to hash # ./app/controllers/admin/order_cycles_controller.rb:245:in `order_cycle_params' # ./app/controllers/admin/order_cycles_controller.rb:44:in `create' # ./spec/support/controller_requests_helper.rb:49:in `process_action_with_route' # ./spec/support/controller_requests_helper.rb:27:in `spree_post' # ./spec/controllers/admin/order_cycles_controller_spec.rb:124:in `block (5 levels) in ' --- app/services/permitted_attributes/order_cycle.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb index 046726a96f..f46a4c63d2 100644 --- a/app/services/permitted_attributes/order_cycle.rb +++ b/app/services/permitted_attributes/order_cycle.rb @@ -7,7 +7,7 @@ module PermittedAttributes end def call - return @params[:order_cycle] if @params[:order_cycle].blank? + return {} if @params[:order_cycle].blank? @params.require(:order_cycle).permit(attributes) end From 7fc29961e2b774f9a536e24979788d11ddae6d16 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 27 Mar 2021 14:56:45 +0000 Subject: [PATCH 03/10] Fix handling of other empty params --- app/services/permitted_attributes/enterprise.rb | 2 +- app/services/permitted_attributes/subscription.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/permitted_attributes/enterprise.rb b/app/services/permitted_attributes/enterprise.rb index 1f20a62b13..0989511d69 100644 --- a/app/services/permitted_attributes/enterprise.rb +++ b/app/services/permitted_attributes/enterprise.rb @@ -7,7 +7,7 @@ module PermittedAttributes end def call - return @params[:enterprise] if @params[:enterprise].blank? + return {} if @params[:enterprise].blank? @params.require(:enterprise).permit(self.class.attributes) end diff --git a/app/services/permitted_attributes/subscription.rb b/app/services/permitted_attributes/subscription.rb index 21e523e4fe..e80469767f 100644 --- a/app/services/permitted_attributes/subscription.rb +++ b/app/services/permitted_attributes/subscription.rb @@ -7,7 +7,7 @@ module PermittedAttributes end def call - return @params[:subscription] if @params[:subscription].blank? + return {} if @params[:subscription].blank? @params.require(:subscription).permit(basic_permitted_attributes + other_permitted_attributes) end From 0707f3c0ccf3b5875d6843560a448a57ad12b325 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Thu, 8 Apr 2021 00:03:53 +0100 Subject: [PATCH 04/10] Switch from map, that does not exist in ActionController::Parameters, to each --- app/services/cart_service.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/services/cart_service.rb b/app/services/cart_service.rb index 35dcb720f0..c4f3b5a5c0 100644 --- a/app/services/cart_service.rb +++ b/app/services/cart_service.rb @@ -106,13 +106,15 @@ class CartService end def read_variants_hash(data) - (data[:variants] || []).map do |variant_id, quantity| - if quantity.is_a?(Hash) - { variant_id: variant_id, quantity: quantity[:quantity], max_quantity: quantity[:max_quantity] } + variants_array = [] + (data[:variants] || []).each do |variant_id, quantity| + if quantity.is_a?(ActionController::Parameters) + variants_array.push({ variant_id: variant_id, quantity: quantity[:quantity], max_quantity: quantity[:max_quantity] }) else - { variant_id: variant_id, quantity: quantity } + variants_array.push({ variant_id: variant_id, quantity: quantity }) end end + variants_array end def cart_remove(variant_id) From 6fab8db1b0b753a07762143cf01b1c819a07a645 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Thu, 8 Apr 2021 21:26:01 +0100 Subject: [PATCH 05/10] Fix cart_service_spec to adapt to the actual usage of it from cart_controller, params are now sent inside a Parameters object, not as a hash --- spec/services/cart_service_spec.rb | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/spec/services/cart_service_spec.rb b/spec/services/cart_service_spec.rb index b8144f867b..51e8f2b882 100644 --- a/spec/services/cart_service_spec.rb +++ b/spec/services/cart_service_spec.rb @@ -27,7 +27,8 @@ describe CartService do describe "#populate" do it "adds a variant" do cart_service.populate( - { variants: { variant.id.to_s => { quantity: '1', max_quantity: '2' } } }, + ActionController::Parameters.new({ variants: { variant.id.to_s => { quantity: '1', + max_quantity: '2' } } }), true ) li = order.find_line_item_by_variant(variant) @@ -41,7 +42,8 @@ describe CartService do order.add_variant variant, 1, 2 cart_service.populate( - { variants: { variant.id.to_s => { quantity: '2', max_quantity: '3' } } }, + ActionController::Parameters.new({ variants: { variant.id.to_s => { quantity: '2', + max_quantity: '3' } } }), true ) li = order.find_line_item_by_variant(variant) @@ -54,7 +56,7 @@ describe CartService do it "removes a variant" do order.add_variant variant, 1, 2 - cart_service.populate({ variants: {} }, true) + cart_service.populate(ActionController::Parameters.new({ variants: {} }), true) order.line_items.reload li = order.find_line_item_by_variant(variant) expect(li).not_to be @@ -67,7 +69,7 @@ describe CartService do it "does not add the deleted variant to the cart" do variant.delete - cart_service.populate({ variants: { variant.id.to_s => { quantity: '2' } } }, true) + cart_service.populate(ActionController::Parameters.new({ variants: { variant.id.to_s => { quantity: '2' } } }), true) expect(relevant_line_item).to be_nil expect(cart_service.errors.count).to be 0 @@ -82,7 +84,7 @@ describe CartService do it "removes the line_item from the cart" do variant.delete - cart_service.populate({ variants: { variant.id.to_s => { quantity: '3' } } }, true) + cart_service.populate(ActionController::Parameters.new({ variants: { variant.id.to_s => { quantity: '3' } } }), true) expect(Spree::LineItem.where(id: relevant_line_item).first).to be_nil expect(cart_service.errors.count).to be 0 From 3e14138a3fa6f6e74939fc7aaaf9e79830d14696 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Fri, 9 Apr 2021 16:24:36 -0700 Subject: [PATCH 06/10] fix ./spec/features/admin/customers_spec.rb:27 --- app/models/customer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index c1af111f4f..71025435ff 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -46,6 +46,6 @@ class Customer < ActiveRecord::Base return true unless orders.any? errors[:base] << I18n.t('admin.customers.destroy.has_associated_orders') - false + throw :abort end end From 4590db1cc2277945e47cc7d12d00490086d553de Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 10 Apr 2021 10:35:50 +0100 Subject: [PATCH 07/10] Fix view spec by adding mocks to helper methods --- spec/views/spree/admin/orders/invoice.html.haml_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/views/spree/admin/orders/invoice.html.haml_spec.rb b/spec/views/spree/admin/orders/invoice.html.haml_spec.rb index 32c3dd169d..8b49a142e8 100644 --- a/spec/views/spree/admin/orders/invoice.html.haml_spec.rb +++ b/spec/views/spree/admin/orders/invoice.html.haml_spec.rb @@ -21,6 +21,10 @@ describe "spree/admin/orders/invoice.html.haml" do before do assign(:order, order) + allow(view).to receive_messages checkout_adjustments_for: [], + display_checkout_tax_total: '10', + display_checkout_total_less_tax: '8', + outstanding_balance_label: 'Outstanding Balance' end it "displays the customer code" do From ebb413bd83f13dbd61aad49fa60de21c417906b4 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Fri, 9 Apr 2021 10:34:42 -0700 Subject: [PATCH 08/10] only reload payments after updating order --- app/models/spree/payment.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index a1f635db45..62cd64deaa 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -201,7 +201,8 @@ module Spree end def update_order - order.reload.update! + order.payments.reload + order.update! end # Necessary because some payment gateways will refuse payments with From a4cb698d6fda5b6b573d3710d94671b05073fe06 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 11 Apr 2021 12:44:23 +0100 Subject: [PATCH 09/10] Don't reload payments in after_save callback --- app/models/spree/payment.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 62cd64deaa..185a253f6d 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -201,7 +201,6 @@ module Spree end def update_order - order.payments.reload order.update! end From fa14a802950be7cbc87d8df1f7545967a0c2e513 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 11 Apr 2021 13:09:49 +0100 Subject: [PATCH 10/10] Adapt order payment creation in failing specs In the test setups here order.payments is empty --- spec/controllers/api/v0/orders_controller_spec.rb | 2 +- spec/features/admin/orders_spec.rb | 2 +- spec/models/spree/order_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/controllers/api/v0/orders_controller_spec.rb b/spec/controllers/api/v0/orders_controller_spec.rb index cfae14f85a..4a76605138 100644 --- a/spec/controllers/api/v0/orders_controller_spec.rb +++ b/spec/controllers/api/v0/orders_controller_spec.rb @@ -274,7 +274,7 @@ module Api before do order.finalize! - create(:check_payment, order: order, amount: order.total) + order.payments << create(:check_payment, order: order, amount: order.total) allow(controller).to receive(:spree_current_user) { order.distributor.owner } end diff --git a/spec/features/admin/orders_spec.rb b/spec/features/admin/orders_spec.rb index 7408d53005..63e9feb687 100644 --- a/spec/features/admin/orders_spec.rb +++ b/spec/features/admin/orders_spec.rb @@ -65,7 +65,7 @@ feature ' context "with a capturable order" do before do order.finalize! # ensure order has a payment to capture - create :check_payment, order: order, amount: order.total + order.payments << create(:check_payment, order: order, amount: order.total) end scenario "capture payment" do diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 3a1b6988bd..a143290fe7 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1174,7 +1174,7 @@ describe Spree::Order do it "advances to complete state without error" do advance_to_delivery_state(order) order.next! - create(:payment, order: order) + order.payments << create(:payment, order: order) expect { order.next! }.to change { order.state }.from("payment").to("complete") end