---
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