From d02f725d5118d2950771c1c95bc134a333093199 Mon Sep 17 00:00:00 2001 From: carla Date: Fri, 23 Jan 2026 12:54:48 +0100 Subject: [PATCH] refactor --- lib/mv/membership/import/header_mapper.ex | 62 ++---- lib/mv/membership/import/member_csv.ex | 4 +- lib/mv_web/live/global_settings_live.ex | 186 ++++++++++-------- test/fixtures/csv_with_bom_semicolon.csv | 5 + test/fixtures/csv_with_empty_lines.csv | 5 + .../csv_with_unknown_custom_field.csv | 5 + test/fixtures/invalid_member_import.csv | 5 + test/fixtures/valid_member_import.csv | 5 + 8 files changed, 149 insertions(+), 128 deletions(-) diff --git a/lib/mv/membership/import/header_mapper.ex b/lib/mv/membership/import/header_mapper.ex index e224998..709e156 100644 --- a/lib/mv/membership/import/header_mapper.ex +++ b/lib/mv/membership/import/header_mapper.ex @@ -97,31 +97,16 @@ defmodule Mv.Membership.Import.HeaderMapper do } # Build reverse map: normalized_variant -> canonical_field - # Cached on first access for performance + # Computed on each access - the map is small enough that recomputing is fast + # This avoids Module.get_attribute issues while maintaining simplicity defp normalized_to_canonical do - cached = Process.get({__MODULE__, :normalized_to_canonical}) - - if cached do - cached - else - map = build_normalized_to_canonical_map() - Process.put({__MODULE__, :normalized_to_canonical}, map) - map - end - end - - # Builds the normalized variant -> canonical field map - defp build_normalized_to_canonical_map do @member_field_variants_raw - |> Enum.flat_map(&map_variants_to_normalized/1) - |> Map.new() - end - - # Maps a canonical field and its variants to normalized tuples - defp map_variants_to_normalized({canonical, variants}) do - Enum.map(variants, fn variant -> - {normalize_header(variant), canonical} + |> Enum.flat_map(fn {canonical, variants} -> + Enum.map(variants, fn variant -> + {normalize_header(variant), canonical} + end) end) + |> Map.new() end @doc """ @@ -139,28 +124,21 @@ defmodule Mv.Membership.Import.HeaderMapper do iex> HeaderMapper.known_member_fields() #MapSet<["email", "firstname", "lastname", "street", "postalcode", "city"]> """ + # Known member fields computed at compile-time for performance and determinism + @known_member_fields @member_field_variants_raw + |> Map.keys() + |> Enum.map(fn canonical -> + # Normalize the canonical field name (e.g., :first_name -> "firstname") + canonical + |> Atom.to_string() + |> String.replace("_", "") + |> String.downcase() + end) + |> MapSet.new() + @spec known_member_fields() :: MapSet.t(String.t()) def known_member_fields do - cached = Process.get({__MODULE__, :known_member_fields}) - - if cached do - cached - else - fields = - @member_field_variants_raw - |> Map.keys() - |> Enum.map(fn canonical -> - # Normalize the canonical field name (e.g., :first_name -> "firstname") - canonical - |> Atom.to_string() - |> String.replace("_", "") - |> String.downcase() - end) - |> MapSet.new() - - Process.put({__MODULE__, :known_member_fields}, fields) - fields - end + @known_member_fields end @doc """ diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index 537d5ec..319ff81 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -310,10 +310,10 @@ defmodule Mv.Membership.Import.MemberCSV do case process_row(row_map, line_number, custom_field_lookup, actor) do {:ok, _member} -> - update_inserted(acc) + update_inserted({acc_inserted, acc_failed, acc_errors, acc_error_count, acc_truncated?}) {:error, error} -> - handle_row_error(acc, error, current_error_count, max_errors) + handle_row_error({acc_inserted, acc_failed, acc_errors, acc_error_count, acc_truncated?}, error, current_error_count, max_errors) end end) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index a11d84e..c9cb8ac 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -48,6 +48,7 @@ defmodule MvWeb.GlobalSettingsLive do alias Mv.Membership alias Mv.Membership.Import.MemberCSV alias MvWeb.Authorization + alias Mv.Config on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} @@ -140,14 +141,6 @@ defmodule MvWeb.GlobalSettingsLive do "Use the custom field name as the CSV column header (same normalization as member fields applies)" )}

