From e339a37cd5bbf1fe4d1197832a29471ee12c7aed Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 22 May 2020 14:10:23 +0200
Subject: [PATCH 01/16] Allow to create order_cycle_schedules

There's no way we can create an order_cycle_schedules if the schedule
doesn't have an id, which we can't get if to persist it we need an OC
first, which in turn, will create an order_cycle_schedules.
---
 app/models/schedule.rb |  3 +--
 spec/factories.rb      | 11 ++++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/app/models/schedule.rb b/app/models/schedule.rb
index c28b9ae029..5fe5da3205 100644
--- a/app/models/schedule.rb
+++ b/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.*') }
 
diff --git a/spec/factories.rb b/spec/factories.rb
index 32caef0f4f..d13757cb2a 100644
--- a/spec/factories.rb
+++ b/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

From d9686d698238ae3473ed92310fab0d23fa181704 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 22 May 2020 15:54:30 +0200
Subject: [PATCH 02/16] Enable spec files to be executed alone

This fixes the annoying error

```
NameError:
  uninitialized constant OrderManagement::Subscriptions::Whatever
```

and let's you execute the spec file in isolation. It slows down way too
much having the run the entire engine test suite while developing. And
it makes me nervous too.
---
 .../spec/services/order_management/subscriptions/count_spec.rb  | 2 ++
 .../services/order_management/subscriptions/estimator_spec.rb   | 2 ++
 .../order_management/subscriptions/proxy_order_syncer_spec.rb   | 2 ++
 .../services/order_management/subscriptions/summary_spec.rb     | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/engines/order_management/spec/services/order_management/subscriptions/count_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/count_spec.rb
index 14b5a8c041..75b4e47111 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/count_spec.rb
+++ b/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
diff --git a/engines/order_management/spec/services/order_management/subscriptions/estimator_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/estimator_spec.rb
index e8fe7bf957..63032465d6 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/estimator_spec.rb
+++ b/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
diff --git a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
index 501f6fa67e..fc14e7ff4b 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
+++ b/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
diff --git a/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb
index 5e6360c542..454e2e5ba3 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb
+++ b/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

From a33396984f49fc28011e0cd1289eab230bd2f6cc Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 22 May 2020 17:56:01 +0200
Subject: [PATCH 03/16] Fix and DRY specs

---
 .../subscriptions/proxy_order_syncer_spec.rb  | 162 +++++++++++++-----
 1 file changed, 120 insertions(+), 42 deletions(-)

diff --git a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
index fc14e7ff4b..5a352f49ec 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
+++ b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
@@ -18,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
@@ -78,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
@@ -86,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
@@ -102,23 +99,50 @@ 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 }
+
+            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
+            let(:syncer) { ProxyOrderSyncer.new(subscription) }
+
             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
+              schedule = create(:schedule, order_cycles: [oc])
+              subscription = build(
+                :subscription,
+                begins_at: now + 1.minute,
+                ends_at: now + 2.minutes,
+                schedule: schedule
+              )
+
+              syncer = ProxyOrderSyncer.new(subscription)
+              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
@@ -212,7 +236,19 @@ module OrderManagement
 
                 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
+                    schedule = create(:schedule, order_cycles: [oc])
+                    subscription = build(
+                      :subscription,
+                      begins_at: now + 1.minute,
+                      ends_at: now + 2.minutes,
+                      schedule: schedule
+                    )
+
+                    syncer = ProxyOrderSyncer.new(subscription)
+                    syncer.sync!
+
                     expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
                     expect(proxy_orders).to include proxy_order
                   end
@@ -228,7 +264,19 @@ module OrderManagement
 
                 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
+                    schedule = create(:schedule, order_cycles: [oc])
+                    subscription = build(
+                      :subscription,
+                      begins_at: now + 1.minute,
+                      ends_at: now + 2.minutes,
+                      schedule: schedule
+                    )
+
+                    syncer = ProxyOrderSyncer.new(subscription)
+                    syncer.sync!
+
                     expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
                     expect(proxy_orders).to include proxy_order
                   end
@@ -236,7 +284,19 @@ module OrderManagement
 
                 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
+                    schedule = create(:schedule, order_cycles: [oc])
+                    subscription = build(
+                      :subscription,
+                      begins_at: now + 1.minute,
+                      ends_at: now + 2.minutes,
+                      schedule: schedule
+                    )
+
+                    syncer = ProxyOrderSyncer.new(subscription)
+                    syncer.sync!
+
                     expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
                     expect(proxy_orders).to include proxy_order
                   end
