feat: Refactor playlist handling in PlayerService and add comprehensive tests

app/services/player.py

@@ -5,12 +5,12 @@ import threading
 import time
 from collections.abc import Callable, Coroutine
 from enum import Enum
-from pathlib import Path
 from typing import Any
 
 import vlc  # type: ignore[import-untyped]
 
 from app.core.logging import get_logger
+from app.models.playlist import Playlist
 from app.models.sound import Sound
 from app.models.sound_played import SoundPlayed
 from app.repositories.playlist import PlaylistRepository
@@ -316,39 +316,11 @@ class PlayerService:
         session = self.db_session_factory()
         try:
             playlist_repo = PlaylistRepository(session)
-
-            # Get main playlist (fallback for now)
             current_playlist = await playlist_repo.get_main_playlist()
 
             if current_playlist and current_playlist.id:
-                # Load playlist sounds
                 sounds = await playlist_repo.get_playlist_sounds(current_playlist.id)
-
+                await self._handle_playlist_reload(current_playlist, sounds)
-                # Update state
-                self.state.playlist_id = current_playlist.id
-                self.state.playlist_name = current_playlist.name
-                self.state.playlist_sounds = sounds
-                self.state.playlist_length = len(sounds)
-                self.state.playlist_duration = sum(
-                    sound.duration or 0 for sound in sounds
-                )
-
-                # Reset current sound if playlist changed
-                if self.state.current_sound_id and not any(
-                    s.id == self.state.current_sound_id for s in sounds
-                ):
-                    self.state.current_sound_id = None
-                    self.state.current_sound_index = None
-                    self.state.current_sound = None
-                    if self.state.status != PlayerStatus.STOPPED:
-                        await self._stop_playback()
-
-                # Set first track as current if no current track and playlist has sounds
-                if not self.state.current_sound_id and sounds:
-                    self.state.current_sound_index = 0
-                    self.state.current_sound = sounds[0]
-                    self.state.current_sound_id = sounds[0].id
-
                 logger.info(
                     "Loaded playlist: %s (%s sounds)",
                     current_playlist.name,
@@ -361,6 +333,132 @@ class PlayerService:
 
         await self._broadcast_state()
 
+    async def _handle_playlist_reload(
+        self,
+        current_playlist: Playlist,
+        sounds: list[Sound],
+    ) -> None:
+        """Handle playlist reload logic with ID comparison."""
+        # Store previous state for comparison
+        previous_playlist_id = self.state.playlist_id
+        previous_current_sound_id = self.state.current_sound_id
+        previous_current_sound_index = self.state.current_sound_index
+
+        # Update basic playlist state
+        self._update_playlist_state(current_playlist, sounds)
+
+        # Handle playlist changes based on ID comparison
+        if (
+            current_playlist.id is not None
+            and previous_playlist_id != current_playlist.id
+        ):
+            await self._handle_playlist_id_changed(
+                previous_playlist_id, current_playlist.id, sounds,
+            )
+        elif previous_current_sound_id:
+            await self._handle_same_playlist_track_check(
+                previous_current_sound_id,
+                previous_current_sound_index,
+                sounds,
+            )
+        elif sounds:
+            self._set_first_track_as_current(sounds)
+
+    async def _handle_playlist_id_changed(
+        self,
+        previous_id: int | None,
+        current_id: int,
+        sounds: list[Sound],
+    ) -> None:
+        """Handle when playlist ID changes - stop player and reset to first track."""
+        logger.info(
+            "Playlist changed from %s to %s - stopping player and resetting",
+            previous_id,
+            current_id,
+        )
+
+        if self.state.status != PlayerStatus.STOPPED:
+            await self._stop_playback()
+
+        if sounds:
+            self._set_first_track_as_current(sounds)
+        else:
+            self._clear_current_track()
+
+    async def _handle_same_playlist_track_check(
+        self,
+        previous_sound_id: int,
+        previous_index: int | None,
+        sounds: list[Sound],
+    ) -> None:
+        """Handle track checking when playlist ID is the same."""
+        # Find the current track in the new playlist
+        new_index = self._find_sound_index(previous_sound_id, sounds)
+
+        if new_index is not None:
+            # Track still exists - update index if it changed
+            if new_index != previous_index:
+                logger.info(
+                    "Current track %s moved from index %s to %s",
+                    previous_sound_id,
+                    previous_index,
+                    new_index,
+                )
+            # Always set the index and sound reference
+            self.state.current_sound_index = new_index
+            self.state.current_sound = sounds[new_index]
+        else:
+            # Current track no longer exists in playlist
+            await self._handle_track_removed(previous_sound_id, sounds)
+
+    async def _handle_track_removed(
+        self,
+        previous_sound_id: int,
+        sounds: list[Sound],
+    ) -> None:
+        """Handle when current track no longer exists in playlist."""
+        logger.info(
+            "Current track %s no longer exists in playlist - stopping and resetting",
+            previous_sound_id,
+        )
+
+        if self.state.status != PlayerStatus.STOPPED:
+            await self._stop_playback()
+
+        if sounds:
+            self._set_first_track_as_current(sounds)
+        else:
+            self._clear_current_track()
+
+    def _update_playlist_state(
+        self, current_playlist: Playlist, sounds: list[Sound],
+    ) -> None:
+        """Update basic playlist state information."""
+        self.state.playlist_id = current_playlist.id
+        self.state.playlist_name = current_playlist.name
+        self.state.playlist_sounds = sounds
+        self.state.playlist_length = len(sounds)
+        self.state.playlist_duration = sum(sound.duration or 0 for sound in sounds)
+
+    def _find_sound_index(self, sound_id: int, sounds: list[Sound]) -> int | None:
+        """Find the index of a sound in the sounds list."""
+        for i, sound in enumerate(sounds):
+            if sound.id == sound_id:
+                return i
+        return None
+
+    def _set_first_track_as_current(self, sounds: list[Sound]) -> None:
+        """Set the first track as the current track."""
+        self.state.current_sound_index = 0
+        self.state.current_sound = sounds[0]
+        self.state.current_sound_id = sounds[0].id
+
+    def _clear_current_track(self) -> None:
+        """Clear the current track state."""
+        self.state.current_sound_index = None
+        self.state.current_sound = None
+        self.state.current_sound_id = None
+
     def get_state(self) -> dict[str, Any]:
         """Get current player state."""
         return self.state.to_dict()

tests/services/test_player.py

@@ -386,6 +386,239 @@ class TestPlayerService:
                 assert player_service.state.playlist_length == 2
                 assert player_service.state.playlist_duration == 75000
 
+    @pytest.mark.asyncio
+    async def test_handle_playlist_id_changed(self, player_service):
+        """Test handling when playlist ID changes."""
+        # Setup initial state
+        player_service.state.status = PlayerStatus.PLAYING
+        player_service.state.current_sound_id = 1
+        player_service.state.current_sound_index = 0
+
+        # Create test sounds
+        sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sound2 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sounds = [sound1, sound2]
+
+        with patch.object(player_service, "_stop_playback", new_callable=AsyncMock) as mock_stop:
+            await player_service._handle_playlist_id_changed(1, 2, sounds)
+
+            # Should stop playback and set first track as current
+            mock_stop.assert_called_once()
+            assert player_service.state.current_sound_index == 0
+            assert player_service.state.current_sound == sound1
+            assert player_service.state.current_sound_id == 1
+
+    @pytest.mark.asyncio
+    async def test_handle_playlist_id_changed_empty_playlist(self, player_service):
+        """Test handling playlist ID change with empty playlist."""
+        player_service.state.status = PlayerStatus.PLAYING
+
+        with patch.object(player_service, "_stop_playback", new_callable=AsyncMock) as mock_stop:
+            await player_service._handle_playlist_id_changed(1, 2, [])
+
+            mock_stop.assert_called_once()
+            assert player_service.state.current_sound_index is None
+            assert player_service.state.current_sound is None
+            assert player_service.state.current_sound_id is None
+
+    @pytest.mark.asyncio
+    async def test_handle_same_playlist_track_exists_same_index(self, player_service):
+        """Test handling same playlist when track exists at same index."""
+        sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sound2 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sounds = [sound1, sound2]
+
+        await player_service._handle_same_playlist_track_check(1, 0, sounds)
+
+        # Should update sound object reference but keep same index
+        assert player_service.state.current_sound_index == 0  # Should be set to 0 from new_index
+        assert player_service.state.current_sound == sound1
+
+    @pytest.mark.asyncio
+    async def test_handle_same_playlist_track_exists_different_index(self, player_service):
+        """Test handling same playlist when track exists at different index."""
+        sound1 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sound2 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sounds = [sound1, sound2]  # Track with ID 1 is now at index 1
+
+        await player_service._handle_same_playlist_track_check(1, 0, sounds)
+
+        # Should update index and sound reference
+        assert player_service.state.current_sound_index == 1
+        assert player_service.state.current_sound == sound2
+
+    @pytest.mark.asyncio
+    async def test_handle_same_playlist_track_not_found(self, player_service):
+        """Test handling same playlist when current track no longer exists."""
+        player_service.state.status = PlayerStatus.PLAYING
+        sound1 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sound2 = Sound(id=3, name="Song 3", filename="song3.mp3", duration=60000)
+        sounds = [sound1, sound2]  # Track with ID 1 is missing
+
+        with patch.object(player_service, "_handle_track_removed", new_callable=AsyncMock) as mock_removed:
+            await player_service._handle_same_playlist_track_check(1, 0, sounds)
+            mock_removed.assert_called_once_with(1, sounds)
+
+    @pytest.mark.asyncio
+    async def test_handle_track_removed_with_sounds(self, player_service):
+        """Test handling when current track is removed with sounds available."""
+        player_service.state.status = PlayerStatus.PLAYING
+        sound1 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sounds = [sound1]
+
+        with patch.object(player_service, "_stop_playback", new_callable=AsyncMock) as mock_stop:
+            await player_service._handle_track_removed(1, sounds)
+
+            mock_stop.assert_called_once()
+            assert player_service.state.current_sound_index == 0
+            assert player_service.state.current_sound == sound1
+            assert player_service.state.current_sound_id == 2
+
+    @pytest.mark.asyncio
+    async def test_handle_track_removed_empty_playlist(self, player_service):
+        """Test handling when current track is removed with empty playlist."""
+        player_service.state.status = PlayerStatus.PLAYING
+
+        with patch.object(player_service, "_stop_playback", new_callable=AsyncMock) as mock_stop:
+            await player_service._handle_track_removed(1, [])
+
+            mock_stop.assert_called_once()
+            assert player_service.state.current_sound_index is None
+            assert player_service.state.current_sound is None
+            assert player_service.state.current_sound_id is None
+
+    def test_update_playlist_state(self, player_service):
+        """Test updating playlist state information."""
+        mock_playlist = Mock()
+        mock_playlist.id = 5
+        mock_playlist.name = "New Playlist"
+
+        sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sound2 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sounds = [sound1, sound2]
+
+        player_service._update_playlist_state(mock_playlist, sounds)
+
+        assert player_service.state.playlist_id == 5
+        assert player_service.state.playlist_name == "New Playlist"
+        assert player_service.state.playlist_sounds == sounds
+        assert player_service.state.playlist_length == 2
+        assert player_service.state.playlist_duration == 75000
+
+    def test_find_sound_index_found(self, player_service):
+        """Test finding sound index when sound exists."""
+        sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sound2 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sounds = [sound1, sound2]
+
+        index = player_service._find_sound_index(2, sounds)
+        assert index == 1
+
+    def test_find_sound_index_not_found(self, player_service):
+        """Test finding sound index when sound doesn't exist."""
+        sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sounds = [sound1]
+
+        index = player_service._find_sound_index(999, sounds)
+        assert index is None
+
+    def test_set_first_track_as_current(self, player_service):
+        """Test setting first track as current."""
+        sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+        sound2 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+        sounds = [sound1, sound2]
+
+        player_service._set_first_track_as_current(sounds)
+
+        assert player_service.state.current_sound_index == 0
+        assert player_service.state.current_sound == sound1
+        assert player_service.state.current_sound_id == 1
+
+    def test_clear_current_track(self, player_service):
+        """Test clearing current track state."""
+        # Set some initial state
+        player_service.state.current_sound_index = 2
+        player_service.state.current_sound = Mock()
+        player_service.state.current_sound_id = 5
+
+        player_service._clear_current_track()
+
+        assert player_service.state.current_sound_index is None
+        assert player_service.state.current_sound is None
+        assert player_service.state.current_sound_id is None
+
+    @pytest.mark.asyncio
+    async def test_reload_playlist_different_id_scenario(self, player_service):
+        """Test complete reload scenario when playlist ID changes."""
+        # Setup current state
+        player_service.state.playlist_id = 1
+        player_service.state.current_sound_id = 1
+        player_service.state.current_sound_index = 0
+        player_service.state.status = PlayerStatus.PLAYING
+
+        mock_session = AsyncMock()
+        player_service.db_session_factory = lambda: mock_session
+
+        with patch("app.services.player.PlaylistRepository") as mock_repo_class:
+            mock_repo = AsyncMock()
+            mock_repo_class.return_value = mock_repo
+
+            # Mock new playlist with different ID
+            mock_playlist = Mock()
+            mock_playlist.id = 2  # Different ID
+            mock_playlist.name = "New Playlist"
+            mock_repo.get_main_playlist.return_value = mock_playlist
+
+            sound1 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+            mock_sounds = [sound1]
+            mock_repo.get_playlist_sounds.return_value = mock_sounds
+
+            with patch.object(player_service, "_stop_playback", new_callable=AsyncMock) as mock_stop:
+                with patch.object(player_service, "_broadcast_state", new_callable=AsyncMock):
+                    await player_service.reload_playlist()
+
+                    # Should stop and reset to first track
+                    mock_stop.assert_called_once()
+                    assert player_service.state.playlist_id == 2
+                    assert player_service.state.current_sound_index == 0
+                    assert player_service.state.current_sound_id == 1
+
+    @pytest.mark.asyncio
+    async def test_reload_playlist_same_id_track_moved(self, player_service):
+        """Test reload when playlist ID same but track moved to different index."""
+        # Setup current state
+        player_service.state.playlist_id = 1
+        player_service.state.current_sound_id = 2  # Currently playing track 2
+        player_service.state.current_sound_index = 1  # At index 1
+
+        mock_session = AsyncMock()
+        player_service.db_session_factory = lambda: mock_session
+
+        with patch("app.services.player.PlaylistRepository") as mock_repo_class:
+            mock_repo = AsyncMock()
+            mock_repo_class.return_value = mock_repo
+
+            # Same playlist ID
+            mock_playlist = Mock()
+            mock_playlist.id = 1
+            mock_playlist.name = "Same Playlist"
+            mock_repo.get_main_playlist.return_value = mock_playlist
+
+            # Track 2 moved to index 0
+            sound1 = Sound(id=2, name="Song 2", filename="song2.mp3", duration=45000)
+            sound2 = Sound(id=1, name="Song 1", filename="song1.mp3", duration=30000)
+            mock_sounds = [sound1, sound2]  # Track 2 now at index 0
+            mock_repo.get_playlist_sounds.return_value = mock_sounds
+
+            with patch.object(player_service, "_broadcast_state", new_callable=AsyncMock):
+                await player_service.reload_playlist()
+
+                # Should update index but keep same track
+                assert player_service.state.playlist_id == 1
+                assert player_service.state.current_sound_index == 0  # Updated index
+                assert player_service.state.current_sound_id == 2  # Same track
+                assert player_service.state.current_sound == sound1
+
 
     def test_get_next_index_continuous_mode(self, player_service):
         """Test getting next index in continuous mode."""