Merge pull request #4876 from luisramos0/strong_params_oc

[Spree 2.1] Implement strong params in admin Order cycles controller

app/controllers/admin/order_cycles_controller.rb

@@ -40,7 +40,7 @@ module Admin
     end
 
     def create
-      @order_cycle_form = OrderCycleForm.new(@order_cycle, params, spree_current_user)
+      @order_cycle_form = OrderCycleForm.new(@order_cycle, order_cycle_params, spree_current_user)
 
       if @order_cycle_form.save
         flash[:notice] = I18n.t(:order_cycles_create_notice)
@@ -56,7 +56,7 @@ module Admin
     end
 
     def update
-      @order_cycle_form = OrderCycleForm.new(@order_cycle, params, spree_current_user)
+      @order_cycle_form = OrderCycleForm.new(@order_cycle, order_cycle_params, spree_current_user)
 
       if @order_cycle_form.save
         respond_to do |format|
@@ -235,5 +235,9 @@ module Admin
     def ams_prefix_whitelist
       [:basic, :index]
     end
+
+    def order_cycle_params
+      PermittedAttributes::OrderCycle.new(params).call
+    end
   end
 end

app/services/order_cycle_form.rb

@@ -3,16 +3,16 @@ require 'open_food_network/proxy_order_syncer'
 require 'open_food_network/order_cycle_form_applicator'
 
 class OrderCycleForm
-  def initialize(order_cycle, params, user)
+  def initialize(order_cycle, order_cycle_params, user)
     @order_cycle = order_cycle
-    @params = params
+    @order_cycle_params = order_cycle_params
     @user = user
     @permissions = OpenFoodNetwork::Permissions.new(user)
   end
 
   def save
     build_schedule_ids
-    order_cycle.assign_attributes(params[:order_cycle])
+    order_cycle.assign_attributes(order_cycle_params)
     return false unless order_cycle.valid?
 
     order_cycle.transaction do
@@ -27,7 +27,7 @@ class OrderCycleForm
 
   private
 
-  attr_accessor :order_cycle, :params, :user, :permissions
+  attr_accessor :order_cycle, :order_cycle_params, :user, :permissions
 
   def apply_exchange_changes
     return if exchanges_unchanged?
@@ -37,12 +37,12 @@ class OrderCycleForm
 
   def exchanges_unchanged?
     [:incoming_exchanges, :outgoing_exchanges].all? do |direction|
-      params[:order_cycle][direction].nil?
+      order_cycle_params[direction].nil?
     end
   end
 
   def schedule_ids?
-    params[:order_cycle][:schedule_ids].present?
+    order_cycle_params[:schedule_ids].present?
   end
 
   def build_schedule_ids
@@ -51,7 +51,7 @@ class OrderCycleForm
     result = existing_schedule_ids
     result |= (requested_schedule_ids & permitted_schedule_ids) # Add permitted and requested
     result -= ((result & permitted_schedule_ids) - requested_schedule_ids) # Remove permitted but not requested
-    params[:order_cycle][:schedule_ids] = result
+    order_cycle_params[:schedule_ids] = result
   end
 
   def sync_subscriptions
@@ -70,7 +70,7 @@ class OrderCycleForm
   end
 
   def requested_schedule_ids
-    params[:order_cycle][:schedule_ids].map(&:to_i)
+    order_cycle_params[:schedule_ids].map(&:to_i)
   end
 
   def permitted_schedule_ids

app/services/permitted_attributes/order_cycle.rb

