Hooked up outdated file deletion to media download worker

lib/pinchflat/downloading/media_download_worker.ex

@@ -12,6 +12,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
   alias Pinchflat.Tasks
   alias Pinchflat.Repo
   alias Pinchflat.Media
+  alias Pinchflat.Media.FileDeletion
   alias Pinchflat.Downloading.MediaDownloader
 
   alias Pinchflat.Lifecycle.UserScripts.CommandRunner, as: UserScriptRunner
@@ -85,6 +86,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
             media_redownloaded_at: get_redownloaded_at(is_quality_upgrade)
           })
 
+        :ok = FileDeletion.delete_outdated_files(media_item, updated_media_item)
         run_user_script(:media_downloaded, updated_media_item)
 
         :ok
@@ -121,21 +123,6 @@ defmodule Pinchflat.Downloading.MediaDownloadWorker do
     end
   end
 
-  defp delete_outdated_media_files(old_media_item, updated_media_item) do
-    filepath_keys = MediaItem.filepath_attributes()
-
-    Enum.each(filepath_keys, fn key ->
-      old_filepath_attribute = get_in(old_media_item, [Access.key!(key)])
-      new_filepath_attribute = get_in(updated_media_item, [Access.key!(key)])
-      # filepath_is_string = is_binary(old_filepath) && is_binary(new_filepath)
-      # files_exist = old_filepath && new_filepath && File.exists?(old_filepath) && File.exists?(new_filepath)
-      # IO.inspect({old_filepath, new_filepath, files_exist})
-      # if files_exist && !FSUtils.filepaths_reference_same_file?(old_filepath, new_filepath) do
-      #   FSUtils.delete_file_and_remove_empty_directories(old_filepath)
-      # end
-    end)
-  end
-
   # NOTE: I like this pattern of using the default value so that I don't have to
   # define it in config.exs (and friends). Consider using this elsewhere.
   defp run_user_script(event, media_item) do

lib/pinchflat/media/file_deletion.ex

@@ -43,11 +43,10 @@ defmodule Pinchflat.Media.FileDeletion do
 
     Enum.each(new_attributes, fn {key, new_filepath} ->
       old_filepath = Map.get(old_attributes, key)
-      files_do_exist = old_filepath && new_filepath && File.exists?(old_filepath) && File.exists?(new_filepath)
+      files_have_changed = old_filepath && new_filepath && old_filepath != new_filepath
-      filepaths_are_different = old_filepath != new_filepath
+      files_exist_on_disk = files_have_changed && File.exists?(old_filepath) && File.exists?(new_filepath)
 
-      if files_do_exist && filepaths_are_different &&
+      if files_exist_on_disk && !FSUtils.filepaths_reference_same_file?(old_filepath, new_filepath) do
-           !FSUtils.filepaths_reference_same_file?(old_filepath, new_filepath) do
         FSUtils.delete_file_and_remove_empty_directories(old_filepath)
       end
     end)

test/pinchflat/downloading/media_download_worker_test.exs

@@ -236,6 +236,26 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do
 
       perform_job(MediaDownloadWorker, %{id: media_item.id, force: true})
     end
+
+    test "deletes old files if the media item has been updated" do
+      expect(YtDlpRunnerMock, :run, 1, fn _url, _opts, _ot, _addl ->
+        tmp_media_item = media_item_with_attachments()
+        metadata = render_parsed_metadata(:media_metadata)
+        metadata = Map.put(metadata, "filepath", tmp_media_item.media_filepath)
+
+        {:ok, Phoenix.json_library().encode!(metadata)}
+      end)
+
+      expect(YtDlpRunnerMock, :run, 1, fn _url, _opts, _ot, _addl -> {:ok, ""} end)
+
+      old_media_item = media_item_with_attachments()
+      perform_job(MediaDownloadWorker, %{id: old_media_item.id, force: true})
+      updated_media_item = Repo.reload(old_media_item)
+
+      assert updated_media_item.media_filepath != old_media_item.media_filepath
+      refute File.exists?(old_media_item.media_filepath)
+      assert File.exists?(updated_media_item.media_filepath)
+    end
   end
 
   describe "perform/1 when testing user script callbacks" do