From a7e6760028f5394bf2cd62f9198ad7653231163c Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Thu, 23 Feb 2023 07:09:49 +0100 Subject: [PATCH 1/5] limit users who can update shipping/payment method of an order cycle --- app/services/order_cycle_form.rb | 66 ++++++++- spec/services/order_cycle_form_spec.rb | 195 +++++++++++++++++++++---- 2 files changed, 229 insertions(+), 32 deletions(-) diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index 52a3ce10ef..9f99d6ffa6 100644 --- a/app/services/order_cycle_form.rb +++ b/app/services/order_cycle_form.rb @@ -30,8 +30,10 @@ class OrderCycleForm order_cycle.schedule_ids = schedule_ids if parameter_specified?(:schedule_ids) order_cycle.save! apply_exchange_changes - attach_selected_distributor_payment_methods - attach_selected_distributor_shipping_methods + if can_update_selected_payment_or_shipping_methods? + attach_selected_distributor_payment_methods + attach_selected_distributor_shipping_methods + end sync_subscriptions true end @@ -61,14 +63,29 @@ class OrderCycleForm def attach_selected_distributor_payment_methods return if @selected_distributor_payment_method_ids.nil? - order_cycle.selected_distributor_payment_method_ids = selected_distributor_payment_method_ids + if distributor_only? + payment_method_ids = order_cycle.selected_distributor_payment_method_ids + payment_method_ids -= user_distributor_payment_method_ids + payment_method_ids += user_only_selected_distributor_payment_method_ids + order_cycle.selected_distributor_payment_method_ids = payment_method_ids + else + order_cycle.selected_distributor_payment_method_ids = selected_distributor_payment_method_ids + end order_cycle.save! end def attach_selected_distributor_shipping_methods return if @selected_distributor_shipping_method_ids.nil? - order_cycle.selected_distributor_shipping_method_ids = selected_distributor_shipping_method_ids + if distributor_only? + shipping_method_ids = order_cycle.selected_distributor_shipping_method_ids + shipping_method_ids -= user_distributor_shipping_method_ids + shipping_method_ids += user_only_selected_distributor_shipping_method_ids + order_cycle.selected_distributor_shipping_method_ids = shipping_method_ids + else + order_cycle.selected_distributor_shipping_method_ids = selected_distributor_shipping_method_ids + end + order_cycle.save! end @@ -99,6 +116,10 @@ class OrderCycleForm @selected_distributor_payment_method_ids end + def user_only_selected_distributor_payment_method_ids + user_distributor_payment_method_ids.intersection(selected_distributor_payment_method_ids) + end + def selected_distributor_shipping_method_ids @selected_distributor_shipping_method_ids = ( attachable_distributor_shipping_method_ids & @@ -112,6 +133,10 @@ class OrderCycleForm @selected_distributor_shipping_method_ids end + def user_only_selected_distributor_shipping_method_ids + user_distributor_shipping_method_ids.intersection(selected_distributor_shipping_method_ids) + end + def build_schedule_ids return unless parameter_specified?(:schedule_ids) @@ -160,4 +185,37 @@ class OrderCycleForm def new_schedule_ids @order_cycle.schedule_ids - existing_schedule_ids end + + def can_update_selected_payment_or_shipping_methods? + @user.admin? || coordinator? || distributor? + end + + def coordinator? + @user.enterprises.include?(@order_cycle.coordinator) + end + + def distributor? + !user_distributors_ids.empty? + end + + def distributor_only? + distributor? && !@user.admin? && !coordinator? + end + + def user_distributors_ids + @user_distributors_ids ||= @user.enterprises.pluck(:id) + .intersection(@order_cycle.distributors.pluck(:id)) + end + + def user_distributor_payment_method_ids + @user_distributor_payment_method_ids ||= + DistributorPaymentMethod.where(distributor_id: user_distributors_ids) + .pluck(:id) + end + + def user_distributor_shipping_method_ids + @user_distributor_shipping_method_ids ||= + DistributorShippingMethod.where(distributor_id: user_distributors_ids) + .pluck(:id) + end end diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index a9ed405458..18e7b03fcd 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -223,22 +223,93 @@ describe OrderCycleForm do context "updating payment methods" do context "and it's valid" do - it "saves the changes" do - distributor = create(:distributor_enterprise) - distributor_payment_method = create( - :payment_method, - distributors: [distributor] - ).distributor_payment_methods.first - order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + let!(:distributor){ create(:distributor_enterprise) } + let!(:payment_method){ create(:payment_method, distributors: [distributor]) } + let!(:payment_method2){ create(:payment_method, distributors: [distributor]) } + let!(:distributor_payment_method){ payment_method.distributor_payment_methods.first } + let!(:distributor_payment_method2){ payment_method2.distributor_payment_methods.first } + let!(:supplier){ create(:supplier_enterprise) } + context "the submitter is a coordinator" do + it "saves the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) - form = OrderCycleForm.new( - order_cycle, - { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, - order_cycle.coordinator - ) + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + order_cycle.coordinator.users.first + ) - expect(form.save).to be true - expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + expect(form.save).to be true + expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + end + end + + context "submitter is a supplier" do + it "doesn't save the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor], + suppliers: [supplier]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + supplier.users.first + ) + + expect(form).not_to receive(:attach_selected_distributor_payment_methods) + expect(order_cycle.distributor_payment_methods).to match_array [ + distributor_payment_method, distributor_payment_method2 + ] + end + end + + context "submitter is an admin" do + it "saves the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + create(:admin_user) + ) + + expect(form.save).to be true + expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + end + end + + context "submitter is a distributor" do + context "can update his own payment methods" do + it "saves the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + distributor.users.first + ) + + expect(form.save).to be true + expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + end + end + context "can't update other distributors' payment methods" do + let(:distributor2){ create(:distributor_enterprise) } + it "doesn't save the changes" do + order_cycle = create(:distributor_order_cycle, + distributors: [distributor, distributor2]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + distributor2.users.first + ) + + expect(form).not_to receive(:attach_selected_distributor_payment_methods) + expect(order_cycle.distributor_payment_methods).to match_array [ + distributor_payment_method, distributor_payment_method2 + ] + end + end end end @@ -271,22 +342,90 @@ describe OrderCycleForm do context "updating shipping methods" do context "and it's valid" do - it "saves the changes" do - distributor = create(:distributor_enterprise) - distributor_shipping_method = create( - :shipping_method, - distributors: [distributor] - ).distributor_shipping_methods.first - order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + let(:distributor){ create(:distributor_enterprise) } + let(:shipping_method){ create(:shipping_method, distributors: [distributor]) } + let(:shipping_method2){ create(:shipping_method, distributors: [distributor]) } + let(:distributor_shipping_method){ shipping_method.distributor_shipping_methods.first } + let(:distributor_shipping_method2){ shipping_method2.distributor_shipping_methods.first } + let(:supplier){ create(:supplier_enterprise) } + context "the submitter is a coordinator" do + it "saves the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) - form = OrderCycleForm.new( - order_cycle, - { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, - order_cycle.coordinator - ) + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + order_cycle.coordinator.users.first + ) - expect(form.save).to be true - expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + expect(form.save).to be true + expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + end + end + context "submitter is a supplier" do + it "doesn't save the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor], + suppliers: [supplier]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + supplier.users.first + ) + + expect(form).not_to receive(:attach_selected_distributor_shipping_methods) + expect(order_cycle.distributor_shipping_methods).to match_array [ + distributor_shipping_method, distributor_shipping_method2 + ] + end + end + context "submitter is an admin" do + it "saves the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + create(:admin_user) + ) + + expect(form.save).to be true + expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + end + end + context "submitter is a distributor" do + context "can update his own shipping methods" do + it "saves the changes" do + order_cycle = create(:distributor_order_cycle, distributors: [distributor]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + distributor.users.first + ) + + expect(form.save).to be true + expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + end + end + context "can't update other distributors' shipping methods" do + let(:distributor2){ create(:distributor_enterprise) } + it "doesn't save the changes" do + order_cycle = create(:distributor_order_cycle, + distributors: [distributor, distributor2]) + + form = OrderCycleForm.new( + order_cycle, + { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + distributor2.users.first + ) + + expect(form).not_to receive(:attach_selected_distributor_shipping_methods) + expect(order_cycle.distributor_shipping_methods).to match_array [ + distributor_shipping_method, distributor_shipping_method2 + ] + end + end end end From e705aa51bd9f56657e8853ca817ff8abdb2f0ae9 Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Thu, 23 Feb 2023 07:12:47 +0100 Subject: [PATCH 2/5] fix existing tests: OrderCycleForm expects an instance of Spree::User (and not an Enterprise) --- spec/services/order_cycle_form_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index 18e7b03fcd..3c6b18c685 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -331,7 +331,7 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, { selected_distributor_payment_method_ids: [distributor_payment_method_ii.id] }, - order_cycle.coordinator + order_cycle.coordinator.users.first ) expect(form.save).to be true @@ -447,7 +447,7 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, { selected_distributor_shipping_method_ids: [distributor_shipping_method_ii.id] }, - order_cycle.coordinator + order_cycle.coordinator.users.first ) expect(form.save).to be true From 28f0d6954037a5028b9696e21a02cdb13711c3ce Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Thu, 23 Feb 2023 07:22:49 +0100 Subject: [PATCH 3/5] Update app/services/order_cycle_form.rb Co-authored-by: David Cook --- app/services/order_cycle_form.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index 9f99d6ffa6..e588358a3a 100644 --- a/app/services/order_cycle_form.rb +++ b/app/services/order_cycle_form.rb @@ -78,6 +78,10 @@ class OrderCycleForm return if @selected_distributor_shipping_method_ids.nil? if distributor_only? + # A distributor can only update methods associated with their own + # enterprise, so we load all previously selected methods, and replace + # only the distributor's methods with their selection (not touching other + # distributor's methods). shipping_method_ids = order_cycle.selected_distributor_shipping_method_ids shipping_method_ids -= user_distributor_shipping_method_ids shipping_method_ids += user_only_selected_distributor_shipping_method_ids From 13383316afa5df7fdd60278779fda7d3cc274dcd Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Thu, 23 Feb 2023 08:11:11 +0100 Subject: [PATCH 4/5] fix tests --- spec/services/order_cycle_form_spec.rb | 42 +++++++++++++------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index 3c6b18c685..d673d300c3 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -226,21 +226,23 @@ describe OrderCycleForm do let!(:distributor){ create(:distributor_enterprise) } let!(:payment_method){ create(:payment_method, distributors: [distributor]) } let!(:payment_method2){ create(:payment_method, distributors: [distributor]) } - let!(:distributor_payment_method){ payment_method.distributor_payment_methods.first } - let!(:distributor_payment_method2){ payment_method2.distributor_payment_methods.first } + let!(:distributor_payment_method){ distributor.distributor_payment_methods.first.id } + let!(:distributor_payment_method2){ distributor.distributor_payment_methods.second.id } let!(:supplier){ create(:supplier_enterprise) } + context "the submitter is a coordinator" do it "saves the changes" do order_cycle = create(:distributor_order_cycle, distributors: [distributor]) form = OrderCycleForm.new( order_cycle, - { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + { selected_distributor_payment_method_ids: [distributor_payment_method] }, order_cycle.coordinator.users.first ) - expect(form.save).to be true - expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + expect{ form.save }.to change{ order_cycle.distributor_payment_methods.pluck(:id) } + .from([distributor_payment_method, distributor_payment_method2]) + .to([distributor_payment_method]) end end @@ -251,14 +253,11 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + { selected_distributor_payment_method_ids: [distributor_payment_method] }, supplier.users.first ) - expect(form).not_to receive(:attach_selected_distributor_payment_methods) - expect(order_cycle.distributor_payment_methods).to match_array [ - distributor_payment_method, distributor_payment_method2 - ] + expect{ form.save }.to_not change{ order_cycle.distributor_payment_methods.pluck(:id) } end end @@ -268,12 +267,13 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + { selected_distributor_payment_method_ids: [distributor_payment_method2] }, create(:admin_user) ) - expect(form.save).to be true - expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + expect{ form.save }.to change{ order_cycle.distributor_payment_methods.pluck(:id) } + .from([distributor_payment_method, distributor_payment_method2]) + .to([distributor_payment_method2]) end end @@ -284,12 +284,13 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + { selected_distributor_payment_method_ids: [distributor_payment_method] }, distributor.users.first ) - expect(form.save).to be true - expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method] + expect{ form.save }.to change{ order_cycle.distributor_payment_methods.pluck(:id) } + .from([distributor_payment_method, distributor_payment_method2]) + .to([distributor_payment_method]) end end context "can't update other distributors' payment methods" do @@ -300,14 +301,13 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_payment_method_ids: [distributor_payment_method.id] }, + { selected_distributor_payment_method_ids: [distributor_payment_method] }, distributor2.users.first ) - expect(form).not_to receive(:attach_selected_distributor_payment_methods) - expect(order_cycle.distributor_payment_methods).to match_array [ - distributor_payment_method, distributor_payment_method2 - ] + expect{ form.save }.to_not change{ + order_cycle.distributor_payment_methods.pluck(:id) + } end end end From 25d8ce17374ea91468e5e7f586a20ea2052341ad Mon Sep 17 00:00:00 2001 From: Mohamed ABDELLANI Date: Mon, 27 Feb 2023 15:56:14 +0100 Subject: [PATCH 5/5] fix shipping methods related tests --- spec/services/order_cycle_form_spec.rb | 64 +++++++++++++++----------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index d673d300c3..2d0cbf46f4 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -334,19 +334,21 @@ describe OrderCycleForm do order_cycle.coordinator.users.first ) - expect(form.save).to be true - expect(order_cycle.distributor_payment_methods).to eq [distributor_payment_method_i] + expect{ form.save }.not_to change{ + order_cycle.distributor_payment_methods.pluck(:id) + }.from([distributor_payment_method_i.id]) end end end context "updating shipping methods" do context "and it's valid" do - let(:distributor){ create(:distributor_enterprise) } - let(:shipping_method){ create(:shipping_method, distributors: [distributor]) } - let(:shipping_method2){ create(:shipping_method, distributors: [distributor]) } - let(:distributor_shipping_method){ shipping_method.distributor_shipping_methods.first } - let(:distributor_shipping_method2){ shipping_method2.distributor_shipping_methods.first } + let!(:distributor){ create(:distributor_enterprise) } + let!(:shipping_method){ create(:shipping_method, distributors: [distributor]) } + let!(:shipping_method2){ create(:shipping_method, distributors: [distributor]) } + let!(:distributor_shipping_method){ distributor.distributor_shipping_methods.first.id } + let!(:distributor_shipping_method2){ distributor.distributor_shipping_methods.second.id } + let(:supplier){ create(:supplier_enterprise) } context "the submitter is a coordinator" do it "saves the changes" do @@ -354,12 +356,13 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + { selected_distributor_shipping_method_ids: [distributor_shipping_method] }, order_cycle.coordinator.users.first ) - expect(form.save).to be true - expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + expect{ form.save }.to change{ order_cycle.distributor_shipping_methods.pluck(:id) } + .from([distributor_shipping_method, distributor_shipping_method2]) + .to([distributor_shipping_method]) end end context "submitter is a supplier" do @@ -369,14 +372,13 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + { selected_distributor_shipping_method_ids: [distributor_shipping_method] }, supplier.users.first ) - expect(form).not_to receive(:attach_selected_distributor_shipping_methods) - expect(order_cycle.distributor_shipping_methods).to match_array [ - distributor_shipping_method, distributor_shipping_method2 - ] + expect{ form.save }.not_to change{ + order_cycle.distributor_shipping_methods.pluck(:id) + }.from([distributor_shipping_method, distributor_shipping_method2]) end end context "submitter is an admin" do @@ -385,12 +387,13 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + { selected_distributor_shipping_method_ids: [distributor_shipping_method] }, create(:admin_user) ) - expect(form.save).to be true - expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + expect{ form.save }.to change{ order_cycle.distributor_shipping_methods.pluck(:id) } + .from([distributor_shipping_method, distributor_shipping_method2]) + .to([distributor_shipping_method]) end end context "submitter is a distributor" do @@ -400,29 +403,38 @@ describe OrderCycleForm do form = OrderCycleForm.new( order_cycle, - { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + { selected_distributor_shipping_method_ids: [distributor_shipping_method] }, distributor.users.first ) - expect(form.save).to be true - expect(order_cycle.distributor_shipping_methods).to eq [distributor_shipping_method] + expect{ form.save }.to change{ + order_cycle.distributor_shipping_methods.pluck(:id) + }.from([ + distributor_shipping_method, distributor_shipping_method2 + ]).to([distributor_shipping_method]) end end context "can't update other distributors' shipping methods" do - let(:distributor2){ create(:distributor_enterprise) } + let!(:distributor2){ create(:distributor_enterprise) } + let!(:shipping_method3){ create(:shipping_method, distributors: [distributor2]) } + let!(:distributor_shipping_method3){ + distributor2.distributor_shipping_methods.first.id + } it "doesn't save the changes" do order_cycle = create(:distributor_order_cycle, distributors: [distributor, distributor2]) form = OrderCycleForm.new( order_cycle, - { selected_distributor_shipping_method_ids: [distributor_shipping_method.id] }, + { selected_distributor_shipping_method_ids: [distributor_shipping_method] }, distributor2.users.first ) - expect(form).not_to receive(:attach_selected_distributor_shipping_methods) - expect(order_cycle.distributor_shipping_methods).to match_array [ - distributor_shipping_method, distributor_shipping_method2 + expect{ form.save }.not_to change{ + order_cycle.distributor_shipping_methods.pluck(:id) + }.from [ + distributor_shipping_method, distributor_shipping_method2, + distributor_shipping_method3 ] end end