From f0205c76234afa725441f4bb6fe9aa08470dbe58 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Thu, 14 Mar 2024 16:00:01 -0700 Subject: [PATCH] Updated task listing fns to take a record directly --- lib/pinchflat/tasks/tasks.ex | 41 ++++++++---------- lib/pinchflat/yt_dlp/command_runner.ex | 3 +- .../controllers/sources/source_controller.ex | 2 +- .../downloading/downloading_helpers_test.exs | 8 ++-- .../fast_indexing_helpers_test.exs | 2 +- .../media_collection_indexing_worker_test.exs | 2 +- .../slow_indexing_helpers_test.exs | 2 +- test/pinchflat/tasks_test.exs | 42 +++++++++++-------- 8 files changed, 52 insertions(+), 50 deletions(-) diff --git a/lib/pinchflat/tasks/tasks.ex b/lib/pinchflat/tasks/tasks.ex index 35f0461..7b94e3c 100644 --- a/lib/pinchflat/tasks/tasks.ex +++ b/lib/pinchflat/tasks/tasks.ex @@ -20,13 +20,17 @@ defmodule Pinchflat.Tasks do Returns the list of tasks for a given record type and ID. Optionally allows you to specify which worker or job states to include. - IDEA: this should be updated to take a struct instead of a record type and ID - Returns [%Task{}, ...] """ - def list_tasks_for(attached_record_type, attached_record_id, worker_name \\ nil, job_states \\ Oban.Job.states()) do + def list_tasks_for(record, worker_name \\ nil, job_states \\ Oban.Job.states()) do stringified_states = Enum.map(job_states, &to_string/1) + record_type = + case record do + %Source{} -> :source_id + %MediaItem{} -> :media_item_id + end + worker_name_finder = if worker_name do # Workers are the full module name - we want to match on the string ENDING with @@ -43,7 +47,7 @@ defmodule Pinchflat.Tasks do Repo.all( from t in Task, join: j in assoc(t, :job), - where: field(t, ^attached_record_type) == ^attached_record_id, + where: field(t, ^record_type) == ^record.id, where: ^worker_name_finder, where: j.state in ^stringified_states ) @@ -55,10 +59,9 @@ defmodule Pinchflat.Tasks do Returns [%Task{}, ...] """ - def list_pending_tasks_for(attached_record_type, attached_record_id, worker_name \\ nil) do + def list_pending_tasks_for(record, worker_name \\ nil) do list_tasks_for( - attached_record_type, - attached_record_id, + record, worker_name, [:available, :scheduled, :retryable] ) @@ -128,14 +131,10 @@ defmodule Pinchflat.Tasks do Returns :ok """ - def delete_tasks_for(attached_record, worker_name \\ nil) do - tasks = - case attached_record do - %Source{} = source -> list_tasks_for(:source_id, source.id, worker_name) - %MediaItem{} = media_item -> list_tasks_for(:media_item_id, media_item.id, worker_name) - end - - Enum.each(tasks, &delete_task/1) + def delete_tasks_for(record, worker_name \\ nil) do + record + |> list_tasks_for(worker_name) + |> Enum.each(&delete_task/1) end @doc """ @@ -144,14 +143,10 @@ defmodule Pinchflat.Tasks do Returns :ok """ - def delete_pending_tasks_for(attached_record, worker_name \\ nil) do - tasks = - case attached_record do - %Source{} = source -> list_pending_tasks_for(:source_id, source.id, worker_name) - %MediaItem{} = media_item -> list_pending_tasks_for(:media_item_id, media_item.id, worker_name) - end - - Enum.each(tasks, &delete_task/1) + def delete_pending_tasks_for(record, worker_name \\ nil) do + record + |> list_pending_tasks_for(worker_name) + |> Enum.each(&delete_task/1) end @doc """ diff --git a/lib/pinchflat/yt_dlp/command_runner.ex b/lib/pinchflat/yt_dlp/command_runner.ex index 925e5be..3e36e74 100644 --- a/lib/pinchflat/yt_dlp/command_runner.ex +++ b/lib/pinchflat/yt_dlp/command_runner.ex @@ -37,7 +37,8 @@ defmodule Pinchflat.YtDlp.CommandRunner do case System.cmd(command, formatted_command_opts, stderr_to_stdout: true) do {_, 0} -> - # IDEA: consider deleting the file after reading it + # IDEA: consider deleting the file after reading it. It's in the tmp dir, so it's not + # a huge deal, but it's still a good idea to clean up after ourselves. # (even on error? especially on error?) File.read(output_filepath) diff --git a/lib/pinchflat_web/controllers/sources/source_controller.ex b/lib/pinchflat_web/controllers/sources/source_controller.ex index 60039f4..3211ccc 100644 --- a/lib/pinchflat_web/controllers/sources/source_controller.ex +++ b/lib/pinchflat_web/controllers/sources/source_controller.ex @@ -46,7 +46,7 @@ defmodule PinchflatWeb.Sources.SourceController do def show(conn, %{"id" => id}) do source = Repo.preload(Sources.get_source!(id), :media_profile) - pending_tasks = Repo.preload(Tasks.list_pending_tasks_for(:source_id, source.id), :job) + pending_tasks = Repo.preload(Tasks.list_pending_tasks_for(source), :job) pending_media = Media.list_pending_media_items_for(source, limit: 100) downloaded_media = Media.list_downloaded_media_items_for(source, limit: 100) diff --git a/test/pinchflat/downloading/downloading_helpers_test.exs b/test/pinchflat/downloading/downloading_helpers_test.exs index f2af81f..b54a5af 100644 --- a/test/pinchflat/downloading/downloading_helpers_test.exs +++ b/test/pinchflat/downloading/downloading_helpers_test.exs @@ -34,11 +34,11 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do source = source_fixture() media_item = media_item_fixture(source_id: source.id, media_filepath: nil) - assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id) + assert [] = Tasks.list_tasks_for(media_item) assert :ok = DownloadingHelpers.enqueue_pending_download_tasks(source) - assert [_] = Tasks.list_tasks_for(:media_item_id, media_item.id) + assert [_] = Tasks.list_tasks_for(media_item) end test "it does not create a job if the source is set to not download" do @@ -54,7 +54,7 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do media_item = media_item_fixture(source_id: source.id, media_filepath: nil) assert :ok = DownloadingHelpers.enqueue_pending_download_tasks(source) - assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id) + assert [] = Tasks.list_tasks_for(media_item) end end @@ -69,7 +69,7 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do assert :ok = DownloadingHelpers.dequeue_pending_download_tasks(source) refute_enqueued(worker: MediaDownloadWorker) - assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id) + assert [] = Tasks.list_tasks_for(media_item) end end end diff --git a/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs b/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs index 6aa078f..0f0e12d 100644 --- a/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs +++ b/test/pinchflat/fast_indexing/fast_indexing_helpers_test.exs @@ -104,7 +104,7 @@ defmodule Pinchflat.FastIndexing.FastIndexingHelpersTest do test "creates a download task record", %{source: source} do assert {:ok, media_item} = FastIndexingHelpers.index_and_enqueue_download_for_media_item(source, @media_url) - assert [_] = Tasks.list_tasks_for(:media_item_id, media_item.id, "MediaDownloadWorker") + assert [_] = Tasks.list_tasks_for(media_item, "MediaDownloadWorker") end test "does not enqueue a download job if the source does not allow it" do diff --git a/test/pinchflat/slow_indexing/media_collection_indexing_worker_test.exs b/test/pinchflat/slow_indexing/media_collection_indexing_worker_test.exs index 3f91de7..5cc07df 100644 --- a/test/pinchflat/slow_indexing/media_collection_indexing_worker_test.exs +++ b/test/pinchflat/slow_indexing/media_collection_indexing_worker_test.exs @@ -103,7 +103,7 @@ defmodule Pinchflat.SlowIndexing.MediaCollectionIndexingWorkerTest do source = source_fixture(index_frequency_minutes: 10) task_count_fetcher = fn -> - Enum.count(Tasks.list_tasks_for(:source_id, source.id, "MediaCollectionIndexingWorker")) + Enum.count(Tasks.list_tasks_for(source, "MediaCollectionIndexingWorker")) end assert_changed([from: 0, to: 1], task_count_fetcher, fn -> diff --git a/test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs b/test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs index 754f331..8165c5b 100644 --- a/test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs +++ b/test/pinchflat/slow_indexing/slow_indexing_helpers_test.exs @@ -149,7 +149,7 @@ defmodule Pinchflat.SlowIndexing.SlowIndexingHelpersTest do SlowIndexingHelpers.index_and_enqueue_download_for_media_items(source) - assert [] = Tasks.list_tasks_for(:media_item_id, media_item.id) + assert [] = Tasks.list_tasks_for(media_item) end test "it doesn't blow up if a media item cannot be coerced into a struct", %{source: source} do diff --git a/test/pinchflat/tasks_test.exs b/test/pinchflat/tasks_test.exs index d780280..36140d5 100644 --- a/test/pinchflat/tasks_test.exs +++ b/test/pinchflat/tasks_test.exs @@ -36,53 +36,59 @@ defmodule Pinchflat.TasksTest do end end - describe "list_tasks_for/4" do + describe "list_tasks_for/3" do test "it lets you specify which record type/ID to join on" do - task = task_fixture() + source = source_fixture() + task = task_fixture(source_id: source.id) - assert Tasks.list_tasks_for(:source_id, task.source_id) == [task] + assert Tasks.list_tasks_for(source, nil, [:available]) == [task] end test "it lets you specify which job states to include" do - task = task_fixture() + source = source_fixture() + task = task_fixture(source_id: source.id) - assert Tasks.list_tasks_for(:source_id, task.source_id, nil, [:available]) == [task] - assert Tasks.list_tasks_for(:source_id, task.source_id, nil, [:cancelled]) == [] + assert Tasks.list_tasks_for(source, nil, [:available]) == [task] + assert Tasks.list_tasks_for(source, nil, [:cancelled]) == [] end test "it lets you specify which worker to include" do - task = task_fixture() + source = source_fixture() + task = task_fixture(source_id: source.id) - assert Tasks.list_tasks_for(:source_id, task.source_id, "TestJobWorker") == [task] - assert Tasks.list_tasks_for(:source_id, task.source_id, "FooBarWorker") == [] + assert Tasks.list_tasks_for(source, "TestJobWorker") == [task] + assert Tasks.list_tasks_for(source, "FooBarWorker") == [] end test "it includes all workers if no worker is specified" do - task = task_fixture() + source = source_fixture() + task = task_fixture(source_id: source.id) - assert Tasks.list_tasks_for(:source_id, task.source_id, nil) == [task] + assert Tasks.list_tasks_for(source, nil) == [task] end end describe "list_pending_tasks_for/3" do test "it lists pending tasks" do - task = task_fixture() + source = source_fixture() + task = task_fixture(source_id: source.id) - assert Tasks.list_pending_tasks_for(:source_id, task.source_id) == [task] + assert Tasks.list_pending_tasks_for(source) == [task] end test "it does not list non-pending tasks" do - task = Repo.preload(task_fixture(), :job) + task = Repo.preload(task_fixture(), [:job, :source]) :ok = Oban.cancel_job(task.job) - assert Tasks.list_pending_tasks_for(:source_id, task.source_id) == [] + assert Tasks.list_pending_tasks_for(task.source) == [] end test "it lets you specify which worker to include" do - task = task_fixture() + source = source_fixture() + task = task_fixture(source_id: source.id) - assert Tasks.list_pending_tasks_for(:source_id, task.source_id, "TestJobWorker") == [task] - assert Tasks.list_pending_tasks_for(:source_id, task.source_id, "FooBarWorker") == [] + assert Tasks.list_pending_tasks_for(source, "TestJobWorker") == [task] + assert Tasks.list_pending_tasks_for(source, "FooBarWorker") == [] end end