Merge branch 'main' into feature/280_membership_fee_ui
All checks were successful
continuous-integration/drone/push Build is passing
All checks were successful
continuous-integration/drone/push Build is passing
This commit is contained in:
commit
0df5d1c0b9
8 changed files with 643 additions and 62 deletions
318
code_review_fixes_-_membership_fee_features_e886dc4b.plan.md
Normal file
318
code_review_fixes_-_membership_fee_features_e886dc4b.plan.md
Normal file
|
|
@ -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
|
||||
|
||||
|
|
@ -153,15 +153,14 @@ defmodule MvWeb.MemberLive.Show do
|
|||
</div>
|
||||
|
||||
<%!-- Custom Fields Section --%>
|
||||
<%= if Enum.any?(@member.custom_field_values) do %>
|
||||
<%= if Enum.any?(@custom_fields) do %>
|
||||
<div>
|
||||
<.section_box title={gettext("Custom Fields")}>
|
||||
<div class="grid grid-cols-2 gap-4">
|
||||
<%= 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)}
|
||||
</.data_field>
|
||||
<% end %>
|
||||
</div>
|
||||
|
|
@ -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"""
|
||||
<a
|
||||
href={"mailto:#{@email}"}
|
||||
class="text-blue-700 hover:text-blue-800 underline"
|
||||
>
|
||||
{@display}
|
||||
</a>
|
||||
"""
|
||||
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,20 +440,38 @@ defmodule MvWeb.MemberLive.Show do
|
|||
end
|
||||
|
||||
defp format_custom_field_value(value, :email) when is_binary(value) do
|
||||
if String.trim(value) == "" do
|
||||
render_empty_value()
|
||||
else
|
||||
assigns = %{email: value}
|
||||
|
||||
~H"""
|
||||
<a href={"mailto:#{@email}"} class="text-blue-700 hover:text-blue-800 underline">{@email}</a>
|
||||
<.mailto_link email={@email} display={@email} />
|
||||
"""
|
||||
end
|
||||
end
|
||||
|
||||
defp format_custom_field_value(value, :integer) when is_integer(value) do
|
||||
Integer.to_string(value)
|
||||
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"""
|
||||
<span class="text-base-content/50 italic">
|
||||
<span aria-hidden="true">—</span>
|
||||
<span class="sr-only">{@text}</span>
|
||||
</span>
|
||||
"""
|
||||
end
|
||||
end
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 ""
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
175
test/mv_web/member_live/show_test.exs
Normal file
175
test/mv_web/member_live/show_test.exs
Normal file
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue