apply review feedback (3)

mopidy_spotify/playlists.py

@@ -56,9 +56,15 @@ def _get_playlist(self, uri, as_items=False, with_owner=False):
         )
 
     @staticmethod
-    def _span(p, xs):  # like haskell's Data.List.span
-        i = next((i for i, v in enumerate(xs) if not p(v)), len(xs))
-        return xs[:i], xs[i:]
+    def _split_ended_movs(value, movs):
+        def _span(p, xs):
+            # Returns a tuple where first element is the longest prefix
+            # (possibly empty) of list xs of elements that satisfy predicate p
+            # and second element is the remainder of the list.
+            i = next((i for i, v in enumerate(xs) if not p(v)), len(xs))
+            return xs[:i], xs[i:]
+
+        return _span(lambda e: e[0] < value, movs)
 
     def _patch_playlist(self, playlist, operations):
         # Note: We need two distinct delta_f/t to be able to keep track of move
@@ -73,14 +79,10 @@ def _patch_playlist(self, playlist, operations):
         for op in operations:
             # from the list of "active" mov-deltas, split off the ones newly
             # outside the range and neutralize them:
-            ended_ranges_f, unwind_f = self._span(
-                lambda e: e[0] < op.frm, unwind_f
-            )
-            ended_ranges_t, unwind_t = self._span(
-                lambda e: e[0] < op.to, unwind_t
-            )
-            delta_f -= sum((v for k, v in ended_ranges_f))
-            delta_t -= sum((v for k, v in ended_ranges_t))
+            ended_ranges_f, unwind_f = self._split_ended_movs(op.frm, unwind_f)
+            ended_ranges_t, unwind_t = self._split_ended_movs(op.to, unwind_t)
+            delta_f -= sum((amount for _, amount in ended_ranges_f))
+            delta_t -= sum((amount for _, amount in ended_ranges_t))
 
             length = len(op.tracks)
             if op.op == "-":
@@ -144,24 +146,29 @@ def refresh(self):
         self._loaded = True
 
     def create(self, name):
-        logger.info(f"Creating playlist {name}")
+        logger.info(f"Creating Spotify playlist '{name}'")
         uri = web.create_playlist(self._backend._web_client, name)
-        self._get_flattened_playlist_refs()
         return self.lookup(uri) if uri else None
 
     def delete(self, uri):
-        logger.info(f"Deleting playlist {uri}")
+        logger.info(f"Deleting Spotify playlist {uri!r}")
         ok = web.delete_playlist(self._backend._web_client, uri)
-        self._get_flattened_playlist_refs()
         return ok
 
     @staticmethod
-    def _len_replace(playlist, n=_chunk_size):
-        return math.ceil(len(playlist.tracks) / n)
+    def _len_replace(playlist_tracks, n=_chunk_size):
+        return math.ceil(len(playlist_tracks) / n)
 
     @staticmethod
     def _is_spotify_track(track_uri):
-        return track_uri.startswith("spotify:track:")
+        try:
+            return web.WebLink.from_uri(track_uri).type == web.LinkType.TRACK
+        except ValueError:
+            return False  # not a valid spotify URI
+
+    @staticmethod
+    def _is_spotify_local(track_uri):
+        return track_uri.startswith("spotify:local:")
 
     def save(self, playlist):
         saved_playlist = self._get_playlist(playlist.uri, with_owner=True)
@@ -173,21 +180,22 @@ def save(self, playlist):
         # mangles the names of other people's playlists.
         if owner and owner != self._backend._web_client.user_id:
             logger.error(
-                f"Refusing to modify someone else's playlist ({playlist.uri})"
+                f"Cannot modify Spotify playlist {playlist.uri!r} owned by "
+                f"other user {owner}"
             )
             return
 
         # We cannot add or (easily) remove spotify:local: tracks, so refuse
         # editing if the current playlist contains such tracks.
