Use Active Storage to handle cached attachments

This fixes a few issues we've had for years.

First, when attaching an image and then sending a form with validation
errors, the image preview would not be rendered when the form was
displayed once again. Now it's rendered as expected.

Second, when attaching an image, removing it, and attaching a new
one, browsers were displaying the image preview of the first one. That's
because Paperclip generated the same URL from both files (as they both
had the same hash data and prefix). Browsers usually cache images and
render the cached image when getting the same URL.

Since now we're storing each image in a different Blob, the images have
different URLs and so the preview of the second one is correctly
displayed.

Finally, when users downloaded a document, they were getting files with
a very long hexadecimal hash as filename. Now they get the original
filename.

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

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_content_type,         if: -> { storage_attachment.attached? }
-    validate :validate_attachment_size,                 if: -> { attachment.present? }
+    validate :validate_attachment_size,                 if: -> { storage_attachment.attached? }
 
-    before_save :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? }
+    before_validation :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? }
-
-    Paperclip.interpolates :prefix do |attachment, style|
-      attachment.instance.prefix(attachment, style)
-    end
   end
 
   def association_class
@@ -26,27 +22,35 @@ module Attachable
   end
 
   def set_cached_attachment_from_attachment
-    self.cached_attachment = if filesystem_storage?
+    self.cached_attachment = storage_attachment.signed_id
-                               attachment.path
-                             else
-                               attachment.url
-                             end
   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)
+  def attachment_content_type
-    if attachment.instance.persisted?
+    storage_attachment.blob.content_type if storage_attachment.attached?
-      ":attachment/:id_partition"
-    else
-      "cached_attachments/user/#{attachment.instance.user_id}"
   end
+
+  def attachment_file_size
+    if storage_attachment.attached?
+      storage_attachment.blob.byte_size
+    else
+      0
+    end
+  end
+
+  def file_path
+    ActiveStorage::Blob.service.path_for(storage_attachment.blob.key)
   end
 
   private
@@ -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

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
+      define_method :"storage_#{attribute}=" do |file|
-      alias_method :"paperclip_#{attribute}=", :"#{attribute}="
+        if file.is_a?(IO)
-
-      define_method :"#{attribute}=" do |file|
-        if file.is_a?(IO) || file.is_a?(Tempfile) && !file.is_a?(Ckeditor::Http::QqFile)
           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

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
       }

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

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_width, required_min_width: min_width) if width < min_width
-        errors.add(:attachment, :min_image_height, required_min_height: min_height) if dimensions.height < min_height
+        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

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
     }

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/**/*"))
+    ActiveStorage::Blob.unattached
-    logger = ApplicationLogger.new
+                       .where("active_storage_blobs.created_at <= ?", 1.day.ago)
-
+                       .find_each(&:purge_later)
-    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/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,
+      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,
+      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

spec/lib/tasks/files_spec.rb

@@ -11,26 +11,26 @@ describe "files tasks" do
       image = build(:image)
       document = build(:document)
 
-      image.attachment.save
+      image.storage_attachment.blob.save!
-      document.attachment.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?(image.file_path)).to be false
-      expect(File.exists?(document.attachment.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
+      image.storage_attachment.blob.save!
-      document.attachment.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?(image.file_path)).to be true
-      expect(File.exists?(document.attachment.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?(image.file_path)).to be true
-      expect(File.exists?(document.attachment.path)).to be true
+      expect(File.exists?(document.file_path)).to be true
     end
   end
 end

spec/models/direct_upload_spec.rb

@@ -34,13 +34,14 @@ describe DirectUpload do
   end
 
   context "save_attachment" do
-    it "saves uploaded file" do
+    it "saves uploaded file without creating an attachment record" do
-      proposal_document_direct_upload = build(:direct_upload, :proposal, :documents)
+      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(File.exist?(direct_upload.relation.file_path)).to be true
-      expect(proposal_document_direct_upload.relation.attachment.path).to include("cached_attachments")
+      expect(direct_upload.relation.storage_attachment.blob).to be_persisted
+      expect(direct_upload.relation.storage_attachment.attachment).not_to be_persisted
     end
   end
 end

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"

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