From e166a1e9bd09db2128b8dbabb0da92a24965f532 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Fri, 19 Jul 2024 15:54:14 -0700 Subject: [PATCH] Changed the purpose of the 'culled_at' flag. renamed some methods --- .../downloading/downloading_helpers.ex | 3 +- .../media_quality_upgrade_worker.ex | 6 +-- .../downloading/media_retention_worker.ex | 41 +++++++++++++++++++ lib/pinchflat/media/media.ex | 10 +++-- lib/pinchflat/media/media_query.ex | 20 ++++++++- .../downloading/downloading_helpers_test.exs | 3 -- test/pinchflat/media_test.exs | 22 +++++----- 7 files changed, 80 insertions(+), 25 deletions(-) diff --git a/lib/pinchflat/downloading/downloading_helpers.ex b/lib/pinchflat/downloading/downloading_helpers.ex index cb679a8..5898533 100644 --- a/lib/pinchflat/downloading/downloading_helpers.ex +++ b/lib/pinchflat/downloading/downloading_helpers.ex @@ -91,8 +91,7 @@ defmodule Pinchflat.Downloading.DownloadingHelpers do [m, s, mp], ^MediaQuery.for_source(source) and ^MediaQuery.downloaded() and - not (^MediaQuery.download_prevented()) and - not (^MediaQuery.culled()) + not (^MediaQuery.download_prevented()) ) ) |> Repo.all() diff --git a/lib/pinchflat/downloading/media_quality_upgrade_worker.ex b/lib/pinchflat/downloading/media_quality_upgrade_worker.ex index 4f554fa..8fcfb22 100644 --- a/lib/pinchflat/downloading/media_quality_upgrade_worker.ex +++ b/lib/pinchflat/downloading/media_quality_upgrade_worker.ex @@ -23,10 +23,10 @@ defmodule Pinchflat.Downloading.MediaQualityUpgradeWorker do """ @impl Oban.Worker def perform(%Oban.Job{}) do - redownloadable_media = Media.list_redownloadable_media_items() - Logger.info("Redownloading #{length(redownloadable_media)} media items") + upgradable_media = Media.list_upgradeable_media_items() + Logger.info("Redownloading #{length(upgradable_media)} media items") - Enum.each(redownloadable_media, fn media_item -> + Enum.each(upgradable_media, fn media_item -> MediaDownloadWorker.kickoff_with_task(media_item, %{quality_upgrade?: true}) end) end diff --git a/lib/pinchflat/downloading/media_retention_worker.ex b/lib/pinchflat/downloading/media_retention_worker.ex index 3e5c0d0..cfcc2da 100644 --- a/lib/pinchflat/downloading/media_retention_worker.ex +++ b/lib/pinchflat/downloading/media_retention_worker.ex @@ -6,8 +6,11 @@ defmodule Pinchflat.Downloading.MediaRetentionWorker do unique: [period: :infinity, states: [:available, :scheduled, :retryable, :executing]], tags: ["media_item", "local_metadata"] + use Pinchflat.Media.MediaQuery + require Logger + alias Pinchflat.Repo alias Pinchflat.Media @doc """ @@ -20,14 +23,52 @@ defmodule Pinchflat.Downloading.MediaRetentionWorker do """ @impl Oban.Worker def perform(%Oban.Job{}) do + cull_cullable_media_items() + # 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() 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 Media.delete_media_files(media_item, %{ prevent_download: true, culled_at: DateTime.utc_now() }) end) end + + # TODO: test + defp delete_media_items_from_before_cutoff do + deletable_media = + MediaQuery.new() + |> MediaQuery.require_assoc(:source) + |> where(^MediaQuery.deletable_from_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 + # 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. + # 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) + end) + end end diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index 159c8b7..868ace5 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -38,8 +38,10 @@ defmodule Pinchflat.Media do end @doc """ - Returns a list of media_items that are redownloadable based on the redownload delay - of the media_profile their source belongs to. + 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 + that it's been long enough since upload that the video may be in a higher quality + or have better sponsorblock segments (or similar). The logic is that a media_item is past_redownload_delay if the media_item's uploaded_at is at least redownload_delay_days ago AND `media_downloaded_at` - `redownload_delay_days` @@ -52,10 +54,10 @@ defmodule Pinchflat.Media do Returns [%MediaItem{}, ...] """ - def list_redownloadable_media_items do + def list_upgradeable_media_items do MediaQuery.new() |> MediaQuery.require_assoc(:media_profile) - |> where(^MediaQuery.redownloadable()) + |> where(^MediaQuery.upgradeable()) |> Repo.all() end diff --git a/lib/pinchflat/media/media_query.ex b/lib/pinchflat/media/media_query.ex index 2adb0ce..37d10e3 100644 --- a/lib/pinchflat/media/media_query.ex +++ b/lib/pinchflat/media/media_query.ex @@ -45,6 +45,14 @@ defmodule Pinchflat.Media.MediaQuery do ) end + # def upload_date_before_source_cutoff do + # dynamic( + # [mi, source], + # not is_nil(source.download_cutoff_date) and + # fragment("date(?) < ?", mi.uploaded_at, source.download_cutoff_date) + # ) + # end + def format_matching_profile_preference do dynamic( [mi, source, media_profile], @@ -108,6 +116,15 @@ defmodule Pinchflat.Media.MediaQuery do ) end + def deletable_from_source_cutoff do + dynamic( + [mi, source], + ^downloaded() and + not (^upload_date_after_source_cutoff()) and + not (^culling_prevented()) + ) + end + def pending do dynamic( [mi], @@ -119,12 +136,11 @@ defmodule Pinchflat.Media.MediaQuery do ) end - def redownloadable do + def upgradeable do dynamic( [mi, source], ^downloaded() and not (^download_prevented()) and - not (^culled()) and not (^redownloaded()) and ^past_redownload_delay() ) diff --git a/test/pinchflat/downloading/downloading_helpers_test.exs b/test/pinchflat/downloading/downloading_helpers_test.exs index 01b0518..5ab4f40 100644 --- a/test/pinchflat/downloading/downloading_helpers_test.exs +++ b/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) diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index c95b4d0..1e3c7a2 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -133,7 +133,7 @@ defmodule Pinchflat.MediaTest do 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 +149,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 +160,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 +171,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 +183,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 +195,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 +206,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 +218,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 +229,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 +240,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 +254,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