properly transactionalize image hiding and merging

lib/philomena/duplicate_reports.ex

@@ -74,45 +74,32 @@ defmodule Philomena.DuplicateReports do
     |> Repo.insert()
   end
 
-  # TODO: can we get this in a single transaction?
+  def accept_duplicate_report(multi \\ nil, %DuplicateReport{} = duplicate_report, user) do
-  def accept_duplicate_report(%DuplicateReport{} = duplicate_report, user) do
+    duplicate_report = Repo.preload(duplicate_report, [:image, :duplicate_of_image])
-    changeset =
-      duplicate_report
-      |> DuplicateReport.accept_changeset(user)
 
-    Multi.new()
+    other_duplicate_reports =
+      DuplicateReport
+      |> where(
+        [dr],
+        (dr.image_id == ^duplicate_report.image_id and
+           dr.duplicate_of_image_id == ^duplicate_report.duplicate_of_image_id) or
+          (dr.image_id == ^duplicate_report.duplicate_of_image_id and
+             dr.duplicate_of_image_id == ^duplicate_report.image_id)
+      )
+      |> where([dr], dr.id != ^duplicate_report.id)
+      |> update(set: [state: "rejected"])
+
+    changeset = DuplicateReport.accept_changeset(duplicate_report, user)
+
+    multi = multi || Multi.new()
+
+    multi
     |> Multi.update(:duplicate_report, changeset)
-    |> Multi.run(:other_reports, fn repo, %{duplicate_report: duplicate_report} ->
+    |> Multi.update_all(:other_reports, other_duplicate_reports, [])
-      {count, nil} =
+    |> Images.merge_image(duplicate_report.image, duplicate_report.duplicate_of_image)
-        DuplicateReport
-        |> where(
-          [dr],
-          (dr.image_id == ^duplicate_report.image_id and
-             dr.duplicate_of_image_id == ^duplicate_report.duplicate_of_image_id) or
-            (dr.image_id == ^duplicate_report.duplicate_of_image_id and
-               dr.duplicate_of_image_id == ^duplicate_report.image_id)
-        )
-        |> where([dr], dr.id != ^duplicate_report.id)
-        |> repo.update_all(set: [state: "rejected"])
-
-      {:ok, count}
-    end)
-    |> Repo.isolated_transaction(:serializable)
-    |> case do
-      {:ok, %{duplicate_report: duplicate_report}} ->
-        duplicate_report = Repo.preload(duplicate_report, [:image, :duplicate_of_image])
-
-        Images.merge_image(duplicate_report.image, duplicate_report.duplicate_of_image)
-
-      error ->
-        error
-    end
   end
 
   def accept_reverse_duplicate_report(%DuplicateReport{} = duplicate_report, user) do
-    {:ok, duplicate_report} = reject_duplicate_report(duplicate_report, user)
-
-    # Need a constraint for upsert, so have to do it the hard way
     new_report =
       DuplicateReport
       |> where(duplicate_of_image_id: ^duplicate_report.image_id)
@@ -123,20 +110,21 @@ defmodule Philomena.DuplicateReports do
       if new_report do
         new_report
       else
-        {:ok, duplicate_report} =
+        %DuplicateReport{
-          %DuplicateReport{
+          image_id: duplicate_report.duplicate_of_image_id,
-            image_id: duplicate_report.duplicate_of_image_id,
+          duplicate_of_image_id: duplicate_report.image_id,
-            duplicate_of_image_id: duplicate_report.image_id,
+          reason: Enum.join([duplicate_report.reason, "(Reverse accepted)"], "\n"),
-            reason: Enum.join([duplicate_report.reason, "(Reverse accepted)"], "\n"),
+          user_id: user.id
-            user_id: user.id
+        }
-          }
+        |> DuplicateReport.changeset(%{})
-          |> DuplicateReport.changeset(%{})
+        |> Repo.insert!()
-          |> Repo.insert()
-
-        duplicate_report
       end
 
