Allow HistoricDataDeletionWorker to delete Subscribers/SubscriberLists that have older inactive subscriptions.

app/models/subscription.rb

@@ -30,6 +30,7 @@ class Subscription < ApplicationRecord
 
   scope :active, -> { where(ended_at: nil) }
   scope :ended, -> { where.not(ended_at: nil) }
+  scope :not_historical, -> { where("ended_at IS NULL OR ended_at >= ?", 1.week.ago) }
 
   scope :active_on,
         lambda { |date|

app/workers/historical_data_deletion_worker.rb

@@ -1,21 +1,21 @@
 class HistoricalDataDeletionWorker < ApplicationWorker
   def perform
     # cascades matched content changes
-    delete_and_log("content changes") { ContentChange.where("created_at < ?", max_retention_period) }
+    destroy_and_log("content changes") { ContentChange.where("created_at < ?", max_retention_period) }
 
     # cascades matched messages
-    delete_and_log("messages") { Message.where("created_at < ?", max_retention_period) }
+    destroy_and_log("messages") { Message.where("created_at < ?", max_retention_period) }
 
     # cascades digest run subscribers
-    delete_and_log("digest runs") { DigestRun.where("created_at < ?", max_retention_period) }
+    destroy_and_log("digest runs") { DigestRun.where("created_at < ?", max_retention_period) }
 
     # deleting subscriptions must be done before deleting subscriber lists or subscribers
-    delete_and_log("subscriptions") { Subscription.where("ended_at < ?", max_retention_period) }
+    destroy_and_log("subscriptions") { Subscription.where("ended_at < ?", max_retention_period) }
 
-    delete_and_log("subscriber lists") { historic_subscriber_lists }
+    destroy_and_log("subscriber lists") { historic_subscriber_lists }
 
     # restricts deletion if emails are present
-    delete_and_log("subscribers") { historic_subscribers }
+    destroy_and_log("subscribers") { historic_subscribers }
   end
 
 private
@@ -28,17 +28,17 @@ def empty_list_retention_period
     @empty_list_retention_period ||= 7.days.ago
   end
 
-  def delete_and_log(model)
+  def destroy_and_log(model)
     start_time = Time.zone.now
-    deleted_count = yield.delete_all
+    deleted_count = yield.destroy_all
     seconds = (Time.zone.now - start_time).round(2)
 
     message = "Deleted #{deleted_count} #{model} in #{seconds} seconds"
     logger.info(message)
   end
 
   def historic_subscriber_lists
-    subscriptions_exist = Subscription.where(
+    subscriptions_exist = Subscription.not_historical.where(
       "subscriber_lists.id = subscriptions.subscriber_list_id",
     ).arel.exists
 
@@ -48,7 +48,7 @@ def historic_subscriber_lists
   end
 
   def historic_subscribers
-    subscriptions_exist = Subscription.where(
+    subscriptions_exist = Subscription.not_historical.where(
       "subscribers.id = subscriptions.subscriber_id",
     ).arel.exists
 

spec/workers/historical_data_deletion_worker_spec.rb

@@ -71,11 +71,17 @@ def perform
         expect { perform }.to_not change(SubscriberList, :count)
       end
 
-      it "shouldn't remove subscriber lists which have recent active subscriptions" do
+      it "shouldn't remove subscriber lists which have recently ended subscriptions" do
         subscriber_list = create(:subscriber_list, created_at: historic_date)
-        create(:subscription, ended_at: 1.week.ago, subscriber_list:)
+        create(:subscription, ended_at: 6.days.ago, subscriber_list:)
         expect { perform }.to_not change(SubscriberList, :count)
       end
+
+      it "should remove subscriber lists which have older ended subscriptions" do
+        subscriber_list = create(:subscriber_list, created_at: historic_date)
+        create(:subscription, ended_at: 2.weeks.ago, subscriber_list:)
+        expect { perform }.to change(SubscriberList, :count).by(-1)
+      end
     end
 
     context "when deleting subscribers" do
@@ -100,6 +106,12 @@ def perform
         create(:subscription, subscriber:)
         expect { perform }.to_not change(Subscriber, :count)
       end
+
+      it "shouldn't remove old subscribers with recently ended subscriptions" do
+        subscriber = create(:subscriber, created_at: historic_date)
+        create(:subscription, ended_at: 6.days.ago, subscriber:)
+        expect { perform }.to_not change(SubscriberList, :count)
+      end
     end
   end
 end