Merge pull request #5487 from coopdevs/fix-schedule-factory

[Spree 2.1] Allow to create order_cycle_schedules

app/controllers/admin/schedules_controller.rb

@@ -3,11 +3,11 @@ require 'order_management/subscriptions/proxy_order_syncer'
 
 module Admin
   class SchedulesController < ResourceController
-    before_filter :adapt_params, only: [:create, :update]
-    before_filter :check_editable_order_cycle_ids, only: [:create, :update]
+    before_filter :adapt_params, only: [:update]
+    before_filter :editable_order_cycle_ids_for_create, only: [:create]
+    before_filter :editable_order_cycle_ids_for_update, only: [:update]
     before_filter :check_dependent_subscriptions, only: [:destroy]
-    create.after :sync_subscriptions
-    update.after :sync_subscriptions
+    update.after :sync_subscriptions_for_update
 
     respond_to :json
 
@@ -28,6 +28,26 @@ module Admin
       end
     end
 
+    def create
+      if params[:order_cycle_ids].blank?
+        return respond_with(@schedule)
+      end
+
+      @schedule.attributes = permitted_resource_params
+
+      if @schedule.save
+        @schedule.order_cycle_ids = params[:order_cycle_ids]
+        @schedule.save!
+
+        sync_subscriptions_for_create
+
+        flash[:success] = flash_message_for(@schedule, :successfully_created)
+        respond_with(@schedule)
+      else
+        respond_with(@schedule)
+      end
+    end
+
     private
 
     def collection
@@ -57,17 +77,34 @@ module Admin
       params[:schedule][:order_cycle_ids] = params[:order_cycle_ids]
     end
 
-    def check_editable_order_cycle_ids
+    def editable_order_cycle_ids_for_create
+      return unless params[:order_cycle_ids]
+
+      @existing_order_cycle_ids = []
+      result = editable_order_cycles(params[:order_cycle_ids])
+
+      params[:order_cycle_ids] = result
+    end
+
+    def editable_order_cycle_ids_for_update
       return unless params[:schedule][:order_cycle_ids]
 
-      requested = params[:schedule][:order_cycle_ids]
-      @existing_order_cycle_ids = @schedule.persisted? ? @schedule.order_cycle_ids : []
-      permitted = OrderCycle.where(id: params[:schedule][:order_cycle_ids] | @existing_order_cycle_ids).merge(OrderCycle.managed_by(spree_current_user)).pluck(:id)
+      @existing_order_cycle_ids = @schedule.order_cycle_ids
+      result = editable_order_cycles(params[:schedule][:order_cycle_ids])
+
+      params[:schedule][:order_cycle_ids] = result
+      @schedule.order_cycle_ids = result
+    end
+
+    def editable_order_cycles(requested)
+      permitted = OrderCycle
+        .where(id: params[:order_cycle_ids] | @existing_order_cycle_ids)
+        .merge(OrderCycle.managed_by(spree_current_user))
+        .pluck(:id)
       result = @existing_order_cycle_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[:schedule][:order_cycle_ids] = result
-      @object.order_cycle_ids = result
+      result
     end
 
     def check_dependent_subscriptions
@@ -82,9 +119,19 @@ module Admin
       @permissions = OpenFoodNetwork::Permissions.new(spree_current_user)
     end
 
-    def sync_subscriptions
+    def sync_subscriptions_for_update
       return unless params[:schedule][:order_cycle_ids]
 
+      sync_subscriptions
+    end
+
+    def sync_subscriptions_for_create
+      return unless params[:order_cycle_ids]
+
+      sync_subscriptions
+    end
+
+    def sync_subscriptions
       removed_ids = @existing_order_cycle_ids - @schedule.order_cycle_ids
       new_ids = @schedule.order_cycle_ids - @existing_order_cycle_ids
       return unless removed_ids.any? || new_ids.any?
@@ -95,11 +142,7 @@ module Admin
     end
 
     def permitted_resource_params
-      params.require(:schedule).permit(
-        :id,
-        :name,
-        order_cycle_ids: []
-      )
+      params.require(:schedule).permit(:id, :name)
     end
   end
 end

app/models/schedule.rb