@@ -0,0 +1,46 @@
+module PermittedAttributes
+  class OrderCycle
+    def initialize(params)
+      @params = params
+    end
+
+    def call
+      return @params[:order_cycle] if @params[:order_cycle].empty?
+
+      @params.require(:order_cycle).permit(
+        :name, :orders_open_at, :orders_close_at, :coordinator_id,
+        incoming_exchanges: permitted_exchange_attributes,
+        outgoing_exchanges: permitted_exchange_attributes,
+        schedule_ids: [], coordinator_fee_ids: []
+      )
+    end
+
+    private
+
+    def permitted_exchange_attributes
+      [
+        :id, :sender_id, :receiver_id, :enterprise_id, :incoming, :active,
+        :select_all_variants, :receival_instructions,
+        :pickup_time, :pickup_instructions,
+        :tag_list, :tags => [:text],
+        :enterprise_fee_ids => [],
+        :variants => permitted_variant_ids
+      ]
+    end
+
+    # In rails 5 we will be able to permit random hash keys simply with :variants => {}
+    # See https://github.com/rails/rails/commit/e86524c0c5a26ceec92895c830d1355ae47a7034
+    #
+    # Until then, we need to create an array of variant IDs in order to permit them
+    def permitted_variant_ids
+      variant_ids(@params[:order_cycle][:incoming_exchanges]) +
+        variant_ids(@params[:order_cycle][:outgoing_exchanges])
+    end
+
+    def variant_ids(exchange_params)
+      return [] unless exchange_params
+
+      exchange_params.map { |exchange| exchange[:variants].map { |key, _value| key } }.flatten
+    end
+  end
+end

spec/controllers/admin/order_cycles_controller_spec.rb

@@ -203,7 +203,7 @@ module Admin
 
       context "as a manager of the coordinator" do
         let(:user) { coordinator.owner }
-        let(:expected) { [order_cycle, hash_including(order_cycle: allowed.merge(restricted)), user] }
+        let(:expected) { [order_cycle, allowed.merge(restricted), user] }
 
         it "allows me to update exchange information for exchanges, name and dates" do
           expect(OrderCycleForm).to receive(:new).with(*expected) { form_mock }
@@ -213,7 +213,7 @@ module Admin
 
       context "as a producer supplying to an order cycle" do
         let(:user) { producer.owner }
-        let(:expected) { [order_cycle, hash_including(order_cycle: allowed), user] }
+        let(:expected) { [order_cycle, allowed, user] }
 
         it "allows me to update exchange information for exchanges, but not name or dates" do
           expect(OrderCycleForm).to receive(:new).with(*expected) { form_mock }

spec/services/order_cycle_form_spec.rb

@@ -1,3 +1,5 @@
+require 'spec_helper'
+
 describe OrderCycleForm do
   describe "save" do
     describe "creating a new order cycle from params" do
@@ -6,7 +8,7 @@ describe OrderCycleForm do
       let(:form) { OrderCycleForm.new(order_cycle, params, shop.owner) }
 
       context "when creation is successful" do
-        let(:params) { { order_cycle: { name: "Test Order Cycle", coordinator_id: shop.id } } }
+        let(:params) { { name: "Test Order Cycle", coordinator_id: shop.id } }
 
         it "returns true" do
           expect do
@@ -16,7 +18,7 @@ describe OrderCycleForm do
       end
 
       context "when creation fails" do
-        let(:params) { { order_cycle: { name: "Test Order Cycle" } } }
+        let(:params) { { name: "Test Order Cycle" } }
 
         it "returns false" do
           expect do
@@ -32,7 +34,7 @@ describe OrderCycleForm do
       let(:form) { OrderCycleForm.new(order_cycle, params, shop.owner) }
 
       context "when update is successful" do
-        let(:params) { { order_cycle: { name: "Test Order Cycle", coordinator_id: shop.id } } }
+        let(:params) { { name: "Test Order Cycle", coordinator_id: shop.id } }
 
         it "returns true" do
           expect do
@@ -42,7 +44,7 @@ describe OrderCycleForm do
       end
 
       context "when updating fails" do
-        let(:params) { { order_cycle: { name: nil } } }
+        let(:params) { { name: nil } }
 
         it "returns false" do
           expect do
@@ -73,7 +75,7 @@ describe OrderCycleForm do
       end
 
       context "and I add an schedule that I own, and remove another that I own" do
