From 471aec27b633a1edce4501e0599111c022091331 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Wed, 10 Apr 2024 08:34:07 -0700 Subject: [PATCH] Filled out redownload worker + tests --- .../downloading/media_download_worker.ex | 28 ++++++--- .../downloading/media_redownload_worker.ex | 14 ++++- lib/pinchflat/media/media.ex | 10 +++- lib/pinchflat/media/media_query.ex | 29 +++++---- .../media_download_worker_test.exs | 26 +++++++- .../media_redownload_worker_test.exs | 44 ++++++++++++++ test/pinchflat/media_test.exs | 60 +++++++++++++++++-- 7 files changed, 182 insertions(+), 29 deletions(-) create mode 100644 test/pinchflat/downloading/media_redownload_worker_test.exs diff --git a/lib/pinchflat/downloading/media_download_worker.ex b/lib/pinchflat/downloading/media_download_worker.ex index 17ab5b5..0e12067 100644 --- a/lib/pinchflat/downloading/media_download_worker.ex +++ b/lib/pinchflat/downloading/media_download_worker.ex @@ -35,14 +35,17 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do """ @impl Oban.Worker def perform(%Oban.Job{args: %{"id" => media_item_id} = args}) do + should_force = Map.get(args, "force", false) + is_redownload = Map.get(args, "redownload?", false) + media_item = media_item_id |> Media.get_media_item!() |> Repo.preload(:source) # If the source or media item is set to not download media, perform a no-op unless forced - if (media_item.source.download_media && !media_item.prevent_download) || args["force"] do - download_media_and_schedule_jobs(media_item) + if (media_item.source.download_media && !media_item.prevent_download) || should_force do + download_media_and_schedule_jobs(media_item, is_redownload) else :ok end @@ -51,10 +54,13 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do Ecto.StaleEntryError -> Logger.info("#{__MODULE__} discarded: media item #{media_item_id} stale") end - defp download_media_and_schedule_jobs(media_item) do + defp download_media_and_schedule_jobs(media_item, is_redownload) do case MediaDownloader.download_for_media_item(media_item) do {:ok, updated_media_item} -> - compute_and_save_media_filesize(updated_media_item) + Media.update_media_item(updated_media_item, %{ + media_size_bytes: compute_media_filesize(updated_media_item), + media_redownloaded_at: get_redownloaded_at(is_redownload) + }) {:ok, updated_media_item} @@ -66,13 +72,21 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do end end - defp compute_and_save_media_filesize(media_item) do + defp compute_media_filesize(media_item) do case File.stat(media_item.media_filepath) do {:ok, %{size: size}} -> - Media.update_media_item(media_item, %{media_size_bytes: size}) + size _ -> - :ok + nil + end + end + + defp get_redownloaded_at(is_redownload) do + if is_redownload do + DateTime.utc_now() + else + nil end end end diff --git a/lib/pinchflat/downloading/media_redownload_worker.ex b/lib/pinchflat/downloading/media_redownload_worker.ex index 4b38c58..b3b6d82 100644 --- a/lib/pinchflat/downloading/media_redownload_worker.ex +++ b/lib/pinchflat/downloading/media_redownload_worker.ex @@ -9,11 +9,23 @@ defmodule Pinchflat.Downloading.MediaRedownloadWorker do require Logger alias Pinchflat.Media + alias Pinchflat.Downloading.MediaDownloadWorker @doc """ + Redownloads media items that are eligible for redownload. + + This worker is scheduled to run daily via the Oban Cron plugin + and it should run _after_ the retention worker. + + Returns :ok """ - # TODO @impl Oban.Worker def perform(%Oban.Job{}) do + redownloadable_media = Media.list_redownloadable_media_items() + Logger.info("Redownloading #{length(redownloadable_media)} media items") + + Enum.each(redownloadable_media, fn media_item -> + MediaDownloadWorker.kickoff_with_task(media_item, %{redownload?: true}) + end) end end diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index a2c24e9..d458af4 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -40,11 +40,19 @@ defmodule Pinchflat.Media do Returns a list of media_items that are redownloadable based on the redownload delay of the media_profile their source belongs to. + The logic is that a media_item is past_redownload_delay if the media_item's + upload_date is at least redownload_delay_days ago AND + `media_downloaded_at` - `redownload_delay_days` is before the media_item's `upload_date`. + This logic grabs media that we've recently downloaded AND is recently uploaded, but + doesn't grab media that we've recently downloaded and was uploaded a long time ago. + This also makes things work as expected when downloading media from a source for the + first time. + Returns [%MediaItem{}, ...] """ def list_redownloadable_media_items do MediaQuery.new() - |> MediaQuery.with_media_filepath() + |> MediaQuery.with_media_downloaded_at() |> MediaQuery.where_download_not_prevented() |> MediaQuery.where_not_culled() |> MediaQuery.where_media_not_redownloaded() diff --git a/lib/pinchflat/media/media_query.ex b/lib/pinchflat/media/media_query.ex index d187aec..024f745 100644 --- a/lib/pinchflat/media/media_query.ex +++ b/lib/pinchflat/media/media_query.ex @@ -38,12 +38,10 @@ defmodule Pinchflat.Media.MediaQuery do |> require_assoc(:source) |> where( [mi, source], - fragment( - "IFNULL(?, 0) > 0 AND DATETIME('now', '-' || ? || ' day') > ?", - source.retention_period_days, - source.retention_period_days, - mi.media_downloaded_at - ) + fragment(""" + IFNULL(retention_period_days, 0) > 0 AND + DATETIME('now', '-' || retention_period_days || ' day') > media_downloaded_at + """) ) end @@ -52,13 +50,14 @@ defmodule Pinchflat.Media.MediaQuery do |> require_assoc(:source) |> require_assoc(:media_profile) |> where( - [mi, source, media_profile], - fragment( - "IFNULL(?, 0) > 0 AND DATETIME('now', '-' || ? || ' day') > ?", - media_profile.redownload_delay_days, - media_profile.redownload_delay_days, - mi.upload_date - ) + [_mi, _source, _media_profile], + # Returns media items where the upload_date is at least redownload_delay_days ago AND + # downloaded_at minus the redownload_delay_days is before the upload date + fragment(""" + IFNULL(redownload_delay_days, 0) > 0 AND + DATETIME('now', '-' || redownload_delay_days || ' day') > upload_date AND + DATETIME(media_downloaded_at, '-' || redownload_delay_days || ' day') < upload_date + """) ) end @@ -82,6 +81,10 @@ defmodule Pinchflat.Media.MediaQuery do where(query, [mi], mi.media_id in ^media_ids) end + def with_media_downloaded_at(query) do + where(query, [mi], not is_nil(mi.media_downloaded_at)) + end + def with_media_filepath(query) do where(query, [mi], not is_nil(mi.media_filepath)) end diff --git a/test/pinchflat/downloading/media_download_worker_test.exs b/test/pinchflat/downloading/media_download_worker_test.exs index b227c31..4f3b72e 100644 --- a/test/pinchflat/downloading/media_download_worker_test.exs +++ b/test/pinchflat/downloading/media_download_worker_test.exs @@ -62,7 +62,9 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do assert media_item.media_filepath == nil perform_job(MediaDownloadWorker, %{id: media_item.id}) - assert Repo.reload(media_item).media_filepath != nil + media_item = Repo.reload(media_item) + + assert media_item.media_filepath != nil end test "it saves the metadata to the media_item", %{media_item: media_item} do @@ -149,6 +151,28 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do assert media_item.media_size_bytes > 0 end + test "saves redownloaded_at if this is for a redownload", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> + {:ok, render_metadata(:media_metadata)} + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id, redownload?: true}) + media_item = Repo.reload(media_item) + + assert media_item.media_redownloaded_at != nil + end + + test "doesn't save redownloaded_at if this is not for a redownload", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> + {:ok, render_metadata(:media_metadata)} + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id}) + media_item = Repo.reload(media_item) + + assert media_item.media_redownloaded_at == nil + end + test "does not blow up if the record doesn't exist" do assert :ok = perform_job(MediaDownloadWorker, %{id: 0}) end diff --git a/test/pinchflat/downloading/media_redownload_worker_test.exs b/test/pinchflat/downloading/media_redownload_worker_test.exs new file mode 100644 index 0000000..70d9cc0 --- /dev/null +++ b/test/pinchflat/downloading/media_redownload_worker_test.exs @@ -0,0 +1,44 @@ +defmodule Pinchflat.Downloading.MediaRedownloadWorkerTest do + use Pinchflat.DataCase + + import Pinchflat.MediaFixtures + import Pinchflat.SourcesFixtures + import Pinchflat.ProfilesFixtures + + alias Pinchflat.Downloading.MediaDownloadWorker + alias Pinchflat.Downloading.MediaRedownloadWorker + + describe "perform/1" do + test "kicks off a task for redownloadable media items" do + media_profile = media_profile_fixture(%{redownload_delay_days: 4}) + source = source_fixture(%{media_profile_id: media_profile.id, inserted_at: now_minus(10, :days)}) + + media_item = + media_item_fixture(%{ + source_id: source.id, + upload_date: now_minus(6, :days), + media_downloaded_at: now_minus(5, :days) + }) + + perform_job(MediaRedownloadWorker, %{}) + + assert [_] = all_enqueued(worker: MediaDownloadWorker, args: %{id: media_item.id, redownload?: true}) + end + + test "does not kickoff a task for non-redownloadable media items" do + media_profile = media_profile_fixture(%{redownload_delay_days: 4}) + source = source_fixture(%{media_profile_id: media_profile.id, inserted_at: now_minus(10, :days)}) + + _media_item = + media_item_fixture(%{ + source_id: source.id, + upload_date: now_minus(6, :days), + media_downloaded_at: now_minus(1, :day) + }) + + perform_job(MediaRedownloadWorker, %{}) + + assert [] = all_enqueued(worker: MediaDownloadWorker) + end + end +end diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index 15d021d..fad36c0 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -133,23 +133,39 @@ defmodule Pinchflat.MediaTest do describe "list_redownloadable_media_items/0" do setup do media_profile = media_profile_fixture(%{redownload_delay_days: 4}) - source = source_fixture(%{media_profile_id: media_profile.id}) + source = source_fixture(%{media_profile_id: media_profile.id, inserted_at: now_minus(10, :days)}) {:ok, %{media_profile: media_profile, source: source}} end test "returns media eligible for redownload", %{source: source} do - media_item = media_item_fixture(%{source_id: source.id, upload_date: now_minus(5, :days)}) + media_item = + media_item_fixture(%{ + source_id: source.id, + upload_date: now_minus(6, :days), + media_downloaded_at: now_minus(5, :days) + }) assert Media.list_redownloadable_media_items() == [media_item] end - test "does not return media items without a media_filepath", %{source: source} do + test "returns media items that were downloaded in past but still meet redownload delay", %{source: source} do + media_item = + media_item_fixture(%{ + source_id: source.id, + upload_date: now_minus(20, :days), + media_downloaded_at: now_minus(19, :days) + }) + + assert Media.list_redownloadable_media_items() == [media_item] + end + + test "does not return media items without a media_downloaded_at", %{source: source} do _media_item = media_item_fixture(%{ source_id: source.id, upload_date: now_minus(5, :days), - media_filepath: nil + media_downloaded_at: nil }) assert Media.list_redownloadable_media_items() == [] @@ -160,6 +176,7 @@ defmodule Pinchflat.MediaTest do media_item_fixture(%{ source_id: source.id, upload_date: now_minus(5, :days), + media_downloaded_at: now(), prevent_download: true }) @@ -171,6 +188,7 @@ defmodule Pinchflat.MediaTest do media_item_fixture(%{ source_id: source.id, upload_date: now_minus(5, :days), + media_downloaded_at: now(), culled_at: now() }) @@ -181,7 +199,8 @@ defmodule Pinchflat.MediaTest do _media_item = media_item_fixture(%{ source_id: source.id, - upload_date: now_minus(3, :days) + upload_date: now_minus(3, :days), + media_downloaded_at: now_minus(3, :days) }) assert Media.list_redownloadable_media_items() == [] @@ -192,16 +211,45 @@ defmodule Pinchflat.MediaTest do media_item_fixture(%{ source_id: source.id, upload_date: now_minus(5, :days), + media_downloaded_at: now(), media_redownloaded_at: now() }) assert Media.list_redownloadable_media_items() == [] end + test "does not return media items that were first downloaded well after the upload_date", %{source: source} do + _media_item = + media_item_fixture(%{ + source_id: source.id, + media_downloaded_at: now(), + upload_date: now_minus(20, :days) + }) + + assert Media.list_redownloadable_media_items() == [] + end + + test "does not return media items that were recently uploaded", %{source: source} do + _media_item = + media_item_fixture(%{ + source_id: source.id, + media_downloaded_at: now(), + upload_date: now_minus(2, :days) + }) + + assert Media.list_redownloadable_media_items() == [] + end + test "does not return media items without a redownload delay" do media_profile = media_profile_fixture(%{redownload_delay_days: nil}) source = source_fixture(%{media_profile_id: media_profile.id}) - _media_item = media_item_fixture(%{source_id: source.id, upload_date: now_minus(5, :days)}) + + _media_item = + media_item_fixture(%{ + source_id: source.id, + upload_date: now_minus(6, :days), + media_downloaded_at: now_minus(5, :days) + }) assert Media.list_redownloadable_media_items() == [] end