Docs, etc

lib/pinchflat/downloading/media_download_worker.ex

@@ -39,7 +39,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
     - `quality_upgrade?`: 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}
+  Returns :ok | {:error, any, ...any}
   """
   @impl Oban.Worker
   def perform(%Oban.Job{args: %{"id" => media_item_id} = args}) do
@@ -59,12 +59,14 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
     Ecto.StaleEntryError -> Logger.info("#{__MODULE__} discarded: media item #{media_item_id} stale")
   end
 
+  # If a user script exists and, when run, returns a non-zero exit code, prevent this and all future downloads
+  # of the media item.
   defp fetch_and_run_prevent_download_user_script(media_item_id) do
     media_item = Media.get_media_item!(media_item_id)
 
     {:ok, media_item} =
       case run_user_script(:media_pre_download, media_item) do
-        {:ok, _, exit_code} when exit_code > 0 -> Media.update_media_item(media_item, %{prevent_download: true})
+        {:ok, _, exit_code} when exit_code != 0 -> Media.update_media_item(media_item, %{prevent_download: true})
         _ -> {:ok, media_item}
       end
 
@@ -83,9 +85,9 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
             media_redownloaded_at: get_redownloaded_at(is_quality_upgrade)
           })
 
-        :ok = run_user_script(:media_downloaded, updated_media_item)
+        run_user_script(:media_downloaded, updated_media_item)
 
-        {:ok, updated_media_item}
+        :ok
 
       {:recovered, _} ->
         {:error, :retry}

lib/pinchflat/lifecycle/user_scripts/command_runner.ex

@@ -23,13 +23,14 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do
 
   This function will succeed in almost all cases, even if the user script command
   failed - this is because I don't want bad scripts to stop the whole process.
-  If something fails, it'll be logged.
+  If something fails, it'll be logged and returned BUT the tuple will always
+  start with {:ok, ...}.
 
   The only things that can cause a true failure are passing in an invalid event
   type or if the passed data cannot be encoded into JSON - both indicative of
   failures in the development process.
 
-  Returns :ok
+  Returns {:ok, :no_executable} | {:ok, output, exit_code}
   """
   @impl UserScriptCommandRunner
   def run(event_type, encodable_data) when event_type in @event_types do
@@ -63,7 +64,7 @@ defmodule Pinchflat.Lifecycle.UserScripts.CommandRunner do
     if FilesystemUtils.exists_and_nonempty?(filepath) do
       {:ok, filepath}
     else
-      Logger.warning("User scripts lifecyle file either not present or is empty. Skipping.")
+      Logger.info("User scripts lifecyle file either not present or is empty. Skipping.")
 
       {:ok, :no_executable}
     end

test/pinchflat/downloading/media_download_worker_test.exs

@@ -162,20 +162,6 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
       assert media_item.media_redownloaded_at == nil
     end
 
-    test "calls the user script runner", %{media_item: media_item} do
-      expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl ->
-        {:ok, render_metadata(:media_metadata)}
-      end)
-
-      expect(UserScriptRunnerMock, :run, fn :media_downloaded, data ->
-        assert data.id == media_item.id
-
-        {:ok, "", 0}
-      end)
-
-      perform_job(MediaDownloadWorker, %{id: media_item.id})
-    end
-
     test "does not blow up if the record doesn't exist" do
       assert :ok = perform_job(MediaDownloadWorker, %{id: 0})
     end
@@ -237,4 +223,59 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
       perform_job(MediaDownloadWorker, %{id: media_item.id, force: true})
     end
   end
+
+  describe "perform/1 when testing user script callbacks" do
+    setup do
+      stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl ->
+        {:ok, render_metadata(:media_metadata)}
+      end)
+
+      :ok
+    end
+
+    test "calls the media_pre_download user script runner", %{media_item: media_item} do
+      expect(UserScriptRunnerMock, :run, fn :media_pre_download, data ->
+        assert data.id == media_item.id
+
+        {:ok, "", 0}
+      end)
+
+      expect(UserScriptRunnerMock, :run, fn :media_downloaded, _ -> {:ok, "", 0} end)
+
+      perform_job(MediaDownloadWorker, %{id: media_item.id})
+    end
+
+    test "does not download the media if the pre-download script returns an error", %{media_item: media_item} do
+      expect(UserScriptRunnerMock, :run, fn :media_pre_download, _ -> {:ok, "", 1} end)
+
+      assert :ok = perform_job(MediaDownloadWorker, %{id: media_item.id})
+      media_item = Repo.reload!(media_item)
+
+      refute media_item.media_filepath
+      assert media_item.prevent_download
+    end
+
+    test "downloads media if the pre-download script is not present", %{media_item: media_item} do
+      expect(UserScriptRunnerMock, :run, fn :media_pre_download, _ -> {:ok, :no_executable} end)
+      expect(UserScriptRunnerMock, :run, fn :media_downloaded, _ -> {:ok, :no_executable} end)
+
+      assert :ok = perform_job(MediaDownloadWorker, %{id: media_item.id})
+      media_item = Repo.reload!(media_item)
+
+      assert media_item.media_filepath
+      refute media_item.prevent_download
+    end
+
+    test "calls the media_downloaded user script runner", %{media_item: media_item} do
+      expect(UserScriptRunnerMock, :run, fn :media_pre_download, _ -> {:ok, "", 0} end)
+
+      expect(UserScriptRunnerMock, :run, fn :media_downloaded, data ->
+        assert data.id == media_item.id
+
+        {:ok, "", 0}
+      end)
+
+      perform_job(MediaDownloadWorker, %{id: media_item.id})
+    end
+  end
 end