From 1ac51203ad461b6fa8dd777cde2fcc2a22ff6b54 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sun, 10 Mar 2024 21:16:32 -0700 Subject: [PATCH] Applies cutoff date logic to pending media logic --- lib/pinchflat/media.ex | 14 +++++- lib/pinchflat/sources.ex | 42 +++++++++++++----- lib/pinchflat/sources/source.ex | 30 ++++++++++--- test/pinchflat/media_test.exs | 45 ++++++++++++++++++++ test/pinchflat/sources_test.exs | 16 ++++++- test/pinchflat/yt_dlp/backend/media_test.exs | 13 ++++++ test/support/fixtures/sources_fixtures.ex | 22 ++++++---- test/support/testing_helper_methods.ex | 8 ++++ 8 files changed, 161 insertions(+), 29 deletions(-) diff --git a/lib/pinchflat/media.ex b/lib/pinchflat/media.ex index f3821ea..28147b1 100644 --- a/lib/pinchflat/media.ex +++ b/lib/pinchflat/media.ex @@ -64,6 +64,7 @@ defmodule Pinchflat.Media do MediaItem |> where([mi], mi.source_id == ^source.id and is_nil(mi.media_filepath)) |> where(^build_format_clauses(media_profile)) + |> where(^maybe_apply_cutoff_date(source)) |> Repo.maybe_limit(limit) |> Repo.all() end @@ -92,11 +93,12 @@ defmodule Pinchflat.Media do Returns boolean() """ def pending_download?(%MediaItem{} = media_item) do - media_profile = Repo.preload(media_item, source: :media_profile).source.media_profile + media_item = Repo.preload(media_item, source: :media_profile) MediaItem |> where([mi], mi.id == ^media_item.id and is_nil(mi.media_filepath)) - |> where(^build_format_clauses(media_profile)) + |> where(^build_format_clauses(media_item.source.media_profile)) + |> where(^maybe_apply_cutoff_date(media_item.source)) |> Repo.exists?() end @@ -260,6 +262,14 @@ defmodule Pinchflat.Media do {:ok, media_item} end + defp maybe_apply_cutoff_date(source) do + if source.download_cutoff_date do + dynamic([mi], mi.upload_date >= ^source.download_cutoff_date) + else + dynamic(true) + end + end + defp build_format_clauses(media_profile) do mapped_struct = Map.from_struct(media_profile) diff --git a/lib/pinchflat/sources.ex b/lib/pinchflat/sources.ex index eee7c64..dd53971 100644 --- a/lib/pinchflat/sources.ex +++ b/lib/pinchflat/sources.ex @@ -41,13 +41,24 @@ defmodule Pinchflat.Sources do original_url (if provided). Will attempt to start indexing the source's media if successfully inserted. + Runs an initial `change_source` check to ensure most of the source is valid + before making an expensive API call. Runs it through `Repo.insert` even + though we know it's going to fail so it picks up any addl. database errors + and fulfills our return contract. + Returns {:ok, %Source{}} | {:error, %Ecto.Changeset{}} """ def create_source(attrs) do - %Source{} - |> change_source_from_url(attrs) - |> maybe_change_indexing_frequency() - |> commit_and_handle_tasks() + case change_source(%Source{}, attrs, :initial) do + %Ecto.Changeset{valid?: true} -> + %Source{} + |> change_source_from_url(attrs) + |> maybe_change_indexing_frequency() + |> commit_and_handle_tasks() + + changeset -> + Repo.insert(changeset) + end end @doc """ @@ -58,13 +69,24 @@ defmodule Pinchflat.Sources do Existing indexing tasks will be cancelled if the indexing frequency has been changed (logic in `SourceTasks.kickoff_indexing_task`) + Runs an initial `change_source` check to ensure most of the source is valid + before making an expensive API call. Runs it through `Repo.update` even + though we know it's going to fail so it picks up any addl. database errors + and fulfills our return contract. + Returns {:ok, %Source{}} | {:error, %Ecto.Changeset{}} """ def update_source(%Source{} = source, attrs) do - source - |> change_source_from_url(attrs) - |> maybe_change_indexing_frequency() - |> commit_and_handle_tasks() + case change_source(source, attrs, :initial) do + %Ecto.Changeset{valid?: true} -> + source + |> change_source_from_url(attrs) + |> maybe_change_indexing_frequency() + |> commit_and_handle_tasks() + + changeset -> + Repo.update(changeset) + end end @doc """ @@ -89,8 +111,8 @@ defmodule Pinchflat.Sources do @doc """ Returns an `%Ecto.Changeset{}` for tracking source changes. """ - def change_source(%Source{} = source, attrs \\ %{}) do - Source.changeset(source, attrs) + def change_source(%Source{} = source, attrs \\ %{}, validation_stage \\ :pre_insert) do + Source.changeset(source, attrs, validation_stage) end @doc """ diff --git a/lib/pinchflat/sources/source.ex b/lib/pinchflat/sources/source.ex index 9b7f15d..3022a7d 100644 --- a/lib/pinchflat/sources/source.ex +++ b/lib/pinchflat/sources/source.ex @@ -25,11 +25,11 @@ defmodule Pinchflat.Sources.Source do media_profile_id )a - @required_fields ~w( - collection_name - collection_id - collection_type - custom_name + # Expensive API calls are made when a source is inserted/updated so + # we want to ensure that the source is valid before making the call. + # This way, we check that the other attributes are valid before ensuring + # that all fields are valid. + @initially_required_fields ~w( index_frequency_minutes fast_index download_media @@ -37,6 +37,14 @@ defmodule Pinchflat.Sources.Source do media_profile_id )a + @pre_insert_required_fields @initially_required_fields ++ + ~w( + custom_name + collection_name + collection_id + collection_type + )a + schema "sources" do field :custom_name, :string field :collection_name, :string @@ -59,11 +67,19 @@ defmodule Pinchflat.Sources.Source do end @doc false - def changeset(source, attrs) do + def changeset(source, attrs, validation_stage) do + # See above for rationale + required_fields = + if validation_stage == :initial do + @initially_required_fields + else + @pre_insert_required_fields + end + source |> cast(attrs, @allowed_fields) |> dynamic_default(:custom_name, fn cs -> get_field(cs, :collection_name) end) - |> validate_required(@required_fields) + |> validate_required(required_fields) |> unique_constraint([:collection_id, :media_profile_id]) end diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index ffddebf..1437178 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -215,6 +215,30 @@ defmodule Pinchflat.MediaTest do end end + describe "list_pending_media_items_for/1 when testing cutoff dates" do + test "does not return media items with an upload date before the cutoff date" do + source = source_fixture(%{download_cutoff_date: now_minus(1, :day)}) + + _old_media_item = + media_item_fixture(%{source_id: source.id, media_filepath: nil, upload_date: now_minus(2, :days)}) + + new_media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, upload_date: now()}) + + assert Media.list_pending_media_items_for(source) == [new_media_item] + end + + test "does not apply a cutoff if there is no cutoff date" do + source = source_fixture(%{download_cutoff_date: nil}) + + old_media_item = + media_item_fixture(%{source_id: source.id, media_filepath: nil, upload_date: now_minus(2, :days)}) + + new_media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, upload_date: now()}) + + assert Media.list_pending_media_items_for(source) == [old_media_item, new_media_item] + end + end + describe "list_downloaded_media_items_for/1" do test "returns only media items with a media_filepath" do source = source_fixture() @@ -260,6 +284,27 @@ defmodule Pinchflat.MediaTest do refute Media.pending_download?(media_item) end + + test "returns true if there is a cutoff date before the media's 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, upload_date: now_minus(1, :day)}) + + 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, upload_date: now_minus(2, :days)}) + + refute Media.pending_download?(media_item) + end + + test "returns true if there is no cutoff date" do + source = source_fixture(%{download_cutoff_date: nil}) + media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, upload_date: now_minus(1, :day)}) + + assert Media.pending_download?(media_item) + end end describe "search/1" do diff --git a/test/pinchflat/sources_test.exs b/test/pinchflat/sources_test.exs index fe1db7e..6841e27 100644 --- a/test/pinchflat/sources_test.exs +++ b/test/pinchflat/sources_test.exs @@ -115,6 +115,12 @@ defmodule Pinchflat.SourcesTest do assert {:error, %Ecto.Changeset{}} = Sources.create_source(@invalid_source_attrs) end + test "creation with invalid data fails fast and does not call the runner" do + expect(YtDlpRunnerMock, :run, 0, &channel_mock/3) + + assert {:error, %Ecto.Changeset{}} = Sources.create_source(@invalid_source_attrs) + end + test "creation enforces uniqueness of collection_id scoped to the media_profile" do expect(YtDlpRunnerMock, :run, 2, fn _url, _opts, _ot -> {:ok, @@ -225,6 +231,14 @@ defmodule Pinchflat.SourcesTest do assert source.collection_name == "some updated name" end + test "updates with invalid data fails fast and does not call the runner" do + expect(YtDlpRunnerMock, :run, 0, &channel_mock/3) + + source = source_fixture() + + assert {:error, %Ecto.Changeset{}} = Sources.update_source(source, @invalid_source_attrs) + end + test "updating the original_url will re-fetch the source details for channels" do expect(YtDlpRunnerMock, :run, &channel_mock/3) @@ -430,7 +444,7 @@ defmodule Pinchflat.SourcesTest do end end - describe "change_source/2" do + describe "change_source/3" do test "it returns a changeset" do source = source_fixture() diff --git a/test/pinchflat/yt_dlp/backend/media_test.exs b/test/pinchflat/yt_dlp/backend/media_test.exs index e69ce2c..02d4043 100644 --- a/test/pinchflat/yt_dlp/backend/media_test.exs +++ b/test/pinchflat/yt_dlp/backend/media_test.exs @@ -140,5 +140,18 @@ defmodule Pinchflat.YtDlp.Backend.MediaTest do assert %Media{short_form_content: false} = Media.response_to_struct(response) end + + test "parses the upload date" do + response = %{ + "webpage_url" => "https://www.youtube.com/watch?v=TiZPUDkDYbk", + "aspect_ratio" => 1.0, + "duration" => 61, + "upload_date" => "20210101" + } + + expected_date = Date.from_iso8601!("2021-01-01") + + assert %Media{upload_date: ^expected_date} = Media.response_to_struct(response) + end end end diff --git a/test/support/fixtures/sources_fixtures.ex b/test/support/fixtures/sources_fixtures.ex index 9f4b076..f6fda5a 100644 --- a/test/support/fixtures/sources_fixtures.ex +++ b/test/support/fixtures/sources_fixtures.ex @@ -15,15 +15,19 @@ defmodule Pinchflat.SourcesFixtures do {:ok, source} = %Source{} |> Source.changeset( - Enum.into(attrs, %{ - collection_name: "Source ##{:rand.uniform(1_000_000)}", - collection_id: Base.encode16(:crypto.hash(:md5, "#{:rand.uniform(1_000_000)}")), - collection_type: "channel", - custom_name: "Cool and good internal name!", - original_url: "https://www.youtube.com/channel/#{Faker.String.base64(12)}", - media_profile_id: ProfilesFixtures.media_profile_fixture().id, - index_frequency_minutes: 60 - }) + Enum.into( + attrs, + %{ + collection_name: "Source ##{:rand.uniform(1_000_000)}", + collection_id: Base.encode16(:crypto.hash(:md5, "#{:rand.uniform(1_000_000)}")), + collection_type: "channel", + custom_name: "Cool and good internal name!", + original_url: "https://www.youtube.com/channel/#{Faker.String.base64(12)}", + media_profile_id: ProfilesFixtures.media_profile_fixture().id, + index_frequency_minutes: 60 + } + ), + :pre_insert ) |> Repo.insert() diff --git a/test/support/testing_helper_methods.ex b/test/support/testing_helper_methods.ex index 09f16ad..41eedd4 100644 --- a/test/support/testing_helper_methods.ex +++ b/test/support/testing_helper_methods.ex @@ -11,6 +11,14 @@ defmodule Pinchflat.TestingHelperMethods do DateTime.add(now(), offset, :minute) end + def now_minus(offset, unit) when unit in [:minute, :minutes] do + DateTime.add(now(), -offset, :minute) + end + + def now_minus(offset, unit) when unit in [:day, :days] do + DateTime.add(now(), -offset, :day) + end + def assert_changed(checker_fun, action_fn) do before_res = checker_fun.() action_fn.()