diff --git a/lib/pinchflat/downloading/media_retention_worker.ex b/lib/pinchflat/downloading/media_retention_worker.ex index cfcc2da..65752b4 100644 --- a/lib/pinchflat/downloading/media_retention_worker.ex +++ b/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 = Media.list_cullable_media_items() + cullable_media = + 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, - # TODO: finish these docs + # Setting `prevent_download` does what it says on the tin, but `culled_at` is purely informational. + # 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. - # 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 + # TODO: 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). - Media.delete_media_files(media_item) + # from after the cutoff date (among other things), so it's not like the media will just immediately + # be re-downloaded. + Media.delete_media_files(media_item, %{ + culled_at: DateTime.utc_now() + }) end) end end diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index 868ace5..ba46e7c 100644 --- a/lib/pinchflat/media/media.ex +++ b/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 diff --git a/lib/pinchflat/media/media_query.ex b/lib/pinchflat/media/media_query.ex index 37d10e3..a0bc47a 100644 --- a/lib/pinchflat/media/media_query.ex +++ b/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 diff --git a/test/pinchflat/downloading/media_retention_worker_test.exs b/test/pinchflat/downloading/media_retention_worker_test.exs index 85ffdd9..e49bb39 100644 --- a/test/pinchflat/downloading/media_retention_worker_test.exs +++ b/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 diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index 1e3c7a2..5f1efb5 100644 --- a/test/pinchflat/media_test.exs +++ b/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})