Use Active Storage to render attachments

This way we fix a bug we mentioned in commit 930bb753c 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 commit is contained in:
Javi Martín
2021-07-27 20:48:33 +02:00
parent 074934c154
commit 091abfc944
39 changed files with 88 additions and 55 deletions

View File

@@ -33,6 +33,7 @@ gem "jquery-fileupload-rails"
gem "jquery-rails", "~> 4.4.0" gem "jquery-rails", "~> 4.4.0"
gem "jquery-ui-rails", "~> 6.0.1" gem "jquery-ui-rails", "~> 6.0.1"
gem "kaminari", "~> 1.2.1" gem "kaminari", "~> 1.2.1"
gem "mini_magick", "~> 4.11.0"
gem "omniauth", "~> 2.0.4" gem "omniauth", "~> 2.0.4"
gem "omniauth-facebook", "~> 8.0.0" gem "omniauth-facebook", "~> 8.0.0"
gem "omniauth-google-oauth2", "~> 1.0.0" gem "omniauth-google-oauth2", "~> 1.0.0"

View File

@@ -373,6 +373,7 @@ GEM
mimemagic (0.3.10) mimemagic (0.3.10)
nokogiri (~> 1) nokogiri (~> 1)
rake rake
mini_magick (4.11.0)
mini_mime (1.1.0) mini_mime (1.1.0)
mini_portile2 (2.6.1) mini_portile2 (2.6.1)
minitest (5.14.4) minitest (5.14.4)
@@ -742,6 +743,7 @@ DEPENDENCIES
launchy (~> 2.5.0) launchy (~> 2.5.0)
letter_opener_web (~> 1.4.0) letter_opener_web (~> 1.4.0)
mdl (~> 0.11.0) mdl (~> 0.11.0)
mini_magick (~> 4.11.0)
omniauth (~> 2.0.4) omniauth (~> 2.0.4)
omniauth-facebook (~> 8.0.0) omniauth-facebook (~> 8.0.0)
omniauth-google-oauth2 (~> 1.0.0) omniauth-google-oauth2 (~> 1.0.0)

View File

@@ -12,7 +12,7 @@
<!-- remove conditional once specs have image validations --> <!-- remove conditional once specs have image validations -->
<td> <td>
<% if card.image.present? %> <% if card.image.present? %>
<%= link_to t("admin.shared.show_image"), card.image_url(:large), <%= link_to t("admin.shared.show_image"), card.image.variant(:large),
title: card.image.title, target: "_blank" %> title: card.image.title, target: "_blank" %>
<% end %> <% end %>
</td> </td>

View File

@@ -7,7 +7,7 @@
<%= f.text_field :title, placeholder: t("#{plural_name}.form.title_placeholder") %> <%= f.text_field :title, placeholder: t("#{plural_name}.form.title_placeholder") %>
</div> </div>
<% if attachable.attachment.exists? && attachable.attachment.styles.keys.include?(:thumb) %> <% if attachable.storage_attachment.attached? && attachable.storage_attachment.image? %>
<%= render_image(attachable, :thumb, false) %> <%= render_image(attachable, :thumb, false) %>
<% end %> <% end %>

View File

@@ -24,7 +24,7 @@ class Attachable::FieldsComponent < ApplicationComponent
end end
def file_name def file_name
attachable.attachment_file_name attachable.storage_attachment.filename if attachable.storage_attachment.attached?
end end
def destroy_link def destroy_link

View File

@@ -1,6 +1,6 @@
<% if budget.image.present? %> <% if budget.image.present? %>
<div class="budget-header with-background-image" <div class="budget-header with-background-image"
style="background-image: url(<%= asset_url budget.image.attachment.url(:large) %>);"> style="background-image: url(<%= polymorphic_path(budget.image.variant(:large)) %>);">
<% else %> <% else %>
<div class="budget-header"> <div class="budget-header">
<% end %> <% end %>

View File

