[Enhancement] Improve ordering of models (#164)

* [WIP] ordering app queries

* Refactored media queries to be self-contained

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
+  def list_pending_media_items_for(%Source{} = source) do
-    limit = Keyword.get(opts, :limit, nil)
-    source = Repo.preload(source, :media_profile)
-
     MediaQuery.new()
     |> MediaQuery.for_source(source)
-    |> matching_download_criteria_for(source)
+    |> MediaQuery.with_media_pending_download()
-    |> 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)
     |> 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

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
+  to compose queries. Each method should strive to do
-  strive to do _one_ thing. These don't need to be tested
+  _one_ thing. These don't need to be tested as
-  as they are just building blocks for other functionality
+  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
-      query,
+    |> require_assoc(:source)
-      [mi, sources],
+    |> where(
+      [mi, source],
       fragment(
         "IFNULL(?, 0) > 0 AND DATETIME('now', '-' || ? || ' day') > ?",
-        sources.retention_period_days,
+        source.retention_period_days,
-        sources.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_source_cutoff(query) do
-
+    query
-  def with_upload_date_after(query, date) do
+    |> require_assoc(:source)
-    where(query, [mi], mi.upload_date >= ^date)
+    |> 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_source_title_regex(query) do
-
+    query
-  def matching_title_regex(query, regex) do
+    |> require_assoc(:source)
-    where(query, [mi], fragment("regexp_like(?, ?)", mi.title, ^regex))
+    |> 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
+  def with_format_matching_profile_preference(query) do
-  # takes a media_profile struct instead of taking just the attributes it
+    query
-  # cares about. Consider refactoring but low priority.
+    |> require_assoc(:media_profile)
-  def with_format_preference(query, media_profile) do
+    |> where(
-    mapped_struct = Map.from_struct(media_profile)
+      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 =
+  def with_media_pending_download(query) do
-      Enum.reduce(mapped_struct, dynamic(true), fn attr, dynamic ->
+    query
-        case {attr, media_profile} do
+    |> with_no_prevented_download()
-          {{:shorts_behaviour, :only}, %{livestream_behaviour: :only}} ->
+    |> with_no_media_filepath()
-            dynamic(
+    |> with_upload_date_after_source_cutoff()
-              [mi],
+    |> with_format_matching_profile_preference()
-              ^dynamic and (mi.livestream == true or mi.short_form_content == true)
+    |> matching_source_title_regex()
-            )
+  end
 
-          # Technically redundant, but makes the other clauses easier to parse
+  defp require_assoc(query, identifier) do
-          # (redundant because this condition is the same as the condition above, just flipped)
+    if has_named_binding?(query, identifier) do
-          {{:livestream_behaviour, :only}, %{shorts_behaviour: :only}} ->
+      query
-            dynamic
+    else
+      do_require_assoc(query, identifier)
+    end
+  end
 
-          {{:shorts_behaviour, :only}, _} ->
+  defp do_require_assoc(query, :source) do
-            dynamic([mi], ^dynamic and mi.short_form_content == true)
+    from(mi in query, join: s in assoc(mi, :source), as: :source)
+  end
 
-          {{:livestream_behaviour, :only}, _} ->
+  defp do_require_assoc(query, :media_profile) do
-            dynamic([mi], ^dynamic and mi.livestream == true)
+    query
-
+    |> require_assoc(:source)
-          {{:shorts_behaviour, :exclude}, %{livestream_behaviour: lb}} when lb != :only ->
+    |> join(:inner, [mi, source], mp in assoc(source, :media_profile), as: :media_profile)
-            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)
   end
 end

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
+    MediaQuery.new()
-    |> Media.list_downloaded_media_items_for(limit: limit)
+    |> 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
 

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

lib/pinchflat_web/components/custom_components/tab_components.ex

@@ -34,7 +34,7 @@ defmodule PinchflatWeb.CustomComponents.TabComponents do
           <%= render_slot(@tab_append) %>
         </div>
       </header>
-      <div class="mt-4 min-h-96">
+      <div class="mt-4 min-h-60">
         <div :for={{tab, idx} <- Enum.with_index(@tab)} x-show={"openTab === #{idx}"} class="font-medium leading-relaxed">
           <%= render_slot(tab) %>
         </div>

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 =
+    media_profile = Profiles.get_media_profile!(id)
-      id
-      |> Profiles.get_media_profile!()
-      |> Repo.preload(:sources)
 
-    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

lib/pinchflat_web/controllers/media_profiles/media_profile_html/show.html.heex

@@ -50,7 +50,7 @@
         </div>
       </:tab>
       <: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 %>

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
+    # This is used to fetch a fallback cover image
-    # we know exists.
+    # if the source doesn't have any usable images
-    media_items = Media.list_downloaded_media_items_for(source, limit: 1)
+    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, _} ->

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)
+    pending_media =
-    downloaded_media = Media.list_downloaded_media_items_for(source, limit: 100)
+      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

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})