Fix ML-143 review: security, worker, and test improvements

Closes ML-143 review findings:
- cast_id/1 uses Integer.parse instead of String.to_integer
- add_to_cart validates against server-side release_groups
- JS.push payload trimmed to {id, format} only
- Oban.insert_all result checked for partial failures
- handle_async success resets @importing? to false
- ImportFromMusicbrainzReleaseGroup gets unique constraint
- Record.parse_datetime/1 made public with safe match
- Workers delegate to Record.parse_datetime/1
- Test coverage: change_format, clear_cart, wishlist single import

lib/music_library/records/record.ex

@@ -349,10 +349,13 @@ defmodule MusicLibrary.Records.Record do
     defp parse_format(unquote(Atom.to_string(format))), do: unquote(format)
   end
 
-  defp parse_datetime(nil), do: nil
+  @doc false
+  def parse_datetime(nil), do: nil
 
-  defp parse_datetime(dt_string) do
-    {:ok, dt, _offset} = DateTime.from_iso8601(dt_string)
-    dt
+  def parse_datetime(str) when is_binary(str) do
+    case DateTime.from_iso8601(str) do
+      {:ok, dt, _offset} -> dt
+      {:error, _} -> nil
+    end
   end
 end

lib/music_library/worker/import_from_musicbrainz_release.ex

@@ -7,11 +7,13 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do
 
   use Oban.Worker, queue: :music_brainz, max_attempts: 3
 
+  alias MusicLibrary.Records.Record
+
   @impl Oban.Worker
   def perform(%Oban.Job{args: %{"release_id" => release_id} = args}) do
     opts = [
       format: args["format"],
-      purchased_at: parse_datetime(args["purchased_at"]),
+      purchased_at: Record.parse_datetime(args["purchased_at"]),
       selected_release_id: args["selected_release_id"]
     ]
 
@@ -20,11 +22,4 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do
       {:error, reason} -> {:error, reason}
     end
   end
-
-  defp parse_datetime(nil), do: nil
-
-  defp parse_datetime(str) do
-    {:ok, datetime, _offset} = DateTime.from_iso8601(str)
-    datetime
-  end
 end

lib/music_library/worker/import_from_musicbrainz_release_group.ex