@@ -1,6 +1,6 @@
<div id="<%= dom_id(investment) %>" class="investments-list-item"> <div id="<%= dom_id(investment) %>" class="investments-list-item">
<% if investment.image.present? %> <% if investment.image.present? %>
<%= image_tag investment.image_url(:large), alt: investment.image.title.unicode_normalize %> <%= image_tag investment.image.variant(:large), alt: investment.image.title.unicode_normalize %>
<% else %> <% else %>
<%= image_tag "budget_investment_no_image.jpg", alt: investment.title %> <%= image_tag "budget_investment_no_image.jpg", alt: investment.title %>
<% end %> <% end %>

View File

@@ -55,7 +55,7 @@
<% if phase.image.present? %> <% if phase.image.present? %>
<div class="budget-phase-image"> <div class="budget-phase-image">
<%= image_tag phase.image.attachment.url(:large), alt: "" %> <%= image_tag phase.image.variant(:large), alt: "" %>
</div> </div>
<% end %> <% end %>
</div> </div>

View File

@@ -9,7 +9,7 @@
<% if proposal.image.present? %> <% if proposal.image.present? %>
<div class="small-12 medium-6 large-3 column"> <div class="small-12 medium-6 large-3 column">
<div class="feed-image"> <div class="feed-image">
<%= image_tag proposal.image_url(:thumb), <%= image_tag proposal.image.variant(:thumb),
alt: proposal.image.title.unicode_normalize %> alt: proposal.image.title.unicode_normalize %>
</div> </div>
</div> </div>

View File

@@ -3,7 +3,7 @@ module DocumentsHelper
info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}" info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}"
link_to safe_join([document.title, tag.small("(#{info_text})")], " "), link_to safe_join([document.title, tag.small("(#{info_text})")], " "),
document.attachment.url, document.storage_attachment,
target: "_blank", target: "_blank",
title: t("shared.target_blank") title: t("shared.target_blank")
end end

View File

@@ -2,11 +2,7 @@ module ImagesHelper
def image_absolute_url(image, version) def image_absolute_url(image, version)
return "" unless image return "" unless image
if Paperclip::Attachment.default_options[:storage] == :filesystem polymorphic_url(image.variant(version))
URI(request.url) + image.attachment.url(version)
else
image.attachment.url(version)
end
end end
def image_class(image) def image_class(image)
@@ -14,7 +10,8 @@ module ImagesHelper
end end
def render_image(image, version, show_caption = true) def render_image(image, version, show_caption = true)
version = image.persisted? ? version : :original render "images/image", image: image,
render "images/image", image: image, version: version, show_caption: show_caption version: (version if image.persisted?),
show_caption: show_caption
end end
end end

View File

@@ -24,8 +24,9 @@ module WelcomeHelper
end end
def calculate_image_path(recommended, image_default) def calculate_image_path(recommended, image_default)
if recommended.respond_to?(:image) && recommended.image.present? && recommended.image.attachment.exists? if recommended.respond_to?(:image) && recommended.image.present? &&
recommended.image.attachment.send("url", :medium) recommended.image.storage_attachment.attached?
recommended.image.variant(:medium)
elsif image_default.present? elsif image_default.present?
image_default image_default
end end

View File

@@ -4,9 +4,5 @@ module Imageable
included do included do
has_one :image, as: :imageable, inverse_of: :imageable, dependent: :destroy has_one :image, as: :imageable, inverse_of: :imageable, dependent: :destroy
accepts_nested_attributes_for :image, allow_destroy: true, update_only: true accepts_nested_attributes_for :image, allow_destroy: true, update_only: true
def image_url(style)
image&.attachment&.url(style) || ""
end
end end
end end

View File

@@ -11,6 +11,14 @@ class Image < ApplicationRecord
use_timestamp: false, use_timestamp: false,
hash_secret: Rails.application.secrets.secret_key_base hash_secret: Rails.application.secrets.secret_key_base
def self.styles
{
large: { resize: "x#{Setting["uploads.images.min_height"]}" },
medium: { combine_options: { gravity: "center", resize: "300x300^", crop: "300x300+0+0" }},
thumb: { combine_options: { gravity: "center", resize: "140x245^", crop: "140x245+0+0" }}
}
end
belongs_to :user belongs_to :user
belongs_to :imageable, polymorphic: true, touch: true belongs_to :imageable, polymorphic: true, touch: true
@@ -41,6 +49,14 @@ class Image < ApplicationRecord
self.class.accepted_content_types self.class.accepted_content_types
end end
def variant(style)
if style
storage_attachment.variant(self.class.styles[style])
else
storage_attachment
end
end
private private
def association_name def association_name

