diff --git a/app/controllers/admin/schedules_controller.rb b/app/controllers/admin/schedules_controller.rb index 44ccb48d64..d0728ebc18 100644 --- a/app/controllers/admin/schedules_controller.rb +++ b/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 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/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 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..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 @@ -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 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 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 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