diff --git a/mopidy_spotify/playlists.py b/mopidy_spotify/playlists.py index eda50501..87b46bd4 100644 --- a/mopidy_spotify/playlists.py +++ b/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) diff --git a/mopidy_spotify/web.py b/mopidy_spotify/web.py index df7ddbb2..42f645cc 100644 --- a/mopidy_spotify/web.py +++ b/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 diff --git a/tests/test_playlists.py b/tests/test_playlists.py index 2c317b6e..225dc542 100644 --- a/tests/test_playlists.py +++ b/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)] diff --git a/tests/test_translator.py b/tests/test_translator.py index bb02ec90..8237a803 100644 --- a/tests/test_translator.py +++ b/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): diff --git a/tests/test_web.py b/tests/test_web.py index b55c6ebb..caac1afb 100644 --- a/tests/test_web.py +++ b/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