From 905811ccb31639d820997d333ac2428785737135 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 23 Feb 2020 17:32:00 +0000 Subject: [PATCH 1/6] Handle strong params in admin order_cycles controller --- app/controllers/admin/order_cycles_controller.rb | 11 +++++++++-- app/services/order_cycle_form.rb | 16 ++++++++-------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index 18521a6649..c2a38a5245 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,12 @@ module Admin def ams_prefix_whitelist [:basic, :index] end + + def order_cycle_params + params.require(:order_cycle).permit( + :incoming_exchanges, :outgoing_exchanges, + :name, :orders_open_at, :orders_close_at, :coordinator_id, :schedule_ids, :coordinator_fee_ids + ) + 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 From 1a46e7b7eef9be5ff4de0b69bba4877c5dd1292e Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sun, 23 Feb 2020 18:59:35 +0000 Subject: [PATCH 2/6] Improve strong params implementation on order_cycle controller and fix corresponding specs --- .../admin/order_cycles_controller.rb | 2 ++ .../admin/order_cycles_controller_spec.rb | 4 ++-- spec/services/order_cycle_form_spec.rb | 20 ++++++++++--------- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index c2a38a5245..23cf8257e2 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/app/controllers/admin/order_cycles_controller.rb @@ -237,6 +237,8 @@ module Admin end def order_cycle_params + return params[:order_cycle] if params[:order_cycle].empty? + params.require(:order_cycle).permit( :incoming_exchanges, :outgoing_exchanges, :name, :orders_open_at, :orders_close_at, :coordinator_id, :schedule_ids, :coordinator_fee_ids diff --git a/spec/controllers/admin/order_cycles_controller_spec.rb b/spec/controllers/admin/order_cycles_controller_spec.rb index 28a453fa99..74fb3303ea 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, hash_including(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, {}, 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 From 57f8fa26abedbce46409693e3ab75b5f21d32031 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 28 Feb 2020 13:00:50 +0000 Subject: [PATCH 3/6] Fix strong params in order_cycles --- .../admin/order_cycles_controller.rb | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index 23cf8257e2..e4048e418d 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/app/controllers/admin/order_cycles_controller.rb @@ -240,9 +240,37 @@ module Admin return params[:order_cycle] if params[:order_cycle].empty? params.require(:order_cycle).permit( - :incoming_exchanges, :outgoing_exchanges, - :name, :orders_open_at, :orders_close_at, :coordinator_id, :schedule_ids, :coordinator_fee_ids + :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 + + 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 From 20c7a0d3ef03d2d8d16b8bb8d90e00b6185caf8d Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 4 Mar 2020 10:03:04 +0000 Subject: [PATCH 4/6] Adapt to latest changes in order cycles controller strong params changes --- spec/controllers/admin/order_cycles_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/admin/order_cycles_controller_spec.rb b/spec/controllers/admin/order_cycles_controller_spec.rb index 74fb3303ea..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(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, {}, 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 } From fd2cf7295e74d6229bd3d349060539067baa77f3 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 21 Mar 2020 16:15:48 +0000 Subject: [PATCH 5/6] Extract permitted_attributes from order_cycle_controller into a specific service --- .../admin/order_cycles_controller.rb | 35 +------------- .../permitted_attributes/order_cycle.rb | 46 +++++++++++++++++++ .../permitted_attributes/order_cycle_spec.rb | 42 +++++++++++++++++ 3 files changed, 89 insertions(+), 34 deletions(-) create mode 100644 app/services/permitted_attributes/order_cycle.rb create mode 100644 spec/services/permitted_attributes/order_cycle_spec.rb diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index e4048e418d..19c2240d25 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/app/controllers/admin/order_cycles_controller.rb @@ -237,40 +237,7 @@ module Admin end def order_cycle_params - 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 - - 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 + PermittedAttributes::OrderCycle.new(params).call end end end diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb new file mode 100644 index 0000000000..e76cbe85be --- /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/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 From 980fdd65a132f6d977c80b930adfe57a50da3064 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 21 Mar 2020 17:00:08 +0000 Subject: [PATCH 6/6] Replace hash rockets syntax --- app/services/permitted_attributes/order_cycle.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/permitted_attributes/order_cycle.rb b/app/services/permitted_attributes/order_cycle.rb index e76cbe85be..df15b47d58 100644 --- a/app/services/permitted_attributes/order_cycle.rb +++ b/app/services/permitted_attributes/order_cycle.rb @@ -9,9 +9,9 @@ module PermittedAttributes @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 => [] + incoming_exchanges: permitted_exchange_attributes, + outgoing_exchanges: permitted_exchange_attributes, + schedule_ids: [], coordinator_fee_ids: [] ) end