From a312778b3d7f37a4c9bd3170012ea91ba1292d4a Mon Sep 17 00:00:00 2001 From: Renovate Bot Date: Sun, 1 Feb 2026 00:07:43 +0000 Subject: [PATCH 01/38] chore(deps): update ghcr.io/sebadob/rauthy docker tag to v0.34.2 --- docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index 4c169b5..626f353 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -25,7 +25,7 @@ services: rauthy: container_name: rauthy-dev - image: ghcr.io/sebadob/rauthy:0.33.4 + image: ghcr.io/sebadob/rauthy:0.34.2 environment: - LOCAL_TEST=true - SMTP_URL=mailcrab From 9fd617e45a302d25e811ab672889f00db166901d Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 09:48:37 +0100 Subject: [PATCH 02/38] tests: add tests for config --- .../mv_web/live/global_settings_live_test.exs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index f217311..1ea1d12 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -687,5 +687,42 @@ defmodule MvWeb.GlobalSettingsLiveTest do # Check that file input has accept attribute for CSV assert html =~ ~r/accept=["'][^"']*csv["']/i or html =~ "CSV files only" end + + test "configured row limit is enforced", %{conn: conn} do + # Business rule: CSV import respects configured row limits + # Test that a custom limit (500) is enforced, not just the default (1000) + original_config = Application.get_env(:mv, :csv_import, []) + + try do + Application.put_env(:mv, :csv_import, [max_rows: 500]) + + {:ok, view, _html} = live(conn, ~p"/settings") + + # Generate CSV with 501 rows (exceeding custom limit of 500) + header = "first_name;last_name;email;street;postal_code;city\n" + + rows = + for i <- 1..501 do + "Row#{i};Last#{i};email#{i}@example.com;Street#{i};12345;City#{i}\n" + end + + large_csv = header <> Enum.join(rows) + + # Simulate file upload using helper function + upload_csv_file(view, large_csv, "too_many_rows_custom.csv") + + view + |> form("#csv-upload-form", %{}) + |> render_submit() + + html = render(view) + # Business rule: import should be rejected when exceeding configured limit + assert html =~ "exceeds" or html =~ "maximum" or html =~ "limit" or + html =~ "Failed to prepare" + after + # Restore original config + Application.put_env(:mv, :csv_import, original_config) + end + end end end From 3f551c5f8d47695f23b094f3100c73b6815478c0 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 09:49:13 +0100 Subject: [PATCH 03/38] feat: add configs for impor tlimits --- config/config.exs | 6 ++++ lib/mv/config.ex | 46 +++++++++++++++++++++++++ lib/mv_web/live/global_settings_live.ex | 7 ++-- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/config/config.exs b/config/config.exs index cc338b2..6dfb1d1 100644 --- a/config/config.exs +++ b/config/config.exs @@ -51,6 +51,12 @@ config :mv, generators: [timestamp_type: :utc_datetime], ash_domains: [Mv.Membership, Mv.Accounts, Mv.MembershipFees, Mv.Authorization] +# CSV Import configuration +config :mv, csv_import: [ + max_file_size_mb: 10, + max_rows: 1000 +] + # Configures the endpoint config :mv, MvWeb.Endpoint, url: [host: "localhost"], diff --git a/lib/mv/config.ex b/lib/mv/config.ex index 5e6ba90..edf8428 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -21,4 +21,50 @@ defmodule Mv.Config do def sql_sandbox? do Application.get_env(:mv, :sql_sandbox, false) end + + @doc """ + Returns the maximum file size for CSV imports in bytes. + + Reads the `max_file_size_mb` value from the CSV import configuration + and converts it to bytes. + + ## Returns + + - Maximum file size in bytes (default: 10_485_760 bytes = 10 MB) + + ## Examples + + iex> Mv.Config.csv_import_max_file_size_bytes() + 10_485_760 + """ + @spec csv_import_max_file_size_bytes() :: non_neg_integer() + def csv_import_max_file_size_bytes do + max_file_size_mb = get_csv_import_config(:max_file_size_mb, 10) + max_file_size_mb * 1024 * 1024 + end + + @doc """ + Returns the maximum number of rows allowed in CSV imports. + + Reads the `max_rows` value from the CSV import configuration. + + ## Returns + + - Maximum number of rows (default: 1000) + + ## Examples + + iex> Mv.Config.csv_import_max_rows() + 1000 + """ + @spec csv_import_max_rows() :: pos_integer() + def csv_import_max_rows do + get_csv_import_config(:max_rows, 1000) + end + + # Helper function to get CSV import config values + defp get_csv_import_config(key, default) do + Application.get_env(:mv, :csv_import, []) + |> Keyword.get(key, default) + end end diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index bd0036b..aa41cd5 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -54,8 +54,6 @@ defmodule MvWeb.GlobalSettingsLive do on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} # CSV Import configuration constants - # 10 MB - @max_file_size_bytes 10_485_760 @max_errors 50 @impl true @@ -82,7 +80,7 @@ defmodule MvWeb.GlobalSettingsLive do |> allow_upload(:csv_file, accept: ~w(.csv), max_entries: 1, - max_file_size: @max_file_size_bytes, + max_file_size: Config.csv_import_max_file_size_bytes(), auto_upload: true ) @@ -409,7 +407,8 @@ defmodule MvWeb.GlobalSettingsLive do # Processes CSV upload and starts import defp process_csv_upload(socket) do with {:ok, content} <- consume_and_read_csv(socket), - {:ok, import_state} <- MemberCSV.prepare(content) do + {:ok, import_state} <- + MemberCSV.prepare(content, max_rows: Config.csv_import_max_rows()) do start_import(socket, import_state) else {:error, reason} when is_binary(reason) -> From d61a939debf907bfabdc8aedfb3b6b5435907689 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 09:50:47 +0100 Subject: [PATCH 04/38] formatting --- config/config.exs | 9 +++++---- test/mv_web/live/global_settings_live_test.exs | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/config/config.exs b/config/config.exs index 6dfb1d1..64f3604 100644 --- a/config/config.exs +++ b/config/config.exs @@ -52,10 +52,11 @@ config :mv, ash_domains: [Mv.Membership, Mv.Accounts, Mv.MembershipFees, Mv.Authorization] # CSV Import configuration -config :mv, csv_import: [ - max_file_size_mb: 10, - max_rows: 1000 -] +config :mv, + csv_import: [ + max_file_size_mb: 10, + max_rows: 1000 + ] # Configures the endpoint config :mv, MvWeb.Endpoint, diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index 1ea1d12..0926681 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -694,7 +694,7 @@ defmodule MvWeb.GlobalSettingsLiveTest do original_config = Application.get_env(:mv, :csv_import, []) try do - Application.put_env(:mv, :csv_import, [max_rows: 500]) + Application.put_env(:mv, :csv_import, max_rows: 500) {:ok, view, _html} = live(conn, ~p"/settings") From e74154581c31034be095f6d0671a76e62f2b9573 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 10:10:02 +0100 Subject: [PATCH 05/38] feat: changes UI info based on config for limits --- lib/mv/config.ex | 19 +++++++++++++++++++ lib/mv_web/live/global_settings_live.ex | 8 +++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/mv/config.ex b/lib/mv/config.ex index edf8428..98a5b65 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -62,6 +62,25 @@ defmodule Mv.Config do get_csv_import_config(:max_rows, 1000) end + @doc """ + Returns the maximum file size for CSV imports in megabytes. + + Reads the `max_file_size_mb` value from the CSV import configuration. + + ## Returns + + - Maximum file size in megabytes (default: 10) + + ## Examples + + iex> Mv.Config.csv_import_max_file_size_mb() + 10 + """ + @spec csv_import_max_file_size_mb() :: pos_integer() + def csv_import_max_file_size_mb do + get_csv_import_config(:max_file_size_mb, 10) + end + # Helper function to get CSV import config values defp get_csv_import_config(key, default) do Application.get_env(:mv, :csv_import, []) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index aa41cd5..29cd3f3 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -34,8 +34,8 @@ defmodule MvWeb.GlobalSettingsLive do ### Limits - - Maximum file size: 10 MB - - Maximum rows: 1,000 rows (excluding header) + - Maximum file size: configurable via `config :mv, csv_import: [max_file_size_mb: ...]` + - Maximum rows: configurable via `config :mv, csv_import: [max_rows: ...]` (excluding header) - Processing: chunks of 200 rows - Errors: capped at 50 per import @@ -74,6 +74,8 @@ defmodule MvWeb.GlobalSettingsLive do |> assign(:import_status, :idle) |> assign(:locale, locale) |> assign(:max_errors, @max_errors) + |> assign(:csv_import_max_rows, Config.csv_import_max_rows()) + |> assign(:csv_import_max_file_size_mb, Config.csv_import_max_file_size_mb()) |> assign_form() # Configure file upload with auto-upload enabled # Files are uploaded automatically when selected, no need for manual trigger @@ -198,7 +200,7 @@ defmodule MvWeb.GlobalSettingsLive do /> From b6d53d2826752a532445f317bb38f13380a17a36 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 10:22:05 +0100 Subject: [PATCH 06/38] refactor: add test to seperate async false module --- .../live/global_settings_live_config_test.exs | 73 +++++++++++++++++++ .../mv_web/live/global_settings_live_test.exs | 37 ---------- 2 files changed, 73 insertions(+), 37 deletions(-) create mode 100644 test/mv_web/live/global_settings_live_config_test.exs diff --git a/test/mv_web/live/global_settings_live_config_test.exs b/test/mv_web/live/global_settings_live_config_test.exs new file mode 100644 index 0000000..c940594 --- /dev/null +++ b/test/mv_web/live/global_settings_live_config_test.exs @@ -0,0 +1,73 @@ +defmodule MvWeb.GlobalSettingsLiveConfigTest do + @moduledoc """ + Tests for GlobalSettingsLive that modify global Application configuration. + + These tests run with `async: false` to prevent race conditions when + modifying global Application environment variables (Application.put_env). + This follows the same pattern as Mv.ConfigTest. + """ + use MvWeb.ConnCase, async: false + import Phoenix.LiveViewTest + + # Helper function to upload CSV file in tests + defp upload_csv_file(view, csv_content, filename \\ "test_import.csv") do + view + |> file_input("#csv-upload-form", :csv_file, [ + %{ + last_modified: System.system_time(:second), + name: filename, + content: csv_content, + size: byte_size(csv_content), + type: "text/csv" + } + ]) + |> render_upload(filename) + end + + describe "CSV Import - Configuration Tests" do + setup %{conn: conn} do + # Ensure admin user + admin_user = Mv.Fixtures.user_with_role_fixture("admin") + conn = MvWeb.ConnCase.conn_with_password_user(conn, admin_user) + + {:ok, conn: conn, admin_user: admin_user} + end + + test "configured row limit is enforced", %{conn: conn} do + # Business rule: CSV import respects configured row limits + # Test that a custom limit (500) is enforced, not just the default (1000) + original_config = Application.get_env(:mv, :csv_import, []) + + try do + Application.put_env(:mv, :csv_import, max_rows: 500) + + {:ok, view, _html} = live(conn, ~p"/settings") + + # Generate CSV with 501 rows (exceeding custom limit of 500) + header = "first_name;last_name;email;street;postal_code;city\n" + + rows = + for i <- 1..501 do + "Row#{i};Last#{i};email#{i}@example.com;Street#{i};12345;City#{i}\n" + end + + large_csv = header <> Enum.join(rows) + + # Simulate file upload using helper function + upload_csv_file(view, large_csv, "too_many_rows_custom.csv") + + view + |> form("#csv-upload-form", %{}) + |> render_submit() + + html = render(view) + # Business rule: import should be rejected when exceeding configured limit + assert html =~ "exceeds" or html =~ "maximum" or html =~ "limit" or + html =~ "Failed to prepare" + after + # Restore original config + Application.put_env(:mv, :csv_import, original_config) + end + end + end +end diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index 0926681..f217311 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -687,42 +687,5 @@ defmodule MvWeb.GlobalSettingsLiveTest do # Check that file input has accept attribute for CSV assert html =~ ~r/accept=["'][^"']*csv["']/i or html =~ "CSV files only" end - - test "configured row limit is enforced", %{conn: conn} do - # Business rule: CSV import respects configured row limits - # Test that a custom limit (500) is enforced, not just the default (1000) - original_config = Application.get_env(:mv, :csv_import, []) - - try do - Application.put_env(:mv, :csv_import, max_rows: 500) - - {:ok, view, _html} = live(conn, ~p"/settings") - - # Generate CSV with 501 rows (exceeding custom limit of 500) - header = "first_name;last_name;email;street;postal_code;city\n" - - rows = - for i <- 1..501 do - "Row#{i};Last#{i};email#{i}@example.com;Street#{i};12345;City#{i}\n" - end - - large_csv = header <> Enum.join(rows) - - # Simulate file upload using helper function - upload_csv_file(view, large_csv, "too_many_rows_custom.csv") - - view - |> form("#csv-upload-form", %{}) - |> render_submit() - - html = render(view) - # Business rule: import should be rejected when exceeding configured limit - assert html =~ "exceeds" or html =~ "maximum" or html =~ "limit" or - html =~ "Failed to prepare" - after - # Restore original config - Application.put_env(:mv, :csv_import, original_config) - end - end end end From 4997819c7380270b2f9b7953728dce9a32c1398d Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 10:22:21 +0100 Subject: [PATCH 07/38] feat: validate config --- lib/mv/config.ex | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/lib/mv/config.ex b/lib/mv/config.ex index 98a5b65..007309a 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -85,5 +85,35 @@ defmodule Mv.Config do defp get_csv_import_config(key, default) do Application.get_env(:mv, :csv_import, []) |> Keyword.get(key, default) + |> parse_and_validate_integer(default) + end + + # Parses and validates integer configuration values. + # + # Accepts: + # - Integer values (passed through) + # - String integers (e.g., "1000") - parsed to integer + # - Invalid values (e.g., "abc", nil) - falls back to default + # + # Always clamps the result to a minimum of 1 to ensure positive values. + # + # Note: We don't log warnings for unparseable values because: + # - These functions may be called frequently (e.g., on every request) + # - Logging would create excessive log spam + # - The fallback to default provides a safe behavior + # - Configuration errors should be caught during deployment/testing + defp parse_and_validate_integer(value, _default) when is_integer(value) do + max(1, value) + end + + defp parse_and_validate_integer(value, default) when is_binary(value) do + case Integer.parse(value) do + {int, _remainder} -> max(1, int) + :error -> default + end + end + + defp parse_and_validate_integer(_value, default) do + default end end From ce6240133d329a0d064c393ad78834ea8574ac3d Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 10:23:49 +0100 Subject: [PATCH 08/38] i18n: update translations --- priv/gettext/de/LC_MESSAGES/default.po | 10 +++++----- priv/gettext/default.pot | 10 +++++----- priv/gettext/en/LC_MESSAGES/default.po | 10 +++++----- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index d650aa2..fa126c1 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1980,11 +1980,6 @@ msgstr " (Datenfeld: %{field})" msgid "CSV File" msgstr "CSV Datei" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "CSV files only, maximum 10 MB" -msgstr "Nur CSV Dateien, maximal 10 MB" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Custom fields must be created in Mila before importing CSV files with custom field columns" @@ -2272,3 +2267,8 @@ msgstr "Nicht berechtigt." #, elixir-autogen, elixir-format msgid "Could not load data fields. Please check your permissions." msgstr "Datenfelder konnten nicht geladen werden. Bitte überprüfen Sie Ihre Berechtigungen." + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "CSV files only, maximum %{size} MB" +msgstr "Nur CSV Dateien, maximal %{size} MB" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 98f9d7b..b0e74ab 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1981,11 +1981,6 @@ msgstr "" msgid "CSV File" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "CSV files only, maximum 10 MB" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Custom fields must be created in Mila before importing CSV files with custom field columns" @@ -2273,3 +2268,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Could not load data fields. Please check your permissions." msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "CSV files only, maximum %{size} MB" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 95a3c3a..6d3013d 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1981,11 +1981,6 @@ msgstr "" msgid "CSV File" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "CSV files only, maximum 10 MB" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Custom fields must be created in Mila before importing CSV files with custom field columns" @@ -2273,3 +2268,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Could not load data fields. Please check your permissions." msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "CSV files only, maximum %{size} MB" +msgstr "" From 3f8797c35629357388dd706407f94a76593e210c Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 11:42:07 +0100 Subject: [PATCH 09/38] feat: import custom fields via CSV --- lib/mv/membership/import/member_csv.ex | 244 +++++++++++++----- .../live/custom_field_live/index_component.ex | 115 +++++---- lib/mv_web/live/global_settings_live.ex | 24 +- 3 files changed, 251 insertions(+), 132 deletions(-) diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index e351d68..5924001 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -63,7 +63,9 @@ defmodule Mv.Membership.Import.MemberCSV do chunks: list(list({pos_integer(), map()})), column_map: %{atom() => non_neg_integer()}, custom_field_map: %{String.t() => non_neg_integer()}, - custom_field_lookup: %{String.t() => %{id: String.t(), value_type: atom()}}, + custom_field_lookup: %{ + String.t() => %{id: String.t(), value_type: atom(), name: String.t()} + }, warnings: list(String.t()) } @@ -79,6 +81,11 @@ defmodule Mv.Membership.Import.MemberCSV do use Gettext, backend: MvWeb.Gettext + alias Mv.Helpers.SystemActor + + # Import FieldTypes for human-readable type labels + alias MvWeb.Translations.FieldTypes + # Configuration constants @default_max_errors 50 @default_chunk_size 200 @@ -102,6 +109,7 @@ defmodule Mv.Membership.Import.MemberCSV do - `opts` - Optional keyword list: - `:max_rows` - Maximum number of data rows allowed (default: 1000) - `:chunk_size` - Number of rows per chunk (default: 200) + - `:actor` - Actor for authorization (default: system actor for systemic operations) ## Returns @@ -120,9 +128,10 @@ defmodule Mv.Membership.Import.MemberCSV do def prepare(file_content, opts \\ []) do max_rows = Keyword.get(opts, :max_rows, @default_max_rows) chunk_size = Keyword.get(opts, :chunk_size, @default_chunk_size) + actor = Keyword.get(opts, :actor, SystemActor.get_system_actor()) with {:ok, headers, rows} <- CsvParser.parse(file_content), - {:ok, custom_fields} <- load_custom_fields(), + {:ok, custom_fields} <- load_custom_fields(actor), {:ok, maps, warnings} <- build_header_maps(headers, custom_fields), :ok <- validate_row_count(rows, max_rows) do chunks = chunk_rows(rows, maps, chunk_size) @@ -142,10 +151,10 @@ defmodule Mv.Membership.Import.MemberCSV do end # Loads all custom fields from the database - defp load_custom_fields do + defp load_custom_fields(actor) do custom_fields = Mv.Membership.CustomField - |> Ash.read!() + |> Ash.read!(actor: actor) {:ok, custom_fields} rescue @@ -158,7 +167,7 @@ defmodule Mv.Membership.Import.MemberCSV do custom_fields |> Enum.reduce(%{}, fn cf, acc -> id_str = to_string(cf.id) - Map.put(acc, id_str, %{id: cf.id, value_type: cf.value_type}) + Map.put(acc, id_str, %{id: cf.id, value_type: cf.value_type, name: cf.name}) end) end @@ -508,32 +517,39 @@ defmodule Mv.Membership.Import.MemberCSV do {:ok, %{member: trimmed_member_attrs, custom: custom_attrs}} -> # Prepare custom field values for Ash - custom_field_values = prepare_custom_field_values(custom_attrs, custom_field_lookup) + case prepare_custom_field_values(custom_attrs, custom_field_lookup) do + {:error, validation_errors} -> + # Custom field validation errors - return first error + first_error = List.first(validation_errors) + {:error, %Error{csv_line_number: line_number, field: nil, message: first_error}} - # Create member with custom field values - member_attrs_with_cf = - trimmed_member_attrs - |> Map.put(:custom_field_values, custom_field_values) + {:ok, custom_field_values} -> + # Create member with custom field values + member_attrs_with_cf = + trimmed_member_attrs + |> Map.put(:custom_field_values, custom_field_values) - # Only include custom_field_values if not empty - final_attrs = - if Enum.empty?(custom_field_values) do - Map.delete(member_attrs_with_cf, :custom_field_values) - else - member_attrs_with_cf - end + # Only include custom_field_values if not empty + final_attrs = + if Enum.empty?(custom_field_values) do + Map.delete(member_attrs_with_cf, :custom_field_values) + else + member_attrs_with_cf + end - case Mv.Membership.create_member(final_attrs, actor: actor) do - {:ok, member} -> - {:ok, member} + case Mv.Membership.create_member(final_attrs, actor: actor) do + {:ok, member} -> + {:ok, member} - {:error, %Ash.Error.Invalid{} = error} -> - # Extract email from final_attrs for better error messages - email = Map.get(final_attrs, :email) || Map.get(trimmed_member_attrs, :email) - {:error, format_ash_error(error, line_number, email)} + {:error, %Ash.Error.Invalid{} = error} -> + # Extract email from final_attrs for better error messages + email = Map.get(final_attrs, :email) || Map.get(trimmed_member_attrs, :email) + {:error, format_ash_error(error, line_number, email)} - {:error, error} -> - {:error, %Error{csv_line_number: line_number, field: nil, message: inspect(error)}} + {:error, error} -> + {:error, + %Error{csv_line_number: line_number, field: nil, message: inspect(error)}} + end end end rescue @@ -542,70 +558,160 @@ defmodule Mv.Membership.Import.MemberCSV do end # Prepares custom field values from row map for Ash + # Returns {:ok, [custom_field_value_maps]} or {:error, [validation_errors]} defp prepare_custom_field_values(custom_attrs, custom_field_lookup) when is_map(custom_attrs) do - custom_attrs - |> Enum.filter(fn {_id, value} -> value != nil && value != "" end) - |> Enum.map(fn {custom_field_id_str, value} -> - case Map.get(custom_field_lookup, custom_field_id_str) do - nil -> - # Custom field not found, skip - nil + {values, errors} = + custom_attrs + |> Enum.filter(fn {_id, value} -> value != nil && value != "" end) + |> Enum.reduce({[], []}, fn {custom_field_id_str, value}, {acc_values, acc_errors} -> + case Map.get(custom_field_lookup, custom_field_id_str) do + nil -> + # Custom field not found, skip + {acc_values, acc_errors} - %{id: custom_field_id, value_type: value_type} -> - %{ - "custom_field_id" => to_string(custom_field_id), - "value" => format_custom_field_value(value, value_type) - } - end - end) - |> Enum.filter(&(&1 != nil)) - end + %{id: custom_field_id, value_type: value_type, name: custom_field_name} -> + case format_custom_field_value(value, value_type, custom_field_name) do + {:ok, formatted_value} -> + value_map = %{ + "custom_field_id" => to_string(custom_field_id), + "value" => formatted_value + } - defp prepare_custom_field_values(_, _), do: [] + {[value_map | acc_values], acc_errors} - # Formats a custom field value according to its type - # Uses _union_type and _union_value format as expected by Ash - defp format_custom_field_value(value, :string) when is_binary(value) do - %{"_union_type" => "string", "_union_value" => String.trim(value)} - end + {:error, reason} -> + {acc_values, [reason | acc_errors]} + end + end + end) - defp format_custom_field_value(value, :integer) when is_binary(value) do - case Integer.parse(value) do - {int_value, _} -> %{"_union_type" => "integer", "_union_value" => int_value} - :error -> %{"_union_type" => "string", "_union_value" => String.trim(value)} + if Enum.empty?(errors) do + {:ok, Enum.reverse(values)} + else + {:error, Enum.reverse(errors)} end end - defp format_custom_field_value(value, :boolean) when is_binary(value) do + defp prepare_custom_field_values(_, _), do: {:ok, []} + + # Formats a custom field value according to its type + # Uses _union_type and _union_value format as expected by Ash + # Returns {:ok, formatted_value} or {:error, error_message} + defp format_custom_field_value(value, :string, _custom_field_name) when is_binary(value) do + {:ok, %{"_union_type" => "string", "_union_value" => String.trim(value)}} + end + + defp format_custom_field_value(value, :integer, custom_field_name) when is_binary(value) do + trimmed = String.trim(value) + + case Integer.parse(trimmed) do + {int_value, ""} -> + # Fully consumed - valid integer + {:ok, %{"_union_type" => "integer", "_union_value" => int_value}} + + {_int_value, _remaining} -> + # Not fully consumed - invalid + {:error, format_custom_field_error(custom_field_name, :integer, trimmed)} + + :error -> + {:error, format_custom_field_error(custom_field_name, :integer, trimmed)} + end + end + + defp format_custom_field_value(value, :boolean, custom_field_name) when is_binary(value) do + trimmed = String.trim(value) + lower = String.downcase(trimmed) + bool_value = - value - |> String.trim() - |> String.downcase() - |> case do + case lower do "true" -> true "1" -> true "yes" -> true "ja" -> true - _ -> false + "false" -> false + "0" -> false + "no" -> false + "nein" -> false + _ -> nil end - %{"_union_type" => "boolean", "_union_value" => bool_value} - end - - defp format_custom_field_value(value, :date) when is_binary(value) do - case Date.from_iso8601(String.trim(value)) do - {:ok, date} -> %{"_union_type" => "date", "_union_value" => date} - {:error, _} -> %{"_union_type" => "string", "_union_value" => String.trim(value)} + if bool_value != nil do + {:ok, %{"_union_type" => "boolean", "_union_value" => bool_value}} + else + {:error, + format_custom_field_error_with_details( + custom_field_name, + :boolean, + trimmed, + gettext("(true/false/1/0/yes/no/ja/nein)") + )} end end - defp format_custom_field_value(value, :email) when is_binary(value) do - %{"_union_type" => "email", "_union_value" => String.trim(value)} + defp format_custom_field_value(value, :date, custom_field_name) when is_binary(value) do + trimmed = String.trim(value) + + case Date.from_iso8601(trimmed) do + {:ok, date} -> + {:ok, %{"_union_type" => "date", "_union_value" => date}} + + {:error, _} -> + {:error, + format_custom_field_error_with_details( + custom_field_name, + :date, + trimmed, + gettext("(ISO-8601 format: YYYY-MM-DD)") + )} + end end - defp format_custom_field_value(value, _type) when is_binary(value) do + defp format_custom_field_value(value, :email, custom_field_name) when is_binary(value) do + trimmed = String.trim(value) + + # Use simple validation: must contain @ and have valid format + # For CSV import, we use a simpler check than EctoCommons.EmailValidator + # to avoid dependencies and keep it fast + if String.contains?(trimmed, "@") and String.length(trimmed) >= 5 and + String.length(trimmed) <= 254 do + # Basic format check: username@domain.tld + if Regex.match?(~r/^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$/, trimmed) do + {:ok, %{"_union_type" => "email", "_union_value" => trimmed}} + else + {:error, format_custom_field_error(custom_field_name, :email, trimmed)} + end + else + {:error, format_custom_field_error(custom_field_name, :email, trimmed)} + end + end + + defp format_custom_field_value(value, _type, _custom_field_name) when is_binary(value) do # Default to string if type is unknown - %{"_union_type" => "string", "_union_value" => String.trim(value)} + {:ok, %{"_union_type" => "string", "_union_value" => String.trim(value)}} + end + + # Generates a consistent error message for custom field validation failures + # Uses human-readable field type labels (e.g., "Number" instead of "integer") + defp format_custom_field_error(custom_field_name, value_type, value) do + type_label = FieldTypes.label(value_type) + + gettext("custom_field: %{name} – expected %{type}, got: %{value}", + name: custom_field_name, + type: type_label, + value: value + ) + end + + # Generates an error message with additional details (e.g., format hints) + defp format_custom_field_error_with_details(custom_field_name, value_type, value, details) do + type_label = FieldTypes.label(value_type) + + gettext("custom_field: %{name} – expected %{type} %{details}, got: %{value}", + name: custom_field_name, + type: type_label, + details: details, + value: value + ) end # Trims all string values in member attributes diff --git a/lib/mv_web/live/custom_field_live/index_component.ex b/lib/mv_web/live/custom_field_live/index_component.ex index 784d1ef..5cf0f6b 100644 --- a/lib/mv_web/live/custom_field_live/index_component.ex +++ b/lib/mv_web/live/custom_field_live/index_component.ex @@ -50,66 +50,69 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do <%!-- Hide table when form is visible --%> - <.table - :if={!@show_form} - id="custom_fields" - rows={@streams.custom_fields} - row_click={ - fn {_id, custom_field} -> - JS.push("edit_custom_field", value: %{id: custom_field.id}, target: @myself) - end - } - > - <:col :let={{_id, custom_field}} label={gettext("Name")}>{custom_field.name} - - <:col :let={{_id, custom_field}} label={gettext("Value Type")}> - {@field_type_label.(custom_field.value_type)} - - - <:col :let={{_id, custom_field}} label={gettext("Description")}> - {custom_field.description} - - - <:col - :let={{_id, custom_field}} - label={gettext("Required")} - class="max-w-[9.375rem] text-center" +
+ <.table + id="custom_fields_table" + rows={@streams.custom_fields} + row_click={ + fn {_id, custom_field} -> + JS.push("edit_custom_field", value: %{id: custom_field.id}, target: @myself) + end + } > - - {gettext("Required")} - - - {gettext("Optional")} - - + <:col :let={{_id, custom_field}} label={gettext("Name")}>{custom_field.name} - <:col - :let={{_id, custom_field}} - label={gettext("Show in overview")} - class="max-w-[9.375rem] text-center" - > - - {gettext("Yes")} - - - {gettext("No")} - - + <:col :let={{_id, custom_field}} label={gettext("Value Type")}> + {@field_type_label.(custom_field.value_type)} + - <:action :let={{_id, custom_field}}> - <.link phx-click={ - JS.push("edit_custom_field", value: %{id: custom_field.id}, target: @myself) - }> - {gettext("Edit")} - - + <:col :let={{_id, custom_field}} label={gettext("Description")}> + {custom_field.description} + - <:action :let={{_id, custom_field}}> - <.link phx-click={JS.push("prepare_delete", value: %{id: custom_field.id}, target: @myself)}> - {gettext("Delete")} - - - + <:col + :let={{_id, custom_field}} + label={gettext("Required")} + class="max-w-[9.375rem] text-center" + > + + {gettext("Required")} + + + {gettext("Optional")} + + + + <:col + :let={{_id, custom_field}} + label={gettext("Show in overview")} + class="max-w-[9.375rem] text-center" + > + + {gettext("Yes")} + + + {gettext("No")} + + + + <:action :let={{_id, custom_field}}> + <.link phx-click={ + JS.push("edit_custom_field", value: %{id: custom_field.id}, target: @myself) + }> + {gettext("Edit")} + + + + <:action :let={{_id, custom_field}}> + <.link phx-click={ + JS.push("prepare_delete", value: %{id: custom_field.id}, target: @myself) + }> + {gettext("Delete")} + + + +
<%!-- Delete Confirmation Modal --%> diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index bd0036b..9d3bec9 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -138,16 +138,24 @@ defmodule MvWeb.GlobalSettingsLive do <%= if Authorization.can?(@current_user, :create, Mv.Membership.Member) do %> <.form_section title={gettext("Import Members (CSV)")}>
+ <.icon name="hero-information-circle" class="size-5" aria-hidden="true" />
-