-        if any(
-            (t.uri.startswith("spotify:local:") for t in saved_playlist.tracks)
-        ):
+        if any((self._is_spotify_local(t.uri) for t in saved_playlist.tracks)):
             logger.error(
-                "Cannot modify playlist containing 'Spotify Local Files'."
+                "Cannot modify Spotify playlist containing Spotify 'local files'."
             )
             for t in saved_playlist.tracks:
                 if t.uri.startswith("spotify:local:"):
-                    logger.error(f"{t.name} ({t.uri})")
+                    logger.debug(
+                        f"Unsupported Spotify local file: '{t.name}' ({t.uri!r})"
+                    )
             return
 
         new_tracks = [track.uri for track in playlist.tracks]
@@ -196,21 +204,22 @@ def save(self, playlist):
         if any((not self._is_spotify_track(t) for t in new_tracks)):
             new_tracks = list(filter(self._is_spotify_track, new_tracks))
             logger.warning(
-                "Cannot add non-spotify tracks to spotify playlist; skipping those."
+                f"Skipping adding non-Spotify tracks to Spotify playlist "
+                f"{playlist.uri!r}"
             )
 
         operations = utils.diff(cur_tracks, new_tracks, self._chunk_size)
 
         # calculate number of operations required for each strategy:
         ops_patch = len(operations)
-        ops_replace = self._len_replace(playlist)
+        ops_replace = self._len_replace(new_tracks)
 
         try:
             if ops_replace < ops_patch:
                 self._replace_playlist(saved_playlist, new_tracks)
             else:
                 self._patch_playlist(saved_playlist, operations)
-        except RuntimeError as e:
+        except web.OAuthClientError as e:
             logger.error(f"Failed to save Spotify playlist: {e}")
             # In the unlikely event that we used the replace strategy, and the
             # first PUT went through but the following POSTs didn't, we have
@@ -237,14 +246,20 @@ def save(self, playlist):
                 logger.error(f'Created backup in "{filename}"')
             return None
 
-        # Playlist rename logic
-        if playlist.name != saved_playlist.name:
-            logger.info(
-                f"Renaming playlist [{saved_playlist.name}] to [{playlist.name}]"
-            )
-            web.rename_playlist(
-                self._backend._web_client, playlist.uri, playlist.name
-            )
+        if playlist.name and playlist.name != saved_playlist.name:
+            try:
+                web.rename_playlist(
+                    self._backend._web_client, playlist.uri, playlist.name
+                )
+                logger.info(
+                    f"Renamed Spotify playlist '{saved_playlist.name}' to "
+                    f"'{playlist.name}'"
+                )
+            except web.OAuthClientError as e:
+                logger.error(
+                    f"Renaming Spotify playlist '{saved_playlist.name}'"
+                    f"to '{playlist.name}' failed with error {e}"
+                )
 
         return self.lookup(saved_playlist.uri)
 

mopidy_spotify/web.py

@@ -73,7 +73,7 @@ def request(self, method, path, *args, cache=None, **kwargs):
         params = kwargs.pop("params", None)
         path = self._normalise_query_string(path, params)
 
-        _trace(f"Get '{path}'")
+        _trace(f"{method} '{path}'")
 
         ignore_expiry = kwargs.pop("ignore_expiry", False)
         if cache is not None and path in cache:
@@ -559,7 +559,7 @@ def _playlist_edit(web_client, playlist, method, **kwargs):
     logger.debug(f"API request: {method} {url}")
     response = method(url, json=kwargs)
     if not response.status_ok:
-        raise RuntimeError(response.message)
+        raise OAuthClientError(response.message)
 
     logger.debug(f"API response: {response}")
 
@@ -581,6 +581,7 @@ def delete_playlist(web_client, uri):
     url = f"playlists/{playlist_id}/followers"
     response = web_client.delete(url)
     web_client.remove_from_cache("me/playlists")