@@ -3,9 +3,8 @@ class Schedule < ActiveRecord::Base
 
   has_many :order_cycles, through: :order_cycle_schedules
   has_many :order_cycle_schedules, dependent: :destroy
-  has_many :coordinators, -> { uniq }, through: :order_cycles
 
-  validates :order_cycles, presence: true
+  has_many :coordinators, -> { uniq }, through: :order_cycles
 
   scope :with_coordinator, lambda { |enterprise| joins(:order_cycles).where('coordinator_id = ?', enterprise.id).select('DISTINCT schedules.*') }
 

app/services/order_cycle_form.rb

@@ -8,14 +8,17 @@ class OrderCycleForm
     @order_cycle_params = order_cycle_params
     @user = user
     @permissions = OpenFoodNetwork::Permissions.new(user)
+    @schedule_ids = order_cycle_params.delete(:schedule_ids)
   end
 
   def save
-    build_schedule_ids
+    schedule_ids = build_schedule_ids
     order_cycle.assign_attributes(order_cycle_params)
     return false unless order_cycle.valid?
 
     order_cycle.transaction do
+      order_cycle.save!
+      order_cycle.schedule_ids = schedule_ids
       order_cycle.save!
       apply_exchange_changes
       sync_subscriptions
@@ -42,7 +45,7 @@ class OrderCycleForm
   end
 
   def schedule_ids?
-    order_cycle_params[:schedule_ids].present?
+    @schedule_ids.present?
   end
 
   def build_schedule_ids
@@ -51,7 +54,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
-    order_cycle_params[:schedule_ids] = result
+    result
   end
 
   def sync_subscriptions
@@ -70,7 +73,7 @@ class OrderCycleForm
   end
 
   def requested_schedule_ids
-    order_cycle_params[:schedule_ids].map(&:to_i)
+    @schedule_ids.map(&:to_i)
   end
 
   def permitted_schedule_ids

engines/order_management/spec/services/order_management/subscriptions/count_spec.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'spec_helper'
+
 module OrderManagement
   module Subscriptions
     describe Count do

engines/order_management/spec/services/order_management/subscriptions/estimator_spec.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'spec_helper'
+
 module OrderManagement
   module Subscriptions
     describe Estimator do

engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'spec_helper'
+
 module OrderManagement
   module Subscriptions
     describe ProxyOrderSyncer do
@@ -16,59 +18,51 @@ module OrderManagement
 
       describe "#sync!" do
         let(:now) { Time.zone.now }
-        let(:schedule) { create(:schedule) }
-        let(:closed_oc) { # Closed
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now - 1.minute,
-                                      orders_close_at: now)
+        let(:schedule) { create(:schedule, order_cycles: [oc]) }
+
+        let(:closed_oc) {
+          create(:simple_order_cycle, orders_open_at: now - 1.minute, orders_close_at: now)
         }
         let(:open_oc_closes_before_begins_at_oc) { # Open, but closes before begins at
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now - 1.minute,
-                                      orders_close_at: now + 59.seconds)
+          create(:simple_order_cycle, orders_open_at: now - 1.minute, orders_close_at: now + 59.seconds)
         }
         let(:open_oc) { # Open & closes between begins at and ends at
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now - 1.minute,
-                                      orders_close_at: now + 90.seconds)
+          create(:simple_order_cycle, orders_open_at: now - 1.minute, orders_close_at: now + 90.seconds)
         }
         let(:upcoming_closes_before_begins_at_oc) { # Upcoming, but closes before begins at
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now + 30.seconds,
-                                      orders_close_at: now + 59.seconds)
+          create(:simple_order_cycle, orders_open_at: now + 30.seconds, orders_close_at: now + 59.seconds)
         }
         let(:upcoming_closes_on_begins_at_oc) { # Upcoming & closes on begins at
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now + 30.seconds,
-                                      orders_close_at: now + 1.minute)
+          create(:simple_order_cycle, orders_open_at: now + 30.seconds, orders_close_at: now + 1.minute)
         }
         let(:upcoming_closes_on_ends_at_oc) { # Upcoming & closes on ends at
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now + 30.seconds,
-                                      orders_close_at: now + 2.minutes)
+          create(:simple_order_cycle, orders_open_at: now + 30.seconds, orders_close_at: now + 2.minutes)
         }
         let(:upcoming_closes_after_ends_at_oc) { # Upcoming & closes after ends at
-          create(:simple_order_cycle, schedules: [schedule],
-                                      orders_open_at: now + 30.seconds,
-                                      orders_close_at: now + 121.seconds)
+          create(:simple_order_cycle, orders_open_at: now + 30.seconds, orders_close_at: now + 121.seconds)
         }
