diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index d58f657..d81fde2 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -30,7 +30,6 @@ defmodule Pinchflat.Media do """ def list_cullable_media_items do MediaQuery.new() - |> MediaQuery.join_sources() |> MediaQuery.with_media_filepath() |> MediaQuery.with_passed_retention_period() |> MediaQuery.with_no_culling_prevention() @@ -42,34 +41,15 @@ defmodule Pinchflat.Media do pending means the `media_filepath` is `nil` AND the media_item matches the format selection rules of the parent media_profile. - See `matching_download_criteria_for` but tl;dr is it _may_ filter based + See `where_pending` but tl;dr is it _may_ filter based on shorts livestreams depending on the media_profile settings. Returns [%MediaItem{}, ...]. """ - def list_pending_media_items_for(%Source{} = source, opts \\ []) do - limit = Keyword.get(opts, :limit, nil) - source = Repo.preload(source, :media_profile) - + def list_pending_media_items_for(%Source{} = source) do MediaQuery.new() |> MediaQuery.for_source(source) - |> matching_download_criteria_for(source) - |> Repo.maybe_limit(limit) - |> Repo.all() - end - - @doc """ - Returns a list of downloaded media_items for a given source. - - Returns [%MediaItem{}, ...]. - """ - def list_downloaded_media_items_for(%Source{} = source, opts \\ []) do - limit = Keyword.get(opts, :limit, nil) - - MediaQuery.new() - |> MediaQuery.for_source(source) - |> MediaQuery.with_media_filepath() - |> Repo.maybe_limit(limit) + |> MediaQuery.with_media_pending_download() |> Repo.all() end @@ -87,7 +67,7 @@ defmodule Pinchflat.Media do MediaQuery.new() |> MediaQuery.with_id(media_item.id) - |> matching_download_criteria_for(media_item.source) + |> MediaQuery.with_media_pending_download() |> Repo.exists?() end @@ -234,13 +214,4 @@ defmodule Pinchflat.Media do |> Enum.filter(&is_binary/1) |> Enum.each(&FilesystemHelpers.delete_file_and_remove_empty_directories/1) end - - defp matching_download_criteria_for(query, source_with_preloads) do - query - |> MediaQuery.with_no_prevented_download() - |> MediaQuery.with_no_media_filepath() - |> MediaQuery.with_upload_date_after(source_with_preloads.download_cutoff_date) - |> MediaQuery.with_format_preference(source_with_preloads.media_profile) - |> MediaQuery.matching_title_regex(source_with_preloads.title_filter_regex) - end end diff --git a/lib/pinchflat/media/media_query.ex b/lib/pinchflat/media/media_query.ex index 7b7a3c7..6751c28 100644 --- a/lib/pinchflat/media/media_query.ex +++ b/lib/pinchflat/media/media_query.ex @@ -3,13 +3,10 @@ defmodule Pinchflat.Media.MediaQuery do Query helpers for the Media context. These methods are made to be one-ish liners used - to compose queries for media items. Each method should - strive to do _one_ thing. These don't need to be tested - as they are just building blocks for other functionality + to compose queries. Each method should strive to do + _one_ thing. These don't need to be tested as + they are just building blocks for other functionality which, itself, will be tested. - - ALSO, this is me trying something new. If I like it, - I'll refactor other contexts to use this pattern. """ import Ecto.Query, warn: false @@ -37,13 +34,14 @@ defmodule Pinchflat.Media.MediaQuery do end def with_passed_retention_period(query) do - where( - query, - [mi, sources], + query + |> require_assoc(:source) + |> where( + [mi, source], fragment( "IFNULL(?, 0) > 0 AND DATETIME('now', '-' || ? || ' day') > ?", - sources.retention_period_days, - sources.retention_period_days, + source.retention_period_days, + source.retention_period_days, mi.media_downloaded_at ) ) @@ -69,20 +67,23 @@ defmodule Pinchflat.Media.MediaQuery do where(query, [mi], is_nil(mi.media_filepath)) end - def with_upload_date_after(query, nil), do: query - - def with_upload_date_after(query, date) do - where(query, [mi], mi.upload_date >= ^date) + def with_upload_date_after_source_cutoff(query) do + query + |> require_assoc(:source) + |> where([mi, source], is_nil(source.download_cutoff_date) or mi.upload_date >= source.download_cutoff_date) end def with_no_prevented_download(query) do where(query, [mi], mi.prevent_download == false) end - def matching_title_regex(query, nil), do: query - - def matching_title_regex(query, regex) do - where(query, [mi], fragment("regexp_like(?, ?)", mi.title, ^regex)) + def matching_source_title_regex(query) do + query + |> require_assoc(:source) + |> where( + [mi, source], + is_nil(source.title_filter_regex) or fragment("regexp_like(?, ?)", mi.title, source.title_filter_regex) + ) end def matching_search_term(query, nil), do: query @@ -103,44 +104,55 @@ defmodule Pinchflat.Media.MediaQuery do ) end - # NOTE: this method breaks the contract set by other methods in that it - # takes a media_profile struct instead of taking just the attributes it - # cares about. Consider refactoring but low priority. - def with_format_preference(query, media_profile) do - mapped_struct = Map.from_struct(media_profile) + def with_format_matching_profile_preference(query) do + query + |> require_assoc(:media_profile) + |> where( + fragment(""" + CASE + WHEN shorts_behaviour = 'only' AND livestream_behaviour = 'only' THEN + livestream = true OR short_form_content = true + WHEN shorts_behaviour = 'only' THEN + short_form_content = true + WHEN livestream_behaviour = 'only' THEN + livestream = true + WHEN shorts_behaviour = 'exclude' AND livestream_behaviour = 'exclude' THEN + short_form_content = false AND livestream = false + WHEN shorts_behaviour = 'exclude' THEN + short_form_content = false + WHEN livestream_behaviour = 'exclude' THEN + livestream = false + ELSE + true + END + """) + ) + end - finders = - Enum.reduce(mapped_struct, dynamic(true), fn attr, dynamic -> - case {attr, media_profile} do - {{:shorts_behaviour, :only}, %{livestream_behaviour: :only}} -> - dynamic( - [mi], - ^dynamic and (mi.livestream == true or mi.short_form_content == true) - ) + def with_media_pending_download(query) do + query + |> with_no_prevented_download() + |> with_no_media_filepath() + |> with_upload_date_after_source_cutoff() + |> with_format_matching_profile_preference() + |> matching_source_title_regex() + end - # Technically redundant, but makes the other clauses easier to parse - # (redundant because this condition is the same as the condition above, just flipped) - {{:livestream_behaviour, :only}, %{shorts_behaviour: :only}} -> - dynamic + defp require_assoc(query, identifier) do + if has_named_binding?(query, identifier) do + query + else + do_require_assoc(query, identifier) + end + end - {{:shorts_behaviour, :only}, _} -> - dynamic([mi], ^dynamic and mi.short_form_content == true) + defp do_require_assoc(query, :source) do + from(mi in query, join: s in assoc(mi, :source), as: :source) + end - {{:livestream_behaviour, :only}, _} -> - dynamic([mi], ^dynamic and mi.livestream == true) - - {{:shorts_behaviour, :exclude}, %{livestream_behaviour: lb}} when lb != :only -> - dynamic([mi], ^dynamic and mi.short_form_content == false) - - {{:livestream_behaviour, :exclude}, %{shorts_behaviour: sb}} when sb != :only -> - # return records with livestream: false - dynamic([mi], ^dynamic and mi.livestream == false) - - _ -> - dynamic - end - end) - - where(query, ^finders) + defp do_require_assoc(query, :media_profile) do + query + |> require_assoc(:source) + |> join(:inner, [mi, source], mp in assoc(source, :media_profile), as: :media_profile) end end diff --git a/lib/pinchflat/podcasts/podcast_helpers.ex b/lib/pinchflat/podcasts/podcast_helpers.ex index 33e328e..3353070 100644 --- a/lib/pinchflat/podcasts/podcast_helpers.ex +++ b/lib/pinchflat/podcasts/podcast_helpers.ex @@ -5,7 +5,7 @@ defmodule Pinchflat.Podcasts.PodcastHelpers do """ alias Pinchflat.Repo - alias Pinchflat.Media + alias Pinchflat.Media.MediaQuery alias Pinchflat.Metadata.MediaMetadata alias Pinchflat.Metadata.SourceMetadata @@ -25,8 +25,11 @@ defmodule Pinchflat.Podcasts.PodcastHelpers do def persisted_media_items_for(source, opts \\ []) do limit = Keyword.get(opts, :limit, 500) - source - |> Media.list_downloaded_media_items_for(limit: limit) + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_filepath() + |> Repo.maybe_limit(limit) + |> Repo.all() |> Enum.filter(fn media_item -> File.exists?(media_item.media_filepath) end) end diff --git a/lib/pinchflat/sources/sources_query.ex b/lib/pinchflat/sources/sources_query.ex new file mode 100644 index 0000000..81bd449 --- /dev/null +++ b/lib/pinchflat/sources/sources_query.ex @@ -0,0 +1,31 @@ +defmodule Pinchflat.Sources.SourcesQuery do + @moduledoc """ + Query helpers for the Sources context. + + These methods are made to be one-ish liners used + to compose queries. Each method should strive to do + _one_ thing. These don't need to be tested as + they are just building blocks for other functionality + which, itself, will be tested. + """ + import Ecto.Query, warn: false + + alias Pinchflat.Sources.Source + + # Prefixes: + # - for_* - belonging to a certain record + # - join_* - for joining on a certain record + # - with_* - for filtering based on full, concrete attributes + # - matching_* - for filtering based on partial attributes (e.g. LIKE, regex, full-text search) + # + # Suffixes: + # - _for - the arg passed is an association record + + def new do + Source + end + + def for_media_profile(query, media_profile) do + where(query, [s], s.media_profile_id == ^media_profile.id) + end +end diff --git a/lib/pinchflat_web/components/custom_components/tab_components.ex b/lib/pinchflat_web/components/custom_components/tab_components.ex index f8c5a71..fda08bd 100644 --- a/lib/pinchflat_web/components/custom_components/tab_components.ex +++ b/lib/pinchflat_web/components/custom_components/tab_components.ex @@ -34,7 +34,7 @@ defmodule PinchflatWeb.CustomComponents.TabComponents do <%= render_slot(@tab_append) %> -
+
<%= render_slot(tab) %>
diff --git a/lib/pinchflat_web/controllers/media_profiles/media_profile_controller.ex b/lib/pinchflat_web/controllers/media_profiles/media_profile_controller.ex index 4b130e7..cc638ec 100644 --- a/lib/pinchflat_web/controllers/media_profiles/media_profile_controller.ex +++ b/lib/pinchflat_web/controllers/media_profiles/media_profile_controller.ex @@ -1,12 +1,19 @@ defmodule PinchflatWeb.MediaProfiles.MediaProfileController do use PinchflatWeb, :controller + import Ecto.Query, warn: false + alias Pinchflat.Repo alias Pinchflat.Profiles + alias Pinchflat.Sources.SourcesQuery alias Pinchflat.Profiles.MediaProfile def index(conn, _params) do - media_profiles = Profiles.list_media_profiles() + media_profiles = + MediaProfile + |> order_by(asc: :name) + |> Repo.all() + render(conn, :index, media_profiles: media_profiles) end @@ -32,12 +39,15 @@ defmodule PinchflatWeb.MediaProfiles.MediaProfileController do end def show(conn, %{"id" => id}) do - media_profile = - id - |> Profiles.get_media_profile!() - |> Repo.preload(:sources) + media_profile = Profiles.get_media_profile!(id) - render(conn, :show, media_profile: media_profile) + sources = + SourcesQuery.new() + |> SourcesQuery.for_media_profile(media_profile) + |> order_by(asc: :custom_name) + |> Repo.all() + + render(conn, :show, media_profile: media_profile, sources: sources) end def edit(conn, %{"id" => id}) do diff --git a/lib/pinchflat_web/controllers/media_profiles/media_profile_html/show.html.heex b/lib/pinchflat_web/controllers/media_profiles/media_profile_html/show.html.heex index 009f2dd..62e8603 100644 --- a/lib/pinchflat_web/controllers/media_profiles/media_profile_html/show.html.heex +++ b/lib/pinchflat_web/controllers/media_profiles/media_profile_html/show.html.heex @@ -50,7 +50,7 @@
<:tab title="Sources"> - <.table rows={@media_profile.sources} table_class="text-black dark:text-white"> + <.table rows={@sources} table_class="text-black dark:text-white"> <:col :let={source} label="Name"> <.subtle_link href={~p"/sources/#{source.id}"}> <%= source.custom_name || source.collection_name %> diff --git a/lib/pinchflat_web/controllers/podcasts/podcast_controller.ex b/lib/pinchflat_web/controllers/podcasts/podcast_controller.ex index aa65c57..f59e579 100644 --- a/lib/pinchflat_web/controllers/podcasts/podcast_controller.ex +++ b/lib/pinchflat_web/controllers/podcasts/podcast_controller.ex @@ -2,7 +2,7 @@ defmodule PinchflatWeb.Podcasts.PodcastController do use PinchflatWeb, :controller alias Pinchflat.Repo - alias Pinchflat.Media + alias Pinchflat.Media.MediaQuery alias Pinchflat.Sources.Source alias Pinchflat.Podcasts.RssFeedBuilder alias Pinchflat.Podcasts.PodcastHelpers @@ -20,10 +20,15 @@ defmodule PinchflatWeb.Podcasts.PodcastController do def feed_image(conn, %{"uuid" => uuid}) do source = Repo.get_by!(Source, uuid: uuid) - # This provides a fallback image if the source has none. - # We only need one since we're using the internal metadata image which - # we know exists. - media_items = Media.list_downloaded_media_items_for(source, limit: 1) + + # This is used to fetch a fallback cover image + # if the source doesn't have any usable images + media_items = + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_filepath() + |> Repo.maybe_limit(1) + |> Repo.all() case PodcastHelpers.select_cover_image(source, media_items) do {:error, _} -> diff --git a/lib/pinchflat_web/controllers/sources/source_controller.ex b/lib/pinchflat_web/controllers/sources/source_controller.ex index 168c148..d8e904d 100644 --- a/lib/pinchflat_web/controllers/sources/source_controller.ex +++ b/lib/pinchflat_web/controllers/sources/source_controller.ex @@ -4,17 +4,21 @@ defmodule PinchflatWeb.Sources.SourceController do import Ecto.Query, warn: false alias Pinchflat.Repo - alias Pinchflat.Media alias Pinchflat.Tasks alias Pinchflat.Sources - alias Pinchflat.Profiles + alias Pinchflat.MediaQuery alias Pinchflat.Sources.Source alias Pinchflat.Media.MediaQuery + alias Pinchflat.Profiles.MediaProfile alias Pinchflat.Downloading.DownloadingHelpers alias Pinchflat.SlowIndexing.SlowIndexingHelpers def index(conn, _params) do - sources = Repo.preload(Sources.list_sources(), :media_profile) + sources = + Source + |> order_by(asc: :custom_name) + |> Repo.all() + |> Repo.preload(:media_profile) render(conn, :index, sources: sources) end @@ -56,8 +60,21 @@ defmodule PinchflatWeb.Sources.SourceController do |> Tasks.list_tasks_for(nil, [:executing, :available, :scheduled, :retryable]) |> Repo.preload(:job) - pending_media = Media.list_pending_media_items_for(source, limit: 100) - downloaded_media = Media.list_downloaded_media_items_for(source, limit: 100) + pending_media = + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_pending_download() + |> order_by(desc: :id) + |> limit(100) + |> Repo.all() + + downloaded_media = + MediaQuery.new() + |> MediaQuery.for_source(source) + |> MediaQuery.with_media_filepath() + |> order_by(desc: :id) + |> limit(100) + |> Repo.all() render(conn, :show, source: source, @@ -129,7 +146,9 @@ defmodule PinchflatWeb.Sources.SourceController do end defp media_profiles do - Profiles.list_media_profiles() + MediaProfile + |> order_by(asc: :name) + |> Repo.all() end defp total_downloaded_for(source) do diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index 3d46571..52eff4d 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -149,14 +149,6 @@ defmodule Pinchflat.MediaTest do assert Media.list_pending_media_items_for(source) == [] end - - test "optionally accepts a limit" do - source = source_fixture() - media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil}) - - assert Media.list_pending_media_items_for(source, limit: 1) == [media_item] - assert Media.list_pending_media_items_for(source, limit: 0) == [] - end end describe "list_pending_media_items_for/1 when testing shorts" do @@ -335,25 +327,6 @@ defmodule Pinchflat.MediaTest do end end - describe "list_downloaded_media_items_for/1" do - test "returns only media items with a media_filepath" do - source = source_fixture() - _media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil}) - media_item = media_item_fixture(%{source_id: source.id, media_filepath: "/video/#{Faker.File.file_name(:video)}"}) - - assert Media.list_downloaded_media_items_for(source) == [media_item] - end - - test "optionally accepts a limit" do - source = source_fixture() - _media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil}) - media_item = media_item_fixture(%{source_id: source.id, media_filepath: "/video/#{Faker.File.file_name(:video)}"}) - - assert Media.list_downloaded_media_items_for(source, limit: 1) == [media_item] - assert Media.list_downloaded_media_items_for(source, limit: 0) == [] - end - end - describe "pending_download?/1" do test "returns true when the media hasn't been downloaded" do media_item = media_item_fixture(%{media_filepath: nil})