diff --git a/app/controllers/direct_uploads_controller.rb b/app/controllers/direct_uploads_controller.rb index a1995c2e3..9bc565a29 100644 --- a/app/controllers/direct_uploads_controller.rb +++ b/app/controllers/direct_uploads_controller.rb @@ -15,9 +15,9 @@ class DirectUploadsController < ApplicationController @direct_upload.relation.set_cached_attachment_from_attachment render json: { cached_attachment: @direct_upload.relation.cached_attachment, - filename: @direct_upload.relation.attachment.original_filename, + filename: @direct_upload.relation.storage_attachment.filename.to_s, destroy_link: render_destroy_upload_link(@direct_upload), - attachment_url: @direct_upload.relation.attachment.url } + attachment_url: polymorphic_path(@direct_upload.relation.storage_attachment) } else render json: { errors: @direct_upload.errors[:attachment].join(", ") }, status: :unprocessable_entity diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index 9fb8c7a80..a8fda86ac 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -9,14 +9,10 @@ module Attachable # Paperclip do not allow to use Procs on valiations definition do_not_validate_attachment_file_type :attachment validate :attachment_presence - validate :validate_attachment_content_type, if: -> { attachment.present? } - validate :validate_attachment_size, if: -> { attachment.present? } + validate :validate_attachment_content_type, if: -> { storage_attachment.attached? } + validate :validate_attachment_size, if: -> { storage_attachment.attached? } - before_save :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } - - Paperclip.interpolates :prefix do |attachment, style| - attachment.instance.prefix(attachment, style) - end + before_validation :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } end def association_class @@ -26,29 +22,37 @@ module Attachable end def set_cached_attachment_from_attachment - self.cached_attachment = if filesystem_storage? - attachment.path - else - attachment.url - end + self.cached_attachment = storage_attachment.signed_id end def set_attachment_from_cached_attachment + self.storage_attachment = cached_attachment + if filesystem_storage? - File.open(cached_attachment) { |file| self.attachment = file } + File.open(file_path) do |file| + self.paperclip_attachment = file + end else - self.attachment = URI.parse(cached_attachment).open + self.paperclip_attachment = URI.parse(cached_attachment).open end end - def prefix(attachment, _style) - if attachment.instance.persisted? - ":attachment/:id_partition" + def attachment_content_type + storage_attachment.blob.content_type if storage_attachment.attached? + end + + def attachment_file_size + if storage_attachment.attached? + storage_attachment.blob.byte_size else - "cached_attachments/user/#{attachment.instance.user_id}" + 0 end end + def file_path + ActiveStorage::Blob.service.path_for(storage_attachment.blob.key) + end + private def filesystem_storage? @@ -73,7 +77,7 @@ module Attachable end def attachment_presence - if attachment.blank? && cached_attachment.blank? + unless storage_attachment.attached? errors.add(:attachment, I18n.t("errors.messages.blank")) end end diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb index 6f3f67844..8fcafa3ec 100644 --- a/app/models/concerns/has_attachment.rb +++ b/app/models/concerns/has_attachment.rb @@ -5,18 +5,21 @@ module HasAttachment def has_attachment(attribute, paperclip_options = {}) has_one_attached :"storage_#{attribute}" - has_attached_file attribute, paperclip_options - alias_method :"paperclip_#{attribute}=", :"#{attribute}=" - - define_method :"#{attribute}=" do |file| - if file.is_a?(IO) || file.is_a?(Tempfile) && !file.is_a?(Ckeditor::Http::QqFile) + define_method :"storage_#{attribute}=" do |file| + if file.is_a?(IO) send(:"storage_#{attribute}").attach(io: file, filename: File.basename(file.path)) elsif file.nil? send(:"storage_#{attribute}").detach else send(:"storage_#{attribute}").attach(file) end + end + has_attached_file attribute, paperclip_options + alias_method :"paperclip_#{attribute}=", :"#{attribute}=" + + define_method :"#{attribute}=" do |file| + send(:"storage_#{attribute}=", file) send(:"paperclip_#{attribute}=", file) end end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 7e66375e7..66a9d45ab 100644 --- a/app/models/direct_upload.rb +++ b/app/models/direct_upload.rb @@ -34,7 +34,7 @@ class DirectUpload end def save_attachment - @relation.attachment.save + @relation.storage_attachment.blob.save! end def persisted? @@ -53,7 +53,7 @@ class DirectUpload def relation_attributtes { - paperclip_attachment: @attachment, + storage_attachment: @attachment, cached_attachment: @cached_attachment, user: @user } diff --git a/app/models/document.rb b/app/models/document.rb index a121e3bbd..4b9349f3e 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -1,7 +1,7 @@ class Document < ApplicationRecord include Attachable - has_attachment :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", + has_attachment :attachment, url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", hash_data: ":class/:style/:custom_hash_data", use_timestamp: false, hash_secret: Rails.application.secrets.secret_key_base diff --git a/app/models/image.rb b/app/models/image.rb index a86091648..098515114 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -6,7 +6,7 @@ class Image < ApplicationRecord medium: "300x300#", thumb: "140x245#" }, - url: "/system/:class/:prefix/:style/:hash.:extension", + url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", hash_data: ":class/:style", use_timestamp: false, hash_secret: Rails.application.secrets.secret_key_base @@ -27,7 +27,7 @@ class Image < ApplicationRecord validates :user_id, presence: true validates :imageable_id, presence: true, if: -> { persisted? } validates :imageable_type, presence: true, if: -> { persisted? } - validate :validate_image_dimensions, if: -> { attachment.present? && attachment.dirty? } + validate :validate_image_dimensions, if: -> { storage_attachment.attached? && storage_attachment.new_record? } def self.max_file_size Setting["uploads.images.max_size"].to_i @@ -68,14 +68,17 @@ class Image < ApplicationRecord end def validate_image_dimensions - if attachment_of_valid_content_type? + if accepted_content_types.include?(attachment_content_type) return true if imageable_class == Widget::Card - dimensions = Paperclip::Geometry.from_file(attachment.queued_for_write[:original].path) + storage_attachment.analyze unless storage_attachment.analyzed? + + width = storage_attachment.metadata[:width] + height = storage_attachment.metadata[:height] min_width = Setting["uploads.images.min_width"].to_i min_height = Setting["uploads.images.min_height"].to_i - errors.add(:attachment, :min_image_width, required_min_width: min_width) if dimensions.width < min_width - errors.add(:attachment, :min_image_height, required_min_height: min_height) if dimensions.height < min_height + errors.add(:attachment, :min_image_width, required_min_width: min_width) if width < min_width + errors.add(:attachment, :min_image_height, required_min_height: min_height) if height < min_height end end @@ -93,8 +96,4 @@ class Image < ApplicationRecord end end end - - def attachment_of_valid_content_type? - attachment.present? && accepted_content_types.include?(attachment_content_type) - end end diff --git a/db/dev_seeds/widgets.rb b/db/dev_seeds/widgets.rb index b672d28d5..41711c937 100644 --- a/db/dev_seeds/widgets.rb +++ b/db/dev_seeds/widgets.rb @@ -1,7 +1,7 @@ section "Creating header and cards for the homepage" do def create_image_attachment(type) { - cached_attachment: Rails.root.join("db/dev_seeds/images/#{type}_background.jpg"), + attachment: File.new(Rails.root.join("db/dev_seeds/images/#{type}_background.jpg")), title: "#{type}_background.jpg", user: User.first } diff --git a/lib/tasks/files.rake b/lib/tasks/files.rake index ef46097cc..978b5f4f1 100644 --- a/lib/tasks/files.rake +++ b/lib/tasks/files.rake @@ -1,18 +1,8 @@ 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 + ActiveStorage::Blob.unattached + .where("active_storage_blobs.created_at <= ?", 1.day.ago) + .find_each(&:purge_later) end end diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index f554d05de..2a8043556 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -11,9 +11,10 @@ describe "active storage tasks" do before { FileUtils.rm_rf storage_root } it "migrates records and attachments" do - document = create(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = build(:document, + attachment: nil, + paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document.save(validate: false) expect(ActiveStorage::Attachment.count).to eq 0 expect(ActiveStorage::Blob.count).to eq 0 @@ -30,9 +31,10 @@ describe "active storage tasks" do end it "migrates records with deleted files ignoring the files" do - document = create(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = build(:document, + attachment: nil, + paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document.save(validate: false) FileUtils.rm(document.attachment.path) run_rake_task diff --git a/spec/lib/tasks/files_spec.rb b/spec/lib/tasks/files_spec.rb index 2c464bc82..454abe7be 100644 --- a/spec/lib/tasks/files_spec.rb +++ b/spec/lib/tasks/files_spec.rb @@ -11,26 +11,26 @@ describe "files tasks" do image = build(:image) document = build(:document) - image.attachment.save - document.attachment.save + image.storage_attachment.blob.save! + document.storage_attachment.blob.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 + expect(File.exists?(image.file_path)).to be false + expect(File.exists?(document.file_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 + image.storage_attachment.blob.save! + document.storage_attachment.blob.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 + expect(File.exists?(image.file_path)).to be true + expect(File.exists?(document.file_path)).to be true end it "does not delete old regular attachments" do @@ -39,8 +39,8 @@ describe "files tasks" do 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 + expect(File.exists?(image.file_path)).to be true + expect(File.exists?(document.file_path)).to be true end end end diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index 9c5657d55..7f281cc22 100644 --- a/spec/models/direct_upload_spec.rb +++ b/spec/models/direct_upload_spec.rb @@ -34,13 +34,14 @@ describe DirectUpload do end context "save_attachment" do - it "saves uploaded file" do - proposal_document_direct_upload = build(:direct_upload, :proposal, :documents) + it "saves uploaded file without creating an attachment record" do + direct_upload = build(:direct_upload, :proposal, :documents) - proposal_document_direct_upload.save_attachment + direct_upload.save_attachment - expect(File.exist?(proposal_document_direct_upload.relation.attachment.path)).to eq(true) - expect(proposal_document_direct_upload.relation.attachment.path).to include("cached_attachments") + expect(File.exist?(direct_upload.relation.file_path)).to be true + expect(direct_upload.relation.storage_attachment.blob).to be_persisted + expect(direct_upload.relation.storage_attachment.attachment).not_to be_persisted end end end diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index da0a72b0a..3e5d03f96 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -137,9 +137,15 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + click_link "Add new document" - expect_document_has_cached_attachment(0, ".pdf") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty + + attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") + + expect(page).to have_css(".loading-bar.complete") + expect(cached_attachment_field.value).not_to be_empty end scenario "Should not update document cached_attachment field after invalid file upload" do @@ -151,7 +157,8 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na false ) - expect_document_has_cached_attachment(0, "") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty end scenario "Should show document errors after documentable submit with @@ -350,14 +357,6 @@ def expect_document_has_title(index, title) end end -def expect_document_has_cached_attachment(index, extension) - document = all(".document")[index] - - within document do - expect(find("input[name$='[cached_attachment]']", visible: :hidden).value).to end_with(extension) - end -end - def documentable_fill_new_valid_proposal fill_in_new_proposal_title with: "Proposal title #{rand(9999)}" fill_in "Proposal summary", with: "Proposal summary" diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 57c2329ec..f6fe1b723 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -90,9 +90,15 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + click_link "Add image" - expect_image_has_cached_attachment(".jpg") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty + + attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + + expect(page).to have_css(".loading-bar.complete") + expect(cached_attachment_field.value).not_to be_empty end scenario "Should not update image cached_attachment field after invalid file upload" do @@ -101,7 +107,9 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) - expect_image_has_cached_attachment("") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + + expect(cached_attachment_field.value).to be_empty end scenario "Should show nested image errors after invalid form submit" do @@ -120,6 +128,19 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end + scenario "Render image preview after sending the form with validation errors" do + skip "Question answers behave differently" if imageable.is_a?(Poll::Question::Answer) + do_login_for user + visit send(path, arguments) + + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + within_fieldset("Descriptive image") { fill_in "Title", with: "" } + click_on submit_button + + expect(page).to have_content "can't be blank" + expect(page).to have_css "img[src$='clippy.jpg']" + end + scenario "Should remove nested image after valid file upload and click on remove button" do do_login_for user visit send(path, arguments) @@ -180,6 +201,22 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end + scenario "Different URLs for different images" do + do_login_for user + visit send(path, arguments) + + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + + original_src = find(:fieldset, "Descriptive image").find("img")[:src] + + click_link "Remove image" + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/custom_map.jpg")) + + updated_src = find(:fieldset, "Descriptive image").find("img")[:src] + + expect(updated_src).not_to eq original_src + end + if path.include? "edit" scenario "show persisted image" do create(:image, imageable: imageable) @@ -268,16 +305,6 @@ def expect_image_has_title(title) end end -def expect_image_has_cached_attachment(extension) - within "#nested-image" do - image = find(".image") - - within image do - expect(find("input[name$='[cached_attachment]']", visible: :hidden).value).to end_with(extension) - end - end -end - def show_caption_for?(imageable_factory_name) imageable_factory_name != "budget" end