From 593da4996f3b933c3e772aa6895048d686cdbb71 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 24 Jun 2022 16:39:37 +0100 Subject: [PATCH] A shop won't be shown as open if it doesn't have useable shipping method so these shipping method validations are really necessary --- app/models/order_cycle.rb | 3 +- app/models/order_cycle_shipping_method.rb | 32 ------ app/models/spree/shipping_method.rb | 17 --- config/locales/en.yml | 12 -- .../order_cycle_shipping_method_spec.rb | 82 -------------- spec/models/order_cycle_spec.rb | 17 +-- spec/models/spree/shipping_method_spec.rb | 104 ------------------ spec/services/order_cycle_form_spec.rb | 6 +- 8 files changed, 15 insertions(+), 258 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 0d49dd8d13..424b477e5f 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -179,7 +179,8 @@ class OrderCycle < ApplicationRecord oc.schedule_ids = schedule_ids oc.save! exchanges.each { |e| e.clone!(oc) } - oc.selected_shipping_method_ids = selected_shipping_method_ids + oc.selected_shipping_method_ids = attachable_shipping_methods.map(&:id) & + selected_shipping_method_ids sync_subscriptions oc.reload end diff --git a/app/models/order_cycle_shipping_method.rb b/app/models/order_cycle_shipping_method.rb index e2df849ad6..8b8f08d7ef 100644 --- a/app/models/order_cycle_shipping_method.rb +++ b/app/models/order_cycle_shipping_method.rb @@ -5,42 +5,10 @@ class OrderCycleShippingMethod < ApplicationRecord belongs_to :shipping_method, class_name: "Spree::ShippingMethod" validate :shipping_method_belongs_to_order_cycle_distributor - validate :shipping_method_available_at_checkout - validate :order_cycle_not_simple validates :shipping_method, uniqueness: { scope: :order_cycle_id } - before_destroy :check_shipping_method_not_selected_on_any_orders - private - def shipping_method_not_selected_on_any_orders? - Spree::Order.joins(shipments: :shipping_rates).where( - "order_cycle_id = ? AND spree_shipping_rates.shipping_method_id = ?", - order_cycle_id, shipping_method_id - ).empty? - end - - def check_shipping_method_not_selected_on_any_orders - return if order_cycle.nil? || - shipping_method.nil? || - shipping_method_not_selected_on_any_orders? - - errors.add(:base, :shipping_method_already_used_in_order_cycle) - throw :abort - end - - def order_cycle_not_simple - return if order_cycle.nil? || !order_cycle.simple? - - errors.add(:order_cycle, :must_not_be_simple) - end - - def shipping_method_available_at_checkout - return if shipping_method.nil? || shipping_method.frontend? - - errors.add(:shipping_method, :must_be_available_at_checkout) - end - def shipping_method_belongs_to_order_cycle_distributor return if order_cycle.nil? || shipping_method.nil? || diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index ad1f831fa8..589bc8ce8c 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -30,11 +30,8 @@ module Spree validates :name, presence: true validate :distributor_validation validate :at_least_one_shipping_category - validate :switching_to_backoffice_only_wont_leave_order_cycles_without_shipping_methods validates :display_on, inclusion: { in: DISPLAY_ON_OPTIONS.values }, allow_nil: true - before_destroy :check_destroy_wont_leave_order_cycles_without_shipping_methods - after_save :touch_distributors scope :managed_by, lambda { |user| @@ -140,19 +137,5 @@ module Spree def distributor_validation validates_with DistributorsValidator end - - def check_destroy_wont_leave_order_cycles_without_shipping_methods - return if no_active_or_upcoming_order_cycle_distributors_with_only_one_shipping_method? - - errors.add(:base, :destroy_leaves_order_cycles_without_shipping_methods) - throw :abort - end - - def switching_to_backoffice_only_wont_leave_order_cycles_without_shipping_methods - return if frontend? || - no_active_or_upcoming_order_cycle_distributors_with_only_one_shipping_method? - - errors.add(:base, :switching_to_backoffice_only_leaves_order_cycles_without_shipping_methods) - end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index 1321a077f4..014d8dd07f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -73,25 +73,13 @@ en: attributes: base: card_expired: "has expired" - spree/shipping_method: - attributes: - base: - switching_to_backoffice_only_leaves_order_cycles_without_shipping_methods: Unable to switch to backoffice only, some open or upcoming order cycles would be left without any shipping methods - destroy_leaves_order_cycles_without_shipping_methods: Unable to delete, some open or upcoming order cycles would be left without any shipping methods order_cycle: attributes: - base: - at_least_one_shipping_method_per_distributor: "You need to select at least one shipping method for each distributor" orders_close_at: after_orders_open_at: must be after open date order_cycle_shipping_method: attributes: - base: - shipping_method_already_used_in_order_cycle: "This shipping method has already been selected on orders in this order cycle and cannot be removed" - order_cycle: - must_not_be_simple: "is simple, all shipping methods are available by default and cannot be customised" shipping_method: - must_be_available_at_checkout: "must be available at checkout" must_belong_to_order_cycle_distributor: "must be from a distributor on the order cycle" variant_override: count_on_hand: diff --git a/spec/models/order_cycle_shipping_method_spec.rb b/spec/models/order_cycle_shipping_method_spec.rb index 352cee2565..4914313be7 100644 --- a/spec/models/order_cycle_shipping_method_spec.rb +++ b/spec/models/order_cycle_shipping_method_spec.rb @@ -3,55 +3,6 @@ require 'spec_helper' describe OrderCycleShippingMethod do - it "is valid when the shipping method is available at checkout" do - shipping_method = create(:shipping_method, display_on: nil) - enterprise = create(:enterprise, shipping_methods: [shipping_method]) - order_cycle = create(:simple_order_cycle, distributors: [enterprise]) - - order_cycle_shipping_method = OrderCycleShippingMethod.new( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - - expect(order_cycle_shipping_method).to be_valid - - shipping_method.display_on = "both" - - expect(order_cycle_shipping_method).to be_valid - end - - it "is not valid when the shipping method is only available in the backoffice" do - shipping_method = create(:shipping_method, display_on: "back_end") - enterprise = create(:enterprise, shipping_methods: [shipping_method]) - order_cycle = create(:simple_order_cycle, distributors: [enterprise]) - - order_cycle_shipping_method = OrderCycleShippingMethod.new( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - - expect(order_cycle_shipping_method).to_not be_valid - expect(order_cycle_shipping_method.errors.to_a).to include( - "Shipping method must be available at checkout" - ) - end - - it "is not valid if the order cycle is simple i.e. :sells is 'own'" do - order_cycle = create(:sells_own_order_cycle) - shipping_method = create(:shipping_method, distributors: [order_cycle.coordinator]) - - order_cycle_shipping_method = OrderCycleShippingMethod.new( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - - expect(order_cycle_shipping_method).to_not be_valid - expect(order_cycle_shipping_method.errors.to_a).to include( - "Order cycle is simple, all shipping methods are available by default and cannot be " \ - "customised" - ) - end - it "is valid if the shipping method belongs to one of the order cycle distributors" do shipping_method = create(:shipping_method) enterprise = create(:enterprise, shipping_methods: [shipping_method]) @@ -80,37 +31,4 @@ describe OrderCycleShippingMethod do "Shipping method must be from a distributor on the order cycle" ] end - - it "can be destroyed if the shipping method hasn't been used on any orders in the order cycle" do - shipping_method = create(:shipping_method) - enterprise = create(:enterprise, shipping_methods: [shipping_method]) - order_cycle = create(:simple_order_cycle, distributors: [enterprise]) - - order_cycle_shipping_method = OrderCycleShippingMethod.create!( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - order_cycle_shipping_method.destroy - - expect(order_cycle_shipping_method).to be_destroyed - end - - it "cannot be destroyed if the shipping method has been used on some orders in the order cycle" do - shipping_method = create(:shipping_method) - enterprise = create(:enterprise, shipping_methods: [shipping_method]) - order_cycle = create(:simple_order_cycle, distributors: [enterprise]) - order = create(:order_ready_for_payment, distributor: enterprise, order_cycle: order_cycle) - - order_cycle_shipping_method = OrderCycleShippingMethod.create!( - order_cycle: order_cycle, - shipping_method: shipping_method - ) - order_cycle_shipping_method.destroy - - expect(order_cycle_shipping_method).not_to be_destroyed - expect(order_cycle_shipping_method.errors.to_a).to eq [ - "This shipping method has already been selected on orders in this order cycle and cannot " \ - "be removed" - ] - end end diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 5d708d7751..9035b853e1 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -406,17 +406,20 @@ describe OrderCycle do context "when it has preferred shipping methods which can longer be applied validly e.g. shipping method is backoffice only" do - it "raises an error (TODO: display a message to user explaining why clone failed)" do + it "only attaches the valid ones to the clone" do distributor = create(:distributor_enterprise) - shipping_method = create(:shipping_method, distributors: [distributor]) + shipping_method_i = create(:shipping_method, distributors: [distributor]) + shipping_method_ii = create( + :shipping_method, + distributors: [distributor], + display_on: Spree::ShippingMethod::DISPLAY_ON_OPTIONS[:back_end] + ) order_cycle = create(:distributor_order_cycle, distributors: [distributor]) - order_cycle.selected_shipping_methods << shipping_method + order_cycle.selected_shipping_methods = [shipping_method_i, shipping_method_ii] - shipping_method.update_column(:display_on, "back_end") + cloned_order_cycle = order_cycle.clone! - expect { - order_cycle.clone! - }.to raise_error ActiveRecord::RecordInvalid + expect(cloned_order_cycle.shipping_methods).to eq [shipping_method_i] end end end diff --git a/spec/models/spree/shipping_method_spec.rb b/spec/models/spree/shipping_method_spec.rb index fb8c2651e2..d6ba024989 100644 --- a/spec/models/spree/shipping_method_spec.rb +++ b/spec/models/spree/shipping_method_spec.rb @@ -182,47 +182,6 @@ module Spree it { expect(shipping_method).to be_valid } end end - - context "when it is being changed to backoffice only" do - let!(:order_cycle) { create(:distributor_order_cycle, distributors: [distributor]) } - let(:distributor) { create(:distributor_enterprise, shipping_methods: [shipping_method]) } - let(:shipping_method) { create(:shipping_method) } - - context "when one of its distributors has no other shipping methods available - on an active or upcoming order cycle" do - it "should not be valid" do - shipping_method.display_on = "back_end" - - expect(shipping_method).not_to be_valid - expect(shipping_method.errors.to_a).to eq [ - "Unable to switch to backoffice only, some open or upcoming order cycles would be " \ - "left without any shipping methods" - ] - end - end - - context "when one of its distributors has no other shipping methods available - on an order cycle which isn't active or upcoming" do - it "is valid" do - order_cycle.update!(orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago) - - shipping_method.display_on = "back_end" - - expect(shipping_method).to be_valid - end - end - - context "when one of its distributors has other shipping methods available - on an active or upcoming order cycle" do - it "is valid" do - create(:shipping_method, distributors: [distributor]) - - shipping_method.display_on = "back_end" - - expect(shipping_method).to be_valid - end - end - end end # Regression test for Spree #4320 @@ -256,68 +215,5 @@ module Spree expect(shipping_method.shipments).to include(shipment) end end - - context "#destroy" do - let(:shipping_method) { create(:shipping_method) } - let(:distributor) { create(:distributor_enterprise, shipping_methods: [shipping_method]) } - let!(:order_cycle) { create(:distributor_order_cycle, distributors: [distributor]) } - - context "when its distributors are part of some order cycles - and have no other shipping methods available" do - it "can be deleted if the order cycle is closed" do - order_cycle.update!(orders_close_at: 1.minute.ago) - - shipping_method.destroy - - expect(shipping_method).to be_deleted - end - - it "cannot be deleted if the order cycle is active" do - order_cycle.update!(orders_open_at: 1.day.ago, orders_close_at: 1.week.from_now) - - shipping_method.destroy - - expect(shipping_method).not_to be_deleted - expect(shipping_method.errors.to_a).to eq [ - "Unable to delete, some open or upcoming order cycles would be left without any " \ - "shipping methods" - ] - end - - it "cannot be deleted if the order cycle is upcoming" do - order_cycle.update!(orders_open_at: 1.day.from_now, orders_close_at: 1.week.from_now) - - shipping_method.destroy - - expect(shipping_method).not_to be_deleted - expect(shipping_method.errors.to_a).to eq [ - "Unable to delete, some open or upcoming order cycles would be left without any " \ - "shipping methods" - ] - end - end - - context "when its distributors are part of some active or upcoming order cycles - and have other shipping methods available" do - it "can be deleted" do - create(:shipping_method, distributors: [distributor]) - - shipping_method.destroy - - expect(shipping_method).to be_deleted - end - end - - context "when its distributors have no other shipping methods available - but aren't part of active/upcoming order cycles" do - it "can be deleted" do - order_cycle.update!(orders_open_at: 2.weeks.ago, orders_close_at: 1.week.ago) - - shipping_method.destroy - - expect(shipping_method).to be_deleted - end - end - end end end diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index 9d831daba5..fc3a137030 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -247,15 +247,15 @@ describe OrderCycleForm do shipping_method_i = create(:shipping_method, distributors: [distributor_i]) shipping_method_ii = create(:shipping_method, distributors: [distributor_ii]) order_cycle = create(:distributor_order_cycle, - distributors: [distributor_i, distributor_ii]) + distributors: [distributor_i]) form = OrderCycleForm.new(order_cycle, - { selected_shipping_method_ids: [shipping_method_i.id] }, + { selected_shipping_method_ids: [shipping_method_ii.id] }, order_cycle.coordinator) expect(form.save).to be false expect(order_cycle.errors.to_a).to eq [ - "You need to select at least one shipping method for each distributor" + "Shipping method must be from a distributor on the order cycle" ] end end