diff --git a/lib/music_library/records/record.ex b/lib/music_library/records/record.ex index e2efd30b..f3ac9285 100644 --- a/lib/music_library/records/record.ex +++ b/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 diff --git a/lib/music_library/worker/import_from_musicbrainz_release.ex b/lib/music_library/worker/import_from_musicbrainz_release.ex index 44e090df..24cbdbbf 100644 --- a/lib/music_library/worker/import_from_musicbrainz_release.ex +++ b/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 diff --git a/lib/music_library/worker/import_from_musicbrainz_release_group.ex b/lib/music_library/worker/import_from_musicbrainz_release_group.ex index 71968ba8..b8ff775f 100644 --- a/lib/music_library/worker/import_from_musicbrainz_release_group.ex +++ b/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 diff --git a/lib/music_library_web/components/add_record.ex b/lib/music_library_web/components/add_record.ex index a83d4bae..b5ac8e62 100644 --- a/lib/music_library_web/components/add_record.ex +++ b/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) - notify_parent({:imported_async, length(items)}) + 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) diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 391c7ac5..7d60f26a 100644 --- a/priv/gettext/default.pot +++ b/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 "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 064d74ac..f4cad4ac 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/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 "" diff --git a/test/music_library/worker/import_from_musicbrainz_release_group_test.exs b/test/music_library/worker/import_from_musicbrainz_release_group_test.exs index 37ba8ed4..02ccc956 100644 --- a/test/music_library/worker/import_from_musicbrainz_release_group_test.exs +++ b/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 diff --git a/test/music_library_web/live/collection_live/index_test.exs b/test/music_library_web/live/collection_live/index_test.exs index d6f1172b..cc7650e6 100644 --- a/test/music_library_web/live/collection_live/index_test.exs +++ b/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) diff --git a/test/music_library_web/live/wishlist_live/index_test.exs b/test/music_library_web/live/wishlist_live/index_test.exs index 71d312c8..048dc0c4 100644 --- a/test/music_library_web/live/wishlist_live/index_test.exs +++ b/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