View File

@@ -40,14 +40,14 @@
</td> </td>
<td class="small"> <td class="small">
<%= link_to t("admin.milestones.index.show_image"), <%= link_to t("admin.milestones.index.show_image"),
milestone.image_url(:large), milestone.image.variant(:large),
target: :_blank if milestone.image.present? %> target: :_blank if milestone.image.present? %>
</td> </td>
<td class="small"> <td class="small">
<% if milestone.documents.present? %> <% if milestone.documents.present? %>
<% milestone.documents.each do |document| %> <% milestone.documents.each do |document| %>
<%= link_to document.title, <%= link_to document.title,
document.attachment.url, document.storage_attachment,
target: "_blank", target: "_blank",
rel: "nofollow" %><br> rel: "nofollow" %><br>
<% end %> <% end %>

View File

@@ -33,7 +33,7 @@
<% @answer.documents.each do |document| %> <% @answer.documents.each do |document| %>
<tr> <tr>
<td> <td>
<%= link_to document.title, document.attachment.url %> <%= link_to document.title, document.storage_attachment %>
</td> </td>
<td> <td>
<%= render Admin::TableActionsComponent.new(document, <%= render Admin::TableActionsComponent.new(document,
@@ -42,7 +42,7 @@
) do |actions| %> ) do |actions| %>
<%= actions.action(:download, <%= actions.action(:download,
text: t("documents.buttons.download_document"), text: t("documents.buttons.download_document"),
path: document.attachment.url, path: document.storage_attachment,
target: "_blank", target: "_blank",
rel: "nofollow") %> rel: "nofollow") %>

View File

