diff --git a/docker-compose.yml b/docker-compose.yml index 0281aa4a4..5d810ca9a 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 @@ -63,6 +65,7 @@ services: - postgres - opensearch - valkey + - files ports: - '5173:5173' @@ -72,6 +75,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: diff --git a/lib/philomena/images.ex b/lib/philomena/images.ex index b56e21cd1..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, @@ -69,6 +70,16 @@ 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 + } + @doc """ Creates a image. @@ -81,6 +92,8 @@ defmodule Philomena.Images do {:error, %Ecto.Changeset{}} """ + @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"] @@ -1074,13 +1087,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 +1154,10 @@ 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() result @@ -1153,7 +1167,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 +1180,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 90b354b55..27041c234 100644 --- a/lib/philomena/tag_changes.ex +++ b/lib/philomena/tag_changes.ex @@ -12,52 +12,139 @@ 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 - tag_changes = + 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. + """ + @type mass_revert_result :: + {: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(), tag_id()} + + # Accepts a list of `TagChanges.TagChange` IDs. + @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_tag_ids + |> Enum.group_by(&elem(&1, 0), &elem(&1, 1)) + |> mass_revert_impl(principal) + end + + @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 {tc_id, tag_ids} -> + %{ + tc_id: tc_id, + tag_ids: if(not is_nil(tag_ids), do: tag_ids |> Enum.uniq()) + } + end) + + tags_by_image = 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: [:tag, :tag_change]] + inner_join: + input in fragment( + "SELECT * FROM jsonb_to_recordset(?) AS (tc_id int, tag_ids int[])", + ^input + ), + on: tc.id == input.tc_id, + inner_join: tct in TagChanges.Tag, + 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, + tag_id: tct.tag_id, + last_change: + fragment("(array_agg(row (?, ?) ORDER BY ? DESC))[1]", tc.id, tct.added, tc.id) + } ) + |> 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) - case mass_revert_tags(Enum.flat_map(tag_changes, & &1.tags), attributes) do - {:ok, _result} -> - {:ok, tag_changes} - - error -> - error - end - 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() - # 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} -> - changed_tags = - instances - |> Enum.sort_by(& &1.tag_change.created_at, :desc) - |> Enum.uniq_by(& &1.tag_id) - - {added_tags, removed_tags} = Enum.split_with(changed_tags, & &1.added) + # Calculate the revert operations for each image. + reverts_per_image = + tags_by_image + |> Enum.map(fn {image_id, tags} -> + {added_tags, removed_tags} = + 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! %{ 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) + |> Enum.reject(&(&1.added_tag_ids == [] and &1.removed_tag_ids == [])) - Images.batch_update(changes_per_image, attributes) + with {:ok, {total_tags_affected, _}} <- Images.batch_update(reverts_per_image, principal) do + {:ok, total_tags_affected} + end end def full_revert(%{user_id: _user_id, attributes: _attributes} = params), @@ -149,7 +236,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/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. 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 """ diff --git a/lib/philomena_web/controllers/tag_change/revert_controller.ex b/lib/philomena_web/controllers/tag_change/revert_controller.ex index 2fd94f88f..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,20 @@ defmodule PhilomenaWeb.TagChange.RevertController do } case TagChanges.mass_revert(ids, attributes) do - {:ok, tag_changes} -> + {:ok, total_tags_affected} -> conn - |> put_flash(:info, "Successfully reverted #{length(tag_changes)} tag changes.") + |> put_flash( + :info, + 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, - data: %{user: conn.assigns.current_user, count: length(tag_changes)} + data: %{user: conn.assigns.current_user, count: length(ids)} ) |> redirect(external: conn.assigns.referrer) @@ -33,6 +41,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 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 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/image/tag_controller_test.exs b/test/philomena_web/controllers/image/tag_controller_test.exs new file mode 100644 index 000000000..fb397860d --- /dev/null +++ b/test/philomena_web/controllers/image/tag_controller_test.exs @@ -0,0 +1,42 @@ +defmodule PhilomenaWeb.Image.TagControllerTest do + use PhilomenaWeb.ConnCase + 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 "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"]) + + 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"]) + + 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]" + ] + ) + + # Noop should not create a new tag change + assert snap(update_image_tags(ctx, add: [])) == snap + 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 new file mode 100644 index 000000000..5625d93f0 --- /dev/null +++ b/test/philomena_web/controllers/tag_change/revert_controller_test.exs @@ -0,0 +1,171 @@ +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, first_tag_change} = create_image_with_tag_change(ctx) + + 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 + 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, first_tag_change} = create_image_with_tag_change(ctx) + + 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." + } + ) + + 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, [first_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(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 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 == + ["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 + # 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 0 tags actually updated." + } + ) + + 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): [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 + 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 dd4240a4d..804f81295 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 @@ -30,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} @@ -58,8 +70,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/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/philomena/dummy.png b/test/support/philomena/dummy.png new file mode 100644 index 000000000..770601f79 Binary files /dev/null and b/test/support/philomena/dummy.png differ diff --git a/test/support/philomena/images.ex b/test/support/philomena/images.ex new file mode 100644 index 000000000..25589b1e6 --- /dev/null +++ b/test/support/philomena/images.ex @@ -0,0 +1,70 @@ +defmodule Philomena.Test.Images do + alias Philomena.Repo + alias Philomena.Images.Image + alias Philomena.Images + alias Philomena.Test + alias Philomena.Users.User + + 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 + + @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{ + 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: ctx.user + ] + + {: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) + + receive do + {:DOWN, ^upload_ref, :process, ^upload_pid, _reason} -> + Map.put(ctx, :image, image) + end + 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/pagination.ex b/test/support/philomena/pagination.ex new file mode 100644 index 000000000..dbcdb02e0 --- /dev/null +++ b/test/support/philomena/pagination.ex @@ -0,0 +1,32 @@ +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)) + + @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 + 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..8696ec286 --- /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 snap(%TagChanges.TagChange{} = tag_change) do + suffix = + tag_change.tags + |> Enum.map(fn %{tag: tag, added: added} -> + "#{if(added, do: "+", else: "-")}#{Test.Tags.snap(tag)}" + end) + |> Enum.join(" ") + + "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 diff --git a/test/support/philomena_web/tag_changes.ex b/test/support/philomena_web/tag_changes.ex new file mode 100644 index 000000000..c62d6df64 --- /dev/null +++ b/test/support/philomena_web/tag_changes.ex @@ -0,0 +1,88 @@ +defmodule PhilomenaWeb.Test.TagChanges do + alias Philomena.Images.Image + alias Philomena.Test + + import Phoenix.ConnTest + + use ExUnit.Case + + @endpoint PhilomenaWeb.Endpoint + + 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(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 = ctx.image.tags |> Enum.map(& &1.name) + + Enum.each(added_tags, fn tag -> + assert tag not in current_tags + end) + + Enum.each(removed_tags, fn tag -> + assert tag in current_tags + end) + + new_tags = + current_tags + |> Enum.reject(&(&1 in removed_tags)) + |> Enum.concat(added_tags) + + conn = + ctx.conn + |> post(~p"/images/#{ctx.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 + + ctx = put_in(ctx.conn, conn) + put_in(ctx.image, Test.Images.load_image!(ctx.image.id, preload: [:tags])) + end + + @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 = + tag_changes + |> Enum.map(&Test.TagChanges.snap/1) + + [Test.Images.snap(ctx.image) | tag_changes] + end +end