UX improvements v1 (#39)

* Removed collection_type user input instead inferring from yt-dlp response

* Updated docs

* Added delete buttons for source; Refactored the way deletion methods work

* Update source to always run an initial index

* Added deletion to the last models

* Improved clarity around deletion operation

* Improved task fetching and deletion methods

* More tests

lib/pinchflat/media.ex

@@ -12,12 +12,25 @@ defmodule Pinchflat.Media do
   alias Pinchflat.Media.MediaMetadata
 
   @doc """
-  Returns the list of media_items. Returns [%MediaItem{}, ...].
+  Returns the list of media_items.
+
+  Returns [%MediaItem{}, ...].
   """
   def list_media_items do
     Repo.all(MediaItem)
   end
 
+  @doc """
+  Returns a list of media_items for a given source.
+
+  Returns [%MediaItem{}, ...].
+  """
+  def list_media_items_for(%Source{} = source) do
+    MediaItem
+    |> where([mi], mi.source_id == ^source.id)
+    |> Repo.all()
+  end
+
   @doc """
   Returns a list of pending media_items for a given source, where
   pending means the `media_filepath` is `nil` AND the media_item
@@ -138,26 +151,30 @@ defmodule Pinchflat.Media do
   end
 
   @doc """
-  Deletes a media_item and its associated tasks. Will leave files on disk.
+  Deletes a media_item and its associated tasks.
+  Can optionally delete the media_item's files.
 
   Returns {:ok, %MediaItem{}} | {:error, %Ecto.Changeset{}}.
   """
-  def delete_media_item(%MediaItem{} = media_item) do
+  def delete_media_item(%MediaItem{} = media_item, opts \\ []) do
+    delete_files = Keyword.get(opts, :delete_files, false)
+
+    if delete_files do
+      {:ok, _} = delete_all_attachments(media_item)
+    end
+
     Tasks.delete_tasks_for(media_item)
     Repo.delete(media_item)
   end
 
   @doc """
-  Deletes the media_item's associated files. Will leave the media_item in the database.
-
-  NOTE: this deletes the metadata files as well, but maybe it shouldn't? I'm wondering if
-  the metadata is more a concern of the DB record itself and should be lumped in with those
-  delete operations. But the metadata does come from the download operation of the file.
-  Food for thought but not a priority at the moment.
-
-  Returns {:ok, %MediaItem{}}
+  Returns an `%Ecto.Changeset{}` for tracking media_item changes.
   """
-  def delete_attachments(media_item) do
+  def change_media_item(%MediaItem{} = media_item, attrs \\ %{}) do
+    MediaItem.changeset(media_item, attrs)
+  end
+
+  defp delete_all_attachments(media_item) do
     media_item = Repo.preload(media_item, :metadata)
 
     media_item
@@ -177,25 +194,6 @@ defmodule Pinchflat.Media do
     {:ok, media_item}
   end
 
-  @doc """
-  Deletes the media_item and all associated files. Attempts to delete the root directory
-  but only if it is empty.
-
-  Returns {:ok, %MediaItem{}}
-  """
-  def delete_media_item_and_attachments(media_item) do
-    {:ok, _} = delete_attachments(media_item)
-
-    delete_media_item(media_item)
-  end
-
-  @doc """
-  Returns an `%Ecto.Changeset{}` for tracking media_item changes.
-  """
-  def change_media_item(%MediaItem{} = media_item, attrs \\ %{}) do
-    MediaItem.changeset(media_item, attrs)
-  end
-
   defp build_format_clauses(media_profile) do
     mapped_struct = Map.from_struct(media_profile)
 

lib/pinchflat/media_source/source.ex

@@ -27,6 +27,7 @@ defmodule Pinchflat.Sources.Source do
     collection_id
     collection_type
     friendly_name
+    index_frequency_minutes
     download_media
     original_url
     media_profile_id

lib/pinchflat/profiles.ex

@@ -4,12 +4,15 @@ defmodule Pinchflat.Profiles do
   """
 
   import Ecto.Query, warn: false
-  alias Pinchflat.Repo
 
+  alias Pinchflat.Repo
+  alias Pinchflat.Sources
   alias Pinchflat.Profiles.MediaProfile
 
   @doc """
-  Returns the list of media_profiles. Returns [%MediaProfile{}, ...]
+  Returns the list of media_profiles.
+
+  Returns [%MediaProfile{}, ...]
   """
   def list_media_profiles do
     Repo.all(MediaProfile)
@@ -23,7 +26,9 @@ defmodule Pinchflat.Profiles do
   def get_media_profile!(id), do: Repo.get!(MediaProfile, id)
 
   @doc """
-  Creates a media_profile. Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}}
+  Creates a media_profile.
+
+  Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}}
   """
   def create_media_profile(attrs) do
     %MediaProfile{}
@@ -32,7 +37,9 @@ defmodule Pinchflat.Profiles do
   end
 
   @doc """
-  Updates a media_profile. Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}}
+  Updates a media_profile.
+
+  Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}}
   """
   def update_media_profile(%MediaProfile{} = media_profile, attrs) do
     media_profile
@@ -41,14 +48,25 @@ defmodule Pinchflat.Profiles do
   end
 
   @doc """
-  Deletes a media_profile. Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}}
+  Deletes a media_profile, all its sources, and all their media items.
+  Can optionally delete the media files.
+
+  Returns {:ok, %MediaProfile{}} | {:error, %Ecto.Changeset{}}
   """
-  def delete_media_profile(%MediaProfile{} = media_profile) do
+  def delete_media_profile(%MediaProfile{} = media_profile, opts \\ []) do
+    delete_files = Keyword.get(opts, :delete_files, false)
+
+    media_profile
+    |> Sources.list_sources_for()
+    |> Enum.each(fn source ->
+      Sources.delete_source(source, delete_files: delete_files)
+    end)
+
     Repo.delete(media_profile)
   end
 
   @doc """
-  Returns an `%Ecto.Changeset{}` for tracking media_profile changes.
+  Returns `%Ecto.Changeset{}`
   """
   def change_media_profile(%MediaProfile{} = media_profile, attrs \\ %{}) do
     MediaProfile.changeset(media_profile, attrs)

lib/pinchflat/sources.ex

@@ -6,9 +6,11 @@ defmodule Pinchflat.Sources do
   import Ecto.Query, warn: false
   alias Pinchflat.Repo
 
+  alias Pinchflat.Media
   alias Pinchflat.Tasks
-  alias Pinchflat.Tasks.SourceTasks
   alias Pinchflat.Sources.Source
+  alias Pinchflat.Tasks.SourceTasks
+  alias Pinchflat.Profiles.MediaProfile
   alias Pinchflat.MediaClient.SourceDetails
 
   @doc """
@@ -18,6 +20,15 @@ defmodule Pinchflat.Sources do
     Repo.all(Source)
   end
 
+  @doc """
+  Returns the list of sources for a media_profile.
+
+  Returns [%Source{}, ...]
+  """
+  def list_sources_for(%MediaProfile{} = media_profile) do
+    Repo.all(from s in Source, where: s.media_profile_id == ^media_profile.id)
+  end
+
   @doc """
   Gets a single source.
 
@@ -55,13 +66,20 @@ defmodule Pinchflat.Sources do
   end
 
   @doc """
-  Deletes a source and it's associated tasks (of any state).
-  NOTE: will fail if the source has associated media items. Intended
-  for now, will almost certainly change in the future.
+  Deletes a source, its media items, and its associated tasks (of any state).
+  Can optionally delete the source's media files.
 
   Returns {:ok, %Source{}} | {:error, %Ecto.Changeset{}}
   """
-  def delete_source(%Source{} = source) do
+  def delete_source(%Source{} = source, opts \\ []) do
+    delete_files = Keyword.get(opts, :delete_files, false)
+
+    source
+    |> Media.list_media_items_for()
+    |> Enum.each(fn media_item ->
+      Media.delete_media_item(media_item, delete_files: delete_files)
+    end)
+
     Tasks.delete_tasks_for(source)
     Repo.delete(source)
   end
@@ -84,10 +102,6 @@ defmodule Pinchflat.Sources do
 
   NOTE: When operating in the ideal path, this effectively adds an API call
   to the source creation/update process. Should be used only when needed.
-
-  IDEA: Maybe I could discern `collection_type` based on the original URL?
-  It also seems like it's a channel when the returned yt-dlp channel_id is the
-  same as the playlist_id - maybe could use that?
   """
   def change_source_from_url(%Source{} = source, attrs) do
     case change_source(source, attrs) do
@@ -118,24 +132,20 @@ defmodule Pinchflat.Sources do
 
   defp add_source_details_by_collection_type(source, changeset, source_details) do
     %Ecto.Changeset{changes: changes} = changeset
-    collection_type = Ecto.Changeset.get_field(changeset, :collection_type)
 
     collection_changes =
-      case collection_type do
-        :channel ->
-          %{
-            collection_id: source_details.channel_id,
-            collection_name: source_details.channel_name
-          }
-
-        :playlist ->
-          %{
-            collection_id: source_details.playlist_id,
-            collection_name: source_details.playlist_name
-          }
-
-        _ ->
-          %{}
+      if source_details.playlist_id == source_details.channel_id do
+        %{
+          collection_type: :channel,
+          collection_id: source_details.channel_id,
+          collection_name: source_details.channel_name
+        }
+      else
+        %{
+          collection_type: :playlist,
+          collection_id: source_details.playlist_id,
+          collection_name: source_details.playlist_name
+        }
       end
 
     change_source(source, Map.merge(changes, collection_changes))
@@ -169,21 +179,19 @@ defmodule Pinchflat.Sources do
     {:ok, source}
   end
 
-  # IDEA: this uses a pattern where `kickoff_indexing_task` controls whether
-  # it should run based on the source, but `maybe_handle_media_tasks` handles that
-  # logic itself. Consider updating one or the other to be consistent (once I've
-  # decided which I like more)
   defp maybe_run_indexing_task(changeset, source) do
     case changeset.data do
       # If the changeset is new (not persisted), attempt indexing no matter what
       %{__meta__: %{state: :built}} ->
         SourceTasks.kickoff_indexing_task(source)
 
-      # If the record has been persisted, only attempt indexing if the
-      # indexing frequency has been changed
+      # If the record has been persisted, only run indexing if the
+      # indexing frequency has been changed and is now greater than 0
       %{__meta__: %{state: :loaded}} ->
-        if Map.has_key?(changeset.changes, :index_frequency_minutes) do
-          SourceTasks.kickoff_indexing_task(source)
+        case changeset.changes do
+          %{index_frequency_minutes: mins} when mins > 0 -> SourceTasks.kickoff_indexing_task(source)
+          %{index_frequency_minutes: _} -> Tasks.delete_pending_tasks_for(source, "MediaIndexingWorker")
+          _ -> :ok
         end
     end
 

lib/pinchflat/tasks.ex

@@ -19,30 +19,46 @@ defmodule Pinchflat.Tasks do
 
   @doc """
   Returns the list of tasks for a given record type and ID. Optionally allows you to specify
-  which job states to include.
+  which worker or job states to include.
 
   Returns [%Task{}, ...]
   """
-  def list_tasks_for(attached_record_type, attached_record_id, job_states \\ Oban.Job.states()) do
+  def list_tasks_for(attached_record_type, attached_record_id, worker_name \\ nil, job_states \\ Oban.Job.states()) do
     stringified_states = Enum.map(job_states, &to_string/1)
 
+    worker_name_finder =
+      if worker_name do
+        # Workers are the full module name - we want to match on the string ENDING with
+        # the passed worker name and it should be preceeded with a . so we aren't matching
+        # on a substring. You can pass in more fragments of the worker name if you need
+        # to disambiguate. eg: "TestWorker" or "FooBar.TestWorker"
+        worker_finder = "%.#{worker_name}"
+
+        dynamic([_t, j], fragment("? LIKE ?", j.worker, ^worker_finder))
+      else
+        true
+      end
+
     Repo.all(
       from t in Task,
         join: j in assoc(t, :job),
         where: field(t, ^attached_record_type) == ^attached_record_id,
+        where: ^worker_name_finder,
         where: j.state in ^stringified_states
     )
   end
 
   @doc """
-  Returns the list of pending tasks for a given record type and ID.
+  Returns the list of pending tasks for a given record type and ID. Optionally allows you to specify
+  which worker to include.
 
   Returns [%Task{}, ...]
   """
-  def list_pending_tasks_for(attached_record_type, attached_record_id) do
+  def list_pending_tasks_for(attached_record_type, attached_record_id, worker_name \\ nil) do
     list_tasks_for(
       attached_record_type,
       attached_record_id,
+      worker_name,
       [:available, :scheduled, :retryable]
     )
   end
@@ -107,14 +123,15 @@ defmodule Pinchflat.Tasks do
 
   @doc """
   Deletes all tasks attached to a given record, cancelling any attached jobs.
+  Optionally allows you to specify which worker to include.
 
   Returns :ok
   """
-  def delete_tasks_for(attached_record) do
+  def delete_tasks_for(attached_record, worker_name \\ nil) do
     tasks =
       case attached_record do
-        %Source{} = source -> list_tasks_for(:source_id, source.id)
-        %MediaItem{} = media_item -> list_tasks_for(:media_item_id, media_item.id)
+        %Source{} = source -> list_tasks_for(:source_id, source.id, worker_name)
+        %MediaItem{} = media_item -> list_tasks_for(:media_item_id, media_item.id, worker_name)
       end
 
     Enum.each(tasks, &delete_task/1)
@@ -122,14 +139,15 @@ defmodule Pinchflat.Tasks do
 
   @doc """
   Deletes all _pending_ tasks attached to a given record, cancelling any attached jobs.
+  Optionally allows you to specify which worker to include.
 
   Returns :ok
   """
-  def delete_pending_tasks_for(attached_record) do
+  def delete_pending_tasks_for(attached_record, worker_name \\ nil) do
     tasks =
       case attached_record do
-        %Source{} = source -> list_pending_tasks_for(:source_id, source.id)
-        %MediaItem{} = media_item -> list_pending_tasks_for(:media_item_id, media_item.id)
+        %Source{} = source -> list_pending_tasks_for(:source_id, source.id, worker_name)
+        %MediaItem{} = media_item -> list_pending_tasks_for(:media_item_id, media_item.id, worker_name)
       end
 
     Enum.each(tasks, &delete_task/1)

lib/pinchflat/tasks/source_tasks.ex

@@ -12,26 +12,23 @@ defmodule Pinchflat.Tasks.SourceTasks do
   alias Pinchflat.Workers.VideoDownloadWorker
 
   @doc """
-  Starts tasks for indexing a source's media.
+  Starts tasks for indexing a source's media regardless of the source's indexing
+  frequency. It's assumed the caller will check for that.
 
-  Returns {:ok, :should_not_index} | {:ok, %Task{}}.
+  Returns {:ok, %Task{}}.
   """
   def kickoff_indexing_task(%Source{} = source) do
-    Tasks.delete_pending_tasks_for(source)
+    Tasks.delete_pending_tasks_for(source, "MediaIndexingWorker")
 
-    if source.index_frequency_minutes <= 0 do
-      {:ok, :should_not_index}
-    else
-      source
-      |> Map.take([:id])
-      # Schedule this one immediately, but future ones will be on an interval
-      |> MediaIndexingWorker.new()
-      |> Tasks.create_job_with_task(source)
-      |> case do
-        # This should never return {:error, :duplicate_job} since we just deleted
-        # any pending tasks. I'm being assertive about it so it's obvious if I'm wrong
-        {:ok, task} -> {:ok, task}
-      end
+    source
+    |> Map.take([:id])
+    # Schedule this one immediately, but future ones will be on an interval
+    |> MediaIndexingWorker.new()
+    |> Tasks.create_job_with_task(source)
+    |> case do
+      # This should never return {:error, :duplicate_job} since we just deleted
+      # any pending tasks. I'm being assertive about it so it's obvious if I'm wrong
+      {:ok, task} -> {:ok, task}
     end
   end