[Enhancement] Media from before a source's cutoff date is now automatically deleted (#329)

* Changed the purpose of the 'culled_at' flag. renamed some methods

* implemented new culling behaviour for source cutoff dates

* Removed culled_at flag if a media item gets redownloaded

* Updated source form

* Removed unused method

test/pinchflat/downloading/downloading_helpers_test.exs

@@ -130,9 +130,6 @@ defmodule Pinchflat.Downloading.DownloadingHelpersTest do
       _download_prevented =
         media_item_fixture(source_id: source.id, media_filepath: "some/filepath.mp4", prevent_download: true)
 
-      _culled =
-        media_item_fixture(source_id: source.id, media_filepath: "some/filepath.mp4", culled_at: now())
-
       assert [] = DownloadingHelpers.kickoff_redownload_for_existing_media(source)
 
       refute_enqueued(worker: MediaDownloadWorker)

test/pinchflat/downloading/media_downloader_test.exs

@@ -5,6 +5,7 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do
   import Pinchflat.SourcesFixtures
   import Pinchflat.ProfilesFixtures
 
+  alias Pinchflat.Media
   alias Pinchflat.Downloading.MediaDownloader
 
   setup do
@@ -123,6 +124,12 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do
       assert DateTime.diff(DateTime.utc_now(), updated_media_item.media_downloaded_at) < 2
     end
 
+    test "it sets the culled_at to nil", %{media_item: media_item} do
+      Media.update_media_item(media_item, %{culled_at: DateTime.utc_now()})
+      assert {:ok, updated_media_item} = MediaDownloader.download_for_media_item(media_item)
+      assert updated_media_item.culled_at == nil
+    end
+
     test "it extracts the title", %{media_item: media_item} do
       assert {:ok, updated_media_item} = MediaDownloader.download_for_media_item(media_item)
       assert updated_media_item.title == "Pinchflat Example Video"

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)
+  setup do
+    stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end)
 
-      :ok
+    :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
-      {_source, old_media_item, new_media_item} = prepare_records()
-
-      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()
+    test "sets culled_at and prevent_download" 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
+      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
-    source = source_fixture(%{retention_period_days: 2})
+  describe "perform/1 when testing source cutoff-based culling" do
+    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,99 +41,7 @@ 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_redownloadable_media_items/0" do
+  describe "list_upgradeable_media_items/0" do
     setup 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)})
@@ -149,7 +57,7 @@ defmodule Pinchflat.MediaTest do
           media_downloaded_at: now_minus(5, :days)
         })
 
-      assert Media.list_redownloadable_media_items() == [media_item]
+      assert Media.list_upgradeable_media_items() == [media_item]
     end
 
     test "returns media items that were downloaded in past but still meet redownload delay", %{source: source} do
@@ -160,7 +68,7 @@ defmodule Pinchflat.MediaTest do
           media_downloaded_at: now_minus(19, :days)
         })
 
-      assert Media.list_redownloadable_media_items() == [media_item]
+      assert Media.list_upgradeable_media_items() == [media_item]
     end
 
     test "does not return media items without a media_downloaded_at", %{source: source} do
@@ -171,7 +79,7 @@ defmodule Pinchflat.MediaTest do
           media_downloaded_at: nil
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items that are set to prevent download", %{source: source} do
@@ -183,7 +91,7 @@ defmodule Pinchflat.MediaTest do
           prevent_download: true
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items that have been culled", %{source: source} do
@@ -195,7 +103,7 @@ defmodule Pinchflat.MediaTest do
           culled_at: now()
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items before the download delay", %{source: source} do
@@ -206,7 +114,7 @@ defmodule Pinchflat.MediaTest do
           media_downloaded_at: now_minus(3, :days)
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items that have already been redownloaded", %{source: source} do
@@ -218,7 +126,7 @@ defmodule Pinchflat.MediaTest do
           media_redownloaded_at: now()
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items that were first downloaded well after the uploaded_at", %{source: source} do
@@ -229,7 +137,7 @@ defmodule Pinchflat.MediaTest do
           uploaded_at: now_minus(20, :days)
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items that were recently uploaded", %{source: source} do
@@ -240,7 +148,7 @@ defmodule Pinchflat.MediaTest do
           uploaded_at: now_minus(2, :days)
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
 
     test "does not return media items without a redownload delay" do
@@ -254,7 +162,7 @@ defmodule Pinchflat.MediaTest do
           media_downloaded_at: now_minus(5, :days)
         })
 
-      assert Media.list_redownloadable_media_items() == []
+      assert Media.list_upgradeable_media_items() == []
     end
   end