From dc57909c2470c433107f9946f761d9048ad29dbe Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Mon, 13 May 2024 11:21:49 -0700 Subject: [PATCH] Added ability to specify overwrite behaviour when downloading media --- .../downloading/download_option_builder.ex | 11 +-- .../downloading/media_download_worker.ex | 15 +++- lib/pinchflat/downloading/media_downloader.ex | 8 +- .../download_option_builder_test.exs | 7 ++ .../media_download_worker_test.exs | 79 ++++++++++++++----- .../downloading/media_downloader_test.exs | 15 ++++ 6 files changed, 102 insertions(+), 33 deletions(-) diff --git a/lib/pinchflat/downloading/download_option_builder.ex b/lib/pinchflat/downloading/download_option_builder.ex index 5745012..a966e4b 100644 --- a/lib/pinchflat/downloading/download_option_builder.ex +++ b/lib/pinchflat/downloading/download_option_builder.ex @@ -15,11 +15,11 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do Returns {:ok, [Keyword.t()]} """ - def build(%MediaItem{} = media_item_with_preloads) do + def build(%MediaItem{} = media_item_with_preloads, override_opts \\ []) do media_profile = media_item_with_preloads.source.media_profile built_options = - default_options() ++ + default_options(override_opts) ++ subtitle_options(media_profile) ++ thumbnail_options(media_item_with_preloads) ++ metadata_options(media_profile) ++ @@ -50,11 +50,12 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do build_output_path_for(%MediaItem{source: source_with_preloads}) end - defp default_options do + defp default_options(override_opts) do + overwrite_behaviour = Keyword.get(override_opts, :overwrite_behaviour, :force_overwrites) + [ :no_progress, - # Add force-overwrites to make sure redownloading works - :force_overwrites, + overwrite_behaviour, # This makes the date metadata conform to what jellyfin expects parse_metadata: "%(upload_date>%Y-%m-%d)s:(?P.+)" ] diff --git a/lib/pinchflat/downloading/media_download_worker.ex b/lib/pinchflat/downloading/media_download_worker.ex index c211977..aa3e86c 100644 --- a/lib/pinchflat/downloading/media_download_worker.ex +++ b/lib/pinchflat/downloading/media_download_worker.ex @@ -33,6 +33,12 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do Does not download media if its source is set to not download media (unless forced). + Options: + - `force`: force download even if the source is set to not download media. Fully + re-downloads media, including the video + - `redownload?`: re-downloads media, including the video. Does not force download + if the source is set to not download media + Returns :ok | {:ok, %MediaItem{}} | {:error, any, ...any} """ @impl Oban.Worker @@ -47,7 +53,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do # 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) || should_force do - download_media_and_schedule_jobs(media_item, is_redownload) + download_media_and_schedule_jobs(media_item, is_redownload, should_force) else :ok end @@ -56,8 +62,11 @@ 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, is_redownload) do - case MediaDownloader.download_for_media_item(media_item) do + defp download_media_and_schedule_jobs(media_item, is_redownload, should_force) do + overwrite_behaviour = if should_force || is_redownload, do: :force_overwrites, else: :no_force_overwrites + override_opts = [overwrite_behaviour: overwrite_behaviour] + + case MediaDownloader.download_for_media_item(media_item, override_opts) do {:ok, downloaded_media_item} -> {:ok, updated_media_item} = Media.update_media_item(downloaded_media_item, %{ diff --git a/lib/pinchflat/downloading/media_downloader.ex b/lib/pinchflat/downloading/media_downloader.ex index b1fb91a..7d4ab06 100644 --- a/lib/pinchflat/downloading/media_downloader.ex +++ b/lib/pinchflat/downloading/media_downloader.ex @@ -29,11 +29,11 @@ defmodule Pinchflat.Downloading.MediaDownloader do Returns {:ok, %MediaItem{}} | {:error, any, ...any} """ - def download_for_media_item(%MediaItem{} = media_item) do + def download_for_media_item(%MediaItem{} = media_item, override_opts \\ []) do output_filepath = FilesystemUtils.generate_metadata_tmpfile(:json) media_with_preloads = Repo.preload(media_item, [:metadata, source: :media_profile]) - case download_with_options(media_item.original_url, media_with_preloads, output_filepath) do + case download_with_options(media_item.original_url, media_with_preloads, output_filepath, override_opts) do {:ok, parsed_json} -> update_media_item_from_parsed_json(media_with_preloads, parsed_json) @@ -103,8 +103,8 @@ defmodule Pinchflat.Downloading.MediaDownloader do end end - defp download_with_options(url, item_with_preloads, output_filepath) do - {:ok, options} = DownloadOptionBuilder.build(item_with_preloads) + defp download_with_options(url, item_with_preloads, output_filepath, override_opts) do + {:ok, options} = DownloadOptionBuilder.build(item_with_preloads, override_opts) YtDlpMedia.download(url, options, output_filepath: output_filepath) end diff --git a/test/pinchflat/downloading/download_option_builder_test.exs b/test/pinchflat/downloading/download_option_builder_test.exs index c0b941a..e356ec8 100644 --- a/test/pinchflat/downloading/download_option_builder_test.exs +++ b/test/pinchflat/downloading/download_option_builder_test.exs @@ -65,6 +65,13 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do assert :force_overwrites in res assert {:parse_metadata, "%(upload_date>%Y-%m-%d)s:(?P.+)"} in res end + + test "includes override options if specified", %{media_item: media_item} do + assert {:ok, res} = DownloadOptionBuilder.build(media_item, overwrite_behaviour: :no_force_overwrites) + + refute :force_overwrites in res + assert :no_force_overwrites in res + end end describe "build/1 when testing subtitle options" do diff --git a/test/pinchflat/downloading/media_download_worker_test.exs b/test/pinchflat/downloading/media_download_worker_test.exs index ea055ce..6afb574 100644 --- a/test/pinchflat/downloading/media_download_worker_test.exs +++ b/test/pinchflat/downloading/media_download_worker_test.exs @@ -136,15 +136,6 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do perform_job(MediaDownloadWorker, %{id: media_item.id}) end - test "downloads anyway if forced", %{media_item: media_item} do - expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> :ok end) - - Sources.update_source(media_item.source, %{download_media: false}) - Media.update_media_item(media_item, %{prevent_download: true}) - - perform_job(MediaDownloadWorker, %{id: media_item.id, force: true}) - end - test "it saves the file's size to the database", %{media_item: media_item} do expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> metadata = render_parsed_metadata(:media_metadata) @@ -159,18 +150,7 @@ 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 + test "does not set redownloaded_at by default", %{media_item: media_item} do expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> {:ok, render_metadata(:media_metadata)} end) @@ -198,5 +178,62 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do test "does not blow up if the record doesn't exist" do assert :ok = perform_job(MediaDownloadWorker, %{id: 0}) end + + test "sets the no_force_overwrites runner option", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, opts, _ot, _addl -> + assert :no_force_overwrites in opts + refute :force_overwrites in opts + + {:ok, render_metadata(:media_metadata)} + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id}) + end + end + + describe "perform/1 when testing forced downloads" do + test "ignores 'prevent_download' if forced", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> :ok end) + + Sources.update_source(media_item.source, %{download_media: false}) + Media.update_media_item(media_item, %{prevent_download: true}) + + perform_job(MediaDownloadWorker, %{id: media_item.id, force: true}) + end + + test "sets force_overwrites runner option", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, opts, _ot, _addl -> + assert :force_overwrites in opts + refute :no_force_overwrites in opts + + {:ok, render_metadata(:media_metadata)} + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id, force: true}) + end + end + + describe "perform/1 when testing re-downloads" do + test "sets redownloaded_at on the media_item", %{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 "sets force_overwrites runner option", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, opts, _ot, _addl -> + assert :force_overwrites in opts + refute :no_force_overwrites in opts + + {:ok, render_metadata(:media_metadata)} + end) + + perform_job(MediaDownloadWorker, %{id: media_item.id, force: true}) + end end end diff --git a/test/pinchflat/downloading/media_downloader_test.exs b/test/pinchflat/downloading/media_downloader_test.exs index 387a335..463b7b8 100644 --- a/test/pinchflat/downloading/media_downloader_test.exs +++ b/test/pinchflat/downloading/media_downloader_test.exs @@ -65,6 +65,21 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do end end + describe "download_for_media_item/3 when testing override options" do + test "includes override opts if specified", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn _url, opts, _ot, _addl -> + refute :force_overwrites in opts + assert :no_force_overwrites in opts + + {:ok, render_metadata(:media_metadata)} + end) + + override_opts = [overwrite_behaviour: :no_force_overwrites] + + assert {:ok, _} = MediaDownloader.download_for_media_item(media_item, override_opts) + end + end + describe "download_for_media_item/3 when testing retries" do test "returns a recovered tuple on recoverable errors", %{media_item: media_item} do message = "Unable to communicate with SponsorBlock"