Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Active Storage for Picture and File attachments #2968

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Jul 9, 2024

What is this pull request for?

Use ActiveStorage instead of Dragonfly for picture and file attachments.

Important changes

  • Cropping animated images (gif and webp) does not work with vips. It works with mini_magick though
  • Upfront processing of image variants after upload is not supported in Rails 7.0 (Need Rails 7.1 to re-implement support)
  • Passing additional params to the picture url is not supported anymore

TODO

  • Use ActiveStorage for Alchemy::Attachment as well
  • Remove Dragonfly dependency and related code
  • Remove our custom picture variants and accompany files
  • Make displaying animated GIFs work again
  • Write upgrade task to create ActiveStorage variants from our variants
  • Fix image cropping with mini_magick
  • Add custom pictures controller to handle authentication
  • Make CDN env vars work so that S3 images get delivered by cloudfront.
  • Figure out how to configure alchemy active storage service for different envs

Checklist

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have added tests to cover this change

@tvdeyen tvdeyen changed the title Active storage Use Active Storage for Picture and File attachments Jul 10, 2024
@tvdeyen tvdeyen force-pushed the active-storage branch 2 times, most recently from 0b43fa0 to 2b7123c Compare July 10, 2024 20:13
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 95.79832% with 10 lines in your changes missing coverage. Please review.

Project coverage is 96.44%. Comparing base (88075e2) to head (9587e59).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
app/models/alchemy/picture.rb 88.37% 5 Missing ⚠️
app/models/alchemy/picture/url.rb 88.88% 2 Missing ⚠️
app/controllers/alchemy/attachments_controller.rb 92.85% 1 Missing ⚠️
app/models/alchemy/attachment.rb 96.96% 1 Missing ⚠️
app/models/alchemy/picture/preprocessor.rb 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2968      +/-   ##
==========================================
- Coverage   96.63%   96.44%   -0.19%     
==========================================
  Files         235      224      -11     
  Lines        6324     6242      -82     
==========================================
- Hits         6111     6020      -91     
- Misses        213      222       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvdeyen tvdeyen self-assigned this Jul 22, 2024
@tvdeyen tvdeyen force-pushed the active-storage branch 3 times, most recently from b4a85f8 to 9bedb48 Compare August 10, 2024 09:45
@HatsuMora
Copy link

It would be great to support uploads on the Resources, to use it when Alchemy is being used as admin for other models.

@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 11, 2024

This change is not related to allow attachments in Rails models. You can already do this with established Rails ways. There is nothing special about that in Alchemy. The model is yours. Just use it as normal Rails model.

@@ -9,6 +9,8 @@
require "active_model_serializers"
require "awesome_nested_set"
require "cancan"
require "dragonfly"
require "dragonfly_svg"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this to upgrader

Alchemy::Upgrader.display_todos
end

namespace "7.3" do
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to 8.0

@tvdeyen tvdeyen force-pushed the active-storage branch 2 times, most recently from 59d4563 to 77a78ef Compare September 18, 2024 10:14
Necessary for active storage image transforms
ActiveStorage uses the image_processing gem to process images. This uses a hash to describe the conversion instead of a string
Instead of using the Dragonfly Model validations we write our own. That way we can use another image attachment library more easily
The vips image_processing processor sharpens images by default. This disables this behavior.
Active storage already knows if a file is convertible
This avoids N+1 in the image library
Using activestorage methods to read the values.
Ideally we would cache these values, but active
storage asynchronously sets these values and provides
no callback we could use.
Now that we use ActiveStorage we can remove our custom picture variant handling
This method is the only one left after the PictureVariant and PictureThumb removal.
Active storage has the mime type stored not the extension.
ruby-vips and mini_magick need to be told to ignore
the page in order to resize animated gifs.
Before we where not telling active storage that we want
a certain format explicitely.
We need that for displaying animated PNGs properly.
Proxy downloads the file.
Only works in Rails 7.1
ActiveStorage expects an extension and not an mime type.
It is only configured by default in unreleased Rails 7.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants