From 02f8f293da0e60e4ccff46b0b2d0e9ac757ad83f Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Sat, 11 Apr 2015 18:32:04 +1000 Subject: [PATCH] Only allow managers or coordinator to add/remove fees from exchanges --- .../order_cycles/_exchange_form.html.haml | 4 +- .../order_cycle_form_applicator.rb | 22 ++- .../order_cycle_form_applicator_spec.rb | 184 ++++++++++++------ 3 files changed, 136 insertions(+), 74 deletions(-) diff --git a/app/views/admin/order_cycles/_exchange_form.html.haml b/app/views/admin/order_cycles/_exchange_form.html.haml index d26691cc30..7f0fb983f9 100644 --- a/app/views/admin/order_cycles/_exchange_form.html.haml +++ b/app/views/admin/order_cycles/_exchange_form.html.haml @@ -13,7 +13,7 @@ %br/ = text_field_tag 'order_cycle_outgoing_exchange_{{ $index }}_pickup_instructions', '', 'id' => 'order_cycle_outgoing_exchange_{{ $index }}_pickup_instructions', 'placeholder' => 'Pick-up instructions', 'ng-model' => 'exchange.pickup_instructions', 'ng-disabled' => '!enterprises[exchange.enterprise_id].managed && !order_cycle.viewing_as_coordinator' %td.fees - %ol + %ol{ ng: { show: 'enterprises[exchange.enterprise_id].managed || order_cycle.viewing_as_coordinator' } } %li{'ng-repeat' => 'enterprise_fee in exchange.enterprise_fees'} = select_tag 'order_cycle_{{ exchangeDirection(exchange) }}_exchange_{{ $parent.$index }}_enterprise_fees_{{ $index }}_enterprise_id', nil, {'id' => 'order_cycle_{{ exchangeDirection(exchange) }}_exchange_{{ $parent.$index }}_enterprise_fees_{{ $index }}_enterprise_id', 'ng-model' => 'enterprise_fee.enterprise_id', 'ng-options' => 'enterprise.id as enterprise.name for enterprise in enterprisesWithFees()'} @@ -21,6 +21,6 @@ = link_to 'Remove', '#', {'id' => 'order_cycle_{{ exchangeDirection(exchange) }}_exchange_{{ $parent.$index }}_enterprise_fees_{{ $index }}_remove', 'ng-click' => 'removeExchangeFee($event, exchange, $index)'} - = f.submit 'Add fee', 'ng-click' => 'addExchangeFee($event, exchange)' + = f.submit 'Add fee', 'ng-click' => 'addExchangeFee($event, exchange)', 'ng-hide' => '!enterprises[exchange.enterprise_id].managed && !order_cycle.viewing_as_coordinator' %td.actions %a{'ng-click' => 'removeExchange($event, exchange)', :class => "icon-trash no-text remove-exchange"} diff --git a/lib/open_food_network/order_cycle_form_applicator.rb b/lib/open_food_network/order_cycle_form_applicator.rb index c23df65e8c..3447699538 100644 --- a/lib/open_food_network/order_cycle_form_applicator.rb +++ b/lib/open_food_network/order_cycle_form_applicator.rb @@ -60,7 +60,7 @@ module OpenFoodNetwork attrs = attrs.reverse_merge(:sender_id => sender_id, :receiver_id => receiver_id, :incoming => incoming) exchange = @order_cycle.exchanges.build attrs - if permission_for exchange + if manages_coordinator? exchange.save! @touched_exchanges << exchange end @@ -69,6 +69,12 @@ module OpenFoodNetwork def update_exchange(sender_id, receiver_id, incoming, attrs={}) exchange = @order_cycle.exchanges.where(:sender_id => sender_id, :receiver_id => receiver_id, :incoming => incoming).first + unless manages_coordinator? || manager_for(exchange) + attrs.delete :enterprise_fee_ids + attrs.delete :pickup_time + attrs.delete :pickup_instructions + end + if permission_for exchange exchange.update_attributes!(attrs) @touched_exchanges << exchange @@ -76,8 +82,8 @@ module OpenFoodNetwork end def destroy_untouched_exchanges - if user_manages_coordinator? - with_permission(untouched_exchanges).each(&:destroy) + if manages_coordinator? + untouched_exchanges.each(&:destroy) end end @@ -86,8 +92,8 @@ module OpenFoodNetwork @order_cycle.exchanges.reject { |ex| touched_exchange_ids.include? ex.id } end - def with_permission(exchanges) - exchanges.select { |ex| permission_for(ex) } + def manager_for(exchange) + Enterprise.managed_by(@spree_current_user).include? exchange.participant end def permission_for(exchange) @@ -100,9 +106,9 @@ module OpenFoodNetwork order_cycle_enterprises_for(@order_cycle) end - def user_manages_coordinator? - return @user_manages_coordinator unless @user_manages_coordinator.nil? - @user_manages_coordinator = Enterprise.managed_by(@spree_current_user).include? @order_cycle.coordinator + def manages_coordinator? + return @manages_coordinator unless @manages_coordinator.nil? + @manages_coordinator = Enterprise.managed_by(@spree_current_user).include? @order_cycle.coordinator end def editable_variant_ids_for_incoming_exchange_between(sender, receiver) 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 e7761c6193..6a03964039 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 @@ -120,26 +120,27 @@ 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, user) - applicator.stub(:user_manages_coordinator?) { true } - exchanges = double(:exchanges) - permitted_exchanges = [double(:exchange), double(:exchange)] + context "as a manager of the coordinator" do + let(:applicator) { OrderCycleFormApplicator.new(nil, user) } + before { applicator.stub(:manages_coordinator?) { true } } - applicator.should_receive(:with_permission).with(exchanges) { permitted_exchanges } - applicator.stub(:untouched_exchanges) { exchanges } - permitted_exchanges.each { |ex| ex.should_receive(:destroy) } + it "destroys exchanges" do + exchanges = [double(:exchange), double(:exchange)] + expect(applicator).to receive(:untouched_exchanges) { exchanges } + exchanges.each { |ex| expect(ex).to receive(:destroy) } - applicator.send(:destroy_untouched_exchanges) + applicator.send(:destroy_untouched_exchanges) + end end - it "does not destroy any exchanges when it is not the coordinator" do - applicator = OrderCycleFormApplicator.new(nil, user) - applicator.stub(:user_manages_coordinator?) { false } + context "as a non-manager of the coordinator" do + let(:applicator) { OrderCycleFormApplicator.new(nil, user) } + before { applicator.stub(:manages_coordinator?) { false } } - expect(applicator).to_not receive(:with_permission) - - applicator.send(:destroy_untouched_exchanges) + it "does not destroy any exchanges" do + expect(applicator).to_not receive(:with_permission) + applicator.send(:destroy_untouched_exchanges) + end end end @@ -255,15 +256,6 @@ module OpenFoodNetwork applicator.send(:permission_for, ex).should be_false end end - - describe "filtering many exchanges" do - it "returns exchanges involving enterprises we have permission to touch" do - ex1, ex2 = double(:exchange), double(:exchange) - applicator = OrderCycleFormApplicator.new(nil, user) - applicator.stub(:permission_for).and_return(true, false) - applicator.send(:with_permission, [ex1, ex2]).should == [ex1] - end - end end end @@ -285,55 +277,119 @@ module OpenFoodNetwork applicator.send(:exchange_exists?, 999999, 888888, exchange.incoming).should be_false end - it "adds exchanges" do - sender = FactoryGirl.create(:enterprise) - receiver = FactoryGirl.create(:enterprise) - oc = FactoryGirl.create(:simple_order_cycle) - applicator = OrderCycleFormApplicator.new(oc, user) - applicator.stub(:permitted_enterprises) { [sender, receiver] } - incoming = true - variant1 = FactoryGirl.create(:variant) - variant2 = FactoryGirl.create(:variant) - enterprise_fee1 = FactoryGirl.create(:enterprise_fee) - enterprise_fee2 = FactoryGirl.create(:enterprise_fee) + describe "adding exchanges" do + let!(:sender) { create(:enterprise) } + let!(:receiver) { create(:enterprise) } + let!(:oc) { create(:simple_order_cycle) } + let!(:applicator) { OrderCycleFormApplicator.new(oc, user) } + let!(:incoming) { true } + let!(:variant1) { create(:variant) } + let!(:variant2) { create(:variant) } + let!(:enterprise_fee1) { create(:enterprise_fee) } + let!(:enterprise_fee2) { create(:enterprise_fee) } - applicator.send(:touched_exchanges=, []) - applicator.send(:add_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant2.id], :enterprise_fee_ids => [enterprise_fee1.id, enterprise_fee2.id]}) + context "as a manager of the coorindator" do + before do + allow(applicator).to receive(:manages_coordinator?) { true } + applicator.send(:touched_exchanges=, []) + applicator.send(:add_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant2.id], :enterprise_fee_ids => [enterprise_fee1.id, enterprise_fee2.id]}) + end - exchange = Exchange.last - exchange.sender.should == sender - exchange.receiver.should == receiver - exchange.incoming.should == incoming - exchange.variants.sort.should == [variant1, variant2].sort - exchange.enterprise_fees.sort.should == [enterprise_fee1, enterprise_fee2].sort + it "adds new exchanges" do + exchange = Exchange.last + expect(exchange.sender).to eq sender + expect(exchange.receiver).to eq receiver + expect(exchange.incoming).to eq incoming + expect(exchange.variants.sort).to eq [variant1, variant2].sort + expect(exchange.enterprise_fees.sort).to eq [enterprise_fee1, enterprise_fee2].sort - applicator.send(:touched_exchanges).should == [exchange] + applicator.send(:touched_exchanges).should == [exchange] + end + end + + context "as a user which does not manage the coorindator" do + before do + allow(applicator).to receive(:manages_coordinator?) { false } + applicator.send(:add_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant2.id], :enterprise_fee_ids => [enterprise_fee1.id, enterprise_fee2.id]}) + end + + it "does not add new exchanges" do + expect(Exchange.last).to be_nil + end + end end - it "updates exchanges" do - sender = FactoryGirl.create(:enterprise) - receiver = FactoryGirl.create(:enterprise) - oc = FactoryGirl.create(:simple_order_cycle) - applicator = OrderCycleFormApplicator.new(oc, user) - applicator.stub(:permitted_enterprises) { [sender, receiver] } + describe "updating exchanges" do + let!(:sender) { create(:enterprise) } + let!(:receiver) { create(:enterprise) } + let!(:oc) { create(:simple_order_cycle) } + let!(:applicator) { OrderCycleFormApplicator.new(oc, user) } + let!(:incoming) { true } + let!(:variant1) { create(:variant) } + let!(:variant2) { create(:variant) } + let!(:variant3) { create(:variant) } + let!(:enterprise_fee1) { create(:enterprise_fee) } + let!(:enterprise_fee2) { create(:enterprise_fee) } + let!(:enterprise_fee3) { create(:enterprise_fee) } - incoming = true - variant1 = FactoryGirl.create(:variant) - variant2 = FactoryGirl.create(:variant) - variant3 = FactoryGirl.create(:variant) - enterprise_fee1 = FactoryGirl.create(:enterprise_fee) - enterprise_fee2 = FactoryGirl.create(:enterprise_fee) - enterprise_fee3 = FactoryGirl.create(:enterprise_fee) + let!(:exchange) { create(:exchange, order_cycle: oc, sender: sender, receiver: receiver, incoming: incoming, variant_ids: [variant1.id, variant2.id], enterprise_fee_ids: [enterprise_fee1.id, enterprise_fee2.id]) } - exchange = FactoryGirl.create(:exchange, order_cycle: oc, sender: sender, receiver: receiver, incoming: incoming, variant_ids: [variant1.id, variant2.id], enterprise_fee_ids: [enterprise_fee1.id, enterprise_fee2.id]) + context "as a manager of the coorindator" do + before do + allow(applicator).to receive(:manages_coordinator?) { true } + allow(applicator).to receive(:manager_for) { false } + allow(applicator).to receive(:permission_for) { true } + applicator.send(:touched_exchanges=, []) + applicator.send(:update_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant3.id], :enterprise_fee_ids => [enterprise_fee2.id, enterprise_fee3.id], :pickup_time => 'New Pickup Time', :pickup_instructions => 'New Pickup Instructions'}) + end - applicator.send(:touched_exchanges=, []) - applicator.send(:update_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant3.id], :enterprise_fee_ids => [enterprise_fee2.id, enterprise_fee3.id]}) + it "updates the variants, enterprise fees and pickup information of the exchange" do + exchange.reload + expect(exchange.variants.sort).to eq [variant1, variant3].sort + expect(exchange.enterprise_fees.sort).to eq [enterprise_fee2, enterprise_fee3] + expect(exchange.pickup_time).to eq 'New Pickup Time' + expect(exchange.pickup_instructions).to eq 'New Pickup Instructions' + expect(applicator.send(:touched_exchanges)).to eq [exchange] + end + end - exchange.reload - exchange.variants.sort.should == [variant1, variant3].sort - exchange.enterprise_fees.sort.should == [enterprise_fee2, enterprise_fee3] - applicator.send(:touched_exchanges).should == [exchange] + context "as a manager of the participating enterprise" do + before do + allow(applicator).to receive(:manages_coordinator?) { false } + allow(applicator).to receive(:manager_for) { true } + allow(applicator).to receive(:permission_for) { true } + applicator.send(:touched_exchanges=, []) + applicator.send(:update_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant3.id], :enterprise_fee_ids => [enterprise_fee2.id, enterprise_fee3.id], :pickup_time => 'New Pickup Time', :pickup_instructions => 'New Pickup Instructions'}) + end + + it "updates the variants, enterprise fees and pickup information of the exchange" do + exchange.reload + expect(exchange.variants.sort).to eq [variant1, variant3].sort + expect(exchange.enterprise_fees.sort).to eq [enterprise_fee2, enterprise_fee3] + expect(exchange.pickup_time).to eq 'New Pickup Time' + expect(exchange.pickup_instructions).to eq 'New Pickup Instructions' + expect(applicator.send(:touched_exchanges)).to eq [exchange] + end + end + + context "where the participating enterprise is permitted for the user" do + before do + allow(applicator).to receive(:manages_coordinator?) { false } + allow(applicator).to receive(:manager_for) { false } + allow(applicator).to receive(:permission_for) { true } + applicator.send(:touched_exchanges=, []) + applicator.send(:update_exchange, sender.id, receiver.id, incoming, {:variant_ids => [variant1.id, variant3.id], :enterprise_fee_ids => [enterprise_fee2.id, enterprise_fee3.id], :pickup_time => 'New Pickup Time', :pickup_instructions => 'New Pickup Instructions'}) + end + + it "updates the variants in the exchange, but not the fees or pickup information" do + exchange.reload + expect(exchange.variants.sort).to eq [variant1, variant3].sort + expect(exchange.enterprise_fees.sort).to eq [enterprise_fee1, enterprise_fee2] + expect(exchange.pickup_time).to_not eq 'New Pickup Time' + expect(exchange.pickup_instructions).to_not eq 'New Pickup Instructions' + expect(applicator.send(:touched_exchanges)).to eq [exchange] + end + end end it "does not add exchanges it is not permitted to touch" do