+

+ {gettext("Custom Fields in CSV Import")} +

+

{gettext( - "Custom fields must be created in Mila before importing CSV files with custom field columns" + "Custom fields must be created in Mila before importing. Use the custom field name as the CSV column header. Unknown custom field columns will be ignored with a warning." )}

-

- {gettext( - "Use the custom field name as the CSV column header (same normalization as member fields applies)" - )} +

+ <.link + href="#custom_fields" + class="link link-primary" + data-testid="custom-fields-link" + > + {gettext("Manage Custom Fields")} +

@@ -408,8 +416,10 @@ defmodule MvWeb.GlobalSettingsLive do # Processes CSV upload and starts import defp process_csv_upload(socket) do + actor = MvWeb.LiveHelpers.current_actor(socket) + with {:ok, content} <- consume_and_read_csv(socket), - {:ok, import_state} <- MemberCSV.prepare(content) do + {:ok, import_state} <- MemberCSV.prepare(content, actor: actor) do start_import(socket, import_state) else {:error, reason} when is_binary(reason) -> From 86a3c4e50e390a1dd12827adc6a554b3b5f7443e Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 13:07:00 +0100 Subject: [PATCH 10/38] tests: add tests for import --- test/mv/config_test.exs | 14 + test/mv/membership/import/member_csv_test.exs | 325 ++++++++++++++++-- 2 files changed, 315 insertions(+), 24 deletions(-) create mode 100644 test/mv/config_test.exs diff --git a/test/mv/config_test.exs b/test/mv/config_test.exs new file mode 100644 index 0000000..076915f --- /dev/null +++ b/test/mv/config_test.exs @@ -0,0 +1,14 @@ +defmodule Mv.ConfigTest do + @moduledoc """ + Tests for Mv.Config module. + """ + use ExUnit.Case, async: false + + alias Mv.Config + + # Note: CSV import configuration functions were never implemented. + # The codebase uses hardcoded constants instead: + # - @max_file_size_bytes 10_485_760 in GlobalSettingsLive + # - @default_max_rows 1000 in MemberCSV + # These tests have been removed as they tested non-existent functions. +end diff --git a/test/mv/membership/import/member_csv_test.exs b/test/mv/membership/import/member_csv_test.exs index 0304989..b4a099a 100644 --- a/test/mv/membership/import/member_csv_test.exs +++ b/test/mv/membership/import/member_csv_test.exs @@ -1,5 +1,5 @@ defmodule Mv.Membership.Import.MemberCSVTest do - use Mv.DataCase, async: false + use Mv.DataCase, async: true alias Mv.Membership.Import.MemberCSV @@ -35,11 +35,10 @@ defmodule Mv.Membership.Import.MemberCSVTest do end describe "prepare/2" do - test "function exists and accepts file_content and opts" do + test "accepts file_content and opts and returns tagged tuple" do file_content = "email\njohn@example.com" opts = [] - # This will fail until the function is implemented result = MemberCSV.prepare(file_content, opts) assert match?({:ok, _}, result) or match?({:error, _}, result) end @@ -65,11 +64,6 @@ defmodule Mv.Membership.Import.MemberCSVTest do assert {:error, _reason} = MemberCSV.prepare(file_content, opts) end - - test "function has documentation" do - # Check that @doc exists by reading the module - assert function_exported?(MemberCSV, :prepare, 2) - end end describe "process_chunk/4" do @@ -78,7 +72,7 @@ defmodule Mv.Membership.Import.MemberCSVTest do %{actor: system_actor} end - test "function exists and accepts chunk_rows_with_lines, column_map, custom_field_map, and opts", + test "accepts chunk_rows_with_lines, column_map, custom_field_map, and opts and returns tagged tuple", %{ actor: actor } do @@ -87,7 +81,6 @@ defmodule Mv.Membership.Import.MemberCSVTest do custom_field_map = %{} opts = [actor: actor] - # This will fail until the function is implemented result = MemberCSV.process_chunk(chunk_rows_with_lines, column_map, custom_field_map, opts) assert match?({:ok, _}, result) or match?({:error, _}, result) end @@ -231,7 +224,11 @@ defmodule Mv.Membership.Import.MemberCSVTest do custom_field_map = %{to_string(custom_field.id) => 1} custom_field_lookup = %{ - to_string(custom_field.id) => %{id: custom_field.id, value_type: custom_field.value_type} + to_string(custom_field.id) => %{ + id: custom_field.id, + value_type: custom_field.value_type, + name: custom_field.name + } } opts = [custom_field_lookup: custom_field_lookup, actor: actor] @@ -332,11 +329,6 @@ defmodule Mv.Membership.Import.MemberCSVTest do assert chunk_result.errors == [] end - test "function has documentation" do - # Check that @doc exists by reading the module - assert function_exported?(MemberCSV, :process_chunk, 4) - end - test "error capping collects exactly 50 errors", %{actor: actor} do # Create 50 rows with invalid emails chunk_rows_with_lines = @@ -611,15 +603,300 @@ defmodule Mv.Membership.Import.MemberCSVTest do end end - describe "module documentation" do - test "module has @moduledoc" do - # Check that the module exists and has documentation - assert Code.ensure_loaded?(MemberCSV) + describe "custom field import" do + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + %{actor: system_actor} + end - # Try to get the module documentation - {:docs_v1, _, _, _, %{"en" => moduledoc}, _, _} = Code.fetch_docs(MemberCSV) - assert is_binary(moduledoc) - assert String.length(moduledoc) > 0 + test "creates member with valid integer custom field value", %{actor: actor} do + # Create integer custom field + {:ok, custom_field} = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Alter", + value_type: :integer + }) + |> Ash.create(actor: actor) + + chunk_rows_with_lines = [ + {2, + %{ + member: %{email: "withage@example.com"}, + custom: %{to_string(custom_field.id) => "25"} + }} + ] + + column_map = %{email: 0} + custom_field_map = %{to_string(custom_field.id) => 1} + + custom_field_lookup = %{ + to_string(custom_field.id) => %{ + id: custom_field.id, + value_type: custom_field.value_type, + name: custom_field.name + } + } + + opts = [custom_field_lookup: custom_field_lookup, actor: actor] + + assert {:ok, chunk_result} = + MemberCSV.process_chunk(chunk_rows_with_lines, column_map, custom_field_map, opts) + + assert chunk_result.inserted == 1 + assert chunk_result.failed == 0 + + # Verify member and custom field value were created + members = Mv.Membership.list_members!(actor: actor) + member = Enum.find(members, &(&1.email == "withage@example.com")) + assert member != nil + + {:ok, member_with_cf} = Ash.load(member, :custom_field_values, actor: actor) + assert length(member_with_cf.custom_field_values) == 1 + cfv = List.first(member_with_cf.custom_field_values) + assert cfv.custom_field_id == custom_field.id + assert cfv.value.value == 25 + assert cfv.value.type == :integer + end + + test "returns error for invalid integer custom field value", %{actor: actor} do + # Create integer custom field + {:ok, custom_field} = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Alter", + value_type: :integer + }) + |> Ash.create(actor: actor) + + chunk_rows_with_lines = [ + {2, + %{ + member: %{email: "invalidage@example.com"}, + custom: %{to_string(custom_field.id) => "abc"} + }} + ] + + column_map = %{email: 0} + custom_field_map = %{to_string(custom_field.id) => 1} + + custom_field_lookup = %{ + to_string(custom_field.id) => %{ + id: custom_field.id, + value_type: custom_field.value_type, + name: custom_field.name + } + } + + opts = [custom_field_lookup: custom_field_lookup, actor: actor] + + assert {:ok, chunk_result} = + MemberCSV.process_chunk(chunk_rows_with_lines, column_map, custom_field_map, opts) + + assert chunk_result.inserted == 0 + assert chunk_result.failed == 1 + assert length(chunk_result.errors) == 1 + + error = List.first(chunk_result.errors) + assert error.csv_line_number == 2 + assert error.message =~ "custom_field: Alter" + assert error.message =~ "Number" + assert error.message =~ "abc" + end + + test "returns error for invalid date custom field value", %{actor: actor} do + # Create date custom field + {:ok, custom_field} = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Geburtstag", + value_type: :date + }) + |> Ash.create(actor: actor) + + chunk_rows_with_lines = [ + {3, + %{ + member: %{email: "invaliddate@example.com"}, + custom: %{to_string(custom_field.id) => "not-a-date"} + }} + ] + + column_map = %{email: 0} + custom_field_map = %{to_string(custom_field.id) => 1} + + custom_field_lookup = %{ + to_string(custom_field.id) => %{ + id: custom_field.id, + value_type: custom_field.value_type, + name: custom_field.name + } + } + + opts = [custom_field_lookup: custom_field_lookup, actor: actor] + + assert {:ok, chunk_result} = + MemberCSV.process_chunk(chunk_rows_with_lines, column_map, custom_field_map, opts) + + assert chunk_result.inserted == 0 + assert chunk_result.failed == 1 + assert length(chunk_result.errors) == 1 + + error = List.first(chunk_result.errors) + assert error.csv_line_number == 3 + assert error.message =~ "custom_field: Geburtstag" + assert error.message =~ "Date" + end + + test "returns error for invalid email custom field value", %{actor: actor} do + # Create email custom field + {:ok, custom_field} = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Work Email", + value_type: :email + }) + |> Ash.create(actor: actor) + + chunk_rows_with_lines = [ + {4, + %{ + member: %{email: "invalidemailcf@example.com"}, + custom: %{to_string(custom_field.id) => "not-an-email"} + }} + ] + + column_map = %{email: 0} + custom_field_map = %{to_string(custom_field.id) => 1} + + custom_field_lookup = %{ + to_string(custom_field.id) => %{ + id: custom_field.id, + value_type: custom_field.value_type, + name: custom_field.name + } + } + + opts = [custom_field_lookup: custom_field_lookup, actor: actor] + + assert {:ok, chunk_result} = + MemberCSV.process_chunk(chunk_rows_with_lines, column_map, custom_field_map, opts) + + assert chunk_result.inserted == 0 + assert chunk_result.failed == 1 + assert length(chunk_result.errors) == 1 + + error = List.first(chunk_result.errors) + assert error.csv_line_number == 4 + assert error.message =~ "custom_field: Work Email" + assert error.message =~ "E-Mail" + end + + test "returns error for invalid boolean custom field value", %{actor: actor} do + # Create boolean custom field + {:ok, custom_field} = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Is Active", + value_type: :boolean + }) + |> Ash.create(actor: actor) + + chunk_rows_with_lines = [ + {5, + %{ + member: %{email: "invalidbool@example.com"}, + custom: %{to_string(custom_field.id) => "maybe"} + }} + ] + + column_map = %{email: 0} + custom_field_map = %{to_string(custom_field.id) => 1} + + custom_field_lookup = %{ + to_string(custom_field.id) => %{ + id: custom_field.id, + value_type: custom_field.value_type, + name: custom_field.name + } + } + + opts = [custom_field_lookup: custom_field_lookup, actor: actor] + + assert {:ok, chunk_result} = + MemberCSV.process_chunk(chunk_rows_with_lines, column_map, custom_field_map, opts) + + assert chunk_result.inserted == 0 + assert chunk_result.failed == 1 + assert length(chunk_result.errors) == 1 + + error = List.first(chunk_result.errors) + assert error.csv_line_number == 5 + assert error.message =~ "custom_field: Is Active" + # Error message should indicate boolean/Yes-No validation failure + assert String.contains?(error.message, "Yes/No") || + String.contains?(error.message, "true/false") || + String.contains?(error.message, "boolean") + end + end + + describe "prepare/2 with custom fields" do + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + # Create a custom field + {:ok, custom_field} = + Mv.Membership.CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "Membership Number", + value_type: :string + }) + |> Ash.create(actor: system_actor) + + %{actor: system_actor, custom_field: custom_field} + end + + test "includes custom field in custom_field_map when header matches", %{ + custom_field: custom_field + } do + # CSV with custom field column + csv_content = "email;Membership Number\njohn@example.com;12345" + + assert {:ok, import_state} = MemberCSV.prepare(csv_content) + + # Check that custom field is mapped + assert Map.has_key?(import_state.custom_field_map, to_string(custom_field.id)) + assert import_state.column_map[:email] == 0 + end + + test "includes warning for unknown custom field column", %{custom_field: _custom_field} do + # CSV with unknown custom field column (not matching any existing custom field) + csv_content = "email;NichtExistierend\njohn@example.com;value" + + assert {:ok, import_state} = MemberCSV.prepare(csv_content) + + # Check that warning is present + assert import_state.warnings != [] + warning = List.first(import_state.warnings) + assert warning =~ "NichtExistierend" + assert warning =~ "ignored" + assert warning =~ "custom field" + + # Check that unknown column is not in custom_field_map + assert import_state.custom_field_map == %{} + # Member import should still succeed + assert import_state.column_map[:email] == 0 + end + + test "import succeeds even with unknown custom field columns", %{custom_field: _custom_field} do + # CSV with unknown custom field column + csv_content = "email;UnknownField\njohn@example.com;value" + + assert {:ok, import_state} = MemberCSV.prepare(csv_content) + + # Import state should be valid + assert import_state.column_map[:email] == 0 + assert import_state.chunks != [] end end end From 12715f3d8523f7345de9434b995518320c41b4bd Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 13:07:08 +0100 Subject: [PATCH 11/38] refactoring --- lib/mv/membership/import/member_csv.ex | 172 +++++++++++++++---------- 1 file changed, 104 insertions(+), 68 deletions(-) diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index 5924001..2a1c0b4 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -524,32 +524,12 @@ defmodule Mv.Membership.Import.MemberCSV do {:error, %Error{csv_line_number: line_number, field: nil, message: first_error}} {:ok, custom_field_values} -> - # Create member with custom field values - member_attrs_with_cf = - trimmed_member_attrs - |> Map.put(:custom_field_values, custom_field_values) - - # Only include custom_field_values if not empty - final_attrs = - if Enum.empty?(custom_field_values) do - Map.delete(member_attrs_with_cf, :custom_field_values) - else - member_attrs_with_cf - end - - case Mv.Membership.create_member(final_attrs, actor: actor) do - {:ok, member} -> - {:ok, member} - - {:error, %Ash.Error.Invalid{} = error} -> - # Extract email from final_attrs for better error messages - email = Map.get(final_attrs, :email) || Map.get(trimmed_member_attrs, :email) - {:error, format_ash_error(error, line_number, email)} - - {:error, error} -> - {:error, - %Error{csv_line_number: line_number, field: nil, message: inspect(error)}} - end + create_member_with_custom_fields( + trimmed_member_attrs, + custom_field_values, + line_number, + actor + ) end end rescue @@ -557,6 +537,40 @@ defmodule Mv.Membership.Import.MemberCSV do {:error, %Error{csv_line_number: line_number, field: nil, message: Exception.message(e)}} end + # Creates a member with custom field values, handling errors appropriately + defp create_member_with_custom_fields( + trimmed_member_attrs, + custom_field_values, + line_number, + actor + ) do + # Create member with custom field values + member_attrs_with_cf = + trimmed_member_attrs + |> Map.put(:custom_field_values, custom_field_values) + + # Only include custom_field_values if not empty + final_attrs = + if Enum.empty?(custom_field_values) do + Map.delete(member_attrs_with_cf, :custom_field_values) + else + member_attrs_with_cf + end + + case Mv.Membership.create_member(final_attrs, actor: actor) do + {:ok, member} -> + {:ok, member} + + {:error, %Ash.Error.Invalid{} = error} -> + # Extract email from final_attrs for better error messages + email = Map.get(final_attrs, :email) || Map.get(trimmed_member_attrs, :email) + {:error, format_ash_error(error, line_number, email)} + + {:error, error} -> + {:error, %Error{csv_line_number: line_number, field: nil, message: inspect(error)}} + end + end + # Prepares custom field values from row map for Ash # Returns {:ok, [custom_field_value_maps]} or {:error, [validation_errors]} defp prepare_custom_field_values(custom_attrs, custom_field_lookup) when is_map(custom_attrs) do @@ -564,25 +578,13 @@ defmodule Mv.Membership.Import.MemberCSV do custom_attrs |> Enum.filter(fn {_id, value} -> value != nil && value != "" end) |> Enum.reduce({[], []}, fn {custom_field_id_str, value}, {acc_values, acc_errors} -> - case Map.get(custom_field_lookup, custom_field_id_str) do - nil -> - # Custom field not found, skip - {acc_values, acc_errors} - - %{id: custom_field_id, value_type: value_type, name: custom_field_name} -> - case format_custom_field_value(value, value_type, custom_field_name) do - {:ok, formatted_value} -> - value_map = %{ - "custom_field_id" => to_string(custom_field_id), - "value" => formatted_value - } - - {[value_map | acc_values], acc_errors} - - {:error, reason} -> - {acc_values, [reason | acc_errors]} - end - end + process_single_custom_field( + custom_field_id_str, + value, + custom_field_lookup, + acc_values, + acc_errors + ) end) if Enum.empty?(errors) do @@ -594,6 +596,35 @@ defmodule Mv.Membership.Import.MemberCSV do defp prepare_custom_field_values(_, _), do: {:ok, []} + # Processes a single custom field value and returns updated accumulator + defp process_single_custom_field( + custom_field_id_str, + value, + custom_field_lookup, + acc_values, + acc_errors + ) do + case Map.get(custom_field_lookup, custom_field_id_str) do + nil -> + # Custom field not found, skip + {acc_values, acc_errors} + + %{id: custom_field_id, value_type: value_type, name: custom_field_name} -> + case format_custom_field_value(value, value_type, custom_field_name) do + {:ok, formatted_value} -> + value_map = %{ + "custom_field_id" => to_string(custom_field_id), + "value" => formatted_value + } + + {[value_map | acc_values], acc_errors} + + {:error, reason} -> + {acc_values, [reason | acc_errors]} + end + end + end + # Formats a custom field value according to its type # Uses _union_type and _union_value format as expected by Ash # Returns {:ok, formatted_value} or {:error, error_message} @@ -620,31 +651,19 @@ defmodule Mv.Membership.Import.MemberCSV do defp format_custom_field_value(value, :boolean, custom_field_name) when is_binary(value) do trimmed = String.trim(value) - lower = String.downcase(trimmed) - bool_value = - case lower do - "true" -> true - "1" -> true - "yes" -> true - "ja" -> true - "false" -> false - "0" -> false - "no" -> false - "nein" -> false - _ -> nil - end + case parse_boolean_value(trimmed) do + {:ok, bool_value} -> + {:ok, %{"_union_type" => "boolean", "_union_value" => bool_value}} - if bool_value != nil do - {:ok, %{"_union_type" => "boolean", "_union_value" => bool_value}} - else - {:error, - format_custom_field_error_with_details( - custom_field_name, - :boolean, - trimmed, - gettext("(true/false/1/0/yes/no/ja/nein)") - )} + :error -> + {:error, + format_custom_field_error_with_details( + custom_field_name, + :boolean, + trimmed, + gettext("(true/false/1/0/yes/no/ja/nein)") + )} end end @@ -690,6 +709,23 @@ defmodule Mv.Membership.Import.MemberCSV do {:ok, %{"_union_type" => "string", "_union_value" => String.trim(value)}} end + # Parses a boolean value from a string, supporting multiple formats + defp parse_boolean_value(value) when is_binary(value) do + lower = String.downcase(value) + parse_boolean_value_lower(lower) + end + + # Helper function with pattern matching for boolean values + defp parse_boolean_value_lower("true"), do: {:ok, true} + defp parse_boolean_value_lower("1"), do: {:ok, true} + defp parse_boolean_value_lower("yes"), do: {:ok, true} + defp parse_boolean_value_lower("ja"), do: {:ok, true} + defp parse_boolean_value_lower("false"), do: {:ok, false} + defp parse_boolean_value_lower("0"), do: {:ok, false} + defp parse_boolean_value_lower("no"), do: {:ok, false} + defp parse_boolean_value_lower("nein"), do: {:ok, false} + defp parse_boolean_value_lower(_), do: :error + # Generates a consistent error message for custom field validation failures # Uses human-readable field type labels (e.g., "Number" instead of "integer") defp format_custom_field_error(custom_field_name, value_type, value) do From f5591c392ad3cce2831fe05dd3df6aa1993da108 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 13:41:59 +0100 Subject: [PATCH 12/38] i18n: add translation --- lib/mv_web/live/global_settings_live.ex | 10 ++--- priv/gettext/de/LC_MESSAGES/default.po | 55 ++++++++++++++++++++----- priv/gettext/default.pot | 40 +++++++++++++----- priv/gettext/en/LC_MESSAGES/default.po | 55 ++++++++++++++++++++----- 4 files changed, 124 insertions(+), 36 deletions(-) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 9d3bec9..e35b064 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -140,21 +140,19 @@ defmodule MvWeb.GlobalSettingsLive do
<.icon name="hero-information-circle" class="size-5" aria-hidden="true" />
-