@@ -376,8 +436,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
@@ -386,6 +458,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
@@ -394,14 +467,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
@@ -410,22 +485,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

From 06b7a95fb167df7c077a8a8e398241be31db9308 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 28 May 2020 14:00:53 +0200
Subject: [PATCH 04/16] DRY duplicate subscription declaration

---
 .../subscriptions/proxy_order_syncer_spec.rb              | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
index 5a352f49ec..2413667b9c 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
+++ b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
@@ -102,14 +102,6 @@ module OrderManagement
             let!(:oc) { upcoming_closes_on_begins_at_oc }
 
             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
             let(:syncer) { ProxyOrderSyncer.new(subscription) }
 
             it "creates a new proxy order for that oc" do

From 9a29e634fc07eab9c09b27a2e79456272bef316e Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Thu, 28 May 2020 17:49:36 +0200
Subject: [PATCH 05/16] Deal with Schedule creation in its own controller

This way it can assign the order cycles to the schedule when this is
persisted. An OrderCycleSchedule (the join table) can't be created until
both schedule and order_cycle got an id.

Also, we do not call `#adapt_params` when creating the Schedule as that
assigns `order_cycle_ids` to `@object.attributes` thus, attempting to
create the OrderCycleSchedule without a schedule_id.
---
 app/controllers/admin/schedules_controller.rb | 27 ++++++++++++++-----
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 44ccb48d64..59528cf158 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -3,7 +3,7 @@ require 'order_management/subscriptions/proxy_order_syncer'
 
 module Admin
   class SchedulesController < ResourceController
-    before_filter :adapt_params, only: [:create, :update]
+    before_filter :adapt_params, only: [:update]
     before_filter :check_editable_order_cycle_ids, only: [:create, :update]
     before_filter :check_dependent_subscriptions, only: [:destroy]
     create.after :sync_subscriptions
@@ -28,6 +28,25 @@ module Admin
       end
     end
 
+    def create
+      invoke_callbacks(:create, :before)
+      @object.attributes = permitted_resource_params
+      @object.save!
+
+      @object.order_cycle_ids = params[:order_cycle_ids]
+      if @object.save
+        invoke_callbacks(:create, :after)
+        flash[:success] = flash_message_for(@object, :successfully_created)
+        respond_with(@object) do |format|
+          format.html { redirect_to location_after_save }
+          format.js   { render layout: false }
+        end
+      else
+        invoke_callbacks(:create, :fails)
+        respond_with(@object)
+      end
+    end
+
     private
 
     def collection
@@ -95,11 +114,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

From 4f635339e365229ab17cc63a40ac62d56d611cb7 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 09:39:12 +0200
Subject: [PATCH 06/16] Fix schedule creation and subs syncing

---
 app/controllers/admin/schedules_controller.rb | 43 +++++++++++++++++--
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 59528cf158..59fedaf839 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -4,9 +4,9 @@ require 'order_management/subscriptions/proxy_order_syncer'
 module Admin
   class SchedulesController < ResourceController
     before_filter :adapt_params, only: [:update]
-    before_filter :check_editable_order_cycle_ids, only: [:create, :update]
+    before_filter :check_editable_order_cycle_ids_create, only: [:create]
+    before_filter :check_editable_order_cycle_ids, only: [:update]
     before_filter :check_dependent_subscriptions, only: [:destroy]
-    create.after :sync_subscriptions
     update.after :sync_subscriptions
 
     respond_to :json
@@ -30,12 +30,19 @@ module Admin
 
     def create
       invoke_callbacks(:create, :before)
+
+      if params[:order_cycle_ids].blank?
+        invoke_callbacks(:create, :fails)
+        return respond_with(@object)
+      end
+
       @object.attributes = permitted_resource_params
       @object.save!
 
       @object.order_cycle_ids = params[:order_cycle_ids]
       if @object.save
-        invoke_callbacks(:create, :after)
+        sync_subscriptions_create
+
         flash[:success] = flash_message_for(@object, :successfully_created)
         respond_with(@object) do |format|
           format.html { redirect_to location_after_save }
@@ -76,8 +83,24 @@ module Admin
       params[:schedule][:order_cycle_ids] = params[:order_cycle_ids]
     end
 
+    def check_editable_order_cycle_ids_create
+      return unless params[:order_cycle_ids]
+
+      requested = params[:order_cycle_ids]
+
+      @existing_order_cycle_ids = @schedule.persisted? ? @schedule.order_cycle_ids : []
+
+      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[:order_cycle_ids] = result
+    end
+
     def check_editable_order_cycle_ids