-    accept_duplicate_report(new_report, user)
+    Multi.new()
+    |> Multi.run(:reject_duplicate_report, fn _, %{} ->
+      reject_duplicate_report(duplicate_report, user)
+    end)
+    |> accept_duplicate_report(new_report, user)
   end
 
   def claim_duplicate_report(%DuplicateReport{} = duplicate_report, user) do

lib/philomena/images.ex

@@ -24,6 +24,7 @@ defmodule Philomena.Images do
   alias Philomena.Tags.Tag
   alias Philomena.Notifications
   alias Philomena.Interactions
+  alias Philomena.Reports
   alias Philomena.Reports.Report
   alias Philomena.Comments
 
@@ -365,66 +366,79 @@ defmodule Philomena.Images do
     |> Repo.update()
   end
 
-  def hide_image(%Image{} = image, user, attrs) do
-    DuplicateReport
-    |> where(state: "open")
-    |> where([d], d.image_id == ^image.id or d.duplicate_of_image_id == ^image.id)
-    |> Repo.update_all(set: [state: "rejected"])
-
-    Image.hide_changeset(image, attrs, user)
-    |> internal_hide_image(image)
-  end
-
   def update_hide_reason(%Image{} = image, attrs) do
     image
     |> Image.hide_reason_changeset(attrs)
     |> Repo.update()
-  end
+    |> case do
+      {:ok, image} ->
+        reindex_image(image)
 
-  def merge_image(%Image{} = image, duplicate_of_image) do
+        {:ok, image}
-    result =
-      Image.merge_changeset(image, duplicate_of_image)
-      |> internal_hide_image(image)
 
-    case result do
+      error ->
-      {:ok, changes} ->
+        error
-        update_first_seen_at(
-          duplicate_of_image,
-          image.first_seen_at,
-          duplicate_of_image.first_seen_at
-        )
-
-        tags = Tags.copy_tags(image, duplicate_of_image)
-        Comments.migrate_comments(image, duplicate_of_image)
-        Interactions.migrate_interactions(image, duplicate_of_image)
-
-        {:ok, %{changes | tags: changes.tags ++ tags}}
-
-      _error ->
-        result
     end
   end
 
-  defp update_first_seen_at(image, time_1, time_2) do
+  def hide_image(%Image{} = image, user, attrs) do
-    min_time =
+    duplicate_reports =
-      case NaiveDateTime.compare(time_1, time_2) do
+      DuplicateReport
-        :gt -> time_2
+      |> where(state: "open")
-        _ -> time_1
+      |> where([d], d.image_id == ^image.id or d.duplicate_of_image_id == ^image.id)
-      end
+      |> update(set: [state: "rejected"])
 
-    Image
+    image
-    |> where(id: ^image.id)
+    |> Image.hide_changeset(attrs, user)
-    |> Repo.update_all(set: [first_seen_at: min_time])
+    |> hide_image_multi(image, Ecto.Multi.new())
+    |> Multi.update_all(:duplicate_reports, duplicate_reports, [])
+    |> Repo.transaction()
+    |> process_after_hide()
   end
 
-  defp internal_hide_image(changeset, image) do
+  def merge_image(multi \\ nil, %Image{} = image, duplicate_of_image) do
+    multi = multi || Multi.new()
+
+    image
+    |> Image.merge_changeset(duplicate_of_image)
+    |> hide_image_multi(image, multi)
+    |> Multi.run(:first_seen_at, fn _, %{} ->
+      update_first_seen_at(
+        duplicate_of_image,
+        image.first_seen_at,
+        duplicate_of_image.first_seen_at
+      )
+    end)
+    |> Multi.run(:copy_tags, fn _, %{} ->
+      {:ok, Tags.copy_tags(image, duplicate_of_image)}
+    end)
+    |> Multi.run(:migrate_comments, fn _, %{} ->
+      {:ok, Comments.migrate_comments(image, duplicate_of_image)}
+    end)
+    |> Multi.run(:migrate_interactions, fn _, %{} ->
+      {:ok, Interactions.migrate_interactions(image, duplicate_of_image)}
+    end)
+    |> Repo.transaction()
+    |> process_after_hide()
+    |> case do
+      {:ok, result} ->
+        reindex_image(duplicate_of_image)
+
+        {:ok, result}
+
+      error ->
+        error
+    end
+  end
+
+  defp hide_image_multi(changeset, image, multi) do
     reports =
       Report
       |> where(reportable_type: "Image", reportable_id: ^image.id)
       |> select([r], r.id)
       |> update(set: [open: false, state: "closed"])
 
