diff --git a/app/controllers/alchemy/admin/attachments_controller.rb b/app/controllers/alchemy/admin/attachments_controller.rb index cd5b4088d8..d07087773c 100644 --- a/app/controllers/alchemy/admin/attachments_controller.rb +++ b/app/controllers/alchemy/admin/attachments_controller.rb @@ -39,6 +39,15 @@ def index .page(params[:page] || 1) .per(items_per_page) + # Preload deletable ids for the current page in a single query so the + # view can decide which delete buttons to enable without calling + # +deletable?+ (a two-query check) per row. We pass the already-loaded + # ids (via +map(&:id)+) rather than the relation itself, because passing + # a paginated relation produces +IN (SELECT ... LIMIT n)+, which older + # MariaDB versions reject. + @deletable_attachment_ids = + Attachment.where(id: @attachments.map(&:id)).deletable.pluck(:id).to_set + if in_overlay? archive_overlay end diff --git a/app/controllers/alchemy/admin/pictures_controller.rb b/app/controllers/alchemy/admin/pictures_controller.rb index 72af9a33ac..934d56f476 100644 --- a/app/controllers/alchemy/admin/pictures_controller.rb +++ b/app/controllers/alchemy/admin/pictures_controller.rb @@ -8,6 +8,8 @@ class PicturesController < Alchemy::Admin::ResourcesController include CurrentLanguage include PictureDescriptionsFormHelper + before_action :load_pictures, only: :index + before_action :load_resource, only: [:edit, :update, :url, :destroy] @@ -19,6 +21,12 @@ class PicturesController < Alchemy::Admin::ResourcesController @picture = Picture.find(params[:id]) end + # Preload deletable ids for the current page (index) or the current + # resource (update, which re-renders the +_picture+ partial via a + # turbo stream). One query replaces the per-row +deletable?+ check + # that would otherwise fire for every picture the view renders. + before_action :load_deletable_picture_ids, only: [:index, :update] + add_alchemy_filter :by_file_format, type: :select, options: ->(query) do Alchemy::Picture.file_formats(query.result) end @@ -30,8 +38,6 @@ class PicturesController < Alchemy::Admin::ResourcesController helper_method :picture_offset def index - @pictures = filtered_pictures(page: params[:page]) - if in_overlay? archive_overlay end @@ -159,6 +165,21 @@ def items_per_page_options private + def load_pictures + @pictures = filtered_pictures(page: params[:page]) + end + + # Preload deletable ids in a single query so the view can decide which + # delete buttons to enable without calling +deletable?+ (a two-query + # check) per row. We pass already-loaded ids (via +map(&:id)+) rather + # than the relation itself, because passing a paginated relation + # produces +IN (SELECT ... LIMIT n)+, which older MariaDB versions + # reject. + def load_deletable_picture_ids + ids = @pictures ? @pictures.map(&:id) : [@picture.id] + @deletable_picture_ids = Picture.where(id: ids).deletable.pluck(:id).to_set + end + def picture_offset ((params[:page] || 1).to_i - 1) * items_per_page end diff --git a/app/models/alchemy/attachment.rb b/app/models/alchemy/attachment.rb index 6b9ecbe6c5..572e5ed74e 100644 --- a/app/models/alchemy/attachment.rb +++ b/app/models/alchemy/attachment.rb @@ -42,6 +42,33 @@ class Attachment < BaseRecord scope :recent, -> { where("#{table_name}.created_at > ?", Time.current - 24.hours).order(:created_at) } scope :without_tag, -> { left_outer_joins(:taggings).where(gutentag_taggings: {id: nil}) } + # Override +Alchemy::RelatableResource#deletable+ to also exclude + # attachments referenced from +/attachment/:id/download+ URLs inside + # ingredient values (e.g. Richtext markup, Link ingredients, raw Html). + # Those URLs are written by the file tab of the link dialog and are + # not tracked via the polymorphic +related_object+ association, so the + # base scope cannot see them. + # + # Uses a correlated +NOT EXISTS+ subquery that builds the per-row LIKE + # pattern with +Arel::Nodes::Concat+, which compiles to +||+ on + # SQLite/PostgreSQL and +CONCAT()+ on MySQL. + scope :deletable, -> do + ingredients = Alchemy::Ingredient.arel_table + pattern = Arel::Nodes::Concat.new( + Arel::Nodes::Concat.new( + Arel::Nodes.build_quoted("%/attachment/"), + arel_table[:id] + ), + Arel::Nodes.build_quoted("/download%") + ) + referenced = ingredients + .project(1) + .where(ingredients[:value].matches(pattern)) + + where("#{table_name}.id NOT IN (#{RelatableResource::RELATED_INGREDIENTS_SUBQUERY})", type: name) + .where.not(referenced.exists) + end + # We need to define this method here to have it available in the validations below. class << self # The class used to generate URLs for attachments @@ -112,6 +139,13 @@ def slug CGI.escape(file_name.gsub(/\.#{extension}$/, "").tr(".", " ")) end + # Override +Alchemy::RelatableResource#deletable?+ to also consider + # +/attachment/:id/download+ links inside ingredient values (e.g. + # Richtext markup, Link ingredients, raw Html). + def deletable? + super && !referenced_in_ingredient_value? + end + # Checks if the attachment is restricted, because it is attached on restricted pages only def restricted? related_pages.any? && related_pages.not_restricted.blank? @@ -178,6 +212,12 @@ def file_type_allowed end end + def referenced_in_ingredient_value? + Alchemy::Ingredient + .where("value LIKE ?", "%/attachment/#{id}/download%") + .exists? + end + def set_name self.name ||= Alchemy.storage_adapter.file_basename(self).humanize end diff --git a/app/models/concerns/alchemy/relatable_resource.rb b/app/models/concerns/alchemy/relatable_resource.rb index 964afbbbe5..a07a1b44f9 100644 --- a/app/models/concerns/alchemy/relatable_resource.rb +++ b/app/models/concerns/alchemy/relatable_resource.rb @@ -2,6 +2,19 @@ module Alchemy module RelatableResource extend ActiveSupport::Concern + # SQL subquery selecting +related_object_id+ values for ingredients that + # reference a given polymorphic type. Intended to be composed into a + # +NOT IN (...)+ clause by +deletable+ and any overrides in including + # classes. Takes one named bind :type - the polymorphic type name, + # typically the class name of the including model + # (e.g. +"Alchemy::Attachment"+). + RELATED_INGREDIENTS_SUBQUERY = <<~SQL.squish + SELECT related_object_id + FROM alchemy_ingredients + WHERE related_object_id IS NOT NULL + AND related_object_type = :type + SQL + class_methods do # Preload associations for element editor display # @@ -18,10 +31,7 @@ def alchemy_element_preloads(records) included do scope :deletable, -> do - where( - "#{table_name}.id NOT IN (SELECT related_object_id FROM alchemy_ingredients WHERE related_object_id IS NOT NULL AND related_object_type = ?)", - name - ) + where("#{table_name}.id NOT IN (#{RELATED_INGREDIENTS_SUBQUERY})", type: name) end has_many :related_ingredients, diff --git a/app/views/alchemy/admin/attachments/_files_list.html.erb b/app/views/alchemy/admin/attachments/_files_list.html.erb index 43d8aed398..023f7f96e0 100644 --- a/app/views/alchemy/admin/attachments/_files_list.html.erb +++ b/app/views/alchemy/admin/attachments/_files_list.html.erb @@ -61,7 +61,7 @@ file_attribute: 'file' %> <% end %> <% table.with_action(:destroy) do |attachment| %> - <% if attachment.deletable? %> + <% if @deletable_attachment_ids.include?(attachment.id) %> <%= link_to_confirm_dialog render_icon(:minus), Alchemy.t(:confirm_to_delete_file), diff --git a/app/views/alchemy/admin/pictures/_picture.html.erb b/app/views/alchemy/admin/pictures/_picture.html.erb index a0ea89d419..9a17190b95 100644 --- a/app/views/alchemy/admin/pictures/_picture.html.erb +++ b/app/views/alchemy/admin/pictures/_picture.html.erb @@ -2,7 +2,7 @@ <%= check_box_tag "picture_ids[]", picture.id %> - <% if picture.deletable? && can?(:destroy, picture) %> + <% if @deletable_picture_ids.include?(picture.id) && can?(:destroy, picture) %>
<%= link_to_confirm_dialog( diff --git a/spec/controllers/alchemy/admin/attachments_controller_spec.rb b/spec/controllers/alchemy/admin/attachments_controller_spec.rb index 98289e8202..6c767ceaa8 100644 --- a/spec/controllers/alchemy/admin/attachments_controller_spec.rb +++ b/spec/controllers/alchemy/admin/attachments_controller_spec.rb @@ -70,6 +70,32 @@ module Alchemy end end + describe "@deletable_attachment_ids" do + let!(:deletable_attachment) { create(:alchemy_attachment) } + let!(:referenced_attachment) { create(:alchemy_attachment) } + + before do + create( + :alchemy_ingredient_richtext, + value: %(x) + ) + end + + it "is a Set of ids of attachments on the current page that are deletable" do + get :index + expect(assigns(:deletable_attachment_ids)).to be_a(Set) + expect(assigns(:deletable_attachment_ids)).to include(deletable_attachment.id) + expect(assigns(:deletable_attachment_ids)).not_to include(referenced_attachment.id) + end + + it "is scoped to the current page only" do + get :index, params: {per_page: 1} + # With per_page: 1 only one attachment is on the current page, + # so the set must have at most one element. + expect(assigns(:deletable_attachment_ids).size).to be <= 1 + end + end + describe "by_file_type filter" do let!(:png) { create(:alchemy_attachment) } diff --git a/spec/controllers/alchemy/admin/pictures_controller_spec.rb b/spec/controllers/alchemy/admin/pictures_controller_spec.rb index 18b9a7f4e7..d7b196e3e4 100644 --- a/spec/controllers/alchemy/admin/pictures_controller_spec.rb +++ b/spec/controllers/alchemy/admin/pictures_controller_spec.rb @@ -136,6 +136,27 @@ module Alchemy end end end + + describe "@deletable_picture_ids" do + let!(:deletable_picture) { create(:alchemy_picture) } + let!(:referenced_picture) { create(:alchemy_picture) } + + before do + create(:alchemy_ingredient_picture, related_object: referenced_picture) + end + + it "is a Set of ids of pictures on the current page that are deletable" do + get :index + expect(assigns(:deletable_picture_ids)).to be_a(Set) + expect(assigns(:deletable_picture_ids)).to include(deletable_picture.id) + expect(assigns(:deletable_picture_ids)).not_to include(referenced_picture.id) + end + + it "is scoped to the current page only" do + get :index, params: {per_page: 1} + expect(assigns(:deletable_picture_ids).size).to be <= 1 + end + end end describe "#create" do diff --git a/spec/models/alchemy/attachment_spec.rb b/spec/models/alchemy/attachment_spec.rb index 69a83fa17a..f2402da99e 100644 --- a/spec/models/alchemy/attachment_spec.rb +++ b/spec/models/alchemy/attachment_spec.rb @@ -21,6 +21,72 @@ module Alchemy resource_name: :attachment, ingredient_type: :file + describe ".deletable" do + let!(:richtext_linked_attachment) { create(:alchemy_attachment) } + let!(:link_linked_attachment) { create(:alchemy_attachment) } + let!(:unlinked_attachment) { create(:alchemy_attachment) } + + before do + create( + :alchemy_ingredient_richtext, + value: %(

See this

) + ) + create( + :alchemy_ingredient_link, + value: "/attachment/#{link_linked_attachment.id}/download" + ) + end + + it "excludes attachments linked from any ingredient download URL" do + expect(described_class.deletable).to include(unlinked_attachment) + expect(described_class.deletable).not_to include(richtext_linked_attachment) + expect(described_class.deletable).not_to include(link_linked_attachment) + end + end + + describe "#deletable?" do + let(:attachment) { create(:alchemy_attachment) } + + subject { attachment.deletable? } + + context "when referenced by a /attachment/:id/download URL in a Richtext ingredient" do + before do + create( + :alchemy_ingredient_richtext, + value: %(x) + ) + end + + it { is_expected.to be(false) } + end + + context "when referenced by a /attachment/:id/download URL in a Link ingredient" do + before do + create( + :alchemy_ingredient_link, + value: "/attachment/#{attachment.id}/download" + ) + end + + it { is_expected.to be(false) } + end + + context "when another ingredient links to a different attachment whose id shares a digit prefix" do + let(:other_id) { attachment.id * 10 } + + before do + create( + :alchemy_ingredient_richtext, + value: %(x) + ) + end + + it "does not falsely report as referenced" do + is_expected.to be(true) + end + end + end + it "has file mime type accessor" do expect(attachment.file_mime_type).to eq("image/png") end