diff --git a/lib/mv_web/live/import_export_live.ex b/lib/mv_web/live/import_export_live.ex index f844305..384c39b 100644 --- a/lib/mv_web/live/import_export_live.ex +++ b/lib/mv_web/live/import_export_live.ex @@ -45,6 +45,9 @@ defmodule MvWeb.ImportExportLive do # after this limit is reached. @max_errors 50 + # Maximum length for error messages before truncation + @max_error_message_length 200 + @impl true def mount(_params, session, socket) do # Get locale from session for translations @@ -95,11 +98,11 @@ defmodule MvWeb.ImportExportLive do <%= if Authorization.can?(@current_user, :create, Mv.Membership.Member) do %> <%!-- CSV Import Section --%> <.form_section title={gettext("Import Members (CSV)")}> - <%= import_info_box(assigns) %> - <%= template_links(assigns) %> - <%= import_form(assigns) %> + {import_info_box(assigns)} + {template_links(assigns)} + {import_form(assigns)} <%= if @import_status == :running or @import_status == :done do %> - <%= import_progress(assigns) %> + {import_progress(assigns)} <% end %> @@ -243,7 +246,7 @@ defmodule MvWeb.ImportExportLive do <% end %> <%= if @import_progress.status == :done do %> - <%= import_results(assigns) %> + {import_results(assigns)} <% end %> <% end %> @@ -487,9 +490,7 @@ defmodule MvWeb.ImportExportLive do # Formats Ash validation errors for display defp format_ash_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do - errors - |> Enum.map(&format_single_error/1) - |> Enum.join(", ") + Enum.map_join(errors, ", ", &format_single_error/1) end defp format_ash_error(error) do @@ -498,9 +499,7 @@ defmodule MvWeb.ImportExportLive do # Formats a list of errors into a readable string defp format_error_list(errors) do - errors - |> Enum.map(&format_single_error/1) - |> Enum.join(", ") + Enum.map_join(errors, ", ", &format_single_error/1) end # Formats a single error item @@ -516,8 +515,8 @@ defmodule MvWeb.ImportExportLive do defp format_unknown_error(other) do error_str = inspect(other, limit: :infinity, pretty: true) - if String.length(error_str) > 200 do - String.slice(error_str, 0, 197) <> "..." + if String.length(error_str) > @max_error_message_length do + String.slice(error_str, 0, @max_error_message_length - 3) <> "..." else error_str end @@ -558,6 +557,49 @@ defmodule MvWeb.ImportExportLive do handle_chunk_error(socket, :processing_failed, idx, reason) end + # Processes a chunk with error handling and sends result message to LiveView. + # + # Handles errors from MemberCSV.process_chunk and sends appropriate messages + # to the LiveView process for progress tracking. + @spec process_chunk_with_error_handling( + list(), + map(), + map(), + keyword(), + pid(), + non_neg_integer() + ) :: :ok + defp process_chunk_with_error_handling( + chunk, + column_map, + custom_field_map, + opts, + live_view_pid, + idx + ) do + result = + try do + MemberCSV.process_chunk(chunk, column_map, custom_field_map, opts) + rescue + e -> + {:error, Exception.message(e)} + catch + :exit, reason -> + {:error, inspect(reason)} + + :throw, reason -> + {:error, inspect(reason)} + end + + case result do + {:ok, chunk_result} -> + send(live_view_pid, {:chunk_done, idx, chunk_result}) + + {:error, reason} -> + send(live_view_pid, {:chunk_error, idx, reason}) + end + end + # Starts async task to process a chunk of CSV rows. # # In tests (SQL sandbox mode), runs synchronously to avoid Ecto Sandbox issues. @@ -586,33 +628,16 @@ defmodule MvWeb.ImportExportLive do if Config.sql_sandbox?() do # Run synchronously in tests to avoid Ecto Sandbox issues with async tasks - result = - try do - MemberCSV.process_chunk( - chunk, - import_state.column_map, - import_state.custom_field_map, - opts - ) - rescue - e -> - {:error, Exception.message(e)} - catch - :exit, reason -> - {:error, inspect(reason)} - :throw, reason -> - {:error, inspect(reason)} - end - - case result do - {:ok, chunk_result} -> - # In test mode, send the message - it will be processed when render() is called - # in the test. The test helper wait_for_import_completion() handles message processing - send(live_view_pid, {:chunk_done, idx, chunk_result}) - - {:error, reason} -> - send(live_view_pid, {:chunk_error, idx, reason}) - end + # In test mode, send the message - it will be processed when render() is called + # in the test. The test helper wait_for_import_completion() handles message processing + process_chunk_with_error_handling( + chunk, + import_state.column_map, + import_state.custom_field_map, + opts, + live_view_pid, + idx + ) else # Start async task to process chunk in production # Use start_child for fire-and-forget: no monitor, no Task messages @@ -621,31 +646,14 @@ defmodule MvWeb.ImportExportLive do # Set locale in task process for translations Gettext.put_locale(MvWeb.Gettext, locale) - result = - try do - MemberCSV.process_chunk( - chunk, - import_state.column_map, - import_state.custom_field_map, - opts - ) - rescue - e -> - {:error, Exception.message(e)} - catch - :exit, reason -> - {:error, inspect(reason)} - :throw, reason -> - {:error, inspect(reason)} - end - - case result do - {:ok, chunk_result} -> - send(live_view_pid, {:chunk_done, idx, chunk_result}) - - {:error, reason} -> - send(live_view_pid, {:chunk_error, idx, reason}) - end + process_chunk_with_error_handling( + chunk, + import_state.column_map, + import_state.custom_field_map, + opts, + live_view_pid, + idx + ) end) end @@ -712,8 +720,14 @@ defmodule MvWeb.ImportExportLive do @spec consume_and_read_csv(Phoenix.LiveView.Socket.t()) :: {:ok, String.t()} | {:error, String.t()} defp consume_and_read_csv(socket) do - case consume_uploaded_entries(socket, :csv_file, &read_file_entry/2) do - [{:ok, content}] -> + raw = consume_uploaded_entries(socket, :csv_file, &read_file_entry/2) + + case raw do + [{:ok, content}] when is_binary(content) -> + {:ok, content} + + # Phoenix LiveView test (render_upload) can return raw content list when callback return is treated as value + [content] when is_binary(content) -> {:ok, content} [{:error, reason}] -> diff --git a/test/accounts/user_authentication_test.exs b/test/accounts/user_authentication_test.exs index da84e81..3530dd1 100644 --- a/test/accounts/user_authentication_test.exs +++ b/test/accounts/user_authentication_test.exs @@ -41,18 +41,6 @@ defmodule Mv.Accounts.UserAuthenticationTest do assert is_nil(found_user.oidc_id) end - @tag :test_proposal - test "password authentication uses email as identity_field" do - # Verify the configuration: password strategy should use email as identity_field - # This test checks the AshAuthentication configuration - - strategies = AshAuthentication.Info.authentication_strategies(Mv.Accounts.User) - password_strategy = Enum.find(strategies, fn s -> s.name == :password end) - - assert password_strategy != nil - assert password_strategy.identity_field == :email - end - @tag :test_proposal test "multiple users can exist with different emails" do user1 = diff --git a/test/membership/custom_field_slug_test.exs b/test/membership/custom_field_slug_test.exs index 76ab5c7..aa8e649 100644 --- a/test/membership/custom_field_slug_test.exs +++ b/test/membership/custom_field_slug_test.exs @@ -1,13 +1,14 @@ defmodule Mv.Membership.CustomFieldSlugTest do @moduledoc """ - Tests for automatic slug generation on CustomField resource. + Tests for CustomField slug business rules only. - This test suite verifies: - 1. Slugs are automatically generated from the name attribute - 2. Slugs are unique (cannot have duplicates) - 3. Slugs are immutable (don't change when name changes) - 4. Slugs handle various edge cases (unicode, special chars, etc.) - 5. Slugs can be used for lookups + We test our business logic, not Ash/slugify implementation details: + - Slug is generated from name on create (one smoke test) + - Slug is unique (business rule) + - Slug is immutable (does not change when name is updated; cannot be set manually) + - Slug cannot be empty (rejects name with only special characters) + + We do not test: slugify edge cases (umlauts, truncation, etc.) or Ash/Ecto struct/load behavior. """ use Mv.DataCase, async: true @@ -18,8 +19,8 @@ defmodule Mv.Membership.CustomFieldSlugTest do %{actor: system_actor} end - describe "automatic slug generation on create" do - test "generates slug from name with simple ASCII text", %{actor: actor} do + describe "slug generation (business rule)" do + test "slug is generated from name on create", %{actor: actor} do {:ok, custom_field} = CustomField |> Ash.Changeset.for_create(:create, %{ @@ -30,78 +31,6 @@ defmodule Mv.Membership.CustomFieldSlugTest do assert custom_field.slug == "mobile-phone" end - - test "generates slug from name with German umlauts", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Café Müller", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "cafe-muller" - end - - test "generates slug with lowercase conversion", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "TEST NAME", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "test-name" - end - - test "generates slug by removing special characters", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "E-Mail & Address!", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "e-mail-address" - end - - test "generates slug by replacing multiple spaces with single hyphen", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Multiple Spaces", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "multiple-spaces" - end - - test "trims leading and trailing hyphens", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "-Test-", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "test" - end - - test "handles unicode characters properly (ß becomes ss)", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Straße", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "strasse" - end end describe "slug uniqueness" do @@ -248,29 +177,8 @@ defmodule Mv.Membership.CustomFieldSlugTest do end end - describe "slug edge cases" do - test "handles very long names by truncating slug", %{actor: actor} do - # Create a name at the maximum length (100 chars) - long_name = String.duplicate("abcdefghij", 10) - # 100 characters exactly - - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: long_name, - value_type: :string - }) - |> Ash.create(actor: actor) - - # Slug should be truncated to maximum 100 characters - assert String.length(custom_field.slug) <= 100 - # Should be the full slugified version since name is exactly 100 chars - assert custom_field.slug == long_name - end - + describe "slug cannot be empty (business rule)" do test "rejects name with only special characters", %{actor: actor} do - # When name contains only special characters, slug would be empty - # This should fail validation assert {:error, %Ash.Error.Invalid{} = error} = CustomField |> Ash.Changeset.for_create(:create, %{ @@ -279,107 +187,9 @@ defmodule Mv.Membership.CustomFieldSlugTest do }) |> Ash.create(actor: actor) - # Should fail because slug would be empty error_message = Exception.message(error) assert error_message =~ "Slug cannot be empty" or error_message =~ "is required" end - - test "handles mixed special characters and text", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Test@#$%Name", - value_type: :string - }) - |> Ash.create(actor: actor) - - # slugify keeps the hyphen between words - assert custom_field.slug == "test-name" - end - - test "handles numbers in name", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Field 123 Test", - value_type: :string - }) - |> Ash.create(actor: actor) - - assert custom_field.slug == "field-123-test" - end - - test "handles consecutive hyphens in name", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Test---Name", - value_type: :string - }) - |> Ash.create(actor: actor) - - # Should reduce multiple hyphens to single hyphen - assert custom_field.slug == "test-name" - end - - test "handles name with dots and underscores", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "test.field_name", - value_type: :string - }) - |> Ash.create(actor: actor) - - # Dots and underscores should be handled (either kept or converted) - assert custom_field.slug =~ ~r/^[a-z0-9-]+$/ - end - end - - describe "slug in queries and responses" do - test "slug is included in struct after create", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Test", - value_type: :string - }) - |> Ash.create(actor: actor) - - # Slug should be present in the struct - assert Map.has_key?(custom_field, :slug) - assert custom_field.slug != nil - end - - test "can load custom field and slug is present", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Test", - value_type: :string - }) - |> Ash.create(actor: actor) - - # Load it back - loaded_custom_field = Ash.get!(CustomField, custom_field.id, actor: actor) - - assert loaded_custom_field.slug == "test" - end - - test "slug is returned in list queries", %{actor: actor} do - {:ok, custom_field} = - CustomField - |> Ash.Changeset.for_create(:create, %{ - name: "Test", - value_type: :string - }) - |> Ash.create(actor: actor) - - custom_fields = Ash.read!(CustomField, actor: actor) - - found = Enum.find(custom_fields, &(&1.id == custom_field.id)) - assert found.slug == "test" - end end describe "slug-based lookup (future feature)" do diff --git a/test/membership/group_test.exs b/test/membership/group_test.exs index c51bc66..724d930 100644 --- a/test/membership/group_test.exs +++ b/test/membership/group_test.exs @@ -232,23 +232,7 @@ defmodule Mv.Membership.GroupTest do end describe "Relationships & Deletion" do - test "group has many_to_many members relationship (load with preloading)", %{actor: actor} do - {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) - {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) - - {:ok, _mg} = - Membership.create_member_group(%{member_id: member.id, group_id: group.id}, - actor: actor - ) - - # Load group with members - {:ok, group_with_members} = - Ash.load(group, :members, actor: actor, domain: Mv.Membership) - - assert length(group_with_members.members) == 1 - assert hd(group_with_members.members).id == member.id - end - + # We test business/data rules (CASCADE), not Ash relationship loading (framework). test "delete group cascades to member_groups (members remain intact)", %{actor: actor} do {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) diff --git a/test/membership_fees/membership_fee_type_test.exs b/test/membership_fees/membership_fee_type_test.exs index 80b7839..1194ad8 100644 --- a/test/membership_fees/membership_fee_type_test.exs +++ b/test/membership_fees/membership_fee_type_test.exs @@ -1,6 +1,10 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do @moduledoc """ - Tests for MembershipFeeType resource. + Tests for MembershipFeeType business rules only. + + We test: required fields, allowed interval values, uniqueness, amount constraints, + interval immutability, and referential integrity (cannot delete when in use). + We do not test: standard CRUD (create/update/delete when no constraints apply). """ use Mv.DataCase, async: true @@ -11,34 +15,7 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do %{actor: system_actor} end - describe "create MembershipFeeType" do - test "can create membership fee type with valid attributes", %{actor: actor} do - attrs = %{ - name: "Standard Membership", - amount: Decimal.new("120.00"), - interval: :yearly, - description: "Standard yearly membership fee" - } - - assert {:ok, %MembershipFeeType{} = fee_type} = - Ash.create(MembershipFeeType, attrs, actor: actor) - - assert fee_type.name == "Standard Membership" - assert Decimal.equal?(fee_type.amount, Decimal.new("120.00")) - assert fee_type.interval == :yearly - assert fee_type.description == "Standard yearly membership fee" - end - - test "can create membership fee type without description", %{actor: actor} do - attrs = %{ - name: "Basic", - amount: Decimal.new("60.00"), - interval: :monthly - } - - assert {:ok, %MembershipFeeType{}} = Ash.create(MembershipFeeType, attrs, actor: actor) - end - + describe "create MembershipFeeType - business rules" do test "requires name", %{actor: actor} do attrs = %{ amount: Decimal.new("100.00"), @@ -69,28 +46,24 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do assert error_on_field?(error, :interval) end - test "validates interval enum values - monthly", %{actor: actor} do - attrs = %{name: "Monthly", amount: Decimal.new("10.00"), interval: :monthly} - assert {:ok, fee_type} = Ash.create(MembershipFeeType, attrs, actor: actor) - assert fee_type.interval == :monthly - end + test "accepts valid interval values (monthly, quarterly, half_yearly, yearly)", %{ + actor: actor + } do + for {interval, name} <- [ + monthly: "Monthly", + quarterly: "Quarterly", + half_yearly: "Half Yearly", + yearly: "Yearly" + ] do + attrs = %{ + name: "#{name} #{System.unique_integer([:positive])}", + amount: Decimal.new("10.00"), + interval: interval + } - test "validates interval enum values - quarterly", %{actor: actor} do - attrs = %{name: "Quarterly", amount: Decimal.new("30.00"), interval: :quarterly} - assert {:ok, fee_type} = Ash.create(MembershipFeeType, attrs, actor: actor) - assert fee_type.interval == :quarterly - end - - test "validates interval enum values - half_yearly", %{actor: actor} do - attrs = %{name: "Half Yearly", amount: Decimal.new("60.00"), interval: :half_yearly} - assert {:ok, fee_type} = Ash.create(MembershipFeeType, attrs, actor: actor) - assert fee_type.interval == :half_yearly - end - - test "validates interval enum values - yearly", %{actor: actor} do - attrs = %{name: "Yearly", amount: Decimal.new("120.00"), interval: :yearly} - assert {:ok, fee_type} = Ash.create(MembershipFeeType, attrs, actor: actor) - assert fee_type.interval == :yearly + assert {:ok, fee_type} = Ash.create(MembershipFeeType, attrs, actor: actor) + assert fee_type.interval == interval + end end test "rejects invalid interval values", %{actor: actor} do @@ -128,13 +101,13 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do end end - describe "update MembershipFeeType" do + describe "update MembershipFeeType - business rules" do setup %{actor: actor} do {:ok, fee_type} = Ash.create( MembershipFeeType, %{ - name: "Original Name", + name: "Original Name #{System.unique_integer([:positive])}", amount: Decimal.new("100.00"), interval: :yearly, description: "Original description" @@ -145,28 +118,6 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do %{fee_type: fee_type} end - test "can update name", %{actor: actor, fee_type: fee_type} do - assert {:ok, updated} = Ash.update(fee_type, %{name: "Updated Name"}, actor: actor) - assert updated.name == "Updated Name" - end - - test "can update amount", %{actor: actor, fee_type: fee_type} do - assert {:ok, updated} = Ash.update(fee_type, %{amount: Decimal.new("150.00")}, actor: actor) - assert Decimal.equal?(updated.amount, Decimal.new("150.00")) - end - - test "can update description", %{actor: actor, fee_type: fee_type} do - assert {:ok, updated} = - Ash.update(fee_type, %{description: "Updated description"}, actor: actor) - - assert updated.description == "Updated description" - end - - test "can clear description", %{actor: actor, fee_type: fee_type} do - assert {:ok, updated} = Ash.update(fee_type, %{description: nil}, actor: actor) - assert updated.description == nil - end - test "interval immutability: update fails when interval is changed", %{ actor: actor, fee_type: fee_type @@ -179,7 +130,7 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do end end - describe "delete MembershipFeeType" do + describe "delete MembershipFeeType - business rules (referential integrity)" do setup %{actor: actor} do {:ok, fee_type} = Ash.create( @@ -195,12 +146,6 @@ defmodule Mv.MembershipFees.MembershipFeeTypeTest do %{fee_type: fee_type} end - test "can delete when not in use", %{actor: actor, fee_type: fee_type} do - result = Ash.destroy(fee_type, actor: actor) - # Ash.destroy returns :ok or {:ok, _} depending on version - assert result == :ok or match?({:ok, _}, result) - end - test "cannot delete when members are assigned", %{actor: actor, fee_type: fee_type} do alias Mv.Membership.Member diff --git a/test/mv_web/live/import_export_live_test.exs b/test/mv_web/live/import_export_live_test.exs index 4558ba8..a165ea6 100644 --- a/test/mv_web/live/import_export_live_test.exs +++ b/test/mv_web/live/import_export_live_test.exs @@ -380,18 +380,14 @@ defmodule MvWeb.ImportExportLiveTest do |> form("#csv-upload-form", %{}) |> render_submit() - # Wait a bit for processing to start - Process.sleep(200) + # In test mode chunks run synchronously, so we may already be :done when we check. + # Accept either progress container (if we caught :running) or results panel (if already :done). + _html = render(view) - # Check that import-progress-container exists (with aria-live for accessibility) - assert has_element?(view, "[data-testid='import-progress-container']") + assert has_element?(view, "[data-testid='import-progress-container']") or + has_element?(view, "[data-testid='import-results-panel']") - # Check that progress text is shown when running - html = render(view) - assert has_element?(view, "[data-testid='import-progress-text']") or - html =~ "Processing chunk" - - # Final state should show import-results-panel + # Wait for final state and assert results panel is shown Process.sleep(500) assert has_element?(view, "[data-testid='import-results-panel']") end @@ -552,9 +548,8 @@ defmodule MvWeb.ImportExportLiveTest do assert html =~ "English Template" or html =~ "German Template" or html =~ "English" or html =~ "German" - # Custom Fields section should have descriptive text (Data Field button) - # The component uses "New Data Field" button, not a link - assert html =~ "Data Field" or html =~ "New Data Field" or html =~ "Manage Memberdata" + # Import page has link "Manage Member Data" and info text about "data field" + assert html =~ "Manage Member Data" or html =~ "data field" or html =~ "Data field" end end