-    Multi.new()
+    multi
     |> Multi.update(:image, changeset)
     |> Multi.update_all(:reports, reports, [])
     |> Multi.run(:tags, fn repo, %{image: image} ->
@@ -440,12 +454,40 @@ defmodule Philomena.Images do
 
       {:ok, image.tags}
     end)
-    |> Multi.run(:file, fn _repo, %{image: image} ->
+  end
-      Hider.hide_thumbnails(image, image.hidden_image_key)
 
-      {:ok, nil}
+  defp process_after_hide(result) do
-    end)
+    case result do
-    |> Repo.isolated_transaction(:serializable)
+      {:ok, %{image: image, tags: tags, reports: {_count, reports}} = result} ->
+        Hider.hide_thumbnails(image, image.hidden_image_key)
+
+        Reports.reindex_reports(reports)
+        Tags.reindex_tags(tags)
+        reindex_image(image)
+        reindex_copied_tags(result)
+
+        {:ok, result}
+
+      error ->
+        error
+    end
+  end
+
+  defp reindex_copied_tags(%{copy_tags: tags}), do: Tags.reindex_tags(tags)
+  defp reindex_copied_tags(_result), do: nil
+
+  defp update_first_seen_at(image, time_1, time_2) do
+    min_time =
+      case NaiveDateTime.compare(time_1, time_2) do
+        :gt -> time_2
+        _ -> time_1
+      end
+
+    Image
+    |> where(id: ^image.id)
+    |> Repo.update_all(set: [first_seen_at: min_time])
+
+    {:ok, image}
   end
 
   def unhide_image(%Image{hidden_from_users: true} = image) do
@@ -463,12 +505,19 @@ defmodule Philomena.Images do
 
       {:ok, image.tags}
     end)
-    |> Multi.run(:file, fn _repo, %{image: image} ->
+    |> Repo.transaction()
-      Hider.unhide_thumbnails(image, key)
+    |> case do
+      {:ok, %{image: image, tags: tags}} ->
+        Hider.unhide_thumbnails(image, key)
 
-      {:ok, nil}
+        reindex_image(image)
-    end)
+        Tags.reindex_tags(tags)
-    |> Repo.isolated_transaction(:serializable)
+
+        {:ok, image}
+
+      error ->
+        error
+    end
   end
 
   def unhide_image(image), do: {:ok, image}

lib/philomena_web/controllers/duplicate_report/accept_controller.ex

@@ -3,8 +3,6 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptController do
 
   alias Philomena.DuplicateReports.DuplicateReport
   alias Philomena.DuplicateReports
-  alias Philomena.Reports
-  alias Philomena.Images
 
   plug PhilomenaWeb.CanaryMapPlug, create: :edit, delete: :edit
 
@@ -15,18 +13,14 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptController do
     preload: [:image, :duplicate_of_image]
 
   def create(conn, _params) do
-    {:ok, %{reports: {_count, reports}}} =
+    {:ok, _report} =
       DuplicateReports.accept_duplicate_report(
         conn.assigns.duplicate_report,
         conn.assigns.current_user
       )
 
-    Reports.reindex_reports(reports)
-    Images.reindex_image(conn.assigns.duplicate_report.image)
-    Images.reindex_image(conn.assigns.duplicate_report.duplicate_of_image)
-
     conn
     |> put_flash(:info, "Successfully accepted report.")
-    |> redirect(external: conn.assigns.referrer)
+    |> redirect(to: Routes.duplicate_report_path(conn, :index))
   end
 end

lib/philomena_web/controllers/duplicate_report/accept_reverse_controller.ex

