Move applicator calls to OrderCycleForm

app/controllers/admin/order_cycles_controller.rb

@@ -1,5 +1,3 @@
-require 'open_food_network/order_cycle_form_applicator'
-
 module Admin
   class OrderCyclesController < ResourceController
     include OrderCyclesHelper
@@ -41,7 +39,6 @@ module Admin
       @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!
         flash[:notice] = I18n.t(:order_cycles_create_notice)
         render json: { success: true }
       else
@@ -53,10 +50,6 @@ module Admin
       @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
         flash[:notice] = I18n.t(:order_cycles_update_notice) if params[:reloading] == '1'
         render json: { :success => true }
       else

app/services/order_cycle_form.rb

@@ -1,10 +1,12 @@
 require 'open_food_network/permissions'
 require 'open_food_network/proxy_order_syncer'
+require 'open_food_network/order_cycle_form_applicator'
 
 class OrderCycleForm
   def initialize(order_cycle, params, user)
     @order_cycle = order_cycle
     @params = params
+    @user = user
     @permissions = OpenFoodNetwork::Permissions.new(user)
   end
 
@@ -14,6 +16,7 @@ class OrderCycleForm
     return false unless order_cycle.valid?
     order_cycle.transaction do
       order_cycle.save!
+      apply_exchange_changes
       sync_subscriptions
       true
     end
@@ -23,7 +26,18 @@ class OrderCycleForm
 
   private
 
-  attr_accessor :order_cycle, :params, :permissions
+  attr_accessor :order_cycle, :params, :user, :permissions
+
+  def apply_exchange_changes
+    return if exchanges_unchanged?
+    OpenFoodNetwork::OrderCycleFormApplicator.new(order_cycle, user).go!
+  end
+
+  def exchanges_unchanged?
+    [:incoming_exchanges, :outgoing_exchanges].all? do |direction|
+      params[:order_cycle][direction].nil?
+    end
+  end
 
   def schedule_ids?
     params[:order_cycle][:schedule_ids].present?

spec/controllers/admin/order_cycles_controller_spec.rb

@@ -131,6 +131,51 @@ module Admin
     end
 
     describe "update" do
+      let(:order_cycle) { create(:simple_order_cycle) }
+      let(:coordinator) { order_cycle.coordinator }
+      let(:form_mock) { instance_double(OrderCycleForm) }
+
+      before do
+        allow(OrderCycleForm).to receive(:new) { form_mock }
+      end
+
+      context "as a manager of the coordinator" do
+        before { login_as_enterprise_user([coordinator]) }
+        let(:params) { { format: :json, id: order_cycle.id, order_cycle: {} } }
+
+        context "when updating succeeds" do
+          before { allow(form_mock).to receive(:save) { true } }
+
+          context "when the page is reloading" do
+            before { params[:reloading] = '1' }
+
+            it "sets flash message" do
+              spree_put :update, params
+              flash[:notice].should == 'Your order cycle has been updated.'
+            end
+          end
+
+          context "when the page is not reloading" do
+            it "does not set flash message" do
+              spree_put :update, params
+              flash[:notice].should be nil
+            end
+          end
+        end
+
+        context "when a validation error occurs" do
+          before { allow(form_mock).to receive(:save) { false } }
+
+          it "returns an error message" do
+            spree_put :update, params
+            json_response = JSON.parse(response.body)
+            expect(json_response['errors']).to be
+          end
+        end
+      end
+    end
+
+    describe "limiting update scope" do
       let(:order_cycle) { create(:simple_order_cycle) }
       let(:producer) { create(:supplier_enterprise) }
       let(:coordinator) { order_cycle.coordinator }
@@ -139,74 +184,30 @@ module Admin
       let!(:incoming_exchange) { create(:exchange, order_cycle: order_cycle, sender: producer, receiver: coordinator, incoming: true, variants: [v]) }
       let!(:outgoing_exchange) { create(:exchange, order_cycle: order_cycle, sender: coordinator, receiver: hub, incoming: false, variants: [v]) }
 
+      let(:allowed) { { incoming_exchanges: [], outgoing_exchanges: [] } }
+      let(:restricted) { { name: 'some name', orders_open_at: 1.day.from_now, orders_close_at: 1.day.ago } }
+      let(:params) { { format: :json, id: order_cycle.id, order_cycle: allowed.merge(restricted) } }
+      let(:form_mock) { instance_double(OrderCycleForm, save: true) }
+
+      before { allow(controller).to receive(:spree_current_user) { user } }
+
       context "as a manager of the coordinator" do