- {gettext("Custom Fields in CSV Import")} -

+

{gettext( - "Custom fields must be created in Mila before importing. Use the custom field name as the CSV column header. Unknown custom field columns will be ignored with a warning." + "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." )}

<.link href="#custom_fields" - class="link link-primary" + class="link" data-testid="custom-fields-link" > - {gettext("Manage Custom Fields")} + {gettext("Manage Memberdata")}

diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index d650aa2..0db43c7 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1985,11 +1985,6 @@ msgstr "CSV Datei" msgid "CSV files only, maximum 10 MB" msgstr "Nur CSV Dateien, maximal 10 MB" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "Custom fields must be created in Mila before importing CSV files with custom field columns" -msgstr "Individuelle Datenfelder müssen zuerst in Mila angelegt werden bevor das Importieren von diesen Feldern mit CSV Dateien mölich ist." - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Download CSV templates:" @@ -2120,11 +2115,6 @@ msgstr "Erfolgreich eingefügt: %{count} Mitglied(er)" msgid "Summary" msgstr "Zusammenfassung" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "Use the custom field name as the CSV column header (same normalization as member fields applies)" -msgstr "Verwenden Sie den Namen des benutzerdefinierten Feldes als CSV-Spaltenüberschrift (gleiche Normalisierung wie bei Mitgliedsfeldern)" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format, fuzzy msgid "Warnings" @@ -2272,3 +2262,48 @@ msgstr "Nicht berechtigt." #, elixir-autogen, elixir-format msgid "Could not load data fields. Please check your permissions." msgstr "Datenfelder konnten nicht geladen werden. Bitte überprüfen Sie Ihre Berechtigungen." + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "(ISO-8601 format: YYYY-MM-DD)" +msgstr "(ISO-8601 Format: JJJJ-MM-TT)" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "(true/false/1/0/yes/no/ja/nein)" +msgstr "(true/false/1/0/yes/no/ja/nein)" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "custom_field: %{name} – expected %{type} %{details}, got: %{value}" +msgstr "Datenfeld: %{name} – erwartet %{type} %{details}, erhalten: %{value}" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "custom_field: %{name} – expected %{type}, got: %{value}" +msgstr "Datenfeld: %{name} – erwartet %{type}, erhalten: %{value}" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Manage Memberdata" +msgstr "Mitgliederdaten verwalten" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." +msgstr "Verwende die Namen der Datenfelder als Spaltennamen in der CSV Datei. Datenfelder müssen in Mila bereits angelegt sein, damit sie importiert werden können. Sie müssen in der Liste der Mitgliederdaten als Datenfeld enthalten sein (z.B. E-Mail). Spalten mit unbekannten Spaltenüberschriften werden mit einer Warnung ignoriert." + +#~ #: lib/mv_web/live/global_settings_live.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Custom Fields in CSV Import" +#~ msgstr "Benutzerdefinierte Felder" + +#~ #: lib/mv_web/live/global_settings_live.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Individual data fields must be created in Mila before importing. Use the field name as the CSV column header. Unknown custom field columns will be ignored with a warning." +#~ msgstr "Individuelle Datenfelder müssen in Mila erstellt werden, bevor sie importiert werden können. Verwenden Sie den Namen des Datenfeldes als CSV-Spaltenüberschrift. Unbekannte Spaltenüberschriften werden mit einer Warnung ignoriert." + +#~ #: lib/mv_web/live/global_settings_live.ex +#~ #, elixir-autogen, elixir-format +#~ msgid "Manage Custom Fields" +#~ msgstr "Benutzerdefinierte Felder verwalten" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 98f9d7b..c474aef 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1986,11 +1986,6 @@ msgstr "" msgid "CSV files only, maximum 10 MB" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "Custom fields must be created in Mila before importing CSV files with custom field columns" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Download CSV templates:" @@ -2121,11 +2116,6 @@ msgstr "" msgid "Summary" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "Use the custom field name as the CSV column header (same normalization as member fields applies)" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Warnings" @@ -2273,3 +2263,33 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Could not load data fields. Please check your permissions." msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "(ISO-8601 format: YYYY-MM-DD)" +msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "(true/false/1/0/yes/no/ja/nein)" +msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "custom_field: %{name} – expected %{type} %{details}, got: %{value}" +msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "custom_field: %{name} – expected %{type}, got: %{value}" +msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Manage Memberdata" +msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 95a3c3a..f9fd014 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1986,11 +1986,6 @@ msgstr "" msgid "CSV files only, maximum 10 MB" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "Custom fields must be created in Mila before importing CSV files with custom field columns" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format msgid "Download CSV templates:" @@ -2121,11 +2116,6 @@ msgstr "" msgid "Summary" msgstr "" -#: lib/mv_web/live/global_settings_live.ex -#, elixir-autogen, elixir-format -msgid "Use the custom field name as the CSV column header (same normalization as member fields applies)" -msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format, fuzzy msgid "Warnings" @@ -2273,3 +2263,48 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Could not load data fields. Please check your permissions." msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "(ISO-8601 format: YYYY-MM-DD)" +msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "(true/false/1/0/yes/no/ja/nein)" +msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "custom_field: %{name} – expected %{type} %{details}, got: %{value}" +msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "custom_field: %{name} – expected %{type}, got: %{value}" +msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "Manage Memberdata" +msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." +msgstr "" + +#~ #: lib/mv_web/live/global_settings_live.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Custom Fields in CSV Import" +#~ msgstr "" + +#~ #: lib/mv_web/live/global_settings_live.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Individual data fields must be created in Mila before importing. Use the field name as the CSV column header. Unknown custom field columns will be ignored with a warning." +#~ msgstr "" + +#~ #: lib/mv_web/live/global_settings_live.ex +#~ #, elixir-autogen, elixir-format +#~ msgid "Manage Custom Fields" +#~ msgstr "" From c56ca68922a9db4fcaa5924b5f2576e8ed8cd114 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 13:42:24 +0100 Subject: [PATCH 13/38] docs: update docs --- docs/csv-member-import-v1.md | 100 ++++++++++++++++++++++++----------- 1 file changed, 69 insertions(+), 31 deletions(-) diff --git a/docs/csv-member-import-v1.md b/docs/csv-member-import-v1.md index 25a4e11..0cd8a02 100644 --- a/docs/csv-member-import-v1.md +++ b/docs/csv-member-import-v1.md @@ -2,7 +2,7 @@ **Version:** 1.0 **Last Updated:** 2026-01-13 -**Status:** In Progress (Backend Complete, UI Pending) +**Status:** In Progress (Backend Complete, UI Complete, Tests Pending) **Related Documents:** - [Feature Roadmap](./feature-roadmap.md) - Overall feature planning @@ -15,15 +15,15 @@ - ✅ Issue #4: Header Normalization + Per-Header Mapping - ✅ Issue #5: Validation (Required Fields) + Error Formatting - ✅ Issue #6: Persistence via Ash Create + Per-Row Error Capture (with Error-Capping) -- ✅ Issue #11: Custom Field Import (Backend) +- ✅ Issue #7: Admin Global Settings LiveView UI (Upload + Start Import + Results + Template Links) +- ✅ Issue #8: Authorization + Limits +- ✅ Issue #11: Custom Field Import (Backend + UI) **In Progress / Pending:** -- ⏳ Issue #7: Admin Global Settings LiveView UI (Upload + Start Import + Results) -- ⏳ Issue #8: Authorization + Limits - ⏳ Issue #9: End-to-End LiveView Tests + Fixtures - ⏳ Issue #10: Documentation Polish -**Latest Update:** Error-Capping in `process_chunk/4` implemented (2025-01-XX) +**Latest Update:** CSV Import UI fully implemented in GlobalSettingsLive with chunk processing, progress tracking, error display, and custom field support (2026-01-13) --- @@ -161,6 +161,13 @@ A **basic CSV member import feature** that allows administrators to upload a CSV - Any custom field column using the custom field's **name** as the header (e.g., `membership_number`, `birth_date`) - **Important:** Custom fields must be created in Mila before importing. The CSV header must match the custom field name exactly (same normalization as member fields). - **Behavior:** If the CSV contains custom field columns that don't exist in Mila, a warning message will be shown and those columns will be ignored during import. +- **Value Validation:** Custom field values are validated according to the custom field type: + - **string**: Any text value (trimmed) + - **integer**: Must be a valid integer (e.g., `42`, `-10`). Invalid values will cause a row error with the custom field name and reason. + - **boolean**: Accepts `true`, `false`, `1`, `0`, `yes`, `no`, `ja`, `nein` (case-insensitive). Invalid values will cause a row error. + - **date**: Must be in ISO-8601 format (YYYY-MM-DD, e.g., `2024-01-15`). Invalid values will cause a row error. + - **email**: Must be a valid email format (contains `@`, 5-254 characters, valid format). Invalid values will cause a row error. +- **Error Messages:** Custom field validation errors are included in the import error list with format: `custom_field: – ` (e.g., `custom_field: Alter – expected integer, got: abc`) **Member Field Header Mapping:** @@ -496,36 +503,51 @@ Use `Mv.Authorization.PermissionSets` (preferred) instead of hard-coded string c **Dependencies:** Issue #6 +**Status:** ✅ **COMPLETED** + **Goal:** UI section with upload, progress, results, and template links. **Tasks:** -- [ ] Render import section only for admins -- [ ] **Add prominent UI notice about custom fields:** +- [x] Render import section only for admins +- [x] **Add prominent UI notice about custom fields:** - Display alert/info box: "Custom fields must be created in Mila before importing CSV files with custom field columns" - Explain: "Use the custom field name as the CSV column header (same normalization as member fields applies)" - Add link to custom fields management section -- [ ] Configure `allow_upload/3`: - - `.csv` only, `max_entries: 1`, `max_file_size: 10MB`, `auto_upload: false` -- [ ] `handle_event("start_import", ...)`: +- [x] Configure `allow_upload/3`: + - `.csv` only, `max_entries: 1`, `max_file_size: 10MB`, `auto_upload: true` (auto-upload enabled for better UX) +- [x] `handle_event("start_import", ...)`: - Admin permission check - Consume upload -> read file content - Call `MemberCSV.prepare/2` - Store `import_state` in assigns (chunks + column_map + metadata) - Initialize progress assigns - `send(self(), {:process_chunk, 0})` -- [ ] `handle_info({:process_chunk, idx}, socket)`: +- [x] `handle_info({:process_chunk, idx}, socket)`: - Fetch chunk from `import_state` - - Call `MemberCSV.process_chunk/3` + - Call `MemberCSV.process_chunk/4` with error capping support - Merge counts/errors into progress assigns (cap errors at 50 overall) - Schedule next chunk (or finish and show results) -- [ ] Results UI: + - Async task processing with SQL sandbox support for tests +- [x] Results UI: - Success count - Failure count - Error list (line number + message + field) - **Warning messages for unknown custom field columns** (non-existent names) shown in results + - Progress indicator during import + - Error truncation notice when errors exceed limit **Template links:** -- Link `/templates/member_import_en.csv` and `/templates/member_import_de.csv` via Phoenix static path helpers. +- [x] Link `/templates/member_import_en.csv` and `/templates/member_import_de.csv` via Phoenix static path helpers. + +**Definition of Done:** +- [x] Upload area with drag & drop support +- [x] Template download links (EN/DE) +- [x] Progress tracking during import +- [x] Results display with success/error counts +- [x] Error list with line numbers and field information +- [x] Warning display for unknown custom field columns +- [x] Admin-only access control +- [x] Async chunk processing with proper error handling --- @@ -533,19 +555,32 @@ Use `Mv.Authorization.PermissionSets` (preferred) instead of hard-coded string c **Dependencies:** None (can be parallelized) +**Status:** ✅ **COMPLETED** + **Goal:** Ensure admin-only access and enforce limits. **Tasks:** -- [ ] Admin check in start import event handler -- [ ] File size enforced in upload config -- [ ] Row limit enforced in `MemberCSV.prepare/2` (max_rows from config) -- [ ] Configuration: - ```elixir - config :mv, csv_import: [ - max_file_size_mb: 10, - max_rows: 1000 - ] - ``` +- [x] Admin check in start import event handler (via `Authorization.can?/3`) +- [x] File size enforced in upload config (`max_file_size: 10MB`) +- [x] Row limit enforced in `MemberCSV.prepare/2` (max_rows: 1000, configurable via opts) +- [x] Chunk size limit (200 rows per chunk) +- [x] Error limit (50 errors per import) +- [x] UI-level authorization check (import section only visible to admins) +- [x] Event-level authorization check (prevents unauthorized import attempts) + +**Implementation Notes:** +- File size limit: 10 MB (10,485,760 bytes) enforced via `allow_upload/3` +- Row limit: 1,000 rows (excluding header) enforced in `MemberCSV.prepare/2` +- Chunk size: 200 rows per chunk (configurable via opts) +- Error limit: 50 errors per import (configurable via `@max_errors`) +- Authorization uses `MvWeb.Authorization.can?/3` with `:create` permission on `Mv.Membership.Member` + +**Definition of Done:** +- [x] Admin-only access enforced at UI and event level +- [x] File size limit enforced +- [x] Row count limit enforced +- [x] Chunk processing with size limits +- [x] Error capping implemented --- @@ -589,7 +624,7 @@ Use `Mv.Authorization.PermissionSets` (preferred) instead of hard-coded string c **Priority:** High (Core v1 Feature) -**Status:** ✅ **COMPLETED** (Backend Implementation) +**Status:** ✅ **COMPLETED** (Backend + UI Implementation) **Goal:** Support importing custom field values from CSV columns. Custom fields should exist in Mila before import for best results. @@ -604,23 +639,26 @@ Use `Mv.Authorization.PermissionSets` (preferred) instead of hard-coded string c - [x] Query existing custom fields during `prepare/2` to map custom field columns - [x] Collect unknown custom field columns and add warning messages (don't fail import) - [x] Map custom field CSV values to `CustomFieldValue` creation in `process_chunk/4` -- [x] Handle custom field type validation (string, integer, boolean, date, email) +- [x] Handle custom field type validation (string, integer, boolean, date, email) with proper error messages - [x] Create `CustomFieldValue` records linked to members during import -- [ ] Update error messages to include custom field validation errors (if needed) -- [ ] Add UI help text explaining custom field requirements (pending Issue #7): +- [x] Validate custom field values and return structured errors with custom field name and reason +- [x] UI help text and link to custom field management (implemented in Issue #7) +- [x] Update error messages to include custom field validation errors (format: `custom_field: – expected , got: `) +- [x] Add UI help text explaining custom field requirements (completed in Issue #7): - "Custom fields must be created in Mila before importing" - "Use the custom field name as the CSV column header (same normalization as member fields)" - Link to custom fields management section -- [ ] Update CSV templates documentation to explain custom field columns (pending Issue #1) +- [x] Update CSV templates documentation to explain custom field columns (documented in Issue #1) - [x] Add tests for custom field import (valid, invalid name, type validation, warning for unknown) **Definition of Done:** - [x] Custom field columns are recognized by name (with normalization) - [x] Warning messages shown for unknown custom field columns (import continues) - [x] Custom field values are created and linked to members -- [x] Type validation works for all custom field types -- [ ] UI clearly explains custom field requirements (pending Issue #7) +- [x] Type validation works for all custom field types (string, integer, boolean, date, email) +- [x] UI clearly explains custom field requirements (completed in Issue #7) - [x] Tests cover custom field import scenarios (including warning for unknown names) +- [x] Error messages include custom field validation errors with proper formatting **Implementation Notes:** - Custom field lookup is built in `prepare/2` and passed via `custom_field_lookup` in opts From 71db9cf3c1cd6bfc09c4ff648202cba31f8c0896 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 13:54:27 +0100 Subject: [PATCH 14/38] formatting --- lib/mv_web/live/global_settings_live.ex | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index b0a8640..bbd19ca 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -140,7 +140,6 @@ defmodule MvWeb.GlobalSettingsLive do
<.icon name="hero-information-circle" class="size-5" aria-hidden="true" />
-

