diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index 18521a6649..19c2240d25 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/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 diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index 2ab5d109de..fa397aed44 100644 --- a/app/services/order_cycle_form.rb +++ b/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 diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb new file mode 100644 index 0000000000..df15b47d58 --- /dev/null +++ b/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 diff --git a/spec/controllers/admin/order_cycles_controller_spec.rb b/spec/controllers/admin/order_cycles_controller_spec.rb index 28a453fa99..2ba98c9f8e 100644 --- a/spec/controllers/admin/order_cycles_controller_spec.rb +++ b/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 } diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index 476adb3e39..f264592763 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/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 diff --git a/spec/services/permitted_attributes/order_cycle_spec.rb b/spec/services/permitted_attributes/order_cycle_spec.rb new file mode 100644 index 0000000000..f91f358aab --- /dev/null +++ b/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