@@ -3,8 +3,6 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptReverseController do
 
   alias Philomena.DuplicateReports.DuplicateReport
   alias Philomena.DuplicateReports
-  alias Philomena.Reports
-  alias Philomena.Images
 
   plug PhilomenaWeb.CanaryMapPlug, create: :edit, delete: :edit
 
@@ -15,18 +13,14 @@ defmodule PhilomenaWeb.DuplicateReport.AcceptReverseController do
     preload: [:image, :duplicate_of_image]
 
   def create(conn, _params) do
-    {:ok, %{reports: {_count, reports}}} =
+    {:ok, _report} =
       DuplicateReports.accept_reverse_duplicate_report(
         conn.assigns.duplicate_report,
         conn.assigns.current_user
       )
 
-    Reports.reindex_reports(reports)
-    Images.reindex_image(conn.assigns.duplicate_report.image)
-    Images.reindex_image(conn.assigns.duplicate_report.duplicate_of_image)
-
     conn
     |> put_flash(:info, "Successfully accepted report in reverse.")
-    |> redirect(external: conn.assigns.referrer)
+    |> redirect(to: Routes.duplicate_report_path(conn, :index))
   end
 end

lib/philomena_web/controllers/duplicate_report/reject_controller.ex

@@ -3,7 +3,6 @@ defmodule PhilomenaWeb.DuplicateReport.RejectController do
 
   alias Philomena.DuplicateReports.DuplicateReport
   alias Philomena.DuplicateReports
-  alias Philomena.Images
 
   plug PhilomenaWeb.CanaryMapPlug, create: :edit, delete: :edit
 
@@ -20,10 +19,6 @@ defmodule PhilomenaWeb.DuplicateReport.RejectController do
         conn.assigns.current_user
       )
 
-    {:ok, _image} = Images.unhide_image(conn.assigns.duplicate_report.image)
-    Images.reindex_image(conn.assigns.duplicate_report.image)
-    Images.reindex_image(conn.assigns.duplicate_report.duplicate_of_image)
-
     conn
     |> put_flash(:info, "Successfully rejected report.")
     |> redirect(external: conn.assigns.referrer)

lib/philomena_web/controllers/image/delete_controller.ex

@@ -6,8 +6,6 @@ defmodule PhilomenaWeb.Image.DeleteController do
 
   alias Philomena.Images.Image
   alias Philomena.Images
-  alias Philomena.Tags
-  alias Philomena.Reports
 
   plug PhilomenaWeb.CanaryMapPlug, create: :hide, delete: :hide
   plug :load_and_authorize_resource, model: Image, id_name: "image_id", persisted: true
@@ -18,11 +16,7 @@ defmodule PhilomenaWeb.Image.DeleteController do
     user = conn.assigns.current_user
 
     case Images.hide_image(image, user, image_params) do
-      {:ok, %{image: image, tags: tags, reports: {_count, reports}}} ->
+      {:ok, _image} ->
-        Images.reindex_image(image)
-        Reports.reindex_reports(reports)
-        Tags.reindex_tags(tags)
-
         conn
         |> put_flash(:info, "Image successfully hidden.")
         |> redirect(to: Routes.image_path(conn, :show, image))
@@ -37,8 +31,6 @@ defmodule PhilomenaWeb.Image.DeleteController do
 
     case Images.update_hide_reason(image, image_params) do
       {:ok, image} ->
-        Images.reindex_image(image)
-
         conn
         |> put_flash(:info, "Hide reason updated.")
         |> redirect(to: Routes.image_path(conn, :show, image))
@@ -66,9 +58,7 @@ defmodule PhilomenaWeb.Image.DeleteController do
   def delete(conn, _params) do
     image = conn.assigns.image
 
-    {:ok, %{image: image, tags: tags}} = Images.unhide_image(image)
+    {:ok, _image} = Images.unhide_image(image)
-    Images.reindex_image(image)
-    Tags.reindex_tags(tags)
 
     conn
     |> put_flash(:info, "Image successfully unhidden.")