@@ -6,15 +6,19 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup do
   records to import at once.
   """
 
-  use Oban.Worker, queue: :music_brainz, max_attempts: 3
+  use Oban.Worker,
+    queue: :music_brainz,
+    max_attempts: 3,
+    unique: [period: 300, keys: [:release_group_id, :format]]
 
   alias MusicLibrary.Records
+  alias MusicLibrary.Records.Record
 
   @impl Oban.Worker
   def perform(%Oban.Job{args: %{"release_group_id" => release_group_id} = args}) do
     opts = [
       format: args["format"],
-      purchased_at: parse_datetime(args["purchased_at"])
+      purchased_at: Record.parse_datetime(args["purchased_at"])
     ]
 
     case Records.import_from_musicbrainz_release_group(release_group_id, opts) do
@@ -22,11 +26,4 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup do
       {:error, reason} -> {:error, reason}
     end
   end
-
-  defp parse_datetime(nil), do: nil
-
-  defp parse_datetime(str) do
-    {:ok, datetime, _offset} = DateTime.from_iso8601(str)
-    datetime
-  end
 end

lib/music_library_web/components/add_record.ex

@@ -291,14 +291,7 @@ defmodule MusicLibraryWeb.Components.AddRecord do
               id={"actions-#{@release_group.id}-#{format}-add"}
               phx-click={
                 JS.push("add_to_cart",
-                  value: %{
-                    id: @release_group.id,
-                    format: format,
-                    title: @release_group.title,
-                    artists: @release_group.artists,
-                    release_date: @release_group.release_date,
-                    thumb_url: ReleaseGroupSearchResult.thumb_url(@release_group)
-                  },
+                  value: %{id: @release_group.id, format: format},
                   target: @myself
                 )
               }
@@ -418,27 +411,29 @@ defmodule MusicLibraryWeb.Components.AddRecord do
     end
   end
 
-  def handle_event("add_to_cart", params, socket) do
-    case parse_format(params["format"]) do
-      {:ok, format} ->
-        {:noreply, add_to_cart(socket, params, format)}
-
-      :error ->
-        {:noreply, socket}
+  def handle_event("add_to_cart", %{"id" => rg_id, "format" => format_str}, socket) do
+    with release_group when not is_nil(release_group) <-
+           Enum.find(socket.assigns.release_groups, &(&1.id == rg_id)),
+         {:ok, format} <- parse_format(format_str) do
+      {:noreply, add_to_cart(socket, release_group, format)}
+    else
+      _ -> {:noreply, socket}
     end
   end
 
   def handle_event("remove_from_cart", %{"cart_item_id" => id}, socket) do
-    id = cast_id(id)
-    {:noreply, remove_cart_item(socket, id)}
+    case cast_id(id) do
+      {:ok, id} -> {:noreply, remove_cart_item(socket, id)}
+      :error -> {:noreply, socket}
+    end
   end
 
   def handle_event("change_format", %{"cart_item_id" => id, "format" => format_str}, socket) do
-    id = cast_id(id)
-
-    case parse_format(format_str) do
-      {:ok, format} -> {:noreply, change_cart_format(socket, id, format)}
-      :error -> {:noreply, socket}
+    with {:ok, id} <- cast_id(id),
+         {:ok, format} <- parse_format(format_str) do
+      {:noreply, change_cart_format(socket, id, format)}
+    else
+      _ -> {:noreply, socket}
     end
   end
 
@@ -484,8 +479,18 @@ defmodule MusicLibraryWeb.Components.AddRecord do
             })
           end)
 
-        Oban.insert_all(changesets)
+        jobs = Oban.insert_all(changesets)
+
+        if length(jobs) == length(changesets) do
           notify_parent({:imported_async, length(items)})
+        else
+          Logger.error(
+            "Cart import job enqueue failed: only #{length(jobs)}/#{length(changesets)} jobs inserted"
+          )
+
+          put_toast!(:error, gettext("Error queuing records for import"))
+        end
+
         {:noreply, socket}
     end
   end
@@ -495,7 +500,7 @@ defmodule MusicLibraryWeb.Components.AddRecord do
   # component and is silently dropped by Phoenix — no user-visible bug.
   def handle_async(:import_cart, {:ok, {:ok, record}}, socket) do
     notify_parent({:imported_single, record})
-    {:noreply, socket}
+    {:noreply, assign(socket, :importing?, false)}
   end
 
   def handle_async(:import_cart, {:ok, {:error, reason}}, socket) do
@@ -513,20 +518,19 @@ defmodule MusicLibraryWeb.Components.AddRecord do
     {:noreply, assign(socket, :importing?, false)}
   end
 
-  defp add_to_cart(socket, params, format) do
-    rg_id = params["id"]
-    pair = {rg_id, format}
+  defp add_to_cart(socket, %ReleaseGroupSearchResult{} = release_group, format) do
+    pair = {release_group.id, format}
 
     if MapSet.member?(socket.assigns.cart_pairs, pair) do
       socket
     else
       item = %{
         cart_item_id: System.unique_integer([:positive]),
-        release_group_id: rg_id,
-        title: params["title"],
-        artists: params["artists"],
-        release_date: params["release_date"],
-        thumb_url: params["thumb_url"],
+        release_group_id: release_group.id,
+        title: release_group.title,
+        artists: release_group.artists,
+        release_date: release_group.release_date,
+        thumb_url: ReleaseGroupSearchResult.thumb_url(release_group),
         format: format
       }
 
@@ -598,8 +602,14 @@ defmodule MusicLibraryWeb.Components.AddRecord do
     if format in Records.Record.formats(), do: {:ok, format}, else: :error
   end
 
-  defp cast_id(id) when is_integer(id), do: id
-  defp cast_id(id) when is_binary(id), do: String.to_integer(id)
+  defp cast_id(id) when is_integer(id), do: {:ok, id}
+
+  defp cast_id(id) when is_binary(id) do
+    case Integer.parse(id) do
+      {int, ""} -> {:ok, int}
+      _ -> :error
+    end
+  end
 
   defp in_cart?(cart_pairs, rg_id) do
     Enum.any?(cart_pairs, fn {id, _format} -> id == rg_id end)

priv/gettext/default.pot

@@ -2511,3 +2511,8 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "Your cart is empty"
 msgstr ""
+
+#: lib/music_library_web/components/add_record.ex
+#, elixir-autogen, elixir-format
+msgid "Error queuing records for import"
+msgstr ""

priv/gettext/en/LC_MESSAGES/default.po

@@ -2511,3 +2511,8 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "Your cart is empty"
 msgstr ""
+
+#: lib/music_library_web/components/add_record.ex
+#, elixir-autogen, elixir-format
+msgid "Error queuing records for import"
+msgstr ""

test/music_library/worker/import_from_musicbrainz_release_group_test.exs

@@ -1,10 +1,11 @@
 defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroupTest do
-  use MusicLibrary.DataCase
+  use MusicLibrary.DataCase, async: true
 
   import MusicBrainz.Fixtures.ReleaseGroup
   import MusicLibrary.Fixtures.Records
 
   alias MusicLibrary.Records.Record
+  alias MusicLibrary.Worker.FetchArtistInfo
   alias MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup
 
   describe "perform/1" do
@@ -41,6 +42,11 @@ defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroupTest do
       assert imported_record.title == "Marbles"
       assert imported_record.format == :cd
       assert imported_record.purchased_at == DateTime.truncate(purchased_at, :second)
+
+      assert_enqueued(
+        worker: FetchArtistInfo,
+        args: %{"id" => "1932f5b6-0b7b-4050-b1df-833ca89e5f44"}
+      )
     end
 
     test "imports a wishlist record when purchased_at is nil" do

test/music_library_web/live/collection_live/index_test.exs

@@ -348,6 +348,82 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do
       assert_has(session, "#cart-empty")
     end
 
+    test "changes the format of a cart item", %{conn: conn} do
+      alias Phoenix.LiveViewTest, as: LVT
+
+      stub_release_group_search()
+
+      [first | _] = Map.get(release_group_search_results(), "release-groups")
+      first_id = first["id"]
+
+      {:ok, view, _html} = LVT.live(conn, ~p"/collection/import")
+
+      view
+      |> LVT.form("#import_form", %{"mb_query" => "Marillion Marbles"})
+      |> LVT.render_change()
+
+      view
+      |> LVT.element("#musicbrainz_#{first_id} a", "CD")
+      |> LVT.render_click()
+
+      view
+      |> LVT.element("#cart-items form")
+      |> LVT.render_change(%{"format" => "vinyl"})
+
+      assert LVT.render(view) =~ "In cart · Vinyl"
+    end
+
+    test "rejects change_format when the resulting pair is already in the cart", %{conn: conn} do
+      alias Phoenix.LiveViewTest, as: LVT
+
+      stub_release_group_search()
+
+      [first | _] = Map.get(release_group_search_results(), "release-groups")
+      first_id = first["id"]
+
+      {:ok, view, _html} = LVT.live(conn, ~p"/collection/import")
+
+      view
+      |> LVT.form("#import_form", %{"mb_query" => "Marillion Marbles"})
+      |> LVT.render_change()
+
+      view
+      |> LVT.element("#musicbrainz_#{first_id} a", "CD")
+      |> LVT.render_click()
+
+      view
+      |> LVT.element("#musicbrainz_#{first_id} a", "Vinyl")
+      |> LVT.render_click()
+
+      html = LVT.render(view)
+      cart_item_ids = Regex.scan(~r/id="cart-item-(\d+)"/, html, capture: :all_but_first)
+      assert length(cart_item_ids) == 2
+
+      [first_id_match | _] = cart_item_ids
+      [item_id] = first_id_match
+
+      view
+      |> LVT.element("#cart-item-#{item_id} form")
+      |> LVT.render_change(%{"cart_item_id" => item_id, "format" => "vinyl"})
+
+      html_after = LVT.render(view)
+      assert Regex.scan(~r/id="cart-item-/, html_after) |> length() == 2
+    end
+
+    test "clears the cart", %{conn: conn} do
+      stub_release_group_search()
+
+      [first | _] = Map.get(release_group_search_results(), "release-groups")
+      first_id = first["id"]
+
+      conn
+      |> visit(~p"/collection/import")
+      |> fill_in("Search for a record", with: "Marillion Marbles")
+      |> click_link("#musicbrainz_#{first_id} a", "CD")
+      |> click_button("Clear all")
+      |> assert_has("#cart-empty")
+    end
+
     test "imports a single cart item synchronously and navigates", %{conn: conn} do
       alias Phoenix.LiveViewTest, as: LVT
 
@@ -378,7 +454,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do
       assert record.musicbrainz_id == first_id
       assert record.title == "Marbles"
       assert record.format == :cd
-      assert record.purchased_at != nil
+      refute is_nil(record.purchased_at)
 
       {:ok, resized_cover_data} = Image.resize(marbles_cover_data())
       assets = Assets.get(record.cover_hash)

test/music_library_web/live/wishlist_live/index_test.exs

@@ -2,8 +2,10 @@ defmodule MusicLibraryWeb.WishlistLive.IndexTest do
   use MusicLibraryWeb.ConnCase
   use Oban.Testing, repo: MusicLibrary.BackgroundRepo
 
+  import Ecto.Query, only: [from: 2]
   import MusicBrainz.Fixtures.ReleaseGroup
   import MusicLibrary.Fixtures.Records
+  import Phoenix.LiveViewTest, only: [assert_redirect: 2]
 
   alias MusicLibrary.Records.Record
   alias MusicLibrary.Worker.ImportFromMusicbrainzReleaseGroup
@@ -29,18 +31,13 @@ defmodule MusicLibraryWeb.WishlistLive.IndexTest do
 
       purchased_record = MusicLibrary.Records.get_record!(record.id)
 
-      assert purchased_record.purchased_at !== nil
+      refute is_nil(purchased_record.purchased_at)
     end
   end
 
   describe "Adding a new record" do
     test "enqueues one job per cart item with purchased_at: nil for wishlist", %{conn: conn} do
-      Req.Test.stub(MusicBrainz.API, fn conn ->
-        case conn.path_info do
-          [_ws, _version, "release-group"] ->
-            Req.Test.json(conn, release_group_search_results())
-        end
-      end)
+      stub_release_group_search()
 
       [first, second | _] = Map.get(release_group_search_results(), "release-groups")
       first_id = first["id"]
@@ -72,9 +69,77 @@ defmodule MusicLibraryWeb.WishlistLive.IndexTest do
         }
       )
 
-      import Ecto.Query, only: [from: 2]
       query = from(r in Record, where: not is_nil(r.purchased_at))
       assert MusicLibrary.Repo.all(query) == []
     end
+
+    test "imports a single cart item synchronously and navigates to wishlist", %{conn: conn} do
+      alias Phoenix.LiveViewTest, as: LVT
+
+      stub_full_import()
+
+      [first | _] = Map.get(release_group_search_results(), "release-groups")
+      first_id = first["id"]
+
+      {:ok, view, _html} = LVT.live(conn, ~p"/wishlist/import")
+
+      view
+      |> LVT.form("#import_form", %{"mb_query" => "Marillion Marbles"})
+      |> LVT.render_change()
+
+      view
+      |> LVT.element("#musicbrainz_#{first_id} a", "CD")
+      |> LVT.render_click()
+
+      view
+      |> LVT.element("button", "Import 1 record")
+      |> LVT.render_click()
+
+      {path, _flash} = assert_redirect(view, 2_000)
+      "/wishlist/" <> record_id = path
+
+      record = MusicLibrary.Records.get_record!(record_id)
+
+      assert record.musicbrainz_id == first_id
+      assert record.title == "Marbles"
+      assert record.format == :cd
+      assert record.purchased_at == nil
+
+      refute_enqueued(worker: ImportFromMusicbrainzReleaseGroup)
+    end
+  end
+
+  defp stub_release_group_search do
+    Req.Test.stub(MusicBrainz.API, fn conn ->
+      case conn.path_info do
+        [_ws, _version, "release-group"] ->
+          Req.Test.json(conn, release_group_search_results())
+      end
+    end)
+  end
+
+  defp stub_full_import do
+    [first | _] = Map.get(release_group_search_results(), "release-groups")
+    first_id = first["id"]
+
+    release_group = release_group(:marbles)
+    release_group_releases = release_group_releases(:marbles)
+    cover_data = marbles_cover_data()
+
+    Req.Test.stub(MusicBrainz.API, fn conn ->
+      case conn.path_info do
+        [_ws, _version, "release-group", ^first_id] ->
+          Req.Test.json(conn, release_group)
+
+        [_ws, _version, "release-group"] ->
+          Req.Test.json(conn, release_group_search_results())
+
+        [_ws, _version, "release"] ->
+          Req.Test.json(conn, release_group_releases)
+
+        [_release_group, ^first_id, "front"] ->
+          Plug.Conn.send_resp(conn, 200, cover_data)
+      end
+    end)
   end
 end