From 632093611e30148128a1195c45e68a06a170080e Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Thu, 14 Mar 2024 15:24:58 -0700 Subject: [PATCH] Refactored file deletion for media items --- lib/pinchflat/media/media.ex | 70 ++++++++++++---------------------- test/pinchflat/media_test.exs | 71 ++++++++++------------------------- 2 files changed, 44 insertions(+), 97 deletions(-) diff --git a/lib/pinchflat/media/media.ex b/lib/pinchflat/media/media.ex index 74a4414..170968e 100644 --- a/lib/pinchflat/media/media.ex +++ b/lib/pinchflat/media/media.ex @@ -142,42 +142,6 @@ defmodule Pinchflat.Media do """ def get_media_item!(id), do: Repo.get!(MediaItem, id) - @doc """ - Produces a flat list of the filesystem paths for a media_item's downloaded files - - NOTE: this can almost certainly be made private - - Returns [binary()] - """ - def media_filepaths(media_item) do - mapped_struct = Map.from_struct(media_item) - - MediaItem.filepath_attributes() - |> Enum.map(fn - :subtitle_filepaths = field -> Enum.map(mapped_struct[field], fn [_, filepath] -> filepath end) - field -> List.wrap(mapped_struct[field]) - end) - |> List.flatten() - |> Enum.filter(&is_binary/1) - end - - @doc """ - Produces a flat list of the filesystem paths for a media_item's metadata files. - Returns an empty list if the media_item has no metadata. - - NOTE: this can almost certainly be made private - - Returns [binary()] | [] - """ - def metadata_filepaths(media_item) do - metadata = Repo.preload(media_item, :metadata).metadata || %MediaMetadata{} - mapped_struct = Map.from_struct(metadata) - - MediaMetadata.filepath_attributes() - |> Enum.map(fn field -> mapped_struct[field] end) - |> Enum.filter(&is_binary/1) - end - @doc """ Creates a media_item. @@ -224,19 +188,20 @@ defmodule Pinchflat.Media do end @doc """ - Deletes a media_item and its associated tasks. - Can optionally delete the media_item's files. + Deletes a media_item, its associated tasks, and our internal metadata files. + Can optionally delete the media_item's media files (media, thumbnail, subtitles, etc). Returns {:ok, %MediaItem{}} | {:error, %Ecto.Changeset{}} """ def delete_media_item(%MediaItem{} = media_item, opts \\ []) do delete_files = Keyword.get(opts, :delete_files, false) - # NOTE: this should delete metadata no matter what if delete_files do - {:ok, _} = delete_all_attachments(media_item) + {:ok, _} = delete_media_files(media_item) end + # Should delete these no matter what + delete_internal_metadata_files(media_item) Tasks.delete_tasks_for(media_item) Repo.delete(media_item) end @@ -248,18 +213,31 @@ defmodule Pinchflat.Media do MediaItem.changeset(media_item, attrs) end - # NOTE: refactor this - defp delete_all_attachments(media_item) do - media_item = Repo.preload(media_item, :metadata) + defp delete_media_files(media_item) do + mapped_struct = Map.from_struct(media_item) - media_item - |> media_filepaths() - |> Enum.concat(metadata_filepaths(media_item)) + MediaItem.filepath_attributes() + |> Enum.map(fn + :subtitle_filepaths = field -> Enum.map(mapped_struct[field], fn [_, filepath] -> filepath end) + field -> List.wrap(mapped_struct[field]) + end) + |> List.flatten() + |> Enum.filter(&is_binary/1) |> Enum.each(&FilesystemHelpers.delete_file_and_remove_empty_directories/1) {:ok, media_item} end + defp delete_internal_metadata_files(media_item) do + metadata = Repo.preload(media_item, :metadata).metadata || %MediaMetadata{} + mapped_struct = Map.from_struct(metadata) + + MediaMetadata.filepath_attributes() + |> Enum.map(fn field -> mapped_struct[field] end) + |> Enum.filter(&is_binary/1) + |> Enum.each(&FilesystemHelpers.delete_file_and_remove_empty_directories/1) + end + defp maybe_apply_cutoff_date(source) do if source.download_cutoff_date do dynamic([mi], mi.upload_date >= ^source.download_cutoff_date) diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index a91e8b1..500628e 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -360,57 +360,6 @@ defmodule Pinchflat.MediaTest do end end - describe "media_filepaths/1" do - test "returns filepaths in a flat list" do - filepaths = %{ - media_filepath: "/video/test.mp4", - thumbnail_filepath: "/video/test.jpg", - subtitle_filepaths: [["en", "video/test.srt"]] - } - - media_item = media_item_fixture(filepaths) - - assert Media.media_filepaths(media_item) == [ - "/video/test.mp4", - "/video/test.jpg", - "video/test.srt" - ] - end - - test "strips out nil values" do - filepaths = %{ - media_filepath: "/video/test.mp4", - thumbnail_filepath: nil, - subtitle_filepaths: [["en", nil]] - } - - media_item = media_item_fixture(filepaths) - - assert Media.media_filepaths(media_item) == ["/video/test.mp4"] - end - end - - describe "metadata_filepaths" do - test "returns filepaths in a flat list" do - filepaths = %{ - metadata_filepath: "/metadata.json.gz", - thumbnail_filepath: "/thumbnail.jpg" - } - - media_item = media_item_fixture(%{metadata: filepaths}) - - assert Media.metadata_filepaths(media_item) == [ - "/metadata.json.gz", - "/thumbnail.jpg" - ] - end - - test "returns an empty list when there is no metadata" do - media_item = media_item_fixture() - assert Media.metadata_filepaths(media_item) == [] - end - end - describe "create_media_item/1" do test "creating with valid data creates a media_item" do valid_attrs = %{ @@ -515,6 +464,26 @@ defmodule Pinchflat.MediaTest do assert {:ok, _} = Media.delete_media_item(media_item) assert File.exists?(media_item.media_filepath) end + + test "does 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) + + assert {:ok, _} = Media.delete_media_item(updated_media_item) + refute File.exists?(updated_media_item.metadata.metadata_filepath) + end end describe "delete_media_item/2 when testing file deletion" do