implemented new culling behaviour for source cutoff dates

lib/pinchflat/downloading/media_retention_worker.ex

@@ -24,19 +24,24 @@ defmodule Pinchflat.Downloading.MediaRetentionWorker do
   @impl Oban.Worker
   def perform(%Oban.Job{}) do
     cull_cullable_media_items()
-    # delete_media_items_from_before_cutoff()
+    delete_media_items_from_before_cutoff()
 
     :ok
   end
 
   defp cull_cullable_media_items do
-    # TODO: consider bringing `list_cullable_media_items` into this module since it's only used here
+    cullable_media =
-    cullable_media = Media.list_cullable_media_items()
+      MediaQuery.new()
+      |> MediaQuery.require_assoc(:source)
+      |> where(^MediaQuery.cullable())
+      |> Repo.all()
+
     Logger.info("Culling #{length(cullable_media)} media items past their retention date")
 
     Enum.each(cullable_media, fn media_item ->
-      # Setting `prevent_download` does what it says on the tin,
+      # Setting `prevent_download` does what it says on the tin, but `culled_at` is purely informational.
-      # TODO: finish these docs
+      # We don't actually do anything with that in terms of queries and it gets set to nil if the media item
+      # gets re-downloaded.
       Media.delete_media_files(media_item, %{
         prevent_download: true,
         culled_at: DateTime.utc_now()
@@ -44,31 +49,28 @@ defmodule Pinchflat.Downloading.MediaRetentionWorker do
     end)
   end
 
-  # TODO: test
   defp delete_media_items_from_before_cutoff do
+    # TODO: update the source form to explain this new behavior
     deletable_media =
       MediaQuery.new()
       |> MediaQuery.require_assoc(:source)
-      |> where(^MediaQuery.deletable_from_source_cutoff())
+      |> where(^MediaQuery.deletable_based_on_source_cutoff())
       |> Repo.all()
 
     Logger.info("Deleting #{length(deletable_media)} media items that are from before the source cutoff")
-    # TODO: Maybe I should be setting `culled_at` here since it does capture what's actually happening.
+    # TODO: I should ensure that `culled_at` is set to nil if the media item
-    # There are also a few spots in code that check for `culled_at` to determine if a media item is
-    # eligible for redownload, for instance. I should re-evalute what `culled_at` actually "means" and,
-    # since culled_at really only gets set for items that we prevent download on OR items that shouldn't
-    # be redownloaded anyway, maybe it should become purely informational rather than functional.
-    #
-    # TODO: depending on the above, maybe I should ensure that `culled_at` is set to nil if the media item
     # gets re-downloaded. Because in this case the user could just change the cutoff date and re-download
     # and I don't think it makes sense to still indicate that the media item was culled.
     Enum.each(deletable_media, fn media_item ->
-      # Note that I'm not setting any attributes like `prevent_download` on the media_item here.
+      # Note that I'm not setting `prevent_download` on the media_item here.
       # That's because cutoff_date can easily change and it's a valid behavior to re-download older
       # media items if the cutoff_date changes.
       # Download is ultimately prevented because `MediaQuery.pending()` only returns media items
-      # from after the cutoff date (among other things).
+      # from after the cutoff date (among other things), so it's not like the media will just immediately
-      Media.delete_media_files(media_item)
+      # be re-downloaded.
+      Media.delete_media_files(media_item, %{
+        culled_at: DateTime.utc_now()
+      })
     end)
   end
 end

lib/pinchflat/media/media.ex

@@ -24,19 +24,6 @@ defmodule Pinchflat.Media do
     Repo.all(MediaItem)
   end
 
-  @doc """
-  Returns a list of media_items that are cullable based on the retention period
-  of the source they belong to.
-
-  Returns [%MediaItem{}, ...]
-  """
-  def list_cullable_media_items do
-    MediaQuery.new()
-    |> MediaQuery.require_assoc(:source)
-    |> where(^MediaQuery.cullable())
-    |> Repo.all()
-  end
-
   @doc """
   Returns a list of media_items that are upgradeable based on the redownload delay
   of the media_profile their source belongs to. In this context, upgradeable means

lib/pinchflat/media/media_query.ex

@@ -33,7 +33,6 @@ defmodule Pinchflat.Media.MediaQuery do
   def downloaded, do: dynamic([mi], not is_nil(mi.media_filepath))
   def download_prevented, do: dynamic([mi], mi.prevent_download == true)
   def culling_prevented, do: dynamic([mi], mi.prevent_culling == true)
-  def culled, do: dynamic([mi], not is_nil(mi.culled_at))
   def redownloaded, do: dynamic([mi], not is_nil(mi.media_redownloaded_at))
   def upload_date_matches(other_date), do: dynamic([mi], fragment("date(?) = date(?)", mi.uploaded_at, ^other_date))
 
@@ -116,7 +115,7 @@ defmodule Pinchflat.Media.MediaQuery do
     )
   end
 
-  def deletable_from_source_cutoff do
+  def deletable_based_on_source_cutoff do
     dynamic(
       [mi, source],
       ^downloaded() and

test/pinchflat/downloading/media_retention_worker_test.exs

@@ -7,15 +7,36 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
   alias Pinchflat.Media
   alias Pinchflat.Downloading.MediaRetentionWorker
 
-  describe "perform/1" do
   setup do
     stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end)
 
     :ok
   end
 
+  describe "perform/1" do
+    test "sets deleted media to not re-download" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_retention_date()
+
+      perform_job(MediaRetentionWorker, %{})
+
+      refute Repo.reload!(new_media_item).prevent_download
+      assert Repo.reload!(old_media_item).prevent_download
+    end
+
+    test "sets culled_at timestamp on deleted media" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_retention_date()
+
+      perform_job(MediaRetentionWorker, %{})
+
+      refute Repo.reload!(new_media_item).culled_at
+      assert Repo.reload!(old_media_item).culled_at
+      assert DateTime.diff(now(), Repo.reload!(old_media_item).culled_at) < 1
+    end
+  end
+
+  describe "perform/1 when testing retention_period-based culling" do
     test "deletes media files that are past their retention date" do
-      {_source, old_media_item, new_media_item} = prepare_records()
+      {_source, old_media_item, new_media_item} = prepare_records_for_retention_date()
 
       perform_job(MediaRetentionWorker, %{})
 
@@ -25,27 +46,33 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
       refute Repo.reload!(old_media_item).media_filepath
     end
 
-    test "sets deleted media to not re-download" do
+    test "sets culled_at and prevent_download" do
-      {_source, old_media_item, new_media_item} = prepare_records()
+      {_source, old_media_item, new_media_item} = prepare_records_for_retention_date()
-
-      perform_job(MediaRetentionWorker, %{})
-
-      refute Repo.reload!(new_media_item).prevent_download
-      assert Repo.reload!(old_media_item).prevent_download
-    end
-
-    test "sets culled_at timestamp on deleted media" do
-      {_source, old_media_item, new_media_item} = prepare_records()
 
       perform_job(MediaRetentionWorker, %{})
 
       refute Repo.reload!(new_media_item).culled_at
       assert Repo.reload!(old_media_item).culled_at
-      assert DateTime.diff(now(), Repo.reload!(old_media_item).culled_at) < 1
+      refute Repo.reload!(new_media_item).prevent_download
+      assert Repo.reload!(old_media_item).prevent_download
+    end
+
+    test "doesn't cull if the source doesn't have a retention period" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_retention_date(nil)
+
+      perform_job(MediaRetentionWorker, %{})
+
+      assert File.exists?(new_media_item.media_filepath)
+      assert File.exists?(old_media_item.media_filepath)
+      assert Repo.reload!(new_media_item).media_filepath
+      assert Repo.reload!(old_media_item).media_filepath
+
+      refute Repo.reload!(new_media_item).culled_at
+      refute Repo.reload!(old_media_item).culled_at
     end
 
     test "doesn't cull media items that have prevent_culling set" do
-      {_source, old_media_item, _new_media_item} = prepare_records()
+      {_source, old_media_item, _new_media_item} = prepare_records_for_retention_date()
 
       Media.update_media_item(old_media_item, %{prevent_culling: true})
 
@@ -53,11 +80,99 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
 
       assert File.exists?(old_media_item.media_filepath)
       assert Repo.reload!(old_media_item).media_filepath
+      refute Repo.reload!(old_media_item).culled_at
+    end
+
+    test "doesn't cull if the media item has no media_filepath" do
+      {_source, old_media_item, _new_media_item} = prepare_records_for_retention_date()
+
+      Media.update_media_item(old_media_item, %{media_filepath: nil})
+
+      perform_job(MediaRetentionWorker, %{})
+
+      refute Repo.reload!(old_media_item).culled_at
     end
   end
 
-  defp prepare_records do
+  describe "perform/1 when testing source cutoff-based culling" do
-    source = source_fixture(%{retention_period_days: 2})
+    test "culls media from before the cutoff date" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_source_cutoff_date()
+
+      perform_job(MediaRetentionWorker, %{})
+
+      assert File.exists?(new_media_item.media_filepath)
+      refute File.exists?(old_media_item.media_filepath)
+      assert Repo.reload!(new_media_item).media_filepath
+      refute Repo.reload!(old_media_item).media_filepath
+    end
+
+    test "sets culled_at but not prevent_download" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_source_cutoff_date()
+
+      perform_job(MediaRetentionWorker, %{})
+
+      refute Repo.reload!(new_media_item).culled_at
+      assert Repo.reload!(old_media_item).culled_at
+      refute Repo.reload!(new_media_item).prevent_download
+      refute Repo.reload!(old_media_item).prevent_download
+    end
+
+    test "doesn't cull media if the source doesn't have a cutoff date" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_source_cutoff_date(nil)
+
+      perform_job(MediaRetentionWorker, %{})
+
+      assert File.exists?(new_media_item.media_filepath)
+      assert File.exists?(old_media_item.media_filepath)
+      assert Repo.reload!(new_media_item).media_filepath
+      assert Repo.reload!(old_media_item).media_filepath
+
+      refute Repo.reload!(new_media_item).culled_at
+      refute Repo.reload!(old_media_item).culled_at
+    end
+
+    test "doesn't cull media from on or after the cutoff date" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_source_cutoff_date(2)
+
+      Media.update_media_item(old_media_item, %{uploaded_at: now_minus(2, :days)})
+      Media.update_media_item(new_media_item, %{uploaded_at: now_minus(1, :day)})
+
+      perform_job(MediaRetentionWorker, %{})
+
+      assert File.exists?(new_media_item.media_filepath)
+      assert File.exists?(old_media_item.media_filepath)
+      assert Repo.reload!(new_media_item).media_filepath
+      assert Repo.reload!(old_media_item).media_filepath
+
+      refute Repo.reload!(new_media_item).culled_at
+      refute Repo.reload!(old_media_item).culled_at
+    end
+
+    test "doesn't cull media items that have prevent_culling set" do
+      {_source, old_media_item, _new_media_item} = prepare_records_for_source_cutoff_date()
+
+      Media.update_media_item(old_media_item, %{prevent_culling: true})
+
+      perform_job(MediaRetentionWorker, %{})
+
+      assert File.exists?(old_media_item.media_filepath)
+      assert Repo.reload!(old_media_item).media_filepath
+      refute Repo.reload!(old_media_item).culled_at
+    end
+
+    test "doesn't cull if the media item has no media_filepath" do
+      {_source, old_media_item, _new_media_item} = prepare_records_for_source_cutoff_date()
+
+      Media.update_media_item(old_media_item, %{media_filepath: nil})
+
+      perform_job(MediaRetentionWorker, %{})
+
+      refute Repo.reload!(old_media_item).culled_at
+    end
+  end
+
+  defp prepare_records_for_retention_date(retention_period_days \\ 2) do
+    source = source_fixture(%{retention_period_days: retention_period_days})
 
     old_media_item =
       media_item_with_attachments(%{
@@ -73,4 +188,23 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
 
     {source, old_media_item, new_media_item}
   end
+
+  defp prepare_records_for_source_cutoff_date(download_cutoff_date_days_ago \\ 2) do
+    cutoff_date = if download_cutoff_date_days_ago, do: now_minus(download_cutoff_date_days_ago, :days), else: nil
+    source = source_fixture(%{download_cutoff_date: cutoff_date})
+
+    old_media_item =
+      media_item_with_attachments(%{
+        source_id: source.id,
+        uploaded_at: now_minus(3, :days)
+      })
+
+    new_media_item =
+      media_item_with_attachments(%{
+        source_id: source.id,
+        uploaded_at: now_minus(1, :day)
+      })
+
+    {source, old_media_item, new_media_item}
+  end
 end

test/pinchflat/media_test.exs

@@ -41,98 +41,6 @@ defmodule Pinchflat.MediaTest do
     end
   end
 
-  describe "list_cullable_media_items/0" do
-    test "returns media items where the source has a retention period" do
-      source_one = source_fixture(%{retention_period_days: 2})
-      source_two = source_fixture(%{retention_period_days: 0})
-      source_three = source_fixture(%{retention_period_days: nil})
-
-      _media_item =
-        media_item_fixture(%{
-          source_id: source_two.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      _media_item =
-        media_item_fixture(%{
-          source_id: source_three.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      expected_media_item =
-        media_item_fixture(%{
-          source_id: source_one.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      assert Media.list_cullable_media_items() == [expected_media_item]
-    end
-
-    test "returns media_items with a media_filepath" do
-      source = source_fixture(%{retention_period_days: 2})
-
-      _media_item =
-        media_item_fixture(%{
-          source_id: source.id,
-          media_filepath: nil,
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      expected_media_item =
-        media_item_fixture(%{
-          source_id: source.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      assert Media.list_cullable_media_items() == [expected_media_item]
-    end
-
-    test "returns items that have passed their retention period" do
-      source = source_fixture(%{retention_period_days: 2})
-
-      _media_item =
-        media_item_fixture(%{
-          source_id: source.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(2, :days)
-        })
-
-      expected_media_item =
-        media_item_fixture(%{
-          source_id: source.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      assert Media.list_cullable_media_items() == [expected_media_item]
-    end
-
-    test "doesn't return items that are set to prevent culling" do
-      source = source_fixture(%{retention_period_days: 2})
-
-      _media_item =
-        media_item_fixture(%{
-          source_id: source.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days),
-          prevent_culling: true
-        })
-
-      expected_media_item =
-        media_item_fixture(%{
-          source_id: source.id,
-          media_filepath: "/video/#{Faker.File.file_name(:video)}",
-          media_downloaded_at: now_minus(3, :days)
-        })
-
-      assert Media.list_cullable_media_items() == [expected_media_item]
-    end
-  end
-
   describe "list_upgradeable_media_items/0" do
     setup do
       media_profile = media_profile_fixture(%{redownload_delay_days: 4})