FEATURE: Delete backups based on time window (#24296)

* FEATURE: core code, tests for feature to allow backups to removed based on a time window

* FEATURE: getting tests working for time-based backup

* FEATURE: getting tests running

* FEATURE: linting

config/locales/server.en.yml

@@ -1824,6 +1824,7 @@ en:
     enable_backups: "Allow administrators to create backups of the forum"
     allow_restore: "Allow restore, which can replace ALL site data! Leave disabled unless you plan to restore a backup"
     maximum_backups: "The maximum amount of backups to keep. Older backups are automatically deleted"
+    remove_older_backups: "Remove backups older than the specified number of days. Leave blank to disable."
     automatic_backups_enabled: "Run automatic backups as defined in backup frequency"
     backup_frequency: "The number of days between backups."
     s3_backup_bucket: "The remote bucket to hold backups. WARNING: Make sure it is a private bucket."

config/site_settings.yml

@@ -2337,6 +2337,9 @@ backups:
   maximum_backups:
     client: true
     default: 5
+  remove_older_backups:
+    client: true
+    default: ""
   automatic_backups_enabled:
     default: true
   backup_frequency:

lib/backup_restore/backup_store.rb

@@ -42,6 +42,16 @@ module BackupRestore
       reset_cache
     end
 
+    def delete_prior_to_n_days
+      window = SiteSetting.remove_older_backups.to_i
+      return unless window && window.is_a?(Numeric) && window > 0
+      return unless cleanup_allowed?
+      files.each do |file|
+        delete_file(file.filename) if file.last_modified < Time.now.ago(window.days)
+      end
+      reset_cache
+    end
+
     def remote?
       fail NotImplementedError
     end

lib/backup_restore/backuper.rb

@@ -51,6 +51,7 @@ module BackupRestore
       @backup_filename
     ensure
       delete_old
+      delete_prior_to_n_days
       clean_up
       notify_user
       log "Finished!"
@@ -358,6 +359,15 @@ module BackupRestore
       log "Something went wrong while deleting old backups.", ex
     end
 
+    def delete_prior_to_n_days
+      return if Rails.env.development?
+
+      log "Deleting backups prior to n days..."
+      store.delete_prior_to_n_days
+    rescue => ex
+      log "Something went wrong while deleting backups prior to n days....", ex
+    end
+
     def notify_user
       return if success && @user.id == Discourse::SYSTEM_USER_ID
 

lib/backup_restore/local_backup_store.rb

@@ -35,7 +35,6 @@ module BackupRestore
 
     def delete_file(filename)
       path = path_from_filename(filename)
-
       if File.exist?(path)
         File.delete(path)
         reset_cache

spec/lib/backup_restore/backuper_spec.rb

@@ -81,20 +81,14 @@ RSpec.describe BackupRestore::Backuper do
 
     before { backup.stubs(:success).returns(success) }
 
-    context "when the result isn't successful" do
-      let(:success) { false }
-
-      it "doesn't refresh disk stats" do
-        store.expects(:reset_cache).never
-        run
-      end
-    end
-
     context "when the result is successful" do
       let(:success) { true }
-
       it "refreshes disk stats" do
-        store.expects(:reset_cache)
+        store.expects(:reset_cache).at_least_once
+        run
+      end
+      it "deletes any old backups" do
+        store.expects(:delete_prior_to_n_days)
         run
       end
     end

spec/lib/backup_restore/shared_examples_for_backup_store.rb

@@ -148,6 +148,29 @@ RSpec.shared_examples "backup store" do
       end
     end
 
+    include ActiveSupport::Testing::TimeHelpers
+    describe "#delete_prior_to_n_days" do
+      it "does nothing if the number of days blank or <1" do
+        SiteSetting.remove_older_backups = ""
+
+        store.delete_prior_to_n_days
+        expect(store.files).to eq([backup1, backup2, backup3])
+      end
+
+      it "removes the two backups that are older than 7 days" do
+        travel_to Time.new(2018, 9, 19, 0, 0, 00)
+        SiteSetting.remove_older_backups = "7"
+
+        # 7 days before 9/19 is 9/12. that's earlier than date
+        # of backup1 (9/13, last_modified= "2018-09-13T15:10:00Z").
+        # Hence backup1 should be retained because it's within the last 7 days.
+        # and backup2 & backup3, which are older, should be deleted
+
+        store.delete_prior_to_n_days
+        expect(store.files).to eq([backup1])
+      end
+    end
+
     describe "#file" do
       it "returns information about the file when the file exists" do
         expect(store.file(backup1.filename)).to eq(backup1)