From 930bb753c58d0dd889c62afffcc702ee6c0104bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 23 Jul 2021 18:59:55 +0200 Subject: [PATCH] Use a rake task to delete cached attachments Our previous system to delete cached attachments didn't work for documents because the `custom_hash_data` is different for files created from a file and files created from cached attachments. When creating a document attachment, the name of the file is taken into account to calculate the hash. Let's say the original file name is "logo.pdf", and the generated hash is "123456". The cached attachment will be "123456.pdf", so the generated hash using the cached attachment will be something different, like "28af3". So the file that will be removed will be "28af3.pdf", and not "123456.pdf", which will still be present. Furthermore, there are times where users choose a file and then they close the browser or go to a different page. In those cases, we weren't deleting the cached attachments either. So we're adding a rake task to delete these files once a day. This way we can simplify the logic we were using to destroy cached attachments. Note there's related a bug in documents: when editing a record (for example, a proposal), if the title of the document changes, its hash changes, and so it will be impossible to generate a link to that document. Changing the way this hash is generated is not an option because it would break links to existing files. We'll try to fix it when moving to Active Storage. --- app/assets/javascripts/documentable.js | 3 +- app/assets/javascripts/imageable.js | 3 +- app/components/documents/fields_component.rb | 11 +---- app/components/images/fields_component.rb | 11 +---- app/controllers/direct_uploads_controller.rb | 12 ----- app/helpers/direct_uploads_helper.rb | 12 +---- app/models/direct_upload.rb | 4 -- app/models/document.rb | 17 +------- app/models/image.rb | 9 ---- config/routes/direct_upload.rb | 1 - config/schedule.rb | 4 ++ lib/tasks/files.rake | 18 ++++++++ spec/lib/tasks/files_spec.rb | 46 ++++++++++++++++++++ spec/models/direct_upload_spec.rb | 12 ----- 14 files changed, 77 insertions(+), 86 deletions(-) create mode 100644 lib/tasks/files.rake create mode 100644 spec/lib/tasks/files_spec.rb diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 7aa9e1b29..3a2880da5 100644 --- a/app/assets/javascripts/documentable.js +++ b/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(); }); diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index a6e658796..84ad8ce8f 100644 --- a/app/assets/javascripts/imageable.js +++ b/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(); }); diff --git a/app/components/documents/fields_component.rb b/app/components/documents/fields_component.rb index 3aa0126bf..1ef9febea 100644 --- a/app/components/documents/fields_component.rb +++ b/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"), - 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" + link_to t("documents.form.delete_button"), "#", 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 diff --git a/app/components/images/fields_component.rb b/app/components/images/fields_component.rb index 9b6ceb80d..ddf60fa45 100644 --- a/app/components/images/fields_component.rb +++ b/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"), - 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" + link_to t("images.form.delete_button"), "#", class: "delete remove-cached-attachment" else link_to_remove_association t("images.form.delete_button"), f, class: "delete remove-image" end diff --git a/app/controllers/direct_uploads_controller.rb b/app/controllers/direct_uploads_controller.rb index a127967c7..b46fdda29 100644 --- a/app/controllers/direct_uploads_controller.rb +++ b/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 diff --git a/app/helpers/direct_uploads_helper.rb b/app/helpers/direct_uploads_helper.rb index eabb0425e..bc89bb285 100644 --- a/app/helpers/direct_uploads_helper.rb +++ b/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"), - 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" + link_to t("#{label}.form.delete_button"), "#", class: "delete remove-cached-attachment" end end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 2d2e9c14b..2b2760011 100644 --- a/app/models/direct_upload.rb +++ b/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 diff --git a/app/models/document.rb b/app/models/document.rb index 045370663..e6e21b526 100644 --- a/app/models/document.rb +++ b/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 - attachment.instance.original_filename - elsif !attachment.instance.persisted? + original_filename = if !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 diff --git a/app/models/image.rb b/app/models/image.rb index 9268fbfcc..7bcac83f1 100644 --- a/app/models/image.rb +++ b/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 diff --git a/config/routes/direct_upload.rb b/config/routes/direct_upload.rb index dbcf9a9d0..434926f00 100644 --- a/config/routes/direct_upload.rb +++ b/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 diff --git a/config/schedule.rb b/config/schedule.rb index 0c70a3ec7..9acb31c19 100644 --- a/config/schedule.rb +++ b/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 diff --git a/lib/tasks/files.rake b/lib/tasks/files.rake new file mode 100644 index 000000000..ef46097cc --- /dev/null +++ b/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 diff --git a/spec/lib/tasks/files_spec.rb b/spec/lib/tasks/files_spec.rb new file mode 100644 index 000000000..2c464bc82 --- /dev/null +++ b/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 diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index c4bc491aa..9c5657d55 100644 --- a/spec/models/direct_upload_spec.rb +++ b/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