diff --git a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md b/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md new file mode 100644 index 0000000..eebf419 --- /dev/null +++ b/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md @@ -0,0 +1,318 @@ +--- +name: Code Review Fixes - Membership Fee Features +overview: Umsetzung der validen Code Review Punkte aus beiden Reviews mit Priorisierung nach Kritikalität. Fokus auf Transaktionssicherheit, Code-Qualität, Performance und UX-Verbesserungen. +todos: + - id: fix-after-action-tasks + content: "after_action mit Task.start → after_transaction + Task.Supervisor: Task.Supervisor zu application.ex hinzufügen, after_action Hooks in after_transaction umwandeln, Task.Supervisor.async_nolink verwenden" + status: pending + - id: reduce-code-duplication + content: "Code-Duplikation reduzieren: handle_cycle_generation/2 private Funktion extrahieren, alle drei Stellen (Create, Type Change, Date Change) verwenden" + status: pending + dependencies: + - fix-after-action-tasks + - id: fix-join-date-validation + content: "join_date Validierung: Entweder Validierung wieder hinzufügen (validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0)) oder Dokumentation anpassen" + status: pending + - id: fix-load-cycles-docs + content: "load_cycles_for_members: Entweder Dokumentation korrigieren (ehrlich machen) oder echte Filterung implementieren (z.B. nur letzte 2 Intervalle)" + status: pending + - id: fix-get-current-cycle-sort + content: "get_current_cycle nondeterministisch: Vor List.first() nach cycle_start sortieren (desc) in MembershipFeeHelpers.get_current_cycle" + status: pending + - id: fix-n1-query-member-count + content: "N+1 Query beheben: Aggregate auf MembershipFeeType definieren oder member_count einmalig vorab laden und in assigns cachen" + status: pending + - id: fix-assign-new-stale + content: "assign_new → assign: In MembershipFeesComponent.update/2 immer assign(:cycles, cycles) und assign(:available_fee_types, available_fee_types) setzen" + status: pending + - id: fix-regenerating-flag + content: "@regenerating auf true setzen: Direkt beim Event-Start in handle_event(\"regenerate_cycles\", ...) socket |> assign(:regenerating, true) setzen" + status: pending + - id: fix-create-cycle-parsing + content: "Create-cycle parsing Fix: Decimal.parse explizit behandeln und {:error, :invalid_amount} zurückgeben statt :error" + status: pending + - id: fix-delete-all-atomic + content: "Delete all cycles atomar: Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) statt Enum.map" + status: pending + - id: improve-async-error-handling + content: "Fehlerbehandlung bei async Tasks: Strukturierte Error-Logs mit Context, optional Retry-Mechanismus oder Event-System für Benachrichtigung" + status: pending + - id: improve-format-currency + content: "format_currency Robustheit: Number.Currency verwenden oder robusteres Pattern Matching + Tests für Edge Cases (negative Zahlen, sehr große Zahlen)" + status: pending + - id: add-missing-typespecs + content: "Fehlende Typespecs: @spec für SetDefaultMembershipFeeType.change/3 hinzufügen" + status: pending + - id: fix-race-condition + content: "Potenzielle Race Condition: Prüfen ob Ash doppelte Auslösung verhindert, ggf. Logik anpassen (beide Änderungen in einem Hook zusammenfassen)" + status: pending + - id: extract-magic-values + content: "Magic Numbers/Strings: Application.get_env(:mv, :sql_sandbox, false) in Konstante/Helper extrahieren (z.B. Mv.Config.sql_sandbox?/0)" + status: pending + - id: fix-domain-consistency + content: "Domain-Konsistenz: Überall in MembershipFeesComponent domain: MembershipFees explizit angeben" + status: pending + - id: fix-test-helper + content: "Test-Helper Fix: create_cycle/3 Helper - Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen" + status: pending + - id: fix-date-utc-today-param + content: "Date.utc_today() Parameter: today Parameter durchgeben in get_cycle_status_for_member und Helper-Funktionen" + status: pending + - id: fix-ui-locale-input + content: "UI/Locale Input Fix: type=\"number\" → type=\"text\" + inputmode=\"decimal\" + serverseitig \",\" → \".\" normalisieren" + status: pending + - id: fix-delete-confirmation + content: "Delete-all-Confirmation robuster: String.trim() + case-insensitive Vergleich oder \"type DELETE\" Pattern" + status: pending + - id: fix-warning-state + content: "Warning-State Fix: Bei Decimal.parse(:error) explizit hide_amount_warning(socket) aufrufen" + status: pending + - id: fix-double-toggle + content: "Toggle entfernen: Toggle-Button im Spalten-Header entfernen (nur in Toolbar behalten)" + status: pending + - id: fix-format-consistency + content: "Format-Konsistenz: Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren" + status: pending + dependencies: + - fix-ui-locale-input +--- + +# Code Review Fixes - Membership Fee Features + +## Kritische Probleme (Müssen vor Merge behoben werden) + +### 1. after_action mit Task.start - Transaktionsprobleme + +**Dateien:** `lib/membership/member.ex` (Zeilen 142, 279) + +**Problem:** `Task.start/1` wird innerhalb von `after_action` Hooks verwendet. `after_action` läuft innerhalb der DB-Transaktion, daher: + +- Tasks sehen möglicherweise noch nicht committed state +- Tasks werden auch bei Rollback gestartet +- Keine Supervision → Memory Leaks möglich + +**Lösung:** + +- `after_transaction` Hook verwenden (Ash Best Practice) +- `Task.Supervisor` zum Supervision Tree hinzufügen (`lib/mv/application.ex`) +- `Task.Supervisor.async_nolink/3` statt `Task.start/1` verwenden + +**Betroffene Stellen:** + +- Member Creation (Zeile 116-164) +- Join/Exit Date Change (Zeile 250-301) + +### 2. Code-Duplikation in Cycle-Generation-Logik + +**Datei:** `lib/membership/member.ex` + +**Problem:** Cycle-Generation-Logik ist dreimal dupliziert (Create, Type Change, Date Change) + +**Lösung:** Extrahiere in private Funktion `handle_cycle_generation/2` + +## Wichtige Probleme (Sollten behoben werden) + +### 3. join_date Validierung entfernt, aber Dokumentation behauptet Gegenteil + +**Datei:** `lib/membership/member.ex` (Zeile 27, 516-518) + +**Problem:** Dokumentation sagt "join_date not in future", aber Validierung fehlt + +**Lösung:** Dokumentation anpassen + +### 4. load_cycles_for_members overpromises + +**Datei:** `lib/mv_web/member_live/index/membership_fee_status.ex` (Zeile 36-40) + +**Problem:** Dokumentation sagt "Only loads the relevant cycle per member" und "Filters cycles at database level", aber lädt alle Cycles + +**Lösung:** echte Filterung implementieren (z.B. nur letzte 2 Intervalle) + +### 5. get_current_cycle nondeterministisch + +**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeile 178-182) + +**Problem:** `List.first()` ohne explizite Sortierung → Ergebnis hängt von Reihenfolge ab + +**Lösung:** Vor `List.first()` nach `cycle_start` sortieren (desc) + +### 6. N+1 Query durch get_member_count + +**Datei:** `lib/mv_web/live/membership_fee_type_live/index.ex` (Zeile 134-140) + +**Problem:** `get_member_count/1` wird pro Row aufgerufen → N+1 Query + +**Lösung:** Aggregate auf MembershipFeeType definieren oder einmalig vorab laden + +### 7. assign_new kann stale werden + +**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 402-403) + +**Problem:** `assign_new(:cycles, ...)` und `assign_new(:available_fee_types, ...)` werden nur gesetzt, wenn Assign noch nicht existiert + +**Lösung:** In `update/2` immer `assign(:cycles, cycles)` / `assign(:available_fee_types, available_fee_types)` setzen + +### 8. @regenerating wird nie auf true gesetzt + +**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 526-561) + +**Problem:** `regenerating` wird nur auf `false` gesetzt, nie auf `true` → Button/Spinner werden nie disabled + +**Lösung:** Direkt beim Event-Start `socket |> assign(:regenerating, true)` setzen + +### 9. Create-cycle parsing: invalid amount zeigt falsche Fehlermeldung + +**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 748-812) + +**Problem:** `Decimal.parse/1` gibt `:error` zurück, aber `with` behandelt es als `:error` → landet in "Invalid date format" Branch + +**Lösung:** Explizit `{:error, :invalid_amount}` zurückgeben: + +```elixir +amount = case Decimal.parse(amount_str) do + {d, _} -> {:ok, d} + :error -> {:error, :invalid_amount} +end +``` + +### 10. Delete all cycles: nicht atomar + +**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 666-714) + +**Problem:** `Enum.map(cycles, &Ash.destroy/1)` → nicht atomar, teilweise gelöscht möglich + +**Lösung:** Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) + +### 11. Fehlerbehandlung bei async Tasks + +**Datei:** `lib/membership/member.ex` + +**Problem:** Bei Fehlern in async Tasks wird nur geloggt, aber der Benutzer erhält keine Rückmeldung. Die Member-Aktion wird als erfolgreich zurückgegeben, auch wenn die Cycle-Generierung fehlschlägt. Keine Retry-Logik oder Monitoring. + +**Lösung:** + +- Für kritische Fälle: synchron ausführen oder Retry-Mechanismus implementieren +- Für nicht-kritische Fälle: Event-System für spätere Benachrichtigung +- Strukturierte Error-Logs mit Context +- Optional: Error-Tracking (Sentry, etc.) + +### 12. format_currency Robustheit + +**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeilen 27-51) + +**Problem:** Die Funktion verwendet String-Manipulation für Formatierung. Edge Cases könnten problematisch sein (z.B. sehr große Zahlen, negative Werte). + +**Lösung:** + +- `Number.Currency` oder ähnliche Bibliothek verwenden +- Oder: Robusteres Pattern Matching für Edge Cases +- Tests für Edge Cases hinzufügen (negative Zahlen, sehr große Zahlen) + +### 13. Fehlende Typespecs + +**Datei:** `lib/membership/member/changes/set_default_membership_fee_type.ex` + +**Problem:** Keine `@spec` für die `change/3` Funktion. + +**Lösung:** Typespecs hinzufügen für bessere Dokumentation und Dialyzer-Support. + +### 14. Potenzielle Race Condition + +**Datei:** `lib/membership/member.ex` (Zeile 250-301) + +**Problem:** Wenn `join_date` und `exit_date` gleichzeitig geändert werden, könnte die Cycle-Generierung zweimal ausgelöst werden (einmal pro Änderung). + +**Lösung:** Prüfen, ob Ash dies bereits verhindert, oder Logik anpassen (z.B. beide Änderungen in einem Hook zusammenfassen). + +### 15. Magic Numbers/Strings + +**Problem:** `Application.get_env(:mv, :sql_sandbox, false)` wird mehrfach verwendet. + +**Lösung:** Extrahiere in Konstante oder Helper-Funktion (z.B. `Mv.Config.sql_sandbox?/0`). + +## Mittlere Probleme (Nice-to-have) + +### 16. Inconsistent use of domain + +**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 819-821) + +**Problem:** Einige Actions verwenden `domain: MembershipFees`, andere nicht + +**Lösung:** Konsistent `domain` überall verwenden + +### 17. Tests: create_cycle/3 löscht jedes Mal alle Cycles + +**Datei:** `test/mv_web/member_live/index/membership_fee_status_test.exs` (Zeile 45-52) + +**Problem:** Helper löscht vor jedem Create alle Cycles → Tests prüfen nicht, was sie denken + +**Lösung:** Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen + +### 18. Tests/Design: Date.utc_today() macht Tests flaky + +**Problem:** Tests hängen von `Date.utc_today()` ab → nicht deterministisch + +**Lösung:** `today` Parameter durchgeben (z.B. `get_cycle_status_for_member(member, show_current, today \\ Date.utc_today())`) + +### 19. UI/Locale: input type="number" + Decimal/Komma + +**Problem:** `type="number"` funktioniert nicht zuverlässig mit Komma als Dezimaltrenner + +**Lösung:** `type="text"` + `inputmode="decimal"` + serverseitig "," → "." normalisieren + +### 20. Delete-all-Confirmation: String-Vergleich ist fragil + +**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 296-298) + +**Problem:** String-Vergleich gegen `gettext("Yes")` und `"Yes"` → fragil bei Whitespace/Locale + +**Lösung:** `String.trim()` + case-insensitive Vergleich oder "type DELETE" Pattern + +### 21. MembershipFeeType Form: Warning-State kann hängen bleiben + +**Datei:** `lib/mv_web/live/membership_fee_type_live/form.ex` (Zeile 367-378) + +**Problem:** Bei `Decimal.parse(:error)` wird nur `socket` zurückgegeben → Warning kann stehen bleiben + +**Lösung:** Bei `:error` explizit `hide_amount_warning(socket)` aufrufen + +### 22. UI/UX: Toggle ist doppelt vorhanden + +**Datei:** `lib/mv_web/live/member_live/index.html.heex` (Zeile 45-72, 284-296) + +**Problem:** Toggle-Button sowohl in Toolbar als auch im Spalten-Header + +**Lösung:** Toggle im Spalten-Header entfernen (nur in Toolbar behalten) + +### 23. Konsistenz: format_currency vs Inputs + +**Problem:** `format_currency` formatiert deutsch (Komma), aber Inputs erwarten Punkt + +**Lösung:** Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren + +## Implementierungsreihenfolge + +1. **Kritisch:** after_action → after_transaction + Task.Supervisor +2. **Kritisch:** Code-Duplikation reduzieren +3. **Wichtig:** join_date Validierung/Dokumentation +4. **Wichtig:** load_cycles_for_members Dokumentation/Implementierung +5. **Wichtig:** get_current_cycle Sortierung +6. **Wichtig:** N+1 Query beheben +7. **Wichtig:** assign_new → assign +8. **Wichtig:** @regenerating auf true setzen +9. **Wichtig:** Create-cycle parsing Fix +10. **Wichtig:** Delete all cycles atomar +11. **Wichtig:** Fehlerbehandlung bei async Tasks +12. **Wichtig:** format_currency Robustheit +13. **Wichtig:** Fehlende Typespecs +14. **Wichtig:** Potenzielle Race Condition prüfen/beheben +15. **Wichtig:** Magic Numbers/Strings extrahieren +16. **Mittel:** Domain-Konsistenz +17. **Mittel:** Test-Helper Fix +18. **Mittel:** Date.utc_today() Parameter +19. **Mittel:** UI/Locale Fixes +20. **Mittel:** String-Vergleich robuster +21. **Mittel:** Warning-State Fix +22. **Mittel:** Toggle entfernen +23. **Mittel:** Format-Konsistenz + diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex index f8676a5..c2af0a9 100644 --- a/lib/mv_web/live/member_live/show.ex +++ b/lib/mv_web/live/member_live/show.ex @@ -153,15 +153,14 @@ defmodule MvWeb.MemberLive.Show do <%!-- Custom Fields Section --%> - <%= if Enum.any?(@member.custom_field_values) do %> + <%= if Enum.any?(@custom_fields) do %>
<.section_box title={gettext("Custom Fields")}>
- <%= for cfv <- sort_custom_field_values(@member.custom_field_values) do %> - <% custom_field = cfv.custom_field %> - <% value_type = custom_field && custom_field.value_type %> - <.data_field label={custom_field && custom_field.name}> - {format_custom_field_value(cfv.value, value_type)} + <%= for custom_field <- @custom_fields do %> + <% cfv = find_custom_field_value(@member.custom_field_values, custom_field.id) %> + <.data_field label={custom_field.name}> + {format_custom_field_value(cfv, custom_field.value_type)} <% end %>
@@ -239,6 +238,14 @@ defmodule MvWeb.MemberLive.Show do @impl true def handle_params(%{"id" => id}, _, socket) do + # Load custom fields once using assign_new to avoid repeated queries + socket = + assign_new(socket, :custom_fields, fn -> + Mv.Membership.CustomField + |> Ash.Query.sort(name: :asc) + |> Ash.read!() + end) + query = Mv.Membership.Member |> filter(id == ^id) @@ -318,12 +325,35 @@ defmodule MvWeb.MemberLive.Show do """ end + # Renders a mailto link if email is present, otherwise renders empty value placeholder + attr :email, :string, required: true + attr :display, :string, default: nil + + defp mailto_link(assigns) do + display_text = assigns.display || assigns.email + + if assigns.email && String.trim(assigns.email) != "" do + assigns = %{email: assigns.email, display: display_text} + + ~H""" + + {@display} + + """ + else + render_empty_value() + end + end + # ----------------------------------------------------------------- # Helper Functions # ----------------------------------------------------------------- - defp display_value(nil), do: "" - defp display_value(""), do: "" + defp display_value(nil), do: render_empty_value() + defp display_value(""), do: render_empty_value() defp display_value(value), do: value defp format_status_label(:paid), do: gettext("Paid") @@ -373,20 +403,34 @@ defmodule MvWeb.MemberLive.Show do defp format_date(date), do: to_string(date) - # Sorts custom field values by custom field name - defp sort_custom_field_values(custom_field_values) do - Enum.sort_by(custom_field_values, fn cfv -> - (cfv.custom_field && cfv.custom_field.name) || "" + # Finds custom field value for a given custom field id + # Returns the value (not the CustomFieldValue struct) or nil + defp find_custom_field_value(nil, _custom_field_id), do: nil + + defp find_custom_field_value(custom_field_values, custom_field_id) + when is_list(custom_field_values) do + Enum.find_value(custom_field_values, fn cfv -> + if cfv.custom_field_id == custom_field_id or + (cfv.custom_field && cfv.custom_field.id == custom_field_id) do + cfv.value + end end) end + defp find_custom_field_value(_custom_field_values, _custom_field_id), do: nil + # Formats custom field value based on type + # Handles both CustomFieldValue structs and direct values + defp format_custom_field_value(nil, _type), do: render_empty_value() + + defp format_custom_field_value(%Mv.Membership.CustomFieldValue{} = cfv, value_type) do + format_custom_field_value(cfv.value, value_type) + end + defp format_custom_field_value(%Ash.Union{value: value, type: type}, _expected_type) do format_custom_field_value(value, type) end - defp format_custom_field_value(nil, _type), do: "—" - defp format_custom_field_value(value, :boolean) when is_boolean(value) do if value, do: gettext("Yes"), else: gettext("No") end @@ -396,11 +440,15 @@ defmodule MvWeb.MemberLive.Show do end defp format_custom_field_value(value, :email) when is_binary(value) do - assigns = %{email: value} + if String.trim(value) == "" do + render_empty_value() + else + assigns = %{email: value} - ~H""" - {@email} - """ + ~H""" + <.mailto_link email={@email} display={@email} /> + """ + end end defp format_custom_field_value(value, :integer) when is_integer(value) do @@ -408,8 +456,22 @@ defmodule MvWeb.MemberLive.Show do end defp format_custom_field_value(value, _type) when is_binary(value) do - if String.trim(value) == "", do: "—", else: value + if String.trim(value) == "", do: render_empty_value(), else: value end defp format_custom_field_value(value, _type), do: to_string(value) + + # Renders accessible placeholder for empty values + # Uses translated text for screen readers while maintaining visual consistency + # The visual "—" is hidden from screen readers, while the translated text is only visible to screen readers + defp render_empty_value do + assigns = %{text: gettext("Not set")} + + ~H""" + + + {@text} + + """ + end end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 4d87ac7..6c6bcef 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1675,16 +1675,6 @@ msgstr "Wählen Sie eine Mitgliedsbeitragsart für dieses Mitglied. Mitglieder k msgid "Select interval" msgstr "Intervall auswählen" -#: lib/mv_web/live/member_live/index.html.heex -#, elixir-autogen, elixir-format -msgid "Switch to current cycle" -msgstr "Zum aktuellen Zyklus wechseln" - -#: lib/mv_web/live/member_live/index.html.heex -#, elixir-autogen, elixir-format -msgid "Switch to last completed cycle" -msgstr "Zum letzten abgeschlossenen Zyklus wechseln" - #: lib/mv_web/live/member_live/show.ex #, elixir-autogen, elixir-format msgid "Type" @@ -1761,11 +1751,6 @@ msgstr "Zyklus löschen" msgid "Delete cycle" msgstr "Zyklus löschen" -#: lib/mv_web/live/member_live/show/membership_fees_component.ex -#, elixir-autogen, elixir-format, fuzzy -msgid "Failed to delete some cycles: %{errors}" -msgstr "Konnte Feld nicht löschen: %{error}" - #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy msgid "Invalid date format" @@ -1821,6 +1806,21 @@ msgstr "" msgid "Edit membership fee type" msgstr "Mitgliedsbeitragsart bearbeiten" +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format +msgid "Confirmation text does not match" +msgstr "" + +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "No cycles to delete" +msgstr "Keine Zyklen" + +#: lib/mv_web/live/member_live/show.ex +#, elixir-autogen, elixir-format +msgid "Not set" +msgstr "Nicht gesetzt" + #~ #: lib/mv_web/live/components/payment_filter_component.ex #~ #, elixir-autogen, elixir-format #~ msgid "All payment statuses" @@ -1863,6 +1863,11 @@ msgstr "Mitgliedsbeitragsart bearbeiten" #~ msgid "Edit amount" #~ msgstr "Betrag bearbeiten" +#~ #: lib/mv_web/live/member_live/show/membership_fees_component.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Failed to delete some cycles: %{errors}" +#~ msgstr "Konnte Feld nicht löschen: %{error}" + #~ #: lib/mv_web/live/custom_field_live/form_component.ex #~ #, elixir-autogen, elixir-format #~ msgid "Immutable" @@ -1883,12 +1888,6 @@ msgstr "Mitgliedsbeitragsart bearbeiten" #~ msgid "Not paid" #~ msgstr "Nicht bezahlt" -#~ #: lib/mv_web/live/user_live/form.ex -#~ #: lib/mv_web/live/user_live/show.ex -#~ #, elixir-autogen, elixir-format -#~ msgid "Not set" -#~ msgstr "Nicht gesetzt" - #~ #: lib/mv_web/live/member_live/show.ex #~ #, elixir-autogen, elixir-format #~ msgid "Payment Cycle" @@ -1919,6 +1918,16 @@ msgstr "Mitgliedsbeitragsart bearbeiten" #~ msgid "Show last completed cycle" #~ msgstr "Letzten abgeschlossenen Zyklus anzeigen" +#~ #: lib/mv_web/live/member_live/index.html.heex +#~ #, elixir-autogen, elixir-format +#~ msgid "Switch to current cycle" +#~ msgstr "Zum aktuellen Zyklus wechseln" + +#~ #: lib/mv_web/live/member_live/index.html.heex +#~ #, elixir-autogen, elixir-format +#~ msgid "Switch to last completed cycle" +#~ msgstr "Zum letzten abgeschlossenen Zyklus wechseln" + #~ #: lib/mv_web/live/member_live/form.ex #~ #: lib/mv_web/live/member_live/show.ex #~ #, elixir-autogen, elixir-format diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 84cd5ec..73094a2 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1816,3 +1816,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "No cycles to delete" msgstr "" + +#: lib/mv_web/live/member_live/show.ex +#, elixir-autogen, elixir-format +msgid "Not set" +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 7ced824..6a6ea13 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1676,16 +1676,6 @@ msgstr "" msgid "Select interval" msgstr "" -#: lib/mv_web/live/member_live/index.html.heex -#, elixir-autogen, elixir-format -msgid "Switch to current cycle" -msgstr "" - -#: lib/mv_web/live/member_live/index.html.heex -#, elixir-autogen, elixir-format -msgid "Switch to last completed cycle" -msgstr "" - #: lib/mv_web/live/member_live/show.ex #, elixir-autogen, elixir-format msgid "Type" @@ -1762,11 +1752,6 @@ msgstr "" msgid "Delete cycle" msgstr "" -#: lib/mv_web/live/member_live/show/membership_fees_component.ex -#, elixir-autogen, elixir-format, fuzzy -msgid "Failed to delete some cycles: %{errors}" -msgstr "" - #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy msgid "Invalid date format" @@ -1822,6 +1807,21 @@ msgstr "" msgid "Edit membership fee type" msgstr "" +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format +msgid "Confirmation text does not match" +msgstr "" + +#: lib/mv_web/live/member_live/show/membership_fees_component.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "No cycles to delete" +msgstr "" + +#: lib/mv_web/live/member_live/show.ex +#, elixir-autogen, elixir-format, fuzzy +msgid "Not set" +msgstr "" + #~ #: lib/mv_web/live/components/payment_filter_component.ex #~ #, elixir-autogen, elixir-format #~ msgid "All payment statuses" @@ -1875,6 +1875,11 @@ msgstr "" #~ msgid "Example: Member Contribution View" #~ msgstr "" +#~ #: lib/mv_web/live/member_live/show/membership_fees_component.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Failed to delete some cycles: %{errors}" +#~ msgstr "" + #~ #: lib/mv_web/live/membership_fee_settings_live.ex #~ #, elixir-autogen, elixir-format #~ msgid "Failed to save settings. Please check the errors below." @@ -1906,11 +1911,6 @@ msgstr "" #~ msgid "Not paid" #~ msgstr "" -#~ #: lib/mv_web/live/user_live/show.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Not set" -#~ msgstr "" - #~ #: lib/mv_web/live/member_live/show.ex #~ #, elixir-autogen, elixir-format, fuzzy #~ msgid "Payment Cycle" @@ -1941,6 +1941,16 @@ msgstr "" #~ msgid "Show last completed cycle" #~ msgstr "" +#~ #: lib/mv_web/live/member_live/index.html.heex +#~ #, elixir-autogen, elixir-format +#~ msgid "Switch to current cycle" +#~ msgstr "" + +#~ #: lib/mv_web/live/member_live/index.html.heex +#~ #, elixir-autogen, elixir-format +#~ msgid "Switch to last completed cycle" +#~ msgstr "" + #~ #: lib/mv_web/live/member_live/form.ex #~ #: lib/mv_web/live/member_live/show.ex #~ #, elixir-autogen, elixir-format diff --git a/test/mv_web/member_live/index_field_visibility_test.exs b/test/mv_web/member_live/index_field_visibility_test.exs index 6e1642a..05fa768 100644 --- a/test/mv_web/member_live/index_field_visibility_test.exs +++ b/test/mv_web/member_live/index_field_visibility_test.exs @@ -10,7 +10,8 @@ defmodule MvWeb.MemberLive.IndexFieldVisibilityTest do - Integration with member list display - Custom fields visibility """ - use MvWeb.ConnCase, async: true + # async: false to prevent PostgreSQL deadlocks when creating members and custom fields + use MvWeb.ConnCase, async: false import Phoenix.LiveViewTest require Ash.Query diff --git a/test/mv_web/member_live/show_test.exs b/test/mv_web/member_live/show_test.exs new file mode 100644 index 0000000..1e04559 --- /dev/null +++ b/test/mv_web/member_live/show_test.exs @@ -0,0 +1,175 @@ +defmodule MvWeb.MemberLive.ShowTest do + @moduledoc """ + Tests for the member show page. + + Tests cover: + - Displaying member information + - Custom Fields section visibility (Issue #282 regression test) + - Custom field values formatting + + ## Note on async: false + Tests use `async: false` (not `async: true`) to prevent PostgreSQL deadlocks + when creating members and custom fields concurrently. This is intentional and + documented here to avoid confusion in commit messages. + """ + # async: false to prevent PostgreSQL deadlocks when creating members and custom fields + use MvWeb.ConnCase, async: false + import Phoenix.LiveViewTest + require Ash.Query + use Gettext, backend: MvWeb.Gettext + + alias Mv.Membership.{CustomField, CustomFieldValue, Member} + + setup do + # Create test member + {:ok, member} = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Alice", + last_name: "Anderson", + email: "alice@example.com" + }) + |> Ash.create() + + %{member: member} + end + + describe "custom fields section visibility (Issue #282)" do + test "displays Custom Fields section even when member has no custom field values", %{ + conn: conn, + member: member + } do + # Create a custom field but no value for the member + {:ok, custom_field} = + CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "phone_mobile", + value_type: :string + }) + |> Ash.create() + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members/#{member}") + + # Custom Fields section should be visible + assert html =~ gettext("Custom Fields") + + # Custom field label should be visible + assert html =~ custom_field.name + + # Value should show placeholder for empty value + assert html =~ "—" or html =~ gettext("Not set") + end + + test "displays Custom Fields section with multiple custom fields, some without values", %{ + conn: conn, + member: member + } do + # Create multiple custom fields + {:ok, field1} = + CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "phone_mobile", + value_type: :string + }) + |> Ash.create() + + {:ok, field2} = + CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "membership_number", + value_type: :integer + }) + |> Ash.create() + + # Create value only for first field + {:ok, _cfv} = + CustomFieldValue + |> Ash.Changeset.for_create(:create, %{ + member_id: member.id, + custom_field_id: field1.id, + value: %{"_union_type" => "string", "_union_value" => "+49123456789"} + }) + |> Ash.create() + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members/#{member}") + + # Custom Fields section should be visible + assert html =~ gettext("Custom Fields") + + # Both field labels should be visible + assert html =~ field1.name + assert html =~ field2.name + + # First field should show value + assert html =~ "+49123456789" + + # Second field should show placeholder + assert html =~ "—" or html =~ gettext("Not set") + end + + test "does not display Custom Fields section when no custom fields exist", %{ + conn: conn, + member: member + } do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members/#{member}") + + # Custom Fields section should NOT be visible + refute html =~ gettext("Custom Fields") + end + end + + describe "custom field value formatting" do + test "formats string custom field values", %{conn: conn, member: member} do + {:ok, custom_field} = + CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "phone_mobile", + value_type: :string + }) + |> Ash.create() + + {:ok, _cfv} = + CustomFieldValue + |> Ash.Changeset.for_create(:create, %{ + member_id: member.id, + custom_field_id: custom_field.id, + value: %{"_union_type" => "string", "_union_value" => "+49123456789"} + }) + |> Ash.create() + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members/#{member}") + + assert html =~ "+49123456789" + end + + test "formats email custom field values as mailto links", %{conn: conn, member: member} do + {:ok, custom_field} = + CustomField + |> Ash.Changeset.for_create(:create, %{ + name: "private_email", + value_type: :email + }) + |> Ash.create() + + {:ok, _cfv} = + CustomFieldValue + |> Ash.Changeset.for_create(:create, %{ + member_id: member.id, + custom_field_id: custom_field.id, + value: %{"_union_type" => "email", "_union_value" => "private@example.com"} + }) + |> Ash.create() + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members/#{member}") + + # Should contain mailto link + assert html =~ ~s(href="mailto:private@example.com") + assert html =~ "private@example.com" + end + end +end diff --git a/test/mv_web/user_live/form_test.exs b/test/mv_web/user_live/form_test.exs index b8f7313..334dedd 100644 --- a/test/mv_web/user_live/form_test.exs +++ b/test/mv_web/user_live/form_test.exs @@ -1,5 +1,6 @@ defmodule MvWeb.UserLive.FormTest do - use MvWeb.ConnCase, async: true + # async: false to prevent PostgreSQL deadlocks when creating members and users + use MvWeb.ConnCase, async: false import Phoenix.LiveViewTest # Helper to setup authenticated connection and live view