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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix linking to invariable image URLs #10342

Merged
merged 4 commits into from Mar 15, 2023

Conversation

ahukkanen
Copy link
Contributor

@ahukkanen ahukkanen commented Feb 6, 2023

馃帺 What? Why?

Not all environments support creating "variable URLs" for all image types. Let's say the admin user would allow uploading image files that cannot be converted using ImageMagick.

In this case, the file conversion fails because of an exception of type ActiveStorage::InvariableError is thrown.

How the user would see this is that they cannot e.g. preview their proposal or see the proposal page because it is trying to create the thumbnail URLs for these images. This PR fixes the issue by falling back to the original URL of that file.

馃搶 Related Issues

Testing

  • Within the development_app's config/environments/development.rb, add the following configuration: config.active_storage.variable_content_types = %w(image/png)
  • Make sure image/jpeg is allowed from the Decidim system panel (it should be if you haven't changed anything)
  • Allow attachments for proposals
  • Try to submit a proposal with a JPEG image or attachment
  • See error (before this patch)

@ahukkanen ahukkanen added module: core type: fix PRs that implement a fix for a bug labels Feb 6, 2023
@andreslucena andreslucena added this to To Review by Core in Maintainers Mar 1, 2023
@alecslupu
Copy link
Contributor

@ahukkanen I do not have enough informations for testing scenario.
I have addded ActiveStorage.variable_content_types = %w(image/png) to decidim initializer, tried to move it to application.rb, yet in both cases, the image ( jpeg files ) were uploaded.

All the changes ere tested for replication on develop brach, using ruby 3.1.2

@alecslupu
Copy link
Contributor

I have tried with the following image magick versions:

Setting up imagemagick (8:6.9.11.60+dfsg-1.3+deb11u1) ...
Setting up imagemagick-6.q16 (8:6.9.11.60+dfsg-1.3+deb11u1) ...

root@58cdd7681fae:/code# identify --version
Version: ImageMagick 6.9.11-60 Q16 x86_64 2021-01-25 https://imagemagick.org
Copyright: (C) 1999-2021 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC Modules OpenMP(4.5) 
Delegates (built-in): bzlib djvu fftw fontconfig freetype heic jbig jng jp2 jpeg lcms lqr ltdl lzma openexr pangocairo png tiff webp wmf x xml zlib

@alecslupu
Copy link
Contributor

i have upgraded to

Version: ImageMagick 7.1.0-4 Q16 x86_64 2021-07-18 https://imagemagick.org
Copyright: (C) 1999-2021 ImageMagick Studio LLC
License: https://imagemagick.org/script/license.php
Features: Cipher DPC HDRI Modules OpenMP(4.5) 
Delegates (built-in): bzlib djvu fontconfig freetype gslib heic jbig jng jp2 jpeg lcms lqr ltdl lzma openexr pangocairo png ps raqm raw tiff webp wmf x xml zip zlib

Yet, i could not replicate the bug.
My system admin panel looks like:
image

@andreslucena andreslucena added the release: v0.27 Issues or PRs that need to be tackled for v0.27 label Mar 15, 2023
@ahukkanen
Copy link
Contributor Author

@alecslupu Sorry, I have mistakenly written the configuration instructions for this. I have just corrected it. You need to set the variable_content_types configuration through the environment configuration file, not through an initializer.

Rails will actually override the initializer configuration with the value set in the environment configuration.

Here is a screencap how I can replicate this issue at develop:

decidim-invariable-error.mp4

And when running under this branch I don't get that error anymore.

Copy link
Contributor

@alecslupu alecslupu left a comment

Choose a reason for hiding this comment

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

I still could not replicate the upload problem, yet, i could see the ActiveStorage::InvariableError on the homepage using latest develop branch. It dissapeared one the patch was applied.

@alecslupu alecslupu merged commit 3fa1e01 into decidim:develop Mar 15, 2023
Maintainers automation moved this from To Review by Core to Done Mar 15, 2023
@ahukkanen ahukkanen deleted the fix/invariable-variant-urls branch March 16, 2023 07:21
@sdelcroix
Copy link
Contributor

Any chance to see this backported to v0.26 ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: core release: v0.27 Issues or PRs that need to be tackled for v0.27 type: fix PRs that implement a fix for a bug
Projects
Development

Successfully merging this pull request may close these issues.

Invariable attachments cause the proposal page to break
4 participants