[Enhancement] Allow redownloading of files for existing media items (#239)

* Added ability to specify overwrite behaviour when downloading media

* Added helper for redownloading media items

* renamed media redownload worker to disambiguate it from similarly named methods

* Added new redownload option to source actions dropdown

* Refactored MediaQuery to use a __using__ macro

* docs

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<meta_date>.+)"} 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

test/pinchflat/downloading/downloading_helpers_test.exs

@@ -110,4 +110,32 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do
       refute_enqueued(worker: MediaDownloadWorker)
     end
   end
+
+  describe "kickoff_redownload_for_existing_media/1" do
+    test "enqueues a download job for each downloaded media item" do
+      source = source_fixture()
+      media_item = media_item_fixture(source_id: source.id, media_downloaded_at: now())
+
+      assert [{:ok, _}] = DownloadingHelpers.kickoff_redownload_for_existing_media(source)
+
+      assert_enqueued(worker: MediaDownloadWorker, args: %{"id" => media_item.id})
+    end
+
+    test "doesn't enqueue jobs for media that should be ignored" do
+      source = source_fixture()
+      other_source = source_fixture()
+      _not_downloaded = media_item_fixture(source_id: source.id, media_downloaded_at: nil)
+      _other_source = media_item_fixture(source_id: other_source.id, media_downloaded_at: now())
+
+      _download_prevented =
+        media_item_fixture(source_id: source.id, media_downloaded_at: now(), prevent_download: true)
+
+      _culled =
+        media_item_fixture(source_id: source.id, media_downloaded_at: now(), culled_at: now())
+
+      assert [] = DownloadingHelpers.kickoff_redownload_for_existing_media(source)
+
+      refute_enqueued(worker: MediaDownloadWorker)
+    end
+  end
 end

test/pinchflat/downloading/media_download_worker_test.exs

@@ -108,7 +108,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
       end)
 
       Oban.Testing.with_testing_mode(:inline, fn ->
-        {:ok, job} = Oban.insert(MediaDownloadWorker.new(%{id: media_item.id, redownload?: true}))
+        {:ok, job} = Oban.insert(MediaDownloadWorker.new(%{id: media_item.id, quality_upgrade?: true}))
 
         assert job.state == "completed"
       end)
@@ -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, quality_upgrade?: 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

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"

test/pinchflat/downloading/media_quality_upgrade_worker_test.exs

@@ -1,4 +1,4 @@
-defmodule Pinchflat.Downloading.MediaRedownloadWorkerTest do
+defmodule Pinchflat.Downloading.MediaQualityUpgradeWorkerTest do
   use Pinchflat.DataCase
 
   import Pinchflat.MediaFixtures
@@ -6,7 +6,7 @@ defmodule Pinchflat.Downloading.MediaRedownloadWorkerTest do
   import Pinchflat.ProfilesFixtures
 
   alias Pinchflat.Downloading.MediaDownloadWorker
-  alias Pinchflat.Downloading.MediaRedownloadWorker
+  alias Pinchflat.Downloading.MediaQualityUpgradeWorker
 
   describe "perform/1" do
     test "kicks off a task for redownloadable media items" do
@@ -20,9 +20,9 @@ defmodule Pinchflat.Downloading.MediaRedownloadWorkerTest do
           media_downloaded_at: now_minus(5, :days)
         })
 
-      perform_job(MediaRedownloadWorker, %{})
+      perform_job(MediaQualityUpgradeWorker, %{})
 
-      assert [_] = all_enqueued(worker: MediaDownloadWorker, args: %{id: media_item.id, redownload?: true})
+      assert [_] = all_enqueued(worker: MediaDownloadWorker, args: %{id: media_item.id, quality_upgrade?: true})
     end
 
     test "does not kickoff a task for non-redownloadable media items" do
@@ -36,7 +36,7 @@ defmodule Pinchflat.Downloading.MediaRedownloadWorkerTest do
           media_downloaded_at: now_minus(1, :day)
         })
 
-      perform_job(MediaRedownloadWorker, %{})
+      perform_job(MediaQualityUpgradeWorker, %{})
 
       assert [] = all_enqueued(worker: MediaDownloadWorker)
     end

test/pinchflat_web/controllers/source_controller_test.exs

@@ -166,20 +166,38 @@ defmodule PinchflatWeb.SourceControllerTest do
     end
   end
 
-  describe "force_download" do
+  describe "force_download_pending" do
     test "enqueues pending download tasks", %{conn: conn} do
       source = source_fixture()
       _media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil})
 
       assert [] = all_enqueued(worker: MediaDownloadWorker)
-      post(conn, ~p"/sources/#{source.id}/force_download")
+      post(conn, ~p"/sources/#{source.id}/force_download_pending")
       assert [_] = all_enqueued(worker: MediaDownloadWorker)
     end
 
     test "redirects to the source page", %{conn: conn} do
       source = source_fixture()
 
-      conn = post(conn, ~p"/sources/#{source.id}/force_download")
+      conn = post(conn, ~p"/sources/#{source.id}/force_download_pending")
+      assert redirected_to(conn) == ~p"/sources/#{source.id}"
+    end
+  end
+
+  describe "force_redownload" do
+    test "enqueues re-download tasks", %{conn: conn} do
+      source = source_fixture()
+      _media_item = media_item_fixture(source_id: source.id, media_downloaded_at: now())
+
+      assert [] = all_enqueued(worker: MediaDownloadWorker)
+      post(conn, ~p"/sources/#{source.id}/force_redownload")
+      assert [_] = all_enqueued(worker: MediaDownloadWorker)
+    end
+
+    test "redirects to the source page", %{conn: conn} do
+      source = source_fixture()
+
+      conn = post(conn, ~p"/sources/#{source.id}/force_redownload")
       assert redirected_to(conn) == ~p"/sources/#{source.id}"
     end
   end