@@ -21,9 +21,9 @@
<% @documents.each do |document| %> <% @documents.each do |document| %>
<tr id="<%= dom_id(document) %>" class="document"> <tr id="<%= dom_id(document) %>" class="document">
<td><%= document.title %></td> <td><%= document.title %></td>
<td><%= document.attachment.content_type %></td> <td><%= document.storage_attachment.content_type %></td>
<td><%= number_to_human_size(document.attachment.size) %></td> <td><%= number_to_human_size(document.storage_attachment.byte_size) %></td>
<td><%= link_to document.title, document.attachment.url, target: :blank %></td> <td><%= link_to document.title, document.storage_attachment, target: :blank %></td>
<td> <td>
<div class="small-12 medium-6 column"> <div class="small-12 medium-6 column">
<%= render Admin::TableActionsComponent.new( <%= render Admin::TableActionsComponent.new(

View File

@@ -1,9 +1,9 @@
<% milestone = first_milestone_with_image(investment) %> <% milestone = first_milestone_with_image(investment) %>
<% if milestone&.image.present? %> <% if milestone&.image.present? %>
<%= image_tag milestone.image_url(:large), alt: milestone.image.title %> <%= image_tag milestone.image.variant(:large), alt: milestone.image.title %>
<% elsif investment.image.present? %> <% elsif investment.image.present? %>
<%= image_tag investment.image_url(:large), alt: investment.image.title %> <%= image_tag investment.image.variant(:large), alt: investment.image.title %>
<% else %> <% else %>
<%= image_tag "budget_execution_no_image.jpg", alt: investment.title %> <%= image_tag "budget_execution_no_image.jpg", alt: investment.title %>
<% end %> <% end %>

View File

@@ -5,7 +5,7 @@
<div class="row"> <div class="row">
<div class="small-12 medium-3 large-2 column text-center"> <div class="small-12 medium-3 large-2 column text-center">
<%= image_tag investment.image_url(:thumb), alt: investment.image.title.unicode_normalize %> <%= image_tag investment.image.variant(:thumb), alt: investment.image.title.unicode_normalize %>
</div> </div>
<div class="small-12 medium-6 large-7 column"> <div class="small-12 medium-6 large-7 column">

View File

@@ -3,8 +3,8 @@
social_url: budget_investments_path(investment), social_url: budget_investments_path(investment),
social_title: investment.title, social_title: investment.title,
social_description: investment.description, social_description: investment.description,
twitter_image_url: (investment.image.present? ? investment.image_url(:thumb) : nil), twitter_image_url: (investment.image.present? ? polymorphic_path(investment.image.variant(:thumb)) : nil),
og_image_url: (investment.image.present? ? investment.image_url(:thumb) : nil) %> og_image_url: (investment.image.present? ? polymorphic_path(investment.image.variant(:thumb)) : nil) %>
<% end %> <% end %>
<% cache [locale_and_user_status(investment), <% cache [locale_and_user_status(investment),

View File

@@ -1,5 +1,5 @@
<p> <p>
<%= link_to document.attachment.url, target: "_blank" do %> <%= link_to document.storage_attachment, target: "_blank" do %>
<%= document.title %> <%= document.title %>
(<%= document.humanized_content_type %> (<%= document.humanized_content_type %>
&nbsp;|&nbsp; &nbsp;|&nbsp;

View File

@@ -15,7 +15,7 @@
</table> </table>
<% if @proposal.image.present? %> <% if @proposal.image.present? %>
<%= image_tag @proposal.image.attachment.url(:large), style: "width: 100%; box-shadow: -16px 61px 49px -19px rgba(0,0,0,0.1);" %> <%= image_tag @proposal.image.variant(:large), style: "width: 100%; box-shadow: -16px 61px 49px -19px rgba(0,0,0,0.1);" %>
<% else %> <% else %>
<%= image_tag "default_mailing.jpg", style: "width: 100%; box-shadow: -16px 61px 49px -19px rgba(0,0,0,0.1);" %> <%= image_tag "default_mailing.jpg", style: "width: 100%; box-shadow: -16px 61px 49px -19px rgba(0,0,0,0.1);" %>
<% end %> <% end %>

View File

@@ -9,7 +9,7 @@
</div> </div>
<div class="margin-bottom"> <div class="margin-bottom">
<%= image_tag(proposal.image_url(:large).presence || "default_mailing.jpg") %> <%= image_tag(proposal.image&.variant(:large).presence || "default_mailing.jpg") %>
</div> </div>
<div class="mail-body"> <div class="mail-body">

View File

@@ -18,7 +18,7 @@
</p> </p>
<div class="proposal-image"> <div class="proposal-image">
<% if proposal.image.present? %> <% if proposal.image.present? %>
<div class="overflow-image" style="background-image: url(<%= asset_url proposal.image.attachment.url(:large) %>);"></div> <div class="overflow-image" style="background-image: url(<%= polymorphic_path(proposal.image.variant(:large)) %>);"></div>
<% else %> <% else %>
<div class="overflow-image" style="background-image: url(<%= asset_url "default_mailing.jpg" %>);"></div> <div class="overflow-image" style="background-image: url(<%= asset_url "default_mailing.jpg" %>);"></div>
<% end %> <% end %>

View File

@@ -23,7 +23,7 @@
</p> </p>
<div class="proposal-image"> <div class="proposal-image">
<% if proposal.image.present? %> <% if proposal.image.present? %>
<div class="overflow-image" style="background-image: url(<%= asset_url proposal.image.attachment.url(:large) %>);"></div> <div class="overflow-image" style="background-image: url(<%= polymorphic_url(proposal.image.variant(:large)) %>);"></div>
<% else %> <% else %>
<div class="overflow-image" style="background-image:url('<%= "file://#{Rails.root.join("app","assets","images","default_mailing.jpg")}" %>');"></div> <div class="overflow-image" style="background-image:url('<%= "file://#{Rails.root.join("app","assets","images","default_mailing.jpg")}" %>');"></div>
<% end %> <% end %>

View File

@@ -1,6 +1,6 @@
<li id="<%= dom_id(document) %>"> <li id="<%= dom_id(document) %>">
<%= link_to t("documents.buttons.download_document"), <%= link_to t("documents.buttons.download_document"),
document.attachment.url, target: "_blank", document.storage_attachment, target: "_blank",
rel: "nofollow", class: "button hollow medium float-right" %> rel: "nofollow", class: "button hollow medium float-right" %>
<strong><%= document.title %></strong> <strong><%= document.title %></strong>

View File

@@ -1,6 +1,6 @@
<div id="image" class="small-12 column text-center image-preview"> <div id="image" class="small-12 column text-center image-preview">
<figure> <figure>
<%= image_tag image.attachment.url(version), <%= image_tag image.variant(version),
class: image_class(image), class: image_class(image),
alt: image.title.unicode_normalize, alt: image.title.unicode_normalize,
title: image.title.unicode_normalize %> title: image.title.unicode_normalize %>

View File

@@ -45,7 +45,7 @@
<% if @process.image.present? %> <% if @process.image.present? %>
<hr> <hr>
<%= image_tag(@process.image_url(:large), alt: @process.title, id: "image") %> <%= image_tag(@process.image.variant(:large), alt: @process.title, id: "image") %>
<% end %> <% end %>
<% if process.draft_publication.enabled? %> <% if process.draft_publication.enabled? %>

View File

@@ -7,7 +7,7 @@
<% if proposal.image.present? %> <% if proposal.image.present? %>
<div class="row"> <div class="row">
<div class="small-12 medium-3 large-2 column text-center"> <div class="small-12 medium-3 large-2 column text-center">
<%= image_tag proposal.image_url(:thumb), <%= image_tag proposal.image.variant(:thumb),
alt: proposal.image.title.unicode_normalize %> alt: proposal.image.title.unicode_normalize %>
</div> </div>
<div class="small-12 medium-6 large-7 column margin-top"> <div class="small-12 medium-6 large-7 column margin-top">

View File

@@ -22,7 +22,7 @@
</p> </p>
<% end %> <% end %>
<%= image_tag(milestone.image_url(:large), { id: "image_#{milestone.id}", alt: milestone.image.title, class: "margin" }) if milestone.image.present? %> <%= image_tag(milestone.image.variant(:large), { id: "image_#{milestone.id}", alt: milestone.image.title, class: "margin" }) if milestone.image.present? %>
<p> <p>
<%= sanitize_and_auto_link milestone.description %> <%= sanitize_and_auto_link milestone.description %>
@@ -37,7 +37,7 @@
<% milestone.documents.each do |document| %> <% milestone.documents.each do |document| %>
<%= link_to document.title, <%= link_to document.title,
document.attachment.url, document.storage_attachment,
target: "_blank", target: "_blank",
rel: "nofollow" %><br> rel: "nofollow" %><br>
<small> <small>

View File

@@ -18,8 +18,8 @@
<% answer.images.reverse.each_with_index do |image, index| %> <% answer.images.reverse.each_with_index do |image, index| %>
<li class="orbit-slide <%= is_active_class(index) %>"> <li class="orbit-slide <%= is_active_class(index) %>">
<%= link_to image.attachment.url(:original), target: "_blank" do %> <%= link_to image.storage_attachment, target: "_blank" do %>
<%= image_tag image.attachment.url(:original), <%= image_tag image.storage_attachment,
class: "orbit-image", class: "orbit-image",
alt: image.title.unicode_normalize %> alt: image.title.unicode_normalize %>
<% end %> <% end %>

View File

@@ -23,7 +23,7 @@
<div class="small-12 medium-3 column"> <div class="small-12 medium-3 column">
<div class="image-container" data-equalizer-watch> <div class="image-container" data-equalizer-watch>
<% if poll.image.present? %> <% if poll.image.present? %>
<%= image_tag poll.image_url(:large), alt: poll.image.title.unicode_normalize %> <%= image_tag poll.image.variant(:large), alt: poll.image.title.unicode_normalize %>
<% end %> <% end %>
</div> </div>
</div> </div>

View File

@@ -86,7 +86,7 @@
<% answer.documents.each do |document| %> <% answer.documents.each do |document| %>
<%= link_to document.title, <%= link_to document.title,
document.attachment.url, document.storage_attachment,
target: "_blank", target: "_blank",
rel: "nofollow" %><br> rel: "nofollow" %><br>
<% end %> <% end %>

View File

@@ -8,7 +8,7 @@
<% if proposal.image.present? %> <% if proposal.image.present? %>
<div class="row"> <div class="row">
<div class="small-12 medium-3 large-2 column text-center"> <div class="small-12 medium-3 large-2 column text-center">
<%= image_tag proposal.image_url(:thumb), <%= image_tag proposal.image.variant(:thumb),
alt: proposal.image.title.unicode_normalize %> alt: proposal.image.title.unicode_normalize %>
</div> </div>

View File

@@ -6,8 +6,8 @@
social_url: proposal_url(@proposal), social_url: proposal_url(@proposal),
social_title: @proposal.title, social_title: @proposal.title,
social_description: @proposal.summary, social_description: @proposal.summary,
twitter_image_url: (@proposal.image.present? ? @proposal.image_url(:thumb) : nil), twitter_image_url: (@proposal.image.present? ? polymorphic_path(@proposal.image.variant(:thumb)) : nil),
og_image_url: (@proposal.image.present? ? @proposal.image_url(:thumb) : nil) %> og_image_url: (@proposal.image.present? ? polymorphic_path(@proposal.image.variant(:thumb)) : nil) %>
<% end %> <% end %>
<% content_for :canonical do %> <% content_for :canonical do %>
<%= render "shared/canonical", href: proposal_url(@proposal) %> <%= render "shared/canonical", href: proposal_url(@proposal) %>

View File

@@ -4,7 +4,7 @@
<figure class="figure-card"> <figure class="figure-card">
<div class="gradient"></div> <div class="gradient"></div>
<% if card.image.present? %> <% if card.image.present? %>
<%= image_tag(card.image_url(:large), alt: card.image.title) %> <%= image_tag(card.image.variant(:large), alt: card.image.title) %>
<% end %> <% end %>
<figcaption> <figcaption>
<% if card.label.present? %> <% if card.label.present? %>

View File

@@ -16,7 +16,7 @@
<% if header.image.present? %> <% if header.image.present? %>
<div class="small-12 medium-6 column"> <div class="small-12 medium-6 column">
<%= image_tag(header.image_url(:large), <%= image_tag(header.image.variant(:large),
class: "margin", class: "margin",
alt: header.image.title) %> alt: header.image.title) %>
</div> </div>

View File

@@ -279,6 +279,26 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na
expect(page).not_to have_css ".document" expect(page).not_to have_css ".document"
end end
scenario "Same attachment URL after editing the title" do
do_login_for user_to_login
visit send(path, arguments)
documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf"))
within_fieldset("Documents") { fill_in "Title", with: "Original" }
click_button submit_button
expect(page).to have_content documentable_success_notice
original_url = find_link("Download file")[:href]
visit send(path, arguments)
within_fieldset("Documents") { fill_in "Title", with: "Updated" }
click_button submit_button
expect(page).to have_content documentable_success_notice
expect(find_link("Download file")[:href]).to eq original_url
end
end end
describe "When allow attached documents setting is disabled" do describe "When allow attached documents setting is disabled" do

View File

@@ -18,12 +18,12 @@ describe "Documents", :admin do
1.times { create(:document) } 1.times { create(:document) }
document = Document.first document = Document.first
attachment = document.attachment url = polymorphic_path(document.storage_attachment)
visit admin_site_customization_documents_path visit admin_site_customization_documents_path
expect(page).to have_content "There are 3 documents" expect(page).to have_content "There are 3 documents"
expect(page).to have_link document.title, href: attachment.url expect(page).to have_link document.title, href: url
end end
scenario "Index (empty)" do scenario "Index (empty)" do