[Bugfix] prevent duplicate videos from being downloaded if the video's name changes (#396)

* Added methods for deleting outdated files

* Hooked up outdated file deletion to media download worker

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

test/pinchflat/media/file_deletion_test.exs

@@ -0,0 +1,77 @@
+defmodule Pinchflat.Media.FileDeletionTest do
+  use Pinchflat.DataCase
+
+  import Pinchflat.MediaFixtures
+
+  alias Pinchflat.Media.FileDeletion
+
+  describe "delete_outdated_files/2" do
+    test "deletes outdated non-subtitle files" do
+      new_media_item = media_item_with_attachments()
+      old_media_item = media_item_with_attachments()
+
+      assert :ok = FileDeletion.delete_outdated_files(old_media_item, new_media_item)
+
+      assert File.exists?(new_media_item.media_filepath)
+      refute File.exists?(old_media_item.media_filepath)
+    end
+
+    test "doesn't delete non-subtitle files if the new file is the same" do
+      new_media_item = media_item_with_attachments()
+      old_media_item = media_item_fixture(%{media_filepath: new_media_item.media_filepath})
+
+      assert :ok = FileDeletion.delete_outdated_files(old_media_item, new_media_item)
+
+      assert File.exists?(new_media_item.media_filepath)
+      assert File.exists?(old_media_item.media_filepath)
+    end
+
+    test "doesn't delete the old file if the new file is missing that key" do
+      new_media_item = media_item_fixture(%{media_filepath: nil})
+      old_media_item = media_item_with_attachments()
+
+      assert :ok = FileDeletion.delete_outdated_files(old_media_item, new_media_item)
+
+      assert File.exists?(old_media_item.media_filepath)
+    end
+
+    test "deletes outdated subtitle files" do
+      new_media_item = media_item_with_attachments()
+      old_media_item = media_item_with_attachments()
+
+      assert :ok = FileDeletion.delete_outdated_files(old_media_item, new_media_item)
+
+      assert File.exists?(get_subtitle_filepath(new_media_item, "en"))
+      refute File.exists?(get_subtitle_filepath(old_media_item, "en"))
+    end
+
+    test "keeps old subtitle files if the new file is the same" do
+      new_media_item = media_item_with_attachments()
+      old_media_item = media_item_fixture(%{subtitle_filepaths: new_media_item.subtitle_filepaths})
+
+      assert :ok = FileDeletion.delete_outdated_files(old_media_item, new_media_item)
+
+      assert File.exists?(get_subtitle_filepath(new_media_item, "en"))
+      assert File.exists?(get_subtitle_filepath(old_media_item, "en"))
+    end
+
+    test "doesn't delete old subtitle files if the new file is missing that key" do
+      new_media_item = media_item_fixture(%{subtitle_filepaths: []})
+      old_media_item = media_item_with_attachments()
+
+      assert :ok = FileDeletion.delete_outdated_files(old_media_item, new_media_item)
+
+      assert File.exists?(get_subtitle_filepath(old_media_item, "en"))
+    end
+  end
+
+  defp get_subtitle_filepath(media_item, language) do
+    Enum.reduce_while(media_item.subtitle_filepaths, nil, fn [lang, filepath], acc ->
+      if lang == language do
+        {:halt, filepath}
+      else
+        {:cont, acc}
+      end
+    end)
+  end
+end

test/pinchflat/utils/filesystem_utils_test.exs

@@ -37,6 +37,46 @@ defmodule Pinchflat.Utils.FilesystemUtilsTest do
     end
   end
 
+  describe "filepaths_reference_same_file?/2" do
+    setup do
+      filepath = FilesystemUtils.generate_metadata_tmpfile(:json)
+
+      on_exit(fn -> File.rm!(filepath) end)
+
+      {:ok, %{filepath: filepath}}
+    end
+
+    test "returns true if the files are the same", %{filepath: filepath} do
+      assert FilesystemUtils.filepaths_reference_same_file?(filepath, filepath)
+    end
+
+    test "returns true if different filepaths point to the same file", %{filepath: filepath} do
+      short_path = Path.expand(filepath)
+      long_path = Path.join(["/tmp", "..", filepath])
+
+      assert short_path != long_path
+      assert FilesystemUtils.filepaths_reference_same_file?(short_path, long_path)
+    end
+
+    test "returns true if the files are symlinked", %{filepath: filepath} do
+      tmpfile_directory = Application.get_env(:pinchflat, :tmpfile_directory)
+      other_filepath = Path.join([tmpfile_directory, "symlink.json"])
+      :ok = File.ln_s!(filepath, other_filepath)
+
+      assert FilesystemUtils.filepaths_reference_same_file?(filepath, other_filepath)
+
+      File.rm!(other_filepath)
+    end
+
+    test "returns false if the files are different", %{filepath: filepath} do
+      other_filepath = FilesystemUtils.generate_metadata_tmpfile(:json)
+
+      refute FilesystemUtils.filepaths_reference_same_file?(filepath, other_filepath)
+
+      File.rm!(other_filepath)
+    end
+  end
+
   describe "generate_metadata_tmpfile/1" do
     test "creates a tmpfile and returns its path" do
       res = FilesystemUtils.generate_metadata_tmpfile(:json)

test/pinchflat/utils/map_utils_test.exs

@@ -0,0 +1,31 @@
+defmodule Pinchflat.Utils.MapUtilsTest do
+  use Pinchflat.DataCase
+
+  alias Pinchflat.Utils.MapUtils
+
+  describe "from_nested_list/1" do
+    test "creates a map from a nested 2-element tuple list" do
+      list = [
+        {"key1", "value1"},
+        {"key2", "value2"}
+      ]
+
+      assert MapUtils.from_nested_list(list) == %{
+               "key1" => "value1",
+               "key2" => "value2"
+             }
+    end
+
+    test "creates a map from a nested 2-element list of lists" do
+      list = [
+        ["key1", "value1"],
+        ["key2", "value2"]
+      ]
+
+      assert MapUtils.from_nested_list(list) == %{
+               "key1" => "value1",
+               "key2" => "value2"
+             }
+    end
+  end
+end