Update source to always run an initial index

lib/pinchflat/media_source/source.ex

@@ -27,6 +27,7 @@ defmodule Pinchflat.Sources.Source do
     collection_id
     collection_type
     friendly_name
+    index_frequency_minutes
     download_media
     original_url
     media_profile_id

lib/pinchflat/sources.ex

@@ -169,21 +169,19 @@ defmodule Pinchflat.Sources do
     {:ok, source}
   end
 
-  # IDEA: this uses a pattern where `kickoff_indexing_task` controls whether
-  # it should run based on the source, but `maybe_handle_media_tasks` handles that
-  # logic itself. Consider updating one or the other to be consistent (once I've
-  # decided which I like more)
   defp maybe_run_indexing_task(changeset, source) do
     case changeset.data do
       # If the changeset is new (not persisted), attempt indexing no matter what
       %{__meta__: %{state: :built}} ->
         SourceTasks.kickoff_indexing_task(source)
 
-      # If the record has been persisted, only attempt indexing if the
-      # indexing frequency has been changed
+      # If the record has been persisted, only run indexing if the
+      # indexing frequency has been changed and is now greater than 0
       %{__meta__: %{state: :loaded}} ->
-        if Map.has_key?(changeset.changes, :index_frequency_minutes) do
-          SourceTasks.kickoff_indexing_task(source)
+        case changeset.changes do
+          %{index_frequency_minutes: mins} when mins > 0 -> SourceTasks.kickoff_indexing_task(source)
+          %{index_frequency_minutes: _} -> Tasks.delete_pending_tasks_for(source)
+          _ -> :ok
         end
     end
 

lib/pinchflat/tasks/source_tasks.ex

@@ -12,26 +12,23 @@ defmodule Pinchflat.Tasks.SourceTasks do
   alias Pinchflat.Workers.VideoDownloadWorker
 
   @doc """
-  Starts tasks for indexing a source's media.
+  Starts tasks for indexing a source's media regardless of the source's indexing
+  frequency. It's assumed the caller will check for that.
 
-  Returns {:ok, :should_not_index} | {:ok, %Task{}}.
+  Returns {:ok, %Task{}}.
   """
   def kickoff_indexing_task(%Source{} = source) do
     Tasks.delete_pending_tasks_for(source)
 
-    if source.index_frequency_minutes <= 0 do
-      {:ok, :should_not_index}
-    else
-      source
-      |> Map.take([:id])
-      # Schedule this one immediately, but future ones will be on an interval
-      |> MediaIndexingWorker.new()
-      |> Tasks.create_job_with_task(source)
-      |> case do
-        # This should never return {:error, :duplicate_job} since we just deleted
-        # any pending tasks. I'm being assertive about it so it's obvious if I'm wrong
-        {:ok, task} -> {:ok, task}
-      end
+    source
+    |> Map.take([:id])
+    # Schedule this one immediately, but future ones will be on an interval
+    |> MediaIndexingWorker.new()
+    |> Tasks.create_job_with_task(source)
+    |> case do
+      # This should never return {:error, :duplicate_job} since we just deleted
+      # any pending tasks. I'm being assertive about it so it's obvious if I'm wrong
+      {:ok, task} -> {:ok, task}
     end
   end
 

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

@@ -24,7 +24,7 @@
     options={friendly_index_frequencies()}
     type="select"
     label="Index Frequency"
-    help="The time between one index of this source finishing and the next one starting"
+    help="Time between one index of this source finishing and the next one starting. Setting to 'Never' will still run an initial index but no subsequent ones"
   />
 
   <.input

test/pinchflat/sources_test.exs

@@ -159,6 +159,20 @@ defmodule Pinchflat.SourcesTest do
 
       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",
+        index_frequency_minutes: 0
+      }
+
+      assert {:ok, %Source{} = source} = Sources.create_source(valid_attrs)
+
+      assert_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
+    end
   end
 
   describe "update_source/2" do
@@ -201,7 +215,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}
 
@@ -210,11 +224,33 @@ 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()
+      task = task_fixture(source_id: source.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
 

test/pinchflat/tasks/source_tasks_test.exs

@@ -16,15 +16,7 @@ defmodule Pinchflat.Tasks.SourceTasksTest do
   setup :verify_on_exit!
 
   describe "kickoff_indexing_task/1" do
-    test "it does not schedule a job if the interval is <= 0" do
-      source = source_fixture(index_frequency_minutes: -1)
-
-      assert {:ok, :should_not_index} = SourceTasks.kickoff_indexing_task(source)
-
-      refute_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
-    end
-
-    test "it schedules a job if the interval is > 0" do
+    test "it schedules a job" do
       source = source_fixture(index_frequency_minutes: 1)
 
       assert {:ok, _} = SourceTasks.kickoff_indexing_task(source)
@@ -32,7 +24,7 @@ defmodule Pinchflat.Tasks.SourceTasksTest do
       assert_enqueued(worker: MediaIndexingWorker, args: %{"id" => source.id})
     end
 
-    test "it creates and attaches a task if the interval is > 0" do
+    test "it creates and attaches a task" do
       source = source_fixture(index_frequency_minutes: 1)
 
       assert {:ok, %Task{} = task} = SourceTasks.kickoff_indexing_task(source)