diff --git a/app/controllers/admin/order_cycles_controller.rb b/app/controllers/admin/order_cycles_controller.rb index 3ddaaa485f..ff19519d37 100644 --- a/app/controllers/admin/order_cycles_controller.rb +++ b/app/controllers/admin/order_cycles_controller.rb @@ -1,6 +1,4 @@ -require 'open_food_network/permissions' require 'open_food_network/order_cycle_form_applicator' -require 'open_food_network/proxy_order_syncer' module Admin class OrderCyclesController < ResourceController @@ -9,11 +7,8 @@ module Admin prepend_before_filter :load_data_for_index, :only => :index before_filter :require_coordinator, only: :new before_filter :remove_protected_attrs, only: [:update] - before_filter :check_editable_schedule_ids, only: [:create, :update] before_filter :require_order_cycle_set_params, only: [:bulk_update] around_filter :protect_invalid_destroy, only: :destroy - create.after :sync_subscriptions - update.after :sync_subscriptions def index respond_to do |format| @@ -43,11 +38,10 @@ module Admin end def create - @order_cycle_form = OrderCycleForm.new(@order_cycle, params) + @order_cycle_form = OrderCycleForm.new(@order_cycle, params, spree_current_user) if @order_cycle_form.save OpenFoodNetwork::OrderCycleFormApplicator.new(@order_cycle, spree_current_user).go! - invoke_callbacks(:create, :after) flash[:notice] = I18n.t(:order_cycles_create_notice) render json: { success: true } else @@ -56,14 +50,13 @@ module Admin end def update - @order_cycle_form = OrderCycleForm.new(@order_cycle, params) + @order_cycle_form = OrderCycleForm.new(@order_cycle, params, spree_current_user) if @order_cycle_form.save unless params[:order_cycle][:incoming_exchanges].nil? && params[:order_cycle][:outgoing_exchanges].nil? # Only update apply exchange information if it is actually submmitted OpenFoodNetwork::OrderCycleFormApplicator.new(@order_cycle, spree_current_user).go! end - invoke_callbacks(:update, :after) flash[:notice] = I18n.t(:order_cycles_update_notice) if params[:reloading] == '1' render json: { :success => true } else @@ -182,29 +175,6 @@ module Admin end end - def check_editable_schedule_ids - return unless params[:order_cycle][:schedule_ids] - requested = params[:order_cycle][:schedule_ids].map(&:to_i) - @existing_schedule_ids = @order_cycle.persisted? ? @order_cycle.schedule_ids : [] - permitted = Schedule.where(id: requested | @existing_schedule_ids).merge(OpenFoodNetwork::Permissions.new(spree_current_user).editable_schedules).pluck(:id) - result = @existing_schedule_ids - result |= (requested & permitted) # add any requested & permitted ids - result -= ((result & permitted) - requested) # remove any existing and permitted ids that were not specifically requested - params[:order_cycle][:schedule_ids] = result - end - - def sync_subscriptions - return unless params[:order_cycle][:schedule_ids] - removed_ids = @existing_schedule_ids - @order_cycle.schedule_ids - new_ids = @order_cycle.schedule_ids - @existing_schedule_ids - if removed_ids.any? || new_ids.any? - schedules = Schedule.where(id: removed_ids + new_ids) - subscriptions = Subscription.where(schedule_id: schedules) - syncer = OpenFoodNetwork::ProxyOrderSyncer.new(subscriptions) - syncer.sync! - end - end - def order_cycles_from_set remove_unauthorized_bulk_attrs OrderCycle.where(id: params[:order_cycle_set][:collection_attributes].map{ |k,v| v[:id] }) diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb index c60c21fc02..ca983280b5 100644 --- a/app/services/order_cycle_form.rb +++ b/app/services/order_cycle_form.rb @@ -1,13 +1,50 @@ -class OrderCycleForm - attr_accessor :order_cycle, :params +require 'open_food_network/permissions' +require 'open_food_network/proxy_order_syncer' - def initialize(order_cycle, params) +class OrderCycleForm + def initialize(order_cycle, params, user) @order_cycle = order_cycle @params = params + @permissions = OpenFoodNetwork::Permissions.new(user) end def save + check_editable_schedule_ids order_cycle.assign_attributes(params[:order_cycle]) - order_cycle.save + return false unless order_cycle.valid? + order_cycle.transaction do + order_cycle.save! + sync_subscriptions + true + end + rescue ActiveRecord::RecordInvalid + false + end + + private + + attr_accessor :order_cycle, :params, :permissions + + def check_editable_schedule_ids + return unless params[:order_cycle][:schedule_ids] + requested = params[:order_cycle][:schedule_ids].map(&:to_i) + @existing_schedule_ids = @order_cycle.persisted? ? @order_cycle.schedule_ids : [] + permitted = Schedule.where(id: requested | @existing_schedule_ids).merge(permissions.editable_schedules).pluck(:id) + result = @existing_schedule_ids + result |= (requested & permitted) # add any requested & permitted ids + result -= ((result & permitted) - requested) # remove any existing and permitted ids that were not specifically requested + params[:order_cycle][:schedule_ids] = result + end + + def sync_subscriptions + return unless params[:order_cycle][:schedule_ids] + removed_ids = @existing_schedule_ids - @order_cycle.schedule_ids + new_ids = @order_cycle.schedule_ids - @existing_schedule_ids + if removed_ids.any? || new_ids.any? + schedules = Schedule.where(id: removed_ids + new_ids) + subscriptions = Subscription.where(schedule_id: schedules) + syncer = OpenFoodNetwork::ProxyOrderSyncer.new(subscriptions) + syncer.sync! + end end end diff --git a/spec/controllers/admin/order_cycles_controller_spec.rb b/spec/controllers/admin/order_cycles_controller_spec.rb index 4c845239dc..ec2147587f 100644 --- a/spec/controllers/admin/order_cycles_controller_spec.rb +++ b/spec/controllers/admin/order_cycles_controller_spec.rb @@ -211,46 +211,6 @@ module Admin end end - describe "updating schedules" do - let(:user) { create(:user, enterprise_limit: 10) } - let!(:managed_coordinator) { create(:enterprise, owner: user) } - let!(:managed_enterprise) { create(:enterprise, owner: user) } - let!(:coordinated_order_cycle) { create(:simple_order_cycle, coordinator: managed_coordinator ) } - let!(:coordinated_order_cycle2) { create(:simple_order_cycle, coordinator: managed_enterprise ) } - let!(:uncoordinated_order_cycle) { create(:simple_order_cycle, coordinator: create(:enterprise) ) } - let!(:coordinated_schedule) { create(:schedule, order_cycles: [coordinated_order_cycle] ) } - let!(:coordinated_schedule2) { create(:schedule, order_cycles: [coordinated_order_cycle2] ) } - let!(:uncoordinated_schedule) { create(:schedule, order_cycles: [uncoordinated_order_cycle] ) } - - context "where I manage the order_cycle's coordinator" do - render_views - - before do - controller.stub spree_current_user: user - end - - it "allows me to assign only schedules that already I coordinate to the order cycle" do - schedule_ids = [coordinated_schedule2.id, uncoordinated_schedule.id] - spree_put :update, format: :json, id: coordinated_order_cycle.id, order_cycle: { schedule_ids: schedule_ids } - expect(assigns(:order_cycle)).to eq coordinated_order_cycle - # coordinated_order_cycle2 is added - expect(coordinated_order_cycle.reload.schedules).to include coordinated_schedule2 - # coordinated_order_cycle is removed, uncoordinated_order_cycle is NOT added - expect(coordinated_order_cycle.reload.schedules).to_not include coordinated_schedule, uncoordinated_schedule - end - - it "syncs proxy orders when schedule_ids change" do - syncer_mock = double(:syncer) - allow(OpenFoodNetwork::ProxyOrderSyncer).to receive(:new) { syncer_mock } - expect(syncer_mock).to receive(:sync!).exactly(2).times - - spree_put :update, format: :json, id: coordinated_order_cycle.id, order_cycle: { schedule_ids: [coordinated_schedule.id, coordinated_schedule2.id] } - spree_put :update, format: :json, id: coordinated_order_cycle.id, order_cycle: { schedule_ids: [coordinated_schedule.id] } - spree_put :update, format: :json, id: coordinated_order_cycle.id, order_cycle: { schedule_ids: [coordinated_schedule.id] } - end - end - end - describe "bulk_update" do let(:oc) { create(:simple_order_cycle) } let!(:coordinator) { oc.coordinator } diff --git a/spec/services/order_cycle_form_spec.rb b/spec/services/order_cycle_form_spec.rb index ae6014ecfb..4cd312f63a 100644 --- a/spec/services/order_cycle_form_spec.rb +++ b/spec/services/order_cycle_form_spec.rb @@ -3,7 +3,7 @@ describe OrderCycleForm do describe "creating a new order cycle from params" do let(:shop) { create(:enterprise) } let(:order_cycle) { OrderCycle.new } - let(:form) { OrderCycleForm.new(order_cycle, params) } + 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 } } } @@ -29,7 +29,7 @@ describe OrderCycleForm do describe "updating an existing order cycle from params" do let(:shop) { create(:enterprise) } let(:order_cycle) { create(:simple_order_cycle, name: "Old Name") } - let(:form) { OrderCycleForm.new(order_cycle, params) } + 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 } } } @@ -52,4 +52,57 @@ describe OrderCycleForm do end end end + + describe "updating schedules" do + let(:user) { create(:user, enterprise_limit: 10) } + let!(:managed_coordinator) { create(:enterprise, owner: user) } + let!(:managed_enterprise) { create(:enterprise, owner: user) } + let!(:coordinated_order_cycle) { create(:simple_order_cycle, coordinator: managed_coordinator ) } + let!(:coordinated_order_cycle2) { create(:simple_order_cycle, coordinator: managed_enterprise ) } + let!(:uncoordinated_order_cycle) { create(:simple_order_cycle, coordinator: create(:enterprise) ) } + let!(:coordinated_schedule) { create(:schedule, order_cycles: [coordinated_order_cycle] ) } + let!(:coordinated_schedule2) { create(:schedule, order_cycles: [coordinated_order_cycle2] ) } + let!(:uncoordinated_schedule) { create(:schedule, order_cycles: [uncoordinated_order_cycle] ) } + + context "where I manage the order_cycle's coordinator" do + let(:form) { OrderCycleForm.new(coordinated_order_cycle, params, user) } + let(:syncer_mock) { instance_double(OpenFoodNetwork::ProxyOrderSyncer, sync!: true) } + + before do + allow(OpenFoodNetwork::ProxyOrderSyncer).to receive(:new) { syncer_mock } + 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] } } } + + it "associates the order cycle to the schedule" do + expect(form.save).to be true + expect(coordinated_order_cycle.reload.schedules).to include coordinated_schedule2 + expect(coordinated_order_cycle.reload.schedules).to_not include coordinated_schedule + expect(syncer_mock).to have_received(:sync!) + end + 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] } } } + + it "ignores the schedule that I don't own" do + expect(form.save).to be true + expect(coordinated_order_cycle.reload.schedules).to include coordinated_schedule + expect(coordinated_order_cycle.reload.schedules).to_not include uncoordinated_schedule + expect(syncer_mock).to_not have_received(:sync!) + end + end + + context "when I make no changes to the schedule ids" do + let(:params) { { order_cycle: { schedule_ids: [coordinated_schedule.id] } } } + + it "ignores the schedule that I don't own" do + expect(form.save).to be true + expect(coordinated_order_cycle.reload.schedules).to include coordinated_schedule + expect(syncer_mock).to_not have_received(:sync!) + end + end + end + end end