From 127e8379138c4fe70bdf51dfa07a901494999981 Mon Sep 17 00:00:00 2001 From: MareStare Date: Thu, 15 May 2025 21:32:42 +0000 Subject: [PATCH 01/30] Eliminate redundant `tag_change` preload --- lib/philomena/tag_changes.ex | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 90b354b55..6a710db3b 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -21,30 +21,20 @@ defmodule Philomena.TagChanges do inner_join: i in assoc(tc, :image), where: tc.id in ^ids and i.hidden_from_users == false, order_by: [desc: :created_at], - preload: [tags: [:tag, :tag_change]] + preload: [tags: [:tag]] ) - case mass_revert_tags(Enum.flat_map(tag_changes, & &1.tags), attributes) do - {:ok, _result} -> - {:ok, tag_changes} - - error -> - error - end - end - - # Accepts a list of TagChanges.Tag objects with tag_change and tag relations preloaded. - def mass_revert_tags(tags, attributes) do # Sort tags by tag change creation date, then uniq them by tag ID # to keep the first, aka the latest, record. Then prepare the struct # for the batch updater. changes_per_image = - tags - |> Enum.group_by(& &1.tag_change.image_id) - |> Enum.map(fn {image_id, instances} -> + tag_changes + |> Enum.group_by(& &1.image_id) + |> Enum.map(fn {image_id, tag_changes} -> changed_tags = - instances - |> Enum.sort_by(& &1.tag_change.created_at, :desc) + tag_changes + |> Enum.sort_by(& &1.created_at, :desc) + |> Enum.flat_map(& &1.tags) |> Enum.uniq_by(& &1.tag_id) {added_tags, removed_tags} = Enum.split_with(changed_tags, & &1.added) From 4a65f147e027365268903ca300b6f151b2b31a2b Mon Sep 17 00:00:00 2001 From: MareStare Date: Thu, 15 May 2025 21:48:11 +0000 Subject: [PATCH 02/30] Eliminate the redundant `tag` preload --- lib/philomena/images.ex | 29 ++++++++++++++++------------- lib/philomena/tag_changes.ex | 23 +++++++++++------------ lib/philomena/tags.ex | 8 +++++++- 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index b56e21cd1..a85ababec 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -1074,13 +1074,11 @@ defmodule Philomena.Images do to_insert = Enum.flat_map(changes, fn change -> - Enum.map(change.added_tags, &%{tag_id: &1.id, image_id: change.image_id}) + Enum.map(change.added_tag_ids, &%{tag_id: &1, image_id: change.image_id}) end) to_delete_ids = - Enum.flat_map(changes, fn change -> - Enum.map(change.removed_tags, & &1.id) - end) + Enum.flat_map(changes, & &1.removed_tag_ids) to_delete = Tagging @@ -1143,7 +1141,12 @@ defmodule Philomena.Images do {:ok, _} = result -> reindex_images(image_ids) Comments.reindex_comments_on_images(image_ids) - Tags.reindex_tags(Enum.flat_map(changes, &(&1.added_tags ++ &1.removed_tags))) + + changes + |> Enum.flat_map(&(&1.added_tag_ids ++ &1.removed_tag_ids)) + |> Tags.reindex_tags_by_ids() + + IO.inspect(result, label: "Batch update result") result @@ -1153,7 +1156,7 @@ defmodule Philomena.Images do end # Merge any change batches belonging to the same image ID into - # one single batch, then deduplicate added_tags by removing any + # one single batch, then deduplicate added_tag_ids by removing any # which are slated for removal, which is the behavior of the # mass tagger anyway (it inserts anything that needs to be inserted # into image_taggings, and then deletes anything that needs to be deleted, @@ -1166,21 +1169,21 @@ defmodule Philomena.Images do |> Enum.map(fn {image_id, instances} -> added = instances - |> Enum.flat_map(& &1.added_tags) - |> Enum.uniq_by(& &1.id) + |> Enum.flat_map(& &1.added_tag_ids) + |> Enum.uniq() removed = instances - |> Enum.flat_map(& &1.removed_tags) - |> Enum.uniq_by(& &1.id) + |> Enum.flat_map(& &1.removed_tag_ids) + |> Enum.uniq() %{ image_id: image_id, - added_tags: Enum.reject(added, fn a -> Enum.any?(removed, &(&1.id == a.id)) end), - removed_tags: removed + added_tag_ids: Enum.reject(added, fn a -> Enum.member?(removed, a) end), + removed_tag_ids: removed } end) - |> Enum.reject(&(Enum.empty?(&1.added_tags) && Enum.empty?(&1.removed_tags))) + |> Enum.reject(&(Enum.empty?(&1.added_tag_ids) && Enum.empty?(&1.removed_tag_ids))) end # Generate data for TagChanges.Tag struct. diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 6a710db3b..d12ced901 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -21,33 +21,32 @@ defmodule Philomena.TagChanges do inner_join: i in assoc(tc, :image), where: tc.id in ^ids and i.hidden_from_users == false, order_by: [desc: :created_at], - preload: [tags: [:tag]] + preload: [:tags] ) - # Sort tags by tag change creation date, then uniq them by tag ID - # to keep the first, aka the latest, record. Then prepare the struct - # for the batch updater. - changes_per_image = + # Calculate the revert operations for each image. + reverts_per_image = tag_changes |> Enum.group_by(& &1.image_id) |> Enum.map(fn {image_id, tag_changes} -> - changed_tags = + # The tag changes are already sorted by created_at in descending order + # so if we run a `uniq_by` for their tags, we'll leave only the most + # recent change per each tag. + {added_tags, removed_tags} = tag_changes - |> Enum.sort_by(& &1.created_at, :desc) |> Enum.flat_map(& &1.tags) |> Enum.uniq_by(& &1.tag_id) - - {added_tags, removed_tags} = Enum.split_with(changed_tags, & &1.added) + |> Enum.split_with(& &1.added) # We send removed tags to be added, and added to be removed. That's how reverting works! %{ image_id: image_id, - added_tags: Enum.map(removed_tags, & &1.tag), - removed_tags: Enum.map(added_tags, & &1.tag) + added_tag_ids: Enum.map(removed_tags, & &1.tag_id), + removed_tag_ids: Enum.map(added_tags, & &1.tag_id) } end) - Images.batch_update(changes_per_image, attributes) + Images.batch_update(reverts_per_image, attributes) end def full_revert(%{user_id: _user_id, attributes: _attributes} = params), diff --git a/lib/philomena/tags.ex b/lib/philomena/tags.ex index d8a2b3295..06c16ad01 100644 --- a/lib/philomena/tags.ex +++ b/lib/philomena/tags.ex @@ -689,11 +689,17 @@ defmodule Philomena.Tags do """ def reindex_tags(tags) do - Exq.enqueue(Exq, "indexing", IndexWorker, ["Tags", "id", Enum.map(tags, & &1.id)]) + tags + |> Enum.map(& &1.id) + |> reindex_tags_by_ids() tags end + def reindex_tags_by_ids(tag_ids) do + Exq.enqueue(Exq, "indexing", IndexWorker, ["Tags", "id", tag_ids]) + end + @doc """ Returns the list of associations to preload for tag indexing. From 2f3253cfc5bde75ac81575eb3908ec62a0747537 Mon Sep 17 00:00:00 2001 From: MareStare Date: Thu, 15 May 2025 22:24:38 +0000 Subject: [PATCH 03/30] Add a `create` match arm for the case when no tag changes were selected --- .../controllers/tag_change/revert_controller.ex | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/philomena_web/controllers/tag_change/revert_controller.ex b/lib/philomena_web/controllers/tag_change/revert_controller.ex index 2fd94f88f..6f7af639d 100644 --- a/lib/philomena_web/controllers/tag_change/revert_controller.ex +++ b/lib/philomena_web/controllers/tag_change/revert_controller.ex @@ -33,6 +33,13 @@ defmodule PhilomenaWeb.TagChange.RevertController do end end + # Handles the case where no tag changes were selected for submission at all. + def create(conn, _payload) do + conn + |> put_flash(:error, "No tag changes selected.") + |> redirect(external: conn.assigns.referrer) + end + defp verify_authorized(conn, _params) do if Canada.Can.can?(conn.assigns.current_user, :revert, TagChange) do conn From 368951532362755787754ec0b5e1e56e059fcd6a Mon Sep 17 00:00:00 2001 From: MareStare Date: Thu, 15 May 2025 22:58:14 +0000 Subject: [PATCH 04/30] Fix the type error --- lib/philomena/images.ex | 2 -- lib/philomena/tag_changes.ex | 4 +++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index a85ababec..a9ca9a287 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -1146,8 +1146,6 @@ defmodule Philomena.Images do |> Enum.flat_map(&(&1.added_tag_ids ++ &1.removed_tag_ids)) |> Tags.reindex_tags_by_ids() - IO.inspect(result, label: "Batch update result") - result result -> diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index d12ced901..1356aca63 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -46,7 +46,9 @@ defmodule Philomena.TagChanges do } end) - Images.batch_update(reverts_per_image, attributes) + with {:ok, _} <- Images.batch_update(reverts_per_image, attributes) do + {:ok, tag_changes} + end end def full_revert(%{user_id: _user_id, attributes: _attributes} = params), From 76f557adbc775ac9834184bded46e74cb937c153 Mon Sep 17 00:00:00 2001 From: MareStare Date: Thu, 15 May 2025 23:05:44 +0000 Subject: [PATCH 05/30] Add "tags affected" info to the tag revert result flash --- lib/philomena/images.ex | 4 ++-- lib/philomena/tag_changes.ex | 4 ++-- .../controllers/tag_change/revert_controller.ex | 8 ++++++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index a9ca9a287..6b128edb0 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -1138,7 +1138,7 @@ defmodule Philomena.Images do ) end) |> case do - {:ok, _} = result -> + {:ok, {total_tags_affected, _}} -> reindex_images(image_ids) Comments.reindex_comments_on_images(image_ids) @@ -1146,7 +1146,7 @@ defmodule Philomena.Images do |> Enum.flat_map(&(&1.added_tag_ids ++ &1.removed_tag_ids)) |> Tags.reindex_tags_by_ids() - result + {:ok, total_tags_affected} result -> result diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 1356aca63..4db733789 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -46,8 +46,8 @@ defmodule Philomena.TagChanges do } end) - with {:ok, _} <- Images.batch_update(reverts_per_image, attributes) do - {:ok, tag_changes} + with {:ok, total_tags_affected} <- Images.batch_update(reverts_per_image, attributes) do + {:ok, tag_changes, total_tags_affected} end end diff --git a/lib/philomena_web/controllers/tag_change/revert_controller.ex b/lib/philomena_web/controllers/tag_change/revert_controller.ex index 6f7af639d..525b7bba3 100644 --- a/lib/philomena_web/controllers/tag_change/revert_controller.ex +++ b/lib/philomena_web/controllers/tag_change/revert_controller.ex @@ -17,9 +17,13 @@ defmodule PhilomenaWeb.TagChange.RevertController do } case TagChanges.mass_revert(ids, attributes) do - {:ok, tag_changes} -> + {:ok, tag_changes, total_tags_affected} -> conn - |> put_flash(:info, "Successfully reverted #{length(tag_changes)} tag changes.") + |> put_flash( + :info, + "Successfully reverted #{length(tag_changes)} tag changes: " <> + "#{total_tags_affected} tags were actually updated." + ) |> moderation_log( details: &log_details/2, data: %{user: conn.assigns.current_user, count: length(tag_changes)} From 47903ae8d12da98ccf7450b5ae5d11cf96e6f981 Mon Sep 17 00:00:00 2001 From: MareStare Date: Thu, 15 May 2025 23:08:05 +0000 Subject: [PATCH 06/30] To be safe don't change the return type of `Images.batch_update` --- lib/philomena/images.ex | 4 ++-- lib/philomena/tag_changes.ex | 2 +- lib/philomena_web/controllers/tag_change/revert_controller.ex | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index 6b128edb0..a9ca9a287 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -1138,7 +1138,7 @@ defmodule Philomena.Images do ) end) |> case do - {:ok, {total_tags_affected, _}} -> + {:ok, _} = result -> reindex_images(image_ids) Comments.reindex_comments_on_images(image_ids) @@ -1146,7 +1146,7 @@ defmodule Philomena.Images do |> Enum.flat_map(&(&1.added_tag_ids ++ &1.removed_tag_ids)) |> Tags.reindex_tags_by_ids() - {:ok, total_tags_affected} + result result -> result diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 4db733789..57018b7f9 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -46,7 +46,7 @@ defmodule Philomena.TagChanges do } end) - with {:ok, total_tags_affected} <- Images.batch_update(reverts_per_image, attributes) do + with {:ok, {total_tags_affected, _}} <- Images.batch_update(reverts_per_image, attributes) do {:ok, tag_changes, total_tags_affected} end end diff --git a/lib/philomena_web/controllers/tag_change/revert_controller.ex b/lib/philomena_web/controllers/tag_change/revert_controller.ex index 525b7bba3..e91a08737 100644 --- a/lib/philomena_web/controllers/tag_change/revert_controller.ex +++ b/lib/philomena_web/controllers/tag_change/revert_controller.ex @@ -21,8 +21,8 @@ defmodule PhilomenaWeb.TagChange.RevertController do conn |> put_flash( :info, - "Successfully reverted #{length(tag_changes)} tag changes: " <> - "#{total_tags_affected} tags were actually updated." + "Successfully reverted #{length(tag_changes)} tag changes with " <> + "#{total_tags_affected} tags actually updated." ) |> moderation_log( details: &log_details/2, From 38052488ebe5efdf07c6aa555622d3b47317ea9d Mon Sep 17 00:00:00 2001 From: MareStare Date: Fri, 16 May 2025 21:21:01 +0000 Subject: [PATCH 07/30] Add some type specs to `create_image` --- lib/philomena/images.ex | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index a9ca9a287..fe4a5303f 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -69,6 +69,22 @@ defmodule Philomena.Images do |> Enum.map_join(", ", & &1.name) end + @typedoc """ + Result of the `create_image/3` function. The image was created in a DB but + an upload process is probably still running in the background with its PID + given in the `upload_pid` field. + """ + @type image_upload :: %{ + image: Image, + upload_pid: pid + } + + @type performer :: [ + ip: EctoNetwork.INET.t(), + fingerprint: String.t(), + user: User.t() | nil + ] + @doc """ Creates a image. @@ -81,6 +97,7 @@ defmodule Philomena.Images do {:error, %Ecto.Changeset{}} """ + @spec create_image(performer, %{String.t() => any()}) :: {:ok, image_upload()} | {:error, any()} def create_image(attribution, attrs \\ %{}) do tags = Tags.get_or_create_tags(attrs["tag_input"]) sources = attrs["sources"] From 7d9dd4718a76f91370ef7e930afcd8be864f3999 Mon Sep 17 00:00:00 2001 From: MareStare Date: Fri, 16 May 2025 21:39:09 +0000 Subject: [PATCH 08/30] Uninline the `mass_revert_tags` function --- lib/philomena/tag_changes.ex | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 57018b7f9..e9db8e000 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -15,15 +15,22 @@ defmodule Philomena.TagChanges do # Accepts a list of TagChanges.TagChange IDs. def mass_revert(ids, attributes) do - tag_changes = - Repo.all( - from tc in TagChange, - inner_join: i in assoc(tc, :image), - where: tc.id in ^ids and i.hidden_from_users == false, - order_by: [desc: :created_at], - preload: [:tags] - ) + Repo.all( + from tc in TagChange, + inner_join: i in assoc(tc, :image), + where: tc.id in ^ids and i.hidden_from_users == false, + order_by: [desc: :created_at], + preload: [:tags] + ) + |> mass_revert_tags(attributes) + end + # Accepts a list of `TagChange` structs with the `tags` pre-filled. If you + # don't want to revert all tags from a tag change, then filter out some tags + # from the tag changes `tags` fields. + @spec mass_revert_tags([TagChange.t()], map()) :: + {:ok, [TagChange.t()], integer()} | {:error, any()} + def mass_revert_tags(tag_changes, attributes) do # Calculate the revert operations for each image. reverts_per_image = tag_changes From d13e447e9b3cf7798268b37feb7bcf483744c785 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sat, 17 May 2025 17:57:42 +0000 Subject: [PATCH 09/30] Accept [{tag_change_id, tag_id}] in mass_revert_tags` --- lib/philomena/tag_changes.ex | 84 +++++++++++++++++++++++++++++++----- lib/philomena/users.ex | 10 +++++ 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index e9db8e000..f2f05e016 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -12,25 +12,87 @@ defmodule Philomena.TagChanges do alias Philomena.Images alias Philomena.Images.Image alias Philomena.Tags.Tag + alias Philomena.Users - # Accepts a list of TagChanges.TagChange IDs. - def mass_revert(ids, attributes) do + @typedoc """ + In the successful case returns the `TagChange`s that were loaded and affected + plus a non-negative integer with the number of tags affected by the revert. + """ + @type mass_revert_result :: + {:ok, [TagChange.t()], non_neg_integer()} + | {:error, any()} + + @typedoc """ + A tuple with a composite identifier for a `TagChange.Tag`. + """ + @type tag_change_tag_id :: {tag_change_id :: integer(), tag_id :: integer()} + + # Accepts a list of `TagChanges.TagChange` IDs. + @spec mass_revert([integer()], Users.principal()) :: mass_revert_result() + def mass_revert(ids, principal) do Repo.all( from tc in TagChange, inner_join: i in assoc(tc, :image), where: tc.id in ^ids and i.hidden_from_users == false, - order_by: [desc: :created_at], + order_by: [desc: :created_at, desc: tc.id], preload: [:tags] ) - |> mass_revert_tags(attributes) + |> mass_revert_impl(principal) + end + + # Accepts a list of `TagChanges.Tag` IDs. + @spec mass_revert_tags([tag_change_tag_id()], Users.principal()) :: mass_revert_result() + def mass_revert_tags(tag_change_tag_ids, principal) do + {tag_change_ids, tag_ids} = Enum.unzip(tag_change_tag_ids) + + Repo.query!( + """ + WITH + input AS ( + SELECT + unnest($1::integer[]) AS tag_change_id, + unnest($2::integer[]) AS tag_id + ) + SELECT + tag_changes.id, + tag_changes.image_id, + array_agg(row(tag_change_tags.tag_id, tag_change_tags.added)) AS tags + FROM + tag_changes + INNER JOIN + input + ON + tag_changes.id = input.tag_change_id + INNER JOIN + tag_change_tags + ON + tag_changes.id = tag_change_tags.tag_change_id + AND + tag_change_tags.tag_id = input.tag_id + GROUP BY + tag_changes.id + ORDER BY + tag_changes.created_at DESC, + tag_changes.id DESC + """, + [tag_change_ids, tag_ids] + ) + |> Map.get(:rows) + |> Enum.map(fn [tag_change_id, image_id, tags] -> + %TagChange{ + id: tag_change_id, + image_id: image_id, + tags: + Enum.map(tags, fn {tag_id, added} -> %TagChanges.Tag{tag_id: tag_id, added: added} end) + } + end) + |> mass_revert_impl(principal) end - # Accepts a list of `TagChange` structs with the `tags` pre-filled. If you - # don't want to revert all tags from a tag change, then filter out some tags - # from the tag changes `tags` fields. - @spec mass_revert_tags([TagChange.t()], map()) :: - {:ok, [TagChange.t()], integer()} | {:error, any()} - def mass_revert_tags(tag_changes, attributes) do + # Expects that the `tag_changes` are already sorted by created_at in + # descending order. + @spec mass_revert_impl([TagChange.t()], Users.principal()) :: mass_revert_result() + defp mass_revert_impl(tag_changes, principal) do # Calculate the revert operations for each image. reverts_per_image = tag_changes @@ -53,7 +115,7 @@ defmodule Philomena.TagChanges do } end) - with {:ok, {total_tags_affected, _}} <- Images.batch_update(reverts_per_image, attributes) do + with {:ok, {total_tags_affected, _}} <- Images.batch_update(reverts_per_image, principal) do {:ok, tag_changes, total_tags_affected} end end diff --git a/lib/philomena/users.ex b/lib/philomena/users.ex index bd85352c9..5eb593e97 100644 --- a/lib/philomena/users.ex +++ b/lib/philomena/users.ex @@ -21,6 +21,16 @@ defmodule Philomena.Users do alias Philomena.UserEraseWorker alias Philomena.UserRenameWorker + @typedoc """ + Describes the entity performing the action. + The term `principal` was borrowed from AWS IAM terminology. + """ + @type principal :: [ + ip: EctoNetwork.INET.t(), + fingerprint: String.t(), + user: User.t() | nil + ] + ## Database getters @doc """ From 1faeadf3fa1d877fab4a1961b67fad50f39eb91f Mon Sep 17 00:00:00 2001 From: MareStare Date: Sat, 17 May 2025 18:51:07 +0000 Subject: [PATCH 10/30] Use Ecto DSL --- lib/philomena/tag_changes.ex | 70 ++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index f2f05e016..ec547e86c 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -45,46 +45,38 @@ defmodule Philomena.TagChanges do def mass_revert_tags(tag_change_tag_ids, principal) do {tag_change_ids, tag_ids} = Enum.unzip(tag_change_tag_ids) - Repo.query!( - """ - WITH - input AS ( - SELECT - unnest($1::integer[]) AS tag_change_id, - unnest($2::integer[]) AS tag_id - ) - SELECT - tag_changes.id, - tag_changes.image_id, - array_agg(row(tag_change_tags.tag_id, tag_change_tags.added)) AS tags - FROM - tag_changes - INNER JOIN - input - ON - tag_changes.id = input.tag_change_id - INNER JOIN - tag_change_tags - ON - tag_changes.id = tag_change_tags.tag_change_id - AND - tag_change_tags.tag_id = input.tag_id - GROUP BY - tag_changes.id - ORDER BY - tag_changes.created_at DESC, - tag_changes.id DESC - """, - [tag_change_ids, tag_ids] + Repo.all( + from tag_change in TagChange, + inner_join: + input in fragment( + """ + SELECT + unnest(?::integer[]) AS tag_change_id, + unnest(?::integer[]) AS tag_id + """, + ^tag_change_ids, + ^tag_ids + ), + on: tag_change.id == input.tag_change_id, + inner_join: tag_change_tag in TagChanges.Tag, + as: :tag_change_tag, + on: + tag_change_tag.tag_change_id == tag_change.id and + tag_change_tag.tag_id == input.tag_id, + group_by: tag_change.id, + order_by: [desc: tag_change.created_at, desc: tag_change.id], + select: %TagChange{ + id: tag_change.id, + image_id: tag_change.image_id, + tags: fragment("array_agg(row(?, ?))", tag_change_tag.tag_id, tag_change_tag.added) + } ) - |> Map.get(:rows) - |> Enum.map(fn [tag_change_id, image_id, tags] -> - %TagChange{ - id: tag_change_id, - image_id: image_id, - tags: - Enum.map(tags, fn {tag_id, added} -> %TagChanges.Tag{tag_id: tag_id, added: added} end) - } + |> Enum.map(fn tag_change -> + tags = + tag_change.tags + |> Enum.map(fn {tag_id, added} -> %TagChanges.Tag{tag_id: tag_id, added: added} end) + + put_in(tag_change.tags, tags) end) |> mass_revert_impl(principal) end From a661bdccba049190f6fe15b0078b885c8fab83cf Mon Sep 17 00:00:00 2001 From: MareStare Date: Sat, 17 May 2025 22:08:09 +0000 Subject: [PATCH 11/30] Use `principal` terminology in image --- lib/philomena/images.ex | 10 +++------- lib/philomena_web/plugs/user_attribution_plug.ex | 7 +++++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index fe4a5303f..d0e10de6c 100644 --- a/lib/philomena/images.ex +++ b/lib/philomena/images.ex @@ -37,6 +37,7 @@ defmodule Philomena.Images do alias Philomena.Galleries.Gallery alias Philomena.Galleries.Interaction alias Philomena.Users.User + alias Philomena.Users use Philomena.Subscriptions, on_delete: :clear_image_notification, @@ -79,12 +80,6 @@ defmodule Philomena.Images do upload_pid: pid } - @type performer :: [ - ip: EctoNetwork.INET.t(), - fingerprint: String.t(), - user: User.t() | nil - ] - @doc """ Creates a image. @@ -97,7 +92,8 @@ defmodule Philomena.Images do {:error, %Ecto.Changeset{}} """ - @spec create_image(performer, %{String.t() => any()}) :: {:ok, image_upload()} | {:error, any()} + @spec create_image(Users.principal(), %{String.t() => any()}) :: + {:ok, image_upload()} | {:error, any()} def create_image(attribution, attrs \\ %{}) do tags = Tags.get_or_create_tags(attrs["tag_input"]) sources = attrs["sources"] diff --git a/lib/philomena_web/plugs/user_attribution_plug.ex b/lib/philomena_web/plugs/user_attribution_plug.ex index 88bcb75d0..10f04bb1b 100644 --- a/lib/philomena_web/plugs/user_attribution_plug.ex +++ b/lib/philomena_web/plugs/user_attribution_plug.ex @@ -21,14 +21,17 @@ defmodule PhilomenaWeb.UserAttributionPlug do conn = Conn.fetch_cookies(conn) user = conn.assigns.current_user - attributes = [ + # Unfortunately, Elixir has no support for annotating a type of variable, so + # just make sure the shape of this keyword list satisfies the type + # `Philomena.Users.principal` + principal = [ ip: remote_ip, fingerprint: fingerprint(conn, conn.path_info), user: user ] conn - |> Conn.assign(:attributes, attributes) + |> Conn.assign(:attributes, principal) end defp user_agent(conn) do From a2a464daedd112db667f4a1274f795b2cedf3fc0 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sat, 17 May 2025 22:08:36 +0000 Subject: [PATCH 12/30] Start simple tests for tag changes --- docker-compose.yml | 2 + lib/philomena/tag_changes.ex | 2 +- mix.exs | 17 ++- mix.lock | 1 + .../tag_change/revert_controller_test.exs | 111 ++++++++++++++++++ test/support/conn_case.ex | 1 + test/support/fixtures/images_fixtures.ex | 32 +++++ test/support/fixtures/upload-test.png | Bin 0 -> 527 bytes 8 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 test/philomena_web/controllers/tag_change/revert_controller_test.exs create mode 100644 test/support/fixtures/images_fixtures.ex create mode 100644 test/support/fixtures/upload-test.png diff --git a/docker-compose.yml b/docker-compose.yml index 0281aa4a4..cc0760670 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -20,6 +20,8 @@ services: # be a bare `level` which will be used as a catch-all for all other log # events that do not match any of the previous filters. - PHILOMENA_LOG=${PHILOMENA_LOG-Ecto=debug,Exq=none,PhilomenaMedia.Objects=info,debug} + # Set this env var to `y` to regenerate the test snapshots + - ASSERT_VALUE_ACCEPT_DIFFS - MIX_ENV=dev - PGPASSWORD=postgres - ANONYMOUS_NAME_SALT=2fmJRo0OgMFe65kyAJBxPT0QtkVes/jnKDdtP21fexsRqiw8TlSY7yO+uFyMZycp diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index ec547e86c..fb4b7d46c 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -201,7 +201,7 @@ defmodule Philomena.TagChanges do query |> preload([:user, image: [:user, :sources, tags: :aliases], tags: [:tag]]) |> group_by([tc], tc.id) - |> order_by(desc: :created_at) + |> order_by(desc: :created_at, desc: :id) {Repo.paginate(query, pagination), item_count} end diff --git a/mix.exs b/mix.exs index b6b06f1b3..bc22938be 100644 --- a/mix.exs +++ b/mix.exs @@ -93,7 +93,22 @@ defmodule Philomena.MixProject do # Fixes for Elixir v1.15+ {:canary, "~> 1.1", - github: "marcinkoziej/canary", ref: "704debde7a2c0600f78c687807884bf37c45bd79"} + github: "marcinkoziej/canary", ref: "704debde7a2c0600f78c687807884bf37c45bd79"}, + + # Automated testing + { + :assert_value, + "~> 0.10.4", + # This is really stupid but Elixir just doesn't have any mechanism to + # suppress warnings at all, even from code from third-party libraries. + # So we are using the version from + # https://github.com/assert-value/assert_value_elixir/pull/18 which + # fixes the compiler warning "the following clause will never match" + # when using `assert_value`. + only: [:dev, :test], + github: "assert-value/assert_value_elixir", + ref: "4f4e86f0c340d3c29756e68f385ebec85113ba0f" + } ] end diff --git a/mix.lock b/mix.lock index c62f7bf0c..57fc3506a 100644 --- a/mix.lock +++ b/mix.lock @@ -1,4 +1,5 @@ %{ + "assert_value": {:git, "https://github.com/assert-value/assert_value_elixir.git", "4f4e86f0c340d3c29756e68f385ebec85113ba0f", [ref: "4f4e86f0c340d3c29756e68f385ebec85113ba0f"]}, "bandit": {:hex, :bandit, "1.6.11", "2fbadd60c95310eefb4ba7f1e58810aa8956e18c664a3b2029d57edb7d28d410", [:mix], [{:hpax, "~> 1.0", [hex: :hpax, repo: "hexpm", optional: false]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}, {:thousand_island, "~> 1.0", [hex: :thousand_island, repo: "hexpm", optional: false]}, {:websock, "~> 0.5", [hex: :websock, repo: "hexpm", optional: false]}], "hexpm", "543f3f06b4721619a1220bed743aa77bf7ecc9c093ba9fab9229ff6b99eacc65"}, "bcrypt_elixir": {:hex, :bcrypt_elixir, "3.3.1", "9f2e7e00f661a6acfae1431f1bc590e28698aaecc962c2a7b33150dfe9289c3d", [:make, :mix], [{:comeonin, "~> 5.3", [hex: :comeonin, repo: "hexpm", optional: false]}, {:elixir_make, "~> 0.6", [hex: :elixir_make, repo: "hexpm", optional: false]}], "hexpm", "9f539e9d3828fad4ffc8152dadc0d27c6d78cb2853a9a1d6518cfe8a5adb7f50"}, "briefly": {:hex, :briefly, "0.5.1", "ee10d48da7f79ed2aebdc3e536d5f9a0c3e36ff76c0ad0d4254653a152b13a8a", [:mix], [], "hexpm", "bd684aa92ad8b7b4e0d92c31200993c4bc1469fc68cd6d5f15144041bd15cb57"}, diff --git a/test/philomena_web/controllers/tag_change/revert_controller_test.exs b/test/philomena_web/controllers/tag_change/revert_controller_test.exs new file mode 100644 index 000000000..c9a91a055 --- /dev/null +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -0,0 +1,111 @@ +defmodule PhilomenaWeb.TagChange.RevertControllerTest do + use PhilomenaWeb.ConnCase, async: true + + alias Philomena.Users + alias Philomena.TagChanges + alias Philomena.Images + alias Philomena.Images.Image + alias Philomena.Repo + import Philomena.UsersFixtures + import Philomena.ImagesFixtures + import Ecto.Query + + setup :register_and_log_in_user + + describe "POST /tag_changes/revert" do + test "reverts tag changes", %{conn: conn, user: user} do + # TODO: make the user authorized, otherwise the test may fail due to + # exceeding the rate limits for tag changes + + image = image_fixture(user) + + {conn, image} = + update_tags(conn, image, add: ["tag3", "tag4"]) + + assert_value(load_tag_changes(image) == [["+tag3 (images: 1)", "+tag4 (images: 1)"]]) + + {conn, image} = update_tags(conn, image, remove: ["tag3", "tag4"]) + + assert_value( + load_tag_changes(image) == [ + ["-tag3 (images: 0)", "-tag4 (images: 0)"], + ["+tag3 (images: 0)", "+tag4 (images: 0)"] + ] + ) + + {_conn, image} = update_tags(conn, image, add: ["tag3"], remove: ["tag2"]) + + assert_value( + load_tag_changes(image) == [ + ["+tag3 (images: 1)", "-tag2 (images: 0)"], + ["-tag3 (images: 1)", "-tag4 (images: 0)"], + ["+tag3 (images: 1)", "+tag4 (images: 0)"] + ] + ) + end + end + + defp load_tag_changes(image) do + TagChanges.load( + %{ + field: :image_id, + value: image.id + }, + nil + ).entries + |> Enum.map(&tag_change_to_snap/1) + end + + defp tag_change_to_snap(%TagChanges.TagChange{} = tag_change) do + tag_change.tags + |> Enum.map(fn %{tag: tag, added: added} -> + "#{if(added, do: "+", else: "-")}#{tag.name} (images: #{tag.images_count})" + end) + end + + defp update_tags(conn, image, diff) do + added_tags = Keyword.get(diff, :add, []) + removed_tags = Keyword.get(diff, :remove, []) + current_tags = image.tags |> Enum.map(& &1.name) + + for tag <- added_tags do + assert tag not in current_tags + end + + new_tags = + current_tags + |> Enum.reject(&(&1 in removed_tags)) + |> Enum.concat(added_tags) + + conn = + conn + |> post(~p"/images/#{image.id}/tags", %{ + "_method" => "put", + "image" => %{ + "old_tag_input" => current_tags |> Enum.join(", "), + "tag_input" => new_tags |> Enum.join(", ") + } + }) + + response = html_response(conn, 200) + + # `help-block` is returned to display an error to the user + assert not String.contains?(response, "class=\"help-block\"") + + for tag <- new_tags |> Enum.reject(&(not String.starts_with?(&1, "-"))) do + assert response =~ tag + end + + for tag <- new_tags |> Enum.filter(&String.starts_with?(&1, "-")) do + assert response =~ tag |> String.replace_leading("-", "") + end + + image = + Image + |> where(id: ^image.id) + |> preload([:tags]) + |> Repo.one() + + {conn, image} + end +end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index dd4240a4d..986cac7d3 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -21,6 +21,7 @@ defmodule PhilomenaWeb.ConnCase do import Plug.Conn import Phoenix.ConnTest import PhilomenaWeb.ConnCase + import AssertValue # The default endpoint for testing @endpoint PhilomenaWeb.Endpoint diff --git a/test/support/fixtures/images_fixtures.ex b/test/support/fixtures/images_fixtures.ex new file mode 100644 index 000000000..d54c751ab --- /dev/null +++ b/test/support/fixtures/images_fixtures.ex @@ -0,0 +1,32 @@ +defmodule Philomena.ImagesFixtures do + @moduledoc """ + This module defines test helpers for creating + entities via the `Philomena.Images` context. + """ + + alias Philomena.Images + alias Philomena.Users.User + + @default_image_attrs %{ + "image" => %Plug.Upload{ + path: "#{__DIR__}/upload-test.png", + filename: "upload-test.png", + content_type: "image/png" + }, + "tag_input" => "safe,tag1,tag2", + "sources" => ["https://localhost/images_fixtures.ex"] + } + + @spec image_fixture(User.t(), map()) :: Images.Image.t() + def image_fixture(uploader, attrs \\ %{}) do + principal = [ + user: uploader + ] + + attrs = Map.merge(@default_image_attrs, attrs) + + {:ok, %{image: image}} = Images.create_image(principal, attrs) + + image + end +end diff --git a/test/support/fixtures/upload-test.png b/test/support/fixtures/upload-test.png new file mode 100644 index 0000000000000000000000000000000000000000..770601f791c9ab903b5e3512dc49b685b638a34f GIT binary patch literal 527 zcmV+q0`UEbP)&Q5?tduUZsQ6jTt=a0jg=mx^dK3nVZQGtg@2K74`qD)%ZajzO!?&|}GJ zXlbssp*0AC9uUn9O+_ssw|kIM0)zh3hyOYM!#TeL?rKiet+oK@M$wFhf>J!OB6U2| z#vpz87?V}2FdK=4X;~k)xBzba;w=7GJzCOI!6g9!wO|$^Fq($mwHEL0Y|ibkJ>U6#0m1Zg z#*`8=)c^nh32;bRa{vG?BLDy{BLR4&KXw2B00(qQO+^Rj1Qie_2jv)t)c^nh8FWQh zbVF}#ZDnqB07G(RVRU6=Aa`kWXdp*PO;A^X4i^9b01Qb)K~xCWWBAX&000940RR}? Rjj#X!002ovPDHLkV1kc2)Z+jE literal 0 HcmV?d00001 From 8c675a705ad008c78df705953650ee1f7bf6779c Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 13:36:28 +0000 Subject: [PATCH 13/30] Test add/remove combinations --- .../controllers/image/tag_controller_test.exs | 81 +++++++++++++ .../tag_change/revert_controller_test.exs | 111 ------------------ test/support/fixtures/images_fixtures.ex | 32 ----- test/support/fixtures/users_fixtures.ex | 46 ++++++-- .../upload-test.png => philomena/dummy.png} | Bin test/support/philomena/images.ex | 42 +++++++ test/support/philomena/pagination.ex | 28 +++++ test/support/philomena/tag_changes.ex | 29 +++++ test/support/philomena_web/images.ex | 62 ++++++++++ 9 files changed, 281 insertions(+), 150 deletions(-) create mode 100644 test/philomena_web/controllers/image/tag_controller_test.exs delete mode 100644 test/support/fixtures/images_fixtures.ex rename test/support/{fixtures/upload-test.png => philomena/dummy.png} (100%) create mode 100644 test/support/philomena/images.ex create mode 100644 test/support/philomena/pagination.ex create mode 100644 test/support/philomena/tag_changes.ex create mode 100644 test/support/philomena_web/images.ex diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs new file mode 100644 index 000000000..c8e09738b --- /dev/null +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -0,0 +1,81 @@ +defmodule PhilomenaWeb.Image.TagControllerTest do + use PhilomenaWeb.ConnCase, async: true + + alias PhilomenaWeb.Test, as: WebTest + alias Philomena.Test + alias Philomena.UsersFixtures + + setup ctx do + # We need a verified user, because it bypasses the rate limits check + user = UsersFixtures.user_fixture(%{confirmed: true, verified: true}) + %{conn: log_in_user(ctx.conn, user), user: user} + end + + describe "POST /image/{N}/tags" do + test "add/remove combinations", %{conn: conn, user: user} do + ctx = %{ + conn: conn, + image: + Test.Images.create_image(user, %{ + "tag_input" => "safe,a,b" + }) + } + + ctx = update_image_tags(ctx, add: ["c", "d", "e", "f"]) + + assert_value( + snap(ctx) == [ + "Image #1: [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1] [f 1]", + "TagChange #1: +[c 1] +[d 1] +[e 1] +[f 1]" + ] + ) + + ctx = update_image_tags(ctx, add: ["g", "h"], remove: ["a", "e"]) + + assert_value( + snap(ctx) == [ + "Image #1: [b 1] [safe 1] [c 1] [d 1] [f 1] [g 1] [h 1]", + "TagChange #2: +[g 1] +[h 1] -[a 0] -[e 0]", + "TagChange #1: +[c 1] +[d 1] +[e 0] +[f 1]" + ] + ) + + # Noop should not create a new tag change + ctx = update_image_tags(ctx, add: []) + + assert_value( + snap(ctx) == [ + "Image #1: [b 1] [safe 1] [c 1] [d 1] [f 1] [g 1] [h 1]", + "TagChange #2: +[g 1] +[h 1] -[a 0] -[e 0]", + "TagChange #1: +[c 1] +[d 1] +[e 0] +[f 1]" + ] + ) + end + + defp update_image_tags(ctx, diff) do + conn = + WebTest.Images.update_image_tags(ctx.conn, ctx.image, diff) + + image = Test.Images.load_image!(ctx.image.id, preload: [:tags]) + + %{ + conn: conn, + image: image + } + end + + defp snap(ctx) do + image_tags = + ctx.image.tags + |> Enum.map(fn tag -> "[#{tag.name} #{tag.images_count}]" end) + |> Enum.join(" ") + + tag_changes = + ctx.image.id + |> Test.TagChanges.load_tag_changes_by_image_id() + |> Enum.map(&Test.TagChanges.to_snap/1) + + ["Image ##{ctx.image.id}: #{image_tags}"] ++ tag_changes + end + end +end diff --git a/test/philomena_web/controllers/tag_change/revert_controller_test.exs b/test/philomena_web/controllers/tag_change/revert_controller_test.exs index c9a91a055..e69de29bb 100644 --- a/test/philomena_web/controllers/tag_change/revert_controller_test.exs +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -1,111 +0,0 @@ -defmodule PhilomenaWeb.TagChange.RevertControllerTest do - use PhilomenaWeb.ConnCase, async: true - - alias Philomena.Users - alias Philomena.TagChanges - alias Philomena.Images - alias Philomena.Images.Image - alias Philomena.Repo - import Philomena.UsersFixtures - import Philomena.ImagesFixtures - import Ecto.Query - - setup :register_and_log_in_user - - describe "POST /tag_changes/revert" do - test "reverts tag changes", %{conn: conn, user: user} do - # TODO: make the user authorized, otherwise the test may fail due to - # exceeding the rate limits for tag changes - - image = image_fixture(user) - - {conn, image} = - update_tags(conn, image, add: ["tag3", "tag4"]) - - assert_value(load_tag_changes(image) == [["+tag3 (images: 1)", "+tag4 (images: 1)"]]) - - {conn, image} = update_tags(conn, image, remove: ["tag3", "tag4"]) - - assert_value( - load_tag_changes(image) == [ - ["-tag3 (images: 0)", "-tag4 (images: 0)"], - ["+tag3 (images: 0)", "+tag4 (images: 0)"] - ] - ) - - {_conn, image} = update_tags(conn, image, add: ["tag3"], remove: ["tag2"]) - - assert_value( - load_tag_changes(image) == [ - ["+tag3 (images: 1)", "-tag2 (images: 0)"], - ["-tag3 (images: 1)", "-tag4 (images: 0)"], - ["+tag3 (images: 1)", "+tag4 (images: 0)"] - ] - ) - end - end - - defp load_tag_changes(image) do - TagChanges.load( - %{ - field: :image_id, - value: image.id - }, - nil - ).entries - |> Enum.map(&tag_change_to_snap/1) - end - - defp tag_change_to_snap(%TagChanges.TagChange{} = tag_change) do - tag_change.tags - |> Enum.map(fn %{tag: tag, added: added} -> - "#{if(added, do: "+", else: "-")}#{tag.name} (images: #{tag.images_count})" - end) - end - - defp update_tags(conn, image, diff) do - added_tags = Keyword.get(diff, :add, []) - removed_tags = Keyword.get(diff, :remove, []) - current_tags = image.tags |> Enum.map(& &1.name) - - for tag <- added_tags do - assert tag not in current_tags - end - - new_tags = - current_tags - |> Enum.reject(&(&1 in removed_tags)) - |> Enum.concat(added_tags) - - conn = - conn - |> post(~p"/images/#{image.id}/tags", %{ - "_method" => "put", - "image" => %{ - "old_tag_input" => current_tags |> Enum.join(", "), - "tag_input" => new_tags |> Enum.join(", ") - } - }) - - response = html_response(conn, 200) - - # `help-block` is returned to display an error to the user - assert not String.contains?(response, "class=\"help-block\"") - - for tag <- new_tags |> Enum.reject(&(not String.starts_with?(&1, "-"))) do - assert response =~ tag - end - - for tag <- new_tags |> Enum.filter(&String.starts_with?(&1, "-")) do - assert response =~ tag |> String.replace_leading("-", "") - end - - image = - Image - |> where(id: ^image.id) - |> preload([:tags]) - |> Repo.one() - - {conn, image} - end -end diff --git a/test/support/fixtures/images_fixtures.ex b/test/support/fixtures/images_fixtures.ex deleted file mode 100644 index d54c751ab..000000000 --- a/test/support/fixtures/images_fixtures.ex +++ /dev/null @@ -1,32 +0,0 @@ -defmodule Philomena.ImagesFixtures do - @moduledoc """ - This module defines test helpers for creating - entities via the `Philomena.Images` context. - """ - - alias Philomena.Images - alias Philomena.Users.User - - @default_image_attrs %{ - "image" => %Plug.Upload{ - path: "#{__DIR__}/upload-test.png", - filename: "upload-test.png", - content_type: "image/png" - }, - "tag_input" => "safe,tag1,tag2", - "sources" => ["https://localhost/images_fixtures.ex"] - } - - @spec image_fixture(User.t(), map()) :: Images.Image.t() - def image_fixture(uploader, attrs \\ %{}) do - principal = [ - user: uploader - ] - - attrs = Map.merge(@default_image_attrs, attrs) - - {:ok, %{image: image}} = Images.create_image(principal, attrs) - - image - end -end diff --git a/test/support/fixtures/users_fixtures.ex b/test/support/fixtures/users_fixtures.ex index 76a2ff917..f866153ad 100644 --- a/test/support/fixtures/users_fixtures.ex +++ b/test/support/fixtures/users_fixtures.ex @@ -5,6 +5,7 @@ defmodule Philomena.UsersFixtures do """ alias Philomena.Users + alias Philomena.Users.User alias Philomena.Repo def unique_user_email, do: "user#{System.unique_integer()}@example.com" @@ -13,6 +14,18 @@ defmodule Philomena.UsersFixtures do def user_fixture(attrs \\ %{}) do email = unique_user_email() + {role, attrs} = Map.pop(attrs, :role) + role = role || "user" + + {confirmed, attrs} = Map.pop(attrs, :confirmed) + confirmed = confirmed || role != "user" + + {verified, attrs} = Map.pop(attrs, :verified) + verified = verified || role != "user" + + {locked, attrs} = Map.pop(attrs, :locked) + locked = locked || false + {:ok, user} = attrs |> Enum.into(%{ @@ -22,19 +35,38 @@ defmodule Philomena.UsersFixtures do }) |> Users.register_user() - user + updates = + [ + if role != "user" do + fn user -> + user + |> Repo.preload(:roles) + |> User.update_changeset(%{role: role}, []) + end + end, + if(confirmed, do: &User.confirm_changeset/1), + if(verified, do: &User.verify_changeset/1), + if(locked, do: &User.lock_changeset/1) + ] + |> Enum.reject(&is_nil/1) + + case updates do + [] -> + user + + _ -> + updates + |> Enum.reduce(user, fn update, user -> update.(user) end) + |> Repo.update!() + end end def confirmed_user_fixture(attrs \\ %{}) do - user_fixture(attrs) - |> Users.User.confirm_changeset() - |> Repo.update!() + user_fixture(Map.put(attrs, :confirmed, true)) end def locked_user_fixture(attrs \\ %{}) do - user_fixture(attrs) - |> Users.User.lock_changeset() - |> Repo.update!() + user_fixture(Map.put(attrs, :locked, true)) end def extract_user_token(fun) do diff --git a/test/support/fixtures/upload-test.png b/test/support/philomena/dummy.png similarity index 100% rename from test/support/fixtures/upload-test.png rename to test/support/philomena/dummy.png diff --git a/test/support/philomena/images.ex b/test/support/philomena/images.ex new file mode 100644 index 000000000..b857f825e --- /dev/null +++ b/test/support/philomena/images.ex @@ -0,0 +1,42 @@ +defmodule Philomena.Test.Images do + alias Philomena.Repo + alias Philomena.Images.Image + alias Philomena.Images + + import Ecto.Query + + @doc """ + Loads an image by ID and preloads the specified associations. + Raises `Ecto.NoResultsError` if the image is not found. + """ + @spec load_image!(integer(), preload: [atom()]) :: Image.t() + def load_image!(id, opts) do + Image + |> where(id: ^id) + |> preload(^Keyword.get(opts, :preload, [])) + |> Repo.one() + end + + @spec create_image(User.t(), map()) :: Images.Image.t() + def create_image(uploader, attrs \\ %{}) do + attrs = + %{ + "image" => %Plug.Upload{ + path: "#{__DIR__}/dummy.png", + filename: "dummy.png", + content_type: "image/png" + }, + "tag_input" => "safe,a,b", + "sources" => ["https://localhost/images_fixtures.ex"] + } + |> Map.merge(attrs) + + principal = [ + user: uploader + ] + + {:ok, %{image: image}} = Images.create_image(principal, attrs) + + image + end +end diff --git a/test/support/philomena/pagination.ex b/test/support/philomena/pagination.ex new file mode 100644 index 000000000..19a07cfe6 --- /dev/null +++ b/test/support/philomena/pagination.ex @@ -0,0 +1,28 @@ +defmodule Philomena.Test.Pagination do + @type page_params() :: [ + page: non_neg_integer(), + page_size: non_neg_integer() + ] + + @type load_page_fn(t) :: (page_params() -> Scrivener.Page.t(t)) + + @spec load_all(load_page_fn(t), page_params()) :: [t] + when t: var + def load_all(load_page, page_params \\ []) do + load_all_recurse(load_page, page_params, []) + end + + @spec load_all_recurse(load_page_fn(t), page_params(), [t]) :: [t] + when t: var + defp load_all_recurse(load_page, page_params, acc) do + page = load_page.(page_params) + acc = acc ++ page.entries + + if page.page_number >= page.total_pages do + acc + else + next_page_params = Keyword.put(page_params, :page, page.page_number + 1) + load_all_recurse(load_page, next_page_params, acc) + end + end +end diff --git a/test/support/philomena/tag_changes.ex b/test/support/philomena/tag_changes.ex new file mode 100644 index 000000000..61100154d --- /dev/null +++ b/test/support/philomena/tag_changes.ex @@ -0,0 +1,29 @@ +defmodule Philomena.Test.TagChanges do + alias Philomena.TagChanges + alias Philomena.Test + + @spec load_tag_changes_by_image_id(integer()) :: [TagChanges.TagChange.t()] + def load_tag_changes_by_image_id(image_id) do + fn page_params -> + TagChanges.load( + %{ + field: :image_id, + value: image_id + }, + page_params + ) + end + |> Test.Pagination.load_all() + end + + def to_snap(%TagChanges.TagChange{} = tag_change) do + suffix = + tag_change.tags + |> Enum.map(fn %{tag: tag, added: added} -> + "#{if(added, do: "+", else: "-")}[#{tag.name} #{tag.images_count}]" + end) + |> Enum.join(" ") + + "TagChange ##{tag_change.id}: #{suffix}" + end +end diff --git a/test/support/philomena_web/images.ex b/test/support/philomena_web/images.ex new file mode 100644 index 000000000..5602ab949 --- /dev/null +++ b/test/support/philomena_web/images.ex @@ -0,0 +1,62 @@ +defmodule PhilomenaWeb.Test.Images do + alias Philomena.Images.Image + + import Phoenix.ConnTest + + use ExUnit.Case + + @endpoint PhilomenaWeb.Endpoint + + use PhilomenaWeb, :verified_routes + + @type tags_diff :: [ + add: [String.t()], + remove: [String.t()] + ] + + @spec update_image_tags(Plug.Conn.t(), Image.t(), tags_diff()) :: + Plug.Conn.t() + def update_image_tags(conn, image, diff) do + added_tags = Keyword.get(diff, :add, []) + removed_tags = Keyword.get(diff, :remove, []) + current_tags = image.tags |> Enum.map(& &1.name) + + for tag <- added_tags do + assert tag not in current_tags + end + + new_tags = + current_tags + |> Enum.reject(&(&1 in removed_tags)) + |> Enum.concat(added_tags) + + conn = + conn + |> post(~p"/images/#{image.id}/tags", %{ + "_method" => "put", + "image" => %{ + "old_tag_input" => current_tags |> Enum.join(", "), + "tag_input" => new_tags |> Enum.join(", ") + } + }) + + if conn.status != 200 do + raise "Failed to update image tags (#{conn.status}): #{inspect(conn.assigns.flash)}" + end + + response = response_content_type(conn, :html) + + # `help-block` is returned to display an error to the user + assert not String.contains?(response, "class=\"help-block\"") + + for tag <- new_tags |> Enum.reject(&(not String.starts_with?(&1, "-"))) do + assert response =~ tag + end + + for tag <- new_tags |> Enum.filter(&String.starts_with?(&1, "-")) do + assert response =~ tag |> String.replace_leading("-", "") + end + + conn + end +end From e1b45fb5ebf5ae37ad1bd49674a8e85e5e4ac0ba Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 13:40:22 +0000 Subject: [PATCH 14/30] Reduce the data slightly --- .../controllers/image/tag_controller_test.exs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs index c8e09738b..c33db8b35 100644 --- a/test/philomena_web/controllers/image/tag_controller_test.exs +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -21,22 +21,22 @@ defmodule PhilomenaWeb.Image.TagControllerTest do }) } - ctx = update_image_tags(ctx, add: ["c", "d", "e", "f"]) + ctx = update_image_tags(ctx, add: ["c", "d", "e"]) assert_value( snap(ctx) == [ - "Image #1: [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1] [f 1]", - "TagChange #1: +[c 1] +[d 1] +[e 1] +[f 1]" + "Image #1: [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1]", + "TagChange #1: +[c 1] +[d 1] +[e 1]" ] ) - ctx = update_image_tags(ctx, add: ["g", "h"], remove: ["a", "e"]) + ctx = update_image_tags(ctx, add: ["f", "g"], remove: ["a", "d"]) assert_value( snap(ctx) == [ - "Image #1: [b 1] [safe 1] [c 1] [d 1] [f 1] [g 1] [h 1]", - "TagChange #2: +[g 1] +[h 1] -[a 0] -[e 0]", - "TagChange #1: +[c 1] +[d 1] +[e 0] +[f 1]" + "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", + "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", + "TagChange #1: +[c 1] +[d 0] +[e 1]" ] ) @@ -45,9 +45,9 @@ defmodule PhilomenaWeb.Image.TagControllerTest do assert_value( snap(ctx) == [ - "Image #1: [b 1] [safe 1] [c 1] [d 1] [f 1] [g 1] [h 1]", - "TagChange #2: +[g 1] +[h 1] -[a 0] -[e 0]", - "TagChange #1: +[c 1] +[d 1] +[e 0] +[f 1]" + "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", + "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", + "TagChange #1: +[c 1] +[d 0] +[e 1]" ] ) end From e814a93ed88d49a678df8401e873f2553b66f93f Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 13:41:50 +0000 Subject: [PATCH 15/30] Refactor --- .../controllers/image/tag_controller_test.exs | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs index c33db8b35..21030f1bc 100644 --- a/test/philomena_web/controllers/image/tag_controller_test.exs +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -32,8 +32,10 @@ defmodule PhilomenaWeb.Image.TagControllerTest do ctx = update_image_tags(ctx, add: ["f", "g"], remove: ["a", "d"]) + snap = snap(ctx) + assert_value( - snap(ctx) == [ + snap == [ "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", "TagChange #1: +[c 1] +[d 0] +[e 1]" @@ -41,15 +43,7 @@ defmodule PhilomenaWeb.Image.TagControllerTest do ) # Noop should not create a new tag change - ctx = update_image_tags(ctx, add: []) - - assert_value( - snap(ctx) == [ - "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", - "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", - "TagChange #1: +[c 1] +[d 0] +[e 1]" - ] - ) + assert snap(update_image_tags(ctx, add: [])) == snap end defp update_image_tags(ctx, diff) do From 1ba328360e429e85f4bc64e62416385523577b07 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 13:43:39 +0000 Subject: [PATCH 16/30] Denest --- .../controllers/image/tag_controller_test.exs | 94 +++++++++---------- 1 file changed, 46 insertions(+), 48 deletions(-) diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs index 21030f1bc..e09ab295a 100644 --- a/test/philomena_web/controllers/image/tag_controller_test.exs +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -11,65 +11,63 @@ defmodule PhilomenaWeb.Image.TagControllerTest do %{conn: log_in_user(ctx.conn, user), user: user} end - describe "POST /image/{N}/tags" do - test "add/remove combinations", %{conn: conn, user: user} do - ctx = %{ - conn: conn, - image: - Test.Images.create_image(user, %{ - "tag_input" => "safe,a,b" - }) - } + test "POST /image/{N}/tags", %{conn: conn, user: user} do + ctx = %{ + conn: conn, + image: + Test.Images.create_image(user, %{ + "tag_input" => "safe,a,b" + }) + } - ctx = update_image_tags(ctx, add: ["c", "d", "e"]) + ctx = update_image_tags(ctx, add: ["c", "d", "e"]) - assert_value( - snap(ctx) == [ - "Image #1: [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1]", - "TagChange #1: +[c 1] +[d 1] +[e 1]" - ] - ) + assert_value( + snap(ctx) == [ + "Image #1: [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1]", + "TagChange #1: +[c 1] +[d 1] +[e 1]" + ] + ) - ctx = update_image_tags(ctx, add: ["f", "g"], remove: ["a", "d"]) + ctx = update_image_tags(ctx, add: ["f", "g"], remove: ["a", "d"]) - snap = snap(ctx) + snap = snap(ctx) - assert_value( - snap == [ - "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", - "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", - "TagChange #1: +[c 1] +[d 0] +[e 1]" - ] - ) + assert_value( + snap == [ + "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", + "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", + "TagChange #1: +[c 1] +[d 0] +[e 1]" + ] + ) - # Noop should not create a new tag change - assert snap(update_image_tags(ctx, add: [])) == snap - end + # Noop should not create a new tag change + assert snap(update_image_tags(ctx, add: [])) == snap + end - defp update_image_tags(ctx, diff) do - conn = - WebTest.Images.update_image_tags(ctx.conn, ctx.image, diff) + defp update_image_tags(ctx, diff) do + conn = + WebTest.Images.update_image_tags(ctx.conn, ctx.image, diff) - image = Test.Images.load_image!(ctx.image.id, preload: [:tags]) + image = Test.Images.load_image!(ctx.image.id, preload: [:tags]) - %{ - conn: conn, - image: image - } - end + %{ + conn: conn, + image: image + } + end - defp snap(ctx) do - image_tags = - ctx.image.tags - |> Enum.map(fn tag -> "[#{tag.name} #{tag.images_count}]" end) - |> Enum.join(" ") + defp snap(ctx) do + image_tags = + ctx.image.tags + |> Enum.map(fn tag -> "[#{tag.name} #{tag.images_count}]" end) + |> Enum.join(" ") - tag_changes = - ctx.image.id - |> Test.TagChanges.load_tag_changes_by_image_id() - |> Enum.map(&Test.TagChanges.to_snap/1) + tag_changes = + ctx.image.id + |> Test.TagChanges.load_tag_changes_by_image_id() + |> Enum.map(&Test.TagChanges.to_snap/1) - ["Image ##{ctx.image.id}: #{image_tags}"] ++ tag_changes - end + ["Image ##{ctx.image.id}: #{image_tags}"] ++ tag_changes end end From 9ff5c75e8981b74fec31e0e9328575ce23a7845b Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 13:56:25 +0000 Subject: [PATCH 17/30] Move stuff into reusable `snap` functions --- .../controllers/image/tag_controller_test.exs | 19 +++++++------------ test/support/philomena/images.ex | 10 ++++++++++ test/support/philomena/tag_changes.ex | 6 +++--- test/support/philomena/tags.ex | 7 +++++++ 4 files changed, 27 insertions(+), 15 deletions(-) create mode 100644 test/support/philomena/tags.ex diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs index e09ab295a..bf2024df6 100644 --- a/test/philomena_web/controllers/image/tag_controller_test.exs +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -24,8 +24,8 @@ defmodule PhilomenaWeb.Image.TagControllerTest do assert_value( snap(ctx) == [ - "Image #1: [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1]", - "TagChange #1: +[c 1] +[d 1] +[e 1]" + "Image(1): [a 1] [b 1] [safe 1] [c 1] [d 1] [e 1]", + "TagChange(1): +[c 1] +[d 1] +[e 1]" ] ) @@ -35,9 +35,9 @@ defmodule PhilomenaWeb.Image.TagControllerTest do assert_value( snap == [ - "Image #1: [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", - "TagChange #2: +[f 1] +[g 1] -[a 0] -[d 0]", - "TagChange #1: +[c 1] +[d 0] +[e 1]" + "Image(1): [b 1] [safe 1] [c 1] [e 1] [f 1] [g 1]", + "TagChange(2): +[f 1] +[g 1] -[a 0] -[d 0]", + "TagChange(1): +[c 1] +[d 0] +[e 1]" ] ) @@ -58,16 +58,11 @@ defmodule PhilomenaWeb.Image.TagControllerTest do end defp snap(ctx) do - image_tags = - ctx.image.tags - |> Enum.map(fn tag -> "[#{tag.name} #{tag.images_count}]" end) - |> Enum.join(" ") - tag_changes = ctx.image.id |> Test.TagChanges.load_tag_changes_by_image_id() - |> Enum.map(&Test.TagChanges.to_snap/1) + |> Enum.map(&Test.TagChanges.snap/1) - ["Image ##{ctx.image.id}: #{image_tags}"] ++ tag_changes + [Test.Images.snap(ctx.image)] ++ tag_changes end end diff --git a/test/support/philomena/images.ex b/test/support/philomena/images.ex index b857f825e..b2f55ccad 100644 --- a/test/support/philomena/images.ex +++ b/test/support/philomena/images.ex @@ -2,6 +2,7 @@ defmodule Philomena.Test.Images do alias Philomena.Repo alias Philomena.Images.Image alias Philomena.Images + alias Philomena.Test import Ecto.Query @@ -39,4 +40,13 @@ defmodule Philomena.Test.Images do image end + + def snap(%Image{} = image) do + image_tags = + image.tags + |> Enum.map(&Test.Tags.snap/1) + |> Enum.join(" ") + + "Image(#{image.id}): #{image_tags}" + end end diff --git a/test/support/philomena/tag_changes.ex b/test/support/philomena/tag_changes.ex index 61100154d..8696ec286 100644 --- a/test/support/philomena/tag_changes.ex +++ b/test/support/philomena/tag_changes.ex @@ -16,14 +16,14 @@ defmodule Philomena.Test.TagChanges do |> Test.Pagination.load_all() end - def to_snap(%TagChanges.TagChange{} = tag_change) do + def snap(%TagChanges.TagChange{} = tag_change) do suffix = tag_change.tags |> Enum.map(fn %{tag: tag, added: added} -> - "#{if(added, do: "+", else: "-")}[#{tag.name} #{tag.images_count}]" + "#{if(added, do: "+", else: "-")}#{Test.Tags.snap(tag)}" end) |> Enum.join(" ") - "TagChange ##{tag_change.id}: #{suffix}" + "TagChange(#{tag_change.id}): #{suffix}" end end diff --git a/test/support/philomena/tags.ex b/test/support/philomena/tags.ex new file mode 100644 index 000000000..be35227bd --- /dev/null +++ b/test/support/philomena/tags.ex @@ -0,0 +1,7 @@ +defmodule Philomena.Test.Tags do + alias Philomena.Tags.Tag + + def snap(%Tag{} = tag) do + "[#{tag.name} #{tag.images_count}]" + end +end From e28125037f3bf46746454f660b055ae583bfc82e Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 13:58:12 +0000 Subject: [PATCH 18/30] More comments --- test/support/philomena/pagination.ex | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/support/philomena/pagination.ex b/test/support/philomena/pagination.ex index 19a07cfe6..dbcdb02e0 100644 --- a/test/support/philomena/pagination.ex +++ b/test/support/philomena/pagination.ex @@ -6,6 +6,10 @@ defmodule Philomena.Test.Pagination do @type load_page_fn(t) :: (page_params() -> Scrivener.Page.t(t)) + @doc """ + Load all pages into a single flat list. Accepts a function that loads a + page of data with the given `page_params()`. + """ @spec load_all(load_page_fn(t), page_params()) :: [t] when t: var def load_all(load_page, page_params \\ []) do From 1b524dc628af01726dedcb76b43a28604bd7f33c Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 14:09:49 +0000 Subject: [PATCH 19/30] More refactoring in the sake of god of refactoring --- .../controllers/image/tag_controller_test.exs | 24 +------------ .../{images.ex => tag_changes.ex} | 35 ++++++++++++++----- 2 files changed, 28 insertions(+), 31 deletions(-) rename test/support/philomena_web/{images.ex => tag_changes.ex} (63%) diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs index bf2024df6..9e05cf872 100644 --- a/test/philomena_web/controllers/image/tag_controller_test.exs +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -1,9 +1,8 @@ defmodule PhilomenaWeb.Image.TagControllerTest do use PhilomenaWeb.ConnCase, async: true - - alias PhilomenaWeb.Test, as: WebTest alias Philomena.Test alias Philomena.UsersFixtures + import PhilomenaWeb.Test.TagChanges setup ctx do # We need a verified user, because it bypasses the rate limits check @@ -44,25 +43,4 @@ defmodule PhilomenaWeb.Image.TagControllerTest do # Noop should not create a new tag change assert snap(update_image_tags(ctx, add: [])) == snap end - - defp update_image_tags(ctx, diff) do - conn = - WebTest.Images.update_image_tags(ctx.conn, ctx.image, diff) - - image = Test.Images.load_image!(ctx.image.id, preload: [:tags]) - - %{ - conn: conn, - image: image - } - end - - defp snap(ctx) do - tag_changes = - ctx.image.id - |> Test.TagChanges.load_tag_changes_by_image_id() - |> Enum.map(&Test.TagChanges.snap/1) - - [Test.Images.snap(ctx.image)] ++ tag_changes - end end diff --git a/test/support/philomena_web/images.ex b/test/support/philomena_web/tag_changes.ex similarity index 63% rename from test/support/philomena_web/images.ex rename to test/support/philomena_web/tag_changes.ex index 5602ab949..750df5a95 100644 --- a/test/support/philomena_web/images.ex +++ b/test/support/philomena_web/tag_changes.ex @@ -1,5 +1,6 @@ -defmodule PhilomenaWeb.Test.Images do +defmodule PhilomenaWeb.Test.TagChanges do alias Philomena.Images.Image + alias Philomena.Test import Phoenix.ConnTest @@ -9,17 +10,22 @@ defmodule PhilomenaWeb.Test.Images do use PhilomenaWeb, :verified_routes + @doc "Context of the test" + @type ctx :: %{ + conn: Plug.Conn.t(), + image: Image.t() + } + @type tags_diff :: [ add: [String.t()], remove: [String.t()] ] - @spec update_image_tags(Plug.Conn.t(), Image.t(), tags_diff()) :: - Plug.Conn.t() - def update_image_tags(conn, image, diff) do + @spec update_image_tags(ctx(), tags_diff()) :: ctx() + def update_image_tags(ctx, diff) do added_tags = Keyword.get(diff, :add, []) removed_tags = Keyword.get(diff, :remove, []) - current_tags = image.tags |> Enum.map(& &1.name) + current_tags = ctx.image.tags |> Enum.map(& &1.name) for tag <- added_tags do assert tag not in current_tags @@ -31,8 +37,8 @@ defmodule PhilomenaWeb.Test.Images do |> Enum.concat(added_tags) conn = - conn - |> post(~p"/images/#{image.id}/tags", %{ + ctx.conn + |> post(~p"/images/#{ctx.image.id}/tags", %{ "_method" => "put", "image" => %{ "old_tag_input" => current_tags |> Enum.join(", "), @@ -57,6 +63,19 @@ defmodule PhilomenaWeb.Test.Images do assert response =~ tag |> String.replace_leading("-", "") end - conn + %{ + conn: conn, + image: Test.Images.load_image!(ctx.image.id, preload: [:tags]) + } + end + + @spec snap(ctx()) :: any() + def snap(ctx) do + tag_changes = + ctx.image.id + |> Test.TagChanges.load_tag_changes_by_image_id() + |> Enum.map(&Test.TagChanges.snap/1) + + [Test.Images.snap(ctx.image) | tag_changes] end end From 4f052258c7c1c148194b1ab41f3b7bf1584865da Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 14:32:51 +0000 Subject: [PATCH 20/30] Wait for image upload in `create_image` test util --- .../controllers/image/tag_controller_test.exs | 22 ++++++--------- test/support/conn_case.ex | 4 +-- test/support/philomena/images.ex | 28 +++++++++++++++---- 3 files changed, 34 insertions(+), 20 deletions(-) diff --git a/test/philomena_web/controllers/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs index 9e05cf872..fb397860d 100644 --- a/test/philomena_web/controllers/image/tag_controller_test.exs +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -1,23 +1,19 @@ defmodule PhilomenaWeb.Image.TagControllerTest do - use PhilomenaWeb.ConnCase, async: true + use PhilomenaWeb.ConnCase alias Philomena.Test - alias Philomena.UsersFixtures import PhilomenaWeb.Test.TagChanges + # We need a verified user, because it bypasses the rate limits check setup ctx do - # We need a verified user, because it bypasses the rate limits check - user = UsersFixtures.user_fixture(%{confirmed: true, verified: true}) - %{conn: log_in_user(ctx.conn, user), user: user} + register_and_log_in_user(ctx, %{verified: true}) end - test "POST /image/{N}/tags", %{conn: conn, user: user} do - ctx = %{ - conn: conn, - image: - Test.Images.create_image(user, %{ - "tag_input" => "safe,a,b" - }) - } + test "POST /image/{N}/tags", ctx do + ctx = + ctx + |> Test.Images.create_image(%{ + "tag_input" => "safe,a,b" + }) ctx = update_image_tags(ctx, add: ["c", "d", "e"]) diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index 986cac7d3..d855bb0ad 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -59,8 +59,8 @@ defmodule PhilomenaWeb.ConnCase do It stores an updated connection and a registered user in the test context. """ - def register_and_log_in_user(%{conn: conn}) do - user = Philomena.UsersFixtures.confirmed_user_fixture() + def register_and_log_in_user(%{conn: conn}, attrs \\ %{}) do + user = Philomena.UsersFixtures.confirmed_user_fixture(attrs) %{conn: log_in_user(conn, user), user: user} end diff --git a/test/support/philomena/images.ex b/test/support/philomena/images.ex index b2f55ccad..25589b1e6 100644 --- a/test/support/philomena/images.ex +++ b/test/support/philomena/images.ex @@ -3,6 +3,7 @@ defmodule Philomena.Test.Images do alias Philomena.Images.Image alias Philomena.Images alias Philomena.Test + alias Philomena.Users.User import Ecto.Query @@ -18,8 +19,18 @@ defmodule Philomena.Test.Images do |> Repo.one() end - @spec create_image(User.t(), map()) :: Images.Image.t() - def create_image(uploader, attrs \\ %{}) do + @type image_ctx :: %{ + image: Image.t() + } + + @spec create_image(%{user: User.t()}, map()) :: image_ctx() + def create_image(ctx, attrs \\ %{}) do + if Map.get(ctx, :async) do + raise "Image creation in `async` mode is not supported because it spawns " <> + "a background process that needs to access the DB connection checked out " <> + "by the calling process." + end + attrs = %{ "image" => %Plug.Upload{ @@ -33,12 +44,19 @@ defmodule Philomena.Test.Images do |> Map.merge(attrs) principal = [ - user: uploader + user: ctx.user ] - {:ok, %{image: image}} = Images.create_image(principal, attrs) + {:ok, %{image: image, upload_pid: upload_pid}} = Images.create_image(principal, attrs) + + # Wait for the upload process to finish. It should be fast enough in case if + # the default `dummy.png` file is used, which is extremely small. + upload_ref = Process.monitor(upload_pid) - image + receive do + {:DOWN, ^upload_ref, :process, ^upload_pid, _reason} -> + Map.put(ctx, :image, image) + end end def snap(%Image{} = image) do From ff8e31ee9991ddb3884a831687bbd11abcec0e73 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 17:23:10 +0000 Subject: [PATCH 21/30] Expose the DB port in docker-compsoe --- docker-compose.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docker-compose.yml b/docker-compose.yml index cc0760670..09da460df 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -74,6 +74,12 @@ services: - POSTGRES_PASSWORD=postgres volumes: - postgres_data:/var/lib/postgresql/data + + # Expose DB port for connecting from a DB viewer client. `POSTGRES_PORT` + # variable can be used to prevent conflicts with other postgres instances + # potentially already running on the host. + ports: ['${POSTGRES_PORT:-5432}:5432'] + attach: false opensearch: From 0cef4a9d19b7f6152fb628428792392cee70a2f4 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 17:23:30 +0000 Subject: [PATCH 22/30] Add some tests for the tag reverts --- .../tag_change/revert_controller_test.exs | 116 ++++++++++++++++++ test/support/conn_case.ex | 21 +++- test/support/philomena_web/tag_changes.ex | 17 +-- 3 files changed, 142 insertions(+), 12 deletions(-) diff --git a/test/philomena_web/controllers/tag_change/revert_controller_test.exs b/test/philomena_web/controllers/tag_change/revert_controller_test.exs index e69de29bb..9bb7a17c5 100644 --- a/test/philomena_web/controllers/tag_change/revert_controller_test.exs +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -0,0 +1,116 @@ +defmodule PhilomenaWeb.TagChange.RevertControllerTest do + use PhilomenaWeb.ConnCase + alias Philomena.Repo + alias Philomena.Users + alias Philomena.Test + import PhilomenaWeb.Test.TagChanges + + # We need a verified user, because it bypasses the rate limits check + setup ctx do + register_and_log_in_user(ctx, %{verified: true}) + end + + test "unauthorized", ctx do + {ctx, tag_change} = create_image_with_tag_change(ctx) + + ctx = try_revert_tag_changes(ctx, [tag_change.id]) + + # The default test user is a regular user, so it's not authorized to + # revert tag changes + assert_value(ctx.conn.assigns.flash == %{"error" => "You can't access that page."}) + + html_response(ctx.conn, 302) + end + + describe "authorized" do + setup ctx do + {:ok, moderator} = + ctx.user + |> Repo.preload([:roles]) + |> Users.update_user(%{"role" => "moderator"}) + + put_in(ctx.user, moderator) + end + + test "noop reverts", ctx do + {ctx, tag_change} = create_image_with_tag_change(ctx) + + ctx = revert_tag_changes(ctx, [tag_change.id]) + + assert_value( + ctx.conn.assigns.flash == %{ + "info" => "Successfully reverted 1 tag changes with 1 tags actually updated." + } + ) + + snap = snap(ctx) + + assert_value( + snap == [ + "Image(1): [a 1] [b 1] [c 1] [safe 1]", + "TagChange(2): -[d 0]", + "TagChange(1): +[d 0]" + ] + ) + + # Reverting again should be a no-op + ctx = revert_tag_changes(ctx, [tag_change.id]) + + assert_value( + ctx.conn.assigns.flash == %{ + "info" => "Successfully reverted 1 tag changes with 0 tags actually updated." + } + ) + + tag_changes = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) + + assert snap == snap(ctx, tag_changes) + + ctx = revert_tag_changes(ctx, tag_changes |> Enum.map(& &1.id)) + + # The last tag change removed the tag, so now it should be re-added again + assert_value( + ctx.conn.assigns.flash == %{ + "info" => "Successfully reverted 2 tag changes with 1 tags actually updated." + } + ) + + assert_value( + snap(ctx) == + [ + "Image(1): [a 1] [b 1] [c 1] [safe 1] [d 1]", + "TagChange(3): +[d 1]", + "TagChange(2): -[d 1]", + "TagChange(1): +[d 1]" + ] + ) + end + end + + defp create_image_with_tag_change(ctx) do + ctx = + ctx + |> Test.Images.create_image(%{ + "tag_input" => "safe,a,b,c" + }) + |> update_image_tags(add: ["d"]) + + [tag_change] = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) + + {ctx, tag_change} + end + + defp revert_tag_changes(ctx, tag_change_ids) do + ctx = try_revert_tag_changes(ctx, tag_change_ids) + + html_response(ctx.conn, 302) + + put_in(ctx.image, Test.Images.load_image!(ctx.image.id, preload: [:tags])) + end + + defp try_revert_tag_changes(ctx, tag_change_ids) do + conn = post(ctx.conn, ~p"/tag_changes/revert", %{"ids" => tag_change_ids}) + + put_in(ctx.conn, conn) + end +end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index d855bb0ad..804f81295 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -31,11 +31,22 @@ defmodule PhilomenaWeb.ConnCase do end setup tags do - :ok = Ecto.Adapters.SQL.Sandbox.checkout(Philomena.Repo) - - unless tags[:async] do - Ecto.Adapters.SQL.Sandbox.mode(Philomena.Repo, {:shared, self()}) - end + pid = Ecto.Adapters.SQL.Sandbox.start_owner!(Philomena.Repo, shared: not tags[:async]) + on_exit(fn -> Ecto.Adapters.SQL.Sandbox.stop_owner(pid) end) + + # Postgres doesn't revert the auto-increment counters if a transaction + # that modified it was rolled back. This is understandable for performance + # reasons, but unfortunately, it introduces side effects visible between + # tests that test against the generated IDs. + # + # This is the downside of Ecto using transactions mechanism to isolate + # tests, while some other DB libraries create separate databases per each + # test to ensure full isolation and allow for testing of concurrent + # transactions. Ideally `Ecto` should do that or at least provide an + # option to do that. While that's not the case we have to do some manual + # cleanup between tests to reset the auto-increment counters. + Philomena.Repo.query!("ALTER SEQUENCE images_id_seq RESTART WITH 1") + Philomena.Repo.query!("ALTER SEQUENCE tag_changes_id_seq1 RESTART WITH 1") # Insert default filter %Philomena.Filters.Filter{name: "Default", system: true} diff --git a/test/support/philomena_web/tag_changes.ex b/test/support/philomena_web/tag_changes.ex index 750df5a95..d9aa725ed 100644 --- a/test/support/philomena_web/tag_changes.ex +++ b/test/support/philomena_web/tag_changes.ex @@ -63,17 +63,20 @@ defmodule PhilomenaWeb.Test.TagChanges do assert response =~ tag |> String.replace_leading("-", "") end - %{ - conn: conn, - image: Test.Images.load_image!(ctx.image.id, preload: [:tags]) - } + ctx = put_in(ctx.conn, conn) + put_in(ctx.image, Test.Images.load_image!(ctx.image.id, preload: [:tags])) end - @spec snap(ctx()) :: any() + @spec snap(ctx()) :: map() def snap(ctx) do + snap(ctx, ctx.image.id |> Test.TagChanges.load_tag_changes_by_image_id()) + end + + @doc "Create a snapshot with the preloaded tag changes" + @spec snap(ctx(), [TagChange.t()]) :: term() + def snap(ctx, tag_changes) do tag_changes = - ctx.image.id - |> Test.TagChanges.load_tag_changes_by_image_id() + tag_changes |> Enum.map(&Test.TagChanges.snap/1) [Test.Images.snap(ctx.image) | tag_changes] From c608d0d88c7f474bf6eddd70c683051b63d04c82 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 17:46:49 +0000 Subject: [PATCH 23/30] Add a test for reverting the change that was later overwritten. Currently probably not the intended behavior? --- .../tag_change/revert_controller_test.exs | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/test/philomena_web/controllers/tag_change/revert_controller_test.exs b/test/philomena_web/controllers/tag_change/revert_controller_test.exs index 9bb7a17c5..a7de00027 100644 --- a/test/philomena_web/controllers/tag_change/revert_controller_test.exs +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -11,9 +11,9 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do end test "unauthorized", ctx do - {ctx, tag_change} = create_image_with_tag_change(ctx) + {ctx, first_tag_change} = create_image_with_tag_change(ctx) - ctx = try_revert_tag_changes(ctx, [tag_change.id]) + ctx = try_revert_tag_changes(ctx, [first_tag_change.id]) # The default test user is a regular user, so it's not authorized to # revert tag changes @@ -33,9 +33,9 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do end test "noop reverts", ctx do - {ctx, tag_change} = create_image_with_tag_change(ctx) + {ctx, first_tag_change} = create_image_with_tag_change(ctx) - ctx = revert_tag_changes(ctx, [tag_change.id]) + ctx = revert_tag_changes(ctx, [first_tag_change.id]) assert_value( ctx.conn.assigns.flash == %{ @@ -54,7 +54,7 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do ) # Reverting again should be a no-op - ctx = revert_tag_changes(ctx, [tag_change.id]) + ctx = revert_tag_changes(ctx, [first_tag_change.id]) assert_value( ctx.conn.assigns.flash == %{ @@ -75,8 +75,10 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do } ) + tag_changes = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) + assert_value( - snap(ctx) == + snap(ctx, tag_changes) == [ "Image(1): [a 1] [b 1] [c 1] [safe 1] [d 1]", "TagChange(3): +[d 1]", @@ -84,6 +86,26 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do "TagChange(1): +[d 1]" ] ) + + # Try to revert the change that was the first. It should be no-op + # since the tag is already present on the image + ctx = revert_tag_changes(ctx, [first_tag_change.id]) + + assert_value( + ctx.conn.assigns.flash == %{ + "info" => "Successfully reverted 1 tag changes with 1 tags actually updated." + } + ) + + assert_value( + snap(ctx) == [ + "Image(1): [a 1] [b 1] [c 1] [safe 1]", + "TagChange(4): -[d 0]", + "TagChange(3): +[d 0]", + "TagChange(2): -[d 0]", + "TagChange(1): +[d 0]" + ] + ) end end From 4d9da00911395aae22ae1183c01ce319acafd839 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 18:51:33 +0000 Subject: [PATCH 24/30] Add `files` dependency dependency to `app` since test now upload images --- docker-compose.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/docker-compose.yml b/docker-compose.yml index 09da460df..5d810ca9a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -65,6 +65,7 @@ services: - postgres - opensearch - valkey + - files ports: - '5173:5173' From 2a1ed9e00da94f63f8c93ce5790387a759efd464 Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 21:03:39 +0000 Subject: [PATCH 25/30] Don't revert tag changes that were later overwritten --- lib/philomena/tag_changes.ex | 112 ++++++++++-------- .../tag_change/revert_controller.ex | 6 +- .../tag_change/revert_controller_test.exs | 11 +- 3 files changed, 71 insertions(+), 58 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index fb4b7d46c..3c171ea75 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -22,69 +22,83 @@ defmodule Philomena.TagChanges do {:ok, [TagChange.t()], non_neg_integer()} | {:error, any()} + @type tag_change_id :: integer() + @type tag_id :: integer() + @typedoc """ A tuple with a composite identifier for a `TagChange.Tag`. """ - @type tag_change_tag_id :: {tag_change_id :: integer(), tag_id :: integer()} + @type tag_change_tag_id :: {tag_change_id(), tag_id()} # Accepts a list of `TagChanges.TagChange` IDs. - @spec mass_revert([integer()], Users.principal()) :: mass_revert_result() - def mass_revert(ids, principal) do - Repo.all( - from tc in TagChange, - inner_join: i in assoc(tc, :image), - where: tc.id in ^ids and i.hidden_from_users == false, - order_by: [desc: :created_at, desc: tc.id], - preload: [:tags] - ) + @spec mass_revert([tag_change_id()], Users.principal()) :: mass_revert_result() + def mass_revert(tag_change_ids, principal) do + tag_change_ids + |> Map.new(&{&1, nil}) |> mass_revert_impl(principal) end # Accepts a list of `TagChanges.Tag` IDs. @spec mass_revert_tags([tag_change_tag_id()], Users.principal()) :: mass_revert_result() def mass_revert_tags(tag_change_tag_ids, principal) do - {tag_change_ids, tag_ids} = Enum.unzip(tag_change_tag_ids) - - Repo.all( - from tag_change in TagChange, - inner_join: - input in fragment( - """ - SELECT - unnest(?::integer[]) AS tag_change_id, - unnest(?::integer[]) AS tag_id - """, - ^tag_change_ids, - ^tag_ids - ), - on: tag_change.id == input.tag_change_id, - inner_join: tag_change_tag in TagChanges.Tag, - as: :tag_change_tag, - on: - tag_change_tag.tag_change_id == tag_change.id and - tag_change_tag.tag_id == input.tag_id, - group_by: tag_change.id, - order_by: [desc: tag_change.created_at, desc: tag_change.id], - select: %TagChange{ - id: tag_change.id, - image_id: tag_change.image_id, - tags: fragment("array_agg(row(?, ?))", tag_change_tag.tag_id, tag_change_tag.added) - } - ) - |> Enum.map(fn tag_change -> - tags = - tag_change.tags - |> Enum.map(fn {tag_id, added} -> %TagChanges.Tag{tag_id: tag_id, added: added} end) - - put_in(tag_change.tags, tags) - end) + tag_change_tag_ids + |> Enum.group_by(&elem(&1, 0), &elem(&1, 1)) |> mass_revert_impl(principal) end - # Expects that the `tag_changes` are already sorted by created_at in - # descending order. - @spec mass_revert_impl([TagChange.t()], Users.principal()) :: mass_revert_result() - defp mass_revert_impl(tag_changes, principal) do + @spec mass_revert_impl(%{tag_change_id() => [tag_id()] | nil}, Users.principal()) :: + mass_revert_result() + defp mass_revert_impl(input, principal) do + input = + input + |> Enum.map(fn {tag_change_id, tag_ids} -> %{tc_id: tag_change_id, tag_ids: tag_ids} end) + + tag_changes = + Repo.all( + from tc in TagChange, + as: :tc, + inner_join: + input in fragment( + """ + SELECT * FROM json_to_recordset(?) as (tc_id int, tag_ids int[]) + """, + ^input + ), + on: tc.id == input.tc_id, + inner_join: tct in TagChanges.Tag, + as: :tct, + on: + tct.tag_change_id == tc.id and (is_nil(input.tag_ids) or tct.tag_id in input.tag_ids), + where: + not exists( + from newer_tct in TagChanges.Tag, + where: parent_as(:tct).tag_id == newer_tct.tag_id, + join: newer_tc in TagChange, + on: + newer_tc.id == + newer_tct.tag_change_id and newer_tc.image_id == parent_as(:tc).image_id, + where: + newer_tc.created_at > parent_as(:tc).created_at or + newer_tc.id > parent_as(:tc).id + ), + group_by: tc.id, + order_by: [desc: tc.created_at, desc: tc.id], + select: %TagChange{ + id: tc.id, + image_id: tc.image_id, + tags: fragment("array_agg(row(?, ?))", tct.tag_id, tct.added) + } + ) + |> Enum.map(fn tag_change -> + tags = + tag_change.tags + |> Enum.map(fn {tag_id, added} -> + %TagChanges.Tag{tag_id: tag_id, added: added} + end) + + put_in(tag_change.tags, tags) + end) + # Calculate the revert operations for each image. reverts_per_image = tag_changes diff --git a/lib/philomena_web/controllers/tag_change/revert_controller.ex b/lib/philomena_web/controllers/tag_change/revert_controller.ex index e91a08737..8428748d8 100644 --- a/lib/philomena_web/controllers/tag_change/revert_controller.ex +++ b/lib/philomena_web/controllers/tag_change/revert_controller.ex @@ -17,16 +17,16 @@ defmodule PhilomenaWeb.TagChange.RevertController do } case TagChanges.mass_revert(ids, attributes) do - {:ok, tag_changes, total_tags_affected} -> + {:ok, _affected_tag_changes, total_tags_affected} -> conn |> put_flash( :info, - "Successfully reverted #{length(tag_changes)} tag changes with " <> + "Successfully reverted #{length(ids)} tag changes with " <> "#{total_tags_affected} tags actually updated." ) |> moderation_log( details: &log_details/2, - data: %{user: conn.assigns.current_user, count: length(tag_changes)} + data: %{user: conn.assigns.current_user, count: length(ids)} ) |> redirect(external: conn.assigns.referrer) diff --git a/test/philomena_web/controllers/tag_change/revert_controller_test.exs b/test/philomena_web/controllers/tag_change/revert_controller_test.exs index a7de00027..292bb85bf 100644 --- a/test/philomena_web/controllers/tag_change/revert_controller_test.exs +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -93,17 +93,16 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do assert_value( ctx.conn.assigns.flash == %{ - "info" => "Successfully reverted 1 tag changes with 1 tags actually updated." + "info" => "Successfully reverted 1 tag changes with 0 tags actually updated." } ) assert_value( snap(ctx) == [ - "Image(1): [a 1] [b 1] [c 1] [safe 1]", - "TagChange(4): -[d 0]", - "TagChange(3): +[d 0]", - "TagChange(2): -[d 0]", - "TagChange(1): +[d 0]" + "Image(1): [a 1] [b 1] [c 1] [safe 1] [d 1]", + "TagChange(3): +[d 1]", + "TagChange(2): -[d 1]", + "TagChange(1): +[d 1]" ] ) end From 16234db7678bc8598c7e610b7998b8ebb56572ea Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 21:14:30 +0000 Subject: [PATCH 26/30] Add more comments --- lib/philomena/tag_changes.ex | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 3c171ea75..911862159 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -57,6 +57,10 @@ defmodule Philomena.TagChanges do Repo.all( from tc in TagChange, as: :tc, + # Filter the tag changes table by the input tag change ids. Note that + # we use `json_to_recordset` to convert the input into a table that + # contains an array column. This is the simplest way to do this in + # postgres. inner_join: input in fragment( """ @@ -65,10 +69,18 @@ defmodule Philomena.TagChanges do ^input ), on: tc.id == input.tc_id, + + # Join and filter only tags that we are interested in reverting unless + # the `tag_ids` is nil, which means all tags in the change are a + # subject of the revert. inner_join: tct in TagChanges.Tag, as: :tct, on: tct.tag_change_id == tc.id and (is_nil(input.tag_ids) or tct.tag_id in input.tag_ids), + + # Make sure the tag changes that we want to revert are the most recent + # ones. The revert only makes sense for tag changes that influenced the + # current state of the image. where: not exists( from newer_tct in TagChanges.Tag, @@ -81,8 +93,9 @@ defmodule Philomena.TagChanges do newer_tc.created_at > parent_as(:tc).created_at or newer_tc.id > parent_as(:tc).id ), + + # Group all tag changes by ID accumulating all tags into an array. group_by: tc.id, - order_by: [desc: tc.created_at, desc: tc.id], select: %TagChange{ id: tc.id, image_id: tc.image_id, From f24fe69b66e151aa601faed571d8917ad7fe69ea Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 21:17:10 +0000 Subject: [PATCH 27/30] Better comments --- lib/philomena/tag_changes.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 911862159..29f81bc06 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -70,7 +70,7 @@ defmodule Philomena.TagChanges do ), on: tc.id == input.tc_id, - # Join and filter only tags that we are interested in reverting unless + # Join and filter only specific tags that we want to revert, unless # the `tag_ids` is nil, which means all tags in the change are a # subject of the revert. inner_join: tct in TagChanges.Tag, From 88c1abeffc6c1c4c97a0934ff0a64ebe6d60747a Mon Sep 17 00:00:00 2001 From: MareStare Date: Sun, 18 May 2025 21:19:28 +0000 Subject: [PATCH 28/30] Remove the now-unnecessary uniq_by --- lib/philomena/tag_changes.ex | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 29f81bc06..0bd6d2466 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -117,13 +117,9 @@ defmodule Philomena.TagChanges do tag_changes |> Enum.group_by(& &1.image_id) |> Enum.map(fn {image_id, tag_changes} -> - # The tag changes are already sorted by created_at in descending order - # so if we run a `uniq_by` for their tags, we'll leave only the most - # recent change per each tag. {added_tags, removed_tags} = tag_changes |> Enum.flat_map(& &1.tags) - |> Enum.uniq_by(& &1.tag_id) |> Enum.split_with(& &1.added) # We send removed tags to be added, and added to be removed. That's how reverting works! From f97c9e56060ee90852b4195f66fc6d8d629acde8 Mon Sep 17 00:00:00 2001 From: MareStare Date: Tue, 20 May 2025 03:35:48 +0000 Subject: [PATCH 29/30] Yay, the tag reverts are now taking into account self-cancelling reverts, tests pass --- lib/philomena/tag_changes.ex | 120 ++++++++++-------- .../tag_change/revert_controller.ex | 10 +- .../tag_change/revert_controller_test.exs | 66 +++++++--- test/support/philomena_web/tag_changes.ex | 4 + 4 files changed, 127 insertions(+), 73 deletions(-) diff --git a/lib/philomena/tag_changes.ex b/lib/philomena/tag_changes.ex index 0bd6d2466..27041c234 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -14,6 +14,8 @@ defmodule Philomena.TagChanges do alias Philomena.Tags.Tag alias Philomena.Users + require Logger + @typedoc """ In the successful case returns the `TagChange`s that were loaded and affected plus a non-negative integer with the number of tags affected by the revert. @@ -49,78 +51,87 @@ defmodule Philomena.TagChanges do @spec mass_revert_impl(%{tag_change_id() => [tag_id()] | nil}, Users.principal()) :: mass_revert_result() defp mass_revert_impl(input, principal) do + # Load the requested tag changes from the DB. `nil` in the position of the + # tag IDs list means we want to revert all tags for that tag change. input = input - |> Enum.map(fn {tag_change_id, tag_ids} -> %{tc_id: tag_change_id, tag_ids: tag_ids} end) + |> Enum.map(fn {tc_id, tag_ids} -> + %{ + tc_id: tc_id, + tag_ids: if(not is_nil(tag_ids), do: tag_ids |> Enum.uniq()) + } + end) - tag_changes = + tags_by_image = Repo.all( from tc in TagChange, - as: :tc, - # Filter the tag changes table by the input tag change ids. Note that - # we use `json_to_recordset` to convert the input into a table that - # contains an array column. This is the simplest way to do this in - # postgres. inner_join: input in fragment( - """ - SELECT * FROM json_to_recordset(?) as (tc_id int, tag_ids int[]) - """, + "SELECT * FROM jsonb_to_recordset(?) AS (tc_id int, tag_ids int[])", ^input ), on: tc.id == input.tc_id, - - # Join and filter only specific tags that we want to revert, unless - # the `tag_ids` is nil, which means all tags in the change are a - # subject of the revert. inner_join: tct in TagChanges.Tag, - as: :tct, - on: - tct.tag_change_id == tc.id and (is_nil(input.tag_ids) or tct.tag_id in input.tag_ids), - - # Make sure the tag changes that we want to revert are the most recent - # ones. The revert only makes sense for tag changes that influenced the - # current state of the image. - where: - not exists( - from newer_tct in TagChanges.Tag, - where: parent_as(:tct).tag_id == newer_tct.tag_id, - join: newer_tc in TagChange, - on: - newer_tc.id == - newer_tct.tag_change_id and newer_tc.image_id == parent_as(:tc).image_id, - where: - newer_tc.created_at > parent_as(:tc).created_at or - newer_tc.id > parent_as(:tc).id - ), - - # Group all tag changes by ID accumulating all tags into an array. - group_by: tc.id, - select: %TagChange{ - id: tc.id, + on: tc.id == tct.tag_change_id, + where: is_nil(input.tag_ids) or tct.tag_id in input.tag_ids, + group_by: [tc.image_id, tct.tag_id], + # Get min/max rows without subqueries (don't repeat this at home). + # Retain only changes that dont cancel themselves out already i.e. we + # want to ignore change sequences where the first change adds the tag + # and the last change removes it or vice versa, because they are + # already self-cancelling. + having: + fragment("(array_agg(? ORDER BY ? ASC))[1]", tct.added, tc.id) == + fragment("(array_agg(? ORDER BY ? DESC))[1]", tct.added, tc.id), + select: %{ image_id: tc.image_id, - tags: fragment("array_agg(row(?, ?))", tct.tag_id, tct.added) + tag_id: tct.tag_id, + last_change: + fragment("(array_agg(row (?, ?) ORDER BY ? DESC))[1]", tc.id, tct.added, tc.id) } ) - |> Enum.map(fn tag_change -> - tags = - tag_change.tags - |> Enum.map(fn {tag_id, added} -> - %TagChanges.Tag{tag_id: tag_id, added: added} - end) - - put_in(tag_change.tags, tags) + |> Enum.group_by(& &1.image_id) + + # Load latest tag change IDs for each {image, tags} we are interested in. + # We'll use this to filter out tag changes that are non-current, meaning + # reverting them would not be useful as they were already overwritten by + # some other change not in the list of reverted changes. + input = + tags_by_image + |> Enum.map(fn {image_id, tags} -> + %{ + image_id: image_id, + tag_ids: tags |> Enum.map(& &1.tag_id) + } end) + latest_tc_ids = + Repo.all( + from tc in TagChange, + inner_join: + input in fragment( + "SELECT * FROM jsonb_to_recordset(?) AS (image_id int, tag_ids int[])", + ^input + ), + on: tc.image_id == input.image_id, + inner_join: tct in TagChanges.Tag, + on: tc.id == tct.tag_change_id and tct.tag_id in input.tag_ids, + group_by: [tc.image_id, tct.tag_id], + select: max(tc.id), + # It's possible for the same tag change to cover the latest versions + # of several tags, so deduplicate the results. + distinct: true + ) + |> MapSet.new() + # Calculate the revert operations for each image. reverts_per_image = - tag_changes - |> Enum.group_by(& &1.image_id) - |> Enum.map(fn {image_id, tag_changes} -> + tags_by_image + |> Enum.map(fn {image_id, tags} -> {added_tags, removed_tags} = - tag_changes - |> Enum.flat_map(& &1.tags) - |> Enum.split_with(& &1.added) + tags + |> Enum.filter(fn %{last_change: {tc_id, _}} -> tc_id in latest_tc_ids end) + |> Enum.split_with(fn %{last_change: {_, added}} -> added end) # We send removed tags to be added, and added to be removed. That's how reverting works! %{ @@ -129,9 +140,10 @@ defmodule Philomena.TagChanges do removed_tag_ids: Enum.map(added_tags, & &1.tag_id) } end) + |> Enum.reject(&(&1.added_tag_ids == [] and &1.removed_tag_ids == [])) with {:ok, {total_tags_affected, _}} <- Images.batch_update(reverts_per_image, principal) do - {:ok, tag_changes, total_tags_affected} + {:ok, total_tags_affected} end end diff --git a/lib/philomena_web/controllers/tag_change/revert_controller.ex b/lib/philomena_web/controllers/tag_change/revert_controller.ex index 8428748d8..e3edba676 100644 --- a/lib/philomena_web/controllers/tag_change/revert_controller.ex +++ b/lib/philomena_web/controllers/tag_change/revert_controller.ex @@ -17,12 +17,16 @@ defmodule PhilomenaWeb.TagChange.RevertController do } case TagChanges.mass_revert(ids, attributes) do - {:ok, _affected_tag_changes, total_tags_affected} -> + {:ok, total_tags_affected} -> conn |> put_flash( :info, - "Successfully reverted #{length(ids)} tag changes with " <> - "#{total_tags_affected} tags actually updated." + if total_tags_affected == 0 do + "The revert of #{length(ids)} changes resulted in no effective tag updates." + else + "Successfully reverted #{length(ids)} tag changes with " <> + "#{total_tags_affected} tags effectively updated." + end ) |> moderation_log( details: &log_details/2, diff --git a/test/philomena_web/controllers/tag_change/revert_controller_test.exs b/test/philomena_web/controllers/tag_change/revert_controller_test.exs index 292bb85bf..5625d93f0 100644 --- a/test/philomena_web/controllers/tag_change/revert_controller_test.exs +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -64,27 +64,23 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do tag_changes = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) - assert snap == snap(ctx, tag_changes) + assert snap(ctx, tag_changes) == snap ctx = revert_tag_changes(ctx, tag_changes |> Enum.map(& &1.id)) # The last tag change removed the tag, so now it should be re-added again assert_value( ctx.conn.assigns.flash == %{ - "info" => "Successfully reverted 2 tag changes with 1 tags actually updated." + "info" => "Successfully reverted 2 tag changes with 0 tags actually updated." } ) tag_changes = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) + snap = snap(ctx, tag_changes) assert_value( - snap(ctx, tag_changes) == - [ - "Image(1): [a 1] [b 1] [c 1] [safe 1] [d 1]", - "TagChange(3): +[d 1]", - "TagChange(2): -[d 1]", - "TagChange(1): +[d 1]" - ] + snap == + ["Image(1): [a 1] [b 1] [c 1] [safe 1]", "TagChange(2): -[d 0]", "TagChange(1): +[d 0]"] ) # Try to revert the change that was the first. It should be no-op @@ -97,12 +93,52 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do } ) + assert snap(ctx) == snap + + tag_changes = + Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) + |> Enum.drop(1) + + ctx = revert_tag_changes(ctx, tag_changes |> Enum.map(& &1.id)) + + assert_value( + ctx.conn.assigns.flash == %{ + "info" => "Successfully reverted 1 tag changes with 0 tags actually updated." + } + ) + + assert snap(ctx) == snap + end + + test "mixed tag changes reverts", ctx do + ctx = + ctx + |> Test.Images.create_image(%{"tag_input" => "safe,a,b,c"}) + |> update_image_tags(add: ["d", "e"], remove: ["a", "b"]) + |> update_image_tags(add: ["f", "g"], remove: ["c", "d"]) + |> update_image_tags(add: ["h", "i"], remove: ["e", "f"]) + |> update_image_tags(add: ["j"], remove: ["g", "h"]) + |> update_image_tags(add: ["g", "c"]) + + tag_changes = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) + + ctx = revert_tag_changes(ctx, tag_changes |> Enum.map(& &1.id)) + + assert_value( + ctx.conn.assigns.flash == %{ + "info" => "Successfully reverted 5 tag changes with 5 tags actually updated." + } + ) + assert_value( snap(ctx) == [ - "Image(1): [a 1] [b 1] [c 1] [safe 1] [d 1]", - "TagChange(3): +[d 1]", - "TagChange(2): -[d 1]", - "TagChange(1): +[d 1]" + "Image(1): [safe 1] [c 1] [a 1] [b 1]", + "TagChange(6): +[a 1] +[b 1] -[g 0] -[i 0] -[j 0]", + "TagChange(5): +[c 1] +[g 0]", + "TagChange(4): +[j 0] -[g 0] -[h 0]", + "TagChange(3): +[h 0] +[i 0] -[e 0] -[f 0]", + "TagChange(2): +[f 0] +[g 0] -[c 1] -[d 0]", + "TagChange(1): +[d 0] +[e 0] -[a 1] -[b 1]" ] ) end @@ -111,9 +147,7 @@ defmodule PhilomenaWeb.TagChange.RevertControllerTest do defp create_image_with_tag_change(ctx) do ctx = ctx - |> Test.Images.create_image(%{ - "tag_input" => "safe,a,b,c" - }) + |> Test.Images.create_image(%{"tag_input" => "safe,a,b,c"}) |> update_image_tags(add: ["d"]) [tag_change] = Test.TagChanges.load_tag_changes_by_image_id(ctx.image.id) diff --git a/test/support/philomena_web/tag_changes.ex b/test/support/philomena_web/tag_changes.ex index d9aa725ed..2aec0b32e 100644 --- a/test/support/philomena_web/tag_changes.ex +++ b/test/support/philomena_web/tag_changes.ex @@ -31,6 +31,10 @@ defmodule PhilomenaWeb.Test.TagChanges do assert tag not in current_tags end + for tag <- removed_tags do + assert tag in current_tags + end + new_tags = current_tags |> Enum.reject(&(&1 in removed_tags)) From bc24332a9e3a9d78abba02454a44156eb6d2f859 Mon Sep 17 00:00:00 2001 From: MareStare Date: Tue, 20 May 2025 23:01:30 +0000 Subject: [PATCH 30/30] Use `Enum.each()` --- test/support/philomena_web/tag_changes.ex | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/support/philomena_web/tag_changes.ex b/test/support/philomena_web/tag_changes.ex index 2aec0b32e..c62d6df64 100644 --- a/test/support/philomena_web/tag_changes.ex +++ b/test/support/philomena_web/tag_changes.ex @@ -27,13 +27,13 @@ defmodule PhilomenaWeb.Test.TagChanges do removed_tags = Keyword.get(diff, :remove, []) current_tags = ctx.image.tags |> Enum.map(& &1.name) - for tag <- added_tags do + Enum.each(added_tags, fn tag -> assert tag not in current_tags - end + end) - for tag <- removed_tags do + Enum.each(removed_tags, fn tag -> assert tag in current_tags - end + end) new_tags = current_tags