-
- <.link - navigate={~p"/custom_field_values"} - class="link link-primary" - > - {gettext("Manage Custom Fields")} - -
@@ -209,7 +202,8 @@ defmodule MvWeb.GlobalSettingsLive do phx-disable-with={gettext("Starting import...")} variant="primary" disabled={ - Enum.empty?(@uploads.csv_file.entries) or + @import_status == :running or + Enum.empty?(@uploads.csv_file.entries) or @uploads.csv_file.entries |> List.first() |> then(&(&1 && not &1.done?)) } data-testid="start-import-button" @@ -226,7 +220,7 @@ defmodule MvWeb.GlobalSettingsLive do class="mt-4" data-testid="import-progress-container" > - <%= if @import_status == :running do %> + <%= if @import_progress.status == :running do %>

{gettext("Processing chunk %{current} of %{total}...", current: @import_progress.current_chunk, @@ -235,7 +229,7 @@ defmodule MvWeb.GlobalSettingsLive do

<% end %> - <%= if @import_status == :done do %> + <%= if @import_progress.status == :done do %>

{gettext("Import Results")} @@ -372,51 +366,85 @@ defmodule MvWeb.GlobalSettingsLive do def handle_event("start_import", _params, socket) do # Server-side admin check if Authorization.can?(socket.assigns[:current_user], :create, Mv.Membership.Member) do - # Check if upload is completed - upload_entries = socket.assigns.uploads.csv_file.entries - - if Enum.empty?(upload_entries) do + # Prevent concurrent imports + if socket.assigns.import_status == :running do {:noreply, put_flash( socket, :error, - gettext("Please select a CSV file to import.") + gettext("Import is already running. Please wait for it to complete.") )} else - entry = List.first(upload_entries) + # Check if upload is completed + upload_entries = socket.assigns.uploads.csv_file.entries - if entry.done? do - with {:ok, content} <- consume_and_read_csv(socket) do - case MemberCSV.prepare(content) do - {:ok, import_state} -> - total_chunks = length(import_state.chunks) + if Enum.empty?(upload_entries) do + {:noreply, + put_flash( + socket, + :error, + gettext("Please select a CSV file to import.") + )} + else + entry = List.first(upload_entries) - progress = %{ - inserted: 0, - failed: 0, - errors: [], - warnings: import_state.warnings || [], - status: :running, - current_chunk: 0, - total_chunks: total_chunks - } + if entry.done? do + with {:ok, content} <- consume_and_read_csv(socket) do + case MemberCSV.prepare(content) do + {:ok, import_state} -> + total_chunks = length(import_state.chunks) - socket = - socket - |> assign(:import_state, import_state) - |> assign(:import_progress, progress) - |> assign(:import_status, :running) + progress = %{ + inserted: 0, + failed: 0, + errors: [], + warnings: import_state.warnings || [], + status: :running, + current_chunk: 0, + total_chunks: total_chunks, + errors_truncated?: false + } - send(self(), {:process_chunk, 0}) + socket = + socket + |> assign(:import_state, import_state) + |> assign(:import_progress, progress) + |> assign(:import_status, :running) - {:noreply, socket} + send(self(), {:process_chunk, 0}) + + {:noreply, socket} + + {:error, error} -> + error_message = + case error do + %{message: msg} when is_binary(msg) -> msg + %{errors: errors} when is_list(errors) -> inspect(errors) + reason when is_binary(reason) -> reason + other -> inspect(other) + end + + {:noreply, + put_flash( + socket, + :error, + gettext("Failed to prepare CSV import: %{error}", error: error_message) + )} + end + else + {:error, reason} when is_binary(reason) -> + {:noreply, + put_flash( + socket, + :error, + gettext("Failed to prepare CSV import: %{reason}", reason: reason) + )} {:error, error} -> error_message = case error do %{message: msg} when is_binary(msg) -> msg %{errors: errors} when is_list(errors) -> inspect(errors) - reason when is_binary(reason) -> reason other -> inspect(other) end @@ -428,36 +456,13 @@ defmodule MvWeb.GlobalSettingsLive do )} end else - {:error, reason} when is_binary(reason) -> - {:noreply, - put_flash( - socket, - :error, - gettext("Failed to prepare CSV import: %{reason}", reason: reason) - )} - - {:error, error} -> - error_message = - case error do - %{message: msg} when is_binary(msg) -> msg - %{errors: errors} when is_list(errors) -> inspect(errors) - other -> inspect(other) - end - - {:noreply, - put_flash( - socket, - :error, - gettext("Failed to prepare CSV import: %{error}", error: error_message) - )} + {:noreply, + put_flash( + socket, + :error, + gettext("Please wait for the file upload to complete before starting the import.") + )} end - else - {:noreply, - put_flash( - socket, - :error, - gettext("Please wait for the file upload to complete before starting the import.") - )} end end else @@ -576,6 +581,7 @@ defmodule MvWeb.GlobalSettingsLive do end # Starts async task to process a chunk + # In tests (SQL sandbox mode), runs synchronously to avoid Ecto Sandbox issues defp start_chunk_processing_task(socket, import_state, progress, idx) do chunk = Enum.at(import_state.chunks, idx) actor = socket.assigns[:current_user] @@ -589,21 +595,33 @@ defmodule MvWeb.GlobalSettingsLive do actor: actor ] - # Start async task to process chunk - Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> - case MemberCSV.process_chunk( - chunk, - import_state.column_map, - import_state.custom_field_map, - opts - ) do - {:ok, chunk_result} -> - send(live_view_pid, {:chunk_done, idx, chunk_result}) + if Config.sql_sandbox?() do + # Run synchronously in tests to avoid Ecto Sandbox issues with async tasks + {:ok, chunk_result} = + MemberCSV.process_chunk( + chunk, + import_state.column_map, + import_state.custom_field_map, + opts + ) - {:error, reason} -> - send(live_view_pid, {:chunk_error, idx, reason}) - end - end) + # In test mode, send the message - it will be processed when render() is called + # in the test. The test helper wait_for_import_completion() handles message processing + send(live_view_pid, {:chunk_done, idx, chunk_result}) + else + # Start async task to process chunk in production + Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> + {:ok, chunk_result} = + MemberCSV.process_chunk( + chunk, + import_state.column_map, + import_state.custom_field_map, + opts + ) + + send(live_view_pid, {:chunk_done, idx, chunk_result}) + end) + end {:noreply, socket} end @@ -670,10 +688,10 @@ defmodule MvWeb.GlobalSettingsLive do end end) |> case do - [{_name, {:ok, content}}] when is_binary(content) -> + [{:ok, content}] when is_binary(content) -> {:ok, content} - [{_name, {:error, reason}}] -> + [{:error, reason}] -> {:error, gettext("Failed to read file: %{reason}", reason: reason)} [] -> diff --git a/test/fixtures/csv_with_bom_semicolon.csv b/test/fixtures/csv_with_bom_semicolon.csv index 00b9903..9aa53c2 100644 --- a/test/fixtures/csv_with_bom_semicolon.csv +++ b/test/fixtures/csv_with_bom_semicolon.csv @@ -1,3 +1,8 @@ first_name;last_name;email;street;postal_code;city Alice;Smith;alice.smith@example.com;Main Street 1;12345;Berlin + + + + + diff --git a/test/fixtures/csv_with_empty_lines.csv b/test/fixtures/csv_with_empty_lines.csv index 6c70432..15ea79d 100644 --- a/test/fixtures/csv_with_empty_lines.csv +++ b/test/fixtures/csv_with_empty_lines.csv @@ -4,3 +4,8 @@ Alice;Smith;alice.smith@example.com;Main Street 1;12345;Berlin Bob;Johnson;invalid-email;Park Avenue 2;54321;Munich + + + + + diff --git a/test/fixtures/csv_with_unknown_custom_field.csv b/test/fixtures/csv_with_unknown_custom_field.csv index cf19353..204c438 100644 --- a/test/fixtures/csv_with_unknown_custom_field.csv +++ b/test/fixtures/csv_with_unknown_custom_field.csv @@ -3,3 +3,8 @@ Alice;Smith;alice.smith@example.com;Main Street 1;12345;Berlin;SomeValue Bob;Johnson;bob.johnson@example.com;Park Avenue 2;54321;Munich;AnotherValue + + + + + diff --git a/test/fixtures/invalid_member_import.csv b/test/fixtures/invalid_member_import.csv index 0136e1a..642e3d2 100644 --- a/test/fixtures/invalid_member_import.csv +++ b/test/fixtures/invalid_member_import.csv @@ -3,3 +3,8 @@ Alice;Smith;invalid-email;Main Street 1;12345;Berlin Bob;Johnson;;Park Avenue 2;54321;Munich + + + + + diff --git a/test/fixtures/valid_member_import.csv b/test/fixtures/valid_member_import.csv index 07801ca..5cbcfd5 100644 --- a/test/fixtures/valid_member_import.csv +++ b/test/fixtures/valid_member_import.csv @@ -3,3 +3,8 @@ Alice;Smith;alice.smith@example.com;Main Street 1;12345;Berlin Bob;Johnson;bob.johnson@example.com;Park Avenue 2;54321;Munich + + + + +