Merge f23abda908 into 72b4c77161

beetsplug/replace.py

@@ -7,9 +7,12 @@ from typing import TYPE_CHECKING
 import mediafile
 
 from beets import ui, util
+from beets.library import Item, Library
 from beets.plugins import BeetsPlugin
 
 if TYPE_CHECKING:
+    import optparse
+
     from beets.library import Item, Library
 
 
@@ -21,7 +24,9 @@ class ReplacePlugin(BeetsPlugin):
         cmd.func = self.run
         return [cmd]
 
-    def run(self, lib: Library, args: list[str]) -> None:
+    def run(
+        self, lib: Library, _opts: optparse.Values, args: list[str]
+    ) -> None:
         if len(args) < 2:
             raise ui.UserError("Usage: beet replace <query> <new_file_path>")
 
@@ -59,32 +64,27 @@ class ReplacePlugin(BeetsPlugin):
         except mediafile.FileTypeError as fte:
             raise ui.UserError(fte)
 
-    def select_song(self, items: list[Item]):
+    def select_song(self, items: list[Item]) -> Item | None:
         """Present a menu of matching songs and get user selection."""
-        ui.print_("\nMatching songs:")
+        ui.print_("Matching songs:")
         for i, item in enumerate(items, 1):
             ui.print_(f"{i}. {util.displayable_path(item)}")
 
-        while True:
-            try:
-                index = int(
-                    input(
-                        f"Which song would you like to replace? "
-                        f"[1-{len(items)}] (0 to cancel): "
-                    )
-                )
-                if index == 0:
-                    return None
-                if 1 <= index <= len(items):
-                    return items[index - 1]
-                ui.print_(
-                    f"Invalid choice. Please enter a number "
-                    f"between 1 and {len(items)}."
-                )
-            except ValueError:
-                ui.print_("Invalid input. Please type in a number.")
+        index = ui.input_options(
+            [],
+            require=True,
+            prompt=(
+                f"Which song would you like to replace? "
+                f"[1-{len(items)}] (0 to cancel):"
+            ),
+            numrange=(0, len(items)),
+        )
 
-    def confirm_replacement(self, new_file_path: Path, song: Item):
+        if index == 0:
+            return None
+        return items[index - 1]
+
+    def confirm_replacement(self, new_file_path: Path, song: Item) -> bool:
         """Get user confirmation for the replacement."""
         original_file_path: Path = Path(song.path.decode())
 
@@ -95,12 +95,10 @@ class ReplacePlugin(BeetsPlugin):
             f"\nReplacing: {util.displayable_path(new_file_path)} "
             f"-> {util.displayable_path(original_file_path)}"
         )
