mitgliederverwaltung/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md

name	overview	todos
Code Review Fixes - Membership Fee Features	Umsetzung der validen Code Review Punkte aus beiden Reviews mit Priorisierung nach Kritikalität. Fokus auf Transaktionssicherheit, Code-Qualität, Performance und UX-Verbesserungen.	id content status fix-after-action-tasks 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 pending id content status dependencies reduce-code-duplication Code-Duplikation reduzieren: handle_cycle_generation/2 private Funktion extrahieren, alle drei Stellen (Create, Type Change, Date Change) verwenden pending fix-after-action-tasks id content status fix-join-date-validation join_date Validierung: Entweder Validierung wieder hinzufügen (validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0)) oder Dokumentation anpassen pending id content status fix-load-cycles-docs load_cycles_for_members: Entweder Dokumentation korrigieren (ehrlich machen) oder echte Filterung implementieren (z.B. nur letzte 2 Intervalle) pending id content status fix-get-current-cycle-sort get_current_cycle nondeterministisch: Vor List.first() nach cycle_start sortieren (desc) in MembershipFeeHelpers.get_current_cycle pending id content status fix-n1-query-member-count N+1 Query beheben: Aggregate auf MembershipFeeType definieren oder member_count einmalig vorab laden und in assigns cachen pending id content status fix-assign-new-stale assign_new → assign: In MembershipFeesComponent.update/2 immer assign(:cycles, cycles) und assign(:available_fee_types, available_fee_types) setzen pending id content status fix-regenerating-flag @regenerating auf true setzen: Direkt beim Event-Start in handle_event("regenerate_cycles", ...) socket |> assign(:regenerating, true) setzen pending id content status fix-create-cycle-parsing Create-cycle parsing Fix: Decimal.parse explizit behandeln und {:error, :invalid_amount} zurückgeben statt :error pending id content status fix-delete-all-atomic Delete all cycles atomar: Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) statt Enum.map pending id content status improve-async-error-handling Fehlerbehandlung bei async Tasks: Strukturierte Error-Logs mit Context, optional Retry-Mechanismus oder Event-System für Benachrichtigung pending id content status improve-format-currency format_currency Robustheit: Number.Currency verwenden oder robusteres Pattern Matching + Tests für Edge Cases (negative Zahlen, sehr große Zahlen) pending id content status add-missing-typespecs Fehlende Typespecs: @spec für SetDefaultMembershipFeeType.change/3 hinzufügen pending id content status fix-race-condition Potenzielle Race Condition: Prüfen ob Ash doppelte Auslösung verhindert, ggf. Logik anpassen (beide Änderungen in einem Hook zusammenfassen) pending id content status extract-magic-values Magic Numbers/Strings: Application.get_env(:mv, :sql_sandbox, false) in Konstante/Helper extrahieren (z.B. Mv.Config.sql_sandbox?/0) pending id content status fix-domain-consistency Domain-Konsistenz: Überall in MembershipFeesComponent domain: MembershipFees explizit angeben pending id content status fix-test-helper Test-Helper Fix: create_cycle/3 Helper - Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen pending id content status fix-date-utc-today-param Date.utc_today() Parameter: today Parameter durchgeben in get_cycle_status_for_member und Helper-Funktionen pending id content status fix-ui-locale-input UI/Locale Input Fix: type="number" → type="text" + inputmode="decimal" + serverseitig "," → "." normalisieren pending id content status fix-delete-confirmation Delete-all-Confirmation robuster: String.trim() + case-insensitive Vergleich oder "type DELETE" Pattern pending id content status fix-warning-state Warning-State Fix: Bei Decimal.parse(:error) explizit hide_amount_warning(socket) aufrufen pending id content status fix-double-toggle Toggle entfernen: Toggle-Button im Spalten-Header entfernen (nur in Toolbar behalten) pending id content status dependencies fix-format-consistency Format-Konsistenz: Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren pending 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:

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