UX improvements v1 (#39)

* Removed collection_type user input instead inferring from yt-dlp response

* Updated docs

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

* Update source to always run an initial index

* Added deletion to the last models

* Improved clarity around deletion operation

* Improved task fetching and deletion methods

* More tests

test/pinchflat/sources_test.exs

@@ -23,6 +23,15 @@ defmodule Pinchflat.SourcesTest do
     end
   end
 
+  describe "list_sources_for/1" do
+    test "returns all sources for a given media profile" do
+      media_profile = media_profile_fixture()
+      source = source_fixture(media_profile_id: media_profile.id)
+
+      assert Sources.list_sources_for(media_profile) == [source]
+    end
+  end
+
   describe "get_source!/1" do
     test "it returns the source with given id" do
       source = source_fixture()
@@ -32,12 +41,11 @@ defmodule Pinchflat.SourcesTest do
 
   describe "create_source/1" do
     test "creates a source and adds name + ID from runner response for channels" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       valid_attrs = %{
         media_profile_id: media_profile_fixture().id,
-        original_url: "https://www.youtube.com/channel/abc123",
-        collection_type: "channel"
+        original_url: "https://www.youtube.com/channel/abc123"
       }
 
       assert {:ok, %Source{} = source} = Sources.create_source(valid_attrs)
@@ -46,12 +54,11 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "creates a source and adds name + ID for playlists" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &playlist_mock/3)
 
       valid_attrs = %{
         media_profile_id: media_profile_fixture().id,
-        original_url: "https://www.youtube.com/playlist?list=abc123",
-        collection_type: "playlist"
+        original_url: "https://www.youtube.com/playlist?list=abc123"
       }
 
       assert {:ok, %Source{} = source} = Sources.create_source(valid_attrs)
@@ -60,12 +67,11 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "you can specify a custom friendly_name" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       valid_attrs = %{
         media_profile_id: media_profile_fixture().id,
         original_url: "https://www.youtube.com/channel/abc123",
-        collection_type: "channel",
         friendly_name: "some custom name"
       }
 
@@ -75,12 +81,11 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "friendly name is pulled from collection_name if not specified" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       valid_attrs = %{
         media_profile_id: media_profile_fixture().id,
-        original_url: "https://www.youtube.com/channel/abc123",
-        collection_type: "channel"
+        original_url: "https://www.youtube.com/channel/abc123"
       }
 
       assert {:ok, %Source{} = source} = Sources.create_source(valid_attrs)
@@ -88,6 +93,22 @@ defmodule Pinchflat.SourcesTest do
       assert source.friendly_name == "some channel name"
     end
 
+    test "collection_type is inferred from source details" do
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
+      expect(YtDlpRunnerMock, :run, &playlist_mock/3)
+
+      valid_attrs = %{
+        media_profile_id: media_profile_fixture().id,
+        original_url: "https://www.youtube.com/channel/abc123"
+      }
+
+      assert {:ok, %Source{} = source_1} = Sources.create_source(valid_attrs)
+      assert {:ok, %Source{} = source_2} = Sources.create_source(valid_attrs)
+
+      assert source_1.collection_type == :channel
+      assert source_2.collection_type == :playlist
+    end
+
     test "creation with invalid data returns error changeset" do
       assert {:error, %Ecto.Changeset{}} = Sources.create_source(@invalid_source_attrs)
     end
@@ -97,14 +118,15 @@ defmodule Pinchflat.SourcesTest do
         {:ok,
          Phoenix.json_library().encode!(%{
            channel: "some channel name",
-           channel_id: "some_channel_id_12345678"
+           channel_id: "some_channel_id_12345678",
+           playlist_id: "some_channel_id_12345678",
+           playlist_title: "some channel name - videos"
          })}
       end)
 
       valid_once_attrs = %{
         media_profile_id: media_profile_fixture().id,
-        original_url: "https://www.youtube.com/channel/abc123",
-        collection_type: "channel"
+        original_url: "https://www.youtube.com/channel/abc123"
       }
 
       assert {:ok, %Source{}} = Sources.create_source(valid_once_attrs)
@@ -116,14 +138,15 @@ defmodule Pinchflat.SourcesTest do
         {:ok,
          Phoenix.json_library().encode!(%{
            channel: "some channel name",
-           channel_id: "some_channel_id_12345678"
+           channel_id: "some_channel_id_12345678",
+           playlist_id: "some_channel_id_12345678",
+           playlist_title: "some channel name - videos"
          })}
       end)
 
       valid_attrs = %{
         name: "some name",
-        original_url: "https://www.youtube.com/channel/abc123",
-        collection_type: "channel"
+        original_url: "https://www.youtube.com/channel/abc123"
       }
 
       source_1_attrs = Map.merge(valid_attrs, %{media_profile_id: media_profile_fixture().id})
@@ -134,12 +157,25 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "creation will schedule the indexing task" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
+
+      valid_attrs = %{
+        media_profile_id: media_profile_fixture().id,
+        original_url: "https://www.youtube.com/channel/abc123"
+      }
+
+      assert {:ok, %Source{} = source} = Sources.create_source(valid_attrs)
+
+      assert_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
+    end
+
+    test "creation schedules an index test even if the index frequency is 0" do
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       valid_attrs = %{
         media_profile_id: media_profile_fixture().id,
         original_url: "https://www.youtube.com/channel/abc123",
-        collection_type: "channel"
+        index_frequency_minutes: 0
       }
 
       assert {:ok, %Source{} = source} = Sources.create_source(valid_attrs)
