Billable period updater cleans up untouched billable periods for the current billing period

app/jobs/update_billable_periods.rb

@@ -4,6 +4,7 @@ UpdateBillablePeriods = Struct.new("UpdateBillablePeriods") do
     # Otherwise, calculate turnover for the current month
     start_date = (Time.now - 1.day).beginning_of_month
     end_date = Time.now.beginning_of_day
+    job_start_time = Time.now
 
     enterprises = Enterprise.select([:id, :name, :owner_id, :sells, :shop_trial_start_date, :created_at])
 
@@ -30,6 +31,8 @@ UpdateBillablePeriods = Struct.new("UpdateBillablePeriods") do
       ends_at = end_date
 
       split_for_trial(enterprise, begins_at, ends_at, trial_start, trial_expiry)
+
+      clean_up_untouched_billable_periods_for(enterprise, start_date, job_start_time)
     end
   end
 
@@ -60,27 +63,6 @@ UpdateBillablePeriods = Struct.new("UpdateBillablePeriods") do
     sells = enterprise.sells
     orders = Spree::Order.where('distributor_id = (?) AND completed_at >= (?) AND completed_at < (?)', enterprise.id, begins_at, ends_at)
 
-    # Snagging any BillablePeriods which overlap
-    overlapping_billable_periods = BillablePeriod.where('begins_at <= (?) AND ends_at >= (?) AND enterprise_id = (?)',ends_at, begins_at, enterprise.id)
-    overlapping_billable_periods.each do |billable_period|
-      if billable_period.sells != sells || billable_period.trial != trial || billable_period.owner_id != owner_id
-        Bugsnag.notify(RuntimeError.new("Duplicate BillablePeriod"), {
-          billable_periods: {
-            new: {
-              begins_at: begins_at,
-              ends_at: ends_at,
-              sells: sells,
-              trial: trial,
-              turnover: orders.sum(&:total),
-              owner_id: owner_id,
-              enterprise_id: enterprise.id
-            }.as_json,
-            existing: billable_period.as_json
-          }
-        })
-      end
-    end
-
     billable_period = BillablePeriod.where(begins_at: begins_at, enterprise_id: enterprise.id).first
     billable_period ||= BillablePeriod.new(begins_at: begins_at, enterprise_id: enterprise.id)
     billable_period.update_attributes({
@@ -93,4 +75,20 @@ UpdateBillablePeriods = Struct.new("UpdateBillablePeriods") do
 
     billable_period
   end
+
+  def clean_up_untouched_billable_periods_for(enterprise, start_of_month, job_start_time)
+    # Snag and then delete any BillablePeriods which overlap
+    obsolete_billable_periods = enterprise.billable_periods.where('ends_at >= (?) AND updated_at < (?)', start_of_month, job_start_time)
+
+    if obsolete_billable_periods.any?
+      current_billable_periods = enterprise.billable_periods.where('ends_at >= (?) AND updated_at >= (?)', start_of_month, job_start_time)
+
+      Bugsnag.notify(RuntimeError.new("Duplicate BillablePeriod"), {
+        current: current_billable_periods.map(&:as_json),
+        obsolete: obsolete_billable_periods.map(&:as_json)
+      })
+    end
+
+    obsolete_billable_periods.each(&:delete)
+  end
 end

spec/jobs/update_billable_periods_spec.rb

@@ -14,6 +14,7 @@ describe UpdateBillablePeriods do
       let!(:enterprise) { create(:supplier_enterprise, created_at: start_of_july - 1.month, sells: 'any') }
 
       before do
+        expect(updater).to receive(:clean_up_untouched_billable_periods_for).once
         allow(Enterprise).to receive(:select) { [enterprise] }
       end
 
@@ -371,6 +372,29 @@ describe UpdateBillablePeriods do
         end
       end
     end
+
+    context "cleaning up untouched billable periods" do
+      let(:now) { Time.now }
+      let(:enterprise) { create(:enterprise) }
+      let!(:bp1) { create(:billable_period, enterprise: enterprise, updated_at: now + 2.seconds, begins_at: start_of_july, ends_at: start_of_july + 5.days ) }
+      let!(:bp2) { create(:billable_period, enterprise: enterprise, updated_at: now + 2.seconds, begins_at: start_of_july + 5.days, ends_at: start_of_july + 10.days ) }
+      let!(:bp3) { create(:billable_period, enterprise: enterprise, updated_at: now - 5.seconds, begins_at: start_of_july, ends_at: start_of_july + 10.days ) }
+
+      before do
+        allow(Bugsnag).to receive(:notify)
+        updater.clean_up_untouched_billable_periods_for(enterprise, start_of_july, now)
+      end
+
+      it "soft deletes untouched billable_periods" do
+        expect(bp1.reload.deleted_at).to be_nil
+        expect(bp2.reload.deleted_at).to be_nil
+        expect(bp3.reload.deleted_at).to_not be_nil
+      end
+
+      it "notifies bugsnag" do
+        expect(Bugsnag).to have_received(:notify).once
+      end
+    end
   end
 
   describe "validation spec" do
@@ -383,6 +407,14 @@ describe UpdateBillablePeriods do
 
     let!(:new_owner) { create(:user) }
 
+    # This BP was updated before the current run and so should be marked for deletion at the end of the run
+    let!(:obsolete_bp) { create(:billable_period, enterprise: enterprise, updated_at: start_of_july + 10.days, begins_at: start_of_july + 6.5.days, ends_at: start_of_july + 10.days ) }
+
+    # This one has an updated_at in the future (so that it doesn't get deleted)
+    # It also has a begins_at date which matches a period that would otherwise be created,
+    # and so it should be picked up and overwritten
+    let!(:bp_to_overwrite) { create(:billable_period, enterprise: enterprise, updated_at: start_of_july + 21.days, begins_at: start_of_july + 10.days, ends_at: start_of_july + 15.days ) }
+
     let!(:order1) { create(:order, completed_at: start_of_july + 1.days, distributor: enterprise) }
     let!(:order2) { create(:order, completed_at: start_of_july + 3.days, distributor: enterprise) }
     let!(:order3) { create(:order, completed_at: start_of_july + 5.days, distributor: enterprise) }
@@ -437,9 +469,19 @@ describe UpdateBillablePeriods do
         UpdateBillablePeriods.new.perform
       end
 
-      let(:billable_periods) { BillablePeriod.order(:id) }
+      let(:billable_periods) { BillablePeriod.order(:updated_at) }
+
+      it "creates the correct billable periods and deleted obsolete ones" do
+        expect(obsolete_bp.reload.deleted_at).to_not be_nil
+
+        bp_to_overwrite.reload
+        expect(bp_to_overwrite.sells).to eq 'own'
+        expect(bp_to_overwrite.trial).to be true
+        expect(bp_to_overwrite.owner).to eq original_owner
+        expect(bp_to_overwrite.begins_at).to eq start_of_july + 10.days
+        expect(bp_to_overwrite.ends_at).to eq start_of_july + 12.days
+        expect(bp_to_overwrite.turnover).to eq order6.total
 
-      it "creates the correct billable periods" do
         expect(billable_periods.count).to eq 9
 
         expect(billable_periods.map(&:begins_at)).to eq [