Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/controllers/alchemy/admin/attachments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 23 additions & 2 deletions app/controllers/alchemy/admin/pictures_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
40 changes: 40 additions & 0 deletions app/models/alchemy/attachment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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?
Expand Down Expand Up @@ -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
Expand Down
18 changes: 14 additions & 4 deletions app/models/concerns/alchemy/relatable_resource.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
#
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion app/views/alchemy/admin/attachments/_files_list.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
file_attribute: 'file' %>
<% end %>
<% table.with_action(:destroy) do |attachment| %>
<% if attachment.deletable? %>
<% if @deletable_attachment_ids.include?(attachment.id) %>
<sl-tooltip content="<%= Alchemy.t(:delete_file) %>">
<%= link_to_confirm_dialog render_icon(:minus),
Alchemy.t(:confirm_to_delete_file),
Expand Down
2 changes: 1 addition & 1 deletion app/views/alchemy/admin/pictures/_picture.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<span class="picture_tool select">
<%= check_box_tag "picture_ids[]", picture.id %>
</span>
<% if picture.deletable? && can?(:destroy, picture) %>
<% if @deletable_picture_ids.include?(picture.id) && can?(:destroy, picture) %>
<div class="picture_tool delete">
<sl-tooltip content="<%= Alchemy.t('Delete image') %>">
<%= link_to_confirm_dialog(
Expand Down
26 changes: 26 additions & 0 deletions spec/controllers/alchemy/admin/attachments_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: %(<a href="/attachment/#{referenced_attachment.id}/download">x</a>)
)
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) }

Expand Down
21 changes: 21 additions & 0 deletions spec/controllers/alchemy/admin/pictures_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
66 changes: 66 additions & 0 deletions spec/models/alchemy/attachment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: %(<p>See <a href="/attachment/#{richtext_linked_attachment.id}/download/foo.pdf">this</a></p>)
)
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: %(<a href="/attachment/#{attachment.id}/download">x</a>)
)
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: %(<a href="/attachment/#{other_id}/download">x</a>)
)
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
Expand Down
Loading