From ca97a7f52dd20d898477f0a5211a9b6a3e08dd5b Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Wed, 8 Jun 2022 20:25:46 +0100 Subject: [PATCH] Add :shipping_methods association to OrderCyle and validations --- app/models/order_cycle.rb | 35 ++++++++++- config/locales/en.yml | 2 + spec/factories/order_cycle_factory.rb | 27 +++++++++ spec/models/order_cycle_spec.rb | 83 ++++++++++++++++++++++++--- 4 files changed, 138 insertions(+), 9 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 91c9124a21..86f1a14a54 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -22,17 +22,23 @@ class OrderCycle < ApplicationRecord has_many :suppliers, -> { distinct }, source: :sender, through: :cached_incoming_exchanges has_many :distributors, -> { distinct }, source: :receiver, through: :cached_outgoing_exchanges - has_many :order_cycle_schedules has_many :schedules, through: :order_cycle_schedules + has_many :order_cycle_shipping_methods + has_many :shipping_methods, class_name: "Spree::ShippingMethod", + through: :order_cycle_shipping_methods has_paper_trail meta: { custom_data: proc { |order_cycle| order_cycle.schedule_ids.to_s } } attr_accessor :incoming_exchanges, :outgoing_exchanges + attribute :validate_shipping_methods, :boolean, default: true + before_update :reset_processed_at, if: :will_save_change_to_orders_close_at? after_save :sync_subscriptions, if: :opening? validates :name, :coordinator_id, presence: true + validate :at_least_one_shipping_method_selected_for_each_distributor + validate :no_invalid_shipping_methods validate :orders_close_at_after_orders_open_at? preference :product_selection_from_coordinator_inventory_only, :boolean, default: false @@ -159,8 +165,12 @@ class OrderCycle < ApplicationRecord oc.preferred_product_selection_from_coordinator_inventory_only = preferred_product_selection_from_coordinator_inventory_only # rubocop:enable Layout/LineLength oc.schedule_ids = schedule_ids + oc.shipping_methods_customisable = true oc.save! + oc.validate_shipping_methods = false exchanges.each { |e| e.clone!(oc) } + oc.validate_shipping_methods = true + oc.shipping_method_ids = shipping_method_ids sync_subscriptions oc.reload end @@ -280,6 +290,29 @@ class OrderCycle < ApplicationRecord private + def all_distributors_have_at_least_one_shipping_method? + distributors.all? { |distributor| (distributor.shipping_method_ids & shipping_method_ids).any? } + end + + def at_least_one_shipping_method_selected_for_each_distributor + return if !validate_shipping_methods? || + coordinator.nil? || + simple? || + !shipping_methods_customisable? || + all_distributors_have_at_least_one_shipping_method? + + errors.add(:base, :at_least_one_shipping_method_per_distributor) + end + + def no_invalid_shipping_methods + return if order_cycle_shipping_methods.all?(&:valid?) + + errors.add( + :base, + order_cycle_shipping_methods.map(&:errors).map(&:to_a).flatten.uniq.to_sentence + ) + end + def opening? (open? || upcoming?) && saved_change_to_orders_close_at? && was_closed? end diff --git a/config/locales/en.yml b/config/locales/en.yml index 1feb920654..de8bdb19ac 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -75,6 +75,8 @@ en: card_expired: "has expired" 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: diff --git a/spec/factories/order_cycle_factory.rb b/spec/factories/order_cycle_factory.rb index 469fb94019..3ad85ef7e5 100644 --- a/spec/factories/order_cycle_factory.rb +++ b/spec/factories/order_cycle_factory.rb @@ -72,12 +72,15 @@ FactoryBot.define do coordinator { Enterprise.is_distributor.first || FactoryBot.create(:distributor_enterprise) } transient do + shipping_methods { [] } suppliers { [] } distributors { [] } variants { [] } + with_distributor_and_shipping_method { false } end after(:create) do |oc, proxy| + # Incoming Exchanges proxy.suppliers.each.with_index do |supplier, i| ex = create(:exchange, order_cycle: oc, @@ -88,6 +91,10 @@ FactoryBot.define do proxy.variants.each { |v| ex.variants << v } end + if proxy.with_distributor_and_shipping_method + proxy.distributors << oc.coordinator if proxy.distributors.empty? + end + # Outgoing Exchanges proxy.distributors.each.with_index do |distributor, i| ex = create(:exchange, order_cycle: oc, @@ -98,6 +105,26 @@ FactoryBot.define do pickup_instructions: "instructions #{i}") proxy.variants.each { |v| ex.variants << v } end + + if proxy.with_distributor_and_shipping_method || proxy.shipping_methods.any? + oc.reload # so outgoing exchanges/distributors attached above are present + distributor = oc.distributors.first + if proxy.shipping_methods.empty? + proxy.shipping_methods << create(:shipping_method, distributors: [distributor]) + else + proxy.shipping_methods.each do |shipping_method| + # ensure shipping methods belong to a distributor on the order cycle + if !shipping_method.distributors.include?(distributor) + shipping_method.distributors << distributor + end + end + end + oc.shipping_methods = proxy.shipping_methods + end + + if proxy.distributors.any? || proxy.shipping_methods.any? + oc.reload # so shipping methods attached above are present + end end end diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 27cea63be5..16d21fdabd 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -20,6 +20,72 @@ describe OrderCycle do expect(oc).to_not be_valid end + describe "#at_least_one_shipping_method_selected_for_each_distributor" do + context "distributor order cycle i.e. :sells is 'any'" do + context "when multiple distributors have been added to the order cycle already" do + it "is valid when adding a shipping method for *all* distributors" do + distributor_i = create(:distributor_enterprise) + distributor_ii = create(:distributor_enterprise) + distributor_i_shipping_method = create(:shipping_method, distributors: [distributor_i]) + distributor_ii_shipping_method = create(:shipping_method, distributors: [distributor_ii]) + order_cycle = create(:distributor_order_cycle, distributors: [distributor_i, distributor_ii]) + + order_cycle.shipping_method_ids = [distributor_i_shipping_method.id, distributor_ii_shipping_method.id] + + expect(order_cycle).to be_valid + end + end + + it "is not valid when adding a shipping method for *some but not all* distributors" do + distributor_i = create(:distributor_enterprise) + distributor_ii = create(:distributor_enterprise) + distributor_i_shipping_method = create(:shipping_method, distributors: [distributor_i]) + distributor_ii_shipping_method = create(:shipping_method, distributors: [distributor_i]) + order_cycle = create(:distributor_order_cycle, distributors: [distributor_i, distributor_ii]) + + order_cycle.shipping_method_ids = [distributor_i_shipping_method.id] + + expect(order_cycle).to be_invalid + expect(order_cycle.errors.to_a).to eq ["You need to select at least one shipping method for each distributor"] + end + end + end + + describe "#no_invalid_shipping_methods" do + context "when a shipping method is not valid" do + it "adds a validation error, and it is more meaningful than the default 'Order cycle shipping methods is invalid'" do + order_cycle = create(:distributor_order_cycle, with_distributor_and_shipping_method: true) + shipping_method = order_cycle.shipping_methods.first + + shipping_method.update_column(:display_on, "back_end") + + expect(order_cycle).to be_invalid + expect(order_cycle.errors.to_a).to eq ["Shipping method must be available at checkout"] + + shipping_method.update_column(:display_on, "") + + expect(order_cycle.reload).to be_valid + end + end + end + + describe "#validate_shipping_methods" do + it "doesn't skip shipping method validations by default" do + order_cycle = create(:distributor_order_cycle, distributors: [create(:distributor_enterprise)]) + + expect(order_cycle).to be_invalid + expect(order_cycle.errors.to_a).to eq ["You need to select at least one shipping method for each distributor"] + end + + it "allows shipping method validations to be skipped because distributors need to be saved before shipping methods" do + order_cycle = create(:distributor_order_cycle, distributors: [create(:distributor_enterprise)]) + + order_cycle.validate_shipping_methods = false + + expect(order_cycle).to be_valid + end + end + it "has a coordinator and associated fees" do oc = create(:simple_order_cycle) @@ -132,7 +198,7 @@ describe OrderCycle do end it "reports its distributors" do - oc = create(:simple_order_cycle) + oc = create(:simple_order_cycle, validate_shipping_methods: false) e1 = create(:exchange, incoming: false, order_cycle: oc, sender: oc.coordinator, receiver: create(:enterprise)) @@ -143,7 +209,7 @@ describe OrderCycle do end it "checks for existance of distributors" do - oc = create(:simple_order_cycle) + oc = create(:simple_order_cycle, validate_shipping_methods: false) d1 = create(:distributor_enterprise) d2 = create(:distributor_enterprise) create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, incoming: false) @@ -506,7 +572,7 @@ describe OrderCycle do end describe "version tracking", versioning: true do - let!(:oc) { create(:order_cycle, name: "Original") } + let!(:oc) { create(:sells_own_order_cycle, name: "Original") } it "remembers old versions" do expect { @@ -551,11 +617,12 @@ describe OrderCycle do end describe "syncing subscriptions" do - let!(:oc) { - create(:simple_order_cycle, orders_open_at: 1.week.ago, orders_close_at: 1.day.ago) - } - let(:schedule) { create(:schedule, order_cycles: [oc]) } - let!(:subscription) { create(:subscription, schedule: schedule, with_items: true) } + let!(:subscription) { create(:subscription, with_items: true) } + let!(:oc) { subscription.order_cycles.first } + + before do + oc.update_columns(orders_open_at: 1.week.ago, orders_close_at: 1.day.ago) + end it "syncs subscriptions when transitioning from closed to open" do expect(OrderManagement::Subscriptions::ProxyOrderSyncer).to receive(:new).and_call_original