diff --git a/app/controllers/admin/enterprise_fees_controller.rb b/app/controllers/admin/enterprise_fees_controller.rb index 016f697856..13e27e05aa 100644 --- a/app/controllers/admin/enterprise_fees_controller.rb +++ b/app/controllers/admin/enterprise_fees_controller.rb @@ -76,10 +76,10 @@ module Admin def collection case action when :for_order_cycle - options = {} - options[:coordinator] = Enterprise.find(params[:coordinator_id]) if params[:coordinator_id] - options[:order_cycle] = OrderCycle.find(params[:order_cycle_id]) if params[:order_cycle_id] - enterprises = OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(options) + order_cycle = OrderCycle.find_by_id(params[:order_cycle_id]) if params[:order_cycle_id] + coordinator = Enterprise.find_by_id(params[:coordinator_id]) if params[:coordinator_id] + order_cycle = OrderCycle.new(coordinator: coordinator) if order_cycle.nil? && coordinator.present? + enterprises = OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle) return EnterpriseFee.for_enterprises(enterprises).order('enterprise_id', 'fee_type', 'name') else collection = EnterpriseFee.managed_by(spree_current_user).order('enterprise_id', 'fee_type', 'name') diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index e745d22909..1b8b010da1 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -95,10 +95,10 @@ module Admin def collection case action when :for_order_cycle - options = {} - options[:coordinator] = Enterprise.find(params[:coordinator_id]) if params[:coordinator_id] - options[:order_cycle] = OrderCycle.find(params[:order_cycle_id]) if params[:order_cycle_id] - return OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(options) + order_cycle = OrderCycle.find_by_id(params[:order_cycle_id]) if params[:order_cycle_id] + coordinator = Enterprise.find_by_id(params[:coordinator_id]) if params[:coordinator_id] + order_cycle = OrderCycle.new(coordinator: coordinator) if order_cycle.nil? && coordinator.present? + return OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle) else # TODO was ordered with is_distributor DESC as well, not sure why or how we want to sort this now OpenFoodNetwork::Permissions.new(spree_current_user). diff --git a/app/helpers/order_cycles_helper.rb b/app/helpers/order_cycles_helper.rb index 39727bbddb..7aa112816e 100644 --- a/app/helpers/order_cycles_helper.rb +++ b/app/helpers/order_cycles_helper.rb @@ -4,7 +4,7 @@ module OrderCyclesHelper end def permitted_enterprises_for(order_cycle) - OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle: order_cycle) + OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle) end def permitted_producer_enterprises_for(order_cycle) diff --git a/app/serializers/api/admin/order_cycle_serializer.rb b/app/serializers/api/admin/order_cycle_serializer.rb index 53976ee09a..3f2bad8ffa 100644 --- a/app/serializers/api/admin/order_cycle_serializer.rb +++ b/app/serializers/api/admin/order_cycle_serializer.rb @@ -23,7 +23,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer # work out which variants should be editable within incoming exchanges from that enterprise editable = {} permissions = OpenFoodNetwork::Permissions.new(options[:current_user]) - enterprises = permissions.order_cycle_enterprises_for(order_cycle: object) + enterprises = permissions.order_cycle_enterprises_for(object) enterprises.each do |enterprise| variants = permissions.editable_variants_for_incoming_exchanges_between(enterprise, object.coordinator, order_cycle: object).pluck(:id) editable[enterprise.id] = variants if variants.any? @@ -36,7 +36,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer # work out which variants should be editable within incoming exchanges from that enterprise editable = {} permissions = OpenFoodNetwork::Permissions.new(options[:current_user]) - enterprises = permissions.order_cycle_enterprises_for(order_cycle: object) + enterprises = permissions.order_cycle_enterprises_for(object) enterprises.each do |enterprise| variants = permissions.editable_variants_for_outgoing_exchanges_between(object.coordinator, enterprise, order_cycle: object).pluck(:id) editable[enterprise.id] = variants if variants.any? @@ -49,7 +49,7 @@ class Api::Admin::OrderCycleSerializer < ActiveModel::Serializer # work out which variants should be visible within outgoing exchanges from that enterprise visible = {} permissions = OpenFoodNetwork::Permissions.new(options[:current_user]) - enterprises = permissions.order_cycle_enterprises_for(order_cycle: object) + enterprises = permissions.order_cycle_enterprises_for(object) enterprises.each do |enterprise| variants = permissions.visible_variants_for_outgoing_exchanges_between(object.coordinator, enterprise, order_cycle: object).pluck(:id) visible[enterprise.id] = variants if variants.any? diff --git a/app/views/admin/order_cycles/_row.html.haml b/app/views/admin/order_cycles/_row.html.haml index b1d01c74bf..7cf8ea0f7a 100644 --- a/app/views/admin/order_cycles/_row.html.haml +++ b/app/views/admin/order_cycles/_row.html.haml @@ -8,7 +8,7 @@ - unless order_cycles_simple_index %td.suppliers - - suppliers = order_cycle.suppliers.merge(OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle: order_cycle)) + - suppliers = order_cycle.suppliers.merge(OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle)) - supplier_list = suppliers.map(&:name).sort.join ', ' - if suppliers.count > 3 %span.with-tip{'data-powertip' => supplier_list} @@ -18,7 +18,7 @@ = supplier_list %td= order_cycle.coordinator.name %td.distributors - - distributors = order_cycle.distributors.merge(OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle: order_cycle)) + - distributors = order_cycle.distributors.merge(OpenFoodNetwork::Permissions.new(spree_current_user).order_cycle_enterprises_for(order_cycle)) - distributor_list = distributors.map(&:name).sort.join ', ' - if distributors.count > 3 %span.with-tip{'data-powertip' => distributor_list} diff --git a/lib/open_food_network/order_cycle_form_applicator.rb b/lib/open_food_network/order_cycle_form_applicator.rb index a5ed5fefea..c23df65e8c 100644 --- a/lib/open_food_network/order_cycle_form_applicator.rb +++ b/lib/open_food_network/order_cycle_form_applicator.rb @@ -97,7 +97,7 @@ module OpenFoodNetwork def permitted_enterprises return @permitted_enterprises unless @permitted_enterprises.nil? @permitted_enterprises = OpenFoodNetwork::Permissions.new(@spree_current_user). - order_cycle_enterprises_for(order_cycle: @order_cycle) + order_cycle_enterprises_for(@order_cycle) end def user_manages_coordinator? diff --git a/lib/open_food_network/permissions.rb b/lib/open_food_network/permissions.rb index fcad0e829b..03e5d491e4 100644 --- a/lib/open_food_network/permissions.rb +++ b/lib/open_food_network/permissions.rb @@ -19,16 +19,9 @@ module OpenFoodNetwork # NOTE: the enterprises a given user can see actually in the OC interface depend on the relationships # of their enterprises to the coordinator of the order cycle, rather than on the order cycle itself # (until such time as we implement friends of friends) - def order_cycle_enterprises_for(options={}) - # Can provide a coordinator OR an order cycle. Use just coordinator for new order cycles - # if both are provided, coordinator will be ignored, and the coordinator of the OC will be used - return Enterprise.where("1=0") unless options[:coordinator] || options[:order_cycle] - coordinator = options[:coordinator] - order_cycle = nil - if options[:order_cycle] - order_cycle = options[:order_cycle] - coordinator = order_cycle.coordinator - end + def order_cycle_enterprises_for(order_cycle) + return Enterprise.where("1=0") unless order_cycle.andand.coordinator.present? + coordinator = order_cycle.coordinator if managed_enterprises.include? coordinator coordinator_permitted = [] diff --git a/spec/controllers/admin/enterprises_controller_spec.rb b/spec/controllers/admin/enterprises_controller_spec.rb index 6cbe2c6f95..742fceb3d2 100644 --- a/spec/controllers/admin/enterprises_controller_spec.rb +++ b/spec/controllers/admin/enterprises_controller_spec.rb @@ -414,8 +414,9 @@ module Admin before do # As a user with permission controller.stub spree_current_user: user - Enterprise.stub find: "instance of Enterprise" - OrderCycle.stub find: "instance of OrderCycle" + OrderCycle.stub find_by_id: "existing OrderCycle" + Enterprise.stub find_by_id: "existing Enterprise" + OrderCycle.stub new: "new OrderCycle" OpenFoodNetwork::Permissions.stub(:new) { permission_mock } allow(permission_mock).to receive :order_cycle_enterprises_for @@ -424,28 +425,28 @@ module Admin context "when no order_cycle or coordinator is provided in params" do before { spree_get :for_order_cycle } it "returns an empty scope" do - expect(permission_mock).to have_received(:order_cycle_enterprises_for).with({}) + expect(permission_mock).to have_received(:order_cycle_enterprises_for).with(nil) end end context "when an order_cycle_id is provided in params" do before { spree_get :for_order_cycle, order_cycle_id: 1 } - it "calls order_cycle_enterprises_for() with an :order_cycle option" do - expect(permission_mock).to have_received(:order_cycle_enterprises_for).with(order_cycle: "instance of OrderCycle") + it "calls order_cycle_enterprises_for() with the existing OrderCycle" do + expect(permission_mock).to have_received(:order_cycle_enterprises_for).with("existing OrderCycle") end end context "when a coordinator is provided in params" do before { spree_get :for_order_cycle, coordinator_id: 1 } - it "calls order_cycle_enterprises_for() with a :coordinator option" do - expect(permission_mock).to have_received(:order_cycle_enterprises_for).with(coordinator: "instance of Enterprise") + it "calls order_cycle_enterprises_for() with a new OrderCycle" do + expect(permission_mock).to have_received(:order_cycle_enterprises_for).with("new OrderCycle") end end context "when both an order cycle and a coordinator are provided in params" do before { spree_get :for_order_cycle, order_cycle_id: 1, coordinator_id: 1 } - it "calls order_cycle_enterprises_for() with both options" do - expect(permission_mock).to have_received(:order_cycle_enterprises_for).with(coordinator: "instance of Enterprise", order_cycle: "instance of OrderCycle") + it "calls order_cycle_enterprises_for() with the existing OrderCycle" do + expect(permission_mock).to have_received(:order_cycle_enterprises_for).with("existing OrderCycle") end end end diff --git a/spec/lib/open_food_network/permissions_spec.rb b/spec/lib/open_food_network/permissions_spec.rb index adb319d801..31566cbe54 100644 --- a/spec/lib/open_food_network/permissions_spec.rb +++ b/spec/lib/open_food_network/permissions_spec.rb @@ -28,15 +28,19 @@ module OpenFoodNetwork let(:oc) { create(:simple_order_cycle, coordinator: coordinator) } context "when no order_cycle or coordinator are provided for reference" do + before do + permissions.stub(:managed_enterprises) { Enterprise.where(id: [coordinator]) } + end + it "returns an empty scope" do - expect(permissions.order_cycle_enterprises_for()).to be_empty + expect(permissions.order_cycle_enterprises_for(nil)).to be_empty end end context "as a manager of the coordinator" do it "returns the coordinator itself" do permissions.stub(:managed_enterprises) { Enterprise.where(id: [coordinator]) } - expect(permissions.order_cycle_enterprises_for(order_cycle: oc)).to include coordinator + expect(permissions.order_cycle_enterprises_for(oc)).to include coordinator end context "where P-OC has been granted to the coordinator by other enterprises" do @@ -47,7 +51,7 @@ module OpenFoodNetwork context "where the coordinator sells any" do it "returns enterprises which have granted P-OC to the coordinator" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include hub expect(enterprises).to_not include producer end @@ -56,7 +60,7 @@ module OpenFoodNetwork context "where the coordinator sells 'own'" do before { coordinator.stub(:sells) { 'own' } } it "returns just the coordinator" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include hub, producer end end @@ -77,7 +81,7 @@ module OpenFoodNetwork let!(:ex_outgoing) { create(:exchange, order_cycle: oc, sender: coordinator, receiver: hub, incoming: false) } it "returns my hub" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include hub expect(enterprises).to_not include producer, coordinator end @@ -91,7 +95,7 @@ module OpenFoodNetwork let!(:ex_incoming) { create(:exchange, order_cycle: oc, sender: producer, receiver: coordinator, incoming: true) } it "returns the producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer, hub end end @@ -100,7 +104,7 @@ module OpenFoodNetwork # No incoming exchange it "does not return the producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include producer end end @@ -115,7 +119,7 @@ module OpenFoodNetwork let!(:ex_incoming) { create(:exchange, order_cycle: oc, sender: producer, receiver: coordinator, incoming: true) } it "returns the producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer, hub end end @@ -124,7 +128,7 @@ module OpenFoodNetwork # No incoming exchange it "does not return the producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include producer end end @@ -135,7 +139,7 @@ module OpenFoodNetwork # No outgoing exchange for my hub it "does not return my hub" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include hub, producer, coordinator end end @@ -143,7 +147,7 @@ module OpenFoodNetwork context "that has not granted P-OC to the coordinator" do it "does not return my hub" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include hub, producer, coordinator end @@ -151,7 +155,7 @@ module OpenFoodNetwork let!(:ex) { create(:exchange, order_cycle: oc, sender: coordinator, receiver: hub, incoming: false) } it "returns my hub" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include hub expect(enterprises).to_not include producer, coordinator end @@ -161,7 +165,7 @@ module OpenFoodNetwork # TODO: update this when we are confident about P-OCs it "returns that producer as well" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer, hub expect(enterprises).to_not include coordinator end @@ -184,7 +188,7 @@ module OpenFoodNetwork let!(:ex_incoming) { create(:exchange, order_cycle: oc, sender: producer, receiver: coordinator, incoming: true) } it "returns my producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer expect(enterprises).to_not include hub, coordinator end @@ -198,7 +202,7 @@ module OpenFoodNetwork let!(:ex_outgoing) { create(:exchange, order_cycle: oc, sender: coordinator, receiver: hub, incoming: false) } it "returns the hub as well" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer, hub expect(enterprises).to_not include coordinator end @@ -208,7 +212,7 @@ module OpenFoodNetwork # No outgoing exchange it "does not return the hub" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include hub end end @@ -223,7 +227,7 @@ module OpenFoodNetwork let!(:ex_outgoing) { create(:exchange, order_cycle: oc, sender: coordinator, receiver: hub, incoming: false) } it "returns the hub as well" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer, hub expect(enterprises).to_not include coordinator end @@ -233,7 +237,7 @@ module OpenFoodNetwork # No outgoing exchange it "does not return the hub" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include hub end end @@ -244,7 +248,7 @@ module OpenFoodNetwork # No incoming exchange for producer it "does not return my producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include hub, producer, coordinator end end @@ -252,7 +256,7 @@ module OpenFoodNetwork context "which has not granted P-OC to the coordinator" do it "does not return my producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to_not include producer end @@ -261,7 +265,7 @@ module OpenFoodNetwork # TODO: update this when we are confident about P-OCs it "returns my producer" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer expect(enterprises).to_not include hub, coordinator end @@ -272,7 +276,7 @@ module OpenFoodNetwork # TODO: update this when we are confident about P-OCs it "returns that hub as well" do - enterprises = permissions.order_cycle_enterprises_for(order_cycle: oc) + enterprises = permissions.order_cycle_enterprises_for(oc) expect(enterprises).to include producer, hub expect(enterprises).to_not include coordinator end