From b9be62a50080bc529307f7f6b8b8f34a473aa1df Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Sat, 4 Apr 2026 10:20:50 +0100 Subject: [PATCH] Async barcode scan batch import for 2+ new records When barcode scan results contain at least 2 new records, enqueue individual Oban jobs instead of importing synchronously. Wishlisted/collected/not_found results still process synchronously. --- lib/music_library/barcode_scan.ex | 27 +++ .../worker/import_from_musicbrainz_release.ex | 30 +++ .../components/barcode_scanner.ex | 50 ++-- priv/gettext/default.pot | 7 + priv/gettext/en/LC_MESSAGES/default.po | 7 + test/music_library/barcode_scan_test.exs | 214 ++++++++++++++++++ .../import_from_musicbrainz_release_test.exs | 67 ++++++ 7 files changed, 389 insertions(+), 13 deletions(-) create mode 100644 lib/music_library/worker/import_from_musicbrainz_release.ex create mode 100644 test/music_library/worker/import_from_musicbrainz_release_test.exs diff --git a/lib/music_library/barcode_scan.ex b/lib/music_library/barcode_scan.ex index 61ea6afb..4a57c52c 100644 --- a/lib/music_library/barcode_scan.ex +++ b/lib/music_library/barcode_scan.ex @@ -5,6 +5,7 @@ defmodule MusicLibrary.BarcodeScan do alias MusicLibrary.BarcodeScan.Result alias MusicLibrary.Records + alias MusicLibrary.Worker.ImportFromMusicbrainzRelease @spec scan(String.t()) :: {:ok, Result.t()} | {:error, term()} def scan(number) do @@ -31,6 +32,32 @@ defmodule MusicLibrary.BarcodeScan do end end + @spec should_import_async?([Result.t()]) :: boolean() + def should_import_async?(scan_results) do + Enum.count(scan_results, &(&1.status == :new)) >= 2 + end + + @spec import_results_async([Result.t()], DateTime.t()) :: + {:ok, sync_errors :: [{String.t(), term()}], async_count :: non_neg_integer()} + def import_results_async(scan_results, current_time) do + {new_results, other_results} = Enum.split_with(scan_results, &(&1.status == :new)) + + sync_errors = import_results(other_results, current_time) + + Enum.each(new_results, fn scan_result -> + %{ + "release_id" => scan_result.release.id, + "format" => MusicBrainz.ReleaseSearchResult.format(scan_result.release), + "purchased_at" => DateTime.to_iso8601(current_time), + "selected_release_id" => scan_result.release.id + } + |> ImportFromMusicbrainzRelease.new() + |> Oban.insert!() + end) + + {:ok, sync_errors, length(new_results)} + end + @spec import_results([Result.t()], DateTime.t()) :: [{String.t(), term()}] def import_results(scan_results, current_time) do Enum.reduce(scan_results, [], fn scan_result, errors -> diff --git a/lib/music_library/worker/import_from_musicbrainz_release.ex b/lib/music_library/worker/import_from_musicbrainz_release.ex new file mode 100644 index 00000000..44e090df --- /dev/null +++ b/lib/music_library/worker/import_from_musicbrainz_release.ex @@ -0,0 +1,30 @@ +defmodule MusicLibrary.Worker.ImportFromMusicbrainzRelease do + @moduledoc """ + Imports a record from a MusicBrainz release in the background. + + Used by barcode scan batch imports when there are multiple new records to import. + """ + + use Oban.Worker, queue: :music_brainz, max_attempts: 3 + + @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"]), + selected_release_id: args["selected_release_id"] + ] + + case MusicLibrary.Records.import_from_musicbrainz_release(release_id, opts) do + {:ok, _record} -> :ok + {: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/barcode_scanner.ex b/lib/music_library_web/components/barcode_scanner.ex index 95beb8b8..a0fedeaa 100644 --- a/lib/music_library_web/components/barcode_scanner.ex +++ b/lib/music_library_web/components/barcode_scanner.ex @@ -402,23 +402,32 @@ defmodule MusicLibraryWeb.Components.BarcodeScanner do def handle_event("import_releases", _params, socket) do current_time = DateTime.utc_now() + scan_results = socket.assigns.scan_results socket = - case BarcodeScan.import_results(socket.assigns.scan_results, current_time) do - [] -> - put_toast(socket, :info, gettext("Records imported successfully")) + if BarcodeScan.should_import_async?(scan_results) do + {:ok, sync_errors, async_count} = + BarcodeScan.import_results_async(scan_results, current_time) - errors -> - errors_summary = - Enum.map_join(errors, "\n", fn {number, reason} -> - "#{number}: #{ErrorMessages.friendly_message(reason)}" - end) - - put_toast( - socket, - :error, - gettext("Some records could not be imported: %{summary}", summary: errors_summary) + socket + |> maybe_toast_errors(sync_errors) + |> put_toast( + :info, + ngettext( + "Importing %{count} record in the background...", + "Importing %{count} records in the background...", + async_count, + count: async_count ) + ) + else + case BarcodeScan.import_results(scan_results, current_time) do + [] -> + put_toast(socket, :info, gettext("Records imported successfully")) + + errors -> + maybe_toast_errors(socket, errors) + end end qs = %{order: :purchase} @@ -429,6 +438,21 @@ defmodule MusicLibraryWeb.Components.BarcodeScanner do |> push_patch(to: ~p"/collection?#{qs}")} end + defp maybe_toast_errors(socket, []), do: socket + + defp maybe_toast_errors(socket, errors) do + errors_summary = + Enum.map_join(errors, "\n", fn {number, reason} -> + "#{number}: #{ErrorMessages.friendly_message(reason)}" + end) + + put_toast( + socket, + :error, + gettext("Some records could not be imported: %{summary}", summary: errors_summary) + ) + end + defp release_format_label(release) do release |> MusicBrainz.ReleaseSearchResult.format() diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 97651abc..88b5a4f6 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2395,3 +2395,10 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Record has a selected release" msgstr "" + +#: lib/music_library_web/components/barcode_scanner.ex +#, elixir-autogen, elixir-format +msgid "Importing %{count} record in the background..." +msgid_plural "Importing %{count} records in the background..." +msgstr[0] "" +msgstr[1] "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index f0a12235..8756ef95 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2395,3 +2395,10 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Record has a selected release" msgstr "" + +#: lib/music_library_web/components/barcode_scanner.ex +#, elixir-autogen, elixir-format +msgid "Importing %{count} record in the background..." +msgid_plural "Importing %{count} records in the background..." +msgstr[0] "" +msgstr[1] "" diff --git a/test/music_library/barcode_scan_test.exs b/test/music_library/barcode_scan_test.exs index d6946ac2..7cf95fcf 100644 --- a/test/music_library/barcode_scan_test.exs +++ b/test/music_library/barcode_scan_test.exs @@ -254,4 +254,218 @@ defmodule MusicLibrary.BarcodeScanTest do assert {"2222222222", :not_found} in errors end end + + describe "should_import_async?/1" do + test "returns false with zero new results" do + refute BarcodeScan.should_import_async?([]) + end + + test "returns false with one new result" do + results = [Result.new("111", %{})] + refute BarcodeScan.should_import_async?(results) + end + + test "returns true with two new results" do + results = [Result.new("111", %{}), Result.new("222", %{})] + assert BarcodeScan.should_import_async?(results) + end + + test "only counts :new results" do + results = [ + Result.new("111", %{}), + Result.wishlisted("222", "some-id", %{}), + Result.collected("333", "some-id", %{}), + Result.not_found("444") + ] + + refute BarcodeScan.should_import_async?(results) + end + + test "returns true with mixed statuses including two new" do + results = [ + Result.new("111", %{}), + Result.wishlisted("222", "some-id", %{}), + Result.new("333", %{}) + ] + + assert BarcodeScan.should_import_async?(results) + end + end + + describe "import_results_async/2" do + test "enqueues new results as Oban jobs" do + current_time = DateTime.utc_now() + + new_result_1 = %Result{ + status: :new, + number: "111", + release: %MusicBrainz.ReleaseSearchResult{ + id: "release-1", + title: "Album 1", + release_group: %{id: "rg-1", type: :album, title: "Album 1"}, + artists: "Artist 1", + date: "2024", + barcode: "111", + media: [%{format: "CD", disc_count: 1, track_count: 10}] + } + } + + new_result_2 = %Result{ + status: :new, + number: "222", + release: %MusicBrainz.ReleaseSearchResult{ + id: "release-2", + title: "Album 2", + release_group: %{id: "rg-2", type: :album, title: "Album 2"}, + artists: "Artist 2", + date: "2024", + barcode: "222", + media: [%{format: "12\" Vinyl", disc_count: 1, track_count: 8}] + } + } + + assert {:ok, [], 2} = + BarcodeScan.import_results_async([new_result_1, new_result_2], current_time) + + assert_enqueued( + worker: MusicLibrary.Worker.ImportFromMusicbrainzRelease, + args: %{ + "release_id" => "release-1", + "format" => "cd", + "purchased_at" => DateTime.to_iso8601(current_time), + "selected_release_id" => "release-1" + } + ) + + assert_enqueued( + worker: MusicLibrary.Worker.ImportFromMusicbrainzRelease, + args: %{ + "release_id" => "release-2", + "format" => "vinyl", + "purchased_at" => DateTime.to_iso8601(current_time), + "selected_release_id" => "release-2" + } + ) + end + + test "processes wishlisted results synchronously" do + current_time = DateTime.utc_now() + wishlisted_record = record(%{purchased_at: nil}) + + wishlisted_result = %Result{ + status: :wishlisted, + number: "333", + record_id: wishlisted_record.id, + release: %MusicBrainz.ReleaseSearchResult{ + id: "some-release-id", + title: "Test", + release_group: nil, + artists: "Test Artist", + date: "2021", + barcode: "333", + media: [%{format: "CD", disc_count: 1, track_count: 10}] + } + } + + new_result_1 = %Result{ + status: :new, + number: "111", + release: %MusicBrainz.ReleaseSearchResult{ + id: "release-1", + title: "Album 1", + release_group: %{id: "rg-1", type: :album, title: "Album 1"}, + artists: "Artist 1", + date: "2024", + barcode: "111", + media: [%{format: "CD", disc_count: 1, track_count: 10}] + } + } + + new_result_2 = %Result{ + status: :new, + number: "222", + release: %MusicBrainz.ReleaseSearchResult{ + id: "release-2", + title: "Album 2", + release_group: %{id: "rg-2", type: :album, title: "Album 2"}, + artists: "Artist 2", + date: "2024", + barcode: "222", + media: [%{format: "CD", disc_count: 1, track_count: 10}] + } + } + + assert {:ok, [], 2} = + BarcodeScan.import_results_async( + [wishlisted_result, new_result_1, new_result_2], + current_time + ) + + updated_record = Records.get_record!(wishlisted_record.id) + assert updated_record.purchased_at == DateTime.truncate(current_time, :second) + + assert_enqueued( + worker: MusicLibrary.Worker.ImportFromMusicbrainzRelease, + args: %{"release_id" => "release-1"} + ) + + assert_enqueued( + worker: MusicLibrary.Worker.ImportFromMusicbrainzRelease, + args: %{"release_id" => "release-2"} + ) + end + + test "returns sync errors from non-new results" do + current_time = DateTime.utc_now() + + collected_result = %Result{ + status: :collected, + number: "333", + record_id: "some-id", + release: %MusicBrainz.ReleaseSearchResult{ + id: "r1", + title: "T", + release_group: nil, + artists: "A", + date: "2021", + barcode: "333", + media: [%{format: "CD", disc_count: 1, track_count: 1}] + } + } + + new_result_1 = %Result{ + status: :new, + number: "111", + release: %MusicBrainz.ReleaseSearchResult{ + id: "release-1", + title: "Album 1", + release_group: %{id: "rg-1", type: :album, title: "Album 1"}, + artists: "Artist 1", + date: "2024", + barcode: "111", + media: [%{format: "CD", disc_count: 1, track_count: 10}] + } + } + + new_result_2 = %Result{ + status: :new, + number: "222", + release: %MusicBrainz.ReleaseSearchResult{ + id: "release-2", + title: "Album 2", + release_group: %{id: "rg-2", type: :album, title: "Album 2"}, + artists: "Artist 2", + date: "2024", + barcode: "222", + media: [%{format: "CD", disc_count: 1, track_count: 10}] + } + } + + assert {:ok, [{"333", :already_collected}], 2} = + BarcodeScan.import_results_async( + [collected_result, new_result_1, new_result_2], + current_time + ) + end + end end diff --git a/test/music_library/worker/import_from_musicbrainz_release_test.exs b/test/music_library/worker/import_from_musicbrainz_release_test.exs new file mode 100644 index 00000000..467ce370 --- /dev/null +++ b/test/music_library/worker/import_from_musicbrainz_release_test.exs @@ -0,0 +1,67 @@ +defmodule MusicLibrary.Worker.ImportFromMusicbrainzReleaseTest do + use MusicLibrary.DataCase + + import MusicBrainz.Fixtures.Release + import MusicBrainz.Fixtures.ReleaseGroup + import MusicLibrary.Fixtures.Records + + alias MusicLibrary.Records.Record + alias MusicLibrary.Worker.ImportFromMusicbrainzRelease + + describe "perform/1" do + test "imports a record from a MusicBrainz release" do + release_data = release(:marbles) + release_id = release_id(:marbles) + + release_group_data = release_group(:marbles) + release_group_id = release_group_id(:marbles) + release_group_releases_data = 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", ^release_group_id] -> + Req.Test.json(conn, release_group_data) + + [_ws, _version, "release", ^release_id] -> + Req.Test.json(conn, release_data) + + [_ws, _version, "release"] -> + Req.Test.json(conn, release_group_releases_data) + + [_release_group, ^release_group_id, "front"] -> + Plug.Conn.send_resp(conn, 200, cover_data) + end + end) + + purchased_at = DateTime.utc_now() + + assert :ok = + perform_job(ImportFromMusicbrainzRelease, %{ + "release_id" => release_id, + "format" => "cd", + "purchased_at" => DateTime.to_iso8601(purchased_at), + "selected_release_id" => release_id + }) + + imported_record = Repo.get_by!(Record, musicbrainz_id: release_group_id) + assert imported_record.title == "Marbles" + assert imported_record.purchased_at == DateTime.truncate(purchased_at, :second) + end + + test "returns error on transport failure" do + Req.Test.stub(MusicBrainz.API, fn conn -> + Req.Test.transport_error(conn, :timeout) + end) + + assert {:error, %Req.TransportError{reason: :timeout}} = + perform_job(ImportFromMusicbrainzRelease, %{ + "release_id" => "nonexistent-release-id", + "format" => "cd", + "purchased_at" => DateTime.to_iso8601(DateTime.utc_now()), + "selected_release_id" => "nonexistent-release-id" + }) + end + end +end