ML-177: implementation

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
 
 <!-- AC:BEGIN -->
 
-- [ ] #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] #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
+- [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
-- [ ] #3 POST /api/v1/collection/:record_id/scrobble returns 401 when no valid Bearer token is provided
+- [x] #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
+- [x] #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)
+- [x] #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)
+- [x] #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
+- [x] #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] #8 JSON view test verifies selected_release_id in record output
 <!-- AC:END -->
 
 ## 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.
 
 <!-- SECTION:PLAN:END -->
+
+## Final Summary
+
+<!-- SECTION:FINAL_SUMMARY:BEGIN -->
+
+## 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).
+<!-- SECTION:FINAL_SUMMARY:END -->

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

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)}"),

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

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 <- [
+      for {method, path} <- [
-            ~p"/api/v1/collection/latest",
+            {:get, ~p"/api/v1/collection/latest"},
-            ~p"/api/v1/collection/random",
+            {:get, ~p"/api/v1/collection/random"},
-            ~p"/api/v1/collection",
+            {:get, ~p"/api/v1/collection"},
-            ~p"/api/v1/collection/on_this_day"
+            {:get, ~p"/api/v1/collection/on_this_day"},
+            {:post, ~p"/api/v1/collection/nonexistent/scrobble"}
           ] do
-        assert get(conn, path).status == 401,
+        conn =
-               "expected 401 for unauthenticated GET #{path}"
+          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,