Merge pull request #4596 from consul/cached_attachments_task

Use a rake task to delete cached attachments

app/assets/javascripts/documentable.js

@@ -115,7 +115,8 @@
       $("#max-documents-notice").removeClass("hide");
     },
     initializeRemoveCachedDocumentLinks: function() {
-      $("#nested-documents").on("ajax:complete", "a.remove-cached-attachment", function() {
+      $("#nested-documents").on("click", "a.remove-cached-attachment", function(event) {
+        event.preventDefault();
         App.Documentable.unlockUploads();
         $(this).closest(".direct-upload").remove();
       });

app/assets/javascripts/imageable.js

@@ -119,7 +119,8 @@
       }
     },
     initializeRemoveCachedImageLinks: function() {
-      $("#nested-image").on("ajax:complete", "a.remove-cached-attachment", function() {
+      $("#nested-image").on("click", "a.remove-cached-attachment", function(event) {
+        event.preventDefault();
         $("#new_image_link").removeClass("hide");
         $(this).closest(".direct-upload").remove();
       });

app/components/documents/fields_component.rb

@@ -18,16 +18,7 @@ class Documents::FieldsComponent < ApplicationComponent
 
     def destroy_link
       if !document.persisted? && document.cached_attachment.present?
-        link_to t("documents.form.delete_button"),
+        link_to t("documents.form.delete_button"), "#", class: "delete remove-cached-attachment"
-          direct_upload_destroy_path(
-            "direct_upload[resource_type]": document.documentable_type,
-            "direct_upload[resource_id]": document.documentable_id,
-            "direct_upload[resource_relation]": "documents",
-            "direct_upload[cached_attachment]": document.cached_attachment
-          ),
-          method: :delete,
-          remote: true,
-          class: "delete remove-cached-attachment"
       else
         link_to_remove_association document.new_record? ? t("documents.form.cancel_button") : t("documents.form.delete_button"), f, class: "delete remove-document"
       end

app/components/images/fields_component.rb

@@ -19,16 +19,7 @@ class Images::FieldsComponent < ApplicationComponent
 
     def destroy_link
       if !image.persisted? && image.cached_attachment.present?
-        link_to t("images.form.delete_button"),
+        link_to t("images.form.delete_button"), "#", class: "delete remove-cached-attachment"
-          direct_upload_destroy_path(
-            "direct_upload[resource_type]": image.imageable_type,
-            "direct_upload[resource_id]": image.imageable_id,
-            "direct_upload[resource_relation]": "image",
-            "direct_upload[cached_attachment]": image.cached_attachment
-          ),
-          method: :delete,
-          remote: true,
-          class: "delete remove-cached-attachment"
       else
         link_to_remove_association t("images.form.delete_button"), f, class: "delete remove-image"
       end

app/controllers/direct_uploads_controller.rb

@@ -20,23 +20,11 @@ class DirectUploadsController < ApplicationController
                      destroy_link: render_destroy_upload_link(@direct_upload),
                      attachment_url: @direct_upload.relation.attachment.url }
     else
-      @direct_upload.destroy_attachment
       render json: { errors: @direct_upload.errors[:attachment].join(", ") },
              status: 422
     end
   end
 
-  def destroy
-    @direct_upload = DirectUpload.new(direct_upload_params.merge(user: current_user))
-    @direct_upload.relation.set_attachment_from_cached_attachment
-
-    if @direct_upload.destroy_attachment
-      render json: :ok
-    else
-      render json: :error
-    end
-  end
-
   private
 
     def direct_upload_params

app/helpers/direct_uploads_helper.rb

@@ -1,16 +1,6 @@
 module DirectUploadsHelper
   def render_destroy_upload_link(direct_upload)
     label = direct_upload.resource_relation == "image" ? "images" : "documents"
-    link_to t("#{label}.form.delete_button"),
+    link_to t("#{label}.form.delete_button"), "#", class: "delete remove-cached-attachment"
-            direct_upload_destroy_path(
-              "direct_upload[resource_type]": direct_upload.resource_type,
-              "direct_upload[resource_id]": direct_upload.resource_id,
-              "direct_upload[resource_relation]": direct_upload.resource_relation,
-              "direct_upload[cached_attachment]": direct_upload.relation.cached_attachment,
-              format: :json
-            ),
-            method: :delete,
-            remote: true,
-            class: "delete remove-cached-attachment"
   end
 end

app/models/direct_upload.rb

@@ -37,10 +37,6 @@ class DirectUpload
     @relation.attachment.save
   end
 
-  def destroy_attachment
-    @relation.attachment.destroy
-  end
-
   def persisted?
     false
   end

app/models/document.rb

@@ -5,7 +5,7 @@ class Document < ApplicationRecord
                                  hash_data: ":class/:style/:custom_hash_data",
                                  use_timestamp: false,
                                  hash_secret: Rails.application.secrets.secret_key_base
-  attr_accessor :cached_attachment, :remove, :original_filename
+  attr_accessor :cached_attachment
 
   belongs_to :user
   belongs_to :documentable, polymorphic: true
@@ -22,7 +22,6 @@ class Document < ApplicationRecord
   validates :documentable_type, presence: true,       if: -> { persisted? }
 
   before_save :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? }
-  after_save :remove_cached_attachment,               if: -> { cached_attachment.present? }
 
   scope :admin, -> { where(admin: true) }
 
@@ -59,9 +58,7 @@ class Document < ApplicationRecord
   end
 
   def custom_hash_data(attachment)
-    original_filename = if !attachment.instance.persisted? && attachment.instance.remove
+    original_filename = if !attachment.instance.persisted?
-                          attachment.instance.original_filename
-                        elsif !attachment.instance.persisted?
                           attachment.instance.attachment_file_name
                         else
                           attachment.instance.title
@@ -104,14 +101,4 @@ class Document < ApplicationRecord
         errors.add(:attachment, I18n.t("errors.messages.blank"))
       end
     end
-
-    def remove_cached_attachment
-      document = Document.new(documentable: documentable,
-                              cached_attachment: cached_attachment,
-                              user: user,
-                              remove: true,
-                              original_filename: title)
-      document.set_attachment_from_cached_attachment
-      document.attachment.destroy
-    end
 end

app/models/image.rb

@@ -30,7 +30,6 @@ class Image < ApplicationRecord
   validate :validate_image_dimensions, if: -> { attachment.present? && attachment.dirty? }
 
   before_save :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? }
-  after_save :remove_cached_attachment,               if: -> { cached_attachment.present? }
 
   def set_cached_attachment_from_attachment
     self.cached_attachment = if Paperclip::Attachment.default_options[:storage] == :filesystem
@@ -120,12 +119,4 @@ class Image < ApplicationRecord
     def attachment_of_valid_content_type?
       attachment.present? && imageable_accepted_content_types.include?(attachment_content_type)
     end
-
-    def remove_cached_attachment
-      image = Image.new(imageable: imageable,
-                        cached_attachment: cached_attachment,
-                        user: user)
-      image.set_attachment_from_cached_attachment
-      image.attachment.destroy
-    end
 end

config/routes/direct_upload.rb

@@ -1,2 +1 @@
 resources :direct_uploads, only: [:create]
-delete "direct_uploads/destroy", to: "direct_uploads#destroy", as: :direct_upload_destroy

config/schedule.rb

@@ -36,6 +36,10 @@ end
 #   rake "dashboards:send_notifications"
 # end
 
+every 1.day, at: "1:00 am", roles: [:cron] do
+  rake "files:remove_old_cached_attachments"
+end
+
 every 1.day, at: "3:00 am", roles: [:cron] do
   rake "votes:reset_hot_score"
 end

lib/tasks/files.rake

@@ -0,0 +1,18 @@
+namespace :files do
+  desc "Removes cached attachments which weren't deleted for some reason"
+  task remove_old_cached_attachments: :environment do
+    paths = Dir.glob(Rails.root.join("public/system/*/cached_attachments/**/*"))
+    logger = ApplicationLogger.new
+
+    paths.each do |path|
+      if File.file?(path)
+        if File.mtime(path) < 1.day.ago
+          File.delete(path)
+          logger.info "The file #{path} has been removed"
+        end
+      else
+        Dir.delete(path) if Dir.empty?(path)
+      end
+    end
+  end
+end

spec/lib/tasks/files_spec.rb

@@ -0,0 +1,46 @@
+require "rails_helper"
+
+describe "files tasks" do
+  describe "remove_old_cached_attachments" do
+    let(:run_rake_task) do
+      Rake::Task["files:remove_old_cached_attachments"].reenable
+      Rake.application.invoke_task("files:remove_old_cached_attachments")
+    end
+
+    it "deletes old cached attachments" do
+      image = build(:image)
+      document = build(:document)
+
+      image.attachment.save
+      document.attachment.save
+
+      travel_to(2.days.from_now) { run_rake_task }
+
+      expect(File.exists?(image.attachment.path)).to be false
+      expect(File.exists?(document.attachment.path)).to be false
+    end
+
+    it "does not delete recent cached attachments" do
+      image = build(:image)
+      document = build(:document)
+
+      image.attachment.save
+      document.attachment.save
+
+      travel_to(2.minutes.from_now) { run_rake_task }
+
+      expect(File.exists?(image.attachment.path)).to be true
+      expect(File.exists?(document.attachment.path)).to be true
+    end
+
+    it "does not delete old regular attachments" do
+      image = create(:image)
+      document = create(:document)
+
+      travel_to(2.days.from_now) { run_rake_task }
+
+      expect(File.exists?(image.attachment.path)).to be true
+      expect(File.exists?(document.attachment.path)).to be true
+    end
+  end
+end

spec/models/direct_upload_spec.rb

@@ -43,16 +43,4 @@ describe DirectUpload do
       expect(proposal_document_direct_upload.relation.attachment.path).to include("cached_attachments")
     end
   end
-
-  context "destroy_attachment" do
-    it "removes uploaded file" do
-      proposal_document_direct_upload = build(:direct_upload, :proposal, :documents)
-
-      proposal_document_direct_upload.save_attachment
-      uploaded_path = proposal_document_direct_upload.relation.attachment.path
-      proposal_document_direct_upload.destroy_attachment
-
-      expect(File.exist?(uploaded_path)).to eq(false)
-    end
-  end
 end