Skip to content

Fix #50005 transform_job not accepting previewables #50006

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 11, 2023
Merged

Fix #50005 transform_job not accepting previewables #50006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions activestorage/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
* Fix `preprocessed: true` option for named variants of previewable files.

*Nico Wenterodt*

* Allow accepting `service` as a proc as well in `has_one_attached` and `has_many_attached`.

*Yogesh Khater*
Expand Down
2 changes: 1 addition & 1 deletion activestorage/app/jobs/active_storage/transform_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ class ActiveStorage::TransformJob < ActiveStorage::BaseJob
retry_on ActiveStorage::IntegrityError, attempts: 10, wait: :polynomially_longer

def perform(blob, transformations)
blob.variant(transformations).processed
blob.representation(transformations).processed
end
end
11 changes: 11 additions & 0 deletions activestorage/test/jobs/transform_job_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ class ActiveStorage::TransformJobTest < ActiveJob::TestCase
end
end

test "creates variant for previewable file" do
Copy link
Contributor

@chaadow chaadow Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hi @jonathanhefner @nicowenterodt

thanks for your work.
I would like to point out that this test does not test that a variant has been created. it tests that the preview image has been generated

What I mean by that is that each ActiveStorage::Blob has a has_one_attached :preview_image which is another blob

and right now processed only generates this blob, but not the variant that's associated with preview_image

in my production app, I had to do this to preprocess a preview with its variants:

ActiveStorage::Preview.new(blob, transformations).processed.key

Adding the key call is a bit of hack, but it will not only generate the preview_image but also the variant associated with which is, in my opinion, what's expected when we add the preprocessed: true ( the key that's returned is discarded, but it's the newly generated variant's key which i believe won't impact performance)

I will try to create a PR. I'm also considering changing the processed method implementation, but I feel it's a bit complex ( I need to further read on how to check if an image variant has already been processed for a set of transformations)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm creating a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @chaadow, thanks for your feedback.

So when I get it right this still does not really "preprocess" the actual variant you defined as a named variant like this:

has_one_attached :file do |attachable|
    attachable.preview :thumb, resize_to_limit: [400, 600], preprocessed: true
end

So there is no error now within the TransformJob because the preview_image will be preprocessed successfully utilizing the representation method. However the defined variant of the preview itself is not respected and so when you access it like this:

= image_tag my_record.file.representation(:thumb)

... the actual variant of the preview is missing which leads to a new attachment generated on the fly which is not really the purpose of being "preprocssed".

Correct? I think I missed that because I focused too much on the ActiveStorage::InvariableError thrown within the TransformJob itself not checking the actual "preprocssed" characteristic of the defined variant. Oops.

@jonathanhefner Is this worth a new issue? Or shall we reopen #50005 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've created a PR #50026

Copy link
Contributor

@chaadow chaadow Nov 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry i've added my comment without seeing yours

@nicowenterodt that's 100% correct. Since I had to manually pre process big HEIC files on rails 7.0 ( with a custom HEIC previewer, but logic is similar for any previewable blob), I realized that both the preview image and its associated variant were generated on the fly, and it created a bit of an unpleasant/suboptimal user experience.

I also realized that simply calling processed won't generate the variant. which led me to add the key call

I would love your feedback on my PR. thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @chaadow. I will have a look.

@blob = create_file_blob(filename: "report.pdf", content_type: "application/pdf")
transformations = { resize_to_limit: [100, 100] }

assert_changes -> { @blob.reload.preview(transformations).send(:processed?) }, from: false, to: true do
perform_enqueued_jobs do
ActiveStorage::TransformJob.perform_later @blob, transformations
end
end
end

test "creates variant when untracked" do
@was_tracking, ActiveStorage.track_variants = ActiveStorage.track_variants, false
transformations = { resize_to_limit: [100, 100] }
Expand Down