diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 84800086c9..c552c4e45c 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -25,20 +25,19 @@ class OrderCycle < ApplicationRecord 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_many :preferred_shipping_methods, class_name: "Spree::ShippingMethod", + through: :order_cycle_shipping_methods, + source: :shipping_method 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 :no_invalid_order_cycle_shipping_methods validate :orders_close_at_after_orders_open_at? preference :product_selection_from_coordinator_inventory_only, :boolean, default: false @@ -165,8 +164,6 @@ class OrderCycle < ApplicationRecord end def attachable_shipping_methods - return Spree::ShippingMethod.none if simple? || !shipping_methods_customisable? - Spree::ShippingMethod.frontend. joins(:distributor_shipping_methods). where("distributor_id IN (?)", distributor_ids). @@ -182,12 +179,9 @@ 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 + oc.preferred_shipping_method_ids = preferred_shipping_method_ids sync_subscriptions oc.reload end @@ -301,6 +295,14 @@ class OrderCycle < ApplicationRecord items.each { |li| scoper.scope(li.variant) } end + def shipping_methods + if simple? || preferred_shipping_methods.none? + attachable_shipping_methods + else + preferred_shipping_methods + end + end + def simple? coordinator.sells == 'own' end @@ -308,20 +310,21 @@ class OrderCycle < ApplicationRecord private def all_distributors_have_at_least_one_shipping_method? - distributors.all? { |distributor| (distributor.shipping_method_ids & shipping_method_ids).any? } + distributors.all? do |distributor| + (distributor.shipping_method_ids & preferred_shipping_method_ids).any? + end end def at_least_one_shipping_method_selected_for_each_distributor - return if !validate_shipping_methods? || + return if preferred_shipping_methods.none? || 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 + def no_invalid_order_cycle_shipping_methods return if order_cycle_shipping_methods.all?(&:valid?) errors.add( diff --git a/app/models/order_cycle_shipping_method.rb b/app/models/order_cycle_shipping_method.rb index dbdd0ccd0d..ebb0420660 100644 --- a/app/models/order_cycle_shipping_method.rb +++ b/app/models/order_cycle_shipping_method.rb @@ -7,7 +7,6 @@ class OrderCycleShippingMethod < ActiveRecord::Base validate :shipping_method_belongs_to_order_cycle_distributor validate :shipping_method_available_at_checkout validate :order_cycle_not_simple - validate :order_cycle_shipping_methods_customisable validates_uniqueness_of :shipping_method, scope: :order_cycle_id before_destroy :check_shipping_method_not_selected_on_any_orders @@ -36,12 +35,6 @@ class OrderCycleShippingMethod < ActiveRecord::Base errors.add(:order_cycle, :must_not_be_simple) end - def order_cycle_shipping_methods_customisable - return if order_cycle.nil? || order_cycle.shipping_methods_customisable? - - errors.add(:order_cycle, :must_support_customisable_shipping_methods) - end - def shipping_method_available_at_checkout return if shipping_method.nil? || shipping_method.frontend? diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index 7a7eb44487..b7bd7e4092 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -115,18 +115,14 @@ module Spree private - def no_active_or_upcoming_non_simple_order_cycles_with_only_one_shipping_method? + def no_active_or_upcoming_order_cycle_distributors_with_only_one_shipping_method? return true if new_record? - OrderCycle.active.or(OrderCycle.upcoming).joins(:coordinator, :shipping_methods).where(" - sells != 'own' AND - spree_shipping_methods.id = ? AND - NOT EXISTS( - SELECT 1 - FROM order_cycle_shipping_methods - WHERE order_cycle_id = order_cycles.id AND - shipping_method_id != ? - )", id, id).none? + distributors. + with_order_cycles_as_distributor_outer. + merge(OrderCycle.active.or(OrderCycle.upcoming)).none? do |distributor| + distributor.shipping_method_ids.one? + end end def at_least_one_shipping_category @@ -146,7 +142,7 @@ module Spree end def check_destroy_wont_leave_order_cycles_without_shipping_methods - return if no_active_or_upcoming_non_simple_order_cycles_with_only_one_shipping_method? + 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 @@ -154,7 +150,7 @@ module Spree def switching_to_backoffice_only_wont_leave_order_cycles_without_shipping_methods return if frontend? || - no_active_or_upcoming_non_simple_order_cycles_with_only_one_shipping_method? + 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 diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index ba6aa590e7..18e07553a8 100644 --- a/app/services/order_cycle_form.rb +++ b/app/services/order_cycle_form.rb @@ -11,13 +11,12 @@ class OrderCycleForm @user = user @permissions = OpenFoodNetwork::Permissions.new(user) @schedule_ids = order_cycle_params.delete(:schedule_ids) - @shipping_method_ids = order_cycle_params.delete(:shipping_method_ids) + @preferred_shipping_method_ids = order_cycle_params.delete(:preferred_shipping_method_ids) end def save schedule_ids = build_schedule_ids order_cycle.assign_attributes(order_cycle_params) - order_cycle.validate_shipping_methods = false return false unless order_cycle.valid? order_cycle.transaction do @@ -25,7 +24,7 @@ class OrderCycleForm order_cycle.schedule_ids = schedule_ids order_cycle.save! apply_exchange_changes - attach_shipping_methods + attach_preferred_shipping_methods sync_subscriptions true end @@ -49,12 +48,11 @@ class OrderCycleForm OpenFoodNetwork::OrderCycleFormApplicator.new(order_cycle, user).go! end - def attach_shipping_methods - return if @shipping_method_ids.nil? + def attach_preferred_shipping_methods + return if @preferred_shipping_method_ids.nil? order_cycle.reload # so outgoing exchanges are up-to-date for shipping method validations - order_cycle.validate_shipping_methods = true - order_cycle.shipping_method_ids = @shipping_method_ids + order_cycle.preferred_shipping_method_ids = preferred_shipping_method_ids order_cycle.save! end @@ -64,6 +62,16 @@ class OrderCycleForm end end + def preferred_shipping_method_ids + @preferred_shipping_method_ids = @preferred_shipping_method_ids.reject(&:blank?).map(&:to_i) + + if order_cycle.attachable_shipping_methods.map(&:id).sort == @preferred_shipping_method_ids.sort + @preferred_shipping_method_ids = [] + end + + @preferred_shipping_method_ids + end + def schedule_ids? @schedule_ids.present? end diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb index 97282acabf..8f5998b70d 100644 --- a/app/services/permitted_attributes/order_cycle.rb +++ b/app/services/permitted_attributes/order_cycle.rb @@ -17,7 +17,7 @@ module PermittedAttributes :name, :orders_open_at, :orders_close_at, :coordinator_id, :preferred_product_selection_from_coordinator_inventory_only, :automatic_notifications, - { schedule_ids: [], shipping_method_ids: [], coordinator_fee_ids: [] } + { schedule_ids: [], preferred_shipping_method_ids: [], coordinator_fee_ids: [] } ] end diff --git a/app/views/admin/order_cycles/checkout_options.html.haml b/app/views/admin/order_cycles/checkout_options.html.haml index 97f436308c..ca76377eda 100644 --- a/app/views/admin/order_cycles/checkout_options.html.haml +++ b/app/views/admin/order_cycles/checkout_options.html.haml @@ -13,6 +13,8 @@ %fieldset.no-border-bottom %legend{ align: 'center'}= t('.checkout_options') + = hidden_field_tag "order_cycle[preferred_shipping_method_ids][]", "" + %table.checkout-options %thead %tr diff --git a/config/locales/en.yml b/config/locales/en.yml index ef52faec39..1321a077f4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -90,7 +90,6 @@ en: 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" - must_support_customisable_shipping_methods: "shipping methods cannot be customised, all shipping methods are available by default" 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" diff --git a/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb b/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb index ae78acc94a..cc16a501d2 100644 --- a/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb +++ b/db/migrate/20220429092052_create_order_cycle_shipping_methods.rb @@ -1,98 +1,17 @@ class CreateOrderCycleShippingMethods < ActiveRecord::Migration[6.1] - # Before this migration every available shipping method was available to customers on order - # cycles by default. However this migration only populates :order_cycles_shipping_methods records - # for active or upcoming order cycles because retroactively calculating which shipping methods - # should be attached to past, closed order cycles is probaby tricky so skipping that because it - # may not be even necessary. Instead this adds a :shipping_methods_customisable flag to order - # cycles so we have a record of order cycles created before this feature was deployed. - # - # Note: Redefining the Spree::ShippingMethod class in this migration as suggested by the - # :good_migrations gem was not passing Good Migrations checks. This redefines the classes inside - # a Migration class to to bypass this problem. - - class Migration - class DistributorShippingMethod < ActiveRecord::Base - self.table_name = "distributors_shipping_methods" - belongs_to :shipping_method, class_name: "Migration::ShippingMethod", touch: true - belongs_to :distributor, class_name: "Enterprise", touch: true - end - - class Enterprise < ActiveRecord::Base - end - - class Exchange < ActiveRecord::Base - self.table_name = "exchanges" - belongs_to :receiver, class_name: 'Migration::Enterprise' - end - - class OrderCycle < ActiveRecord::Base - self.table_name = "order_cycles" - has_many :order_cycle_shipping_methods, class_name: "Migration::OrderCycleShippingMethod" - has_many :shipping_methods, class_name: "Migration::ShippingMethod", through: :order_cycle_shipping_methods - - has_many :cached_outgoing_exchanges, -> { where incoming: false }, class_name: "Migration::Exchange" - has_many :distributors, -> { distinct }, source: :receiver, through: :cached_outgoing_exchanges - - belongs_to :coordinator, class_name: 'Migration::Enterprise' - - scope :active, lambda { - where('order_cycles.orders_open_at <= ? AND order_cycles.orders_close_at >= ?', - Time.zone.now, - Time.zone.now) - } - scope :upcoming, lambda { where('order_cycles.orders_open_at > ?', Time.zone.now) } - end - - class OrderCycleShippingMethod < ActiveRecord::Base - self.table_name = "order_cycle_shipping_methods" - belongs_to :shipping_method, class_name: "Migration::ShippingMethod" - end - - class ShippingMethod < ActiveRecord::Base - self.table_name = "spree_shipping_methods" - end - - def self.attach_all_shipping_methods_to_non_simple_active_or_upcoming_order_cycles - non_simple_active_or_upcoming_order_cycles.find_each do |order_cycle| - order_cycle.shipping_method_ids = DistributorShippingMethod. - where("display_on != 'back_end'"). - where(distributor_id: order_cycle.distributor_ids). - joins(:shipping_method). - pluck(:shipping_method_id) - end - end - - def self.set_shipping_methods_customisable_to_false_on_past_order_cycles - OrderCycle.update_all(shipping_methods_customisable: false) - active_or_upcoming_order_cycles.update_all(shipping_methods_customisable: true) - end - - private - - def self.active_or_upcoming_order_cycles - OrderCycle.active.or(OrderCycle.upcoming) - end - - def self.non_simple_active_or_upcoming_order_cycles - active_or_upcoming_order_cycles.joins(:coordinator).where("sells != 'own'") - end - end - def up create_table :order_cycle_shipping_methods do |t| t.references :order_cycle t.references :shipping_method, foreign_key: { to_table: :spree_shipping_methods } t.timestamps end - - add_column :order_cycles, :shipping_methods_customisable, :boolean, default: true - - Migration.set_shipping_methods_customisable_to_false_on_past_order_cycles - Migration.attach_all_shipping_methods_to_non_simple_active_or_upcoming_order_cycles + add_index :order_cycle_shipping_methods, + [:order_cycle_id, :shipping_method_id], + name: "order_cycle_shipping_methods_join_index", + unique: true end def down - remove_column :order_cycles, :shipping_methods_customisable drop_table :order_cycle_shipping_methods end end diff --git a/db/schema.rb b/db/schema.rb index bca8dabe52..3f866caefd 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -323,6 +323,7 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do t.bigint "shipping_method_id" t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false + t.index ["order_cycle_id", "shipping_method_id"], name: "order_cycle_shipping_methods_join_index", unique: true t.index ["order_cycle_id"], name: "index_order_cycle_shipping_methods_on_order_cycle_id" t.index ["shipping_method_id"], name: "index_order_cycle_shipping_methods_on_shipping_method_id" end @@ -337,7 +338,6 @@ ActiveRecord::Schema.define(version: 2022_09_07_055044) do t.datetime "processed_at" t.boolean "automatic_notifications", default: false t.boolean "mails_sent", default: false - t.boolean "shipping_methods_customisable", default: true end create_table "producer_properties", id: :serial, force: :cascade do |t| diff --git a/lib/tasks/sample_data/order_cycle_factory.rb b/lib/tasks/sample_data/order_cycle_factory.rb index d6701d5177..0f9dc258e8 100644 --- a/lib/tasks/sample_data/order_cycle_factory.rb +++ b/lib/tasks/sample_data/order_cycle_factory.rb @@ -57,9 +57,6 @@ module SampleData log "- #{name}" cycle = create_order_cycle_with_fee(name, coordinator) create_exchanges(cycle, supplier_names, distributor_names, data) - return if cycle.reload.attachable_shipping_methods.none? - - cycle.shipping_method_ids = cycle.attachable_shipping_methods.pluck(:id) end def create_order_cycle_with_fee(name, coordinator) diff --git a/spec/factories/subscription_factory.rb b/spec/factories/subscription_factory.rb index 5bccb7eaf0..f0b67c0631 100644 --- a/spec/factories/subscription_factory.rb +++ b/spec/factories/subscription_factory.rb @@ -3,13 +3,7 @@ FactoryBot.define do factory :subscription, class: Subscription do shop { create :enterprise } - schedule do - order_cycle = create(:distributor_order_cycle, - coordinator: shop, - distributors: [shop], - shipping_methods: [shipping_method]) - create(:schedule, order_cycles: [order_cycle]) - end + schedule { create(:schedule, order_cycles: [create(:simple_order_cycle, coordinator: shop)]) } customer { create(:customer, enterprise: shop) } bill_address { create(:address, :randomized) } ship_address { create(:address, :randomized) } diff --git a/spec/models/order_cycle_shipping_method_spec.rb b/spec/models/order_cycle_shipping_method_spec.rb index fcc5fd5290..bb503182fb 100644 --- a/spec/models/order_cycle_shipping_method_spec.rb +++ b/spec/models/order_cycle_shipping_method_spec.rb @@ -51,22 +51,6 @@ describe OrderCycleShippingMethod do ) end - it "is not valid if order cycle doesn't support customised shipping methods - e.g. the order cycle was created before the custom shipping methods feature was available" do - order_cycle = create(:distributor_order_cycle, shipping_methods_customisable: false) - 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 shipping methods cannot be customised, all shipping methods are available by default" - ) - 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]) diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index ab66327504..0da971b524 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -28,9 +28,13 @@ describe OrderCycle do 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 = create(:distributor_order_cycle, + distributors: [distributor_i, distributor_ii]) - order_cycle.shipping_method_ids = [distributor_i_shipping_method.id, distributor_ii_shipping_method.id] + order_cycle.preferred_shipping_method_ids = [ + distributor_i_shipping_method.id, + distributor_ii_shipping_method.id + ] expect(order_cycle).to be_valid end @@ -43,19 +47,24 @@ describe OrderCycle do 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] + order_cycle.preferred_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"] + 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 + describe "#no_invalid_order_cycle_shipping_methods" do + context "when a order cycle 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 + shipping_method = create(:shipping_method) + distributor = create(:distributor_enterprise, shipping_methods: [shipping_method]) + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + order_cycle.preferred_shipping_methods << shipping_method shipping_method.update_column(:display_on, "back_end") @@ -69,23 +78,6 @@ describe OrderCycle do 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) @@ -198,7 +190,7 @@ describe OrderCycle do end it "reports its distributors" do - oc = create(:simple_order_cycle, validate_shipping_methods: false) + oc = create(:simple_order_cycle) e1 = create(:exchange, incoming: false, order_cycle: oc, sender: oc.coordinator, receiver: create(:enterprise)) @@ -209,7 +201,7 @@ describe OrderCycle do end it "checks for existance of distributors" do - oc = create(:simple_order_cycle, validate_shipping_methods: false) + oc = create(:simple_order_cycle) d1 = create(:distributor_enterprise) d2 = create(:distributor_enterprise) create(:exchange, order_cycle: oc, sender: oc.coordinator, receiver: d1, incoming: false) @@ -470,22 +462,14 @@ describe OrderCycle do expect(cloned_exchange_attributes).to match_array original_exchange_attributes end - context "distributor order cycle created before the customisable shipping methods feature was available" do - it "allows the clone to have customisable shipping methods" do - order_cycle = create(:distributor_order_cycle, shipping_methods_customisable: false) - - order_cycle.clone! - - order_cycle_clone = OrderCycle.last - expect(order_cycle_clone.shipping_methods_customisable).to eq(true) - end - end - - context "when it has shipping methods which can longer be applied validly e.g. shipping method is backoffice only" 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 distributor = create(:distributor_enterprise) shipping_method = create(:shipping_method, distributors: [distributor]) - order_cycle = create(:distributor_order_cycle, distributors: [distributor], shipping_methods: [shipping_method]) + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + order_cycle.preferred_shipping_methods << shipping_method + shipping_method.update_column(:display_on, "back_end") expect { @@ -721,6 +705,38 @@ describe OrderCycle do end end + describe "#shipping_methods" do + let(:distributor) { create(:distributor_enterprise) } + + it "returns all attachable shipping methods if the order cycle is simple" do + oc = create(:sells_own_order_cycle, distributors: [distributor]) + + shipping_method = create(:shipping_method, distributors: [distributor]) + + expect(oc.shipping_methods).to eq [shipping_method] + end + + context "distributor order cycle i.e. non-simple" do + let(:oc) { create(:distributor_order_cycle, distributors: [distributor]) } + + it "returns all attachable shipping methods if no preferred shipping methods have been chosen" do + shipping_method = create(:shipping_method, distributors: [distributor]) + + expect(oc.preferred_shipping_methods).to be_empty + expect(oc.shipping_methods).to eq [shipping_method] + end + + it "returns preferred shipping methods if they have been specified" do + shipping_method_i = create(:shipping_method, distributors: [distributor]) + shipping_method_ii = create(:shipping_method, distributors: [distributor]) + + oc.preferred_shipping_methods << shipping_method_ii + + expect(oc.shipping_methods).to eq [shipping_method_ii] + end + end + end + describe "#simple?" do it "returns true if the coordinator sells their own products i.e. shops" do order_cycle = build(:simple_order_cycle, coordinator: build(:enterprise, sells: "own")) diff --git a/spec/models/spree/shipping_method_spec.rb b/spec/models/spree/shipping_method_spec.rb index 8c030fa02d..ba4b5d84fb 100644 --- a/spec/models/spree/shipping_method_spec.rb +++ b/spec/models/spree/shipping_method_spec.rb @@ -182,22 +182,38 @@ module Spree end context "when it is being changed to backoffice only" do - let!(:order_cycle) { create(:distributor_order_cycle, with_distributor_and_shipping_method: true) } - let(:distributor) { order_cycle.distributors.first } - let(:shipping_method) { order_cycle.shipping_methods.first } + 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 the shipping method is the only shipping method on a distributor order cycle" do + 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"] + 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 the order cycles the shipping method is attached to has other valid shipping methods" do - it "is is valid" do - order_cycle.shipping_methods << create(:shipping_method, distributors: [distributor]) + 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" @@ -242,9 +258,10 @@ module Spree 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], shipping_methods: [shipping_method]) } + let!(:order_cycle) { create(:distributor_order_cycle, distributors: [distributor]) } - context "when the shipping method is the only shipping method on a distributor order cycle" do + 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) @@ -259,7 +276,10 @@ module Spree 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"] + 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 @@ -268,13 +288,28 @@ module Spree 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"] + 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 the order cycles the shipping method is attached to has other valid shipping methods" do + 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 - order_cycle.shipping_methods << create(:shipping_method, distributors: [distributor]) + 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 diff --git a/spec/services/order_available_shipping_methods_spec.rb b/spec/services/order_available_shipping_methods_spec.rb index 814853d6eb..5cfdde3e21 100644 --- a/spec/services/order_available_shipping_methods_spec.rb +++ b/spec/services/order_available_shipping_methods_spec.rb @@ -26,54 +26,41 @@ describe OrderAvailableShippingMethods do end context "when no tag rules are in effect" do - context "order cycle selling own produce only i.e. shipping methods cannot be customised" do - it "returns all shipping methods belonging to the enterprise" do - order_cycle = create(:sells_own_order_cycle) - enterprise = order_cycle.coordinator - shipping_method = create(:shipping_method, distributors: [enterprise]) - other_enterprise = create(:enterprise) - other_enterprise_shipping_method = create(:shipping_method, - distributors: [other_enterprise]) - order = build(:order, distributor: enterprise, order_cycle: order_cycle) + context "sells own order cycle i.e. simple" do + it "only returns the shipping methods which are available on the order cycle + and belong to the order distributor" do + distributor_i = create(:distributor_enterprise) + distributor_ii = create(:distributor_enterprise) + distributor_iii = create(:distributor_enterprise) + shipping_method_i = create(:shipping_method, distributors: [distributor_i]) + shipping_method_ii = create(:shipping_method, distributors: [distributor_ii]) + shipping_method_iii = create(:shipping_method, distributors: [distributor_iii]) + order_cycle = create(:sells_own_order_cycle, distributors: [distributor_i, distributor_ii]) + order = build(:order, distributor: distributor_i, order_cycle: order_cycle) available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a - expect(order_cycle.shipping_methods).to be_empty - expect(available_shipping_methods).to eq [shipping_method] + expect(available_shipping_methods).to eq [shipping_method_i] end end - context "distributor order cycle" do - it "only returns shipping methods which belong to the order distributor - and have been added to the order cycle" do - distributor = create(:distributor_enterprise) - shipping_method_i = create(:shipping_method, distributors: [distributor]) - shipping_method_ii = create(:shipping_method, distributors: [distributor]) - order_cycle = create(:simple_order_cycle, - distributors: [distributor], shipping_methods: [shipping_method_i]) - order = build(:order, distributor: distributor, order_cycle: order_cycle) - - available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a - - expect(available_shipping_methods).to eq order_cycle.shipping_methods - expect(available_shipping_methods).to eq [shipping_method_i] - end - - it "doesn't return shipping methods which have been added to the order cycle - when they don't belong to the order distributor" do + context "distributor order cycle i.e. not simple" do + it "only returns the shipping methods which are available on the order cycle + and belong to the order distributor" do distributor_i = create(:distributor_enterprise) distributor_ii = create(:distributor_enterprise) shipping_method_i = create(:shipping_method, distributors: [distributor_i]) - shipping_method_ii = create(:shipping_method, distributors: [distributor_ii]) - order_cycle = create(:simple_order_cycle, - distributors: [distributor_i, distributor_ii], - shipping_methods: [shipping_method_i, shipping_method_ii]) - order = build(:order, distributor: distributor_ii, order_cycle: order_cycle) + shipping_method_ii = create(:shipping_method, distributors: [distributor_i]) + shipping_method_iii = create(:shipping_method, distributors: [distributor_ii]) + shipping_method_iv = create(:shipping_method, distributors: [distributor_ii]) + order_cycle = create(:distributor_order_cycle, + distributors: [distributor_i, distributor_ii]) + order_cycle.preferred_shipping_methods << [shipping_method_i, shipping_method_iii] + order = build(:order, distributor: distributor_i, order_cycle: order_cycle) available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a - expect(available_shipping_methods).not_to eq order_cycle.shipping_methods - expect(available_shipping_methods).to eq [shipping_method_ii] + expect(available_shipping_methods).to eq [shipping_method_i] end end end @@ -116,66 +103,39 @@ describe OrderAvailableShippingMethods do 'hidden') } - context "order cycle selling own produce only" do - let(:order_cycle) { create(:sells_own_order_cycle) } - let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) } + let(:order_cycle) { create(:sells_own_order_cycle) } + let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) } - context "when the customer is nil" do - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a } + context "when the customer is nil" do + let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a } - it "applies default action (hide)" do - expect(available_shipping_methods).to include untagged_sm - expect(available_shipping_methods).to_not include tagged_sm - end - end - - context "when a customer is present" do - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a } - - context "and the customer's tags match" do - before do - customer.update_attribute(:tag_list, 'local') - end - - it "applies the action (show)" do - expect(available_shipping_methods).to include tagged_sm, untagged_sm - end - end - - context "and the customer's tags don't match" do - before do - customer.update_attribute(:tag_list, 'something') - end - - it "applies the default action (hide)" do - expect(available_shipping_methods).to include untagged_sm - expect(available_shipping_methods).to_not include tagged_sm - end - end + it "applies default action (hide)" do + expect(available_shipping_methods).to include untagged_sm + expect(available_shipping_methods).to_not include tagged_sm end end - context "distributor order cycle" do - context "when the shipping method without the tag rule is attached to the order cycle - and the shipping method with the tag rule is not" do - let(:order_cycle) do - create(:distributor_order_cycle, - distributors: [distributor], shipping_methods: [untagged_sm]) + context "when a customer is present" do + let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a } + + context "and the customer's tags match" do + before do + customer.update_attribute(:tag_list, 'local') end - let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) } - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a } - context "when the customer's tags match" do - before do - customer.update_attribute(:tag_list, 'local') - end + it "applies the action (show)" do + expect(available_shipping_methods).to include tagged_sm, untagged_sm + end + end - it "doesn't display the shipping method with the prefered visibility 'visible' tag - even though the customer's tags match - because it hasn't been attached to the order cycle" do - expect(available_shipping_methods).to include untagged_sm - expect(available_shipping_methods).to_not include tagged_sm - end + context "and the customer's tags don't match" do + before do + customer.update_attribute(:tag_list, 'something') + end + + it "applies the default action (hide)" do + expect(available_shipping_methods).to include untagged_sm + expect(available_shipping_methods).to_not include tagged_sm end end end @@ -190,77 +150,38 @@ describe OrderAvailableShippingMethods do 'visible') } - context "order cycle selling own produce only" do - let(:order_cycle) { create(:sells_own_order_cycle) } - let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) } + let(:order_cycle) { create(:sells_own_order_cycle) } + let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) } - context "when the customer is nil" do - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a } + context "when the customer is nil" do + let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a } - it "applies default action (show)" do - expect(available_shipping_methods).to include tagged_sm, untagged_sm - end - end - - context "when a customer is present" do - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a } - - context "and the customer's tags match" do - before do - customer.update_attribute(:tag_list, 'local') - end - - it "applies the action (hide)" do - expect(available_shipping_methods).to include untagged_sm - expect(available_shipping_methods).to_not include tagged_sm - end - end - - context "and the customer's tags don't match" do - before do - customer.update_attribute(:tag_list, 'something') - end - - it "applies the default action (show)" do - expect(available_shipping_methods).to include tagged_sm, untagged_sm - end - end + it "applies default action (show)" do + expect(available_shipping_methods).to include tagged_sm, untagged_sm end end - context "distributor order cycle" do - context "when the shipping method without the tag rule is attached to the order cycle - and the shipping method with the tag rule is not" do - let(:order_cycle) do - create(:distributor_order_cycle, - distributors: [distributor], shipping_methods: [untagged_sm]) - end - let(:order) { build(:order, distributor: distributor, order_cycle: order_cycle) } + context "when a customer is present" do + let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a } - context "when the customer is nil" do - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order).to_a } - - it "doesn't display the shipping method tagged to be visible by default - because it is not attached to the order cycle" do - expect(available_shipping_methods).to include untagged_sm - expect(available_shipping_methods).to_not include tagged_sm - end + context "and the customer's tags match" do + before do + customer.update_attribute(:tag_list, 'local') end - context "when a customer is present" do - let(:available_shipping_methods) { OrderAvailableShippingMethods.new(order, customer).to_a } + it "applies the action (hide)" do + expect(available_shipping_methods).to include untagged_sm + expect(available_shipping_methods).to_not include tagged_sm + end + end - context "when the customer's tags don't match" do - before do - customer.update_attribute(:tag_list, 'something') - end + context "and the customer's tags don't match" do + before do + customer.update_attribute(:tag_list, 'something') + end - it "doesn't display the shipping method tagged to be visible by default - because it is not attached to the order cycle" do - expect(available_shipping_methods).to include untagged_sm - expect(available_shipping_methods).to_not include tagged_sm - end - end + it "applies the default action (show)" do + expect(available_shipping_methods).to include tagged_sm, untagged_sm end end end diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index c7b472257d..f2edeacbce 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -146,7 +146,8 @@ describe OrderCycleForm do end end - context "updating basics, incoming and outcoming exchanges, shipping methods simultaneously" do + context "updating basics, incoming exchanges, outcoming exchanges + and preferred shipping methods simultaneously" do before do params.merge!( incoming_exchanges: [{ @@ -158,7 +159,7 @@ describe OrderCycleForm do enterprise_fee_ids: [] }], outgoing_exchanges: [outgoing_exchange_params], - shipping_method_ids: [shipping_method.id] + preferred_shipping_method_ids: [shipping_method.id] ) end @@ -175,7 +176,7 @@ describe OrderCycleForm do before do params.merge!( outgoing_exchanges: [outgoing_exchange_params], - shipping_method_ids: nil + preferred_shipping_method_ids: nil ) end @@ -194,7 +195,7 @@ describe OrderCycleForm do before do params.merge!( outgoing_exchanges: [outgoing_exchange_params], - shipping_method_ids: [other_distributor_shipping_method.id] + preferred_shipping_method_ids: [other_distributor_shipping_method.id] ) end @@ -209,11 +210,13 @@ describe OrderCycleForm do context "when shipping methods already exist and doing an update without the :shipping_methods_id parameter" do it "doesn't return a validation error on shipping methods" do - order_cycle = create(:distributor_order_cycle, with_distributor_and_shipping_method: true) + distributor = create(:distributor_enterprise) + shipping_method = create(:shipping_method, distributors: [distributor]) + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) form = OrderCycleForm.new( order_cycle, - params.except(:shipping_method_ids), + params.except(:preferred_shipping_method_ids), order_cycle.coordinator ) @@ -229,7 +232,7 @@ describe OrderCycleForm do order_cycle = create(:distributor_order_cycle, distributors: [distributor]) form = OrderCycleForm.new(order_cycle, - { shipping_method_ids: [shipping_method.id] }, + { preferred_shipping_method_ids: [shipping_method.id] }, order_cycle.coordinator) expect(form.save).to be true @@ -239,11 +242,15 @@ describe OrderCycleForm do context "and it's invalid" do it "returns a validation error" do - distributor = create(:distributor_enterprise) - order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + distributor_i = create(:distributor_enterprise) + distributor_ii = create(:distributor_enterprise) + 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]) form = OrderCycleForm.new(order_cycle, - { shipping_method_ids: [] }, + { preferred_shipping_method_ids: [shipping_method_i.id] }, order_cycle.coordinator) expect(form.save).to be false