Remove Paperclip and use just Active Storage #4600

javierm · 2021-07-28T01:02:04Z

References

⚠️ Before merging, check whether Allow Procs as values for validator restrictions igorkasyanchuk/active_storage_validations#105 is closed (there's an open pull request for it), as we might use active_storage_validations instead of file_validators
Closes Move to ActiveStorage #2629
Closes Integrate Ckeditor with Amazon S3 uploads #2904

Objectives

Fix a bug which caused document links to be broken if the document title was edited
Fix image preview not being rendered after submitting a form with validation errors
Fix wrong image preview when attaching an image, removing it, and attaching a different one
Improve file names when downloading documents
Make it possible to upgrade to Ruby 3 (Paperclip isn't compatible with Ruby 3)
Make it possible to switch from CKEditor to Action Text in the future

Release Notes

TODO Explain we've removed Paperclip but there might be references to paperclip attachments in any existing content (for example, custom pages) and so Paperclip attachments shouldn't be removed (or removed with care).

TODO Strongly emphasize that after upgrading it'll be necessary to execute the release tasks; othewise there'll be 500 errors on every page displaying an attached image or document.

app/models/ckeditor/picture.rb

app/models/image.rb

lib/tasks/active_storage.rake

app/models/site_customization/image.rb

spec/lib/tasks/active_storage_spec.rb

Senen

It looks good to me!

I did the following checks for testing these changes on a server running the production environment:

Reproduce all the errors mentioned in the objectives section in the current master branch
Deploy the branch active_storage
Run the rake task to migrate attachments from paperclip to active_storage
Check, one by one, all the errors mentioned in the objectives section are solved.
Check the file_validators validations by uploading wrong sized and too heavy images and documents
Check the ActiveStorage integration with AWS S3 works fine (including the Ckeditor specific integration)

javierm · 2022-02-23T17:12:00Z

For now, we'll merge it with the file_validators dependency 👌. We'll consider switching to active_storage_validations when they add support to procs in validations.

This way we fix a bug we mentioned in commit 930bb75 which caused links to documents to be broken when editing their title because the title was used to generate the URL of the document. Note we're still using Paperclip to render cached attachments because this is the only case where we store files with just Paperclip and not Active Storage. With Active Storage, we render attachments just like any other resource, using `polymorphic_path`. Paperclip included the `url` method in the model; since the model doesn't have access to the request parameters (like the host), this was inconvenient because it wasn't possible to generate absolute URLs with Paperclip. In order to simplify the code and make it similar to the way we used Paperclip, we're adding a `variant` method accepting the name of a variant and returning the variant.

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.

Just like we did with regular attachments, we're moving the logic to generate URLs out of the model. Note we're changing the `image_path_for` helper method in order to return a `polymorphic_path` because sometimes it's used in combination with `favicon_link_tag`, and `favicon_link_tag` doesn't automatically generate a polymorphic URL when given an `ActiveStorage::Attachment` record.

The same way we're handling images.

Since we're going to remove Paperclip and Active Storage doesn't provide any validations, we have to either write our own validation rules or use a different gem. We're using the file_validators gem instead of the `active_storage_validations` gem because the latter doesn't support proc/lambda objects in size and content type definitions. We need to use them because in our case these values depend on settings stored in the database.

The code is based on what's generated using CKEditor's code generator. We're doing one minor change to the `Ckeditor::Backend::ActiveStorage` module; we're assigning the data in a `before_validation` instead of a `before_save` callback. Validations with `file_validations` didn't work otherwise; it looks like this backend was written with `active_storage_validations` in mind [1]. Note we don't need to update the `name` column in the attachments table because, when using Active Storage, CKEditor uses both `data` (as attribute accessor) and `storage_data` (as attachment attribute). [1] https://github.com/galetahub/ckeditor/blob/f9e48420ccb6dc/lib/generators/ckeditor/templates/active_record/active_storage/ckeditor/picture.rb#L4

We were getting an error when browsing the server if one file had been deleted.

We were using custom rules because of some issues with Paperclip. These rules work fine, but since we're already using the file_validators gem, we might as well simplify the code a little bit.

This way we don't have to write `"spec/fixtures/files"` every time. Note this method isn't included in factories. We could include it like so: ``` FactoryBot::SyntaxRunner.class_eval do include ActiveSupport::Testing::FileFixtures self.file_fixture_path = RSpec.configuration.file_fixture_path end ``` However, I'm not sure about the possible side effects, and since we only use attachments in a few factories, there isn't much gain in applying the monkey-patch.

We were using this hack in order to allow `File.new` attachments in tests files. However, we can use the `fixture_file_upload` helper instead. Just like it happened with `file_fixture`, this helper method doesn't work in fixtures, so in this case we're using `Rack::Test::UploadedFile` instead.

javierm added the Maintenance label Jul 28, 2021

javierm self-assigned this Jul 28, 2021

javierm added this to Reviewing in Consul Democracy via automation Jul 28, 2021