+
         let(:subscription) {
-          build(:subscription, schedule: schedule,
-                               begins_at: now + 1.minute,
-                               ends_at: now + 2.minutes)
+          build(:subscription, begins_at: now + 1.minute, ends_at: now + 2.minutes)
         }
         let(:proxy_orders) { subscription.reload.proxy_orders }
         let(:order_cycles) { proxy_orders.map(&:order_cycle) }
         let(:syncer) { ProxyOrderSyncer.new(subscription) }
 
         context "when the subscription is not persisted" do
-          before do
-            oc # Ensure oc is created before we attempt to sync
-            expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(0)
+          let(:schedule) { create(:schedule, order_cycles: [oc]) }
+          let(:subscription) do
+            build(
+              :subscription,
+              begins_at: now + 1.minute,
+              ends_at: now + 2.minutes,
+              schedule: schedule
+            )
           end
 
           context "and the schedule includes a closed oc (ie. closed before opens_at)" do
-            let(:oc) { closed_oc }
+            let!(:oc) { closed_oc }
+
             it "does not create a new proxy order for that oc" do
               expect{ subscription.save! }.to_not change(ProxyOrder, :count).from(0)
               expect(order_cycles).to_not include oc
@@ -76,7 +70,8 @@ module OrderManagement
           end
 
           context "and the schedule includes an open oc that closes before begins_at" do
-            let(:oc) { open_oc_closes_before_begins_at_oc }
+            let!(:oc) { open_oc_closes_before_begins_at_oc }
+
             it "does not create a new proxy order for that oc" do
               expect{ subscription.save! }.to_not change(ProxyOrder, :count).from(0)
               expect(order_cycles).to_not include oc
@@ -84,15 +79,19 @@ module OrderManagement
           end
 
           context "and the schedule has an open OC that closes between begins_at and ends_at" do
-            let(:oc) { open_oc }
+            let!(:oc) { open_oc }
+
             it "creates a new proxy order for that oc" do
+              syncer.sync!
+
               expect{ subscription.save! }.to change(ProxyOrder, :count).from(0).to(1)
-              expect(order_cycles).to include oc
+              expect(subscription.reload.proxy_orders.map(&:order_cycle)).to include oc
             end
           end
 
           context "and the schedule includes upcoming oc that closes before begins_at" do
-            let(:oc) { upcoming_closes_before_begins_at_oc }
+            let!(:oc) { upcoming_closes_before_begins_at_oc }
+
             it "does not create a new proxy order for that oc" do
               expect{ subscription.save! }.to_not change(ProxyOrder, :count).from(0)
               expect(order_cycles).to_not include oc
@@ -100,23 +99,30 @@ module OrderManagement
           end
 
           context "and the schedule includes upcoming oc that closes on begins_at" do
-            let(:oc) { upcoming_closes_on_begins_at_oc }
+            let!(:oc) { upcoming_closes_on_begins_at_oc }
+
             it "creates a new proxy order for that oc" do
+              syncer.sync!
+
               expect{ subscription.save! }.to change(ProxyOrder, :count).from(0).to(1)
-              expect(order_cycles).to include oc
+              expect(subscription.reload.proxy_orders.map(&:order_cycle)).to include oc
             end
           end
 
           context "and the schedule includes upcoming oc that closes after ends_at" do
-            let(:oc) { upcoming_closes_on_ends_at_oc }
+            let!(:oc) { upcoming_closes_on_ends_at_oc }
+
             it "creates a new proxy order for that oc" do
+              syncer.sync!
+
               expect{ subscription.save! }.to change(ProxyOrder, :count).from(0).to(1)