-        let(:params) { { order_cycle: { schedule_ids: [coordinated_schedule2.id] } } }
+        let(:params) { { schedule_ids: [coordinated_schedule2.id] } }
 
         it "associates the order cycle to the schedule" do
           expect(form.save).to be true
@@ -84,7 +86,7 @@ describe OrderCycleForm do
       end
 
       context "and I add a schedule that I don't own" do
-        let(:params) { { order_cycle: { schedule_ids: [coordinated_schedule.id, uncoordinated_schedule.id] } } }
+        let(:params) { { schedule_ids: [coordinated_schedule.id, uncoordinated_schedule.id] } }
 
         it "ignores the schedule that I don't own" do
           expect(form.save).to be true
@@ -95,7 +97,7 @@ describe OrderCycleForm do
       end
 
       context "when I make no changes to the schedule ids" do
-        let(:params) { { order_cycle: { schedule_ids: [coordinated_schedule.id] } } }
+        let(:params) { { schedule_ids: [coordinated_schedule.id] } }
 
         it "ignores the schedule that I don't own" do
           expect(form.save).to be true
@@ -111,7 +113,7 @@ describe OrderCycleForm do
     let(:order_cycle) { create(:simple_order_cycle) }
     let(:form_applicator_mock) { instance_double(OpenFoodNetwork::OrderCycleFormApplicator) }
     let(:form) { OrderCycleForm.new(order_cycle, params, user) }
-    let(:params) { { order_cycle: { name: 'Some new name' } } }
+    let(:params) { { name: 'Some new name' } }
 
     before do
       allow(OpenFoodNetwork::OrderCycleFormApplicator).to receive(:new) { form_applicator_mock }
@@ -120,7 +122,7 @@ describe OrderCycleForm do
 
     context "when exchange params are provided" do
       let(:exchange_params) { { incoming_exchanges: [], outgoing_exchanges: [] } }
-      before { params[:order_cycle].merge!(exchange_params) }
+      before { params.merge!(exchange_params) }
 
       it "runs the OrderCycleFormApplicator, and saves other changes" do
         expect(form.save).to be true

spec/services/permitted_attributes/order_cycle_spec.rb

@@ -0,0 +1,42 @@
+require 'spec_helper'
+
+module PermittedAttributes
+  describe OrderCycle do
+    let(:oc_permitted_attributes) { PermittedAttributes::OrderCycle.new(params) }
+
+    describe "with basic attributes" do
+      let(:params) { ActionController::Parameters.new(order_cycle: { id: "2", name: "First Order Cycle" } ) }
+
+      it "keeps permitted and removes not permitted" do
+        permitted_attributes = oc_permitted_attributes.call
+
+        expect(permitted_attributes[:id]).to be nil
+        expect(permitted_attributes[:name]).to eq "First Order Cycle"
+      end
+    end
+
+    describe "nested incoming_exchanges attributes" do
+      let(:params) { ActionController::Parameters.new(order_cycle: { incoming_exchanges: [{ sender_id: "2", name: "Exchange Name", variants: [] }] } ) }
+
+      it "keeps permitted and removes not permitted" do
+        permitted_attributes = oc_permitted_attributes.call
+
+        exchange = permitted_attributes[:incoming_exchanges].first
+        expect(exchange[:name]).to be nil
+        expect(exchange[:sender_id]).to eq "2"
+      end
+    end
+
+    describe "variants inside incoming_exchanges attributes" do
+      let(:params) { ActionController::Parameters.new(order_cycle: { incoming_exchanges: [{ variants: { "7" => true, "12" => true } }] } ) }
+
+      it "keeps all variant_ids provided" do
+        permitted_attributes = oc_permitted_attributes.call
+
+        exchange_variants = permitted_attributes[:incoming_exchanges].first[:variants]
+        expect(exchange_variants["7"]).to be true
+        expect(exchange_variants["12"]).to be true
+      end
+    end    
+  end
+end