-      return unless params[:schedule][:order_cycle_ids]
+      return unless params[:schedule][:order_cycle_ids] || params[:order_cycle_ids]
 
       requested = params[:schedule][:order_cycle_ids]
       @existing_order_cycle_ids = @schedule.persisted? ? @schedule.order_cycle_ids : []
@@ -113,6 +136,18 @@ module Admin
       syncer.sync!
     end
 
+    def sync_subscriptions_create
+      return unless params[:order_cycle_ids]
+
+      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?
+
+      subscriptions = Subscription.where(schedule_id: @schedule)
+      syncer = OrderManagement::Subscriptions::ProxyOrderSyncer.new(subscriptions)
+      syncer.sync!
+    end
+
     def permitted_resource_params
       params.require(:schedule).permit(:id, :name)
     end

From 862364ebbb6db86768ebf7f1f2d10eda83028258 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 09:39:23 +0200
Subject: [PATCH 07/16] Fix schedule destroy spec

---
 spec/features/admin/schedules_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/features/admin/schedules_spec.rb b/spec/features/admin/schedules_spec.rb
index 0a457e1d2a..f017596e53 100644
--- a/spec/features/admin/schedules_spec.rb
+++ b/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

From f25ee8c99841fde47aa3acf8014dedb4413175ff Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 10:16:03 +0200
Subject: [PATCH 08/16] Extract `#editable_order_cycles`

This makes it far easier to spot what's the difference between create
and update regarding the editable order cycles.

I stop here before a slightly deeper refactor which would make the PR
quite hard to review. This will come an upcoming one.
---
 app/controllers/admin/schedules_controller.rb | 33 +++++++++++--------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 59fedaf839..a16cc673a9 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -5,7 +5,7 @@ module Admin
   class SchedulesController < ResourceController
     before_filter :adapt_params, only: [:update]
     before_filter :check_editable_order_cycle_ids_create, only: [:create]
-    before_filter :check_editable_order_cycle_ids, only: [:update]
+    before_filter :check_editable_order_cycle_ids_update, only: [:update]
     before_filter :check_dependent_subscriptions, only: [:destroy]
     update.after :sync_subscriptions
 
@@ -87,29 +87,34 @@ module Admin
       return unless params[:order_cycle_ids]
 
       requested = params[:order_cycle_ids]
+      @existing_order_cycle_ids = []
 
-      @existing_order_cycle_ids = @schedule.persisted? ? @schedule.order_cycle_ids : []
-
-      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
+      result = editable_order_cycles(requested)
 
       params[:order_cycle_ids] = result
     end
 
-    def check_editable_order_cycle_ids
-      return unless params[:schedule][:order_cycle_ids] || params[:order_cycle_ids]
+    def check_editable_order_cycle_ids_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(requested)
+
+      params[:schedule][:order_cycle_ids] = result
+      @object.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

From 65c53df2ef0541ba4c90da9ec849273afec6616e Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 10:21:51 +0200
Subject: [PATCH 09/16] Remove unnecessary callback invocations

There are no registered callback methods to execute.
---
 app/controllers/admin/schedules_controller.rb | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index a16cc673a9..35df2c4190 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -29,10 +29,7 @@ module Admin
     end
 
     def create
-      invoke_callbacks(:create, :before)
-
       if params[:order_cycle_ids].blank?
-        invoke_callbacks(:create, :fails)
         return respond_with(@object)
       end
 
@@ -49,7 +46,6 @@ module Admin
           format.js   { render layout: false }
         end
       else
-        invoke_callbacks(:create, :fails)
         respond_with(@object)
       end
     end

From 56b590a6f8d8a331827de50f905048ac01cbeeec Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 10:41:17 +0200
Subject: [PATCH 10/16] Extract `#sync_subscription`

This makes it far easier to spot what's the difference between create
and update regarding subs syncing without having to mess with `#update`
and its callbacks (for now).
---
 app/controllers/admin/schedules_controller.rb | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 35df2c4190..4debb5f91f 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -7,7 +7,7 @@ module Admin
     before_filter :check_editable_order_cycle_ids_create, only: [:create]
     before_filter :check_editable_order_cycle_ids_update, only: [:update]
     before_filter :check_dependent_subscriptions, only: [:destroy]
-    update.after :sync_subscriptions
+    update.after :sync_subscriptions_update
 
     respond_to :json
 
@@ -125,21 +125,19 @@ module Admin
       @permissions = OpenFoodNetwork::Permissions.new(spree_current_user)
     end
 
