From dc1d6decf5a0a9ba9d445ed7493490e59390db49 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 11 Mar 2014 17:28:12 -0400 Subject: [PATCH] Support for removal of old backups automatically via a site setting --- app/controllers/admin/backups_controller.rb | 10 ++++-- app/models/backup.rb | 29 +++++++++++------ config/locales/client.en.yml | 1 + config/locales/server.en.yml | 1 + config/site_settings.yml | 11 +++++-- lib/export/exporter.rb | 7 +++++ .../admin/backups_controller_spec.rb | 17 ++++++---- spec/models/backup_spec.rb | 31 +++++++++++++++++++ 8 files changed, 86 insertions(+), 21 deletions(-) create mode 100644 spec/models/backup_spec.rb diff --git a/app/controllers/admin/backups_controller.rb b/app/controllers/admin/backups_controller.rb index 9bc6d779a60..94881c12b23 100644 --- a/app/controllers/admin/backups_controller.rb +++ b/app/controllers/admin/backups_controller.rb @@ -49,9 +49,13 @@ class Admin::BackupsController < Admin::AdminController end def destroy - filename = params.fetch(:id) - Backup.remove(filename) - render nothing: true + backup = Backup[params.fetch(:id)] + if backup + backup.remove + render nothing: true + else + render nothing: true, status: 404 + end end def logs diff --git a/app/models/backup.rb b/app/models/backup.rb index f22514ba352..e871dded713 100644 --- a/app/models/backup.rb +++ b/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) - path = File.join(Backup.base_directory, filename) - File.delete(path) if File.exists?(path) + def remove + 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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5e29bbcf029..74fdbc06d8c 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1665,6 +1665,7 @@ en: embedding: "Embedding" legal: "Legal" uncategorized: 'Uncategorized' + backups: "Backups" lightbox: download: "download" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5c820fc1542..85c27b16786 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/config/site_settings.yml b/config/site_settings.yml index 3d59f776a06..e8170f7980c 100644 --- a/config/site_settings.yml +++ b/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 diff --git a/lib/export/exporter.rb b/lib/export/exporter.rb index 37ce8a7df7e..e7e471b43ff 100644 --- a/lib/export/exporter.rb +++ b/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 diff --git a/spec/controllers/admin/backups_controller_spec.rb b/spec/controllers/admin/backups_controller_spec.rb index b6b6b85da09..b9a3ef3be67 100644 --- a/spec/controllers/admin/backups_controller_spec.rb +++ b/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 - Backup.expects(:remove).with(backup_filename) + let(:b) { Backup.new(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 diff --git a/spec/models/backup_spec.rb b/spec/models/backup_spec.rb new file mode 100644 index 00000000000..746cb3b3063 --- /dev/null +++ b/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 +