From 9b15c213d142c04dba147a705fba5b810a1a8caa Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 23 May 2014 11:22:04 +1000 Subject: [PATCH] When an enterprise user saves an order cycle for which it manages only some of the enterprises involved, do not delete the other exchanges --- .../admin/order_cycles_controller.rb | 4 +-- app/helpers/enterprises_helper.rb | 4 +++ app/models/exchange.rb | 4 +++ .../order_cycle_form_applicator.rb | 10 +++++-- spec/features/admin/order_cycles_spec.rb | 9 ++++++ .../order_cycle_form_applicator_spec.rb | 30 +++++++++++++++++++ spec/models/exchange_spec.rb | 22 ++++++++++---- 7 files changed, 74 insertions(+), 9 deletions(-) diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index 46273d0eff..566c660112 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/app/controllers/admin/order_cycles_controller.rb @@ -23,7 +23,7 @@ module Admin respond_to do |format| if @order_cycle.save - OpenFoodNetwork::OrderCycleFormApplicator.new(@order_cycle).go! + OpenFoodNetwork::OrderCycleFormApplicator.new(@order_cycle, managed_enterprises).go! flash[:notice] = 'Your order cycle has been created.' format.html { redirect_to admin_order_cycles_path } @@ -40,7 +40,7 @@ module Admin respond_to do |format| if @order_cycle.update_attributes(params[:order_cycle]) - OpenFoodNetwork::OrderCycleFormApplicator.new(@order_cycle).go! + OpenFoodNetwork::OrderCycleFormApplicator.new(@order_cycle, managed_enterprises).go! flash[:notice] = 'Your order cycle has been updated.' format.html { redirect_to admin_order_cycles_path } diff --git a/app/helpers/enterprises_helper.rb b/app/helpers/enterprises_helper.rb index 4b0f2f4a93..ed22831589 100644 --- a/app/helpers/enterprises_helper.rb +++ b/app/helpers/enterprises_helper.rb @@ -2,6 +2,10 @@ module EnterprisesHelper def current_distributor @current_distributor ||= current_order(false).andand.distributor end + + def managed_enterprises + Enterprise.managed_by(spree_current_user) + end def enterprises_options enterprises enterprises.map { |enterprise| [enterprise.name + ": " + enterprise.address.address1 + ", " + enterprise.address.city, enterprise.id.to_i] } diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 18c3f81447..c491748d5d 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -54,6 +54,10 @@ class Exchange < ActiveRecord::Base incoming? ? 'supplier' : 'distributor' end + def participant + incoming? ? sender : receiver + end + def to_h(core_only=false) h = attributes.merge({ 'variant_ids' => variant_ids.sort, 'enterprise_fee_ids' => enterprise_fee_ids.sort }) h.reject! { |k| %w(id order_cycle_id created_at updated_at).include? k } if core_only diff --git a/lib/open_food_network/order_cycle_form_applicator.rb b/lib/open_food_network/order_cycle_form_applicator.rb index dc5056d53a..4ef5b44e24 100644 --- a/lib/open_food_network/order_cycle_form_applicator.rb +++ b/lib/open_food_network/order_cycle_form_applicator.rb @@ -5,8 +5,10 @@ module OpenFoodNetwork # translation is more a responsibility of Angular, so I'd be inclined to refactor this class to move # as much as possible (if not all) of its logic into Angular. class OrderCycleFormApplicator - def initialize(order_cycle) + # The applicator will only touch exchanges where a permitted enterprise is the participant + def initialize(order_cycle, permitted_enterprises=[]) @order_cycle = order_cycle + @permitted_enterprises = permitted_enterprises end def go! @@ -68,7 +70,7 @@ module OpenFoodNetwork end def destroy_untouched_exchanges - untouched_exchanges.each { |exchange| exchange.destroy } + with_permission(untouched_exchanges).each(&:destroy) end def untouched_exchanges @@ -76,6 +78,10 @@ module OpenFoodNetwork @order_cycle.exchanges.reject { |ex| touched_exchange_ids.include? ex.id } end + def with_permission(exchanges) + exchanges.select { |ex| @permitted_enterprises.include? ex.participant } + end + def exchange_variant_ids(exchange) exchange[:variants].select { |k, v| v }.keys.map { |k| k.to_i } diff --git a/spec/features/admin/order_cycles_spec.rb b/spec/features/admin/order_cycles_spec.rb index 6cb3a501a4..2423a6dc05 100644 --- a/spec/features/admin/order_cycles_spec.rb +++ b/spec/features/admin/order_cycles_spec.rb @@ -514,6 +514,15 @@ feature %q{ # I should not see exchanges for supplier2 or distributor2 page.all('tr.supplier').count.should == 1 page.all('tr.distributor').count.should == 1 + + # When I save, then those exchanges should remain + click_button 'Update' + page.should have_content "Your order cycle has been updated." + + oc.reload + oc.suppliers.sort.should == [supplier1, supplier2] + oc.coordinator.should == supplier1 + oc.distributors.sort.should == [distributor1, distributor2] end scenario "cloning an order cycle" do diff --git a/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb b/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb index 81cd925569..11b7655368 100644 --- a/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb +++ b/spec/lib/open_food_network/order_cycle_form_applicator_spec.rb @@ -115,6 +115,18 @@ module OpenFoodNetwork applicator.send(:untouched_exchanges).should == [] end + + it "does not destroy exchanges involving enterprises it does not have permission to touch" do + applicator = OrderCycleFormApplicator.new(nil) + exchanges = double(:exchanges) + permitted_exchanges = [double(:exchange), double(:exchange)] + + applicator.should_receive(:with_permission).with(exchanges) { permitted_exchanges } + applicator.stub(:untouched_exchanges) { exchanges } + permitted_exchanges.each { |ex| ex.should_receive(:destroy) } + + applicator.send(:destroy_untouched_exchanges) + end end it "converts exchange variant ids hash to an array of ids" do @@ -122,6 +134,24 @@ module OpenFoodNetwork applicator.send(:exchange_variant_ids, {:enterprise_id => 123, :variants => {'1' => true, '2' => false, '3' => true}}).should == [1, 3] end + + describe "filtering exchanges for permission" do + it "returns exchanges involving enterprises we have permission to touch" do + e = double(:enterprise) + ex = double(:exchange, participant: e) + + applicator = OrderCycleFormApplicator.new(nil, [e]) + applicator.send(:with_permission, [ex]).should == [ex] + end + + it "does not return other exchanges" do + e = double(:enterprise) + ex = double(:exchange, participant: e) + + applicator = OrderCycleFormApplicator.new(nil, []) + applicator.send(:with_permission, [ex]).should == [] + end + end end context "integration specs" do diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index ed6cdbfc93..0dfceadbad 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -48,7 +48,7 @@ describe Exchange do e.enterprise_fees.count.should == 1 end - describe "reporting whether it is an incoming exchange" do + describe "exchange directionality" do let(:supplier) { create(:supplier_enterprise) } let(:coordinator) { create(:distributor_enterprise) } let(:distributor) { create(:distributor_enterprise) } @@ -56,12 +56,24 @@ describe Exchange do let(:incoming_exchange) { oc.exchanges.create! sender: supplier, receiver: coordinator, incoming: true } let(:outgoing_exchange) { oc.exchanges.create! sender: coordinator, receiver: distributor, incoming: false } - it "returns true for incoming exchanges" do - incoming_exchange.should be_incoming + describe "reporting whether it is an incoming exchange" do + it "returns true for incoming exchanges" do + incoming_exchange.should be_incoming + end + + it "returns false for outgoing exchanges" do + outgoing_exchange.should_not be_incoming + end end - it "returns false for outgoing exchanges" do - outgoing_exchange.should_not be_incoming + describe "finding the exchange participant (the enterprise other than the coordinator)" do + it "returns the sender for incoming exchanges" do + incoming_exchange.participant.should == supplier + end + + it "returns the receiver for outgoing exchanges" do + outgoing_exchange.participant.should == distributor + end end end