Added delete buttons for source; Refactored the way deletion methods work

lib/pinchflat/media.ex

@@ -12,12 +12,25 @@ defmodule Pinchflat.Media do
   alias Pinchflat.Media.MediaMetadata
 
   @doc """
-  Returns the list of media_items. Returns [%MediaItem{}, ...].
+  Returns the list of media_items.
+
+  Returns [%MediaItem{}, ...].
   """
   def list_media_items do
     Repo.all(MediaItem)
   end
 
+  @doc """
+  Returns a list of media_items for a given source.
+
+  Returns [%MediaItem{}, ...].
+  """
+  def list_media_items_for(%Source{} = source) do
+    MediaItem
+    |> where([mi], mi.source_id == ^source.id)
+    |> Repo.all()
+  end
+
   @doc """
   Returns a list of pending media_items for a given source, where
   pending means the `media_filepath` is `nil` AND the media_item
@@ -138,26 +151,30 @@ defmodule Pinchflat.Media do
   end
 
   @doc """
-  Deletes a media_item and its associated tasks. Will leave files on disk.
+  Deletes a media_item and its associated tasks.
+  Can optionally delete the media_item's files.
 
   Returns {:ok, %MediaItem{}} | {:error, %Ecto.Changeset{}}.
   """
-  def delete_media_item(%MediaItem{} = media_item) do
+  def delete_media_item(%MediaItem{} = media_item, opts \\ []) do
+    delete_files = Keyword.get(opts, :delete_files, false)
+
+    if delete_files do
+      {:ok, _} = delete_all_attachments(media_item)
+    end
+
     Tasks.delete_tasks_for(media_item)
     Repo.delete(media_item)
   end
 
   @doc """
-  Deletes the media_item's associated files. Will leave the media_item in the database.
+  Returns an `%Ecto.Changeset{}` for tracking media_item changes.
-
-  NOTE: this deletes the metadata files as well, but maybe it shouldn't? I'm wondering if
-  the metadata is more a concern of the DB record itself and should be lumped in with those
-  delete operations. But the metadata does come from the download operation of the file.
-  Food for thought but not a priority at the moment.
-
-  Returns {:ok, %MediaItem{}}
   """
-  def delete_attachments(media_item) do
+  def change_media_item(%MediaItem{} = media_item, attrs \\ %{}) do
+    MediaItem.changeset(media_item, attrs)
+  end
+
+  defp delete_all_attachments(media_item) do
     media_item = Repo.preload(media_item, :metadata)
 
     media_item
@@ -177,25 +194,6 @@ defmodule Pinchflat.Media do
     {:ok, media_item}
   end
 
-  @doc """
-  Deletes the media_item and all associated files. Attempts to delete the root directory
-  but only if it is empty.
-
-  Returns {:ok, %MediaItem{}}
-  """
-  def delete_media_item_and_attachments(media_item) do
-    {:ok, _} = delete_attachments(media_item)
-
-    delete_media_item(media_item)
-  end
-
-  @doc """
-  Returns an `%Ecto.Changeset{}` for tracking media_item changes.
-  """
-  def change_media_item(%MediaItem{} = media_item, attrs \\ %{}) do
-    MediaItem.changeset(media_item, attrs)
-  end
-
   defp build_format_clauses(media_profile) do
     mapped_struct = Map.from_struct(media_profile)
 

lib/pinchflat/sources.ex

@@ -6,6 +6,7 @@ defmodule Pinchflat.Sources do
   import Ecto.Query, warn: false
   alias Pinchflat.Repo
 
+  alias Pinchflat.Media
   alias Pinchflat.Tasks
   alias Pinchflat.Tasks.SourceTasks
   alias Pinchflat.Sources.Source
@@ -55,13 +56,20 @@ defmodule Pinchflat.Sources do
   end
 
   @doc """
