Filled out redownload worker + tests

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
+    if (media_item.source.download_media && !media_item.prevent_download) || should_force do
-      download_media_and_schedule_jobs(media_item)
+      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

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

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()

lib/pinchflat/media/media_query.ex

@@ -38,12 +38,10 @@ defmodule Pinchflat.Media.MediaQuery do
     |> require_assoc(:source)
     |> where(
       [mi, source],
-      fragment(
+      fragment("""
-        "IFNULL(?, 0) > 0 AND DATETIME('now', '-' || ? || ' day') > ?",
+      IFNULL(retention_period_days, 0) > 0 AND
-        source.retention_period_days,
+      DATETIME('now', '-' || retention_period_days || ' day') > media_downloaded_at
-        source.retention_period_days,
+      """)
-        mi.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],
+      [_mi, _source, _media_profile],
-      fragment(
+      # Returns media items where the upload_date is at least redownload_delay_days ago AND
-        "IFNULL(?, 0) > 0 AND DATETIME('now', '-' || ? || ' day') > ?",
+      # downloaded_at minus the redownload_delay_days is before the upload date
-        media_profile.redownload_delay_days,
+      fragment("""
-        media_profile.redownload_delay_days,
+        IFNULL(redownload_delay_days, 0) > 0 AND
-        mi.upload_date
+        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

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

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

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