From 855f2bb1ef27ff19fa0510d10d1e0c2cbabdd42f Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sat, 24 Feb 2024 19:21:00 -0800 Subject: [PATCH] Update source to always run an initial index --- lib/pinchflat/media_source/source.ex | 1 + lib/pinchflat/sources.ex | 14 +++---- lib/pinchflat/tasks/source_tasks.ex | 27 ++++++------- .../sources/source_html/source_form.html.heex | 2 +- test/pinchflat/sources_test.exs | 40 ++++++++++++++++++- test/pinchflat/tasks/source_tasks_test.exs | 12 +----- 6 files changed, 60 insertions(+), 36 deletions(-) diff --git a/lib/pinchflat/media_source/source.ex b/lib/pinchflat/media_source/source.ex index 374c614..1c94d64 100644 --- a/lib/pinchflat/media_source/source.ex +++ b/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 diff --git a/lib/pinchflat/sources.ex b/lib/pinchflat/sources.ex index 3b8d130..8e998a2 100644 --- a/lib/pinchflat/sources.ex +++ b/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 diff --git a/lib/pinchflat/tasks/source_tasks.ex b/lib/pinchflat/tasks/source_tasks.ex index be2d2a0..b62bf3a 100644 --- a/lib/pinchflat/tasks/source_tasks.ex +++ b/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 diff --git a/lib/pinchflat_web/controllers/sources/source_html/source_form.html.heex b/lib/pinchflat_web/controllers/sources/source_html/source_form.html.heex index aa1703d..8e3bbfe 100644 --- a/lib/pinchflat_web/controllers/sources/source_html/source_form.html.heex +++ b/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 diff --git a/test/pinchflat/sources_test.exs b/test/pinchflat/sources_test.exs index 547755b..89c479b 100644 --- a/test/pinchflat/sources_test.exs +++ b/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 diff --git a/test/pinchflat/tasks/source_tasks_test.exs b/test/pinchflat/tasks/source_tasks_test.exs index 35cc7e0..abe38e5 100644 --- a/test/pinchflat/tasks/source_tasks_test.exs +++ b/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)