-  Deletes a source and it's associated tasks (of any state).
+  Deletes a source, its media items, and its associated tasks (of any state).
-  NOTE: will fail if the source has associated media items. Intended
+  Can optionally delete the source's media files.
-  for now, will almost certainly change in the future.
 
   Returns {:ok, %Source{}} | {:error, %Ecto.Changeset{}}
   """
-  def delete_source(%Source{} = source) do
+  def delete_source(%Source{} = source, opts \\ []) do
+    delete_files = Keyword.get(opts, :delete_files, false)
+
+    source
+    |> Media.list_media_items_for()
+    |> Enum.each(fn media_item ->
+      Media.delete_media_item(media_item, delete_files: delete_files)
+    end)
+
     Tasks.delete_tasks_for(source)
     Repo.delete(source)
   end

lib/pinchflat_web/controllers/media_items/media_item_controller.ex

@@ -14,7 +14,7 @@ defmodule PinchflatWeb.MediaItems.MediaItemController do
     media_item = Media.get_media_item!(id)
 
     if delete_files do
-      {:ok, _} = Media.delete_media_item_and_attachments(media_item)
+      {:ok, _} = Media.delete_media_item(media_item, delete_files: true)
 
       conn
       |> put_flash(:info, "Record and files deleted successfully.")

lib/pinchflat_web/controllers/media_items/media_item_html/show.html.heex

@@ -7,17 +7,6 @@
       Media Item #<%= @media_item.id %>
     </h2>
   </div>
-  <nav>
-    <.link
-      href={~p"/sources/#{@media_item.source_id}/media/#{@media_item}?delete_files=true"}
-      method="delete"
-      data-confirm="Are you sure?"
-    >
-      <.button color="bg-meta-1" rounding="rounded-full">
-        Delete Record and Files
-      </.button>
-    </.link>
-  </nav>
 </div>
 <div class="rounded-sm border border-stroke bg-white px-5 pb-2.5 pt-6 shadow-default dark:border-strokedark dark:bg-boxdark sm:px-7.5 xl:pb-1">
   <div class="max-w-full overflow-x-auto">
@@ -25,5 +14,17 @@
       <h3 class="font-bold text-xl">Attributes</h3>
       <.list_items_from_map map={Map.from_struct(@media_item)} />
     </div>
+
+    <section class="flex justify-center my-10">
+      <.link
+        href={~p"/sources/#{@media_item.source_id}/media/#{@media_item}?delete_files=true"}
+        method="delete"
+        data-confirm="Are you sure you want to delete this record and ALL files associated with it? This cannot be undone."
+      >
+        <.button color="bg-meta-1" rounding="rounded-full">
+          Delete Files
+        </.button>
+      </.link>
+    </section>
   </div>
 </div>

lib/pinchflat_web/controllers/sources/source_controller.ex

@@ -87,14 +87,24 @@ defmodule PinchflatWeb.Sources.SourceController do
     end
   end
 
-  def delete(conn, %{"id" => id}) do
+  def delete(conn, %{"id" => id} = params) do
+    delete_files = Map.get(params, "delete_files", false)
     source = Sources.get_source!(id)
+
+    if delete_files do
+      {:ok, _source} = Sources.delete_source(source, delete_files: true)
+
+      conn
+      |> put_flash(:info, "Source and files deleted successfully.")
+      |> redirect(to: ~p"/sources")
+    else
       {:ok, _source} = Sources.delete_source(source)
 
       conn
-    |> put_flash(:info, "Source deleted successfully.")
+      |> put_flash(:info, "Source deleted successfully. Files were not deleted.")
       |> redirect(to: ~p"/sources")
     end
+  end
 
   defp media_profiles do
     Profiles.list_media_profiles()

lib/pinchflat_web/controllers/sources/source_html/show.html.heex

@@ -72,5 +72,27 @@
         <p class="text-black dark:text-white">Nothing Here!</p>
       <% end %>
     </div>
+
+    <section class="flex flex-col md:flex-row items-center md:justify-around mt-10">
+      <.link
+        href={~p"/sources/#{@source.id}"}
+        method="delete"
+        data-confirm="Are you sure you want to delete this source (leaving files in place)? This cannot be undone."
+      >
+        <.button color="bg-meta-1" rounding="rounded-full">
+          Delete Source
+        </.button>
+      </.link>
+      <.link
+        href={~p"/sources/#{@source.id}?delete_files=true"}
+        method="delete"
+        data-confirm="Are you sure you want to delete this source AND it's associated files? This cannot be undone."
+        class="mt-5 md:mt-0"
+      >
+        <.button color="bg-meta-1" rounding="rounded-full">
+          Delete Source and Files
+        </.button>
+      </.link>
+    </section>
   </div>
 </div>

test/pinchflat/media_test.exs

@@ -36,6 +36,15 @@ defmodule Pinchflat.MediaTest do
     end
   end
 
+  describe "list_media_items_for/1" do
+    test "it returns media_items for a given source" do
+      source = source_fixture()
+      media_item = media_item_fixture(%{source_id: source.id})
+
+      assert Media.list_media_items_for(source) == [media_item]
+    end
+  end
+
   describe "list_pending_media_items_for/1" do
     test "it returns pending without a filepath for a given source" do
       source = source_fixture()
@@ -345,13 +354,20 @@ defmodule Pinchflat.MediaTest do
       assert {:ok, %MediaItem{}} = Media.delete_media_item(media_item)
       assert_raise Ecto.NoResultsError, fn -> Repo.reload!(task) end
     end
+
+    test "does not delete the media_item's files by default" do
+      media_item = media_item_with_attachments()
+
+      assert {:ok, _} = Media.delete_media_item(media_item)
+      assert File.exists?(media_item.media_filepath)
+    end
   end
 
-  describe "delete_attachments/1" do
+  describe "delete_media_item/1 when testing file deletion" do
     test "deletes the media item's files" do
       media_item = media_item_with_attachments()
 
-      assert {:ok, _} = Media.delete_attachments(media_item)
+      assert {:ok, _} = Media.delete_media_item(media_item, delete_files: true)
       refute File.exists?(media_item.media_filepath)
     end
 
@@ -371,23 +387,21 @@ defmodule Pinchflat.MediaTest do
 
       {:ok, updated_media_item} = Media.update_media_item(media_item, update_attrs)
 
-      assert {:ok, _} = Media.delete_attachments(updated_media_item)
+      assert {:ok, _} = Media.delete_media_item(updated_media_item, delete_files: true)
       refute File.exists?(updated_media_item.metadata.metadata_filepath)
     end
 
-    test "does not delete the media item" do
+    test "deletion deletes the media_item" do
-      media_item = media_item_with_attachments()
+      media_item = media_item_fixture()
-
+      assert {:ok, %MediaItem{}} = Media.delete_media_item(media_item, delete_files: true)
-      assert {:ok, _} = Media.delete_attachments(media_item)
+      assert_raise Ecto.NoResultsError, fn -> Media.get_media_item!(media_item.id) end
-
-      assert Repo.reload!(media_item)
     end
 
     test "deletes the parent folder if it is empty" do
       media_item = media_item_with_attachments()
       root_directory = Path.dirname(media_item.media_filepath)
 
-      assert {:ok, _} = Media.delete_attachments(media_item)
+      assert {:ok, _} = Media.delete_media_item(media_item, delete_files: true)
       refute File.exists?(root_directory)
     end
 
@@ -396,7 +410,7 @@ defmodule Pinchflat.MediaTest do
       root_directory = Path.dirname(media_item.media_filepath)
       File.touch(Path.join([root_directory, "test.txt"]))
 
-      assert {:ok, _} = Media.delete_attachments(media_item)
+      assert {:ok, _} = Media.delete_media_item(media_item, delete_files: true)
       assert File.exists?(root_directory)
 
       :ok = File.rm(Path.join([root_directory, "test.txt"]))
@@ -404,24 +418,6 @@ defmodule Pinchflat.MediaTest do
     end
   end
 
-  describe "delete_media_item_and_attachments/1" do
-    setup do
-      media_item = media_item_with_attachments()
-      {:ok, media_item: media_item}
-    end
-
-    test "deletes the media item", %{media_item: media_item} do
-      assert {:ok, _} = Media.delete_media_item_and_attachments(media_item)
-      assert_raise Ecto.NoResultsError, fn -> Media.get_media_item!(media_item.id) end
-    end
-
-    test "deletes associated files", %{media_item: media_item} do
-      assert File.exists?(media_item.media_filepath)
-      assert {:ok, _} = Media.delete_media_item_and_attachments(media_item)
-      refute File.exists?(media_item.media_filepath)
-    end
-  end
-
   describe "change_media_item/1" do
     test "change_media_item/1 returns a media_item changeset" do
       media_item = media_item_fixture()

test/pinchflat/sources_test.exs

@@ -268,6 +268,43 @@ defmodule Pinchflat.SourcesTest do
       assert {:ok, %Source{}} = Sources.delete_source(source)
       assert_raise Ecto.NoResultsError, fn -> Repo.reload!(task) end
     end
+
+    test "deletion also deletes all associated media items" do
+      source = source_fixture()
+      media_item = media_item_fixture(source_id: source.id)
+
+      assert {:ok, %Source{}} = Sources.delete_source(source)
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(media_item) end
+    end
+
+    test "deletion does not delete media files by default" do
+      source = source_fixture()
+      media_item = media_item_with_attachments(%{source_id: source.id})
+
+      assert {:ok, %Source{}} = Sources.delete_source(source)
+      assert File.exists?(media_item.media_filepath)
+    end
+  end
+
+  describe "delete_source/1 when deleting files" do
+    test "deletes source and media_items" do
+      source = source_fixture()
+      media_item = media_item_with_attachments(%{source_id: source.id})
+
+      assert {:ok, %Source{}} = Sources.delete_source(source, delete_files: true)
+
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(media_item) end
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(source) end
+    end
+
+    test "also deletes media files" do
+      source = source_fixture()
+      media_item = media_item_with_attachments(%{source_id: source.id})
+
+      assert {:ok, %Source{}} = Sources.delete_source(source, delete_files: true)
+
+      refute File.exists?(media_item.media_filepath)
+    end
   end
 
   describe "change_source/2" do

test/pinchflat_web/controllers/media_item_controller_test.exs

@@ -4,7 +4,6 @@ defmodule PinchflatWeb.MediaItemControllerTest do
   import Pinchflat.MediaFixtures
 
   alias Pinchflat.Repo
-  alias Pinchflat.Media
 
   describe "show media" do
     setup [:create_media_item]
@@ -19,10 +18,6 @@ defmodule PinchflatWeb.MediaItemControllerTest do
     setup do
       media_item = media_item_with_attachments()
 
-      on_exit(fn ->
-        Media.delete_attachments(media_item)
-      end)
-
       %{media_item: media_item}
     end
 

test/pinchflat_web/controllers/source_controller_test.exs

@@ -2,8 +2,11 @@ defmodule PinchflatWeb.SourceControllerTest do
   use PinchflatWeb.ConnCase
   import Mox
 
-  import Pinchflat.ProfilesFixtures
+  import Pinchflat.MediaFixtures
   import Pinchflat.SourcesFixtures
+  import Pinchflat.ProfilesFixtures
+
+  alias Pinchflat.Repo
 
   setup do
     media_profile = media_profile_fixture()
@@ -119,21 +122,53 @@ defmodule PinchflatWeb.SourceControllerTest do
     end
   end
 
-  describe "delete source" do
+  describe "delete source when just deleting the records" do
     setup [:create_source]
 
-    test "deletes chosen source", %{conn: conn, source: source} do
+    test "deletes chosen source and media_items", %{conn: conn, source: source, media_item: media_item} do
+      delete(conn, ~p"/sources/#{source}")
+
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(source) end
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(media_item) end
+    end
+
+    test "redirects to the sources page", %{conn: conn, source: source} do
       conn = delete(conn, ~p"/sources/#{source}")
       assert redirected_to(conn) == ~p"/sources"
-
-      assert_error_sent 404, fn ->
-        get(conn, ~p"/sources/#{source}")
     end
+
+    test "does not delete the files", %{conn: conn, source: source, media_item: media_item} do
+      delete(conn, ~p"/sources/#{source}")
+      assert File.exists?(media_item.media_filepath)
+    end
+  end
+
+  describe "delete source when deleting the records and files" do
+    setup [:create_source]
+
+    test "deletes chosen source and media_items", %{conn: conn, source: source, media_item: media_item} do
+      delete(conn, ~p"/sources/#{source}?delete_files=true")
+
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(source) end
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(media_item) end
+    end
+
+    test "redirects to the sources page", %{conn: conn, source: source} do
+      conn = delete(conn, ~p"/sources/#{source}?delete_files=true")
+      assert redirected_to(conn) == ~p"/sources"
+    end
+
+    test "deletes the files", %{conn: conn, source: source, media_item: media_item} do
+      delete(conn, ~p"/sources/#{source}?delete_files=true")
+      refute File.exists?(media_item.media_filepath)
     end
   end
 
   defp create_source(_) do
-    %{source: source_fixture()}
+    source = source_fixture()
+    media_item = media_item_with_attachments(%{source_id: source.id})
+
+    %{source: source, media_item: media_item}
   end
 
   defp runner_function_mock(_url, _opts, _ot) do