-              expect(order_cycles).to include oc
+              expect(subscription.reload.proxy_orders.map(&:order_cycle)).to include oc
             end
           end
 
           context "and the schedule includes upcoming oc that closes after ends_at" do
-            let(:oc) { upcoming_closes_after_ends_at_oc }
+            let!(:oc) { upcoming_closes_after_ends_at_oc }
+
             it "does not create a new proxy order for that oc" do
               expect{ subscription.save! }.to_not change(ProxyOrder, :count).from(0)
               expect(order_cycles).to_not include oc
@@ -194,6 +200,7 @@ module OrderManagement
               context "and the proxy order has not already been placed" do
                 context "the oc is closed (ie. closed before opens_at)" do
                   let(:oc) { closed_oc }
+
                   it "removes the proxy order" do
                     expect{ syncer.sync! }.to change(ProxyOrder, :count).from(1).to(0)
                     expect(proxy_orders).to_not include proxy_order
@@ -202,46 +209,65 @@ module OrderManagement
 
                 context "and the schedule includes an open oc that closes before begins_at" do
                   let(:oc) { open_oc_closes_before_begins_at_oc }
+
                   it "removes the proxy order" do
                     expect{ syncer.sync! }.to change(ProxyOrder, :count).from(1).to(0)
                     expect(proxy_orders).to_not include proxy_order
                   end
                 end
 
-                context "and the oc is open and closes between begins_at and ends_at" do
-                  let(:oc) { open_oc }
-                  it "keeps the proxy order" do
-                    expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
-                    expect(proxy_orders).to include proxy_order
+                context 'and the proxy orders are already synced' do
+                  let(:subscription) do
+                    build(
+                      :subscription,
+                      begins_at: now + 1.minute,
+                      ends_at: now + 2.minutes,
+                      schedule: schedule
+                    )
+                  end
+
+                  before { syncer.sync! }
+
+                  context "and the oc is open and closes between begins_at and ends_at" do
+                    let(:oc) { open_oc }
+
+                    it "keeps the proxy order" do
+                      expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
+                      expect(proxy_orders).to include proxy_order
+                    end
+                  end
+
+                  context "and the oc is upcoming and closes on begins_at" do
+                    let(:oc) { upcoming_closes_on_begins_at_oc }
+
+                    it "keeps the proxy order" do
+                      expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
+                      expect(proxy_orders).to include proxy_order
+                    end
+                  end
+
+                  context "and the oc is upcoming and closes on ends_at" do
+                    let(:oc) { upcoming_closes_on_ends_at_oc }
+
+                    it "keeps the proxy order" do
+                      expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
+                      expect(proxy_orders).to include proxy_order
+                    end
                   end
                 end
 
                 context "and the oc is upcoming and closes before begins_at" do
                   let(:oc) { upcoming_closes_before_begins_at_oc }
+
                   it "removes the proxy order" do
                     expect{ syncer.sync! }.to change(ProxyOrder, :count).from(1).to(0)
                     expect(proxy_orders).to_not include proxy_order
                   end
                 end
 
-                context "and the oc is upcoming and closes on begins_at" do
-                  let(:oc) { upcoming_closes_on_begins_at_oc }
-                  it "keeps the proxy order" do
-                    expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
-                    expect(proxy_orders).to include proxy_order
-                  end
-                end
-
-                context "and the oc is upcoming and closes on ends_at" do
-                  let(:oc) { upcoming_closes_on_ends_at_oc }
-                  it "keeps the proxy order" do
-                    expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
-                    expect(proxy_orders).to include proxy_order
-                  end
-                end
-
                 context "and the oc is upcoming and closes after ends_at" do
                   let(:oc) { upcoming_closes_after_ends_at_oc }
+
                   it "removes the proxy order" do
                     expect{ syncer.sync! }.to change(ProxyOrder, :count).from(1).to(0)
                     expect(proxy_orders).to_not include proxy_order
@@ -374,8 +400,20 @@ module OrderManagement
           end
 
           context "when a proxy order does not exist" do
