From 1290e2ecd36470517ee72f80d37a0531ca99de04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Jul 2021 01:21:00 +0200 Subject: [PATCH] Store files with both Paperclip and ActiveStorage In order to migrate existing files from Paperclip to ActiveStorage, we need Paperclip to find out the files associated to existing database records. So we can't simply replace Paperclip with ActiveStorage. That's why it's usually recommended [1] to first run the migration and then replace Paperclip with ActiveStorage using two consecutive deployments. However, in our case we can't rely on two consecutive deployments because we have to make an easy process so existing CONSUL installations don't run into any issues. We can't just release version 1.4.0 and 1.5.0 and day and ask everyone to upgrade twice on the same day. Instead, we're following a different plan: * We're going to provide a Rake task (which will require Paperclip) to migrate existing files * We still use Paperclip to generate link and image tags * New files are handled using both Paperclip and ActiveStorage; that way, when we make the switch, we won't have to migrate them, and in the meantime they'll be accessible thanks to Paperclip * After we make the switch, we'll update the `name` column in the active storage attachments tables in order to remove the `storage_` prefix Regarding our handling of new files, the exception are cached attachments. Since those attachments are temporary files used while submitting a form and we have to delete them afterwards, we're only handling them with Paperclip. We'll handle these ones in version 1.5.0. Note the task creating the dev seeds was failing after these changes with an `ActiveStorage::IntegrityError` exception because we were opening some files without closing them. If the same file was attached twice, it failed the second time. We're solving it by closing the files with `File.open` and a block. Even though we didn't get any errors, we're doing the same thing in the `Attachable` concern because it's a good practice to close files after we're done with them. Also note we have to change the CKEditor Active Storage code so it's compatible with Paperclip. In this case, I haven't been able to write a test to confirm the attachment exists; I was getting the same `ActiveStorage::IntegrityError` mentioned above. Finally, we're updating the site customization image controller to use `update` so the image and the attachment are updated within the same transaction. This is also what we do in most controllers. [1] https://www.youtube.com/watch?v=tZ_WNUytO9o --- .../site_customization/images_controller.rb | 3 +-- app/models/ckeditor/asset.rb | 1 + app/models/ckeditor/picture.rb | 10 ++++---- app/models/concerns/attachable.rb | 11 +++++---- app/models/concerns/has_attachment.rb | 24 +++++++++++++++++++ app/models/direct_upload.rb | 2 +- app/models/document.rb | 8 +++---- app/models/image.rb | 18 +++++++------- app/models/site_customization/image.rb | 4 +++- db/dev_seeds/budgets.rb | 24 ++++++++++--------- db/dev_seeds/proposals.rb | 22 +++++++++-------- lib/ckeditor/backend/active_storage.rb | 20 ++++++++++------ spec/models/document_spec.rb | 10 ++++++++ spec/models/image_spec.rb | 10 ++++++++ spec/models/site_customization/image_spec.rb | 11 +++++++++ 15 files changed, 124 insertions(+), 54 deletions(-) create mode 100644 app/models/concerns/has_attachment.rb diff --git a/app/controllers/admin/site_customization/images_controller.rb b/app/controllers/admin/site_customization/images_controller.rb index 3280c690d..da0a53412 100644 --- a/app/controllers/admin/site_customization/images_controller.rb +++ b/app/controllers/admin/site_customization/images_controller.rb @@ -26,8 +26,7 @@ class Admin::SiteCustomization::ImagesController < Admin::SiteCustomization::Bas end def destroy - @image.image = nil - if @image.save + if @image.update(image: nil) notice = t("admin.site_customization.images.destroy.notice") else notice = t("admin.site_customization.images.destroy.error") diff --git a/app/models/ckeditor/asset.rb b/app/models/ckeditor/asset.rb index 5c166fdfc..494d4fc76 100644 --- a/app/models/ckeditor/asset.rb +++ b/app/models/ckeditor/asset.rb @@ -1,4 +1,5 @@ class Ckeditor::Asset < ApplicationRecord include Ckeditor::Orm::ActiveRecord::AssetBase + include Ckeditor::Backend::ActiveStorage include Ckeditor::Backend::Paperclip end diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index 4a579b9d2..bcce7ee99 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -1,8 +1,10 @@ class Ckeditor::Picture < Ckeditor::Asset - has_attached_file :data, - url: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", - path: ":rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension", - styles: { content: "800>", thumb: "118x100#" } + include HasAttachment + + has_attachment :data, + url: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", + path: ":rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension", + styles: { content: "800>", thumb: "118x100#" } validates_attachment_presence :data validates_attachment_size :data, less_than: 2.megabytes diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index fbc29a055..fb13dc517 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -1,4 +1,5 @@ module Attachable + include HasAttachment extend ActiveSupport::Concern included do @@ -33,11 +34,11 @@ module Attachable end def set_attachment_from_cached_attachment - self.attachment = if Paperclip::Attachment.default_options[:storage] == :filesystem - File.open(cached_attachment) - else - URI.parse(cached_attachment) - end + if Paperclip::Attachment.default_options[:storage] == :filesystem + File.open(cached_attachment) { |file| self.attachment = file } + else + self.attachment = URI.parse(cached_attachment) + end end def prefix(attachment, _style) diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb new file mode 100644 index 000000000..450e515ea --- /dev/null +++ b/app/models/concerns/has_attachment.rb @@ -0,0 +1,24 @@ +module HasAttachment + extend ActiveSupport::Concern + + class_methods do + 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) + 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 + + send(:"paperclip_#{attribute}=", file) + end + end + end +end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 2b2760011..7e66375e7 100644 --- a/app/models/direct_upload.rb +++ b/app/models/direct_upload.rb @@ -53,7 +53,7 @@ class DirectUpload def relation_attributtes { - attachment: @attachment, + paperclip_attachment: @attachment, cached_attachment: @cached_attachment, user: @user } diff --git a/app/models/document.rb b/app/models/document.rb index 49ad4be9c..a121e3bbd 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -1,10 +1,10 @@ class Document < ApplicationRecord include Attachable - has_attached_file :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", - hash_data: ":class/:style/:custom_hash_data", - use_timestamp: false, - hash_secret: Rails.application.secrets.secret_key_base + has_attachment :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", + hash_data: ":class/:style/:custom_hash_data", + use_timestamp: false, + hash_secret: Rails.application.secrets.secret_key_base belongs_to :user belongs_to :documentable, polymorphic: true, touch: true diff --git a/app/models/image.rb b/app/models/image.rb index 0449042d8..de1405f2b 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -1,15 +1,15 @@ class Image < ApplicationRecord include Attachable - has_attached_file :attachment, styles: { - large: "x#{Setting["uploads.images.min_height"]}", - medium: "300x300#", - thumb: "140x245#" - }, - url: "/system/:class/:prefix/:style/:hash.:extension", - hash_data: ":class/:style", - use_timestamp: false, - hash_secret: Rails.application.secrets.secret_key_base + has_attachment :attachment, styles: { + large: "x#{Setting["uploads.images.min_height"]}", + medium: "300x300#", + thumb: "140x245#" + }, + url: "/system/:class/:prefix/:style/:hash.:extension", + hash_data: ":class/:style", + use_timestamp: false, + hash_secret: Rails.application.secrets.secret_key_base belongs_to :user belongs_to :imageable, polymorphic: true, touch: true diff --git a/app/models/site_customization/image.rb b/app/models/site_customization/image.rb index fc6bcfd71..592ea3b47 100644 --- a/app/models/site_customization/image.rb +++ b/app/models/site_customization/image.rb @@ -1,4 +1,6 @@ class SiteCustomization::Image < ApplicationRecord + include HasAttachment + VALID_IMAGES = { "logo_header" => [260, 80], "social_media_icon" => [470, 246], @@ -9,7 +11,7 @@ class SiteCustomization::Image < ApplicationRecord "logo_email" => [400, 80] }.freeze - has_attached_file :image + has_attachment :image validates :name, presence: true, uniqueness: true, inclusion: { in: VALID_IMAGES.keys } validates_attachment_content_type :image, content_type: ["image/png", "image/jpeg"] diff --git a/db/dev_seeds/budgets.rb b/db/dev_seeds/budgets.rb index 42da7c03c..dcb18be57 100644 --- a/db/dev_seeds/budgets.rb +++ b/db/dev_seeds/budgets.rb @@ -6,21 +6,23 @@ INVESTMENT_IMAGE_FILES = %w[ olesya-grichina-218176-unsplash_713x475.jpg sole-d-alessandro-340443-unsplash_713x475.jpg ].map do |filename| - File.new(Rails.root.join("db", - "dev_seeds", - "images", - "budget", - "investments", filename)) + Rails.root.join("db", + "dev_seeds", + "images", + "budget", + "investments", filename) end def add_image_to(imageable) # imageable should respond to #title & #author - imageable.image = Image.create!({ - imageable: imageable, - title: imageable.title, - attachment: INVESTMENT_IMAGE_FILES.sample, - user: imageable.author - }) + File.open(INVESTMENT_IMAGE_FILES.sample) do |file| + imageable.image = Image.create!({ + imageable: imageable, + title: imageable.title, + attachment: file, + user: imageable.author + }) + end imageable.save! end diff --git a/db/dev_seeds/proposals.rb b/db/dev_seeds/proposals.rb index 8341fd58b..756dd00b3 100644 --- a/db/dev_seeds/proposals.rb +++ b/db/dev_seeds/proposals.rb @@ -4,20 +4,22 @@ IMAGE_FILES = %w[ steve-harvey-597760-unsplash_713x475.jpg tim-mossholder-302931-unsplash_713x475.jpg ].map do |filename| - File.new(Rails.root.join("db", - "dev_seeds", - "images", - "proposals", filename)) + Rails.root.join("db", + "dev_seeds", + "images", + "proposals", filename) end def add_image_to(imageable) # imageable should respond to #title & #author - imageable.image = Image.create!({ - imageable: imageable, - title: imageable.title, - attachment: IMAGE_FILES.sample, - user: imageable.author - }) + File.open(IMAGE_FILES.sample) do |file| + imageable.image = Image.create!({ + imageable: imageable, + title: imageable.title, + attachment: file, + user: imageable.author + }) + end imageable.save! end diff --git a/lib/ckeditor/backend/active_storage.rb b/lib/ckeditor/backend/active_storage.rb index 710db0aed..7ce8aa611 100644 --- a/lib/ckeditor/backend/active_storage.rb +++ b/lib/ckeditor/backend/active_storage.rb @@ -16,7 +16,7 @@ module Ckeditor base.class_eval do before_save :apply_data validate do - if data.nil? || file.nil? + if data.nil? || storage_file.nil? errors.add(:data, :not_data_present, message: "data must be present") end end @@ -46,19 +46,25 @@ module Ckeditor protected - def file - @file ||= storage_data + def storage_file + @storage_file ||= storage_data end def blob - @blob ||= ::ActiveStorage::Blob.find(file.attachment.blob_id) + @blob ||= ::ActiveStorage::Blob.find(storage_file.attachment.blob_id) end def apply_data - if data.is_a?(Ckeditor::Http::QqFile) - storage_data.attach(io: data, filename: data.original_filename) + non_paperclip_data = if data.is_a?(::Paperclip::Attachment) + file.instance_variable_get("@target") + else + data + end + + if non_paperclip_data.is_a?(Ckeditor::Http::QqFile) + storage_data.attach(io: non_paperclip_data, filename: non_paperclip_data.original_filename) else - storage_data.attach(data) + storage_data.attach(non_paperclip_data) end self.data_file_name = storage_data.blob.filename diff --git a/spec/models/document_spec.rb b/spec/models/document_spec.rb index 59a27e036..2f8cf65fe 100644 --- a/spec/models/document_spec.rb +++ b/spec/models/document_spec.rb @@ -4,6 +4,16 @@ describe Document do it_behaves_like "document validations", "budget_investment_document" it_behaves_like "document validations", "proposal_document" + it "stores attachments with both Paperclip and Active Storage" do + document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) + + expect(document.attachment).to exist + expect(document.attachment_file_name).to eq "clippy.pdf" + + expect(document.storage_attachment).to be_attached + expect(document.storage_attachment.filename).to eq "clippy.pdf" + end + context "scopes" do describe "#admin" do it "returns admin documents" do diff --git a/spec/models/image_spec.rb b/spec/models/image_spec.rb index 5f1725521..d119c8138 100644 --- a/spec/models/image_spec.rb +++ b/spec/models/image_spec.rb @@ -3,4 +3,14 @@ require "rails_helper" describe Image do it_behaves_like "image validations", "budget_investment_image" it_behaves_like "image validations", "proposal_image" + + it "stores attachments with both Paperclip and Active Storage" do + image = create(:image, attachment: File.new("spec/fixtures/files/clippy.jpg")) + + expect(image.attachment).to exist + expect(image.attachment_file_name).to eq "clippy.jpg" + + expect(image.storage_attachment).to be_attached + expect(image.storage_attachment.filename).to eq "clippy.jpg" + end end diff --git a/spec/models/site_customization/image_spec.rb b/spec/models/site_customization/image_spec.rb index 5c755dbfe..ddd266d0e 100644 --- a/spec/models/site_customization/image_spec.rb +++ b/spec/models/site_customization/image_spec.rb @@ -1,6 +1,17 @@ require "rails_helper" describe SiteCustomization::Image do + it "stores images with both Paperclip and Active Storage" do + image = create(:site_customization_image, name: "map", + image: File.new("spec/fixtures/files/custom_map.jpg")) + + expect(image.image).to exist + expect(image.image_file_name).to eq "custom_map.jpg" + + expect(image.storage_image).to be_attached + expect(image.storage_image.filename).to eq "custom_map.jpg" + end + describe "logo" do it "is valid with a 260x80 image" do image = build(:site_customization_image,