consul/nairobi
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Use a rake task to delete cached attachments

Browse Source 
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.
This commit is contained in:
Javi Martín
2021-07-23 18:59:55 +02:00
parent a8c4cc3edc
commit 930bb753c5
14 changed files with 77 additions and 86 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
app/assets/javascripts/documentable.js
View File

@@ -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();
});

3
app/assets/javascripts/imageable.js
View File

@@ -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();
});

11
app/components/documents/fields_component.rb
View File

@@ -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

11
app/components/images/fields_component.rb
View File

@@ -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

12
app/controllers/direct_uploads_controller.rb
View File

@@ -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

12
app/helpers/direct_uploads_helper.rb
View File

@@ -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

4
app/models/direct_upload.rb
View File

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

17
app/models/document.rb
View File

@@ -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

9
app/models/image.rb
View File

@@ -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

1
config/routes/direct_upload.rb
View File

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

4
config/schedule.rb
View File

@@ -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

18
lib/tasks/files.rake Normal file
View File

@@ -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

46
spec/lib/tasks/files_spec.rb Normal file
View File

@@ -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

12
spec/models/direct_upload_spec.rb
View File

@@ -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