-    def sync_subscriptions
+    def sync_subscriptions_update
       return unless params[:schedule][:order_cycle_ids]
 
-      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?
-
-      subscriptions = Subscription.where(schedule_id: @schedule)
-      syncer = OrderManagement::Subscriptions::ProxyOrderSyncer.new(subscriptions)
-      syncer.sync!
+      sync_subscriptions
     end
 
     def sync_subscriptions_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?

From 4f3d01bd93090231565baf7cd00ec38295c83dab Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 10:43:16 +0200
Subject: [PATCH 11/16] Remove unused HTML response format

---
 app/controllers/admin/schedules_controller.rb | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 4debb5f91f..e1980e86c5 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -41,10 +41,7 @@ module Admin
         sync_subscriptions_create
 
         flash[:success] = flash_message_for(@object, :successfully_created)
-        respond_with(@object) do |format|
-          format.html { redirect_to location_after_save }
-          format.js   { render layout: false }
-        end
+        respond_with(@object)
       else
         respond_with(@object)
       end

From 768568c75c839ef184eed290fb65c1975d69ca74 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 10:44:06 +0200
Subject: [PATCH 12/16] Rename @object to @schedule

This level of abstraction makes sense in the framework-like code in
ResourceController but here it just makes things more difficult.
---
 app/controllers/admin/schedules_controller.rb | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index e1980e86c5..7b356701c9 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -30,20 +30,20 @@ module Admin
 
     def create
       if params[:order_cycle_ids].blank?
-        return respond_with(@object)
+        return respond_with(@schedule)
       end
 
-      @object.attributes = permitted_resource_params
-      @object.save!
+      @schedule.attributes = permitted_resource_params
+      @schedule.save!
 
-      @object.order_cycle_ids = params[:order_cycle_ids]
-      if @object.save
+      @schedule.order_cycle_ids = params[:order_cycle_ids]
+      if @schedule.save
         sync_subscriptions_create
 
-        flash[:success] = flash_message_for(@object, :successfully_created)
-        respond_with(@object)
+        flash[:success] = flash_message_for(@schedule, :successfully_created)
+        respond_with(@schedule)
       else
-        respond_with(@object)
+        respond_with(@schedule)
       end
     end
 
@@ -96,7 +96,7 @@ module Admin
       result = editable_order_cycles(requested)
 
       params[:schedule][:order_cycle_ids] = result
-      @object.order_cycle_ids = result
+      @schedule.order_cycle_ids = result
     end
 
     def editable_order_cycles(requested)

From 032dce1a8876597ab58306a53f54f9b4a87e7324 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 10:46:29 +0200
Subject: [PATCH 13/16] Move OC save within success block

This makes more evident there are two scenarios: schedule save success
or failure. We deal with OCs failures raising an exception.
---
 app/controllers/admin/schedules_controller.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 7b356701c9..39e1ffc625 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -34,10 +34,11 @@ module Admin
       end
 
       @schedule.attributes = permitted_resource_params
-      @schedule.save!
 
-      @schedule.order_cycle_ids = params[:order_cycle_ids]
       if @schedule.save
+        @schedule.order_cycle_ids = params[:order_cycle_ids]
+        @schedule.save!
+
         sync_subscriptions_create
 
         flash[:success] = flash_message_for(@schedule, :successfully_created)

From 815cd73ff3e5833b0cd1afa4b1ad4916f7585eef Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 11:15:47 +0200
Subject: [PATCH 14/16] DRY specs

---
 .../subscriptions/proxy_order_syncer_spec.rb  | 96 +++++++------------
 1 file changed, 34 insertions(+), 62 deletions(-)

diff --git a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
index 2413667b9c..304eae01ef 100644
--- a/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
+++ b/engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb
@@ -101,9 +101,6 @@ module OrderManagement
           context "and the schedule includes upcoming oc that closes on begins_at" do
             let!(:oc) { upcoming_closes_on_begins_at_oc }
 
-            let(:schedule) { create(:schedule, order_cycles: [oc]) }
-            let(:syncer) { ProxyOrderSyncer.new(subscription) }
-
             it "creates a new proxy order for that oc" do
               syncer.sync!
 
@@ -116,15 +113,6 @@ module OrderManagement
             let!(:oc) { upcoming_closes_on_ends_at_oc }
 
             it "creates a new proxy order for that oc" do
