From f899ea287d4dab8fd50a14ef016afd8f3ea2c8a5 Mon Sep 17 00:00:00 2001 From: Kieran Date: Tue, 2 Apr 2024 15:14:30 -0700 Subject: [PATCH] [Enhancement] Adds ability to stop media from re-downloading (#159) * Added column * Added methods for ignoring media items from future download * Added new deletion options to controller and UI * Added controller actions and UI for editing a media item --- lib/pinchflat/media/media.ex | 29 ++++++- lib/pinchflat/media/media_item.ex | 16 +++- lib/pinchflat/media/media_query.ex | 4 + .../media_items/media_item_controller.ex | 34 +++++--- .../media_items/media_item_html.ex | 8 ++ .../media_item_html/edit.html.heex | 13 ++++ .../media_item_html/media_item_form.html.heex | 24 ++++++ .../media_item_html/show.html.heex | 24 +++++- lib/pinchflat_web/router.ex | 2 +- ...17_add_prevent_download_to_media_items.exs | 9 +++ test/pinchflat/media_test.exs | 78 +++++++++++++++++++ .../media_item_controller_test.exs | 71 +++++++++++------ 12 files changed, 268 insertions(+), 44 deletions(-) create mode 100644 lib/pinchflat_web/controllers/media_items/media_item_html/edit.html.heex create mode 100644 lib/pinchflat_web/controllers/media_items/media_item_html/media_item_form.html.heex create mode 100644 priv/repo/migrations/20240402192417_add_prevent_download_to_media_items.exs diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index 346029b..4ab09d8 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -27,8 +27,8 @@ 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 `build_format_clauses` but tl;dr is it _may_ filter based - on shorts or livestreams depending on the media_profile settings. + See `matching_download_criteria_for` but tl;dr is it _may_ filter based + on shorts livestreams depending on the media_profile settings. Returns [%MediaItem{}, ...]. """ @@ -161,7 +161,7 @@ defmodule Pinchflat.Media do Tasks.delete_tasks_for(media_item) if delete_files do - {:ok, _} = delete_media_files(media_item) + {:ok, _} = do_delete_media_files(media_item) end # Should delete these no matter what @@ -169,6 +169,26 @@ defmodule Pinchflat.Media do Repo.delete(media_item) end + @doc """ + Deletes the tasks and media files associated with a media_item but leaves the + media_item in the database. Does not delete anything to do with associated metadata. + + ## Options: + - `:prevent_download` - If `true`, the media_item will be marked to prevent being redownloaded + + Returns {:ok, %MediaItem{}} | {:error, %Ecto.Changeset{}} + """ + def delete_media_files(%MediaItem{} = media_item, opts \\ []) do + prevent_download = Keyword.get(opts, :prevent_download, false) + filepath_attrs = MediaItem.filepath_attribute_defaults() + opt_attrs = %{prevent_download: prevent_download} + + Tasks.delete_tasks_for(media_item) + {:ok, _} = do_delete_media_files(media_item) + + update_media_item(media_item, Map.merge(filepath_attrs, opt_attrs)) + end + @doc """ Returns an `%Ecto.Changeset{}` for tracking media_item changes. """ @@ -176,7 +196,7 @@ defmodule Pinchflat.Media do MediaItem.changeset(media_item, attrs) end - defp delete_media_files(media_item) do + defp do_delete_media_files(media_item) do mapped_struct = Map.from_struct(media_item) MediaItem.filepath_attributes() @@ -203,6 +223,7 @@ defmodule Pinchflat.Media do 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) diff --git a/lib/pinchflat/media/media_item.ex b/lib/pinchflat/media/media_item.ex index 40353db..04e7038 100644 --- a/lib/pinchflat/media/media_item.ex +++ b/lib/pinchflat/media/media_item.ex @@ -30,7 +30,9 @@ defmodule Pinchflat.Media.MediaItem do :subtitle_filepaths, :thumbnail_filepath, :metadata_filepath, - :nfo_filepath + :nfo_filepath, + # These are user or system controlled fields + :prevent_download ] # Pretty much all the fields captured at index are required. @required_fields ~w( @@ -70,6 +72,8 @@ defmodule Pinchflat.Media.MediaItem do # Will very likely revisit because I can't leave well-enough alone. field :subtitle_filepaths, {:array, {:array, :string}}, default: [] + field :prevent_download, :boolean, default: false + field :matching_search_term, :string, virtual: true belongs_to :source, Source @@ -96,4 +100,14 @@ defmodule Pinchflat.Media.MediaItem do def filepath_attributes do ~w(media_filepath thumbnail_filepath metadata_filepath subtitle_filepaths nfo_filepath)a end + + @doc false + def filepath_attribute_defaults do + filepath_attributes() + |> Enum.map(fn + :subtitle_filepaths -> {:subtitle_filepaths, []} + field -> {field, nil} + end) + |> Enum.into(%{}) + end end diff --git a/lib/pinchflat/media/media_query.ex b/lib/pinchflat/media/media_query.ex index c43b5eb..30960d6 100644 --- a/lib/pinchflat/media/media_query.ex +++ b/lib/pinchflat/media/media_query.ex @@ -53,6 +53,10 @@ defmodule Pinchflat.Media.MediaQuery do where(query, [mi], mi.upload_date >= ^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 diff --git a/lib/pinchflat_web/controllers/media_items/media_item_controller.ex b/lib/pinchflat_web/controllers/media_items/media_item_controller.ex index 31a9ef4..d054564 100644 --- a/lib/pinchflat_web/controllers/media_items/media_item_controller.ex +++ b/lib/pinchflat_web/controllers/media_items/media_item_controller.ex @@ -16,20 +16,34 @@ defmodule PinchflatWeb.MediaItems.MediaItemController do render(conn, :show, media_item: media_item) end - def delete(conn, %{"id" => id} = params) do - delete_files = Map.get(params, "delete_files", false) + def edit(conn, %{"id" => id}) do media_item = Media.get_media_item!(id) - {:ok, _} = Media.delete_media_item(media_item, delete_files: delete_files) + changeset = Media.change_media_item(media_item) - flash_message = - if delete_files do - "Record and files deleted successfully." - else - "Record deleted successfully. Files were not deleted." - end + render(conn, :edit, media_item: media_item, changeset: changeset) + end + + def update(conn, %{"id" => id, "media_item" => params}) do + media_item = Media.get_media_item!(id) + + case Media.update_media_item(media_item, params) do + {:ok, media_item} -> + conn + |> put_flash(:info, "Media Item updated successfully.") + |> redirect(to: ~p"/sources/#{media_item.source_id}/media/#{media_item}") + + {:error, %Ecto.Changeset{} = changeset} -> + render(conn, :edit, media_item: media_item, changeset: changeset) + end + end + + def delete(conn, %{"id" => id} = params) do + prevent_download = Map.get(params, "prevent_download", false) + media_item = Media.get_media_item!(id) + {:ok, _} = Media.delete_media_files(media_item, prevent_download: prevent_download) conn - |> put_flash(:info, flash_message) + |> put_flash(:info, "Files deleted successfully.") |> redirect(to: ~p"/sources/#{media_item.source_id}") end diff --git a/lib/pinchflat_web/controllers/media_items/media_item_html.ex b/lib/pinchflat_web/controllers/media_items/media_item_html.ex index 12d7ffa..659e742 100644 --- a/lib/pinchflat_web/controllers/media_items/media_item_html.ex +++ b/lib/pinchflat_web/controllers/media_items/media_item_html.ex @@ -3,6 +3,14 @@ defmodule PinchflatWeb.MediaItems.MediaItemHTML do embed_templates "media_item_html/*" + @doc """ + Renders a media item form. + """ + attr :changeset, Ecto.Changeset, required: true + attr :action, :string, required: true + + def media_item_form(assigns) + def media_file_exists?(media_item) do !!media_item.media_filepath and File.exists?(media_item.media_filepath) end diff --git a/lib/pinchflat_web/controllers/media_items/media_item_html/edit.html.heex b/lib/pinchflat_web/controllers/media_items/media_item_html/edit.html.heex new file mode 100644 index 0000000..5b1818c --- /dev/null +++ b/lib/pinchflat_web/controllers/media_items/media_item_html/edit.html.heex @@ -0,0 +1,13 @@ +
+

+ Editing "<%= StringUtils.truncate(@media_item.title, 35) %>" +

+
+ +
+
+
+ <.media_item_form changeset={@changeset} action={~p"/sources/#{@media_item.source_id}/media/#{@media_item}"} /> +
+
+
diff --git a/lib/pinchflat_web/controllers/media_items/media_item_html/media_item_form.html.heex b/lib/pinchflat_web/controllers/media_items/media_item_html/media_item_form.html.heex new file mode 100644 index 0000000..acfa301 --- /dev/null +++ b/lib/pinchflat_web/controllers/media_items/media_item_html/media_item_form.html.heex @@ -0,0 +1,24 @@ +<.simple_form + :let={f} + for={@changeset} + action={@action} + x-data="{ advancedMode: !!JSON.parse(localStorage.getItem('advancedMode')) }" + x-init="$watch('advancedMode', value => localStorage.setItem('advancedMode', JSON.stringify(value)))" +> + <.error :if={@changeset.action}> + Oops, something went wrong! Please check the errors below. + + +

+ General Options +

+ + <.input + field={f[:prevent_download]} + type="toggle" + label="Prevent Download" + help="Checking excludes this media item from being downloaded" + /> + + <.button class="my-10 sm:mb-7.5 w-full sm:w-auto" rounding="rounded-lg">Save Media Item + diff --git a/lib/pinchflat_web/controllers/media_items/media_item_html/show.html.heex b/lib/pinchflat_web/controllers/media_items/media_item_html/show.html.heex index fb81b1a..6de101f 100644 --- a/lib/pinchflat_web/controllers/media_items/media_item_html/show.html.heex +++ b/lib/pinchflat_web/controllers/media_items/media_item_html/show.html.heex @@ -4,9 +4,17 @@ <.icon name="hero-arrow-left" class="w-10 h-10 hover:dark:text-white" />

- Media Item #<%= @media_item.id %> + <%= StringUtils.truncate(@media_item.title, 35) %>

+ +
@@ -15,13 +23,22 @@ <.button_dropdown text="Actions" class="justify-center w-full sm:w-50"> <:option> <.link - href={~p"/sources/#{@media_item.source_id}/media/#{@media_item}?delete_files=true"} + href={~p"/sources/#{@media_item.source_id}/media/#{@media_item}"} method="delete" - data-confirm="Are you sure you want to delete this record and all associated files on disk? This cannot be undone." + data-confirm="Are you sure you want to delete all files for this media item? This cannot be undone." > Delete Files + <:option> + <.link + href={~p"/sources/#{@media_item.source_id}/media/#{@media_item}?prevent_download=true"} + method="delete" + data-confirm="Are you sure you want to delete all files for this media item and prevent it from re-downloading in the future? This cannot be undone." + > + Delete and Ignore + + @@ -32,6 +49,7 @@ <.media_preview media_item={@media_item} /> <% end %> +

<%= @media_item.title %>

Attributes

Source: diff --git a/lib/pinchflat_web/router.ex b/lib/pinchflat_web/router.ex index 202f460..e56a4f2 100644 --- a/lib/pinchflat_web/router.ex +++ b/lib/pinchflat_web/router.ex @@ -32,7 +32,7 @@ defmodule PinchflatWeb.Router do resources "/search", Searches.SearchController, only: [:show], singleton: true resources "/sources", Sources.SourceController do - resources "/media", MediaItems.MediaItemController, only: [:show, :delete] + resources "/media", MediaItems.MediaItemController, only: [:show, :edit, :update, :delete] end end diff --git a/priv/repo/migrations/20240402192417_add_prevent_download_to_media_items.exs b/priv/repo/migrations/20240402192417_add_prevent_download_to_media_items.exs new file mode 100644 index 0000000..c43f91f --- /dev/null +++ b/priv/repo/migrations/20240402192417_add_prevent_download_to_media_items.exs @@ -0,0 +1,9 @@ +defmodule Pinchflat.Repo.Migrations.AddPreventDownloadToMediaItems do + use Ecto.Migration + + def change do + alter table(:media_items) do + add :prevent_download, :boolean, default: false, null: false + end + end +end diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index c73c421..cc435b8 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -233,6 +233,16 @@ defmodule Pinchflat.MediaTest do end end + describe "list_pending_media_items_for/1 when testing download prevention" do + test "returns only media items that are not prevented from downloading" do + source = source_fixture() + _prevented_media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, prevent_download: true}) + media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, prevent_download: false}) + + assert Media.list_pending_media_items_for(source) == [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() @@ -320,6 +330,18 @@ defmodule Pinchflat.MediaTest do assert Media.pending_download?(media_item) end + + test "returns true if the media item is not prevented from downloading" do + media_item = media_item_fixture(%{media_filepath: nil, prevent_download: false}) + + assert Media.pending_download?(media_item) + end + + test "returns false if the media item is prevented from downloading" do + media_item = media_item_fixture(%{media_filepath: nil, prevent_download: true}) + + refute Media.pending_download?(media_item) + end end describe "search/1" do @@ -587,6 +609,62 @@ defmodule Pinchflat.MediaTest do end end + describe "delete_media_files/2" do + test "does not delete the media_item" do + media_item = media_item_fixture() + + assert {:ok, %MediaItem{}} = Media.delete_media_files(media_item) + assert Repo.reload!(media_item) + end + + test "deletes attached tasks" do + media_item = media_item_fixture() + task = task_fixture(%{media_item_id: media_item.id}) + + assert {:ok, %MediaItem{}} = Media.delete_media_files(media_item) + assert_raise Ecto.NoResultsError, fn -> Repo.reload!(task) end + end + + test "deletes the media_item's files" do + media_item = media_item_with_attachments() + + assert {:ok, _} = Media.delete_media_files(media_item) + refute File.exists?(media_item.media_filepath) + end + + test "does not delete the media item's metadata files" do + stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) + media_item = Repo.preload(media_item_with_attachments(), :metadata) + + update_attrs = %{ + metadata: %{ + metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_item, %{}), + thumbnail_filepath: + MetadataFileHelpers.download_and_store_thumbnail_for(media_item, %{ + "thumbnail" => "https://example.com/thumbnail.jpg" + }) + } + } + + {:ok, updated_media_item} = Media.update_media_item(media_item, update_attrs) + metadata = Repo.preload(updated_media_item, :metadata).metadata + + assert {:ok, _} = Media.delete_media_files(updated_media_item) + assert Repo.reload(metadata) + assert File.exists?(updated_media_item.metadata.metadata_filepath) + + # cleanup + Media.delete_media_item(updated_media_item, delete_files: true) + end + + test "can prevent the media item from being downloaded" do + media_item = media_item_with_attachments() + + assert {:ok, updated_media_item} = Media.delete_media_files(media_item, prevent_download: true) + assert updated_media_item.prevent_download + end + end + describe "change_media_item/1" do test "change_media_item/1 returns a media_item changeset" do media_item = media_item_fixture() diff --git a/test/pinchflat_web/controllers/media_item_controller_test.exs b/test/pinchflat_web/controllers/media_item_controller_test.exs index 428d337..e8e035a 100644 --- a/test/pinchflat_web/controllers/media_item_controller_test.exs +++ b/test/pinchflat_web/controllers/media_item_controller_test.exs @@ -10,27 +10,58 @@ defmodule PinchflatWeb.MediaItemControllerTest do test "renders the page", %{conn: conn, media_item: media_item} do conn = get(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item}") - assert html_response(conn, 200) =~ "Media Item ##{media_item.id}" + + assert html_response(conn, 200) =~ "#{media_item.title}" end end - describe "delete media when just deleting the records" do + describe "edit media" do + setup [:create_media_item] + + test "renders form for editing chosen media_item", %{conn: conn, media_item: media_item} do + conn = get(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item}/edit") + + assert html_response(conn, 200) =~ "Editing" + end + end + + describe "update media" do + setup [:create_media_item] + + test "redirects when data is valid", %{conn: conn, media_item: media_item} do + update_attrs = %{title: "New Title"} + + conn = put(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item}", media_item: update_attrs) + assert redirected_to(conn) == ~p"/sources/#{media_item.source_id}/media/#{media_item}" + + conn = get(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item}") + assert html_response(conn, 200) =~ update_attrs[:title] + end + + test "renders errors when data is invalid", %{conn: conn, media_item: media_item} do + conn = put(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item}", media_item: %{title: nil}) + + assert html_response(conn, 200) =~ "Editing" + end + end + + describe "delete media" do setup do media_item = media_item_with_attachments() %{media_item: media_item} end - test "the media item is deleted", %{conn: conn, media_item: media_item} do + test "the media item not is deleted", %{conn: conn, media_item: media_item} do delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}") - assert_raise Ecto.NoResultsError, fn -> Repo.reload!(media_item) end + assert Repo.reload!(media_item) end - test "the files are not deleted", %{conn: conn, media_item: media_item} do + test "the files are deleted", %{conn: conn, media_item: media_item} do delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}") - assert File.exists?(media_item.media_filepath) + refute File.exists?(media_item.media_filepath) end test "redirects to the source page", %{conn: conn, media_item: media_item} do @@ -38,31 +69,21 @@ defmodule PinchflatWeb.MediaItemControllerTest do assert redirected_to(conn) == ~p"/sources/#{media_item.source_id}" end - end - describe "delete media when deleting the records and files" do - setup do - media_item = media_item_with_attachments() + test "doesn't prevent re-download by default", %{conn: conn, media_item: media_item} do + delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}") - %{media_item: media_item} + media_item = Repo.reload(media_item) + + refute media_item.prevent_download end - test "the media item is deleted", %{conn: conn, media_item: media_item} do - delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}?delete_files=true") + test "can optionally prevent re-download", %{conn: conn, media_item: media_item} do + delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}?prevent_download=true") - assert_raise Ecto.NoResultsError, fn -> Repo.reload!(media_item) end - end + media_item = Repo.reload(media_item) - test "the files are deleted", %{conn: conn, media_item: media_item} do - delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}?delete_files=true") - - refute File.exists?(media_item.media_filepath) - end - - test "redirects to the source page", %{conn: conn, media_item: media_item} do - conn = delete(conn, ~p"/sources/#{media_item.source_id}/media/#{media_item.id}?delete_files=true") - - assert redirected_to(conn) == ~p"/sources/#{media_item.source_id}" + assert media_item.prevent_download end end