-        before { login_as_enterprise_user([coordinator]) }
+        let(:user) { coordinator.owner }
+        let(:expected) { [order_cycle, hash_including(order_cycle: allowed.merge(restricted)), user] }
 
-        it "sets flash message when page is reloading" do
-          spree_put :update, id: order_cycle.id, reloading: '1', order_cycle: {}
-          flash[:notice].should == 'Your order cycle has been updated.'
-        end
-
-        it "does not set flash message otherwise" do
-          flash[:notice].should be_nil
-        end
-
-        context "when updating without explicitly submitting exchanges" do
-          let(:form_applicator_mock) { double(:form_applicator) }
-
-          before do
-            allow(OpenFoodNetwork::OrderCycleFormApplicator).to receive(:new) { form_applicator_mock }
-            allow(form_applicator_mock).to receive(:go!) { nil }
-          end
-
-          it "does not run the OrderCycleFormApplicator" do
-            expect(order_cycle.exchanges.incoming).to eq [incoming_exchange]
-            expect(order_cycle.exchanges.outgoing).to eq [outgoing_exchange]
-            expect(order_cycle.prefers_product_selection_from_coordinator_inventory_only?).to be false
-            spree_put :update, id: order_cycle.id, order_cycle: { name: 'Some new name', preferred_product_selection_from_coordinator_inventory_only: true }
-            expect(form_applicator_mock).to_not have_received(:go!)
-            order_cycle.reload
-            expect(order_cycle.exchanges.incoming).to eq [incoming_exchange]
-            expect(order_cycle.exchanges.outgoing).to eq [outgoing_exchange]
-            expect(order_cycle.name).to eq 'Some new name'
-            expect(order_cycle.prefers_product_selection_from_coordinator_inventory_only?).to be true
-          end
-        end
-
-        context "when a validation error occurs" do
-          let(:params) {
-            {
-              format: :json,
-              id: order_cycle.id,
-              order_cycle: { orders_open_at: order_cycle.orders_close_at + 1.day }
-            }
-          }
-
-          it "returns an error message" do
-            spree_put :update, params
-            json_response = JSON.parse(response.body)
-            expect(json_response['errors']).to be_present
-          end
+        it "allows me to update exchange information for exchanges, name and dates" do
+          expect(OrderCycleForm).to receive(:new).with(*expected) { form_mock }
+          spree_put :update, params
         end
       end
 
       context "as a producer supplying to an order cycle" do
-        before do
-          login_as_enterprise_user [producer]
-        end
+        let(:user) { producer.owner }
+        let(:expected) { [order_cycle, hash_including(order_cycle: allowed), user] }
 
-        describe "removing a variant from incoming" do
-          let(:params) do
-            {order_cycle: {
-               incoming_exchanges: [{id: incoming_exchange.id, enterprise_id: producer.id, sender_id: producer.id, variants: {v.id => false}}],
-               outgoing_exchanges: [{id: outgoing_exchange.id, enterprise_id: hub.id,      receiver_id: hub.id,    variants: {v.id => false}}] }
-            }
-          end
-
-          it "removes the variant from outgoing also" do
-            spree_put :update, {id: order_cycle.id}.merge(params)
-            Exchange.where(order_cycle_id: order_cycle).with_variant(v).should be_empty
-          end
+        it "allows me to update exchange information for exchanges, but not name or dates" do
+          expect(OrderCycleForm).to receive(:new).with(*expected) { form_mock }
+          spree_put :update, params
         end
       end
     end

spec/services/order_cycle_form_spec.rb

@@ -105,4 +105,36 @@ describe OrderCycleForm do
       end
     end
   end
+
+  describe "updating exchanges" do
+    let(:user) { instance_double(Spree::User) }
+    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' } } }
+
+    before do
+      allow(OpenFoodNetwork::OrderCycleFormApplicator).to receive(:new) { form_applicator_mock }
+      allow(form_applicator_mock).to receive(:go!)
+    end
+
+    context "when exchange params are provided" do
+      let(:exchange_params) { { incoming_exchanges: [], outgoing_exchanges: [] } }
+      before { params[:order_cycle].merge!(exchange_params) }
+
+      it "runs the OrderCycleFormApplicator, and saves other changes" do
+        expect(form.save).to be true
+        expect(form_applicator_mock).to have_received(:go!)
+        expect(order_cycle.name).to eq 'Some new name'
+      end
+    end
+
+    context "when no exchange params are provided" do
+      it "does not run the OrderCycleFormApplicator, but saves other changes" do
+        expect(form.save).to be true
+        expect(form_applicator_mock).to_not have_received(:go!)
+        expect(order_cycle.name).to eq 'Some new name'
+      end
+    end
+  end
 end