@@ -158,7 +194,7 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "updating the original_url will re-fetch the source details for channels" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       source = source_fixture()
       update_attrs = %{original_url: "https://www.youtube.com/channel/abc123"}
@@ -169,9 +205,9 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "updating the original_url will re-fetch the source details for playlists" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &playlist_mock/3)
 
-      source = source_fixture(collection_type: "playlist")
+      source = source_fixture()
       update_attrs = %{original_url: "https://www.youtube.com/playlist?list=abc123"}
 
       assert {:ok, %Source{} = source} = Sources.update_source(source, update_attrs)
@@ -180,7 +216,7 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "not updating the original_url will not re-fetch the source details" do
-      expect(YtDlpRunnerMock, :run, 0, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, 0, &channel_mock/3)
 
       source = source_fixture()
       update_attrs = %{name: "some updated name"}
@@ -188,7 +224,7 @@ defmodule Pinchflat.SourcesTest do
       assert {:ok, %Source{}} = Sources.update_source(source, update_attrs)
     end
 
-    test "updating the index frequency will re-schedule the indexing task" do
+    test "updating the index frequency to >0 will re-schedule the indexing task" do
       source = source_fixture()
       update_attrs = %{index_frequency_minutes: 123}
 
@@ -197,11 +233,34 @@ defmodule Pinchflat.SourcesTest do
       assert_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
     end
 
-    test "not updating the index frequency will not re-schedule the indexing task" do
+    test "updating the index frequency to 0 will not re-schedule the indexing task" do
       source = source_fixture()
+      update_attrs = %{index_frequency_minutes: 0}
+
+      assert {:ok, %Source{}} = Sources.update_source(source, update_attrs)
+
+      refute_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
+    end
+
+    test "updating the index frequency to 0 will delete any pending tasks" do
+      source = source_fixture()
+      {:ok, job} = Oban.insert(MediaIndexingWorker.new(%{"id" => source.id}))
+      task = task_fixture(source_id: source.id, job_id: job.id)
+      update_attrs = %{index_frequency_minutes: 0}
+
+      assert {:ok, %Source{}} = Sources.update_source(source, update_attrs)
+
+      assert_raise Ecto.NoResultsError, fn -> Repo.reload!(task) end
+    end
+
+    test "not updating the index frequency will not re-schedule the indexing task or delete tasks" do
+      source = source_fixture()
+      task = task_fixture(source_id: source.id)
       update_attrs = %{name: "some updated name"}
 
       assert {:ok, %Source{}} = Sources.update_source(source, update_attrs)
+
+      assert Repo.reload!(task)
       refute_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
     end
 
@@ -236,7 +295,7 @@ defmodule Pinchflat.SourcesTest do
     end
   end
 
-  describe "delete_source/1" do
+  describe "delete_source/2" do
     test "it deletes the source" do
       source = source_fixture()
       assert {:ok, %Source{}} = Sources.delete_source(source)
@@ -255,6 +314,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/2 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
@@ -267,14 +363,14 @@ defmodule Pinchflat.SourcesTest do
 
   describe "change_source_from_url/2" do
     test "it returns a changeset" do
-      stub(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      stub(YtDlpRunnerMock, :run, &channel_mock/3)
       source = source_fixture()
 
       assert %Ecto.Changeset{} = Sources.change_source_from_url(source, %{})
     end
 
     test "it does not fetch source details if the original_url isn't in the changeset" do
-      expect(YtDlpRunnerMock, :run, 0, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, 0, &channel_mock/3)
 
       changeset = Sources.change_source_from_url(%Source{}, %{name: "some updated name"})
 
@@ -282,7 +378,7 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "it fetches source details if the original_url is in the changeset" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       changeset =
         Sources.change_source_from_url(%Source{}, %{
@@ -293,13 +389,13 @@ defmodule Pinchflat.SourcesTest do
     end
 
     test "it adds source details to the changeset, keeping the orignal details" do
-      expect(YtDlpRunnerMock, :run, &runner_function_mock/3)
+      expect(YtDlpRunnerMock, :run, &channel_mock/3)
 
       media_profile = media_profile_fixture()
       media_profile_id = media_profile.id
 
       changeset =
-        Sources.change_source_from_url(%Source{collection_type: :channel}, %{
+        Sources.change_source_from_url(%Source{}, %{
           original_url: "https://www.youtube.com/channel/abc123",
           media_profile_id: media_profile.id
         })
@@ -329,15 +425,29 @@ defmodule Pinchflat.SourcesTest do
     end
   end
 
-  defp runner_function_mock(_url, _opts, _ot) do
+  defp playlist_mock(_url, _opts, _ot) do
     {
       :ok,
       Phoenix.json_library().encode!(%{
-        channel: "some channel name",
-        channel_id: "some_channel_id_#{:rand.uniform(1_000_000)}",
+        channel: nil,
+        channel_id: nil,
         playlist_id: "some_playlist_id_#{:rand.uniform(1_000_000)}",
         playlist_title: "some playlist name"
       })
     }
   end
+
+  defp channel_mock(_url, _opts, _ot) do
+    channel_id = "some_channel_id_#{:rand.uniform(1_000_000)}"
+
+    {
+      :ok,
+      Phoenix.json_library().encode!(%{
+        channel: "some channel name",
+        channel_id: channel_id,
+        playlist_id: channel_id,
+        playlist_title: "some channel name - videos"
+      })
+    }
+  end
 end