Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions src/mopidy_spotify/browse.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

_PLAYLISTS_DIR_CONTENTS = [
models.Ref.directory(uri="spotify:playlists:featured", name="Featured"),
models.Ref.directory(uri="spotify:playlists:new-releases", name="New releases"),
]


Expand Down Expand Up @@ -183,20 +184,44 @@ def _browse_your_music(web_client, variant):


def _browse_playlists(web_client, variant):
"""Browse playlist-related directories (featured playlists, new releases).

Args:
web_client: The Spotify OAuth client for API requests.
variant: The playlist variant to browse ('featured' or 'new-releases').

Returns:
A list of Mopidy Ref objects (playlists for 'featured', albums for
'new-releases').
"""
Comment on lines +187 to +196
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment is better encoded as type hints:

def _browse_playlists(
    web_client: SomeTypeIDidntLookupNow,
    variant: Literal["featured", "new-releases",
) -> list[Ref]:

if not web_client.logged_in:
return []

if variant != "featured":
return []
if variant == "featured":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the function is typed with variant as a Literal, that makes the if variant = ... statements a prime candidate for a match variant: instead. We're now requiring a new enough Python version for match to be used freely 🎉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. Thank you for the explanation, I'm still learning Python patterns and the API.

items = flatten(
[
page.get("playlists", {}).get("items", [])
for page in web_client.get_all(
"browse/featured-playlists",
params={"limit": 50},
)
if page
]
)
Comment on lines +201 to +210
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that you're following the pattern of existing code, but I'm leaving a comment anyway:

I think you can drop the call to flatten() if you just change page.get(...) to *page.get(...). If you do the same with the other calls to flatten() in this file, we can get rid of the util function.

return list(translator.to_playlist_refs(items))

items = flatten(
[
page.get("playlists", {}).get("items", [])
for page in web_client.get_all(
"browse/featured-playlists",
params={"limit": 50},
)
if page
]
)
return list(translator.to_playlist_refs(items))
if variant == "new-releases":
items = flatten(
[
page.get("albums", {}).get("items", [])
for page in web_client.get_all(
"browse/new-releases",
params={"limit": 50},
)
if page
]
)
return list(translator.web_to_album_refs(items))

logger.info(f"Failed to browse 'spotify:playlists:{variant}': Unknown URI type")
return []
157 changes: 156 additions & 1 deletion tests/test_browse.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,18 @@ def test_browse_your_music_directory(provider):


def test_browse_playlists_directory(provider):
"""Test browsing the playlists directory returns all playlist categories."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, skip comments if they don't add anything, e.g. important reasoning for why something is the way it is.

results = provider.browse("spotify:playlists")

assert len(results) == 1
assert len(results) == 2
assert (
models.Ref.directory(uri="spotify:playlists:featured", name="Featured")
in results
)
assert (
models.Ref.directory(uri="spotify:playlists:new-releases", name="New releases")
in results
)


def test_browse_playlist(web_client_mock, web_playlist_mock, provider):
Expand Down Expand Up @@ -305,3 +310,153 @@ def test_browse_playlists_featured(web_client_mock, web_playlist_mock, provider)
assert len(results) == 2
assert results[0].name == "Foo"
assert results[0].uri == "spotify:user:alice:playlist:foo"


def test_browse_new_releases(web_client_mock, web_album_mock_base, provider):
"""Test browsing new releases returns album refs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of these comments can be removed without loosing anything. Superflous comments adds content that gets outdated if not maintained, so they are not free.


Verifies successful response handling per Spotify Web API reference:
https://developer.spotify.com/documentation/web-api/reference/get-new-releases

Response structure: { "albums": { "items": [SimplifiedAlbumObject, ...] } }
"""
web_client_mock.get_all.return_value = [
{"albums": {"items": [web_album_mock_base]}},
{"albums": {"items": [web_album_mock_base]}},
]

results = provider.browse("spotify:playlists:new-releases")

web_client_mock.get_all.assert_called_once_with(
"browse/new-releases", params={"limit": 50}
)
assert len(results) == 2
assert results[0] == models.Ref.album(
uri="spotify:album:def", name="ABBA - DEF 456"
)


def test_browse_new_releases_empty(web_client_mock, provider):
"""Test browsing new releases when API returns empty results.

Per Spotify API docs, albums.items may be an empty array when no new
releases are available for the market.
https://developer.spotify.com/documentation/web-api/reference/get-new-releases
"""
web_client_mock.get_all.return_value = [{}]

results = provider.browse("spotify:playlists:new-releases")

web_client_mock.get_all.assert_called_once_with(
"browse/new-releases", params={"limit": 50}
)
assert len(results) == 0


def test_browse_new_releases_when_offline(web_client_mock, provider):
"""Test browsing new releases when not logged in.

Spotify API requires valid OAuth token. Error 401 (bad/expired token) and
403 (bad OAuth request) are handled by web_client before reaching browse.
https://developer.spotify.com/documentation/web-api/reference/get-new-releases
"""
web_client_mock.logged_in = False

results = provider.browse("spotify:playlists:new-releases")

web_client_mock.get_all.assert_not_called()
assert len(results) == 0


def test_browse_playlists_unknown_variant(web_client_mock, provider, caplog):
"""Test browsing unknown playlist variant logs warning and returns empty."""
results = provider.browse("spotify:playlists:unknown")

web_client_mock.get_all.assert_not_called()
assert len(results) == 0
assert "Unknown URI type" in caplog.text


# Defensive programming tests - verifying robust handling of edge cases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this set of tests takes things a bit far. Having one parametrized test that tests a few of these cases is okay, but we don't need to spend 80 lines of code on testing every possible permutation of broken responses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I say "parametrized test", I'm referring to this Pytest feature: https://docs.pytest.org/en/stable/how-to/parametrize.html#parametrize-basics

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this set of tests takes things a bit far. Having one parametrized test that tests a few of these cases is okay, but we don't need to spend 80 lines of code on testing every possible permutation of broken responses.

Thanks for the feedback! I appreciate you taking the time to review this thoroughly.
I hear you on the test coverage. Coming from a Node background, I tend toward comprehensive test cases as documentation and regression protection, but I recognize that may not align with this project's patterns. I'll convert these to a parametrized test as you've suggested - it's a good opportunity for me to learn the preferred testing approach here.
Thanks again for the guidance.

#
# These tests verify graceful handling of malformed or unexpected API responses.
# Per Spotify Web API reference, the expected response structure is:
# { "albums": { "href": str, "limit": int, "next": str|null, "offset": int,
# "previous": str|null, "total": int, "items": [SimplifiedAlbumObject] } }
# https://developer.spotify.com/documentation/web-api/reference/get-new-releases
#
# However, pagination and network issues may produce partial/malformed responses.
# The translator.valid_web_data() validates each album object has type="album" and uri.


def test_browse_new_releases_handles_none_pages(web_client_mock, provider):
"""Test that None pages in the response are safely filtered out.

Pagination via web_client.get_all() may yield None for failed page fetches
(e.g., network timeouts, rate limiting via 429 status).
"""
web_client_mock.get_all.return_value = [
None,
{"albums": {"items": []}},
None,
]

results = provider.browse("spotify:playlists:new-releases")

assert len(results) == 0


def test_browse_new_releases_handles_missing_albums_key(web_client_mock, provider):
"""Test that pages missing the 'albums' key are handled gracefully.

While the API spec defines 'albums' as required, defensive coding handles
unexpected response shapes that may occur during API changes or errors.
"""
web_client_mock.get_all.return_value = [
{"unexpected_key": {"items": []}},
{},
]

results = provider.browse("spotify:playlists:new-releases")

assert len(results) == 0


def test_browse_new_releases_handles_missing_items_key(
web_client_mock, web_album_mock_base, provider
):
"""Test that pages with 'albums' but missing 'items' are handled gracefully.

The API spec shows 'items' as required within 'albums', but we use .get()
with empty list default to handle partial responses defensively.
"""
web_client_mock.get_all.return_value = [
{"albums": {}},
{"albums": {"items": [web_album_mock_base]}},
]

results = provider.browse("spotify:playlists:new-releases")

assert len(results) == 1


def test_browse_new_releases_handles_mixed_valid_invalid_pages(
web_client_mock, web_album_mock_base, provider
):
"""Test that valid data is extracted even when mixed with invalid pages.

Real-world pagination may encounter intermittent failures. This verifies
the implementation extracts all valid albums while gracefully skipping
malformed pages, ensuring maximum data availability.
"""
web_client_mock.get_all.return_value = [
None,
{"albums": {"items": [web_album_mock_base]}},
{},
{"albums": {}},
{"albums": {"items": [web_album_mock_base, web_album_mock_base]}},
]

results = provider.browse("spotify:playlists:new-releases")

assert len(results) == 3