-              schedule = create(:schedule, order_cycles: [oc])
-              subscription = build(
-                :subscription,
-                begins_at: now + 1.minute,
-                ends_at: now + 2.minutes,
-                schedule: schedule
-              )
-
-              syncer = ProxyOrderSyncer.new(subscription)
               syncer.sync!
 
               expect{ subscription.save! }.to change(ProxyOrder, :count).from(0).to(1)
@@ -212,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
@@ -220,82 +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
-                    schedule = create(:schedule, order_cycles: [oc])
-                    subscription = build(
+                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
 
-                    syncer = ProxyOrderSyncer.new(subscription)
-                    syncer.sync!
+                  before { syncer.sync! }
 
-                    expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(1)
-                    expect(proxy_orders).to include proxy_order
+                  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
-                    schedule = create(:schedule, order_cycles: [oc])
-                    subscription = build(
-                      :subscription,
-                      begins_at: now + 1.minute,
-                      ends_at: now + 2.minutes,
-                      schedule: schedule
-                    )
-
-                    syncer = ProxyOrderSyncer.new(subscription)
-                    syncer.sync!
-
-                    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
-                    schedule = create(:schedule, order_cycles: [oc])
-                    subscription = build(
-                      :subscription,
-                      begins_at: now + 1.minute,
-                      ends_at: now + 2.minutes,
-                      schedule: schedule
-                    )
-
-                    syncer = ProxyOrderSyncer.new(subscription)
-                    syncer.sync!
-
-                    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

From 538c53fe9ed8079e498dc37d1b7958fd5a3eb0ec Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 15:30:33 +0200
Subject: [PATCH 15/16] Improve code readability

Also removing an useless vars
---
 app/controllers/admin/schedules_controller.rb | 24 ++++++++-----------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb
index 39e1ffc625..d0728ebc18 100644
--- a/app/controllers/admin/schedules_controller.rb
+++ b/app/controllers/admin/schedules_controller.rb
@@ -4,10 +4,10 @@ require 'order_management/subscriptions/proxy_order_syncer'
 module Admin
   class SchedulesController < ResourceController
     before_filter :adapt_params, only: [:update]
-    before_filter :check_editable_order_cycle_ids_create, only: [:create]
-    before_filter :check_editable_order_cycle_ids_update, 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]
-    update.after :sync_subscriptions_update
+    update.after :sync_subscriptions_for_update
 
     respond_to :json
 
@@ -39,7 +39,7 @@ module Admin
         @schedule.order_cycle_ids = params[:order_cycle_ids]
         @schedule.save!
 
-        sync_subscriptions_create
+        sync_subscriptions_for_create
 
         flash[:success] = flash_message_for(@schedule, :successfully_created)
         respond_with(@schedule)
@@ -77,24 +77,20 @@ module Admin
       params[:schedule][:order_cycle_ids] = params[:order_cycle_ids]
     end
 
-    def check_editable_order_cycle_ids_create
+    def editable_order_cycle_ids_for_create
       return unless params[:order_cycle_ids]
 
-      requested = params[:order_cycle_ids]
       @existing_order_cycle_ids = []
-
-      result = editable_order_cycles(requested)
+      result = editable_order_cycles(params[:order_cycle_ids])
 
       params[:order_cycle_ids] = result
     end
 
-    def check_editable_order_cycle_ids_update
+    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.order_cycle_ids
-
-      result = editable_order_cycles(requested)
+      result = editable_order_cycles(params[:schedule][:order_cycle_ids])
 
       params[:schedule][:order_cycle_ids] = result
       @schedule.order_cycle_ids = result
@@ -123,13 +119,13 @@ module Admin
       @permissions = OpenFoodNetwork::Permissions.new(spree_current_user)
     end
 
-    def sync_subscriptions_update
+    def sync_subscriptions_for_update
       return unless params[:schedule][:order_cycle_ids]
 
       sync_subscriptions
     end
 
-    def sync_subscriptions_create
+    def sync_subscriptions_for_create
       return unless params[:order_cycle_ids]
 
       sync_subscriptions

From 24e897d145dd9729ab4671ea387cc10167ce2eb0 Mon Sep 17 00:00:00 2001
From: Pau Perez <saulopefa@gmail.com>
Date: Fri, 29 May 2020 15:44:31 +0200
Subject: [PATCH 16/16] Do not build OC with schedules at the same time

It's not possible when neither the OC nor the schedule has an id due to
the new join table order_cycle_schedules.
---
 app/services/order_cycle_form.rb | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/services/order_cycle_form.rb b/app/services/order_cycle_form.rb
index f83b2c57e2..87292a8a0c 100644
--- a/app/services/order_cycle_form.rb
+++ b/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