{gettext( "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." From b21c3df7ef09d319b4e738a4c0019e4fe8a73a12 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 14:34:12 +0100 Subject: [PATCH 15/38] refactoring --- lib/mv/membership/import/member_csv.ex | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index 2a1c0b4..bc9acc8 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -233,17 +233,20 @@ defmodule Mv.Membership.Import.MemberCSV do # Builds a row map from raw row values using column maps defp build_row_map(row_values, maps) do + row_tuple = List.to_tuple(row_values) + tuple_size = tuple_size(row_tuple) + member_map = maps.member |> Enum.reduce(%{}, fn {field, index}, acc -> - value = Enum.at(row_values, index, "") + value = if index < tuple_size, do: elem(row_tuple, index), else: "" Map.put(acc, field, value) end) custom_map = maps.custom |> Enum.reduce(%{}, fn {custom_field_id, index}, acc -> - value = Enum.at(row_values, index, "") + value = if index < tuple_size, do: elem(row_tuple, index), else: "" Map.put(acc, custom_field_id, value) end) From aef3aa299f33fe36ebd901bf420f039341b6eb59 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 15:04:07 +0100 Subject: [PATCH 16/38] fix test --- test/mv_web/live/global_settings_live_test.exs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index f217311..083c813 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -110,7 +110,7 @@ defmodule MvWeb.GlobalSettingsLiveTest do {:ok, _view, html} = live(conn, ~p"/settings") # Check for custom fields notice text - assert html =~ "Custom fields" or html =~ "custom field" + assert html =~ "Use the data field name" end test "admin user sees template download links", %{conn: conn} do From 960506d16aba1e1e5ca45759d7c13055197dd4a3 Mon Sep 17 00:00:00 2001 From: carla Date: Mon, 2 Feb 2026 16:52:35 +0100 Subject: [PATCH 17/38] refactoring --- lib/mv/membership/import/member_csv.ex | 56 ++++++++++++++----- priv/gettext/de/LC_MESSAGES/default.po | 5 ++ priv/gettext/default.pot | 5 ++ priv/gettext/en/LC_MESSAGES/default.po | 6 +- test/mv/config_test.exs | 14 ----- .../live/global_settings_live_config_test.exs | 2 +- 6 files changed, 57 insertions(+), 31 deletions(-) delete mode 100644 test/mv/config_test.exs diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index bc9acc8..c967bf5 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -191,8 +191,10 @@ defmodule Mv.Membership.Import.MemberCSV do normalized != "" && not member_field?(normalized) end) |> Enum.map(fn header -> - "Unknown column '#{header}' will be ignored. " <> - "If this is a custom field, create it in Mila before importing." + gettext( + "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing.", + header: header + ) end) {:ok, %{member: member_map, custom: custom_map}, warnings} @@ -311,7 +313,7 @@ defmodule Mv.Membership.Import.MemberCSV do custom_field_lookup = Keyword.get(opts, :custom_field_lookup, %{}) existing_error_count = Keyword.get(opts, :existing_error_count, 0) max_errors = Keyword.get(opts, :max_errors, @default_max_errors) - actor = Keyword.fetch!(opts, :actor) + actor = Keyword.get(opts, :actor, SystemActor.get_system_actor()) {inserted, failed, errors, _collected_error_count, truncated?} = Enum.reduce(chunk_rows_with_lines, {0, 0, [], 0, false}, fn {line_number, row_map}, @@ -607,13 +609,38 @@ defmodule Mv.Membership.Import.MemberCSV do acc_values, acc_errors ) do + # Trim value early and skip if empty + trimmed_value = if is_binary(value), do: String.trim(value), else: value + + # Skip empty values (after trimming) - don't create CFV + if trimmed_value == "" or trimmed_value == nil do + {acc_values, acc_errors} + else + process_non_empty_custom_field( + custom_field_id_str, + trimmed_value, + custom_field_lookup, + acc_values, + acc_errors + ) + end + end + + # Processes a non-empty custom field value + defp process_non_empty_custom_field( + custom_field_id_str, + trimmed_value, + custom_field_lookup, + acc_values, + acc_errors + ) do case Map.get(custom_field_lookup, custom_field_id_str) do nil -> # Custom field not found, skip {acc_values, acc_errors} %{id: custom_field_id, value_type: value_type, name: custom_field_name} -> - case format_custom_field_value(value, value_type, custom_field_name) do + case format_custom_field_value(trimmed_value, value_type, custom_field_name) do {:ok, formatted_value} -> value_map = %{ "custom_field_id" => to_string(custom_field_id), @@ -691,17 +718,16 @@ defmodule Mv.Membership.Import.MemberCSV do defp format_custom_field_value(value, :email, custom_field_name) when is_binary(value) do trimmed = String.trim(value) - # Use simple validation: must contain @ and have valid format - # For CSV import, we use a simpler check than EctoCommons.EmailValidator - # to avoid dependencies and keep it fast - if String.contains?(trimmed, "@") and String.length(trimmed) >= 5 and - String.length(trimmed) <= 254 do - # Basic format check: username@domain.tld - if Regex.match?(~r/^[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}$/, trimmed) do - {:ok, %{"_union_type" => "email", "_union_value" => trimmed}} - else - {:error, format_custom_field_error(custom_field_name, :email, trimmed)} - end + # Use EctoCommons.EmailValidator for consistency with Member email validation + changeset = + {%{}, %{email: :string}} + |> Ecto.Changeset.cast(%{email: trimmed}, [:email]) + |> EctoCommons.EmailValidator.validate_email(:email, + checks: Mv.Constants.email_validator_checks() + ) + + if changeset.valid? do + {:ok, %{"_union_type" => "email", "_union_value" => trimmed}} else {:error, format_custom_field_error(custom_field_name, :email, trimmed)} end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index f1ae5a3..041507b 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2293,6 +2293,11 @@ msgstr "Mitgliederdaten verwalten" msgid "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." msgstr "Verwende die Namen der Datenfelder als Spaltennamen in der CSV Datei. Datenfelder müssen in Mila bereits angelegt sein, damit sie importiert werden können. Sie müssen in der Liste der Mitgliederdaten als Datenfeld enthalten sein (z.B. E-Mail). Spalten mit unbekannten Spaltenüberschriften werden mit einer Warnung ignoriert." +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." +msgstr "Unbekannte Spalte '%{header}' wird ignoriert. Falls dies ein Datenfeld ist, erstellen Sie es in Mila vor dem Import." + #~ #: lib/mv_web/live/global_settings_live.ex #~ #, elixir-autogen, elixir-format, fuzzy #~ msgid "Custom Fields in CSV Import" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 1ff9c81..2861f2d 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2293,3 +2293,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." msgstr "" + +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index d71a397..3fe9ce3 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2259,7 +2259,6 @@ msgstr "" msgid "Could not load data fields. Please check your permissions." msgstr "" - #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format, fuzzy msgid "CSV files only, maximum %{size} MB" @@ -2295,6 +2294,11 @@ msgstr "" msgid "Use the data field name as the CSV column header in your file. Data fields must exist in Mila before importing, so they must be listed in the list of memberdate (like e-mail or first name). Unknown data field columns will be ignored with a warning." msgstr "" +#: lib/mv/membership/import/member_csv.ex +#, elixir-autogen, elixir-format +msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." +msgstr "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." + #~ #: lib/mv_web/live/global_settings_live.ex #~ #, elixir-autogen, elixir-format, fuzzy #~ msgid "Custom Fields in CSV Import" diff --git a/test/mv/config_test.exs b/test/mv/config_test.exs deleted file mode 100644 index 076915f..0000000 --- a/test/mv/config_test.exs +++ /dev/null @@ -1,14 +0,0 @@ -defmodule Mv.ConfigTest do - @moduledoc """ - Tests for Mv.Config module. - """ - use ExUnit.Case, async: false - - alias Mv.Config - - # Note: CSV import configuration functions were never implemented. - # The codebase uses hardcoded constants instead: - # - @max_file_size_bytes 10_485_760 in GlobalSettingsLive - # - @default_max_rows 1000 in MemberCSV - # These tests have been removed as they tested non-existent functions. -end diff --git a/test/mv_web/live/global_settings_live_config_test.exs b/test/mv_web/live/global_settings_live_config_test.exs index c940594..1f06145 100644 --- a/test/mv_web/live/global_settings_live_config_test.exs +++ b/test/mv_web/live/global_settings_live_config_test.exs @@ -10,7 +10,7 @@ defmodule MvWeb.GlobalSettingsLiveConfigTest do import Phoenix.LiveViewTest # Helper function to upload CSV file in tests - defp upload_csv_file(view, csv_content, filename \\ "test_import.csv") do + defp upload_csv_file(view, csv_content, filename) do view |> file_input("#csv-upload-form", :csv_file, [ %{ From 3d46ba655f6ada5ba3ade196f37b984355e07280 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 14:34:24 +0100 Subject: [PATCH 18/38] Add Actor.permission_set_name/1 and admin?/1 for consistent capability checks - Actor.permission_set_name(actor) returns role's permission set (supports nil role load). - Actor.admin?(actor) returns true for system user or admin permission set. - ActorIsAdmin policy check delegates to Actor.admin?/1. --- lib/mv/authorization/actor.ex | 51 +++++++++++++++++-- lib/mv/authorization/checks/actor_is_admin.ex | 13 ++--- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/lib/mv/authorization/actor.ex b/lib/mv/authorization/actor.ex index 3482043..bfc99ed 100644 --- a/lib/mv/authorization/actor.ex +++ b/lib/mv/authorization/actor.ex @@ -1,6 +1,7 @@ defmodule Mv.Authorization.Actor do @moduledoc """ - Helper functions for ensuring User actors have required data loaded. + Helper functions for ensuring User actors have required data loaded + and for querying actor capabilities (e.g. admin, permission set). ## Actor Invariant @@ -27,8 +28,11 @@ defmodule Mv.Authorization.Actor do assign(socket, :current_user, user) end - # In tests - user = Actor.ensure_loaded(user) + # Check if actor is admin (policy checks, validations) + if Actor.admin?(actor), do: ... + + # Get permission set name (string or nil) + ps_name = Actor.permission_set_name(actor) ## Security Note @@ -47,6 +51,8 @@ defmodule Mv.Authorization.Actor do require Logger + alias Mv.Helpers.SystemActor + @doc """ Ensures the actor (User) has their `:role` relationship loaded. @@ -96,4 +102,43 @@ defmodule Mv.Authorization.Actor do actor end end + + @doc """ + Returns the actor's permission set name (string or atom) from their role, or nil. + + Ensures role is loaded (including when role is nil). Supports both atom and + string keys for session/socket assigns. Use for capability checks consistent + with `ActorIsAdmin` and `HasPermission`. + """ + @spec permission_set_name(Mv.Accounts.User.t() | map() | nil) :: String.t() | atom() | nil + def permission_set_name(nil), do: nil + + def permission_set_name(actor) do + actor = actor |> ensure_loaded() |> maybe_load_role() + + get_in(actor, [Access.key(:role), Access.key(:permission_set_name)]) || + get_in(actor, [Access.key("role"), Access.key("permission_set_name")]) + end + + @doc """ + Returns true if the actor is the system user or has the admin permission set. + + Use for validations and policy checks that require admin capability (e.g. + changing a linked member's email). Consistent with `ActorIsAdmin` policy check. + """ + @spec admin?(Mv.Accounts.User.t() | map() | nil) :: boolean() + def admin?(nil), do: false + + def admin?(actor) do + SystemActor.system_user?(actor) or permission_set_name(actor) in ["admin", :admin] + end + + defp maybe_load_role(%Mv.Accounts.User{role: nil} = user) do + case Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) do + {:ok, loaded} -> loaded + _ -> user + end + end + + defp maybe_load_role(actor), do: actor end diff --git a/lib/mv/authorization/checks/actor_is_admin.ex b/lib/mv/authorization/checks/actor_is_admin.ex index 2328876..8ab038a 100644 --- a/lib/mv/authorization/checks/actor_is_admin.ex +++ b/lib/mv/authorization/checks/actor_is_admin.ex @@ -3,20 +3,15 @@ defmodule Mv.Authorization.Checks.ActorIsAdmin do Policy check: true when the actor's role has permission_set_name "admin". Used to restrict actions (e.g. User.update_user for member link/unlink) to admins only. + Delegates to `Mv.Authorization.Actor.admin?/1` for consistency. """ use Ash.Policy.SimpleCheck + alias Mv.Authorization.Actor + @impl true def describe(_opts), do: "actor has admin permission set" @impl true - def match?(nil, _context, _opts), do: false - - def match?(actor, _context, _opts) do - ps_name = - get_in(actor, [Access.key(:role), Access.key(:permission_set_name)]) || - get_in(actor, [Access.key("role"), Access.key("permission_set_name")]) - - ps_name == "admin" - end + def match?(actor, _context, _opts), do: Actor.admin?(actor) end From ad02f8914f2193cefb6f2fe9fda5abc24e6e1665 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 14:35:08 +0100 Subject: [PATCH 19/38] Use EmailSync.Loader.get_linked_user in EmailNotUsedByOtherUser Remove duplicate get_linked_user_id; reuse Loader for linked user lookup. --- .../validations/email_not_used_by_other_user.ex | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index f9fba1b..1ee8ab0 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -8,6 +8,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation + + alias Mv.EmailSync.Loader alias Mv.Helpers require Logger @@ -32,7 +34,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - linked_user_id = get_linked_user_id(changeset.data) + linked_user = Loader.get_linked_user(changeset.data) + linked_user_id = if linked_user, do: linked_user.id, else: nil is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -76,16 +79,4 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, nil), do: query defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) - - defp get_linked_user_id(member_data) do - alias Mv.Helpers.SystemActor - - system_actor = SystemActor.get_system_actor() - opts = Helpers.ash_actor_opts(system_actor) - - case Ash.load(member_data, :user, opts) do - {:ok, %{user: %{id: id}}} -> id - _ -> nil - end - end end From 4ea31f0f37098ece2f10d7a1cf2d89d03bc71492 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 14:35:32 +0100 Subject: [PATCH 20/38] Add email-change permission validation for linked members Only admins or the linked user may change a linked member's email. - New validation EmailChangePermission (uses Actor.admin?, Loader.get_linked_user). - Register on Member update_member; docs and gettext. --- docs/email-sync.md | 1 + lib/membership/member.ex | 4 + .../validations/email_change_permission.ex | 69 +++++ priv/gettext/de/LC_MESSAGES/default.po | 18 +- priv/gettext/default.pot | 5 + priv/gettext/en/LC_MESSAGES/default.po | 18 +- .../member_email_validation_test.exs | 237 ++++++++++++++++++ 7 files changed, 324 insertions(+), 28 deletions(-) create mode 100644 lib/mv/membership/member/validations/email_change_permission.ex create mode 100644 test/mv/membership/member_email_validation_test.exs diff --git a/docs/email-sync.md b/docs/email-sync.md index c191ff4..5675145 100644 --- a/docs/email-sync.md +++ b/docs/email-sync.md @@ -4,6 +4,7 @@ 2. **DB constraints** - Prevent duplicates within same table (users.email, members.email) 3. **Custom validations** - Prevent cross-table conflicts only for linked entities 4. **Sync is bidirectional**: User ↔ Member (but User always wins on link) +5. **Linked member email change** - When a member is linked, only administrators or the linked user may change that member's email (Member resource validation `EmailChangePermission`). This keeps email sync under control and prevents non-admins from changing another user's linked member email. --- diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 7b49c86..8213ecb 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -25,6 +25,7 @@ defmodule Mv.Membership.Member do - Postal code format: exactly 5 digits (German format) - Date validations: join_date not in future, exit_date after join_date - Email uniqueness: prevents conflicts with unlinked users + - Linked member email change: only admins or the linked user may change a linked member's email (see `Mv.Membership.Member.Validations.EmailChangePermission`) ## Full-Text Search Members have a `search_vector` attribute (tsvector) that is automatically @@ -381,6 +382,9 @@ defmodule Mv.Membership.Member do # Validates that member email is not already used by another (unlinked) user validate Mv.Membership.Member.Validations.EmailNotUsedByOtherUser + # Only admins or the linked user may change a linked member's email (prevents breaking sync) + validate Mv.Membership.Member.Validations.EmailChangePermission, on: [:update] + # Prevent linking to a user that already has a member # This validation prevents "stealing" users from other members by checking # if the target user is already linked to a different member diff --git a/lib/mv/membership/member/validations/email_change_permission.ex b/lib/mv/membership/member/validations/email_change_permission.ex new file mode 100644 index 0000000..0a53de1 --- /dev/null +++ b/lib/mv/membership/member/validations/email_change_permission.ex @@ -0,0 +1,69 @@ +defmodule Mv.Membership.Member.Validations.EmailChangePermission do + @moduledoc """ + Validates that only admins or the linked user may change a linked member's email. + + This validation runs on member update when the email attribute is changing. + It allows the change only if: + - The member is not linked to a user, or + - The actor has the admin permission set (via `Mv.Authorization.Actor.admin?/1`), or + - The actor is the user linked to this member (actor.member_id == member.id). + + This prevents non-admins from changing another user's linked member email, + which would sync to that user's account and break email synchronization. + + No system-actor fallback: missing actor is treated as not allowed. + """ + use Ash.Resource.Validation + use Gettext, backend: MvWeb.Gettext, otp_app: :mv + + alias Mv.Authorization.Actor + alias Mv.EmailSync.Loader + + @doc """ + Validates that the actor may change the member's email when the member is linked. + + Only runs when the email attribute is changing (checked inside). Skips when + member is not linked. Allows when actor is admin or owns the linked member. + """ + @impl true + def validate(changeset, _opts, context) do + if Ash.Changeset.changing_attribute?(changeset, :email) do + validate_linked_member_email_change(changeset, context) + else + :ok + end + end + + defp validate_linked_member_email_change(changeset, context) do + linked_user = Loader.get_linked_user(changeset.data) + + if is_nil(linked_user) do + :ok + else + actor = resolve_actor(changeset, context) + member_id = changeset.data.id + + if Actor.admin?(actor) or actor_owns_member?(actor, member_id) do + :ok + else + msg = + dgettext("default", "Only administrators can change email for members linked to users") + + {:error, field: :email, message: msg} + end + end + end + + # Ash stores actor in changeset.context.private.actor; validation context also has .actor + defp resolve_actor(changeset, context) do + get_in(changeset.context || %{}, [:private, :actor]) || + (context && Map.get(context, :actor)) + end + + defp actor_owns_member?(nil, _member_id), do: false + + defp actor_owns_member?(actor, member_id) do + actor_member_id = Map.get(actor, :member_id) || Map.get(actor, "member_id") + actor_member_id == member_id + end +end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 041507b..3f71644 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2298,17 +2298,7 @@ msgstr "Verwende die Namen der Datenfelder als Spaltennamen in der CSV Datei. Da msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." msgstr "Unbekannte Spalte '%{header}' wird ignoriert. Falls dies ein Datenfeld ist, erstellen Sie es in Mila vor dem Import." -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Custom Fields in CSV Import" -#~ msgstr "Benutzerdefinierte Felder" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Individual data fields must be created in Mila before importing. Use the field name as the CSV column header. Unknown custom field columns will be ignored with a warning." -#~ msgstr "Individuelle Datenfelder müssen in Mila erstellt werden, bevor sie importiert werden können. Verwenden Sie den Namen des Datenfeldes als CSV-Spaltenüberschrift. Unbekannte Spaltenüberschriften werden mit einer Warnung ignoriert." - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Manage Custom Fields" -#~ msgstr "Benutzerdefinierte Felder verwalten" +#: lib/mv/membership/member/validations/email_change_permission.ex +#, elixir-autogen, elixir-format +msgid "Only administrators can change email for members linked to users" +msgstr "Nur Administrator*innen können die E-Mail von Mitgliedern ändern, die mit Benutzer*innen verknüpft sind." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 2861f2d..7418c9b 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2298,3 +2298,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." msgstr "" + +#: lib/mv/membership/member/validations/email_change_permission.ex +#, elixir-autogen, elixir-format +msgid "Only administrators can change email for members linked to users" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 3fe9ce3..db00450 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2299,17 +2299,7 @@ msgstr "" msgid "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." msgstr "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Custom Fields in CSV Import" -#~ msgstr "" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Individual data fields must be created in Mila before importing. Use the field name as the CSV column header. Unknown custom field columns will be ignored with a warning." -#~ msgstr "" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Manage Custom Fields" -#~ msgstr "" +#: lib/mv/membership/member/validations/email_change_permission.ex +#, elixir-autogen, elixir-format +msgid "Only administrators can change email for members linked to users" +msgstr "Only administrators can change email for members linked to users" diff --git a/test/mv/membership/member_email_validation_test.exs b/test/mv/membership/member_email_validation_test.exs new file mode 100644 index 0000000..3d2ef68 --- /dev/null +++ b/test/mv/membership/member_email_validation_test.exs @@ -0,0 +1,237 @@ +defmodule Mv.Membership.MemberEmailValidationTest do + @moduledoc """ + Tests for Member email-change permission validation. + + When a member is linked to a user, only admins or the linked user may change + that member's email. Unlinked members and non-email updates are unaffected. + """ + use Mv.DataCase, async: false + + alias Mv.Accounts + alias Mv.Authorization + alias Mv.Helpers.SystemActor + alias Mv.Membership + + setup do + system_actor = SystemActor.get_system_actor() + %{actor: system_actor} + end + + defp create_role_with_permission_set(permission_set_name, actor) do + role_name = "Test Role #{permission_set_name} #{System.unique_integer([:positive])}" + + case Authorization.create_role( + %{ + name: role_name, + description: "Test role for #{permission_set_name}", + permission_set_name: permission_set_name + }, + actor: actor + ) do + {:ok, role} -> role + {:error, error} -> raise "Failed to create role: #{inspect(error)}" + end + end + + defp create_user_with_permission_set(permission_set_name, actor) do + role = create_role_with_permission_set(permission_set_name, actor) + + {:ok, user} = + Accounts.User + |> Ash.Changeset.for_create(:register_with_password, %{ + email: "user#{System.unique_integer([:positive])}@example.com", + password: "testpassword123" + }) + |> Ash.create(actor: actor) + + {:ok, user} = + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) + |> Ash.update(actor: actor) + + {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts, actor: actor) + user_with_role + end + + defp create_admin_user(actor) do + create_user_with_permission_set("admin", actor) + end + + defp create_linked_member_for_user(user, actor) do + admin = create_admin_user(actor) + + {:ok, member} = + Membership.create_member( + %{ + first_name: "Linked", + last_name: "Member", + email: "linked#{System.unique_integer([:positive])}@example.com" + }, + actor: admin + ) + + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.force_change_attribute(:member_id, member.id) + |> Ash.update(actor: admin, domain: Mv.Accounts, return_notifications?: false) + + member + end + + defp create_unlinked_member(actor) do + admin = create_admin_user(actor) + + {:ok, member} = + Membership.create_member( + %{ + first_name: "Unlinked", + last_name: "Member", + email: "unlinked#{System.unique_integer([:positive])}@example.com" + }, + actor: admin + ) + + member + end + + describe "unlinked member" do + test "normal_user can update email of unlinked member", %{actor: actor} do + normal_user = create_user_with_permission_set("normal_user", actor) + unlinked_member = create_unlinked_member(actor) + + new_email = "new#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(unlinked_member, %{email: new_email}, actor: normal_user) + + assert updated.email == new_email + end + + test "validation does not block when member has no linked user", %{actor: actor} do + normal_user = create_user_with_permission_set("normal_user", actor) + unlinked_member = create_unlinked_member(actor) + + new_email = "other#{System.unique_integer([:positive])}@example.com" + + assert {:ok, _} = + Membership.update_member(unlinked_member, %{email: new_email}, actor: normal_user) + end + end + + describe "linked member – another user's member" do + test "normal_user cannot update email of another user's linked member", %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + + normal_user_b = create_user_with_permission_set("normal_user", actor) + new_email = "other#{System.unique_integer([:positive])}@example.com" + + assert {:error, %Ash.Error.Invalid{} = error} = + Membership.update_member(linked_member, %{email: new_email}, actor: normal_user_b) + + error_str = Exception.message(error) + assert error_str =~ "administrators" + assert error_str =~ "linked to users" + end + + test "admin can update email of linked member", %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + admin = create_admin_user(actor) + + new_email = "admin_changed#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: admin) + + assert updated.email == new_email + end + end + + describe "linked member – own member" do + test "own_data user can update email of their own linked member", %{actor: actor} do + own_data_user = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(own_data_user, actor) + + {:ok, own_data_user} = + Ash.get(Accounts.User, own_data_user.id, domain: Mv.Accounts, load: [:role], actor: actor) + + {:ok, own_data_user} = + Ash.load(own_data_user, :member, domain: Mv.Accounts, actor: actor) + + new_email = "own_updated#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: own_data_user) + + assert updated.email == new_email + end + + test "normal_user with linked member can update email of that same member", %{actor: actor} do + normal_user = create_user_with_permission_set("normal_user", actor) + linked_member = create_linked_member_for_user(normal_user, actor) + + {:ok, normal_user} = + Ash.get(Accounts.User, normal_user.id, domain: Mv.Accounts, load: [:role], actor: actor) + + {:ok, normal_user} = Ash.load(normal_user, :member, domain: Mv.Accounts, actor: actor) + + new_email = "normal_own#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: normal_user) + + assert updated.email == new_email + end + end + + describe "no-op / other fields" do + test "updating only other attributes on linked member as normal_user does not trigger validation error", + %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + normal_user_b = create_user_with_permission_set("normal_user", actor) + + assert {:ok, updated} = + Membership.update_member(linked_member, %{first_name: "UpdatedName"}, + actor: normal_user_b + ) + + assert updated.first_name == "UpdatedName" + assert updated.email == linked_member.email + end + + test "updating email of linked member as admin succeeds", %{actor: actor} do + user_a = create_user_with_permission_set("own_data", actor) + linked_member = create_linked_member_for_user(user_a, actor) + admin = create_admin_user(actor) + + new_email = "admin_ok#{System.unique_integer([:positive])}@example.com" + + assert {:ok, updated} = + Membership.update_member(linked_member, %{email: new_email}, actor: admin) + + assert updated.email == new_email + end + end + + describe "read_only" do + test "read_only cannot update any member (policy rejects before validation)", %{actor: actor} do + read_only_user = create_user_with_permission_set("read_only", actor) + linked_member = create_linked_member_for_user(read_only_user, actor) + + {:ok, read_only_user} = + Ash.get(Accounts.User, read_only_user.id, + domain: Mv.Accounts, + load: [:role], + actor: actor + ) + + assert {:error, %Ash.Error.Forbidden{}} = + Membership.update_member(linked_member, %{email: "changed@example.com"}, + actor: read_only_user + ) + end + end +end From 4e6b7305b6a92520681793e8d8938e4a300d691a Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 15:00:14 +0100 Subject: [PATCH 21/38] Doc: Loader auth-independent for link checks; email-sync rule rationale --- docs/email-sync.md | 2 +- lib/mv/email_sync/loader.ex | 14 ++++++++------ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/email-sync.md b/docs/email-sync.md index 5675145..2f765f0 100644 --- a/docs/email-sync.md +++ b/docs/email-sync.md @@ -4,7 +4,7 @@ 2. **DB constraints** - Prevent duplicates within same table (users.email, members.email) 3. **Custom validations** - Prevent cross-table conflicts only for linked entities 4. **Sync is bidirectional**: User ↔ Member (but User always wins on link) -5. **Linked member email change** - When a member is linked, only administrators or the linked user may change that member's email (Member resource validation `EmailChangePermission`). This keeps email sync under control and prevents non-admins from changing another user's linked member email. +5. **Linked member email change** - When a member is linked, only administrators or the linked user may change that member's email (Member resource validation `EmailChangePermission`). Because User.email wins on link and changes sync Member → User, allowing anyone to change a linked member's email would overwrite that user's account email; this rule keeps sync under control. --- diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index 98f85df..31e0468 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -3,13 +3,15 @@ defmodule Mv.EmailSync.Loader do Helper functions for loading linked records in email synchronization. Centralizes the logic for retrieving related User/Member entities. - ## Authorization + ## Authorization-independent link checks - This module runs systemically and uses the system actor for all operations. - This ensures that email synchronization always works, regardless of user permissions. - - All functions use `Mv.Helpers.SystemActor.get_system_actor/0` to bypass - user permission checks, as email sync is a mandatory side effect. + All functions use the **system actor** for the load. Link existence + (linked vs not linked) is therefore determined **independently of the + current request actor**. This is required so that validations (e.g. + `EmailChangePermission`, `EmailNotUsedByOtherUser`) can correctly decide + "member is linked" even when the current user would not have read permission + on the related User. Using the request actor would otherwise allow + treating a linked member as unlinked and bypass the permission rule. """ alias Mv.Helpers alias Mv.Helpers.SystemActor From 60a418125518d8bbac15c783e746650c958c1a71 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 15:00:20 +0100 Subject: [PATCH 22/38] Validation: error message admin or linked user; resolve_actor fallback --- .../member/validations/email_change_permission.ex | 14 ++++++++++---- priv/gettext/de/LC_MESSAGES/default.po | 6 +++--- priv/gettext/default.pot | 2 +- priv/gettext/en/LC_MESSAGES/default.po | 6 +++--- 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/mv/membership/member/validations/email_change_permission.ex b/lib/mv/membership/member/validations/email_change_permission.ex index 0a53de1..2b1c041 100644 --- a/lib/mv/membership/member/validations/email_change_permission.ex +++ b/lib/mv/membership/member/validations/email_change_permission.ex @@ -11,7 +11,7 @@ defmodule Mv.Membership.Member.Validations.EmailChangePermission do This prevents non-admins from changing another user's linked member email, which would sync to that user's account and break email synchronization. - No system-actor fallback: missing actor is treated as not allowed. + Missing actor is not allowed; the system actor counts as admin (via `Actor.admin?/1`). """ use Ash.Resource.Validation use Gettext, backend: MvWeb.Gettext, otp_app: :mv @@ -47,16 +47,22 @@ defmodule Mv.Membership.Member.Validations.EmailChangePermission do :ok else msg = - dgettext("default", "Only administrators can change email for members linked to users") + dgettext( + "default", + "Only administrators or the linked user can change the email for members linked to users" + ) {:error, field: :email, message: msg} end end end - # Ash stores actor in changeset.context.private.actor; validation context also has .actor + # Ash stores actor in changeset.context.private.actor; validation context has .actor; some callsites use context.actor defp resolve_actor(changeset, context) do - get_in(changeset.context || %{}, [:private, :actor]) || + ctx = changeset.context || %{} + + get_in(ctx, [:private, :actor]) || + Map.get(ctx, :actor) || (context && Map.get(context, :actor)) end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 3f71644..c4fd57d 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2299,6 +2299,6 @@ msgid "Unknown column '%{header}' will be ignored. If this is a custom field, cr msgstr "Unbekannte Spalte '%{header}' wird ignoriert. Falls dies ein Datenfeld ist, erstellen Sie es in Mila vor dem Import." #: lib/mv/membership/member/validations/email_change_permission.ex -#, elixir-autogen, elixir-format -msgid "Only administrators can change email for members linked to users" -msgstr "Nur Administrator*innen können die E-Mail von Mitgliedern ändern, die mit Benutzer*innen verknüpft sind." +#, elixir-autogen, elixir-format, fuzzy +msgid "Only administrators or the linked user can change the email for members linked to users" +msgstr "Nur Administrator*innen oder die verknüpfte Benutzer*in können die E-Mail von Mitgliedern ändern, die mit Benutzer*innen verknüpft sind." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 7418c9b..0908fd8 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2301,5 +2301,5 @@ msgstr "" #: lib/mv/membership/member/validations/email_change_permission.ex #, elixir-autogen, elixir-format -msgid "Only administrators can change email for members linked to users" +msgid "Only administrators or the linked user can change the email for members linked to users" msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index db00450..6faa102 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2300,6 +2300,6 @@ msgid "Unknown column '%{header}' will be ignored. If this is a custom field, cr msgstr "Unknown column '%{header}' will be ignored. If this is a custom field, create it in Mila before importing." #: lib/mv/membership/member/validations/email_change_permission.ex -#, elixir-autogen, elixir-format -msgid "Only administrators can change email for members linked to users" -msgstr "Only administrators can change email for members linked to users" +#, elixir-autogen, elixir-format, fuzzy +msgid "Only administrators or the linked user can change the email for members linked to users" +msgstr "Only administrators or the linked user can change the email for members linked to users" From 47b6a16177c50e593a71c619a8204f1fb11311a0 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 15:00:24 +0100 Subject: [PATCH 23/38] Doc: Actor maybe_load_role comment; ActorIsAdmin system user = admin --- lib/mv/authorization/actor.ex | 2 ++ lib/mv/authorization/checks/actor_is_admin.ex | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/mv/authorization/actor.ex b/lib/mv/authorization/actor.ex index bfc99ed..edc6b8b 100644 --- a/lib/mv/authorization/actor.ex +++ b/lib/mv/authorization/actor.ex @@ -133,6 +133,8 @@ defmodule Mv.Authorization.Actor do SystemActor.system_user?(actor) or permission_set_name(actor) in ["admin", :admin] end + # Load role only when it is nil (e.g. actor from session without role). ensure_loaded/1 + # already handles %Ash.NotLoaded{}, so we do not double-load in the normal Ash path. defp maybe_load_role(%Mv.Accounts.User{role: nil} = user) do case Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) do {:ok, loaded} -> loaded diff --git a/lib/mv/authorization/checks/actor_is_admin.ex b/lib/mv/authorization/checks/actor_is_admin.ex index 8ab038a..413c6c7 100644 --- a/lib/mv/authorization/checks/actor_is_admin.ex +++ b/lib/mv/authorization/checks/actor_is_admin.ex @@ -1,9 +1,10 @@ defmodule Mv.Authorization.Checks.ActorIsAdmin do @moduledoc """ - Policy check: true when the actor's role has permission_set_name "admin". + Policy check: true when the actor is the system user or has permission_set_name "admin". Used to restrict actions (e.g. User.update_user for member link/unlink) to admins only. - Delegates to `Mv.Authorization.Actor.admin?/1` for consistency. + Delegates to `Mv.Authorization.Actor.admin?/1`, which returns true for the system actor + or for a user whose role has permission_set_name "admin". """ use Ash.Policy.SimpleCheck From 131904f1720a2f82e9bd824bf5ea4ddd2d03e486 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 15:00:27 +0100 Subject: [PATCH 24/38] Test: assert on error field :email instead of message string --- test/mv/membership/member_email_validation_test.exs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/mv/membership/member_email_validation_test.exs b/test/mv/membership/member_email_validation_test.exs index 3d2ef68..d1b5a10 100644 --- a/test/mv/membership/member_email_validation_test.exs +++ b/test/mv/membership/member_email_validation_test.exs @@ -130,9 +130,8 @@ defmodule Mv.Membership.MemberEmailValidationTest do assert {:error, %Ash.Error.Invalid{} = error} = Membership.update_member(linked_member, %{email: new_email}, actor: normal_user_b) - error_str = Exception.message(error) - assert error_str =~ "administrators" - assert error_str =~ "linked to users" + assert Enum.any?(error.errors, &(&1.field == :email)), + "expected an error for field :email, got: #{inspect(error.errors)}" end test "admin can update email of linked member", %{actor: actor} do From 505e31653a64a8bfb17fcec090ab00ec8582afdf Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 16:35:29 +0100 Subject: [PATCH 25/38] Apply UI authorization to Member LiveViews (Index and Show) Gate New Member button, Edit and Delete links with can?/3. Edit button on Member Show visible only when user can update the member. --- lib/mv_web/live/member_live/index.html.heex | 26 +++++++++++++-------- lib/mv_web/live/member_live/show.ex | 8 ++++--- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/mv_web/live/member_live/index.html.heex b/lib/mv_web/live/member_live/index.html.heex index 394db2c..c44f3a3 100644 --- a/lib/mv_web/live/member_live/index.html.heex +++ b/lib/mv_web/live/member_live/index.html.heex @@ -23,9 +23,11 @@ <.icon name="hero-envelope" /> {gettext("Open in email program")} - <.button variant="primary" navigate={~p"/members/new"}> - <.icon name="hero-plus" /> {gettext("New Member")} - + <%= if can?(@current_user, :create, Mv.Membership.Member) do %> + <.button variant="primary" navigate={~p"/members/new"}> + <.icon name="hero-plus" /> {gettext("New Member")} + + <% end %> @@ -297,16 +299,20 @@ <.link navigate={~p"/members/#{member}"}>{gettext("Show")}

- <.link navigate={~p"/members/#{member}/edit"}>{gettext("Edit")} + <%= if can?(@current_user, :update, member) do %> + <.link navigate={~p"/members/#{member}/edit"}>{gettext("Edit")} + <% end %> <:action :let={member}> - <.link - phx-click={JS.push("delete", value: %{id: member.id}) |> hide("#row-#{member.id}")} - data-confirm={gettext("Are you sure?")} - > - {gettext("Delete")} - + <%= if can?(@current_user, :destroy, member) do %> + <.link + phx-click={JS.push("delete", value: %{id: member.id}) |> hide("#row-#{member.id}")} + data-confirm={gettext("Are you sure?")} + > + {gettext("Delete")} + + <% end %> diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex index d484672..9ac1fc8 100644 --- a/lib/mv_web/live/member_live/show.ex +++ b/lib/mv_web/live/member_live/show.ex @@ -39,9 +39,11 @@ defmodule MvWeb.MemberLive.Show do {MvWeb.Helpers.MemberHelpers.display_name(@member)} - <.button variant="primary" navigate={~p"/members/#{@member}/edit?return_to=show"}> - {gettext("Edit Member")} - + <%= if can?(@current_user, :update, @member) do %> + <.button variant="primary" navigate={~p"/members/#{@member}/edit?return_to=show"}> + {gettext("Edit Member")} + + <% end %>
<%!-- Tab Navigation --%> From 5e361ba4006f2d9cf776eb9ab7992a68b211fdc6 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 16:35:30 +0100 Subject: [PATCH 26/38] Add Member LiveView authorization tests Covers read_only, normal_user, admin, own_data for Index and Show. Asserts New Member / Edit / Delete visibility and redirect for Mitglied. --- .../live/member_live_authorization_test.exs | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 test/mv_web/live/member_live_authorization_test.exs diff --git a/test/mv_web/live/member_live_authorization_test.exs b/test/mv_web/live/member_live_authorization_test.exs new file mode 100644 index 0000000..c8d02b8 --- /dev/null +++ b/test/mv_web/live/member_live_authorization_test.exs @@ -0,0 +1,106 @@ +defmodule MvWeb.MemberLiveAuthorizationTest do + @moduledoc """ + Tests for UI authorization on Member LiveViews (Index and Show). + """ + use MvWeb.ConnCase, async: false + + import Phoenix.LiveViewTest + + alias Mv.Fixtures + + # Use literal strings for button/link text (matches default Gettext locale) + @new_member_text "New Member" + @edit_member_text "Edit Member" + + describe "Member Index - Vorstand (read_only)" do + @tag role: :read_only + test "sees member list but not New Member button", %{conn: conn} do + _member = Fixtures.member_fixture() + + {:ok, _view, html} = live(conn, "/members") + + refute html =~ @new_member_text + end + + @tag role: :read_only + test "does not see Edit or Delete buttons in table", %{conn: conn} do + member = Fixtures.member_fixture() + + {:ok, view, _html} = live(conn, "/members") + + refute has_element?(view, "a[href=\"/members/#{member.id}/edit\"]") + refute has_element?(view, "a[phx-click*='delete']") + end + end + + describe "Member Index - Kassenwart (normal_user)" do + @tag role: :normal_user + test "sees New Member and Edit buttons", %{conn: conn} do + member = Fixtures.member_fixture() + + {:ok, view, html} = live(conn, "/members") + + assert html =~ @new_member_text + assert has_element?(view, "a[href=\"/members/#{member.id}/edit\"]") + end + + @tag role: :normal_user + test "does not see Delete button", %{conn: conn} do + _member = Fixtures.member_fixture() + + {:ok, view, _html} = live(conn, "/members") + + refute has_element?(view, "a[phx-click*='delete']") + end + end + + describe "Member Index - Admin" do + @tag role: :admin + test "sees New Member, Edit and Delete buttons", %{conn: conn} do + member = Fixtures.member_fixture() + + {:ok, view, html} = live(conn, "/members") + + assert html =~ @new_member_text + assert has_element?(view, "a[href=\"/members/#{member.id}/edit\"]") + assert has_element?(view, "a[phx-click*='delete']") + end + end + + describe "Member Index - Mitglied (own_data)" do + @tag role: :member + test "is redirected when accessing /members", %{conn: conn, current_user: user} do + assert {:error, {:redirect, %{to: to}}} = live(conn, "/members") + assert to == "/users/#{user.id}" + end + end + + describe "Member Show - Edit button visibility" do + @tag role: :admin + test "admin sees Edit button", %{conn: conn} do + member = Fixtures.member_fixture() + + {:ok, _view, html} = live(conn, "/members/#{member.id}") + + assert html =~ @edit_member_text + end + + @tag role: :read_only + test "read_only does not see Edit button", %{conn: conn} do + member = Fixtures.member_fixture() + + {:ok, _view, html} = live(conn, "/members/#{member.id}") + + refute html =~ @edit_member_text + end + + @tag role: :normal_user + test "normal_user sees Edit button", %{conn: conn} do + member = Fixtures.member_fixture() + + {:ok, _view, html} = live(conn, "/members/#{member.id}") + + assert html =~ @edit_member_text + end + end +end From 2f67c7099d0b6dfb5a9d5443ddf76644683a7b4d Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 16:35:32 +0100 Subject: [PATCH 27/38] Apply UI authorization to User LiveViews (Index and Show) Gate New User button, Edit and Delete links with can?/3. Edit button on User Show visible only when user can update the user. --- lib/mv_web/live/user_live/index.html.heex | 26 ++++++++++++++--------- lib/mv_web/live/user_live/show.ex | 8 ++++--- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/lib/mv_web/live/user_live/index.html.heex b/lib/mv_web/live/user_live/index.html.heex index 9314f1e..dcb2e83 100644 --- a/lib/mv_web/live/user_live/index.html.heex +++ b/lib/mv_web/live/user_live/index.html.heex @@ -2,9 +2,11 @@ <.header> {gettext("Listing Users")} <:actions> - <.button variant="primary" navigate={~p"/users/new"}> - <.icon name="hero-plus" /> {gettext("New User")} - + <%= if can?(@current_user, :create, Mv.Accounts.User) do %> + <.button variant="primary" navigate={~p"/users/new"}> + <.icon name="hero-plus" /> {gettext("New User")} + + <% end %> @@ -62,16 +64,20 @@ <.link navigate={~p"/users/#{user}"}>{gettext("Show")}
- <.link navigate={~p"/users/#{user}/edit"}>{gettext("Edit")} + <%= if can?(@current_user, :update, user) do %> + <.link navigate={~p"/users/#{user}/edit"}>{gettext("Edit")} + <% end %> <:action :let={user}> - <.link - phx-click={JS.push("delete", value: %{id: user.id}) |> hide("#row-#{user.id}")} - data-confirm={gettext("Are you sure?")} - > - {gettext("Delete")} - + <%= if can?(@current_user, :destroy, user) do %> + <.link + phx-click={JS.push("delete", value: %{id: user.id}) |> hide("#row-#{user.id}")} + data-confirm={gettext("Are you sure?")} + > + {gettext("Delete")} + + <% end %> diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index e961d84..fa4f186 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -41,9 +41,11 @@ defmodule MvWeb.UserLive.Show do <.icon name="hero-arrow-left" /> {gettext("Back to users list")} - <.button variant="primary" navigate={~p"/users/#{@user}/edit?return_to=show"}> - <.icon name="hero-pencil-square" /> {gettext("Edit User")} - + <%= if can?(@current_user, :update, @user) do %> + <.button variant="primary" navigate={~p"/users/#{@user}/edit?return_to=show"}> + <.icon name="hero-pencil-square" /> {gettext("Edit User")} + + <% end %> From cc9e530d8049505b26724704db5350e44ea1cb01 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 16:35:33 +0100 Subject: [PATCH 28/38] Add User LiveView authorization tests Covers admin, read_only, member, normal_user for Index and Show. Asserts New User / Edit / Delete visibility and redirect for non-admin. --- .../live/user_live_authorization_test.exs | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 test/mv_web/live/user_live_authorization_test.exs diff --git a/test/mv_web/live/user_live_authorization_test.exs b/test/mv_web/live/user_live_authorization_test.exs new file mode 100644 index 0000000..9c35d87 --- /dev/null +++ b/test/mv_web/live/user_live_authorization_test.exs @@ -0,0 +1,84 @@ +defmodule MvWeb.UserLiveAuthorizationTest do + @moduledoc """ + Tests for UI authorization on User LiveViews (Index and Show). + """ + use MvWeb.ConnCase, async: false + + import Phoenix.LiveViewTest + + alias Mv.Fixtures + + @new_user_text "New User" + @edit_user_text "Edit User" + + describe "User Index - Admin" do + @tag role: :admin + test "sees New User, Edit and Delete buttons", %{conn: conn} do + user = Fixtures.user_with_role_fixture("admin") + + {:ok, view, html} = live(conn, "/users") + + assert html =~ @new_user_text + assert has_element?(view, "a[href=\"/users/#{user.id}/edit\"]") + assert has_element?(view, "a[phx-click*='delete']") + end + end + + describe "User Index - Non-Admin is redirected" do + @tag role: :read_only + test "read_only is redirected when accessing /users", %{conn: conn, current_user: user} do + assert {:error, {:redirect, %{to: to}}} = live(conn, "/users") + assert to == "/users/#{user.id}" + end + + @tag role: :member + test "member is redirected when accessing /users", %{conn: conn, current_user: user} do + assert {:error, {:redirect, %{to: to}}} = live(conn, "/users") + assert to == "/users/#{user.id}" + end + + @tag role: :normal_user + test "normal_user is redirected when accessing /users", %{conn: conn, current_user: user} do + assert {:error, {:redirect, %{to: to}}} = live(conn, "/users") + assert to == "/users/#{user.id}" + end + end + + describe "User Show - own profile" do + @tag role: :member + test "member sees Edit button on own profile", %{conn: conn, current_user: user} do + {:ok, _view, html} = live(conn, "/users/#{user.id}") + + assert html =~ @edit_user_text + end + + @tag role: :read_only + test "read_only sees Edit button on own profile", %{conn: conn, current_user: user} do + {:ok, _view, html} = live(conn, "/users/#{user.id}") + + assert html =~ @edit_user_text + end + + @tag role: :admin + test "admin sees Edit button on user show", %{conn: conn} do + user = Fixtures.user_with_role_fixture("read_only") + + {:ok, _view, html} = live(conn, "/users/#{user.id}") + + assert html =~ @edit_user_text + end + end + + describe "User Show - other user (non-admin redirected)" do + @tag role: :member + test "member is redirected when accessing other user's profile", %{ + conn: conn, + current_user: current_user + } do + other_user = Fixtures.user_with_role_fixture("admin") + + assert {:error, {:redirect, %{to: to}}} = live(conn, "/users/#{other_user.id}") + assert to == "/users/#{current_user.id}" + end + end +end From f779fd61e054f2862453e87049e373377ead1979 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 16:35:35 +0100 Subject: [PATCH 29/38] Gate sidebar menu items by can_access_page? Members, Fee Types and Administration subitems only shown when user has page permission. Add admin_menu_visible? helper. Sidebar test uses admin user so menu items render. --- lib/mv_web/components/layouts/sidebar.ex | 67 +++++++++++++------ .../components/layouts/sidebar_test.exs | 7 +- 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/lib/mv_web/components/layouts/sidebar.ex b/lib/mv_web/components/layouts/sidebar.ex index 1d564c1..19f5547 100644 --- a/lib/mv_web/components/layouts/sidebar.ex +++ b/lib/mv_web/components/layouts/sidebar.ex @@ -70,33 +70,56 @@ defmodule MvWeb.Layouts.Sidebar do defp sidebar_menu(assigns) do ~H""" """ end + defp admin_menu_visible?(user) do + Enum.any?(admin_page_paths(), &can_access_page?(user, &1)) + end + + defp admin_page_paths do + ["/users", "/groups", "/admin/roles", "/membership_fee_settings", "/settings"] + end + attr :href, :string, required: true, doc: "Navigation path" attr :icon, :string, required: true, doc: "Heroicon name" attr :label, :string, required: true, doc: "Menu item label" diff --git a/test/mv_web/components/layouts/sidebar_test.exs b/test/mv_web/components/layouts/sidebar_test.exs index 75727e3..0975b8f 100644 --- a/test/mv_web/components/layouts/sidebar_test.exs +++ b/test/mv_web/components/layouts/sidebar_test.exs @@ -22,9 +22,14 @@ defmodule MvWeb.Layouts.SidebarTest do # ============================================================================= # Returns assigns for an authenticated user with all required attributes. + # User has admin role so can_access_page? returns true for all sidebar links. defp authenticated_assigns(mobile \\ false) do %{ - current_user: %{id: "user-123", email: "test@example.com"}, + current_user: %{ + id: "user-123", + email: "test@example.com", + role: %{permission_set_name: "admin"} + }, club_name: "Test Club", mobile: mobile } From 1426ef1d38575b385454e210fc182c8d58f4b05f Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 16:35:36 +0100 Subject: [PATCH 30/38] Add sidebar authorization tests Assert menu visibility per role: admin, read_only, normal_user, own_data, nil user, user without role. --- .../components/sidebar_authorization_test.exs | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 test/mv_web/components/sidebar_authorization_test.exs diff --git a/test/mv_web/components/sidebar_authorization_test.exs b/test/mv_web/components/sidebar_authorization_test.exs new file mode 100644 index 0000000..234f7cb --- /dev/null +++ b/test/mv_web/components/sidebar_authorization_test.exs @@ -0,0 +1,120 @@ +defmodule MvWeb.SidebarAuthorizationTest do + @moduledoc """ + Tests for sidebar menu visibility based on user permissions (can_access_page?). + """ + use MvWeb.ConnCase, async: false + + import Phoenix.LiveViewTest + import MvWeb.Layouts.Sidebar + + alias Mv.Fixtures + + defp render_sidebar(assigns) do + render_component(&sidebar/1, assigns) + end + + defp sidebar_assigns(current_user, opts \\ []) do + mobile = Keyword.get(opts, :mobile, false) + club_name = Keyword.get(opts, :club_name, "Test Club") + + %{ + current_user: current_user, + club_name: club_name, + mobile: mobile + } + end + + describe "sidebar menu with admin user" do + test "shows Members, Fee Types and Administration with all subitems" do + user = Fixtures.user_with_role_fixture("admin") + html = render_sidebar(sidebar_assigns(user)) + + assert html =~ ~s(href="/members") + assert html =~ ~s(href="/membership_fee_types") + assert html =~ ~s(aria-label="Administration") + assert html =~ ~s(href="/users") + assert html =~ ~s(href="/groups") + assert html =~ ~s(href="/admin/roles") + assert html =~ ~s(href="/membership_fee_settings") + assert html =~ ~s(href="/settings") + end + end + + describe "sidebar menu with read_only user (Vorstand/Buchhaltung)" do + test "shows Members and Groups (from Administration)" do + user = Fixtures.user_with_role_fixture("read_only") + html = render_sidebar(sidebar_assigns(user)) + + assert html =~ ~s(href="/members") + assert html =~ ~s(href="/groups") + end + + test "does not show Fee Types, Users, Roles or Settings" do + user = Fixtures.user_with_role_fixture("read_only") + html = render_sidebar(sidebar_assigns(user)) + + refute html =~ ~s(href="/membership_fee_types") + refute html =~ ~s(href="/users") + refute html =~ ~s(href="/admin/roles") + refute html =~ ~s(href="/settings") + end + end + + describe "sidebar menu with normal_user (Kassenwart)" do + test "shows Members and Groups" do + user = Fixtures.user_with_role_fixture("normal_user") + html = render_sidebar(sidebar_assigns(user)) + + assert html =~ ~s(href="/members") + assert html =~ ~s(href="/groups") + end + + test "does not show Fee Types, Users, Roles or Settings" do + user = Fixtures.user_with_role_fixture("normal_user") + html = render_sidebar(sidebar_assigns(user)) + + refute html =~ ~s(href="/membership_fee_types") + refute html =~ ~s(href="/users") + refute html =~ ~s(href="/admin/roles") + refute html =~ ~s(href="/settings") + end + end + + describe "sidebar menu with own_data user (Mitglied)" do + test "does not show Members link (no /members page access)" do + user = Fixtures.user_with_role_fixture("own_data") + html = render_sidebar(sidebar_assigns(user)) + + refute html =~ ~s(href="/members") + end + + test "does not show Fee Types or Administration" do + user = Fixtures.user_with_role_fixture("own_data") + html = render_sidebar(sidebar_assigns(user)) + + refute html =~ ~s(href="/membership_fee_types") + refute html =~ ~s(href="/users") + refute html =~ ~s(aria-label="Administration") + end + end + + describe "sidebar with nil current_user" do + test "does not render menu items (only header and footer when present)" do + html = render_sidebar(sidebar_assigns(nil)) + + refute html =~ ~s(role="menubar") + refute html =~ ~s(href="/members") + end + end + + describe "sidebar with user without role" do + test "does not show any navigation links" do + user = %{id: "user-no-role", email: "noreply@test.com", role: nil} + html = render_sidebar(sidebar_assigns(user)) + + refute html =~ ~s(href="/members") + refute html =~ ~s(href="/membership_fee_types") + refute html =~ ~s(href="/users") + end + end +end From 9e8910344e0389fb2fa090ba792d77d0853e1fe9 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 17:16:07 +0100 Subject: [PATCH 31/38] Add MvWeb.PagePaths for central sidebar/page paths Single source for path strings used by Sidebar and can_access_page?. Keep in sync with router when routes change. --- lib/mv_web/page_paths.ex | 42 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 lib/mv_web/page_paths.ex diff --git a/lib/mv_web/page_paths.ex b/lib/mv_web/page_paths.ex new file mode 100644 index 0000000..5606c76 --- /dev/null +++ b/lib/mv_web/page_paths.ex @@ -0,0 +1,42 @@ +defmodule MvWeb.PagePaths do + @moduledoc """ + Central path strings for UI authorization and sidebar menu. + + Keep in sync with `MvWeb.Router`. Used by Sidebar and `can_access_page?/2` + so route changes (prefix, rename) are updated in one place. + """ + + # Sidebar top-level menu paths + @members "/members" + @membership_fee_types "/membership_fee_types" + + # Administration submenu paths (all must match router) + @users "/users" + @groups "/groups" + @admin_roles "/admin/roles" + @membership_fee_settings "/membership_fee_settings" + @settings "/settings" + + @admin_page_paths [ + @users, + @groups, + @admin_roles, + @membership_fee_settings, + @settings + ] + + @doc "Path for Members index (sidebar and page permission check)." + def members, do: @members + + @doc "Path for Membership Fee Types index (sidebar and page permission check)." + def membership_fee_types, do: @membership_fee_types + + @doc "Paths for Administration menu; show group if user can access any of these." + def admin_menu_paths, do: @admin_page_paths + + def users, do: @users + def groups, do: @groups + def admin_roles, do: @admin_roles + def membership_fee_settings, do: @membership_fee_settings + def settings, do: @settings +end From 2ddd22078dbcc93aabe3e7212d1958b8c0dbb841 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 3 Feb 2026 17:16:08 +0100 Subject: [PATCH 32/38] Sidebar: use PagePaths, add testid for Administration Gate menu items via PagePaths; add data-testid=sidebar-administration for stable tests. menu_group accepts optional testid attr. --- lib/mv_web/components/layouts/sidebar.ex | 33 +++++++++++++----------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/lib/mv_web/components/layouts/sidebar.ex b/lib/mv_web/components/layouts/sidebar.ex index 19f5547..26c0d7a 100644 --- a/lib/mv_web/components/layouts/sidebar.ex +++ b/lib/mv_web/components/layouts/sidebar.ex @@ -4,6 +4,8 @@ defmodule MvWeb.Layouts.Sidebar do """ use MvWeb, :html + alias MvWeb.PagePaths + attr :current_user, :map, default: nil, doc: "The current user" attr :club_name, :string, required: true, doc: "The name of the club" attr :mobile, :boolean, default: false, doc: "Whether this is mobile view" @@ -70,7 +72,7 @@ defmodule MvWeb.Layouts.Sidebar do defp sidebar_menu(assigns) do ~H"""