From 091abfc9445ff2a508df4dfc0011cc53d094eae8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 20:48:33 +0200 Subject: [PATCH 01/11] 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. --- Gemfile | 1 + Gemfile.lock | 2 ++ .../admin/widget/cards/row_component.html.erb | 2 +- .../attachable/fields_component.html.erb | 2 +- app/components/attachable/fields_component.rb | 2 +- .../budgets/budget_component.html.erb | 2 +- .../budgets/investment_component.html.erb | 2 +- .../budgets/phases_component.html.erb | 2 +- .../widget/feeds/proposal_component.html.erb | 2 +- app/helpers/documents_helper.rb | 2 +- app/helpers/images_helper.rb | 11 ++++------ app/helpers/welcome_helper.rb | 5 +++-- app/models/concerns/imageable.rb | 4 ---- app/models/image.rb | 16 +++++++++++++++ .../admin/milestones/_milestones.html.erb | 4 ++-- .../poll/questions/answers/documents.html.erb | 4 ++-- .../documents/index.html.erb | 6 +++--- app/views/budgets/executions/_image.html.erb | 4 ++-- .../budgets/investments/_investment.html.erb | 2 +- .../investments/_investment_show.html.erb | 4 ++-- app/views/dashboard/_document.html.erb | 2 +- app/views/dashboard/mailer/forward.html.erb | 2 +- app/views/dashboard/mailing/index.html.erb | 2 +- app/views/dashboard/poster/index.html.erb | 2 +- app/views/dashboard/poster/index.pdf.erb | 2 +- app/views/documents/_document.html.erb | 2 +- app/views/images/_image.html.erb | 2 +- .../legislation/processes/_header.html.erb | 2 +- .../legislation/proposals/_proposal.html.erb | 2 +- app/views/milestones/_milestone.html.erb | 4 ++-- app/views/polls/_gallery.html.erb | 4 ++-- app/views/polls/_poll_group.html.erb | 2 +- app/views/polls/show.html.erb | 2 +- app/views/proposals/_proposal.html.erb | 2 +- app/views/proposals/show.html.erb | 4 ++-- app/views/shared/_card.html.erb | 2 +- app/views/shared/_header.html.erb | 2 +- spec/shared/system/nested_documentable.rb | 20 +++++++++++++++++++ .../site_customization/documents_spec.rb | 4 ++-- 39 files changed, 88 insertions(+), 55 deletions(-) diff --git a/Gemfile b/Gemfile index f71cb950b..28833049f 100644 --- a/Gemfile +++ b/Gemfile @@ -33,6 +33,7 @@ gem "jquery-fileupload-rails" gem "jquery-rails", "~> 4.4.0" gem "jquery-ui-rails", "~> 6.0.1" gem "kaminari", "~> 1.2.1" +gem "mini_magick", "~> 4.11.0" gem "omniauth", "~> 2.0.4" gem "omniauth-facebook", "~> 8.0.0" gem "omniauth-google-oauth2", "~> 1.0.0" diff --git a/Gemfile.lock b/Gemfile.lock index dd0425134..1ea2c5adb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -373,6 +373,7 @@ GEM mimemagic (0.3.10) nokogiri (~> 1) rake + mini_magick (4.11.0) mini_mime (1.1.0) mini_portile2 (2.6.1) minitest (5.14.4) @@ -742,6 +743,7 @@ DEPENDENCIES launchy (~> 2.5.0) letter_opener_web (~> 1.4.0) mdl (~> 0.11.0) + mini_magick (~> 4.11.0) omniauth (~> 2.0.4) omniauth-facebook (~> 8.0.0) omniauth-google-oauth2 (~> 1.0.0) diff --git a/app/components/admin/widget/cards/row_component.html.erb b/app/components/admin/widget/cards/row_component.html.erb index 974753dc8..c66b28708 100644 --- a/app/components/admin/widget/cards/row_component.html.erb +++ b/app/components/admin/widget/cards/row_component.html.erb @@ -12,7 +12,7 @@ <% 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" %> <% end %> diff --git a/app/components/attachable/fields_component.html.erb b/app/components/attachable/fields_component.html.erb index ef329a31e..54f91cb2a 100644 --- a/app/components/attachable/fields_component.html.erb +++ b/app/components/attachable/fields_component.html.erb @@ -7,7 +7,7 @@ <%= f.text_field :title, placeholder: t("#{plural_name}.form.title_placeholder") %> - <% if attachable.attachment.exists? && attachable.attachment.styles.keys.include?(:thumb) %> + <% if attachable.storage_attachment.attached? && attachable.storage_attachment.image? %> <%= render_image(attachable, :thumb, false) %> <% end %> diff --git a/app/components/attachable/fields_component.rb b/app/components/attachable/fields_component.rb index ae217456a..fac93f7be 100644 --- a/app/components/attachable/fields_component.rb +++ b/app/components/attachable/fields_component.rb @@ -24,7 +24,7 @@ class Attachable::FieldsComponent < ApplicationComponent end def file_name - attachable.attachment_file_name + attachable.storage_attachment.filename if attachable.storage_attachment.attached? end def destroy_link diff --git a/app/components/budgets/budget_component.html.erb b/app/components/budgets/budget_component.html.erb index 7ba34034a..232d5da75 100644 --- a/app/components/budgets/budget_component.html.erb +++ b/app/components/budgets/budget_component.html.erb @@ -1,6 +1,6 @@ <% if budget.image.present? %>
+ style="background-image: url(<%= polymorphic_path(budget.image.variant(:large)) %>);"> <% else %>
<% end %> diff --git a/app/components/budgets/investment_component.html.erb b/app/components/budgets/investment_component.html.erb index 4ca171959..1b05e2c9b 100644 --- a/app/components/budgets/investment_component.html.erb +++ b/app/components/budgets/investment_component.html.erb @@ -1,6 +1,6 @@
<% 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 %> <%= image_tag "budget_investment_no_image.jpg", alt: investment.title %> <% end %> diff --git a/app/components/budgets/phases_component.html.erb b/app/components/budgets/phases_component.html.erb index 8c881a4f1..b7921f712 100644 --- a/app/components/budgets/phases_component.html.erb +++ b/app/components/budgets/phases_component.html.erb @@ -55,7 +55,7 @@ <% if phase.image.present? %>
- <%= image_tag phase.image.attachment.url(:large), alt: "" %> + <%= image_tag phase.image.variant(:large), alt: "" %>
<% end %>
diff --git a/app/components/widget/feeds/proposal_component.html.erb b/app/components/widget/feeds/proposal_component.html.erb index 8f683776a..b9bbc83c8 100644 --- a/app/components/widget/feeds/proposal_component.html.erb +++ b/app/components/widget/feeds/proposal_component.html.erb @@ -9,7 +9,7 @@ <% if proposal.image.present? %>
- <%= image_tag proposal.image_url(:thumb), + <%= image_tag proposal.image.variant(:thumb), alt: proposal.image.title.unicode_normalize %>
diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index 3ff564a03..742a88acf 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -3,7 +3,7 @@ module DocumentsHelper info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}" link_to safe_join([document.title, tag.small("(#{info_text})")], " "), - document.attachment.url, + document.storage_attachment, target: "_blank", title: t("shared.target_blank") end diff --git a/app/helpers/images_helper.rb b/app/helpers/images_helper.rb index 0f64a9bc2..06d63e9f8 100644 --- a/app/helpers/images_helper.rb +++ b/app/helpers/images_helper.rb @@ -2,11 +2,7 @@ module ImagesHelper def image_absolute_url(image, version) return "" unless image - if Paperclip::Attachment.default_options[:storage] == :filesystem - URI(request.url) + image.attachment.url(version) - else - image.attachment.url(version) - end + polymorphic_url(image.variant(version)) end def image_class(image) @@ -14,7 +10,8 @@ module ImagesHelper end def render_image(image, version, show_caption = true) - version = image.persisted? ? version : :original - render "images/image", image: image, version: version, show_caption: show_caption + render "images/image", image: image, + version: (version if image.persisted?), + show_caption: show_caption end end diff --git a/app/helpers/welcome_helper.rb b/app/helpers/welcome_helper.rb index 5962ff682..0f26b3a73 100644 --- a/app/helpers/welcome_helper.rb +++ b/app/helpers/welcome_helper.rb @@ -24,8 +24,9 @@ module WelcomeHelper end def calculate_image_path(recommended, image_default) - if recommended.respond_to?(:image) && recommended.image.present? && recommended.image.attachment.exists? - recommended.image.attachment.send("url", :medium) + if recommended.respond_to?(:image) && recommended.image.present? && + recommended.image.storage_attachment.attached? + recommended.image.variant(:medium) elsif image_default.present? image_default end diff --git a/app/models/concerns/imageable.rb b/app/models/concerns/imageable.rb index fffc871de..ad4630a6c 100644 --- a/app/models/concerns/imageable.rb +++ b/app/models/concerns/imageable.rb @@ -4,9 +4,5 @@ module Imageable included do has_one :image, as: :imageable, inverse_of: :imageable, dependent: :destroy accepts_nested_attributes_for :image, allow_destroy: true, update_only: true - - def image_url(style) - image&.attachment&.url(style) || "" - end end end diff --git a/app/models/image.rb b/app/models/image.rb index de1405f2b..a86091648 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -11,6 +11,14 @@ class Image < ApplicationRecord use_timestamp: false, 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 :imageable, polymorphic: true, touch: true @@ -41,6 +49,14 @@ class Image < ApplicationRecord self.class.accepted_content_types end + def variant(style) + if style + storage_attachment.variant(self.class.styles[style]) + else + storage_attachment + end + end + private def association_name diff --git a/app/views/admin/milestones/_milestones.html.erb b/app/views/admin/milestones/_milestones.html.erb index 14e99f1b2..ac4d599a8 100644 --- a/app/views/admin/milestones/_milestones.html.erb +++ b/app/views/admin/milestones/_milestones.html.erb @@ -40,14 +40,14 @@ <%= link_to t("admin.milestones.index.show_image"), - milestone.image_url(:large), + milestone.image.variant(:large), target: :_blank if milestone.image.present? %> <% if milestone.documents.present? %> <% milestone.documents.each do |document| %> <%= link_to document.title, - document.attachment.url, + document.storage_attachment, target: "_blank", rel: "nofollow" %>
<% end %> diff --git a/app/views/admin/poll/questions/answers/documents.html.erb b/app/views/admin/poll/questions/answers/documents.html.erb index 53d78996a..c322bcdaa 100644 --- a/app/views/admin/poll/questions/answers/documents.html.erb +++ b/app/views/admin/poll/questions/answers/documents.html.erb @@ -33,7 +33,7 @@ <% @answer.documents.each do |document| %> - <%= link_to document.title, document.attachment.url %> + <%= link_to document.title, document.storage_attachment %> <%= render Admin::TableActionsComponent.new(document, @@ -42,7 +42,7 @@ ) do |actions| %> <%= actions.action(:download, text: t("documents.buttons.download_document"), - path: document.attachment.url, + path: document.storage_attachment, target: "_blank", rel: "nofollow") %> diff --git a/app/views/admin/site_customization/documents/index.html.erb b/app/views/admin/site_customization/documents/index.html.erb index 491d9e069..f7a8e6409 100644 --- a/app/views/admin/site_customization/documents/index.html.erb +++ b/app/views/admin/site_customization/documents/index.html.erb @@ -21,9 +21,9 @@ <% @documents.each do |document| %> <%= document.title %> - <%= document.attachment.content_type %> - <%= number_to_human_size(document.attachment.size) %> - <%= link_to document.title, document.attachment.url, target: :blank %> + <%= document.storage_attachment.content_type %> + <%= number_to_human_size(document.storage_attachment.byte_size) %> + <%= link_to document.title, document.storage_attachment, target: :blank %>
<%= render Admin::TableActionsComponent.new( diff --git a/app/views/budgets/executions/_image.html.erb b/app/views/budgets/executions/_image.html.erb index 639761875..ebe933504 100644 --- a/app/views/budgets/executions/_image.html.erb +++ b/app/views/budgets/executions/_image.html.erb @@ -1,9 +1,9 @@ <% milestone = first_milestone_with_image(investment) %> <% 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? %> - <%= image_tag investment.image_url(:large), alt: investment.image.title %> + <%= image_tag investment.image.variant(:large), alt: investment.image.title %> <% else %> <%= image_tag "budget_execution_no_image.jpg", alt: investment.title %> <% end %> diff --git a/app/views/budgets/investments/_investment.html.erb b/app/views/budgets/investments/_investment.html.erb index fea6b4241..1a4009952 100644 --- a/app/views/budgets/investments/_investment.html.erb +++ b/app/views/budgets/investments/_investment.html.erb @@ -5,7 +5,7 @@
- <%= image_tag investment.image_url(:thumb), alt: investment.image.title.unicode_normalize %> + <%= image_tag investment.image.variant(:thumb), alt: investment.image.title.unicode_normalize %>
diff --git a/app/views/budgets/investments/_investment_show.html.erb b/app/views/budgets/investments/_investment_show.html.erb index 2b31e476f..d7a46cf51 100644 --- a/app/views/budgets/investments/_investment_show.html.erb +++ b/app/views/budgets/investments/_investment_show.html.erb @@ -3,8 +3,8 @@ social_url: budget_investments_path(investment), social_title: investment.title, social_description: investment.description, - twitter_image_url: (investment.image.present? ? investment.image_url(:thumb) : nil), - og_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? ? polymorphic_path(investment.image.variant(:thumb)) : nil) %> <% end %> <% cache [locale_and_user_status(investment), diff --git a/app/views/dashboard/_document.html.erb b/app/views/dashboard/_document.html.erb index 0b61002a2..e91efb98b 100644 --- a/app/views/dashboard/_document.html.erb +++ b/app/views/dashboard/_document.html.erb @@ -1,5 +1,5 @@

- <%= link_to document.attachment.url, target: "_blank" do %> + <%= link_to document.storage_attachment, target: "_blank" do %> <%= document.title %> (<%= document.humanized_content_type %>  |  diff --git a/app/views/dashboard/mailer/forward.html.erb b/app/views/dashboard/mailer/forward.html.erb index f3f25fe12..95cea001c 100644 --- a/app/views/dashboard/mailer/forward.html.erb +++ b/app/views/dashboard/mailer/forward.html.erb @@ -15,7 +15,7 @@ <% 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 %> <%= image_tag "default_mailing.jpg", style: "width: 100%; box-shadow: -16px 61px 49px -19px rgba(0,0,0,0.1);" %> <% end %> diff --git a/app/views/dashboard/mailing/index.html.erb b/app/views/dashboard/mailing/index.html.erb index 1886d07c8..bf1f01fb9 100644 --- a/app/views/dashboard/mailing/index.html.erb +++ b/app/views/dashboard/mailing/index.html.erb @@ -9,7 +9,7 @@

- <%= image_tag(proposal.image_url(:large).presence || "default_mailing.jpg") %> + <%= image_tag(proposal.image&.variant(:large).presence || "default_mailing.jpg") %>
diff --git a/app/views/dashboard/poster/index.html.erb b/app/views/dashboard/poster/index.html.erb index 7cdfb2faa..e1cddb5d4 100644 --- a/app/views/dashboard/poster/index.html.erb +++ b/app/views/dashboard/poster/index.html.erb @@ -18,7 +18,7 @@

<% if proposal.image.present? %> -
+
<% else %>
);">
<% end %> diff --git a/app/views/dashboard/poster/index.pdf.erb b/app/views/dashboard/poster/index.pdf.erb index c1ba4ea78..c762156d2 100644 --- a/app/views/dashboard/poster/index.pdf.erb +++ b/app/views/dashboard/poster/index.pdf.erb @@ -23,7 +23,7 @@

<% if proposal.image.present? %> -
+
<% else %>
');">
<% end %> diff --git a/app/views/documents/_document.html.erb b/app/views/documents/_document.html.erb index 07d31e026..67c61216b 100644 --- a/app/views/documents/_document.html.erb +++ b/app/views/documents/_document.html.erb @@ -1,6 +1,6 @@
  • <%= 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" %> <%= document.title %> diff --git a/app/views/images/_image.html.erb b/app/views/images/_image.html.erb index 65ef9324e..7cb5feb68 100644 --- a/app/views/images/_image.html.erb +++ b/app/views/images/_image.html.erb @@ -1,6 +1,6 @@
    - <%= image_tag image.attachment.url(version), + <%= image_tag image.variant(version), class: image_class(image), alt: image.title.unicode_normalize, title: image.title.unicode_normalize %> diff --git a/app/views/legislation/processes/_header.html.erb b/app/views/legislation/processes/_header.html.erb index 8917176e5..eb05108a5 100644 --- a/app/views/legislation/processes/_header.html.erb +++ b/app/views/legislation/processes/_header.html.erb @@ -45,7 +45,7 @@ <% if @process.image.present? %>
    - <%= image_tag(@process.image_url(:large), alt: @process.title, id: "image") %> + <%= image_tag(@process.image.variant(:large), alt: @process.title, id: "image") %> <% end %> <% if process.draft_publication.enabled? %> diff --git a/app/views/legislation/proposals/_proposal.html.erb b/app/views/legislation/proposals/_proposal.html.erb index ad3770683..0142be99c 100644 --- a/app/views/legislation/proposals/_proposal.html.erb +++ b/app/views/legislation/proposals/_proposal.html.erb @@ -7,7 +7,7 @@ <% if proposal.image.present? %>
    - <%= image_tag proposal.image_url(:thumb), + <%= image_tag proposal.image.variant(:thumb), alt: proposal.image.title.unicode_normalize %>
    diff --git a/app/views/milestones/_milestone.html.erb b/app/views/milestones/_milestone.html.erb index 6de305b85..ce8ea12bc 100644 --- a/app/views/milestones/_milestone.html.erb +++ b/app/views/milestones/_milestone.html.erb @@ -22,7 +22,7 @@

    <% 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? %>

    <%= sanitize_and_auto_link milestone.description %> @@ -37,7 +37,7 @@ <% milestone.documents.each do |document| %> <%= link_to document.title, - document.attachment.url, + document.storage_attachment, target: "_blank", rel: "nofollow" %>
    diff --git a/app/views/polls/_gallery.html.erb b/app/views/polls/_gallery.html.erb index f62f28831..e62827908 100644 --- a/app/views/polls/_gallery.html.erb +++ b/app/views/polls/_gallery.html.erb @@ -18,8 +18,8 @@ <% answer.images.reverse.each_with_index do |image, index| %>

  • - <%= link_to image.attachment.url(:original), target: "_blank" do %> - <%= image_tag image.attachment.url(:original), + <%= link_to image.storage_attachment, target: "_blank" do %> + <%= image_tag image.storage_attachment, class: "orbit-image", alt: image.title.unicode_normalize %> <% end %> diff --git a/app/views/polls/_poll_group.html.erb b/app/views/polls/_poll_group.html.erb index 3a4d66296..58268e21b 100644 --- a/app/views/polls/_poll_group.html.erb +++ b/app/views/polls/_poll_group.html.erb @@ -23,7 +23,7 @@
    <% 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 %>
    diff --git a/app/views/polls/show.html.erb b/app/views/polls/show.html.erb index ebc55daa7..69272441a 100644 --- a/app/views/polls/show.html.erb +++ b/app/views/polls/show.html.erb @@ -86,7 +86,7 @@ <% answer.documents.each do |document| %> <%= link_to document.title, - document.attachment.url, + document.storage_attachment, target: "_blank", rel: "nofollow" %>
    <% end %> diff --git a/app/views/proposals/_proposal.html.erb b/app/views/proposals/_proposal.html.erb index 733e3adf2..0b3c8ec33 100644 --- a/app/views/proposals/_proposal.html.erb +++ b/app/views/proposals/_proposal.html.erb @@ -8,7 +8,7 @@ <% if proposal.image.present? %>
    - <%= image_tag proposal.image_url(:thumb), + <%= image_tag proposal.image.variant(:thumb), alt: proposal.image.title.unicode_normalize %>
    diff --git a/app/views/proposals/show.html.erb b/app/views/proposals/show.html.erb index 15f52d8c7..02dc1c38c 100644 --- a/app/views/proposals/show.html.erb +++ b/app/views/proposals/show.html.erb @@ -6,8 +6,8 @@ social_url: proposal_url(@proposal), social_title: @proposal.title, social_description: @proposal.summary, - twitter_image_url: (@proposal.image.present? ? @proposal.image_url(:thumb) : nil), - og_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? ? polymorphic_path(@proposal.image.variant(:thumb)) : nil) %> <% end %> <% content_for :canonical do %> <%= render "shared/canonical", href: proposal_url(@proposal) %> diff --git a/app/views/shared/_card.html.erb b/app/views/shared/_card.html.erb index 4e6c1e459..8a591d4f4 100644 --- a/app/views/shared/_card.html.erb +++ b/app/views/shared/_card.html.erb @@ -4,7 +4,7 @@
    <% 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 %>
    <% if card.label.present? %> diff --git a/app/views/shared/_header.html.erb b/app/views/shared/_header.html.erb index 06e0fbc54..9eec4fc3f 100644 --- a/app/views/shared/_header.html.erb +++ b/app/views/shared/_header.html.erb @@ -16,7 +16,7 @@ <% if header.image.present? %>
    - <%= image_tag(header.image_url(:large), + <%= image_tag(header.image.variant(:large), class: "margin", alt: header.image.title) %>
    diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index cb3d0d6bb..da0a72b0a 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -279,6 +279,26 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na expect(page).not_to have_css ".document" 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 describe "When allow attached documents setting is disabled" do diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index f0ff988d9..fe2fe332a 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -18,12 +18,12 @@ describe "Documents", :admin do 1.times { create(:document) } document = Document.first - attachment = document.attachment + url = polymorphic_path(document.storage_attachment) visit admin_site_customization_documents_path 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 scenario "Index (empty)" do From e0e35298d5459c2838a3921cfe0d31bb7f061351 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 22:14:45 +0200 Subject: [PATCH 02/11] Use Active Storage to handle cached attachments 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. --- app/controllers/direct_uploads_controller.rb | 4 +- app/models/concerns/attachable.rb | 42 +++++++++------- app/models/concerns/has_attachment.rb | 13 +++-- app/models/direct_upload.rb | 4 +- app/models/document.rb | 2 +- app/models/image.rb | 19 ++++--- db/dev_seeds/widgets.rb | 2 +- lib/tasks/files.rake | 16 ++---- spec/lib/tasks/active_storage_spec.rb | 14 +++--- spec/lib/tasks/files_spec.rb | 20 ++++---- spec/models/direct_upload_spec.rb | 11 ++-- spec/shared/system/nested_documentable.rb | 21 ++++---- spec/shared/system/nested_imageable.rb | 53 +++++++++++++++----- 13 files changed, 123 insertions(+), 98 deletions(-) diff --git a/app/controllers/direct_uploads_controller.rb b/app/controllers/direct_uploads_controller.rb index a1995c2e3..9bc565a29 100644 --- a/app/controllers/direct_uploads_controller.rb +++ b/app/controllers/direct_uploads_controller.rb @@ -15,9 +15,9 @@ class DirectUploadsController < ApplicationController @direct_upload.relation.set_cached_attachment_from_attachment render json: { cached_attachment: @direct_upload.relation.cached_attachment, - filename: @direct_upload.relation.attachment.original_filename, + filename: @direct_upload.relation.storage_attachment.filename.to_s, destroy_link: render_destroy_upload_link(@direct_upload), - attachment_url: @direct_upload.relation.attachment.url } + attachment_url: polymorphic_path(@direct_upload.relation.storage_attachment) } else render json: { errors: @direct_upload.errors[:attachment].join(", ") }, status: :unprocessable_entity diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index 9fb8c7a80..a8fda86ac 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -9,14 +9,10 @@ module Attachable # Paperclip do not allow to use Procs on valiations definition do_not_validate_attachment_file_type :attachment validate :attachment_presence - validate :validate_attachment_content_type, if: -> { attachment.present? } - validate :validate_attachment_size, if: -> { attachment.present? } + validate :validate_attachment_content_type, if: -> { storage_attachment.attached? } + validate :validate_attachment_size, if: -> { storage_attachment.attached? } - before_save :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } - - Paperclip.interpolates :prefix do |attachment, style| - attachment.instance.prefix(attachment, style) - end + before_validation :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } end def association_class @@ -26,29 +22,37 @@ module Attachable end def set_cached_attachment_from_attachment - self.cached_attachment = if filesystem_storage? - attachment.path - else - attachment.url - end + self.cached_attachment = storage_attachment.signed_id end def set_attachment_from_cached_attachment + self.storage_attachment = cached_attachment + if filesystem_storage? - File.open(cached_attachment) { |file| self.attachment = file } + File.open(file_path) do |file| + self.paperclip_attachment = file + end else - self.attachment = URI.parse(cached_attachment).open + self.paperclip_attachment = URI.parse(cached_attachment).open end end - def prefix(attachment, _style) - if attachment.instance.persisted? - ":attachment/:id_partition" + def attachment_content_type + storage_attachment.blob.content_type if storage_attachment.attached? + end + + def attachment_file_size + if storage_attachment.attached? + storage_attachment.blob.byte_size else - "cached_attachments/user/#{attachment.instance.user_id}" + 0 end end + def file_path + ActiveStorage::Blob.service.path_for(storage_attachment.blob.key) + end + private def filesystem_storage? @@ -73,7 +77,7 @@ module Attachable end def attachment_presence - if attachment.blank? && cached_attachment.blank? + unless storage_attachment.attached? errors.add(:attachment, I18n.t("errors.messages.blank")) end end diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb index 6f3f67844..8fcafa3ec 100644 --- a/app/models/concerns/has_attachment.rb +++ b/app/models/concerns/has_attachment.rb @@ -5,18 +5,21 @@ module HasAttachment def has_attachment(attribute, paperclip_options = {}) has_one_attached :"storage_#{attribute}" - has_attached_file attribute, paperclip_options - alias_method :"paperclip_#{attribute}=", :"#{attribute}=" - - define_method :"#{attribute}=" do |file| - if file.is_a?(IO) || file.is_a?(Tempfile) && !file.is_a?(Ckeditor::Http::QqFile) + define_method :"storage_#{attribute}=" do |file| + if file.is_a?(IO) send(:"storage_#{attribute}").attach(io: file, filename: File.basename(file.path)) elsif file.nil? send(:"storage_#{attribute}").detach else send(:"storage_#{attribute}").attach(file) end + end + has_attached_file attribute, paperclip_options + alias_method :"paperclip_#{attribute}=", :"#{attribute}=" + + define_method :"#{attribute}=" do |file| + send(:"storage_#{attribute}=", file) send(:"paperclip_#{attribute}=", file) end end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 7e66375e7..66a9d45ab 100644 --- a/app/models/direct_upload.rb +++ b/app/models/direct_upload.rb @@ -34,7 +34,7 @@ class DirectUpload end def save_attachment - @relation.attachment.save + @relation.storage_attachment.blob.save! end def persisted? @@ -53,7 +53,7 @@ class DirectUpload def relation_attributtes { - paperclip_attachment: @attachment, + storage_attachment: @attachment, cached_attachment: @cached_attachment, user: @user } diff --git a/app/models/document.rb b/app/models/document.rb index a121e3bbd..4b9349f3e 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -1,7 +1,7 @@ class Document < ApplicationRecord include Attachable - has_attachment :attachment, url: "/system/:class/:prefix/:style/:hash.:extension", + has_attachment :attachment, url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", hash_data: ":class/:style/:custom_hash_data", use_timestamp: false, hash_secret: Rails.application.secrets.secret_key_base diff --git a/app/models/image.rb b/app/models/image.rb index a86091648..098515114 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -6,7 +6,7 @@ class Image < ApplicationRecord medium: "300x300#", thumb: "140x245#" }, - url: "/system/:class/:prefix/:style/:hash.:extension", + url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", hash_data: ":class/:style", use_timestamp: false, hash_secret: Rails.application.secrets.secret_key_base @@ -27,7 +27,7 @@ class Image < ApplicationRecord validates :user_id, presence: true validates :imageable_id, presence: true, if: -> { persisted? } validates :imageable_type, presence: true, if: -> { persisted? } - validate :validate_image_dimensions, if: -> { attachment.present? && attachment.dirty? } + validate :validate_image_dimensions, if: -> { storage_attachment.attached? && storage_attachment.new_record? } def self.max_file_size Setting["uploads.images.max_size"].to_i @@ -68,14 +68,17 @@ class Image < ApplicationRecord end def validate_image_dimensions - if attachment_of_valid_content_type? + if accepted_content_types.include?(attachment_content_type) return true if imageable_class == Widget::Card - dimensions = Paperclip::Geometry.from_file(attachment.queued_for_write[:original].path) + storage_attachment.analyze unless storage_attachment.analyzed? + + width = storage_attachment.metadata[:width] + height = storage_attachment.metadata[:height] min_width = Setting["uploads.images.min_width"].to_i min_height = Setting["uploads.images.min_height"].to_i - errors.add(:attachment, :min_image_width, required_min_width: min_width) if dimensions.width < min_width - errors.add(:attachment, :min_image_height, required_min_height: min_height) if dimensions.height < min_height + errors.add(:attachment, :min_image_width, required_min_width: min_width) if width < min_width + errors.add(:attachment, :min_image_height, required_min_height: min_height) if height < min_height end end @@ -93,8 +96,4 @@ class Image < ApplicationRecord end end end - - def attachment_of_valid_content_type? - attachment.present? && accepted_content_types.include?(attachment_content_type) - end end diff --git a/db/dev_seeds/widgets.rb b/db/dev_seeds/widgets.rb index b672d28d5..41711c937 100644 --- a/db/dev_seeds/widgets.rb +++ b/db/dev_seeds/widgets.rb @@ -1,7 +1,7 @@ section "Creating header and cards for the homepage" do def create_image_attachment(type) { - cached_attachment: Rails.root.join("db/dev_seeds/images/#{type}_background.jpg"), + attachment: File.new(Rails.root.join("db/dev_seeds/images/#{type}_background.jpg")), title: "#{type}_background.jpg", user: User.first } diff --git a/lib/tasks/files.rake b/lib/tasks/files.rake index ef46097cc..978b5f4f1 100644 --- a/lib/tasks/files.rake +++ b/lib/tasks/files.rake @@ -1,18 +1,8 @@ namespace :files do desc "Removes cached attachments which weren't deleted for some reason" task remove_old_cached_attachments: :environment do - paths = Dir.glob(Rails.root.join("public/system/*/cached_attachments/**/*")) - logger = ApplicationLogger.new - - paths.each do |path| - if File.file?(path) - if File.mtime(path) < 1.day.ago - File.delete(path) - logger.info "The file #{path} has been removed" - end - else - Dir.delete(path) if Dir.empty?(path) - end - end + ActiveStorage::Blob.unattached + .where("active_storage_blobs.created_at <= ?", 1.day.ago) + .find_each(&:purge_later) end end diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index f554d05de..2a8043556 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -11,9 +11,10 @@ describe "active storage tasks" do before { FileUtils.rm_rf storage_root } it "migrates records and attachments" do - document = create(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = build(:document, + attachment: nil, + paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document.save(validate: false) expect(ActiveStorage::Attachment.count).to eq 0 expect(ActiveStorage::Blob.count).to eq 0 @@ -30,9 +31,10 @@ describe "active storage tasks" do end it "migrates records with deleted files ignoring the files" do - document = create(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = build(:document, + attachment: nil, + paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) + document.save(validate: false) FileUtils.rm(document.attachment.path) run_rake_task diff --git a/spec/lib/tasks/files_spec.rb b/spec/lib/tasks/files_spec.rb index 2c464bc82..454abe7be 100644 --- a/spec/lib/tasks/files_spec.rb +++ b/spec/lib/tasks/files_spec.rb @@ -11,26 +11,26 @@ describe "files tasks" do image = build(:image) document = build(:document) - image.attachment.save - document.attachment.save + image.storage_attachment.blob.save! + document.storage_attachment.blob.save! travel_to(2.days.from_now) { run_rake_task } - expect(File.exists?(image.attachment.path)).to be false - expect(File.exists?(document.attachment.path)).to be false + expect(File.exists?(image.file_path)).to be false + expect(File.exists?(document.file_path)).to be false end it "does not delete recent cached attachments" do image = build(:image) document = build(:document) - image.attachment.save - document.attachment.save + image.storage_attachment.blob.save! + document.storage_attachment.blob.save! travel_to(2.minutes.from_now) { run_rake_task } - expect(File.exists?(image.attachment.path)).to be true - expect(File.exists?(document.attachment.path)).to be true + expect(File.exists?(image.file_path)).to be true + expect(File.exists?(document.file_path)).to be true end it "does not delete old regular attachments" do @@ -39,8 +39,8 @@ describe "files tasks" do travel_to(2.days.from_now) { run_rake_task } - expect(File.exists?(image.attachment.path)).to be true - expect(File.exists?(document.attachment.path)).to be true + expect(File.exists?(image.file_path)).to be true + expect(File.exists?(document.file_path)).to be true end end end diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index 9c5657d55..7f281cc22 100644 --- a/spec/models/direct_upload_spec.rb +++ b/spec/models/direct_upload_spec.rb @@ -34,13 +34,14 @@ describe DirectUpload do end context "save_attachment" do - it "saves uploaded file" do - proposal_document_direct_upload = build(:direct_upload, :proposal, :documents) + it "saves uploaded file without creating an attachment record" do + direct_upload = build(:direct_upload, :proposal, :documents) - proposal_document_direct_upload.save_attachment + direct_upload.save_attachment - expect(File.exist?(proposal_document_direct_upload.relation.attachment.path)).to eq(true) - expect(proposal_document_direct_upload.relation.attachment.path).to include("cached_attachments") + expect(File.exist?(direct_upload.relation.file_path)).to be true + expect(direct_upload.relation.storage_attachment.blob).to be_persisted + expect(direct_upload.relation.storage_attachment.attachment).not_to be_persisted end end end diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index da0a72b0a..3e5d03f96 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -137,9 +137,15 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + click_link "Add new document" - expect_document_has_cached_attachment(0, ".pdf") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty + + attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") + + expect(page).to have_css(".loading-bar.complete") + expect(cached_attachment_field.value).not_to be_empty end scenario "Should not update document cached_attachment field after invalid file upload" do @@ -151,7 +157,8 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na false ) - expect_document_has_cached_attachment(0, "") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty end scenario "Should show document errors after documentable submit with @@ -350,14 +357,6 @@ def expect_document_has_title(index, title) end end -def expect_document_has_cached_attachment(index, extension) - document = all(".document")[index] - - within document do - expect(find("input[name$='[cached_attachment]']", visible: :hidden).value).to end_with(extension) - end -end - def documentable_fill_new_valid_proposal fill_in_new_proposal_title with: "Proposal title #{rand(9999)}" fill_in "Proposal summary", with: "Proposal summary" diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index 57c2329ec..f6fe1b723 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -90,9 +90,15 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + click_link "Add image" - expect_image_has_cached_attachment(".jpg") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + expect(cached_attachment_field.value).to be_empty + + attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + + expect(page).to have_css(".loading-bar.complete") + expect(cached_attachment_field.value).not_to be_empty end scenario "Should not update image cached_attachment field after invalid file upload" do @@ -101,7 +107,9 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) - expect_image_has_cached_attachment("") + cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) + + expect(cached_attachment_field.value).to be_empty end scenario "Should show nested image errors after invalid form submit" do @@ -120,6 +128,19 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end + scenario "Render image preview after sending the form with validation errors" do + skip "Question answers behave differently" if imageable.is_a?(Poll::Question::Answer) + do_login_for user + visit send(path, arguments) + + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + within_fieldset("Descriptive image") { fill_in "Title", with: "" } + click_on submit_button + + expect(page).to have_content "can't be blank" + expect(page).to have_css "img[src$='clippy.jpg']" + end + scenario "Should remove nested image after valid file upload and click on remove button" do do_login_for user visit send(path, arguments) @@ -180,6 +201,22 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p end end + scenario "Different URLs for different images" do + do_login_for user + visit send(path, arguments) + + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + + original_src = find(:fieldset, "Descriptive image").find("img")[:src] + + click_link "Remove image" + imageable_attach_new_file(Rails.root.join("spec/fixtures/files/custom_map.jpg")) + + updated_src = find(:fieldset, "Descriptive image").find("img")[:src] + + expect(updated_src).not_to eq original_src + end + if path.include? "edit" scenario "show persisted image" do create(:image, imageable: imageable) @@ -268,16 +305,6 @@ def expect_image_has_title(title) end end -def expect_image_has_cached_attachment(extension) - within "#nested-image" do - image = find(".image") - - within image do - expect(find("input[name$='[cached_attachment]']", visible: :hidden).value).to end_with(extension) - end - end -end - def show_caption_for?(imageable_factory_name) imageable_factory_name != "budget" end From 8e6df7f5d93dc0b362d7474bb7cc4ff4189acd3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 22:51:28 +0200 Subject: [PATCH 03/11] Use Active Storage to render custom images 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. --- app/helpers/application_helper.rb | 8 +++++++- app/models/site_customization/image.rb | 13 ++++++++++--- .../site_customization/images/index.html.erb | 4 ++-- .../files/apple-touch-icon-custom-200.png | Bin 0 -> 3463 bytes .../admin/site_customization/images_spec.rb | 11 +++++++++++ 5 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 spec/fixtures/files/apple-touch-icon-custom-200.png diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index e57d06576..0b9fb23f4 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -49,7 +49,13 @@ module ApplicationHelper end def image_path_for(filename) - SiteCustomization::Image.image_path_for(filename) || filename + image = SiteCustomization::Image.image_for(filename) + + if image + polymorphic_path(image) + else + filename + end end def content_block(name, locale = I18n.locale) diff --git a/app/models/site_customization/image.rb b/app/models/site_customization/image.rb index 592ea3b47..bb6859c2a 100644 --- a/app/models/site_customization/image.rb +++ b/app/models/site_customization/image.rb @@ -23,11 +23,10 @@ class SiteCustomization::Image < ApplicationRecord end end - def self.image_path_for(filename) + def self.image_for(filename) image_name = filename.split(".").first - imageable = find_by(name: image_name) - imageable.present? && imageable.image.exists? ? imageable.image.url : nil + find_by(name: image_name)&.persisted_image end def required_width @@ -38,6 +37,14 @@ class SiteCustomization::Image < ApplicationRecord VALID_IMAGES[name]&.second end + def persisted_image + storage_image if persisted_attachment? + end + + def persisted_attachment? + storage_image.attachment&.persisted? + end + private def check_image diff --git a/app/views/admin/site_customization/images/index.html.erb b/app/views/admin/site_customization/images/index.html.erb index f6ce49975..43da53e9e 100644 --- a/app/views/admin/site_customization/images/index.html.erb +++ b/app/views/admin/site_customization/images/index.html.erb @@ -16,12 +16,12 @@ <%= form_for([:admin, image], html: { id: "edit_#{dom_id(image)}" }) do |f| %>
    - <%= image_tag image.image.url if image.image.exists? %> + <%= image_tag image.storage_image if image.persisted_attachment? %> <%= f.file_field :image, label: false %>
    <%= f.submit(t("admin.site_customization.images.index.update"), class: "button hollow") %> - <%= link_to t("admin.site_customization.images.index.delete"), admin_site_customization_image_path(image), method: :delete, class: "button hollow alert" if image.image.exists? %> + <%= link_to t("admin.site_customization.images.index.delete"), admin_site_customization_image_path(image), method: :delete, class: "button hollow alert" if image.persisted_attachment? %>
    <% end %> diff --git a/spec/fixtures/files/apple-touch-icon-custom-200.png b/spec/fixtures/files/apple-touch-icon-custom-200.png new file mode 100644 index 0000000000000000000000000000000000000000..61216ad39b043a5fb91c67bfeb8926438802c393 GIT binary patch literal 3463 zcmcgvc{r478<%#H;%lQAV@ZPcLuCk)ZDNcy%M4A-k{Nq(q$7%PNG4m(5l(jV zAv={VTcON^Y{{|HGRQK{H#*h%{`mU-JnwzI@AKTxb^m_%{oK#*damn@vNS&@Atoax zA|fJTVvNBF$7h@GE>YnZh(fjsha*fw0`mgRgX!<;LlQYdq`8p*CKOjs5{~3b47}1x zIwc~qRh4W_U=qyEqugl}uu0}z8>II07^(eAu(M60Tgd40~LS~(+ds!t0;n*CBT5@Ljvf4;TrBxt&;!*5)9Qro`h+u17Mm^I7AZ(fof}LLQ$GX zlqMAL^#KaA`4Bx&IE>NPTtW{Gyu@VEQ4ol~zdzVt3rzFzgg}u=vwm_kOr=>E!wn^A9NPsIH0tIVs zZs}W~nc4pjrBJ>{GnhEi-}U}aVup1fodm&=7&KoWcj3W#C~l^rqYQjVu1uPbHI3%| zt&5hIXiOU85{(WpFxc!G0-$Q<>Q1I^E}r;;GBZP&P#H{DsyoR9g9ZwDz+^HJ1vk{u zg2UkkClOEsDAW*f24kROfHcyA8A7!Up%~3?SPaeGmqMa4zhQ}gV=+I(ZVG}z7iPwg ze8_$zqLB}c0{F6V6#2)socST&S1j?zwVeGS79uPLvU$A!b==>Ugc{nMeiyIM`7VDF zRj7F%p}4oyoX{2#*}Z6jIb$8rKbuCobeDJJaZ$IU4q~b^JhO4UK(*wUTIH5g*+K_c+y9T8izSWjSYjX3;lW@3`x%ggR&jz#oxscsE55ai}!yD;29UV!Wj^k4)=p6Ed45UUMru?U$gheHo zd;RfmpZ4nbW1Zet?o{I$9Yov+{}d9SskdmgZnTiqI^P`~(M&4@g|3EW#TMAF8WUS< zeloWv3 zVJ=&)28^%17>Z@*=2vSZCe&Xn<=P2$=cpfR8B5IX=W(9mj5{ReE(SpgE7cPPq4vt9 zh#MI*ieP(XWAG`yAm|7PkOTvwj{+U9~X6Tuh5-i@(^Au7wre|zQ?A`v)5JPdr5&@kR!qeAU%oyM-x!r? z>}g9YT0=G_(~)kSQdp1DMKjfj>8s$J`O%RovBs*V#i=9y;d%u{A7thsIs34#T@%kA z-)o3jt|X*K=cxT$X?sU{RYtyIv2>T)c9v7^C{<_5#H{5wEjQ(G#6e(UPhD$T?AY^# zJ!1po5&mw{^UhE7Ls4NyrcJAY%F=4vVy}RUW55%)?s_f8-DU}i{%sj|h)hdzYtCBmK)@eItH+c8tINGx=4=0^RxnWoQ#WVQh zUV;bKr(@hm;u#SRH>ai`VxQGcM9PR7v__s~6L9;|1A{Z)QPb2)w+Qv8>_3yiN?Dzj zOKGjL>F3aMBRfAf7CbOGG0{`-XH#2MFVj2o#vmiRLieYXRcY7|$i@V9t~t3p*g1-o zK%9dnt3IL;YZliX8kAeBP30;q1>4BH_pknaA3OMab4_%}iiYA^^HOHE@4E>&G76BLPw#^rv_I4-7nWE*l>v1|b za1h}XE29gU9BjW6Ns2~vP}F6y9;fuuj8rP;{_xW}6|kSv(HobpebemP07u+;SRYlT8cIMZ2_Tl2nxzoQ6xFw+zzx;yyHOuppq@Z0%a0JE3Y;}+R!r5S7SkxcOU|SyWv#@ahStRTtuw$K3#?(gZJ{f%nA7O# z#-Ng{)xPZF+66W%aH_T<3o_ihZ`!+{ENIr+HuUVM#L+poYjrDI28uG<&UfKjFuZg9 z8uHX@Ox3c6IE_Z81|qcrMy__B?cDZ&J*QUEzn9y~GZ$GIu5R z>tG8b&}S`L?6x;&2AjRsELe78%ZG*pZ~WH$H(s`FD3^>qos9grxV_iS?$(-jMQ|4s z7F5D9EFxqD#tlkvqFZWPM(ccC-pu0JMT@a0&$~Fzp5|*@P^_-UM3hxHo*ayQ>dHCa zRNJxD6o+|t==Sgg?(AFJmNAz7fv$C?LEIIHv))Si3sW1+t<)h8>AAh^*syLjJoE~r zA(Un`1J5p(l6q0EGyArpO@brFT2u7A6T9T3vGT5_c!~m7O06J|6%$sPYYCrCnTcgU}4?1;@L^*Jm3LwWw{Ivf8$9) z>|LMp`Hu^Bwzm9UPiOWOmxQ!VcV&bhIxc`teY#6{JaTNGxCrNq?GxX$f65uTzkcqg aEg}J-NaJOdXGb@GjZ6&9F@ Date: Tue, 27 Jul 2021 23:21:00 +0200 Subject: [PATCH 04/11] Use Active Storage to validate custom image size The same way we're handling images. --- app/models/site_customization/image.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/site_customization/image.rb b/app/models/site_customization/image.rb index bb6859c2a..49479837a 100644 --- a/app/models/site_customization/image.rb +++ b/app/models/site_customization/image.rb @@ -50,14 +50,16 @@ class SiteCustomization::Image < ApplicationRecord def check_image return unless image? - dimensions = Paperclip::Geometry.from_file(image.queued_for_write[:original].path) + storage_image.analyze unless storage_image.analyzed? + width = storage_image.metadata[:width] + height = storage_image.metadata[:height] if name == "logo_header" - errors.add(:image, :image_width, required_width: required_width) unless dimensions.width <= required_width + errors.add(:image, :image_width, required_width: required_width) unless width <= required_width else - errors.add(:image, :image_width, required_width: required_width) unless dimensions.width == required_width + errors.add(:image, :image_width, required_width: required_width) unless width == required_width end - errors.add(:image, :image_height, required_height: required_height) unless dimensions.height == required_height + errors.add(:image, :image_height, required_height: required_height) unless height == required_height end end From 600f5c35e9a721c5d8a074bde29b6136efa73ea9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 27 Jul 2021 23:50:43 +0200 Subject: [PATCH 05/11] Use file_validators instead of Papeclip validations 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. --- Gemfile | 1 + Gemfile.lock | 4 ++++ app/models/ckeditor/picture.rb | 14 +++++++++++--- app/models/site_customization/image.rb | 4 +++- config/i18n-tasks.yml | 1 + config/locales/en/general.yml | 1 + config/locales/es/general.yml | 1 + 7 files changed, 22 insertions(+), 4 deletions(-) diff --git a/Gemfile b/Gemfile index 28833049f..f801c109d 100644 --- a/Gemfile +++ b/Gemfile @@ -19,6 +19,7 @@ gem "dalli", "~> 2.7.11" gem "delayed_job_active_record", "~> 4.1.6" gem "devise", "~> 4.8.0" gem "devise-security", "~> 0.16.0" +gem "file_validators", "~> 3.0.0" gem "font-awesome-sass", "~> 5.15.1" # Remember to update vendor/assets/images/fontawesome when updating this gem gem "foundation-rails", "~> 6.6.2.0" gem "foundation_rails_helper", "~> 4.0.0" diff --git a/Gemfile.lock b/Gemfile.lock index 1ea2c5adb..59d23bcca 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -250,6 +250,9 @@ GEM faraday-patron (1.0.0) faraday-rack (1.0.0) ffi (1.15.4) + file_validators (3.0.0) + activemodel (>= 3.2) + mime-types (>= 1.0) font-awesome-sass (5.15.1) sassc (>= 1.11) foundation-rails (6.6.2.0) @@ -723,6 +726,7 @@ DEPENDENCIES erb_lint (~> 0.0.37) factory_bot_rails (~> 6.2.0) faker (~> 2.18.0) + file_validators (~> 3.0.0) font-awesome-sass (~> 5.15.1) foundation-rails (~> 6.6.2.0) foundation_rails_helper (~> 4.0.0) diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index bcce7ee99..8b0846143 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -6,11 +6,19 @@ class Ckeditor::Picture < Ckeditor::Asset path: ":rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension", styles: { content: "800>", thumb: "118x100#" } - validates_attachment_presence :data - validates_attachment_size :data, less_than: 2.megabytes - validates_attachment_content_type :data, content_type: /\Aimage/ + do_not_validate_attachment_file_type :data + validate :attachment_presence + validates :data, file_content_type: { allow: /^image\/.*/ }, file_size: { less_than: 2.megabytes } def url_content url(:content) end + + private + + def attachment_presence + unless data.present? + errors.add(:data, I18n.t("errors.messages.blank")) + end + end end diff --git a/app/models/site_customization/image.rb b/app/models/site_customization/image.rb index 49479837a..8648cd046 100644 --- a/app/models/site_customization/image.rb +++ b/app/models/site_customization/image.rb @@ -14,7 +14,9 @@ class SiteCustomization::Image < ApplicationRecord has_attachment :image validates :name, presence: true, uniqueness: true, inclusion: { in: VALID_IMAGES.keys } - validates_attachment_content_type :image, content_type: ["image/png", "image/jpeg"] + do_not_validate_attachment_file_type :image + validates :image, file_content_type: { allow: ["image/png", "image/jpeg"] } + validate :check_image def self.all_images diff --git a/config/i18n-tasks.yml b/config/i18n-tasks.yml index b5932360a..8fe6467f7 100644 --- a/config/i18n-tasks.yml +++ b/config/i18n-tasks.yml @@ -169,6 +169,7 @@ ignore_unused: - "admin.dashboard.administrator_tasks.index.filter*" - "admin.dashboard.actions.index.default.*" - "admin.users.index.filter*" + - "errors.messages.allowed_file_content_types" - "moderation.comments.index.filter*" - "moderation.comments.index.orders.*" - "moderation.debates.index.filter*" diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 01477295b..049f090f7 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -150,6 +150,7 @@ en: submit_button: Save changes errors: messages: + allowed_file_content_types: "content type must be one of %{types}" user_not_found: User not found invalid_date_range: "Invalid date range" form: diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index c76541d7e..bb1ecca96 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -150,6 +150,7 @@ es: submit_button: Guardar cambios errors: messages: + allowed_file_content_types: "el tipo de contenido debe ser uno de los siguientes: %{types}" user_not_found: Usuario no encontrado invalid_date_range: "El rango de fechas no es válido" form: From 8c82ff290b23757cbf44ea9388bf0417c52199af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Jul 2021 00:25:02 +0200 Subject: [PATCH 06/11] Handle CKEditor attachments with Active Storage 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 --- app/models/ckeditor/asset.rb | 1 - app/models/ckeditor/picture.rb | 24 +++++++----------------- lib/ckeditor/backend/active_storage.rb | 22 ++++++++-------------- 3 files changed, 15 insertions(+), 32 deletions(-) diff --git a/app/models/ckeditor/asset.rb b/app/models/ckeditor/asset.rb index 494d4fc76..ead24ea61 100644 --- a/app/models/ckeditor/asset.rb +++ b/app/models/ckeditor/asset.rb @@ -1,5 +1,4 @@ class Ckeditor::Asset < ApplicationRecord include Ckeditor::Orm::ActiveRecord::AssetBase include Ckeditor::Backend::ActiveStorage - include Ckeditor::Backend::Paperclip end diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index 8b0846143..eae6dd9de 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -1,24 +1,14 @@ class Ckeditor::Picture < Ckeditor::Asset - include HasAttachment + attr_accessor :data + has_one_attached :storage_data - has_attachment :data, - url: "/ckeditor_assets/pictures/:id/:style_:basename.:extension", - path: ":rails_root/public/ckeditor_assets/pictures/:id/:style_:basename.:extension", - styles: { content: "800>", thumb: "118x100#" } - - do_not_validate_attachment_file_type :data - validate :attachment_presence - validates :data, file_content_type: { allow: /^image\/.*/ }, file_size: { less_than: 2.megabytes } + validates :storage_data, file_content_type: { allow: /^image\/.*/ }, file_size: { less_than: 2.megabytes } def url_content - url(:content) + rails_representation_url(storage_data.variant(resize: "800>").processed, only_path: true) end - private - - def attachment_presence - unless data.present? - errors.add(:data, I18n.t("errors.messages.blank")) - end - end + def url_thumb + rails_representation_url(storage_data.variant(resize: "118x100").processed, only_path: true) + end end diff --git a/lib/ckeditor/backend/active_storage.rb b/lib/ckeditor/backend/active_storage.rb index 7ce8aa611..8ff442443 100644 --- a/lib/ckeditor/backend/active_storage.rb +++ b/lib/ckeditor/backend/active_storage.rb @@ -14,9 +14,9 @@ module Ckeditor module ClassMethods def self.extended(base) base.class_eval do - before_save :apply_data + before_validation :apply_data validate do - if data.nil? || storage_file.nil? + if data.nil? || file.nil? errors.add(:data, :not_data_present, message: "data must be present") end end @@ -46,25 +46,19 @@ module Ckeditor protected - def storage_file - @storage_file ||= storage_data + def file + @file ||= storage_data end def blob - @blob ||= ::ActiveStorage::Blob.find(storage_file.attachment.blob_id) + @blob ||= ::ActiveStorage::Blob.find(file.attachment.blob_id) end def apply_data - non_paperclip_data = if data.is_a?(::Paperclip::Attachment) - file.instance_variable_get("@target") - else - data - end - - if non_paperclip_data.is_a?(Ckeditor::Http::QqFile) - storage_data.attach(io: non_paperclip_data, filename: non_paperclip_data.original_filename) + if data.is_a?(Ckeditor::Http::QqFile) + storage_data.attach(io: data, filename: data.original_filename) else - storage_data.attach(non_paperclip_data) + storage_data.attach(data) end self.data_file_name = storage_data.blob.filename From ca7f2bc9d54f30254539e93a26681130a11394fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Jul 2021 00:57:12 +0200 Subject: [PATCH 07/11] Fix deleted file case in CKEditor We were getting an error when browsing the server if one file had been deleted. --- app/models/ckeditor/picture.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ckeditor/picture.rb b/app/models/ckeditor/picture.rb index eae6dd9de..dc71277ea 100644 --- a/app/models/ckeditor/picture.rb +++ b/app/models/ckeditor/picture.rb @@ -5,10 +5,10 @@ class Ckeditor::Picture < Ckeditor::Asset validates :storage_data, file_content_type: { allow: /^image\/.*/ }, file_size: { less_than: 2.megabytes } def url_content - rails_representation_url(storage_data.variant(resize: "800>").processed, only_path: true) + rails_representation_url(storage_data.variant(resize: "800>"), only_path: true) end def url_thumb - rails_representation_url(storage_data.variant(resize: "118x100").processed, only_path: true) + rails_representation_url(storage_data.variant(resize: "118x100"), only_path: true) end end From 7212657c02121ea649b3fabf7fd69e9fd6f476d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Jul 2021 01:55:54 +0200 Subject: [PATCH 08/11] Remove Paperclip and use just Active Storage --- Gemfile | 1 - Gemfile.lock | 10 -- .../attachable/fields_component.html.erb | 2 +- app/components/attachable/fields_component.rb | 2 +- app/controllers/direct_uploads_controller.rb | 4 +- app/helpers/documents_helper.rb | 2 +- app/helpers/welcome_helper.rb | 2 +- app/models/concerns/attachable.rb | 36 ++---- app/models/concerns/has_attachment.rb | 26 ++-- app/models/direct_upload.rb | 4 +- app/models/document.rb | 18 --- app/models/image.rb | 22 +--- app/models/site_customization/image.rb | 16 ++- .../admin/milestones/_milestones.html.erb | 2 +- .../poll/questions/answers/documents.html.erb | 4 +- .../documents/index.html.erb | 6 +- .../site_customization/images/index.html.erb | 2 +- app/views/dashboard/_document.html.erb | 2 +- app/views/documents/_document.html.erb | 2 +- app/views/milestones/_milestone.html.erb | 2 +- app/views/polls/_gallery.html.erb | 4 +- app/views/polls/show.html.erb | 2 +- config/initializers/paperclip.rb | 13 -- lib/tasks/active_storage.rake | 101 +-------------- lib/tasks/consul.rake | 8 +- spec/controllers/proposals_controller_spec.rb | 34 ------ spec/lib/tasks/active_storage_spec.rb | 115 +++++++----------- spec/lib/tasks/files_spec.rb | 8 +- spec/models/direct_upload_spec.rb | 4 +- spec/models/document_spec.rb | 9 +- spec/models/image_spec.rb | 9 +- spec/models/site_customization/image_spec.rb | 9 +- .../site_customization/documents_spec.rb | 2 +- 33 files changed, 128 insertions(+), 355 deletions(-) delete mode 100644 config/initializers/paperclip.rb diff --git a/Gemfile b/Gemfile index f801c109d..8dde556bb 100644 --- a/Gemfile +++ b/Gemfile @@ -40,7 +40,6 @@ gem "omniauth-facebook", "~> 8.0.0" gem "omniauth-google-oauth2", "~> 1.0.0" gem "omniauth-rails_csrf_protection", "~> 1.0.0" gem "omniauth-twitter", "~> 1.4.0" -gem "paperclip", "~> 6.1.0" gem "paranoia", "~> 2.4.3" gem "pg", "~> 1.2.3" gem "pg_search", "~> 2.3.5" diff --git a/Gemfile.lock b/Gemfile.lock index 59d23bcca..efa05e5f6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -373,9 +373,6 @@ GEM mime-types (3.3.1) mime-types-data (~> 3.2015) mime-types-data (3.2021.0704) - mimemagic (0.3.10) - nokogiri (~> 1) - rake mini_magick (4.11.0) mini_mime (1.1.0) mini_portile2 (2.6.1) @@ -430,12 +427,6 @@ GEM omniauth-oauth (~> 1.1) rack orm_adapter (0.5.0) - paperclip (6.1.0) - activemodel (>= 4.2.0) - activesupport (>= 4.2.0) - mime-types - mimemagic (~> 0.3.0) - terrapin (~> 0.6.0) parallel (1.20.1) paranoia (2.4.3) activerecord (>= 4.0, < 6.2) @@ -753,7 +744,6 @@ DEPENDENCIES omniauth-google-oauth2 (~> 1.0.0) omniauth-rails_csrf_protection (~> 1.0.0) omniauth-twitter (~> 1.4.0) - paperclip (~> 6.1.0) paranoia (~> 2.4.3) pg (~> 1.2.3) pg_search (~> 2.3.5) diff --git a/app/components/attachable/fields_component.html.erb b/app/components/attachable/fields_component.html.erb index 54f91cb2a..e5e9854a6 100644 --- a/app/components/attachable/fields_component.html.erb +++ b/app/components/attachable/fields_component.html.erb @@ -7,7 +7,7 @@ <%= f.text_field :title, placeholder: t("#{plural_name}.form.title_placeholder") %>
    - <% if attachable.storage_attachment.attached? && attachable.storage_attachment.image? %> + <% if attachable.attachment.attached? && attachable.attachment.image? %> <%= render_image(attachable, :thumb, false) %> <% end %> diff --git a/app/components/attachable/fields_component.rb b/app/components/attachable/fields_component.rb index fac93f7be..ae217456a 100644 --- a/app/components/attachable/fields_component.rb +++ b/app/components/attachable/fields_component.rb @@ -24,7 +24,7 @@ class Attachable::FieldsComponent < ApplicationComponent end def file_name - attachable.storage_attachment.filename if attachable.storage_attachment.attached? + attachable.attachment_file_name end def destroy_link diff --git a/app/controllers/direct_uploads_controller.rb b/app/controllers/direct_uploads_controller.rb index 9bc565a29..ca992cd8e 100644 --- a/app/controllers/direct_uploads_controller.rb +++ b/app/controllers/direct_uploads_controller.rb @@ -15,9 +15,9 @@ class DirectUploadsController < ApplicationController @direct_upload.relation.set_cached_attachment_from_attachment render json: { cached_attachment: @direct_upload.relation.cached_attachment, - filename: @direct_upload.relation.storage_attachment.filename.to_s, + filename: @direct_upload.relation.attachment_file_name, destroy_link: render_destroy_upload_link(@direct_upload), - attachment_url: polymorphic_path(@direct_upload.relation.storage_attachment) } + attachment_url: polymorphic_path(@direct_upload.relation.attachment) } else render json: { errors: @direct_upload.errors[:attachment].join(", ") }, status: :unprocessable_entity diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index 742a88acf..9110e45b8 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -3,7 +3,7 @@ module DocumentsHelper info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}" link_to safe_join([document.title, tag.small("(#{info_text})")], " "), - document.storage_attachment, + document.attachment, target: "_blank", title: t("shared.target_blank") end diff --git a/app/helpers/welcome_helper.rb b/app/helpers/welcome_helper.rb index 0f26b3a73..cb77252d3 100644 --- a/app/helpers/welcome_helper.rb +++ b/app/helpers/welcome_helper.rb @@ -25,7 +25,7 @@ module WelcomeHelper def calculate_image_path(recommended, image_default) if recommended.respond_to?(:image) && recommended.image.present? && - recommended.image.storage_attachment.attached? + recommended.image.attachment.attached? recommended.image.variant(:medium) elsif image_default.present? image_default diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index a8fda86ac..f1a069bcb 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -3,14 +3,12 @@ module Attachable extend ActiveSupport::Concern included do + has_attachment :attachment attr_accessor :cached_attachment - # Disable paperclip security validation due to polymorphic configuration - # Paperclip do not allow to use Procs on valiations definition - do_not_validate_attachment_file_type :attachment validate :attachment_presence - validate :validate_attachment_content_type, if: -> { storage_attachment.attached? } - validate :validate_attachment_size, if: -> { storage_attachment.attached? } + validate :validate_attachment_content_type, if: -> { attachment.attached? } + validate :validate_attachment_size, if: -> { attachment.attached? } before_validation :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } end @@ -22,43 +20,35 @@ module Attachable end def set_cached_attachment_from_attachment - self.cached_attachment = storage_attachment.signed_id + self.cached_attachment = attachment.signed_id end def set_attachment_from_cached_attachment - self.storage_attachment = cached_attachment + self.attachment = cached_attachment + end - if filesystem_storage? - File.open(file_path) do |file| - self.paperclip_attachment = file - end - else - self.paperclip_attachment = URI.parse(cached_attachment).open - end + def attachment_file_name + attachment.filename.to_s if attachment.attached? end def attachment_content_type - storage_attachment.blob.content_type if storage_attachment.attached? + attachment.blob.content_type if attachment.attached? end def attachment_file_size - if storage_attachment.attached? - storage_attachment.blob.byte_size + if attachment.attached? + attachment.blob.byte_size else 0 end end def file_path - ActiveStorage::Blob.service.path_for(storage_attachment.blob.key) + ActiveStorage::Blob.service.path_for(attachment.blob.key) end private - def filesystem_storage? - Paperclip::Attachment.default_options[:storage] == :filesystem - end - def validate_attachment_size if association_class && attachment_file_size > max_file_size.megabytes errors.add(:attachment, I18n.t("#{model_name.plural}.errors.messages.in_between", @@ -77,7 +67,7 @@ module Attachable end def attachment_presence - unless storage_attachment.attached? + unless attachment.attached? errors.add(:attachment, I18n.t("errors.messages.blank")) end end diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb index 8fcafa3ec..accca53c2 100644 --- a/app/models/concerns/has_attachment.rb +++ b/app/models/concerns/has_attachment.rb @@ -2,25 +2,17 @@ module HasAttachment extend ActiveSupport::Concern class_methods do - def has_attachment(attribute, paperclip_options = {}) - has_one_attached :"storage_#{attribute}" - - define_method :"storage_#{attribute}=" do |file| - if file.is_a?(IO) - send(:"storage_#{attribute}").attach(io: file, filename: File.basename(file.path)) - elsif file.nil? - send(:"storage_#{attribute}").detach - else - send(:"storage_#{attribute}").attach(file) - end - end - - has_attached_file attribute, paperclip_options - alias_method :"paperclip_#{attribute}=", :"#{attribute}=" + def has_attachment(attribute) + has_one_attached attribute define_method :"#{attribute}=" do |file| - send(:"storage_#{attribute}=", file) - send(:"paperclip_#{attribute}=", file) + if file.is_a?(IO) + send(attribute).attach(io: file, filename: File.basename(file.path)) + elsif file.nil? + send(attribute).detach + else + send(attribute).attach(file) + end end end end diff --git a/app/models/direct_upload.rb b/app/models/direct_upload.rb index 66a9d45ab..cd562ca07 100644 --- a/app/models/direct_upload.rb +++ b/app/models/direct_upload.rb @@ -34,7 +34,7 @@ class DirectUpload end def save_attachment - @relation.storage_attachment.blob.save! + @relation.attachment.blob.save! end def persisted? @@ -53,7 +53,7 @@ class DirectUpload def relation_attributtes { - storage_attachment: @attachment, + attachment: @attachment, cached_attachment: @cached_attachment, user: @user } diff --git a/app/models/document.rb b/app/models/document.rb index 4b9349f3e..9a48ed615 100644 --- a/app/models/document.rb +++ b/app/models/document.rb @@ -1,11 +1,6 @@ class Document < ApplicationRecord include Attachable - has_attachment :attachment, url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", - hash_data: ":class/:style/:custom_hash_data", - use_timestamp: false, - hash_secret: Rails.application.secrets.secret_key_base - belongs_to :user belongs_to :documentable, polymorphic: true, touch: true @@ -16,23 +11,10 @@ class Document < ApplicationRecord scope :admin, -> { where(admin: true) } - Paperclip.interpolates :custom_hash_data do |attachment, _style| - attachment.instance.custom_hash_data(attachment) - end - def self.humanized_accepted_content_types Setting.accepted_content_types_for("documents").join(", ") end - def custom_hash_data(attachment) - original_filename = if attachment.instance.persisted? - attachment.instance.title - else - attachment.instance.attachment_file_name - end - "#{attachment.instance.user_id}/#{original_filename}" - end - def humanized_content_type attachment_content_type.split("/").last.upcase end diff --git a/app/models/image.rb b/app/models/image.rb index 098515114..eda89ecc8 100644 --- a/app/models/image.rb +++ b/app/models/image.rb @@ -1,16 +1,6 @@ class Image < ApplicationRecord include Attachable - has_attachment :attachment, styles: { - large: "x#{Setting["uploads.images.min_height"]}", - medium: "300x300#", - thumb: "140x245#" - }, - url: "/system/:class/:attachment/:id_partition/:style/:hash.:extension", - hash_data: ":class/:style", - use_timestamp: false, - hash_secret: Rails.application.secrets.secret_key_base - def self.styles { large: { resize: "x#{Setting["uploads.images.min_height"]}" }, @@ -27,7 +17,7 @@ class Image < ApplicationRecord validates :user_id, presence: true validates :imageable_id, presence: true, if: -> { persisted? } validates :imageable_type, presence: true, if: -> { persisted? } - validate :validate_image_dimensions, if: -> { storage_attachment.attached? && storage_attachment.new_record? } + validate :validate_image_dimensions, if: -> { attachment.attached? && attachment.new_record? } def self.max_file_size Setting["uploads.images.max_size"].to_i @@ -51,9 +41,9 @@ class Image < ApplicationRecord def variant(style) if style - storage_attachment.variant(self.class.styles[style]) + attachment.variant(self.class.styles[style]) else - storage_attachment + attachment end end @@ -71,10 +61,10 @@ class Image < ApplicationRecord if accepted_content_types.include?(attachment_content_type) return true if imageable_class == Widget::Card - storage_attachment.analyze unless storage_attachment.analyzed? + attachment.analyze unless attachment.analyzed? - width = storage_attachment.metadata[:width] - height = storage_attachment.metadata[:height] + width = attachment.metadata[:width] + height = attachment.metadata[:height] min_width = Setting["uploads.images.min_width"].to_i min_height = Setting["uploads.images.min_height"].to_i errors.add(:attachment, :min_image_width, required_min_width: min_width) if width < min_width diff --git a/app/models/site_customization/image.rb b/app/models/site_customization/image.rb index 8648cd046..c9c41ed6a 100644 --- a/app/models/site_customization/image.rb +++ b/app/models/site_customization/image.rb @@ -14,9 +14,7 @@ class SiteCustomization::Image < ApplicationRecord has_attachment :image validates :name, presence: true, uniqueness: true, inclusion: { in: VALID_IMAGES.keys } - do_not_validate_attachment_file_type :image - validates :image, file_content_type: { allow: ["image/png", "image/jpeg"] } - + validates :image, file_content_type: { allow: ["image/png", "image/jpeg"], if: -> { image.attached? }} validate :check_image def self.all_images @@ -40,21 +38,21 @@ class SiteCustomization::Image < ApplicationRecord end def persisted_image - storage_image if persisted_attachment? + image if persisted_attachment? end def persisted_attachment? - storage_image.attachment&.persisted? + image.attachment&.persisted? end private def check_image - return unless image? + return unless image.attached? - storage_image.analyze unless storage_image.analyzed? - width = storage_image.metadata[:width] - height = storage_image.metadata[:height] + image.analyze unless image.analyzed? + width = image.metadata[:width] + height = image.metadata[:height] if name == "logo_header" errors.add(:image, :image_width, required_width: required_width) unless width <= required_width diff --git a/app/views/admin/milestones/_milestones.html.erb b/app/views/admin/milestones/_milestones.html.erb index ac4d599a8..43479ce86 100644 --- a/app/views/admin/milestones/_milestones.html.erb +++ b/app/views/admin/milestones/_milestones.html.erb @@ -47,7 +47,7 @@ <% if milestone.documents.present? %> <% milestone.documents.each do |document| %> <%= link_to document.title, - document.storage_attachment, + document.attachment, target: "_blank", rel: "nofollow" %>
    <% end %> diff --git a/app/views/admin/poll/questions/answers/documents.html.erb b/app/views/admin/poll/questions/answers/documents.html.erb index c322bcdaa..4c0396849 100644 --- a/app/views/admin/poll/questions/answers/documents.html.erb +++ b/app/views/admin/poll/questions/answers/documents.html.erb @@ -33,7 +33,7 @@ <% @answer.documents.each do |document| %> - <%= link_to document.title, document.storage_attachment %> + <%= link_to document.title, document.attachment %> <%= render Admin::TableActionsComponent.new(document, @@ -42,7 +42,7 @@ ) do |actions| %> <%= actions.action(:download, text: t("documents.buttons.download_document"), - path: document.storage_attachment, + path: document.attachment, target: "_blank", rel: "nofollow") %> diff --git a/app/views/admin/site_customization/documents/index.html.erb b/app/views/admin/site_customization/documents/index.html.erb index f7a8e6409..4f3f93497 100644 --- a/app/views/admin/site_customization/documents/index.html.erb +++ b/app/views/admin/site_customization/documents/index.html.erb @@ -21,9 +21,9 @@ <% @documents.each do |document| %> <%= document.title %> - <%= document.storage_attachment.content_type %> - <%= number_to_human_size(document.storage_attachment.byte_size) %> - <%= link_to document.title, document.storage_attachment, target: :blank %> + <%= document.attachment.content_type %> + <%= number_to_human_size(document.attachment.byte_size) %> + <%= link_to document.title, document.attachment, target: :blank %>
    <%= render Admin::TableActionsComponent.new( diff --git a/app/views/admin/site_customization/images/index.html.erb b/app/views/admin/site_customization/images/index.html.erb index 43da53e9e..354c3626c 100644 --- a/app/views/admin/site_customization/images/index.html.erb +++ b/app/views/admin/site_customization/images/index.html.erb @@ -16,7 +16,7 @@ <%= form_for([:admin, image], html: { id: "edit_#{dom_id(image)}" }) do |f| %>
    - <%= image_tag image.storage_image if image.persisted_attachment? %> + <%= image_tag image.image if image.persisted_attachment? %> <%= f.file_field :image, label: false %>
    diff --git a/app/views/dashboard/_document.html.erb b/app/views/dashboard/_document.html.erb index e91efb98b..d59177a30 100644 --- a/app/views/dashboard/_document.html.erb +++ b/app/views/dashboard/_document.html.erb @@ -1,5 +1,5 @@

    - <%= link_to document.storage_attachment, target: "_blank" do %> + <%= link_to document.attachment, target: "_blank" do %> <%= document.title %> (<%= document.humanized_content_type %>  |  diff --git a/app/views/documents/_document.html.erb b/app/views/documents/_document.html.erb index 67c61216b..977e3acc4 100644 --- a/app/views/documents/_document.html.erb +++ b/app/views/documents/_document.html.erb @@ -1,6 +1,6 @@

  • <%= link_to t("documents.buttons.download_document"), - document.storage_attachment, target: "_blank", + document.attachment, target: "_blank", rel: "nofollow", class: "button hollow medium float-right" %> <%= document.title %> diff --git a/app/views/milestones/_milestone.html.erb b/app/views/milestones/_milestone.html.erb index ce8ea12bc..52f8e6956 100644 --- a/app/views/milestones/_milestone.html.erb +++ b/app/views/milestones/_milestone.html.erb @@ -37,7 +37,7 @@ <% milestone.documents.each do |document| %> <%= link_to document.title, - document.storage_attachment, + document.attachment, target: "_blank", rel: "nofollow" %>
    diff --git a/app/views/polls/_gallery.html.erb b/app/views/polls/_gallery.html.erb index e62827908..43ac86cf6 100644 --- a/app/views/polls/_gallery.html.erb +++ b/app/views/polls/_gallery.html.erb @@ -18,8 +18,8 @@ <% answer.images.reverse.each_with_index do |image, index| %>
  • - <%= link_to image.storage_attachment, target: "_blank" do %> - <%= image_tag image.storage_attachment, + <%= link_to image.attachment, target: "_blank" do %> + <%= image_tag image.attachment, class: "orbit-image", alt: image.title.unicode_normalize %> <% end %> diff --git a/app/views/polls/show.html.erb b/app/views/polls/show.html.erb index 69272441a..1aa0e89ec 100644 --- a/app/views/polls/show.html.erb +++ b/app/views/polls/show.html.erb @@ -86,7 +86,7 @@ <% answer.documents.each do |document| %> <%= link_to document.title, - document.storage_attachment, + document.attachment, target: "_blank", rel: "nofollow" %>
    <% end %> diff --git a/config/initializers/paperclip.rb b/config/initializers/paperclip.rb deleted file mode 100644 index 97a4079d9..000000000 --- a/config/initializers/paperclip.rb +++ /dev/null @@ -1,13 +0,0 @@ -class Paperclip::UrlGenerator - private - - # Code copied from the paperclip gem, only replacing - # `URI.escape` with `URI::DEFAULT_PARSER.escape` - def escape_url(url) - if url.respond_to?(:escape) - url.escape - else - URI::DEFAULT_PARSER.escape(url).gsub(escape_regex) { |m| "%#{m.ord.to_s(16).upcase}" } - end - end -end diff --git a/lib/tasks/active_storage.rake b/lib/tasks/active_storage.rake index 548d5cfc9..2f65e829b 100644 --- a/lib/tasks/active_storage.rake +++ b/lib/tasks/active_storage.rake @@ -1,98 +1,9 @@ -# This code is based on Thoughtbot's guide to migrating from Paperclip -# to Active Storage: -# https://github.com/thoughtbot/paperclip/blob/master/MIGRATING.md namespace :active_storage do - desc "Copy paperclip's attachment database columns to active storage" - task migrate_from_paperclip: :environment do - logger = ApplicationLogger.new - connection = ActiveRecord::Base.connection.raw_connection - statement_name = "active_storage_statement" - - unless connection.exec("SELECT COUNT(*) FROM pg_prepared_statements WHERE name='#{statement_name}'").each_row.to_a[0][0] > 0 - connection.prepare(statement_name, <<-SQL) - with rows as( - INSERT INTO active_storage_blobs ( - key, filename, content_type, metadata, byte_size, checksum, created_at - ) VALUES ($1, $2, $3, '{}', $4, $5, $6) RETURNING id - ) - INSERT INTO active_storage_attachments ( - name, record_type, record_id, blob_id, created_at - ) VALUES ($7, $8, $9, (SELECT id FROM rows), $10) - SQL - end - - Rails.application.eager_load! - models = ActiveRecord::Base.descendants.reject(&:abstract_class?) - paperclip_storage = Paperclip::Attachment.default_options[:storage] - - ActiveRecord::Base.transaction do - models.each do |model| - next if model.name == "OldPassword" && !model.table_exists? - - attachments = model.column_names.map do |c| - if c =~ /(.+)_file_name$/ - $1 - end - end.compact - - if attachments.blank? - next - end - - model.find_each.each do |instance| - attachments.each do |attachment| - next if instance.send(:"storage_#{attachment}").attached? - - source = if paperclip_storage == :filesystem - instance.send(attachment).path - else - instance.send(attachment).url - end - - next if source.blank? || source == "/images/original/missing.png" - - file = if paperclip_storage == :filesystem - File.read(source) if File.exist?(source) - else - Net::HTTP.get(URI(source)) - end - - connection.exec_prepared( - statement_name, [ - SecureRandom.uuid, # Alternatively instance.send("#{attachment}_file_name"), - instance.send("#{attachment}_file_name"), - instance.send("#{attachment}_content_type"), - instance.send("#{attachment}_file_size"), - file && Digest::MD5.base64digest(file) || SecureRandom.hex(32), - instance.updated_at.iso8601, - "storage_#{attachment}", - model.name, - instance.id, - instance.updated_at.iso8601 - ]) - end - end - end - end - - ActiveStorage::Attachment.find_each do |attachment| - blob = attachment.blob - - next if blob.service.exist?(blob.key) || !attachment.record - - name = attachment.name.delete_prefix("storage_") - paperclip_attachment = attachment.record.send(name) - - next unless paperclip_attachment.exists? - - source_file = if paperclip_storage == :filesystem - paperclip_attachment.path - else - URI.parse(paperclip_attachment.url).open.read - end - - logger.info "Copying #{paperclip_attachment.url} to active storage" - blob.service.upload(blob.key, source_file) - end + desc "Updates ActiveStorage::Attachment old records created when we were also using paperclip" + task remove_paperclip_compatibility_in_existing_attachments: :environment do + ApplicationLogger.new.info "Updating Active Storage attachment records" + ActiveStorage::Attachment.where("name ILIKE 'storage_%'") + .where.not(record_type: "Ckeditor::Asset") + .update_all("name = replace(name, 'storage_', '')") end end diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index a45012ab5..6bd19134b 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -2,10 +2,10 @@ namespace :consul do desc "Runs tasks needed to upgrade to the latest version" task execute_release_tasks: ["settings:rename_setting_keys", "settings:add_new_settings", - "execute_release_1.4.0_tasks"] + "execute_release_1.5.0_tasks"] - desc "Runs tasks needed to upgrade from 1.3.0 to 1.4.0" - task "execute_release_1.4.0_tasks": [ - "active_storage:migrate_from_paperclip" + desc "Runs tasks needed to upgrade from 1.4.0 to 1.5.0" + task "execute_release_1.5.0_tasks": [ + "active_storage:remove_paperclip_compatibility_in_existing_attachments" ] end diff --git a/spec/controllers/proposals_controller_spec.rb b/spec/controllers/proposals_controller_spec.rb index ae586123b..05e1cc279 100644 --- a/spec/controllers/proposals_controller_spec.rb +++ b/spec/controllers/proposals_controller_spec.rb @@ -8,38 +8,4 @@ describe ProposalsController do expect { get :index }.to raise_exception(FeatureFlags::FeatureDisabled) end end - - describe "PUT update" do - let(:file) { Rails.root.join("spec/hacked") } - - before do - File.delete(file) if File.exist?(file) - InvisibleCaptcha.timestamp_enabled = false - end - - after { InvisibleCaptcha.timestamp_enabled = true } - - it "ignores malicious cached attachments with remote storages" do - allow_any_instance_of(Image).to receive(:filesystem_storage?).and_return(false) - user = create(:user) - proposal = create(:proposal, author: user) - sign_in user - - begin - put :update, params: { - id: proposal, - proposal: { - image_attributes: { - title: "Hacked!", - user_id: user.id, - cached_attachment: "| touch #{file}" - } - } - } - rescue StandardError - ensure - expect(file).not_to exist - end - end - end end diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index 2a8043556..9b2edc68d 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -1,90 +1,67 @@ require "rails_helper" describe "active storage tasks" do - describe "migrate_from_paperclip" do + describe "remove_paperclip_compatibility_in_existing_attachments" do let(:run_rake_task) do - Rake::Task["active_storage:migrate_from_paperclip"].reenable - Rake.application.invoke_task("active_storage:migrate_from_paperclip") + Rake::Task["active_storage:remove_paperclip_compatibility_in_existing_attachments"].reenable + Rake.application.invoke_task("active_storage:remove_paperclip_compatibility_in_existing_attachments") end - let(:storage_root) { ActiveStorage::Blob.service.root } - before { FileUtils.rm_rf storage_root } + it "updates old regular attachments" do + document = create(:document) + image = create(:image) + site_customization_image = create(:site_customization_image) - it "migrates records and attachments" do - document = build(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) - document.save(validate: false) + document.attachment.attachment.update_column(:name, "storage_attachment") + image.attachment.attachment.update_column(:name, "storage_attachment") + site_customization_image.image.attachment.update_column(:name, "storage_image") - expect(ActiveStorage::Attachment.count).to eq 0 - expect(ActiveStorage::Blob.count).to eq 0 - expect(test_storage_file_paths.count).to eq 0 + document.reload + image.reload + site_customization_image.reload + + expect(document.attachment.attachment).to be nil + expect(image.attachment.attachment).to be nil + expect(site_customization_image.image.attachment).to be nil run_rake_task document.reload + image.reload + site_customization_image.reload - expect(ActiveStorage::Attachment.count).to eq 1 - expect(ActiveStorage::Blob.count).to eq 1 - expect(document.storage_attachment.filename).to eq "clippy.pdf" - expect(test_storage_file_paths.count).to eq 1 - expect(storage_file_path(document)).to eq test_storage_file_paths.first + expect(document.attachment.attachment.name).to eq "attachment" + expect(image.attachment.attachment.name).to eq "attachment" + expect(site_customization_image.reload.image.attachment.name).to eq "image" end - it "migrates records with deleted files ignoring the files" do - document = build(:document, - attachment: nil, - paperclip_attachment: File.new("spec/fixtures/files/clippy.pdf")) - document.save(validate: false) - FileUtils.rm(document.attachment.path) + it "does not modify old ckeditor attachments" do + image = Ckeditor::Picture.create!(data: Rack::Test::UploadedFile.new("spec/fixtures/files/clippy.png")) + + expect(image.storage_data.attachment.name).to eq "storage_data" + + run_rake_task + image.reload + + expect(image.storage_data.attachment.name).to eq "storage_data" + end + + it "does not modify new regular attachments" do + document = create(:document) + image = create(:image) + site_customization_image = create(:site_customization_image) + + expect(document.attachment.attachment.name).to eq "attachment" + expect(image.attachment.attachment.name).to eq "attachment" + expect(site_customization_image.image.attachment.name).to eq "image" run_rake_task document.reload + image.reload + site_customization_image.reload - expect(ActiveStorage::Attachment.count).to eq 1 - expect(ActiveStorage::Blob.count).to eq 1 - expect(document.storage_attachment.filename).to eq "clippy.pdf" - expect(test_storage_file_paths.count).to eq 0 - end - - it "does not migrate already migrated records" do - document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) - - migrated_file = test_storage_file_paths.first - attachment_id = document.storage_attachment.attachment.id - blob_id = document.storage_attachment.blob.id - - run_rake_task - document.reload - - expect(ActiveStorage::Attachment.count).to eq 1 - expect(ActiveStorage::Blob.count).to eq 1 - expect(document.storage_attachment.attachment.id).to eq attachment_id - expect(document.storage_attachment.blob.id).to eq blob_id - - expect(test_storage_file_paths.count).to eq 1 - expect(storage_file_path(document)).to eq migrated_file - expect(test_storage_file_paths.first).to eq migrated_file - end - - it "does not migrate files for deleted records" do - document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) - FileUtils.rm storage_file_path(document) - Document.delete_all - - run_rake_task - - expect(ActiveStorage::Attachment.count).to eq 1 - expect(ActiveStorage::Blob.count).to eq 1 - expect(document.storage_attachment.filename).to eq "clippy.pdf" - expect(test_storage_file_paths.count).to eq 0 - end - - def test_storage_file_paths - Dir.glob("#{storage_root}/**/*").select { |file_or_folder| File.file?(file_or_folder) } - end - - def storage_file_path(record) - ActiveStorage::Blob.service.path_for(record.storage_attachment.blob.key) + expect(document.attachment.attachment.name).to eq "attachment" + expect(image.attachment.attachment.name).to eq "attachment" + expect(site_customization_image.reload.image.attachment.name).to eq "image" end end end diff --git a/spec/lib/tasks/files_spec.rb b/spec/lib/tasks/files_spec.rb index 454abe7be..4d55334bb 100644 --- a/spec/lib/tasks/files_spec.rb +++ b/spec/lib/tasks/files_spec.rb @@ -11,8 +11,8 @@ describe "files tasks" do image = build(:image) document = build(:document) - image.storage_attachment.blob.save! - document.storage_attachment.blob.save! + image.attachment.blob.save! + document.attachment.blob.save! travel_to(2.days.from_now) { run_rake_task } @@ -24,8 +24,8 @@ describe "files tasks" do image = build(:image) document = build(:document) - image.storage_attachment.blob.save! - document.storage_attachment.blob.save! + image.attachment.blob.save! + document.attachment.blob.save! travel_to(2.minutes.from_now) { run_rake_task } diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index 7f281cc22..815421180 100644 --- a/spec/models/direct_upload_spec.rb +++ b/spec/models/direct_upload_spec.rb @@ -40,8 +40,8 @@ describe DirectUpload do direct_upload.save_attachment expect(File.exist?(direct_upload.relation.file_path)).to be true - expect(direct_upload.relation.storage_attachment.blob).to be_persisted - expect(direct_upload.relation.storage_attachment.attachment).not_to be_persisted + expect(direct_upload.relation.attachment.blob).to be_persisted + expect(direct_upload.relation.attachment.attachment).not_to be_persisted end end end diff --git a/spec/models/document_spec.rb b/spec/models/document_spec.rb index 2f8cf65fe..af3294e9f 100644 --- a/spec/models/document_spec.rb +++ b/spec/models/document_spec.rb @@ -4,14 +4,11 @@ describe Document do it_behaves_like "document validations", "budget_investment_document" it_behaves_like "document validations", "proposal_document" - it "stores attachments with both Paperclip and Active Storage" do + it "stores attachments with Active Storage" do document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) - expect(document.attachment).to exist - expect(document.attachment_file_name).to eq "clippy.pdf" - - expect(document.storage_attachment).to be_attached - expect(document.storage_attachment.filename).to eq "clippy.pdf" + expect(document.attachment).to be_attached + expect(document.attachment.filename).to eq "clippy.pdf" end context "scopes" do diff --git a/spec/models/image_spec.rb b/spec/models/image_spec.rb index d119c8138..da5b70d46 100644 --- a/spec/models/image_spec.rb +++ b/spec/models/image_spec.rb @@ -4,13 +4,10 @@ describe Image do it_behaves_like "image validations", "budget_investment_image" it_behaves_like "image validations", "proposal_image" - it "stores attachments with both Paperclip and Active Storage" do + it "stores attachments with Active Storage" do image = create(:image, attachment: File.new("spec/fixtures/files/clippy.jpg")) - expect(image.attachment).to exist - expect(image.attachment_file_name).to eq "clippy.jpg" - - expect(image.storage_attachment).to be_attached - expect(image.storage_attachment.filename).to eq "clippy.jpg" + expect(image.attachment).to be_attached + expect(image.attachment.filename).to eq "clippy.jpg" end end diff --git a/spec/models/site_customization/image_spec.rb b/spec/models/site_customization/image_spec.rb index ddd266d0e..c1bb77880 100644 --- a/spec/models/site_customization/image_spec.rb +++ b/spec/models/site_customization/image_spec.rb @@ -1,15 +1,12 @@ require "rails_helper" describe SiteCustomization::Image do - it "stores images with both Paperclip and Active Storage" do + it "stores images with Active Storage" do image = create(:site_customization_image, name: "map", image: File.new("spec/fixtures/files/custom_map.jpg")) - expect(image.image).to exist - expect(image.image_file_name).to eq "custom_map.jpg" - - expect(image.storage_image).to be_attached - expect(image.storage_image.filename).to eq "custom_map.jpg" + expect(image.image).to be_attached + expect(image.image.filename).to eq "custom_map.jpg" end describe "logo" do diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index fe2fe332a..6a2063299 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -18,7 +18,7 @@ describe "Documents", :admin do 1.times { create(:document) } document = Document.first - url = polymorphic_path(document.storage_attachment) + url = polymorphic_path(document.attachment) visit admin_site_customization_documents_path From 5ff66f96cd58a88ca3c6fa326897bf0e14728af0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Jul 2021 02:51:49 +0200 Subject: [PATCH 09/11] Use file_validators to validate attachments 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. --- app/models/concerns/attachable.rb | 39 +++++++++++----------- spec/shared/models/document_validations.rb | 2 +- spec/shared/models/image_validations.rb | 2 +- 3 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/models/concerns/attachable.rb b/app/models/concerns/attachable.rb index f1a069bcb..ac6c1e33a 100644 --- a/app/models/concerns/attachable.rb +++ b/app/models/concerns/attachable.rb @@ -7,8 +7,26 @@ module Attachable attr_accessor :cached_attachment validate :attachment_presence - validate :validate_attachment_content_type, if: -> { attachment.attached? } - validate :validate_attachment_size, if: -> { attachment.attached? } + + validates :attachment, + file_content_type: { + allow: ->(record) { record.accepted_content_types }, + if: -> { association_class && attachment.attached? }, + message: ->(record, *) do + I18n.t("#{record.model_name.plural}.errors.messages.wrong_content_type", + content_type: record.attachment_content_type, + accepted_content_types: record.class.humanized_accepted_content_types) + end + }, + file_size: { + less_than_or_equal_to: ->(record) { record.max_file_size.megabytes }, + if: -> { association_class && attachment.attached? }, + message: ->(record, *) do + I18n.t("#{record.model_name.plural}.errors.messages.in_between", + min: "0 Bytes", + max: "#{record.max_file_size} MB") + end + } before_validation :set_attachment_from_cached_attachment, if: -> { cached_attachment.present? } end @@ -49,23 +67,6 @@ module Attachable private - def validate_attachment_size - if association_class && attachment_file_size > max_file_size.megabytes - errors.add(:attachment, I18n.t("#{model_name.plural}.errors.messages.in_between", - min: "0 Bytes", - max: "#{max_file_size} MB")) - end - end - - def validate_attachment_content_type - if association_class && !accepted_content_types.include?(attachment_content_type) - message = I18n.t("#{model_name.plural}.errors.messages.wrong_content_type", - content_type: attachment_content_type, - accepted_content_types: self.class.humanized_accepted_content_types) - errors.add(:attachment, message) - end - end - def attachment_presence unless attachment.attached? errors.add(:attachment, I18n.t("errors.messages.blank")) diff --git a/spec/shared/models/document_validations.rb b/spec/shared/models/document_validations.rb index 194c8fbba..3594b1a57 100644 --- a/spec/shared/models/document_validations.rb +++ b/spec/shared/models/document_validations.rb @@ -29,7 +29,7 @@ shared_examples "document validations" do |documentable_factory| end it "is not valid for attachments larger than documentable max_file_size definition" do - allow(document).to receive(:attachment_file_size).and_return(maxfilesize.megabytes + 1.byte) + allow(document.attachment).to receive(:byte_size).and_return(maxfilesize.megabytes + 1.byte) max_size_error_message = "must be in between 0 Bytes and #{maxfilesize} MB" expect(document).not_to be_valid diff --git a/spec/shared/models/image_validations.rb b/spec/shared/models/image_validations.rb index 092372550..ed26d0499 100644 --- a/spec/shared/models/image_validations.rb +++ b/spec/shared/models/image_validations.rb @@ -38,7 +38,7 @@ shared_examples "image validations" do |imageable_factory| it "is not valid for attachments larger than imageable max_file_size definition" do larger_size = Setting["uploads.images.max_size"].to_i.megabytes + 1.byte - allow(image).to receive(:attachment_file_size).and_return(larger_size) + allow(image.attachment).to receive(:byte_size).and_return(larger_size) expect(image).not_to be_valid expect(image.errors[:attachment]).to include "must be in between 0 Bytes and 1 MB" From 4f232c3a258be95cd110d67f264e5d8b46582a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 18 Sep 2021 17:20:52 +0200 Subject: [PATCH 10/11] Use the file_fixture helper in tests 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. --- spec/lib/remote_census_api_spec.rb | 2 +- spec/lib/tasks/active_storage_spec.rb | 2 +- spec/models/direct_upload_spec.rb | 8 ++--- spec/models/document_spec.rb | 2 +- spec/models/image_spec.rb | 2 +- .../local_census_records/import_spec.rb | 10 +++--- spec/models/site_customization/image_spec.rb | 8 ++--- spec/shared/models/acts_as_imageable.rb | 8 ++--- spec/shared/models/document_validations.rb | 2 +- spec/shared/models/image_validations.rb | 4 +-- spec/shared/system/nested_documentable.rb | 32 ++++++++----------- spec/shared/system/nested_imageable.rb | 26 +++++++-------- .../common_actions/remote_census_mock.rb | 6 ++-- spec/system/admin/budget_phases_spec.rb | 2 +- .../admin/legislation/processes_spec.rb | 2 +- .../local_census_records/imports_spec.rb | 13 ++++---- .../questions/answers/images/images_spec.rb | 2 +- .../site_customization/documents_spec.rb | 2 +- .../admin/site_customization/images_spec.rb | 14 ++++---- spec/system/admin/widgets/cards_spec.rb | 2 +- spec/system/ckeditor_spec.rb | 2 +- spec/system/legislation/proposals_spec.rb | 2 +- 22 files changed, 73 insertions(+), 80 deletions(-) diff --git a/spec/lib/remote_census_api_spec.rb b/spec/lib/remote_census_api_spec.rb index 2e25b6e1e..616dfdce5 100644 --- a/spec/lib/remote_census_api_spec.rb +++ b/spec/lib/remote_census_api_spec.rb @@ -26,7 +26,7 @@ describe RemoteCensusApi do end describe "request messages" do - let(:valid_response) { File.read("spec/fixtures/files/remote_census_api/valid.xml") } + let(:valid_response) { File.read(file_fixture("remote_census_api/valid.xml")) } def request_with(params) { "request" => params } diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index 9b2edc68d..0f43b39cd 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -35,7 +35,7 @@ describe "active storage tasks" do end it "does not modify old ckeditor attachments" do - image = Ckeditor::Picture.create!(data: Rack::Test::UploadedFile.new("spec/fixtures/files/clippy.png")) + image = Ckeditor::Picture.create!(data: Rack::Test::UploadedFile.new(file_fixture("clippy.png"))) expect(image.storage_data.attachment.name).to eq "storage_data" diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index 815421180..bdf079680 100644 --- a/spec/models/direct_upload_spec.rb +++ b/spec/models/direct_upload_spec.rb @@ -10,10 +10,10 @@ describe DirectUpload do end it "is not valid for different kind of combinations with invalid atttachment content types" do - expect(build(:direct_upload, :proposal, :documents, attachment: File.new("spec/fixtures/files/clippy.png"))).not_to be_valid - expect(build(:direct_upload, :proposal, :image, attachment: File.new("spec/fixtures/files/empty.pdf"))).not_to be_valid - expect(build(:direct_upload, :budget_investment, :documents, attachment: File.new("spec/fixtures/files/clippy.png"))).not_to be_valid - expect(build(:direct_upload, :budget_investment, :image, attachment: File.new("spec/fixtures/files/empty.pdf"))).not_to be_valid + expect(build(:direct_upload, :proposal, :documents, attachment: File.new(file_fixture("clippy.png")))).not_to be_valid + expect(build(:direct_upload, :proposal, :image, attachment: File.new(file_fixture("empty.pdf")))).not_to be_valid + expect(build(:direct_upload, :budget_investment, :documents, attachment: File.new(file_fixture("clippy.png")))).not_to be_valid + expect(build(:direct_upload, :budget_investment, :image, attachment: File.new(file_fixture("empty.pdf")))).not_to be_valid end it "is not valid without resource_type" do diff --git a/spec/models/document_spec.rb b/spec/models/document_spec.rb index af3294e9f..1d7a087a4 100644 --- a/spec/models/document_spec.rb +++ b/spec/models/document_spec.rb @@ -5,7 +5,7 @@ describe Document do it_behaves_like "document validations", "proposal_document" it "stores attachments with Active Storage" do - document = create(:document, attachment: File.new("spec/fixtures/files/clippy.pdf")) + document = create(:document, attachment: File.new(file_fixture("clippy.pdf"))) expect(document.attachment).to be_attached expect(document.attachment.filename).to eq "clippy.pdf" diff --git a/spec/models/image_spec.rb b/spec/models/image_spec.rb index da5b70d46..7022e0b40 100644 --- a/spec/models/image_spec.rb +++ b/spec/models/image_spec.rb @@ -5,7 +5,7 @@ describe Image do it_behaves_like "image validations", "proposal_image" it "stores attachments with Active Storage" do - image = create(:image, attachment: File.new("spec/fixtures/files/clippy.jpg")) + image = create(:image, attachment: File.new(file_fixture("clippy.jpg"))) expect(image.attachment).to be_attached expect(image.attachment.filename).to eq "clippy.jpg" diff --git a/spec/models/local_census_records/import_spec.rb b/spec/models/local_census_records/import_spec.rb index 3aaca8a11..70ec5038b 100644 --- a/spec/models/local_census_records/import_spec.rb +++ b/spec/models/local_census_records/import_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" describe LocalCensusRecords::Import do - let(:base_files_path) { %w[spec fixtures files local_census_records import] } + let(:base_files_path) { "local_census_records/import/" } let(:import) { build(:local_census_records_import) } describe "Validations" do @@ -17,7 +17,7 @@ describe LocalCensusRecords::Import do context "When file extension" do it "is wrong it should not be valid" do - file = Rack::Test::UploadedFile.new("spec/fixtures/files/clippy.gif") + file = Rack::Test::UploadedFile.new(file_fixture("clippy.gif")) import = build(:local_census_records_import, file: file) expect(import).not_to be_valid @@ -25,7 +25,7 @@ describe LocalCensusRecords::Import do it "is csv it should be valid" do path = base_files_path << "valid.csv" - file = Rack::Test::UploadedFile.new(Rails.root.join(*path)) + file = Rack::Test::UploadedFile.new(file_fixture(path)) import = build(:local_census_records_import, file: file) expect(import).to be_valid @@ -35,7 +35,7 @@ describe LocalCensusRecords::Import do context "When file headers" do it "are all missing it should not be valid" do path = base_files_path << "valid-without-headers.csv" - file = Rack::Test::UploadedFile.new(Rails.root.join(*path)) + file = Rack::Test::UploadedFile.new(file_fixture(path)) import = build(:local_census_records_import, file: file) expect(import).not_to be_valid @@ -64,7 +64,7 @@ describe LocalCensusRecords::Import do it "Add invalid local census records to invalid_records array" do path = base_files_path << "invalid.csv" - file = Rack::Test::UploadedFile.new(Rails.root.join(*path)) + file = Rack::Test::UploadedFile.new(file_fixture(path)) import.file = file import.save! diff --git a/spec/models/site_customization/image_spec.rb b/spec/models/site_customization/image_spec.rb index c1bb77880..eef988bd1 100644 --- a/spec/models/site_customization/image_spec.rb +++ b/spec/models/site_customization/image_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" describe SiteCustomization::Image do it "stores images with Active Storage" do image = create(:site_customization_image, name: "map", - image: File.new("spec/fixtures/files/custom_map.jpg")) + image: File.new(file_fixture("custom_map.jpg"))) expect(image.image).to be_attached expect(image.image.filename).to eq "custom_map.jpg" @@ -13,7 +13,7 @@ describe SiteCustomization::Image do it "is valid with a 260x80 image" do image = build(:site_customization_image, name: "logo_header", - image: File.new("spec/fixtures/files/logo_header-260x80.png")) + image: File.new(file_fixture("logo_header-260x80.png"))) expect(image).to be_valid end @@ -21,7 +21,7 @@ describe SiteCustomization::Image do it "is valid with a 223x80 image" do image = build(:site_customization_image, name: "logo_header", - image: File.new("spec/fixtures/files/logo_header.png")) + image: File.new(file_fixture("logo_header.png"))) expect(image).to be_valid end @@ -29,7 +29,7 @@ describe SiteCustomization::Image do it "is not valid with a 400x80 image" do image = build(:site_customization_image, name: "logo_header", - image: File.new("spec/fixtures/files/logo_email_custom.png")) + image: File.new(file_fixture("logo_email_custom.png"))) expect(image).not_to be_valid end diff --git a/spec/shared/models/acts_as_imageable.rb b/spec/shared/models/acts_as_imageable.rb index b4624372d..ed0d2df14 100644 --- a/spec/shared/models/acts_as_imageable.rb +++ b/spec/shared/models/acts_as_imageable.rb @@ -7,21 +7,21 @@ shared_examples "acts as imageable" do |imageable_factory| describe "file extension" do it "is not valid with '.png' extension" do - image.attachment = File.new("spec/fixtures/files/clippy.png") + image.attachment = File.new(file_fixture("clippy.png")) expect(image).not_to be_valid expect(image.errors[:attachment].size).to eq(1) end it "is not valid with '.gif' extension" do - image.attachment = File.new("spec/fixtures/files/clippy.gif") + image.attachment = File.new(file_fixture("clippy.gif")) expect(image).not_to be_valid expect(image.errors[:attachment].size).to eq(1) end it "is valid with '.jpg' extension" do - image.attachment = File.new("spec/fixtures/files/clippy.jpg") + image.attachment = File.new(file_fixture("clippy.jpg")) expect(image).to be_valid end @@ -33,7 +33,7 @@ shared_examples "acts as imageable" do |imageable_factory| end it "is not valid when image dimmensions are smaller than 475X475" do - image.attachment = File.new("spec/fixtures/files/logo_header.jpg") + image.attachment = File.new(file_fixture("logo_header.jpg")) expect(image).not_to be_valid end diff --git a/spec/shared/models/document_validations.rb b/spec/shared/models/document_validations.rb index 3594b1a57..01d13de23 100644 --- a/spec/shared/models/document_validations.rb +++ b/spec/shared/models/document_validations.rb @@ -22,7 +22,7 @@ shared_examples "document validations" do |documentable_factory| it "is valid for all accepted content types" do acceptedcontenttypes.each do |content_type| extension = content_type.split("/").last - document.attachment = File.new("spec/fixtures/files/empty.#{extension}") + document.attachment = File.new(file_fixture("empty.#{extension}")) expect(document).to be_valid end diff --git a/spec/shared/models/image_validations.rb b/spec/shared/models/image_validations.rb index ed26d0499..230fe4089 100644 --- a/spec/shared/models/image_validations.rb +++ b/spec/shared/models/image_validations.rb @@ -21,7 +21,7 @@ shared_examples "image validations" do |imageable_factory| it "is valid for all accepted content types" do acceptedcontenttypes.each do |content_type| extension = content_type.split("/").last - image.attachment = File.new("spec/fixtures/files/clippy.#{extension}") + image.attachment = File.new(file_fixture("clippy.#{extension}")) expect(image).to be_valid end @@ -30,7 +30,7 @@ shared_examples "image validations" do |imageable_factory| it "is not valid for png and gif image content types" do ["gif", "png"].each do |content_type| extension = content_type.split("/").last - image.attachment = File.new("spec/fixtures/files/clippy.#{extension}") + image.attachment = File.new(file_fixture("clippy.#{extension}")) expect(image).not_to be_valid end diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index 3e5d03f96..51cba390f 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -51,7 +51,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) documentable.class.max_documents_allowed.times.each do - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) end expect(page).to have_css ".max-documents-notice" @@ -77,7 +77,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na click_link "Add new document" within "#nested-documents" do - attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") + attach_file "Choose document", file_fixture("empty.pdf") expect(page).to have_css ".loading-bar.complete" end @@ -90,7 +90,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) expect_document_has_title(0, "empty.pdf") end @@ -104,7 +104,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na within "#nested-documents" do input = find("input[name$='[title]']") fill_in input[:id], with: "My Title" - attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") + attach_file "Choose document", file_fixture("empty.pdf") expect(page).to have_css ".loading-bar.complete" end @@ -116,7 +116,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) expect(page).to have_css ".loading-bar.complete" end @@ -125,10 +125,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file( - Rails.root.join("spec/fixtures/files/logo_header.gif"), - false - ) + documentable_attach_new_file(file_fixture("logo_header.gif"), false) expect(page).to have_css ".loading-bar.errors" end @@ -142,7 +139,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) expect(cached_attachment_field.value).to be_empty - attach_file "Choose document", Rails.root.join("spec/fixtures/files/empty.pdf") + attach_file "Choose document", file_fixture("empty.pdf") expect(page).to have_css(".loading-bar.complete") expect(cached_attachment_field.value).not_to be_empty @@ -152,10 +149,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file( - Rails.root.join("spec/fixtures/files/logo_header.gif"), - false - ) + documentable_attach_new_file(file_fixture("logo_header.gif"), false) cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) expect(cached_attachment_field.value).to be_empty @@ -178,7 +172,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) click_link "Remove document" expect(page).not_to have_css("#nested-documents .document") @@ -201,7 +195,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) click_on submit_button expect(page).to have_content documentable_success_notice @@ -215,7 +209,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) click_on submit_button documentable_redirected_to_resource_show_or_navigate_to @@ -239,7 +233,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na send(fill_resource_method_name) if fill_resource_method_name %w[clippy empty logo].take(documentable.class.max_documents_allowed).each do |filename| - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/#{filename}.pdf")) + documentable_attach_new_file(file_fixture("#{filename}.pdf")) end click_on submit_button @@ -291,7 +285,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login visit send(path, arguments) - documentable_attach_new_file(Rails.root.join("spec/fixtures/files/empty.pdf")) + documentable_attach_new_file(file_fixture("empty.pdf")) within_fieldset("Documents") { fill_in "Title", with: "Original" } click_button submit_button diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index f6fe1b723..123a54425 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -38,7 +38,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) click_link "Add image" - attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + attach_file "Choose image", file_fixture("clippy.jpg") expect(page).to have_selector ".file-name", text: "clippy.jpg" end @@ -47,7 +47,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) expect_image_has_title("clippy.jpg") end @@ -59,7 +59,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p click_link "Add image" input_title = find(".image input[name$='[title]']") fill_in input_title[:id], with: "Title" - attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + attach_file "Choose image", file_fixture("clippy.jpg") if has_many_images expect(find("input[id$='_title']").value).to eq "Title" @@ -72,7 +72,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) expect(page).to have_selector ".loading-bar.complete" end @@ -81,7 +81,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) + imageable_attach_new_file(file_fixture("logo_header.png"), false) expect(page).to have_selector ".loading-bar.errors" end @@ -95,7 +95,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) expect(cached_attachment_field.value).to be_empty - attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + attach_file "Choose image", file_fixture("clippy.jpg") expect(page).to have_css(".loading-bar.complete") expect(cached_attachment_field.value).not_to be_empty @@ -105,7 +105,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/logo_header.png"), false) + imageable_attach_new_file(file_fixture("logo_header.png"), false) cached_attachment_field = find("input[name$='[cached_attachment]']", visible: :hidden) @@ -133,7 +133,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) within_fieldset("Descriptive image") { fill_in "Title", with: "" } click_on submit_button @@ -145,7 +145,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) within "#nested-image .image" do click_link "Remove image" @@ -172,7 +172,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) expect(page).to have_selector ".loading-bar.complete" @@ -186,7 +186,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) send(fill_resource_method_name) if fill_resource_method_name - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) expect(page).to have_selector ".loading-bar.complete" @@ -205,12 +205,12 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p do_login_for user visit send(path, arguments) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) original_src = find(:fieldset, "Descriptive image").find("img")[:src] click_link "Remove image" - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/custom_map.jpg")) + imageable_attach_new_file(file_fixture("custom_map.jpg")) updated_src = find(:fieldset, "Descriptive image").find("img")[:src] diff --git a/spec/support/common_actions/remote_census_mock.rb b/spec/support/common_actions/remote_census_mock.rb index b140c17e4..0ec9aac17 100644 --- a/spec/support/common_actions/remote_census_mock.rb +++ b/spec/support/common_actions/remote_census_mock.rb @@ -5,15 +5,15 @@ module RemoteCensusMock include DocumentParser def mock_valid_remote_census_response - mock_remote_census_response(File.read("spec/fixtures/files/remote_census_api/valid.xml")) + mock_remote_census_response(File.read(file_fixture("remote_census_api/valid.xml"))) end def mock_invalid_remote_census_response - mock_remote_census_response(File.read("spec/fixtures/files/remote_census_api/invalid.xml")) + mock_remote_census_response(File.read(file_fixture("remote_census_api/invalid.xml"))) end def mock_invalid_signature_sheet_remote_census_response - xml = File.read("spec/fixtures/files/remote_census_api/invalid.xml") + xml = File.read(file_fixture("remote_census_api/invalid.xml")) Signature.new.document_types.each do |document_type| get_document_number_variants(document_type, "12345678Z").each do diff --git a/spec/system/admin/budget_phases_spec.rb b/spec/system/admin/budget_phases_spec.rb index 0d07bcc2f..257b33141 100644 --- a/spec/system/admin/budget_phases_spec.rb +++ b/spec/system/admin/budget_phases_spec.rb @@ -46,7 +46,7 @@ describe "Admin budget phases" do scenario "shows successful notice when updating the phase with a valid image" do visit edit_admin_budget_budget_phase_path(budget, budget.current_phase) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) click_on "Save changes" diff --git a/spec/system/admin/legislation/processes_spec.rb b/spec/system/admin/legislation/processes_spec.rb index d19a3a16c..ca001bbf1 100644 --- a/spec/system/admin/legislation/processes_spec.rb +++ b/spec/system/admin/legislation/processes_spec.rb @@ -175,7 +175,7 @@ describe "Admin collaborative legislation", :admin do fill_in "End", with: base_date + 5.days end - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) click_button "Create process" diff --git a/spec/system/admin/local_census_records/imports_spec.rb b/spec/system/admin/local_census_records/imports_spec.rb index 29154ebbb..a5d39d4dd 100644 --- a/spec/system/admin/local_census_records/imports_spec.rb +++ b/spec/system/admin/local_census_records/imports_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" describe "Imports", :admin do - let(:base_files_path) { %w[spec fixtures files local_census_records import] } + let(:base_files_path) { "local_census_records/import/" } describe "New" do scenario "Should show import form" do @@ -17,7 +17,7 @@ describe "Imports", :admin do scenario "Should show success notice after successful import" do within "form#new_local_census_records_import" do path = base_files_path << "valid.csv" - file = File.join(Rails.root, *path) + file = file_fixture(path) attach_file("local_census_records_import_file", file) click_button "Save" end @@ -35,8 +35,7 @@ describe "Imports", :admin do scenario "Should show alert when file is not supported" do within "form#new_local_census_records_import" do - path = ["spec", "fixtures", "files", "clippy.jpg"] - file = File.join(Rails.root, *path) + file = file_fixture("clippy.jpg") attach_file("local_census_records_import_file", file) click_button "Save" end @@ -47,7 +46,7 @@ describe "Imports", :admin do scenario "Should show successfully created local census records at created group" do within "form#new_local_census_records_import" do path = base_files_path << "valid.csv" - file = File.join(Rails.root, *path) + file = file_fixture(path) attach_file("local_census_records_import_file", file) click_button "Save" end @@ -59,7 +58,7 @@ describe "Imports", :admin do scenario "Should show invalid local census records at errored group" do within "form#new_local_census_records_import" do path = base_files_path << "invalid.csv" - file = File.join(Rails.root, *path) + file = file_fixture(path) attach_file("local_census_records_import_file", file) click_button "Save" end @@ -71,7 +70,7 @@ describe "Imports", :admin do scenario "Should show error messages inside cells at errored group" do within "form#new_local_census_records_import" do path = base_files_path << "invalid.csv" - file = File.join(Rails.root, *path) + file = file_fixture(path) attach_file("local_census_records_import_file", file) click_button "Save" end diff --git a/spec/system/admin/poll/questions/answers/images/images_spec.rb b/spec/system/admin/poll/questions/answers/images/images_spec.rb index 2d8049f1f..154e2e8fd 100644 --- a/spec/system/admin/poll/questions/answers/images/images_spec.rb +++ b/spec/system/admin/poll/questions/answers/images/images_spec.rb @@ -38,7 +38,7 @@ describe "Images", :admin do expect(page).not_to have_content("clippy.jpg") visit new_admin_answer_image_path(answer) - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) click_button "Save image" expect(page).to have_css("img[title='clippy.jpg']") diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index 6a2063299..5e86779d6 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -54,7 +54,7 @@ describe "Documents", :admin do scenario "Create" do visit new_admin_site_customization_document_path - attach_file("document_attachment", "#{Rails.root}/spec/fixtures/files/logo.pdf") + attach_file("document_attachment", file_fixture("logo.pdf")) click_button "Upload" expect(page).to have_content "Document uploaded succesfully" diff --git a/spec/system/admin/site_customization/images_spec.rb b/spec/system/admin/site_customization/images_spec.rb index d79468563..c20b33e7e 100644 --- a/spec/system/admin/site_customization/images_spec.rb +++ b/spec/system/admin/site_customization/images_spec.rb @@ -10,7 +10,7 @@ describe "Admin custom images", :admin do end within("tr#image_logo_header") do - attach_file "site_customization_image_image", "spec/fixtures/files/logo_header.png" + attach_file "site_customization_image_image", file_fixture("logo_header.png") click_button "Update" end @@ -22,7 +22,7 @@ describe "Admin custom images", :admin do visit admin_site_customization_images_path within("tr#image_map") do - attach_file "site_customization_image_image", "spec/fixtures/files/custom_map.jpg" + attach_file "site_customization_image_image", file_fixture("custom_map.jpg") click_button "Update" end @@ -37,7 +37,7 @@ describe "Admin custom images", :admin do visit admin_site_customization_images_path within("tr#image_map") do - attach_file "site_customization_image_image", "spec/fixtures/files/custom_map.jpg" + attach_file "site_customization_image_image", file_fixture("custom_map.jpg") click_button "Update" end @@ -63,7 +63,7 @@ describe "Admin custom images", :admin do scenario "Custom apple touch icon is replaced on front views" do create(:site_customization_image, name: "apple-touch-icon-200", - image: File.new("spec/fixtures/files/apple-touch-icon-custom-200.png")) + image: File.new(file_fixture("apple-touch-icon-custom-200.png"))) visit root_path @@ -77,7 +77,7 @@ describe "Admin custom images", :admin do visit admin_site_customization_images_path within("tr#image_logo_email") do - attach_file "site_customization_image_image", "spec/fixtures/files/logo_email_custom.png" + attach_file "site_customization_image_image", file_fixture("logo_email_custom.png") click_button "Update" end @@ -92,7 +92,7 @@ describe "Admin custom images", :admin do visit admin_site_customization_images_path within("tr#image_social_media_icon") do - attach_file "site_customization_image_image", "spec/fixtures/files/logo_header.png" + attach_file "site_customization_image_image", file_fixture("logo_header.png") click_button "Update" end @@ -104,7 +104,7 @@ describe "Admin custom images", :admin do visit admin_site_customization_images_path within("tr#image_social_media_icon") do - attach_file "site_customization_image_image", "spec/fixtures/files/social_media_icon.png" + attach_file "site_customization_image_image", file_fixture("social_media_icon.png") click_button "Update" end diff --git a/spec/system/admin/widgets/cards_spec.rb b/spec/system/admin/widgets/cards_spec.rb index 650e374b3..ed299dd03 100644 --- a/spec/system/admin/widgets/cards_spec.rb +++ b/spec/system/admin/widgets/cards_spec.rb @@ -251,7 +251,7 @@ describe "Cards", :admin do def attach_image_to_card click_link "Add image" - attach_file "Choose image", Rails.root.join("spec/fixtures/files/clippy.jpg") + attach_file "Choose image", file_fixture("clippy.jpg") expect(page).to have_field("widget_card_image_attributes_title", with: "clippy.jpg") end diff --git a/spec/system/ckeditor_spec.rb b/spec/system/ckeditor_spec.rb index 03fc745b0..44d2a1096 100644 --- a/spec/system/ckeditor_spec.rb +++ b/spec/system/ckeditor_spec.rb @@ -33,7 +33,7 @@ describe "CKEditor" do click_link "Upload" within_frame(1) do - attach_file "Send it to the Server", Rails.root.join("spec/fixtures/files/clippy.jpg") + attach_file "Send it to the Server", file_fixture("clippy.jpg") end click_link "Send it to the Server" diff --git a/spec/system/legislation/proposals_spec.rb b/spec/system/legislation/proposals_spec.rb index cfeac71fa..80eef133f 100644 --- a/spec/system/legislation/proposals_spec.rb +++ b/spec/system/legislation/proposals_spec.rb @@ -146,7 +146,7 @@ describe "Legislation Proposals" do fill_in "Proposal title", with: "Legislation proposal with image" fill_in "Proposal summary", with: "Including an image on a legislation proposal" - imageable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.jpg")) + imageable_attach_new_file(file_fixture("clippy.jpg")) check "legislation_proposal_terms_of_service" click_button "Create proposal" From 8eea6f585a31ccd436785275af5ffb9919ea71c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 18 Sep 2021 17:37:54 +0200 Subject: [PATCH 11/11] Remove hack to allow IO files in Active Storage 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. --- app/models/concerns/has_attachment.rb | 4 +--- db/dev_seeds/budgets.rb | 14 ++++++-------- db/dev_seeds/proposals.rb | 14 ++++++-------- db/dev_seeds/widgets.rb | 2 +- spec/factories/administration.rb | 2 +- spec/factories/files.rb | 8 ++++---- spec/lib/tasks/active_storage_spec.rb | 2 +- spec/models/direct_upload_spec.rb | 8 ++++---- spec/models/document_spec.rb | 2 +- spec/models/image_spec.rb | 2 +- spec/models/local_census_records/import_spec.rb | 8 ++++---- spec/models/site_customization/image_spec.rb | 8 ++++---- spec/shared/models/acts_as_imageable.rb | 8 ++++---- spec/shared/models/document_validations.rb | 2 +- spec/shared/models/image_validations.rb | 4 ++-- spec/spec_helper.rb | 1 + .../system/admin/site_customization/images_spec.rb | 2 +- 17 files changed, 43 insertions(+), 48 deletions(-) diff --git a/app/models/concerns/has_attachment.rb b/app/models/concerns/has_attachment.rb index accca53c2..2ffbd931b 100644 --- a/app/models/concerns/has_attachment.rb +++ b/app/models/concerns/has_attachment.rb @@ -6,9 +6,7 @@ module HasAttachment has_one_attached attribute define_method :"#{attribute}=" do |file| - if file.is_a?(IO) - send(attribute).attach(io: file, filename: File.basename(file.path)) - elsif file.nil? + if file.nil? send(attribute).detach else send(attribute).attach(file) diff --git a/db/dev_seeds/budgets.rb b/db/dev_seeds/budgets.rb index c686a5c9c..e3dc861c2 100644 --- a/db/dev_seeds/budgets.rb +++ b/db/dev_seeds/budgets.rb @@ -15,14 +15,12 @@ end def add_image_to(imageable) # imageable should respond to #title & #author - File.open(INVESTMENT_IMAGE_FILES.sample) do |file| - imageable.image = Image.create!({ - imageable: imageable, - title: imageable.title, - attachment: file, - user: imageable.author - }) - end + imageable.image = Image.create!({ + imageable: imageable, + title: imageable.title, + attachment: Rack::Test::UploadedFile.new(INVESTMENT_IMAGE_FILES.sample), + user: imageable.author + }) imageable.save! end diff --git a/db/dev_seeds/proposals.rb b/db/dev_seeds/proposals.rb index 756dd00b3..a2ccbfb08 100644 --- a/db/dev_seeds/proposals.rb +++ b/db/dev_seeds/proposals.rb @@ -12,14 +12,12 @@ end def add_image_to(imageable) # imageable should respond to #title & #author - File.open(IMAGE_FILES.sample) do |file| - imageable.image = Image.create!({ - imageable: imageable, - title: imageable.title, - attachment: file, - user: imageable.author - }) - end + imageable.image = Image.create!({ + imageable: imageable, + title: imageable.title, + attachment: Rack::Test::UploadedFile.new(IMAGE_FILES.sample), + user: imageable.author + }) imageable.save! end diff --git a/db/dev_seeds/widgets.rb b/db/dev_seeds/widgets.rb index 41711c937..d82d655fe 100644 --- a/db/dev_seeds/widgets.rb +++ b/db/dev_seeds/widgets.rb @@ -1,7 +1,7 @@ section "Creating header and cards for the homepage" do def create_image_attachment(type) { - attachment: File.new(Rails.root.join("db/dev_seeds/images/#{type}_background.jpg")), + attachment: Rack::Test::UploadedFile.new("db/dev_seeds/images/#{type}_background.jpg"), title: "#{type}_background.jpg", user: User.first } diff --git a/spec/factories/administration.rb b/spec/factories/administration.rb index a59f30776..07a110ca5 100644 --- a/spec/factories/administration.rb +++ b/spec/factories/administration.rb @@ -58,7 +58,7 @@ FactoryBot.define do end factory :site_customization_image, class: "SiteCustomization::Image" do - image { File.new("spec/fixtures/files/logo_header.png") } + image { Rack::Test::UploadedFile.new("spec/fixtures/files/logo_header.png") } name { "logo_header" } end diff --git a/spec/factories/files.rb b/spec/factories/files.rb index 0926ee576..5876c90c8 100644 --- a/spec/factories/files.rb +++ b/spec/factories/files.rb @@ -1,6 +1,6 @@ FactoryBot.define do factory :image do - attachment { File.new("spec/fixtures/files/clippy.jpg") } + attachment { Rack::Test::UploadedFile.new("spec/fixtures/files/clippy.jpg") } title { "Lorem ipsum dolor sit amet" } association :user, factory: :user @@ -16,7 +16,7 @@ FactoryBot.define do factory :document do sequence(:title) { |n| "Document title #{n}" } association :user, factory: :user - attachment { File.new("spec/fixtures/files/empty.pdf") } + attachment { Rack::Test::UploadedFile.new("spec/fixtures/files/empty.pdf") } trait :proposal_document do association :documentable, factory: :proposal @@ -47,11 +47,11 @@ FactoryBot.define do trait :documents do resource_relation { "documents" } - attachment { File.new("spec/fixtures/files/empty.pdf") } + attachment { Rack::Test::UploadedFile.new("spec/fixtures/files/empty.pdf") } end trait :image do resource_relation { "image" } - attachment { File.new("spec/fixtures/files/clippy.jpg") } + attachment { Rack::Test::UploadedFile.new("spec/fixtures/files/clippy.jpg") } end initialize_with { new(attributes) } end diff --git a/spec/lib/tasks/active_storage_spec.rb b/spec/lib/tasks/active_storage_spec.rb index 0f43b39cd..bf394d84d 100644 --- a/spec/lib/tasks/active_storage_spec.rb +++ b/spec/lib/tasks/active_storage_spec.rb @@ -35,7 +35,7 @@ describe "active storage tasks" do end it "does not modify old ckeditor attachments" do - image = Ckeditor::Picture.create!(data: Rack::Test::UploadedFile.new(file_fixture("clippy.png"))) + image = Ckeditor::Picture.create!(data: fixture_file_upload("clippy.png")) expect(image.storage_data.attachment.name).to eq "storage_data" diff --git a/spec/models/direct_upload_spec.rb b/spec/models/direct_upload_spec.rb index bdf079680..ec342f0de 100644 --- a/spec/models/direct_upload_spec.rb +++ b/spec/models/direct_upload_spec.rb @@ -10,10 +10,10 @@ describe DirectUpload do end it "is not valid for different kind of combinations with invalid atttachment content types" do - expect(build(:direct_upload, :proposal, :documents, attachment: File.new(file_fixture("clippy.png")))).not_to be_valid - expect(build(:direct_upload, :proposal, :image, attachment: File.new(file_fixture("empty.pdf")))).not_to be_valid - expect(build(:direct_upload, :budget_investment, :documents, attachment: File.new(file_fixture("clippy.png")))).not_to be_valid - expect(build(:direct_upload, :budget_investment, :image, attachment: File.new(file_fixture("empty.pdf")))).not_to be_valid + expect(build(:direct_upload, :proposal, :documents, attachment: fixture_file_upload("clippy.png"))).not_to be_valid + expect(build(:direct_upload, :proposal, :image, attachment: fixture_file_upload("empty.pdf"))).not_to be_valid + expect(build(:direct_upload, :budget_investment, :documents, attachment: fixture_file_upload("clippy.png"))).not_to be_valid + expect(build(:direct_upload, :budget_investment, :image, attachment: fixture_file_upload("empty.pdf"))).not_to be_valid end it "is not valid without resource_type" do diff --git a/spec/models/document_spec.rb b/spec/models/document_spec.rb index 1d7a087a4..0180c4010 100644 --- a/spec/models/document_spec.rb +++ b/spec/models/document_spec.rb @@ -5,7 +5,7 @@ describe Document do it_behaves_like "document validations", "proposal_document" it "stores attachments with Active Storage" do - document = create(:document, attachment: File.new(file_fixture("clippy.pdf"))) + document = create(:document, attachment: fixture_file_upload("clippy.pdf")) expect(document.attachment).to be_attached expect(document.attachment.filename).to eq "clippy.pdf" diff --git a/spec/models/image_spec.rb b/spec/models/image_spec.rb index 7022e0b40..6b4603923 100644 --- a/spec/models/image_spec.rb +++ b/spec/models/image_spec.rb @@ -5,7 +5,7 @@ describe Image do it_behaves_like "image validations", "proposal_image" it "stores attachments with Active Storage" do - image = create(:image, attachment: File.new(file_fixture("clippy.jpg"))) + image = create(:image, attachment: fixture_file_upload("clippy.jpg")) expect(image.attachment).to be_attached expect(image.attachment.filename).to eq "clippy.jpg" diff --git a/spec/models/local_census_records/import_spec.rb b/spec/models/local_census_records/import_spec.rb index 70ec5038b..7a97f0df5 100644 --- a/spec/models/local_census_records/import_spec.rb +++ b/spec/models/local_census_records/import_spec.rb @@ -17,7 +17,7 @@ describe LocalCensusRecords::Import do context "When file extension" do it "is wrong it should not be valid" do - file = Rack::Test::UploadedFile.new(file_fixture("clippy.gif")) + file = fixture_file_upload("clippy.gif") import = build(:local_census_records_import, file: file) expect(import).not_to be_valid @@ -25,7 +25,7 @@ describe LocalCensusRecords::Import do it "is csv it should be valid" do path = base_files_path << "valid.csv" - file = Rack::Test::UploadedFile.new(file_fixture(path)) + file = fixture_file_upload(path) import = build(:local_census_records_import, file: file) expect(import).to be_valid @@ -35,7 +35,7 @@ describe LocalCensusRecords::Import do context "When file headers" do it "are all missing it should not be valid" do path = base_files_path << "valid-without-headers.csv" - file = Rack::Test::UploadedFile.new(file_fixture(path)) + file = fixture_file_upload(path) import = build(:local_census_records_import, file: file) expect(import).not_to be_valid @@ -64,7 +64,7 @@ describe LocalCensusRecords::Import do it "Add invalid local census records to invalid_records array" do path = base_files_path << "invalid.csv" - file = Rack::Test::UploadedFile.new(file_fixture(path)) + file = fixture_file_upload(path) import.file = file import.save! diff --git a/spec/models/site_customization/image_spec.rb b/spec/models/site_customization/image_spec.rb index eef988bd1..076fc01d5 100644 --- a/spec/models/site_customization/image_spec.rb +++ b/spec/models/site_customization/image_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" describe SiteCustomization::Image do it "stores images with Active Storage" do image = create(:site_customization_image, name: "map", - image: File.new(file_fixture("custom_map.jpg"))) + image: fixture_file_upload("custom_map.jpg")) expect(image.image).to be_attached expect(image.image.filename).to eq "custom_map.jpg" @@ -13,7 +13,7 @@ describe SiteCustomization::Image do it "is valid with a 260x80 image" do image = build(:site_customization_image, name: "logo_header", - image: File.new(file_fixture("logo_header-260x80.png"))) + image: fixture_file_upload("logo_header-260x80.png")) expect(image).to be_valid end @@ -21,7 +21,7 @@ describe SiteCustomization::Image do it "is valid with a 223x80 image" do image = build(:site_customization_image, name: "logo_header", - image: File.new(file_fixture("logo_header.png"))) + image: fixture_file_upload("logo_header.png")) expect(image).to be_valid end @@ -29,7 +29,7 @@ describe SiteCustomization::Image do it "is not valid with a 400x80 image" do image = build(:site_customization_image, name: "logo_header", - image: File.new(file_fixture("logo_email_custom.png"))) + image: fixture_file_upload("logo_email_custom.png")) expect(image).not_to be_valid end diff --git a/spec/shared/models/acts_as_imageable.rb b/spec/shared/models/acts_as_imageable.rb index ed0d2df14..6e9810538 100644 --- a/spec/shared/models/acts_as_imageable.rb +++ b/spec/shared/models/acts_as_imageable.rb @@ -7,21 +7,21 @@ shared_examples "acts as imageable" do |imageable_factory| describe "file extension" do it "is not valid with '.png' extension" do - image.attachment = File.new(file_fixture("clippy.png")) + image.attachment = fixture_file_upload("clippy.png") expect(image).not_to be_valid expect(image.errors[:attachment].size).to eq(1) end it "is not valid with '.gif' extension" do - image.attachment = File.new(file_fixture("clippy.gif")) + image.attachment = fixture_file_upload("clippy.gif") expect(image).not_to be_valid expect(image.errors[:attachment].size).to eq(1) end it "is valid with '.jpg' extension" do - image.attachment = File.new(file_fixture("clippy.jpg")) + image.attachment = fixture_file_upload("clippy.jpg") expect(image).to be_valid end @@ -33,7 +33,7 @@ shared_examples "acts as imageable" do |imageable_factory| end it "is not valid when image dimmensions are smaller than 475X475" do - image.attachment = File.new(file_fixture("logo_header.jpg")) + image.attachment = fixture_file_upload("logo_header.jpg") expect(image).not_to be_valid end diff --git a/spec/shared/models/document_validations.rb b/spec/shared/models/document_validations.rb index 01d13de23..11235f771 100644 --- a/spec/shared/models/document_validations.rb +++ b/spec/shared/models/document_validations.rb @@ -22,7 +22,7 @@ shared_examples "document validations" do |documentable_factory| it "is valid for all accepted content types" do acceptedcontenttypes.each do |content_type| extension = content_type.split("/").last - document.attachment = File.new(file_fixture("empty.#{extension}")) + document.attachment = fixture_file_upload("empty.#{extension}") expect(document).to be_valid end diff --git a/spec/shared/models/image_validations.rb b/spec/shared/models/image_validations.rb index 230fe4089..5d1245904 100644 --- a/spec/shared/models/image_validations.rb +++ b/spec/shared/models/image_validations.rb @@ -21,7 +21,7 @@ shared_examples "image validations" do |imageable_factory| it "is valid for all accepted content types" do acceptedcontenttypes.each do |content_type| extension = content_type.split("/").last - image.attachment = File.new(file_fixture("clippy.#{extension}")) + image.attachment = fixture_file_upload("clippy.#{extension}") expect(image).to be_valid end @@ -30,7 +30,7 @@ shared_examples "image validations" do |imageable_factory| it "is not valid for png and gif image content types" do ["gif", "png"].each do |content_type| extension = content_type.split("/").last - image.attachment = File.new(file_fixture("clippy.#{extension}")) + image.attachment = fixture_file_upload("clippy.#{extension}") expect(image).not_to be_valid end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index b89e55699..b8b155795 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -9,6 +9,7 @@ Dir["./spec/shared/**/*.rb"].sort.each { |f| require f } RSpec.configure do |config| config.use_transactional_fixtures = true + config.fixture_path = "spec/fixtures/files" config.filter_run_when_matching :focus config.include RequestSpecHelper, type: :request diff --git a/spec/system/admin/site_customization/images_spec.rb b/spec/system/admin/site_customization/images_spec.rb index c20b33e7e..85f3736ee 100644 --- a/spec/system/admin/site_customization/images_spec.rb +++ b/spec/system/admin/site_customization/images_spec.rb @@ -63,7 +63,7 @@ describe "Admin custom images", :admin do scenario "Custom apple touch icon is replaced on front views" do create(:site_customization_image, name: "apple-touch-icon-200", - image: File.new(file_fixture("apple-touch-icon-custom-200.png"))) + image: fixture_file_upload("apple-touch-icon-custom-200.png")) visit root_path