+            let(:schedule) { create(:schedule, order_cycles: [oc]) }
+            let(:subscription) do
+              create(
+                :subscription,
+                begins_at: now + 1.minute,
+                ends_at: now + 2.minutes,
+                schedule: schedule
+              )
+            end
+            let(:syncer) { ProxyOrderSyncer.new(subscription) }
+
             context "and the schedule includes a closed oc (ie. closed before opens_at)" do
               let!(:oc) { closed_oc }
+
               it "does not create a new proxy order for that oc" do
                 expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(0)
                 expect(order_cycles).to_not include oc
@@ -384,6 +422,7 @@ module OrderManagement
 
             context "and the schedule includes an open oc that closes before begins_at" do
               let(:oc) { open_oc_closes_before_begins_at_oc }
+
               it "does not create a new proxy order for that oc" do
                 expect{ subscription.save! }.to_not change(ProxyOrder, :count).from(0)
                 expect(order_cycles).to_not include oc
@@ -392,14 +431,16 @@ module OrderManagement
 
             context "and the schedule has an open oc that closes between begins_at and ends_at" do
               let!(:oc) { open_oc }
+
               it "creates a new proxy order for that oc" do
                 expect{ syncer.sync! }.to change(ProxyOrder, :count).from(0).to(1)
-                expect(order_cycles).to include oc
+                expect(subscription.reload.proxy_orders.map(&:order_cycle)).to include oc
               end
             end
 
             context "and the schedule includes upcoming oc that closes before begins_at" do
               let!(:oc) { upcoming_closes_before_begins_at_oc }
+
               it "does not create a new proxy order for that oc" do
                 expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(0)
                 expect(order_cycles).to_not include oc
@@ -408,22 +449,25 @@ module OrderManagement
 
             context "and the schedule includes upcoming oc that closes on begins_at" do
               let!(:oc) { upcoming_closes_on_begins_at_oc }
+
               it "creates a new proxy order for that oc" do
                 expect{ syncer.sync! }.to change(ProxyOrder, :count).from(0).to(1)
-                expect(order_cycles).to include oc
+                expect(subscription.reload.proxy_orders.map(&:order_cycle)).to include oc
               end
             end
 
             context "and the schedule includes upcoming oc that closes on ends_at" do
               let!(:oc) { upcoming_closes_on_ends_at_oc }
+
               it "creates a new proxy order for that oc" do
                 expect{ syncer.sync! }.to change(ProxyOrder, :count).from(0).to(1)
-                expect(order_cycles).to include oc
+                expect(subscription.reload.proxy_orders.map(&:order_cycle)).to include oc
               end
             end
 
             context "and the schedule includes upcoming oc that closes after ends_at" do
               let!(:oc) { upcoming_closes_after_ends_at_oc }
+
               it "does not create a new proxy order for that oc" do
                 expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(0)
                 expect(order_cycles).to_not include oc

engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require 'spec_helper'
+
 module OrderManagement
   module Subscriptions
     describe Summary do

spec/factories.rb

@@ -28,7 +28,16 @@ FactoryBot.define do
 
   factory :schedule, class: Schedule do
     sequence(:name) { |n| "Schedule #{n}" }
-    order_cycles { [OrderCycle.first || FactoryBot.create(:simple_order_cycle)] }
+
+    transient do
+      order_cycles { [OrderCycle.first || create(:simple_order_cycle)] }
+    end
+
+    before(:create) do |schedule, evaluator|
+      evaluator.order_cycles.each do |order_cycle|
+        order_cycle.schedules << schedule
+      end
+    end
   end
 
   factory :proxy_order, class: ProxyOrder do

spec/features/admin/schedules_spec.rb

@@ -129,10 +129,10 @@ feature 'Schedules', js: true do
         end
 
         expect(Schedule.find_by(id: weekly_schedule.id)).to be_nil
-        expect(oc1.schedules).to eq []
-        expect(oc2.schedules).to eq []
-        expect(oc3.schedules).to eq []
-        expect(oc4.schedules).to eq []
+        expect(oc1.reload.schedules).to eq []
+        expect(oc2.reload.schedules).to eq []
+        expect(oc3.reload.schedules).to eq []
+        expect(oc4.reload.schedules).to eq []
       end
     end
   end