diff --git a/backlog/tasks/ml-177 - Add-scrobble-API-endpoint-and-bottom-scrobble-button-in-record-view-with-Done-state.md b/backlog/completed/ml-177 - Add-scrobble-API-endpoint-and-bottom-scrobble-button-in-record-view-with-Done-state.md similarity index 74% rename from backlog/tasks/ml-177 - Add-scrobble-API-endpoint-and-bottom-scrobble-button-in-record-view-with-Done-state.md rename to backlog/completed/ml-177 - Add-scrobble-API-endpoint-and-bottom-scrobble-button-in-record-view-with-Done-state.md index ad48ef65..b77c94be 100644 --- a/backlog/tasks/ml-177 - Add-scrobble-API-endpoint-and-bottom-scrobble-button-in-record-view-with-Done-state.md +++ b/backlog/completed/ml-177 - Add-scrobble-API-endpoint-and-bottom-scrobble-button-in-record-view-with-Done-state.md @@ -3,10 +3,10 @@ id: ML-177 title: >- Add POST /api/v1/collection/:record_id/scrobble endpoint and expose selected_release_id in API responses -status: To Do +status: Done assignee: [] created_date: "2026-05-10 18:46" -updated_date: "2026-05-10 19:19" +updated_date: "2026-05-10 19:36" labels: - api - scrobble @@ -22,6 +22,11 @@ references: - >- backlog/tasks/ml-170 - Expose-additional-data-points-for-api-v1-collection-records.md +modified_files: + - lib/music_library_web/controllers/collection_json.ex + - lib/music_library_web/router.ex + - lib/music_library_web/controllers/collection_controller.ex + - test/music_library_web/controllers/collection_controller_test.exs priority: medium ordinal: 4000 --- @@ -57,14 +62,14 @@ The record view (`CollectionLive.Show`) in the web UI already has a scrobble but -- [ ] #1 POST /api/v1/collection/:record_id/scrobble returns 200 with {"status": "ok"} when scrobble succeeds and the record has a selected_release_id -- [ ] #2 POST /api/v1/collection/:record_id/scrobble returns 422 with {"status": "error", "reason": "no_selected_release"} when the record has no selected_release_id -- [ ] #3 POST /api/v1/collection/:record_id/scrobble returns 401 when no valid Bearer token is provided -- [ ] #4 POST /api/v1/collection/:record_id/scrobble returns 404 when the record ID does not exist -- [ ] #5 All collection API responses (index, latest, random, on_this_day) include selected_release_id field (string or null) -- [ ] #6 Existing API response fields are unchanged (backward compatible) -- [ ] #7 Controller test covers success, missing release_id, missing record, auth failure, and Last.fm error cases -- [ ] #8 JSON view test verifies selected_release_id in record output +- [x] #1 POST /api/v1/collection/:record_id/scrobble returns 200 with {"status": "ok"} when scrobble succeeds and the record has a selected_release_id +- [x] #2 POST /api/v1/collection/:record_id/scrobble returns 422 with {"status": "error", "reason": "no_selected_release"} when the record has no selected_release_id +- [x] #3 POST /api/v1/collection/:record_id/scrobble returns 401 when no valid Bearer token is provided +- [x] #4 POST /api/v1/collection/:record_id/scrobble returns 404 when the record ID does not exist +- [x] #5 All collection API responses (index, latest, random, on_this_day) include selected_release_id field (string or null) +- [x] #6 Existing API response fields are unchanged (backward compatible) +- [x] #7 Controller test covers success, missing release_id, missing record, auth failure, and Last.fm error cases +- [x] #8 JSON view test verifies selected_release_id in record output ## Implementation Plan @@ -144,3 +149,50 @@ The work comprises 4 steps with explicit dependencies: **Dependencies**: Step 4 depends on Steps 1-3 being complete. Within Step 4, the JSON view assertion can be written as soon as Step 1 is done. + +## Final Summary + + + +## Changes + +### 1. `lib/music_library_web/controllers/collection_json.ex` + +- Added `selected_release_id` field to the `record/1` private function's response map, exposing it in all collection API responses (index, latest, random, on_this_day). + +### 2. `lib/music_library_web/router.ex` + +- Added `post "/collection/:record_id/scrobble", CollectionController, :scrobble` to the `:api` pipeline scope, protected by the existing `require_api_token` plug. + +### 3. `lib/music_library_web/controllers/collection_controller.ex` + +- Added aliases for `MusicBrainz`, `MusicLibrary.Records`, and `MusicLibrary.ScrobbleActivity`. +- Added `scrobble/2` action with the following response codes: + - **404** — Record not found (`Records.get_record/1` returns nil; `phoenix_ecto` 4.x does not auto-convert `Ecto.NoResultsError`, so explicit handling was used) + - **422** — No `selected_release_id` set on the record + - **200** — Scrobble succeeded (`{"status": "ok"}`) + - **422** — MusicBrainz release has zero-duration tracks + - **502** — MusicBrainz API error + - **502** — Last.fm scrobbling error + - **503** — Last.fm session key not configured +- Added private `do_scrobble/2` to keep the action readable and separate record lookup from scrobble logic. + +### 4. `test/music_library_web/controllers/collection_controller_test.exs` + +- Added `selected_release_id` to `expected_record_json/1` (string value matching the fixture's `"d3f9b9e2-73f5-4b47-a2a7-2c2199aad608"`) +- Updated auth test to include the scrobble POST endpoint +- Added scrobble test describe block with: + - 401 test (no bearer token) + - 404 test (non-existent record) + - 422 test (no selected_release_id) + - 200 success test (with MusicBrainz stub, Last.fm stub, and stored session key) + - 502 test for MusicBrainz failure + - 502 test for Last.fm failure (with session key so it reaches the Last.fm call) +- All API stubs use `Req.Test.stub/2` matching the project's test infrastructure pattern + +## Notes + +- The `phoenix_ecto` library in version 4.x does NOT automatically convert `Ecto.NoResultsError` to 404 in controller actions (this was a Phoenix.Ecto 3.x feature). The implementation uses `Records.get_record/1` (non-bang) and handles nil explicitly. +- The scrobble is synchronous — the full MusicBrainz lookup + Last.fm scrobble happens within the HTTP request cycle. This is intentional: the Presto device needs an immediate success/failure response and sub-second latency is acceptable for these API calls. +- All 974 tests pass (43 doctests + 931 ExUnit tests). + diff --git a/lib/music_library_web/controllers/collection_controller.ex b/lib/music_library_web/controllers/collection_controller.ex index 47e9106b..0652a92a 100644 --- a/lib/music_library_web/controllers/collection_controller.ex +++ b/lib/music_library_web/controllers/collection_controller.ex @@ -1,7 +1,10 @@ defmodule MusicLibraryWeb.CollectionController do use MusicLibraryWeb, :controller + alias MusicBrainz alias MusicLibrary.Collection + alias MusicLibrary.Records + alias MusicLibrary.ScrobbleActivity def latest(conn, _params) do latest_record = Collection.get_latest_record!() @@ -39,6 +42,60 @@ defmodule MusicLibraryWeb.CollectionController do render(conn, :index, total: total, limit: limit, offset: offset, records: records) end + def scrobble(conn, %{"record_id" => record_id}) do + case Records.get_record(record_id) do + nil -> + conn + |> put_status(404) + |> json(%{status: "error", reason: "not_found"}) + + record -> + do_scrobble(conn, record) + end + end + + defp do_scrobble(conn, record) do + if is_nil(record.selected_release_id) or record.selected_release_id == "" do + conn + |> put_status(422) + |> json(%{status: "error", reason: "no_selected_release"}) + else + case MusicBrainz.get_release(record.selected_release_id) do + {:ok, release} -> + release_with_tracks = MusicBrainz.Release.from_api_response(release) + + case ScrobbleActivity.scrobble_release( + release_with_tracks, + :finished_at, + DateTime.utc_now() + ) do + {:ok, _response} -> + json(conn, %{status: "ok"}) + + {:error, :no_duration} -> + conn + |> put_status(422) + |> json(%{status: "error", reason: "no_duration"}) + + {:error, :no_session_key} -> + conn + |> put_status(503) + |> json(%{status: "error", reason: "lastfm_not_configured"}) + + {:error, _reason} -> + conn + |> put_status(502) + |> json(%{status: "error", reason: "lastfm_error"}) + end + + {:error, _reason} -> + conn + |> put_status(502) + |> json(%{status: "error", reason: "musicbrainz_error"}) + end + end + end + defp parse_int(nil, default), do: default defp parse_int(value, default) when is_binary(value) do diff --git a/lib/music_library_web/controllers/collection_json.ex b/lib/music_library_web/controllers/collection_json.ex index 65b46813..19971b83 100644 --- a/lib/music_library_web/controllers/collection_json.ex +++ b/lib/music_library_web/controllers/collection_json.ex @@ -31,6 +31,7 @@ defmodule MusicLibraryWeb.CollectionJSON do genres: record.genres, release_date: record.release_date, purchased_at: record.purchased_at, + selected_release_id: record.selected_release_id, artists: Enum.map(record.artists, & &1.name), title: record.title, cover_url: url(~p"/api/v1/assets/#{Transform.new(hash: record.cover_hash)}"), diff --git a/lib/music_library_web/router.ex b/lib/music_library_web/router.ex index 91056788..44bb5cf3 100644 --- a/lib/music_library_web/router.ex +++ b/lib/music_library_web/router.ex @@ -130,6 +130,7 @@ defmodule MusicLibraryWeb.Router do get "/collection/random", CollectionController, :random get "/collection/on_this_day", CollectionController, :on_this_day get "/collection", CollectionController, :index + post "/collection/:record_id/scrobble", CollectionController, :scrobble get "/errors", ErrorController, :index get "/errors/:id", ErrorController, :show post "/errors/:id/mute", ErrorController, :mute diff --git a/test/music_library_web/controllers/collection_controller_test.exs b/test/music_library_web/controllers/collection_controller_test.exs index 59c9aff0..87dc7b8a 100644 --- a/test/music_library_web/controllers/collection_controller_test.exs +++ b/test/music_library_web/controllers/collection_controller_test.exs @@ -3,6 +3,10 @@ defmodule MusicLibraryWeb.CollectionControllerTest do import MusicLibrary.Fixtures.Records + alias MusicBrainz.Fixtures.Release, as: ReleaseFixtures + alias MusicLibrary.Secrets + alias Req.Test + defp create_record(_) do %{record: record_with_artist("Steven Wilson", %{release_date: "2025-06-21"})} end @@ -14,14 +18,21 @@ defmodule MusicLibraryWeb.CollectionControllerTest do describe "authentication" do test "all API endpoints require a bearer token", %{conn: conn} do - for path <- [ - ~p"/api/v1/collection/latest", - ~p"/api/v1/collection/random", - ~p"/api/v1/collection", - ~p"/api/v1/collection/on_this_day" + for {method, path} <- [ + {:get, ~p"/api/v1/collection/latest"}, + {:get, ~p"/api/v1/collection/random"}, + {:get, ~p"/api/v1/collection"}, + {:get, ~p"/api/v1/collection/on_this_day"}, + {:post, ~p"/api/v1/collection/nonexistent/scrobble"} ] do - assert get(conn, path).status == 401, - "expected 401 for unauthenticated GET #{path}" + conn = + case method do + :get -> get(conn, path) + :post -> post(conn, path) + end + + assert conn.status == 401, + "expected 401 for unauthenticated #{String.upcase(to_string(method))} #{path}" end end end @@ -161,6 +172,143 @@ defmodule MusicLibraryWeb.CollectionControllerTest do end end + describe "POST /api/v1/collection/:record_id/scrobble" do + defp stub_musicbrainz_release(_) do + Test.stub(MusicBrainz.API, fn conn -> + case conn.request_path do + "/ws/2/release/" <> _id -> + Test.json(conn, ReleaseFixtures.release_with_media(:marbles)) + + _ -> + Test.json(conn, %{}) + end + end) + + :ok + end + + defp stub_lastfm_success(_) do + Test.stub(LastFm.API, fn conn -> + Test.json(conn, %{"scrobbles" => %{"@attr" => %{"accepted" => 1}}}) + end) + + :ok + end + + defp stub_lastfm_error(_) do + Test.stub(LastFm.API, fn conn -> + Test.json(conn, %{ + "error" => 16, + "message" => "The service is temporarily unavailable" + }) + end) + + :ok + end + + defp stub_musicbrainz_error(_) do + Test.stub(MusicBrainz.API, fn conn -> + Plug.Conn.send_resp(conn, 503, "service unavailable") + end) + + :ok + end + + defp store_session_key(_) do + Secrets.store("last_fm_session_key", "test_session_key") + + on_exit(fn -> + Secrets.store("last_fm_session_key", nil) + end) + + :ok + end + + defp auth_header(conn) do + put_req_header(conn, "authorization", "Bearer #{api_token()}") + end + + setup [:create_record] + + test "returns 401 without bearer token", %{conn: conn, record: record} do + conn = post(conn, ~p"/api/v1/collection/#{record.id}/scrobble") + + assert conn.status == 401 + end + + test "returns 404 for non-existent record", %{conn: conn} do + conn = + conn + |> auth_header() + |> post(~p"/api/v1/collection/00000000-0000-0000-0000-000000000000/scrobble") + + assert json_response(conn, 404) == %{ + "status" => "error", + "reason" => "not_found" + } + end + + test "returns 422 when record has no selected_release_id", %{conn: conn} do + record = record_with_artist("No Release Artist", %{selected_release_id: nil}) + + conn = + conn + |> auth_header() + |> post(~p"/api/v1/collection/#{record.id}/scrobble") + + assert json_response(conn, 422) == %{ + "status" => "error", + "reason" => "no_selected_release" + } + end + + @tag scrobble_success: true + test "returns 200 when scrobble succeeds", %{conn: conn, record: record} do + stub_musicbrainz_release(nil) + stub_lastfm_success(nil) + store_session_key(nil) + + conn = + conn + |> auth_header() + |> post(~p"/api/v1/collection/#{record.id}/scrobble") + + assert json_response(conn, 200) == %{"status" => "ok"} + end + + @tag :capture_log + test "returns 502 when MusicBrainz API fails", %{conn: conn, record: record} do + stub_musicbrainz_error(nil) + + conn = + conn + |> auth_header() + |> post(~p"/api/v1/collection/#{record.id}/scrobble") + + assert json_response(conn, 502) == %{ + "status" => "error", + "reason" => "musicbrainz_error" + } + end + + @tag :capture_log + test "returns 502 when Last.fm API fails", %{conn: conn, record: record} do + stub_musicbrainz_release(nil) + stub_lastfm_error(nil) + store_session_key(nil) + + conn = + conn + |> auth_header() + |> post(~p"/api/v1/collection/#{record.id}/scrobble") + + assert json_response(conn, 502) == %{ + "status" => "error", + "reason" => "lastfm_error" + } + end + end + defp expected_record_json(record) do %{ "id" => record.id, @@ -169,6 +317,7 @@ defmodule MusicLibraryWeb.CollectionControllerTest do "musicbrainz_id" => record.musicbrainz_id, "genres" => record.genres, "release_date" => record.release_date, + "selected_release_id" => record.selected_release_id, "purchased_at" => record.purchased_at |> DateTime.to_iso8601(), "artists" => Enum.map(record.artists, & &1.name), "title" => record.title,