diff --git a/beetsplug/replace.py b/beetsplug/replace.py index b585a13c1f..75e6d1d215 100644 --- a/beetsplug/replace.py +++ b/beetsplug/replace.py @@ -7,9 +7,12 @@ 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 @@ def commands(self): 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 ") @@ -59,32 +64,27 @@ def file_check(self, filepath: Path) -> None: 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.") - - def confirm_replacement(self, new_file_path: Path, song: Item): + 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)), + ) + + 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 @@ def confirm_replacement(self, new_file_path: Path, song: Item): 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 @@ def replace_file(self, new_file_path: Path, song: Item) -> None: 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 @@ def replace_file(self, new_file_path: Path, song: Item) -> None: ): 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) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1ac16a8e27..d42aa56215 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -284,6 +284,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 ~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/plugins/replace.rst b/docs/plugins/replace.rst index 7216f83991..2de6e2936c 100644 --- a/docs/plugins/replace.rst +++ b/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. diff --git a/test/plugins/test_replace.py b/test/plugins/test_replace.py index a247e317ad..3d4f96e501 100644 --- a/test/plugins/test_replace.py +++ b/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 + 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.title = "AAA" + mediafile.save() + + 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) - @pytest.fixture(autouse=True) - def _fake_file(self, tmp_path): - self.fake_file = tmp_path + monkeypatch.setattr(replace, "select_song", Mock(return_value=item)) - def test_path_is_dir(self): - fake_directory = self.fake_dir / "fakeDir" + 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): - fake_file = self.fake_file / "fakefile.txt" + 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): - dest = self.fake_file / "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.save() - - replace.file_check(Path(str(dest))) + 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 @@ def test_select_song_valid_choice(self, monkeypatch, capfd): 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 Song: 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 Song: 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 Song: 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"