From a53a3259a81e16bb307e6a569cbbee9f4c9ec6b7 Mon Sep 17 00:00:00 2001 From: Cillian O'Ruanaidh Date: Fri, 9 Sep 2022 14:00:11 +0100 Subject: [PATCH] Connect DistributorShippingMethods to OrderCycles instead of Spree::ShippingMethods Before if a shipping method was shared between multiple distributors it could only be disabled/enabled on that order cycle for all the distributors which have that shipping method e.g. you couldn't select that shipping method for one distributor but disable it for another. --- app/models/order_cycle.rb | 28 ++++---- .../order_available_shipping_methods.rb | 4 +- app/services/order_cycle_form.rb | 30 ++++---- .../permitted_attributes/order_cycle.rb | 2 +- .../order_cycles/checkout_options.html.haml | 16 ++--- spec/models/order_cycle_spec.rb | 69 ++++++++++++------- .../order_available_shipping_methods_spec.rb | 9 ++- spec/services/order_cycle_form_spec.rb | 49 ++++++++----- .../complex_creating_specific_time_spec.rb | 4 +- 9 files changed, 128 insertions(+), 83 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 923dd659f1..9b706172c8 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -24,9 +24,9 @@ class OrderCycle < ApplicationRecord has_many :distributors, -> { distinct }, source: :receiver, through: :cached_outgoing_exchanges has_many :order_cycle_schedules has_many :schedules, through: :order_cycle_schedules - has_and_belongs_to_many :selected_shipping_methods, - class_name: 'Spree::ShippingMethod', - join_table: 'order_cycles_shipping_methods' + has_and_belongs_to_many :selected_distributor_shipping_methods, + class_name: 'DistributorShippingMethod', + join_table: 'order_cycles_distributor_shipping_methods' has_paper_trail meta: { custom_data: proc { |order_cycle| order_cycle.schedule_ids.to_s } } attr_accessor :incoming_exchanges, :outgoing_exchanges @@ -160,11 +160,10 @@ class OrderCycle < ApplicationRecord distinct end - def attachable_shipping_methods - Spree::ShippingMethod.frontend. - joins(:distributor_shipping_methods). - where("distributor_id IN (?)", distributor_ids). - distinct + def attachable_distributor_shipping_methods + DistributorShippingMethod.joins(:shipping_method). + merge(Spree::ShippingMethod.frontend). + where("distributor_id IN (?)", distributor_ids) end def clone! @@ -178,8 +177,9 @@ class OrderCycle < ApplicationRecord oc.schedule_ids = schedule_ids oc.save! exchanges.each { |e| e.clone!(oc) } - oc.selected_shipping_method_ids = attachable_shipping_methods.map(&:id) & - selected_shipping_method_ids + oc.selected_distributor_shipping_method_ids = ( + attachable_distributor_shipping_methods.map(&:id) & selected_distributor_shipping_method_ids + ) sync_subscriptions oc.reload end @@ -293,11 +293,11 @@ class OrderCycle < ApplicationRecord items.each { |li| scoper.scope(li.variant) } end - def shipping_methods - if simple? || selected_shipping_methods.none? - attachable_shipping_methods + def distributor_shipping_methods + if simple? || selected_distributor_shipping_methods.none? + attachable_distributor_shipping_methods else - selected_shipping_methods + selected_distributor_shipping_methods end end diff --git a/app/services/order_available_shipping_methods.rb b/app/services/order_available_shipping_methods.rb index 36fd37f2c8..96d1b99559 100644 --- a/app/services/order_available_shipping_methods.rb +++ b/app/services/order_available_shipping_methods.rb @@ -29,7 +29,9 @@ class OrderAvailableShippingMethods if order_cycle.nil? || order_cycle.simple? distributor.shipping_methods else - distributor.shipping_methods.where(id: order_cycle.shipping_methods.select(:id)) + distributor.shipping_methods.where( + id: order_cycle.distributor_shipping_methods.select(:shipping_method_id) + ) end.frontend.to_a end end diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index d4334db495..68308dd9bb 100644 --- a/app/services/order_cycle_form.rb +++ b/app/services/order_cycle_form.rb @@ -11,7 +11,9 @@ class OrderCycleForm @user = user @permissions = OpenFoodNetwork::Permissions.new(user) @schedule_ids = order_cycle_params.delete(:schedule_ids) - @selected_shipping_method_ids = order_cycle_params.delete(:selected_shipping_method_ids) + @selected_distributor_shipping_method_ids = order_cycle_params.delete( + :selected_distributor_shipping_method_ids + ) end def save @@ -24,7 +26,7 @@ class OrderCycleForm order_cycle.schedule_ids = schedule_ids order_cycle.save! apply_exchange_changes - attach_selected_shipping_methods + attach_selected_distributor_shipping_methods sync_subscriptions true end @@ -48,16 +50,16 @@ class OrderCycleForm OpenFoodNetwork::OrderCycleFormApplicator.new(order_cycle, user).go! end - def attach_selected_shipping_methods - return if @selected_shipping_method_ids.nil? + def attach_selected_distributor_shipping_methods + return if @selected_distributor_shipping_method_ids.nil? order_cycle.reload # so outgoing exchanges are up-to-date for shipping method validations - order_cycle.selected_shipping_method_ids = selected_shipping_method_ids + order_cycle.selected_distributor_shipping_method_ids = selected_distributor_shipping_method_ids order_cycle.save! end - def attachable_shipping_method_ids - @attachable_shipping_method_ids ||= order_cycle.attachable_shipping_methods.map(&:id) + def attachable_distributor_shipping_method_ids + @attachable_distributor_shipping_method_ids ||= order_cycle.attachable_distributor_shipping_methods.map(&:id) end def exchanges_unchanged? @@ -66,15 +68,17 @@ class OrderCycleForm end end - def selected_shipping_method_ids - @selected_shipping_method_ids = attachable_shipping_method_ids & - @selected_shipping_method_ids.reject(&:blank?).map(&:to_i) + def selected_distributor_shipping_method_ids + @selected_distributor_shipping_method_ids = ( + attachable_distributor_shipping_method_ids & + @selected_distributor_shipping_method_ids.reject(&:blank?).map(&:to_i) + ) - if attachable_shipping_method_ids.sort == @selected_shipping_method_ids.sort - @selected_shipping_method_ids = [] + if attachable_distributor_shipping_method_ids.sort == @selected_distributor_shipping_method_ids.sort + @selected_distributor_shipping_method_ids = [] end - @selected_shipping_method_ids + @selected_distributor_shipping_method_ids end def schedule_ids? diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb index e1b7555d6d..15d652268c 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: [], selected_shipping_method_ids: [], coordinator_fee_ids: [] } + { schedule_ids: [], selected_distributor_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 79cfe59d20..4c3dc61cbc 100644 --- a/app/views/admin/order_cycles/checkout_options.html.haml +++ b/app/views/admin/order_cycles/checkout_options.html.haml @@ -10,7 +10,7 @@ %fieldset.no-border-bottom %legend{ align: 'center'}= t('.checkout_options') - = hidden_field_tag "order_cycle[selected_shipping_method_ids][]", "" + = hidden_field_tag "order_cycle[selected_distributor_shipping_method_ids][]", "" .row .three.columns @@ -21,18 +21,18 @@ %tr %th{ colspan: 2 }= t('.shipping_methods') - @order_cycle.distributors.each do |distributor| - - shipping_methods = @order_cycle.attachable_shipping_methods.where("distributor_id = ?", distributor.id) + - distributor_shipping_methods = @order_cycle.attachable_distributor_shipping_methods.where("distributor_id = ?", distributor.id).includes(:shipping_method) %tr %td %td %em= distributor.name - - shipping_methods.each do |shipping_method| + - distributor_shipping_methods.each do |distributor_shipping_method| %p - %label - = check_box_tag "order_cycle[selected_shipping_method_ids][]", - shipping_method.id, @order_cycle.shipping_methods.include?(shipping_method), - id: "order_cycle_selected_shipping_method_ids_#{shipping_method.id}" - = shipping_method.name + %label{ class: ("disabled" unless distributor_shipping_method.shipping_method.frontend?) } + = check_box_tag "order_cycle[selected_distributor_shipping_method_ids][]", + distributor_shipping_method.id, @order_cycle.distributor_shipping_methods.include?(distributor_shipping_method), + id: "order_cycle_selected_distributor_shipping_method_ids_#{distributor_shipping_method.id}" + = distributor_shipping_method.shipping_method.name - distributor.shipping_methods.backend.each do |shipping_method| %label.disabled = check_box_tag nil, nil, false, disabled: true diff --git a/spec/models/order_cycle_spec.rb b/spec/models/order_cycle_spec.rb index 9035b853e1..91b212ca89 100644 --- a/spec/models/order_cycle_spec.rb +++ b/spec/models/order_cycle_spec.rb @@ -408,18 +408,24 @@ describe OrderCycle do e.g. shipping method is backoffice only" do it "only attaches the valid ones to the clone" do distributor = create(:distributor_enterprise) - shipping_method_i = create(:shipping_method, distributors: [distributor]) - shipping_method_ii = create( + distributor_shipping_method_i = create( + :shipping_method, + distributors: [distributor] + ).distributor_shipping_methods.first + distributor_shipping_method_ii = create( :shipping_method, distributors: [distributor], display_on: Spree::ShippingMethod::DISPLAY_ON_OPTIONS[:back_end] - ) + ).distributor_shipping_methods.first order_cycle = create(:distributor_order_cycle, distributors: [distributor]) - order_cycle.selected_shipping_methods = [shipping_method_i, shipping_method_ii] + order_cycle.selected_distributor_shipping_methods = [ + distributor_shipping_method_i, + distributor_shipping_method_ii + ] cloned_order_cycle = order_cycle.clone! - expect(cloned_order_cycle.shipping_methods).to eq [shipping_method_i] + expect(cloned_order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method_i] end end end @@ -631,52 +637,65 @@ describe OrderCycle do end end - describe "#attachable_shipping_methods" do - it "includes shipping methods from the distributors on the order cycle" do + describe "#attachable_distributor_shipping_methods" do + it "includes distributor shipping methods from the distributors on the order cycle" do shipping_method = create(:shipping_method) - enterprise = create(:enterprise, shipping_methods: [shipping_method]) - oc = create(:simple_order_cycle, distributors: [enterprise]) + oc = create(:simple_order_cycle, distributors: [shipping_method.distributors.first]) + distributor_shipping_method = shipping_method.distributor_shipping_methods.first - expect(oc.attachable_shipping_methods).to eq([shipping_method]) + expect(oc.attachable_distributor_shipping_methods).to eq([distributor_shipping_method]) end - it "does not include backoffice only shipping methods" do + it "does not include backoffice only distributor shipping methods" do shipping_method = create(:shipping_method, display_on: "back_end") enterprise = create(:enterprise, shipping_methods: [shipping_method]) oc = create(:simple_order_cycle, distributors: [enterprise]) - expect(oc.attachable_shipping_methods).to be_empty + expect(oc.attachable_distributor_shipping_methods).to be_empty end end - describe "#shipping_methods" do + describe "#distributor_shipping_methods" do let(:distributor) { create(:distributor_enterprise) } - it "returns all attachable shipping methods if the order cycle is simple" do + it "returns all attachable distributor shipping methods if the order cycle is simple" do oc = create(:sells_own_order_cycle, distributors: [distributor]) - shipping_method = create(:shipping_method, distributors: [distributor]) + distributor_shipping_method = create( + :shipping_method, + distributors: [distributor] + ).distributor_shipping_methods.first - expect(oc.shipping_methods).to eq [shipping_method] + expect(oc.distributor_shipping_methods).to eq [distributor_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]) + it "returns all attachable distributor shipping methods if no preferred shipping methods have + been chosen" do + distributor_shipping_method = create( + :shipping_method, + distributors: [distributor] + ).distributor_shipping_methods.first - expect(oc.selected_shipping_methods).to be_empty - expect(oc.shipping_methods).to eq [shipping_method] + expect(oc.selected_distributor_shipping_methods).to be_empty + expect(oc.distributor_shipping_methods).to eq [distributor_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]) + it "returns selected distributor shipping methods if they have been specified" do + distributor_shipping_method_i = create( + :shipping_method, + distributors: [distributor] + ).distributor_shipping_methods.first + distributor_shipping_method_ii = create( + :shipping_method, + distributors: [distributor] + ).distributor_shipping_methods.first - oc.selected_shipping_methods << shipping_method_ii + oc.selected_distributor_shipping_methods << distributor_shipping_method_ii - expect(oc.shipping_methods).to eq [shipping_method_ii] + expect(oc.distributor_shipping_methods).to eq [distributor_shipping_method_ii] end end end diff --git a/spec/services/order_available_shipping_methods_spec.rb b/spec/services/order_available_shipping_methods_spec.rb index 8157e2156f..59acb35dc6 100644 --- a/spec/services/order_available_shipping_methods_spec.rb +++ b/spec/services/order_available_shipping_methods_spec.rb @@ -47,15 +47,18 @@ describe OrderAvailableShippingMethods 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) + distributor_i = create(:distributor_enterprise, shipping_methods: []) + distributor_ii = create(:distributor_enterprise, shipping_methods: []) shipping_method_i = create(:shipping_method, distributors: [distributor_i]) 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.selected_shipping_methods << [shipping_method_i, shipping_method_iii] + order_cycle.selected_distributor_shipping_methods << [ + distributor_i.distributor_shipping_methods.first, + distributor_ii.distributor_shipping_methods.first, + ] order = build(:order, distributor: distributor_i, order_cycle: order_cycle) available_shipping_methods = OrderAvailableShippingMethods.new(order).to_a diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index be3143fbc9..612a90a058 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -125,6 +125,7 @@ describe OrderCycleForm do let(:supplier) { create(:supplier_enterprise) } let(:user) { distributor.owner } let(:shipping_method) { create(:shipping_method, distributors: [distributor]) } + let(:distributor_shipping_method) { shipping_method.distributor_shipping_methods.first } let(:variant) { create(:variant, product: create(:product, supplier: supplier)) } let(:params) { { name: 'Some new name' } } let(:form) { OrderCycleForm.new(order_cycle, params, user) } @@ -159,7 +160,7 @@ describe OrderCycleForm do enterprise_fee_ids: [] }], outgoing_exchanges: [outgoing_exchange_params], - selected_shipping_method_ids: [shipping_method.id] + selected_distributor_shipping_method_ids: [distributor_shipping_method.id] ) end @@ -168,27 +169,30 @@ describe OrderCycleForm do expect(order_cycle.name).to eq 'Some new name' expect(order_cycle.cached_incoming_exchanges.count).to eq 1 expect(order_cycle.cached_outgoing_exchanges.count).to eq 1 - expect(order_cycle.shipping_methods).to eq [shipping_method] + expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] end end context "updating outgoing exchanges and shipping methods simultaneously but the shipping method doesn't belong to the new or any existing order cycle distributor" do let(:other_distributor_shipping_method) do - create(:shipping_method, distributors: [create(:distributor_enterprise)]) + create( + :shipping_method, + distributors: [create(:distributor_enterprise)] + ).distributor_shipping_methods.first end before do params.merge!( outgoing_exchanges: [outgoing_exchange_params], - selected_shipping_method_ids: [other_distributor_shipping_method.id] + selected_distributor_shipping_method_ids: [other_distributor_shipping_method.id] ) end it "saves the outgoing exchange but ignores the shipping method" do expect(form.save).to be true expect(order_cycle.distributors).to eq [distributor] - expect(order_cycle.shipping_methods).to be_empty + expect(order_cycle.distributor_shipping_methods).to be_empty end end @@ -196,15 +200,20 @@ describe OrderCycleForm do context "and it's valid" do it "saves the changes" do distributor = create(:distributor_enterprise) - shipping_method = create(:shipping_method, distributors: [distributor]) + distributor_shipping_method = create( + :shipping_method, + distributors: [distributor] + ).distributor_shipping_methods.first order_cycle = create(:distributor_order_cycle, distributors: [distributor]) - form = OrderCycleForm.new(order_cycle, - { selected_shipping_method_ids: [shipping_method.id] }, - order_cycle.coordinator) + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + order_cycle.coordinator + ) expect(form.save).to be true - expect(order_cycle.shipping_methods).to eq [shipping_method] + expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] end end @@ -212,17 +221,25 @@ describe OrderCycleForm do it "ignores it" 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]) + distributor_shipping_method_i = create( + :shipping_method, + distributors: [distributor_i] + ).distributor_shipping_methods.first + distributor_shipping_method_ii = create( + :shipping_method, + distributors: [distributor_ii] + ).distributor_shipping_methods.first order_cycle = create(:distributor_order_cycle, distributors: [distributor_i]) - form = OrderCycleForm.new(order_cycle, - { selected_shipping_method_ids: [shipping_method_ii.id] }, - order_cycle.coordinator) + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method_ii.id] }, + order_cycle.coordinator + ) expect(form.save).to be true - expect(order_cycle.shipping_methods).to eq [shipping_method_i] + expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method_i] end end end diff --git a/spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb b/spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb index 25ee70aed4..31f3ce70bc 100644 --- a/spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb +++ b/spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb @@ -66,7 +66,7 @@ describe ' ## UPDATE add_supplier_with_fees add_distributor_with_fees - select_shipping_methods + select_distributor_shipping_methods expect_all_data_saved end @@ -160,7 +160,7 @@ describe ' click_button 'Save and Next' end - def select_shipping_methods + def select_distributor_shipping_methods expect(page).to have_checked_field "Select all" expect(page).to have_checked_field "Pickup - always available"