[Bugfix] Fix off-by-1 error for retention date logic (#432)

* Added a sanity check test to the media context

* Improves logic for handing media item culling dates

lib/pinchflat/downloading/media_retention_worker.ex

@@ -49,6 +49,8 @@ defmodule Pinchflat.Downloading.MediaRetentionWorker do
     end)
   end
 
+  # NOTE: Since this is a date and not a datetime, we can't add logic to have to-the-minute
+  # comparison like we can with retention periods. We can only compare to the day.
   defp delete_media_items_from_before_cutoff do
     deletable_media =
       MediaQuery.new()

lib/pinchflat/media/media_query.ex

@@ -88,7 +88,7 @@ defmodule Pinchflat.Media.MediaQuery do
       [mi, source],
       fragment("""
         IFNULL(retention_period_days, 0) > 0 AND
-        DATETIME('now', '-' || retention_period_days || ' day') > media_downloaded_at
+        DATETIME(media_downloaded_at, '+' || retention_period_days || ' day') < DATETIME('now')
       """)
     )
   end
@@ -100,8 +100,8 @@ defmodule Pinchflat.Media.MediaQuery do
       # downloaded_at minus the redownload_delay_days is before the upload date
       fragment("""
         IFNULL(redownload_delay_days, 0) > 0 AND
-        DATETIME('now', '-' || redownload_delay_days || ' day') > uploaded_at AND
+        DATE('now', '-' || redownload_delay_days || ' day') > DATE(uploaded_at) AND
-        DATETIME(media_downloaded_at, '-' || redownload_delay_days || ' day') < uploaded_at
+        DATE(media_downloaded_at, '-' || redownload_delay_days || ' day') < DATE(uploaded_at)
       """)
     )
   end

test/pinchflat/downloading/media_retention_worker_test.exs

@@ -46,6 +46,23 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
       refute Repo.reload!(old_media_item).media_filepath
     end
 
+    test "deletes media files that are on their retention date per the 24-h clock" do
+      {_source, old_media_item, new_media_item} = prepare_records_for_retention_date(2)
+
+      just_over_two_days_ago = now_minus(2, :days) |> DateTime.add(-1, :minute)
+      just_under_two_days_ago = now_minus(2, :days) |> DateTime.add(1, :minute)
+
+      Media.update_media_item(old_media_item, %{media_downloaded_at: just_over_two_days_ago})
+      Media.update_media_item(new_media_item, %{media_downloaded_at: just_under_two_days_ago})
+
+      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 and prevent_download" do
       {_source, old_media_item, new_media_item} = prepare_records_for_retention_date()
 
@@ -106,6 +123,25 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
       refute Repo.reload!(old_media_item).media_filepath
     end
 
+    # NOTE: Since this is a date and not a datetime, we can't add logic to have to-the-minute
+    # comparison like we can with retention periods. We can only compare to the day.
+    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 "sets culled_at but not prevent_download" do
       {_source, old_media_item, new_media_item} = prepare_records_for_source_cutoff_date()
 
@@ -131,23 +167,6 @@ defmodule Pinchflat.Downloading.MediaRetentionWorkerTest do
       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()
 

test/pinchflat/media_test.exs

@@ -441,6 +441,13 @@ defmodule Pinchflat.MediaTest do
       assert Media.pending_download?(media_item)
     end
 
+    test "returns true if the cutoff date is equal to the upload date" do
+      source = source_fixture(%{download_cutoff_date: now_minus(2, :days)})
+      media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, uploaded_at: now_minus(2, :days)})
+
+      assert Media.pending_download?(media_item)
+    end
+
     test "returns false if there is a cutoff date after the media's upload date" do
       source = source_fixture(%{download_cutoff_date: now_minus(1, :day)})
       media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, uploaded_at: now_minus(2, :days)})