Use placed_at state to determine whether to remove orphaned proxy orders

lib/open_food_network/proxy_order_syncer.rb

@@ -56,9 +56,10 @@ module OpenFoodNetwork
     end
 
     def orphaned_proxy_orders
-      order_cycle_ids = active_or_complete_or_in_range_order_cycle_ids
-      return proxy_orders unless order_cycle_ids.any?
-      proxy_orders.where('order_cycle_id NOT IN (?)', order_cycle_ids)
+      orphaned = proxy_orders.where(placed_at: nil)
+      order_cycle_ids = in_range_order_cycles.pluck(:id)
+      return orphaned unless order_cycle_ids.any?
+      orphaned.where('order_cycle_id NOT IN (?)', order_cycle_ids)
     end
 
     def insert_values
@@ -72,10 +73,6 @@ module OpenFoodNetwork
       in_range_order_cycles.merge(OrderCycle.not_closed)
     end
 
-    def active_or_complete_or_in_range_order_cycle_ids
-      in_range_order_cycles.pluck(:id) | order_cycles.active_or_complete.pluck(:id)
-    end
-
     def in_range_order_cycles
       order_cycles.where('orders_close_at >= ? AND orders_close_at <= ?', begins_at, ends_at || 100.years.from_now)
     end

spec/lib/open_food_network/proxy_order_syncer_spec.rb

@@ -16,6 +16,7 @@ module OpenFoodNetwork
       let(:now) { Time.zone.now }
       let(:schedule) { create(:schedule) }
       let(:closed_oc) { create(:simple_order_cycle, schedules: [schedule], orders_open_at: now - 1.minute, orders_close_at: now) } # Closed
+      let(:open_oc_closes_before_begins_at_oc) { create(:simple_order_cycle, schedules: [schedule], orders_open_at: now - 1.minute, orders_close_at: now + 59.seconds) } # Open, but closes before begins at
       let(:open_oc) { create(:simple_order_cycle, schedules: [schedule], orders_open_at: now - 1.minute, orders_close_at: now + 90.seconds) } # Open & closes between begins at and ends at
       let(:upcoming_closes_before_begins_at_oc) { create(:simple_order_cycle, schedules: [schedule], orders_open_at: now + 30.seconds, orders_close_at: now + 59.seconds) } # Upcoming, but closes before begins at
       let(:upcoming_closes_on_begins_at_oc) { create(:simple_order_cycle, schedules: [schedule], orders_open_at: now + 30.seconds, orders_close_at: now + 1.minute) } # Upcoming & closes on begins at
@@ -32,7 +33,7 @@ module OpenFoodNetwork
           expect{ syncer.sync! }.to_not change(ProxyOrder, :count).from(0)
         end
 
-        context "and the schedule includes a closed oc" do
+        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{ subscription.save! }.to_not change(ProxyOrder, :count).from(0)
@@ -40,6 +41,14 @@ module OpenFoodNetwork
           end
         end
 
+        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
+          end
+        end
+
         context "and the schedule includes 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
@@ -88,51 +97,121 @@ module OpenFoodNetwork
           let!(:proxy_order) { create(:proxy_order, subscription: subscription, order_cycle: oc) }
 
           context "for an oc included in the relevant schedule" do
-            context "the oc is closed" do
-              let(:oc) { closed_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 order has already been placed" do
+              before { proxy_order.update_attributes(placed_at: 5.minutes.ago) }
+
+              context "the oc is closed (ie. closed before opens_at)" do
+                let(:oc) { closed_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 schedule includes an open oc that closes before begins_at" do
+                let(:oc) { open_oc_closes_before_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 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 before begins_at" do
+                let(:oc) { upcoming_closes_before_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 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 "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 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 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
+                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
+              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
-            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
+              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
-            end
 
-            context "and the oc is upcoming and closes after 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
+              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
-            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
+              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
+                end
               end
             end
           end
@@ -144,15 +223,122 @@ module OpenFoodNetwork
               expect(open_oc.save!).to be true
             end
 
-            it "removes the proxy order" do
-              expect{ syncer.sync! }.to change(ProxyOrder, :count).by(-1)
-              expect(proxy_orders).to_not include proxy_order
+            context "and the proxy order has already been placed" do
+              before { proxy_order.update_attributes(placed_at: 5.minutes.ago) }
+
+              context "the oc is closed (ie. closed before opens_at)" do
+                let(:oc) { closed_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 schedule includes an open oc that closes before begins_at" do
+                let(:oc) { open_oc_closes_before_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 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 before begins_at" do
+                let(:oc) { upcoming_closes_before_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 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 "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 proxy order has not already been placed" do
+              # This shouldn't really happen, but it is possible
+              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
+                end
+              end
+
+              # This shouldn't really happen, but it is possible
+              context "and the oc is open and closes between begins_at and ends_at" do
+                let(:oc) { open_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 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 "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 ends_at" do
+                let(:oc) { upcoming_closes_on_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
+                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
+                end
+              end
             end
           end
         end
 
         context "when a proxy order does not exist" do
-          context "and the schedule includes a closed oc" do
+          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)
@@ -160,6 +346,14 @@ module OpenFoodNetwork
             end
           end
 
+          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
+            end
+          end
+
           context "and the schedule includes 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
@@ -184,7 +378,7 @@ module OpenFoodNetwork
             end
           end
 
-          context "and the schedule includes upcoming oc that closes after ends_at" do
+          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)