+    web_client.remove_from_cache(f"playlists/{playlist_id}")
     return response.status_ok
 
 

tests/test_playlists.py

@@ -334,7 +334,11 @@ def test_playlist_save_failed1(provider, mopidy_track_factory, caplog):
     provider._len_replace = lambda x: 0  # force replace mode
     retval = provider.save(new_pl)
     assert retval is None
-    assert "Refusing to modify someone else's playlist" in caplog.text
+    assert (
+        "Cannot modify Spotify playlist 'spotify:user:bob:playlist:baz' "
+        "owned by other user bob"
+        in caplog.text
+    )
 
 
 def test_playlist_save_failed2(provider, mopidy_track_factory, caplog):
@@ -372,7 +376,10 @@ def test_playlist_save_failed3(provider, caplog):
     new_pl = playlist.replace(tracks=tracks)
     retval = provider.save(new_pl)
     assert retval is playlist
-    assert "Cannot add non-spotify tracks to spotify playlist" in caplog.text
+    assert (
+        "Skipping adding non-Spotify tracks to Spotify playlist 'spotify:user:alice:playlist:foo'"
+        in caplog.text
+    )
 
 
 def test_replace_playlist_short(provider, mopidy_track_factory):
@@ -1076,6 +1083,20 @@ def test_refuse_on_local_files(provider, caplog):
     retval = provider.save(playlist)
     assert retval is None
     assert (
-        "Cannot modify playlist containing 'Spotify Local Files'."
+        "Cannot modify Spotify playlist containing Spotify 'local files'."
         in caplog.text
     )
+
+
+def test_span_1():
+    movs = []
+    a, b = playlists.SpotifyPlaylistsProvider._split_ended_movs(1, movs)
+    assert a == []
+    assert b == []
+
+
+def test_span_2():
+    movs = [(0, 10), (2, 20)]
+    a, b = playlists.SpotifyPlaylistsProvider._split_ended_movs(1, movs)
+    assert a == [(0, 10)]
+    assert b == [(2, 20)]

tests/test_translator.py

@@ -736,6 +736,16 @@ def test_ignores_invalid_artists(self, web_album_mock, web_artist_mock):
         assert album.name == "DEF 456"
         assert list(album.artists) == artists
 
+    def test_release_date_formats(self, web_album_mock):
+        web_album_mock["release_date"] = "2001"
+        album = translator.web_to_album(web_album_mock)
+        assert album.date == "2001"
+
+    def test_release_date_formats_truncating(self, web_album_mock):
+        web_album_mock["release_date"] = "2001-12-01"
+        album = translator.web_to_album(web_album_mock)
+        assert album.date == "2001"
+
 
 class TestWebToTrack:
     def test_calls_web_to_track_ref(self, web_track_mock):

tests/test_web.py

@@ -667,18 +667,22 @@ def test_cached_response_unchanged(web_response_mock, oauth_client, mock_time):
 def test_non_idempotent_request_methods(
     web_response_mock, oauth_client, mock_time
 ):
+    # Make sure non-idempotent HTTP methods ignore the cache
+    cache = {"foo": web_response_mock}
     responses.add(responses.POST, "https://api.spotify.com/v1/foo", json={})
     responses.add(responses.PUT, "https://api.spotify.com/v1/foo", json={})
     responses.add(responses.DELETE, "https://api.spotify.com/v1/foo", json={})
     oauth_client._expires = 2000
     mock_time.return_value = 1
 
-    result = oauth_client.post("foo")
-    result = oauth_client.put("foo")
-    result = oauth_client.delete("foo")
+    result1 = oauth_client.post("foo", cache)
+    result2 = oauth_client.put("foo", cache)
+    result3 = oauth_client.delete("foo", cache)
 
     assert len(responses.calls) == 3
-    assert not result.status_unchanged
+    assert not result1.status_unchanged
+    assert not result2.status_unchanged
+    assert not result3.status_unchanged
 
 
 @responses.activate