Support for removal of old backups automatically via a site setting

app/controllers/admin/backups_controller.rb

@@ -49,9 +49,13 @@ class Admin::BackupsController < Admin::AdminController
   end
 
   def destroy
-    filename = params.fetch(:id)
+    backup = Backup[params.fetch(:id)]
-    Backup.remove(filename)
+    if backup
+      backup.remove
       render nothing: true
+    else
+      render nothing: true, status: 404
+    end
   end
 
   def logs

app/models/backup.rb

@@ -2,32 +2,29 @@ class Backup
   include UrlHelper
   include ActiveModel::SerializerSupport
 
-  attr_reader :filename, :size, :path, :link
+  attr_reader :filename
+  attr_accessor :size, :path, :link
 
   def initialize(filename)
     @filename = filename
-    @path = File.join(Backup.base_directory, filename)
-    @link = schemaless "#{Discourse.base_url}/admin/backups/#{filename}"
-    @size = File.size(@path)
   end
 
   def self.all
     backups = Dir.glob(File.join(Backup.base_directory, "*.tar.gz"))
-    backups.sort.reverse.map { |backup| Backup.new(File.basename(backup)) }
+    backups.sort.reverse.map { |backup| Backup.create_from_filename(File.basename(backup)) }
   end
 
   def self.[](filename)
     path = File.join(Backup.base_directory, filename)
     if File.exists?(path)
-      Backup.new(filename)
+      Backup.create_from_filename(filename)
     else
       nil
     end
   end
 
-  def self.remove(filename)
+  def remove
-    path = File.join(Backup.base_directory, filename)
+    File.delete(@path) if File.exists?(path)
-    File.delete(path) if File.exists?(path)
   end
 
   def self.base_directory
@@ -38,4 +35,18 @@ class Backup
     File.join(Backup.base_directory, "tmp", identifier, "#{filename}.part#{chunk_number}")
   end
 
+  def self.create_from_filename(filename)
+    Backup.new(filename).tap do |b|
+      b.path = File.join(Backup.base_directory, b.filename)
+      b.link = b.schemaless "#{Discourse.base_url}/admin/backups/#{b.filename}"
+      b.size = File.size(b.path)
+    end
+  end
+
+  def self.remove_old
+    all_backups = Backup.all
+    return unless all_backups.size > SiteSetting.maximum_backups
+    all_backups[SiteSetting.maximum_backups..-1].each {|b| b.remove}
+  end
+
 end

config/locales/client.en.yml

@@ -1665,6 +1665,7 @@ en:
           embedding: "Embedding"
           legal: "Legal"
           uncategorized: 'Uncategorized'
+          backups: "Backups"
 
     lightbox:
       download: "download"

config/locales/server.en.yml

@@ -712,6 +712,7 @@ en:
     github_client_secret: "Client secret for Github authentication, registered at https://github.com/settings/applications"
 
     allow_restore: "Allow restore, which can replace ALL site data! Leave false unless you plan to do restore a backup"
+    maximum_backups: "The maximum amount of backups to keep on disk. Older backups are automatically deleted"
 
     active_user_rate_limit_secs: "How frequently we update the 'last_seen_at' field, in seconds"
     previous_visit_timeout_hours: "How long a visit lasts before we consider it the 'previous' visit, in hours"

config/site_settings.yml

@@ -409,6 +409,14 @@ legal:
     client: true
     default: false
 
+backups:
+  allow_restore:
+    client: true
+    default: false
+  maximum_backups:
+    client: true
+    default: 7
+
 uncategorized:
 
   faq_url:
@@ -449,9 +457,6 @@ uncategorized:
   summary_likes_required: 1
   summary_percent_filter: 20
   send_welcome_message: true
-  allow_restore:
-    client: true
-    default: false
   educate_until_posts:
     client: true
     default: 2

lib/export/exporter.rb

@@ -38,6 +38,8 @@ module Export
 
       create_archive
 
+      remove_old
+
       notify_user
     rescue SystemExit
       log "Backup process was cancelled!"
@@ -240,6 +242,11 @@ module Export
       SystemMessage.create(@user, :export_succeeded)
     end
 
+    def remove_old
+      log "Removing old backups..."
+      Backup.remove_old
+    end
+
     def clean_up
       log "Cleaning stuff up..."
       remove_tmp_directory

spec/controllers/admin/backups_controller_spec.rb

@@ -36,7 +36,6 @@ describe Admin::BackupsController do
       context "json format" do
 
         it "returns a list of all the backups" do
-          File.stubs(:size).returns(42)
           Backup.expects(:all).returns([Backup.new("backup1"), Backup.new("backup2")])
 
           xhr :get, :index, format: :json
@@ -104,9 +103,7 @@ describe Admin::BackupsController do
       it "uses send_file to transmit the backup" do
         controller.stubs(:render) # we need this since we're stubing send_file
 
-        File.stubs(:size).returns(42)
         backup = Backup.new("backup42")
-
         Backup.expects(:[]).with(backup_filename).returns(backup)
         subject.expects(:send_file).with(backup.path)
 
@@ -125,14 +122,22 @@ describe Admin::BackupsController do
 
     describe ".destroy" do
 
-      it "removes the backup" do
+      let(:b) { Backup.new(backup_filename) }
-        Backup.expects(:remove).with(backup_filename)
 
+      it "removes the backup if found" do
+        Backup.expects(:[]).with(backup_filename).returns(b)
+        b.expects(:remove)
         xhr :delete, :destroy, id: backup_filename
-
         response.should be_success
       end
 
+      it "doesn't remove the backup if not found" do
+        Backup.expects(:[]).with(backup_filename).returns(nil)
+        b.expects(:remove).never
+        xhr :delete, :destroy, id: backup_filename
+        response.should_not be_success
+      end
+
     end
 
     describe ".logs" do

spec/models/backup_spec.rb

@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+require_dependency 'backup'
+
+describe Backup do
+
+  let(:b1) { Backup.new('backup1') }
+  let(:b2) { Backup.new('backup2') }
+  let(:b3) { Backup.new('backup3') }
+
+  before do
+    Backup.stubs(:all).returns([b1, b2, b3])
+  end
+
+  context '#remove_old' do
+    it "does nothing if there aren't more backups than the setting" do
+      SiteSetting.stubs(:maximum_backups).returns(3)
+      Backup.any_instance.expects(:remove).never
+      Backup.remove_old
+    end
+
+    it "calls remove on the backups over our limit" do
+      SiteSetting.stubs(:maximum_backups).returns(1)
+      b1.expects(:remove).never
+      b2.expects(:remove).once
+      b3.expects(:remove).once
+      Backup.remove_old
+    end
+  end
+end
+