-        decision: str = (
-            input("Are you sure you want to replace this track? (y/N): ")
-            .strip()
-            .casefold()
+
+        return ui.input_yn(
+            "Are you sure you want to replace this track (y/n)?", require=True
         )
-        return decision in {"yes", "y"}
 
     def replace_file(self, new_file_path: Path, song: Item) -> None:
         """Replace the existing file with the new one."""
@@ -109,7 +107,7 @@ class ReplacePlugin(BeetsPlugin):
 
         try:
             shutil.move(util.syspath(new_file_path), util.syspath(dest))
-        except Exception as e:
+        except OSError as e:
             raise ui.UserError(f"Error replacing file: {e}")
 
         if (
@@ -118,10 +116,13 @@ class ReplacePlugin(BeetsPlugin):
         ):
             try:
                 original_file_path.unlink()
-            except Exception as e:
+            except OSError as e:
                 raise ui.UserError(f"Could not delete original file: {e}")
 
-        song.path = str(dest).encode()
-        song.store()
+        # Update the path to point to the new file.
+        song.path = util.bytestring_path(dest)
 
-        ui.print_("Replacement successful.")
+        # Synchronise the new file with the database. This copies metadata from the
+        # Item to the new file (i.e. title, artist, album, etc.),
+        # and then from the Item to the database (i.e. path and mtime).
+        song.try_sync(write=True, move=False)

docs/changelog.rst

@@ -347,6 +347,8 @@ Bug fixes
   won't crash beets anymore. If you want to raise exceptions instead, set the
   new configuration option ``raise_on_error`` to ``yes`` :bug:`5903`,
   :bug:`4789`.
+- :doc:`/plugins/replace`: Fixed the command failing to run, and now syncs
+  metadata in the database with the newly swapped-in file. :bug:`6260`
 
 For plugin developers
 ~~~~~~~~~~~~~~~~~~~~~

docs/plugins/replace.rst

@@ -15,5 +15,11 @@ and then type:
 The plugin will show you a list of files for you to pick from, and then ask for
 confirmation.
 
+The file you pick will be replaced with the file at ``path``. Then, the new
+file's metadata will be synced with the database. This means that the tags in
+the database for that track (``title``, ``artist``, etc.) will be written to the
+new file, and the ``path`` and ``mtime`` fields in the database will be updated
+to match the new file's path and the current modification time.
+
 Consider using the ``replaygain`` command from the :doc:`/plugins/replaygain`
 plugin, if you usually use it during imports.

test/plugins/test_replace.py

@@ -1,54 +1,135 @@
+from __future__ import annotations
+
+import optparse
 import shutil
 from pathlib import Path
+from typing import TYPE_CHECKING
+from unittest.mock import Mock
 
 import pytest
 from mediafile import MediaFile
 
 from beets import ui
+from beets.library import Item
+from beets.library.exceptions import WriteError
 from beets.test import _common
+from beets.test.helper import TestHelper, capture_log
 from beetsplug.replace import ReplacePlugin
 
+if TYPE_CHECKING:
+    from collections.abc import Generator
+
+    from beets.library import Library
+
 replace = ReplacePlugin()
 
 
 class TestReplace:
-    @pytest.fixture(autouse=True)
-    def _fake_dir(self, tmp_path):
-        self.fake_dir = tmp_path
-
-    @pytest.fixture(autouse=True)
-    def _fake_file(self, tmp_path):
-        self.fake_file = tmp_path
-
-    def test_path_is_dir(self):
-        fake_directory = self.fake_dir / "fakeDir"
-        fake_directory.mkdir()
-        with pytest.raises(ui.UserError):
-            replace.file_check(fake_directory)
-
-    def test_path_is_unsupported_file(self):
-        fake_file = self.fake_file / "fakefile.txt"
-        fake_file.write_text("test", encoding="utf-8")
-        with pytest.raises(ui.UserError):
-            replace.file_check(fake_file)
-
-    def test_path_is_supported_file(self):
-        dest = self.fake_file / "full.mp3"
+    @pytest.fixture
+    def mp3_file(self, tmp_path) -> Path:
+        dest = tmp_path / "full.mp3"
         src = Path(_common.RSRC.decode()) / "full.mp3"
         shutil.copyfile(src, dest)
 
         mediafile = MediaFile(dest)
-        mediafile.albumartist = "AAA"
-        mediafile.disctitle = "DDD"
-        mediafile.genres = ["a", "b", "c"]
-        mediafile.composer = None
+        mediafile.title = "AAA"
         mediafile.save()
 
-        replace.file_check(Path(str(dest)))
+        return dest
+
+    @pytest.fixture
+    def opus_file(self, tmp_path) -> Path:
+        dest = tmp_path / "full.opus"
+        src = Path(_common.RSRC.decode()) / "full.opus"
+        shutil.copyfile(src, dest)
+
+        return dest
+
+    @pytest.fixture
+    def library(self) -> Generator[Library]:
+        helper = TestHelper()
+        helper.setup_beets()
+
+        yield helper.lib
+
+        helper.teardown_beets()
+
+    def test_run_replace_too_few_args(self):
+        with pytest.raises(ui.UserError):
+            replace.run(None, optparse.Values(), [])
+
+    def test_run_replace_no_matches(self, library):
+        with pytest.raises(ui.UserError):
+            replace.run(library, optparse.Values(), ["BBB", ""])
+
+    def test_run_replace_no_song_selected(
+        self, library, mp3_file, opus_file, monkeypatch
+    ):
+        monkeypatch.setattr(replace, "file_check", Mock(return_value=None))
+        monkeypatch.setattr(replace, "select_song", Mock(return_value=None))
+
+        item = Item.from_path(mp3_file)
+        library.add(item)
+
+        replace.run(library, optparse.Values(), ["AAA", str(opus_file)])
+
+        assert mp3_file.exists()
+        assert opus_file.exists()
+
+    def test_run_replace_not_confirmed(
+        self, library, mp3_file, opus_file, monkeypatch
+    ):
+        monkeypatch.setattr(replace, "file_check", Mock(return_value=None))
+        monkeypatch.setattr(
+            replace, "confirm_replacement", Mock(return_value=False)
+        )
+
+        item = Item.from_path(mp3_file)
+        library.add(item)
+
+        monkeypatch.setattr(replace, "select_song", Mock(return_value=item))
+
+        replace.run(library, optparse.Values(), ["AAA", str(opus_file)])
+
+        assert mp3_file.exists()
+        assert opus_file.exists()
+
+    def test_run_replace(self, library, mp3_file, opus_file, monkeypatch):
+        replace_file = Mock(replace.replace_file, return_value=None)
+        monkeypatch.setattr(replace, "replace_file", replace_file)
+
+        monkeypatch.setattr(replace, "file_check", Mock(return_value=None))
+        monkeypatch.setattr(
+            replace, "confirm_replacement", Mock(return_value=True)
+        )
+
+        item = Item.from_path(mp3_file)
+        library.add(item)
+
+        monkeypatch.setattr(replace, "select_song", Mock(return_value=item))
+
+        replace.run(library, optparse.Values(), ["AAA", str(opus_file)])
+
+        replace_file.assert_called_once()
+
+    def test_path_is_dir(self, tmp_path):
+        fake_directory = tmp_path / "fakeDir"
+        fake_directory.mkdir()
+        with pytest.raises(ui.UserError):
+            replace.file_check(fake_directory)
+
+    def test_path_is_unsupported_file(self, tmp_path):
+        fake_file = tmp_path / "fakefile.txt"
+        fake_file.write_text("test", encoding="utf-8")
+        with pytest.raises(ui.UserError):
+            replace.file_check(fake_file)
+
+    def test_path_is_supported_file(self, mp3_file):
+        replace.file_check(mp3_file)
 
     def test_select_song_valid_choice(self, monkeypatch, capfd):
         songs = ["Song A", "Song B", "Song C"]
-        monkeypatch.setattr("builtins.input", lambda _: "2")
+        monkeypatch.setattr("builtins.input", Mock(return_value="2"))
 
         selected_song = replace.select_song(songs)
 
@@ -61,26 +142,23 @@ class TestReplace:
 
     def test_select_song_cancel(self, monkeypatch):
         songs = ["Song A", "Song B", "Song C"]
-        monkeypatch.setattr("builtins.input", lambda _: "0")
+        monkeypatch.setattr("builtins.input", Mock(return_value="0"))
 
         selected_song = replace.select_song(songs)
 
         assert selected_song is None
 
-    def test_select_song_invalid_then_valid(self, monkeypatch, capfd):
+    def test_select_song_invalid_then_valid(self, monkeypatch):
         songs = ["Song A", "Song B", "Song C"]
-        inputs = iter(["invalid", "4", "3"])
-        monkeypatch.setattr("builtins.input", lambda _: next(inputs))
+        inputs = ["invalid", "4", "3"]
+        mock_input = Mock(side_effect=iter(inputs))
+        monkeypatch.setattr("builtins.input", mock_input)
 
         selected_song = replace.select_song(songs)
 
-        captured = capfd.readouterr()
-
-        assert "Invalid input. Please type in a number." in captured.out
-        assert (
-            "Invalid choice. Please enter a number between 1 and 3."
-            in captured.out
-        )
+        # The first two inputs should be considered invalid, so the third
+        # input of 3 should be used, resulting in Song C being selected.
+        assert mock_input.call_count == 3
         assert selected_song == "Song C"
 
     def test_confirm_replacement_file_not_exist(self):
@@ -94,7 +172,7 @@ class TestReplace:
 
     def test_confirm_replacement_yes(self, monkeypatch):
         src = Path(_common.RSRC.decode()) / "full.mp3"
-        monkeypatch.setattr("builtins.input", lambda _: "YES    ")
+        monkeypatch.setattr("builtins.input", Mock(return_value="yes"))
 
         class Song:
             path = str(src).encode()
@@ -105,7 +183,7 @@ class TestReplace:
 
     def test_confirm_replacement_no(self, monkeypatch):
         src = Path(_common.RSRC.decode()) / "full.mp3"
-        monkeypatch.setattr("builtins.input", lambda _: "test123")
+        monkeypatch.setattr("builtins.input", Mock(return_value="no"))
 
         class Song:
             path = str(src).encode()
@@ -113,3 +191,69 @@ class TestReplace:
         song = Song()
 
         assert replace.confirm_replacement("test", song) is False
+
+    def test_replace_file_move_fails(self, tmp_path):
+        item = Item()
+        item.path = bytes(tmp_path / "not_a_song.mp3")
+
+        with pytest.raises(ui.UserError):
+            replace.replace_file(tmp_path / "not_a_file.opus", item)
+
+    def test_replace_file_delete_fails(
+        self, library, mp3_file, opus_file, monkeypatch
+    ):
+        fail_unlink = Mock(side_effect=OSError("cannot delete"))
+        monkeypatch.setattr(Path, "unlink", fail_unlink)
+
+        item = Item.from_path(mp3_file)
+        library.add(item)
+
+        with pytest.raises(ui.UserError):
+            replace.replace_file(opus_file, item)
+
+    def test_replace_file_write_fails(
+        self, library, mp3_file, opus_file, monkeypatch
+    ):
+        fail_write = Mock(side_effect=WriteError("path", "reason"))
+        monkeypatch.setattr(Item, "write", fail_write)
+
+        item = Item.from_path(mp3_file)
+        library.add(item)
+
+        with capture_log() as logs:
+            replace.replace_file(opus_file, item)
+
+        # Assert that a writing error was logged
+        assert next(m for m in logs if m.startswith("error writing"))
+
+    def test_replace_file(
+        self, mp3_file: Path, opus_file: Path, library: Library
+    ):
+        old_mediafile = MediaFile(mp3_file)
+        old_mediafile.albumartist = "ABC"
+        old_mediafile.disctitle = "DEF"
+        old_mediafile.genre = "GHI"
+        old_mediafile.save()
+
+        item = Item.from_path(mp3_file)
+        library.add(item)
+
+        item.mtime = 0
+        item.store()
+
+        replace.replace_file(opus_file, item)
+
+        # Check that the file has been replaced.
+        assert opus_file.exists()
+        assert not mp3_file.exists()
+
+        # Check that the database path and mtime have been updated.
+        item.load()
+        assert item.path == bytes(opus_file)
+        assert item.mtime > 0
+
+        # Check that the new file has the old file's metadata.
+        new_mediafile = MediaFile(opus_file)
+        assert new_mediafile.albumartist == "ABC"
+        assert new_mediafile.disctitle == "DEF"
+        assert new_mediafile.genre == "GHI"