diff --git a/.drone.yml b/.drone.yml index 8c7f325..06db32b 100644 --- a/.drone.yml +++ b/.drone.yml @@ -4,7 +4,7 @@ name: check services: - name: postgres - image: docker.io/library/postgres:17.7 + image: docker.io/library/postgres:18.1 environment: POSTGRES_USER: postgres POSTGRES_PASSWORD: postgres @@ -57,7 +57,7 @@ steps: - mix gettext.extract --check-up-to-date - name: wait_for_postgres - image: docker.io/library/postgres:17.7 + image: docker.io/library/postgres:18.1 commands: # Wait for postgres to become available - | @@ -166,7 +166,7 @@ environment: steps: - name: renovate - image: renovate/renovate:42.44 + image: renovate/renovate:42.71 environment: RENOVATE_CONFIG_FILE: "renovate_backend_config.js" RENOVATE_TOKEN: diff --git a/.gitignore b/.gitignore index 9517a21..058543c 100644 --- a/.gitignore +++ b/.gitignore @@ -44,3 +44,4 @@ npm-debug.log # Docker secrets directory (generated by `just init-secrets`) /secrets/ +notes.md diff --git a/.tool-versions b/.tool-versions index 489262a..275206c 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1,3 +1,3 @@ elixir 1.18.3-otp-27 erlang 27.3.4 -just 1.45.0 +just 1.46.0 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/config/config.exs b/config/config.exs index 053fc19..5fcfcf5 100644 --- a/config/config.exs +++ b/config/config.exs @@ -95,7 +95,16 @@ config :tailwind, # Configures Elixir's Logger config :logger, :default_formatter, format: "$time $metadata[$level] $message\n", - metadata: [:request_id] + metadata: [ + :request_id, + :user_id, + :member_id, + :member_email, + :error, + :error_type, + :cycles_count, + :notifications_count + ] # Use Jason for JSON parsing in Phoenix config :phoenix, :json_library, Jason diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index 5b35e10..1ed863a 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -33,7 +33,7 @@ services: restart: unless-stopped db-prod: - image: postgres:17.7-alpine + image: postgres:18.1-alpine container_name: mv-prod-db environment: POSTGRES_USER: postgres diff --git a/docker-compose.yml b/docker-compose.yml index feff34c..8621603 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,7 +4,7 @@ networks: services: db: - image: postgres:17.7-alpine + image: postgres:18.1-alpine environment: POSTGRES_USER: postgres POSTGRES_PASSWORD: postgres @@ -29,7 +29,7 @@ services: rauthy: container_name: rauthy-dev - image: ghcr.io/sebadob/rauthy:0.33.1 + image: ghcr.io/sebadob/rauthy:0.33.4 environment: - LOCAL_TEST=true - SMTP_URL=mailcrab diff --git a/docs/csv-member-import-v1.md b/docs/csv-member-import-v1.md new file mode 100644 index 0000000..30409b8 --- /dev/null +++ b/docs/csv-member-import-v1.md @@ -0,0 +1,611 @@ +# CSV Member Import v1 - Implementation Plan + +**Version:** 1.0 +**Date:** 2025-01-XX +**Status:** Ready for Implementation +**Related Documents:** +- [Feature Roadmap](./feature-roadmap.md) - Overall feature planning + +--- + +## Table of Contents + +- [Overview & Scope](#overview--scope) +- [UX Flow](#ux-flow) +- [CSV Specification](#csv-specification) +- [Technical Design Notes](#technical-design-notes) +- [Implementation Issues](#implementation-issues) +- [Rollout & Risks](#rollout--risks) + +--- + +## Overview & Scope + +### What We're Building + +A **basic CSV member import feature** that allows administrators to upload a CSV file and import new members into the system. This is a **v1 minimal implementation** focused on establishing the import structure without advanced features. + +**Core Functionality (v1 Minimal):** +- Upload CSV file via LiveView file upload +- Parse CSV with bilingual header support for core member fields (English/German) +- Auto-detect delimiter (`;` or `,`) using header recognition +- Map CSV columns to core member fields (`first_name`, `last_name`, `email`, `phone_number`, `street`, `postal_code`, `city`) +- Validate each row (required fields: `first_name`, `last_name`, `email`) +- Create members via Ash resource (one-by-one, **no background jobs**, processed in chunks of 200 rows via LiveView messages) +- Display import results: success count, error count, and error details +- Provide static CSV templates (EN/DE) + +**Optional Enhancement (v1.1 - Last Issue):** +- Custom field import (if time permits, otherwise defer to v2) + +**Key Constraints (v1):** +- ✅ **Admin-only feature** +- ✅ **No upsert** (create only) +- ✅ **No deduplication** (duplicate emails fail and show as errors) +- ✅ **No mapping wizard** (fixed header mapping via bilingual variants) +- ✅ **No background jobs** (progress via LiveView `handle_info`) +- ✅ **Best-effort import** (row-by-row, no rollback) +- ✅ **UI-only error display** (no error CSV export) +- ✅ **Safety limits** (10 MB, 1,000 rows, chunks of 200) + +### Out of Scope (v1) + +**Deferred to Future Versions:** +- ❌ Upsert/update existing members +- ❌ Advanced deduplication strategies +- ❌ Column mapping wizard UI +- ❌ Background job processing (Oban/GenStage) +- ❌ Transactional all-or-nothing import +- ❌ Error CSV export/download +- ⚠️ Custom field import (optional, last issue - defer to v2 if scope is tight) +- ❌ Batch validation preview before import +- ❌ Date/boolean field parsing +- ❌ Dynamic template generation +- ❌ Import history/audit log +- ❌ Import templates for other entities + +--- + +## UX Flow + +### Access & Location + +**Entry Point:** +- **Location:** Global Settings page (`/settings`) +- **UI Element:** New section "Import Members (CSV)" below "Custom Fields" section +- **Access Control:** Admin-only (enforced at LiveView event level, not entire `/settings` route) + +### User Journey + +1. **Navigate to Global Settings** +2. **Access Import Section** + - Upload area (drag & drop or file picker) + - Template download links (English / German) + - Help text explaining CSV format +3. **Download Template (Optional)** +4. **Prepare CSV File** +5. **Upload CSV** +6. **Start Import** + - Runs server-side via LiveView messages (may take up to ~30 seconds for large files) +7. **View Results** + - Success count + - Error count + - First 50 errors, each with: + - **CSV line number** (header is line 1, first data record begins at line 2) + - Error message + - Field name (if applicable) + +### Error Handling + +- **File too large:** Flash error before upload starts +- **Too many rows:** Flash error before import starts +- **Invalid CSV format:** Error shown in results +- **Partial success:** Results show both success and error counts + +--- + +## CSV Specification + +### Delimiter + +**Recommended:** Semicolon (`;`) +**Supported:** `;` and `,` + +**Auto-Detection (Header Recognition):** +- Remove UTF-8 BOM *first* +- Extract header record and try parsing with both delimiters +- For each delimiter, count how many recognized headers are present (via normalized variants) +- Choose delimiter with higher recognition; prefer `;` if tied +- If neither yields recognized headers, default to `;` + +### Quoting Rules + +- Fields may be quoted with double quotes (`"`) +- Escaped quotes: `""` inside quoted field represents a single `"` +- **v1 assumption:** CSV records do **not** contain embedded newlines inside quoted fields. (If they do, parsing may fail or line numbers may be inaccurate.) + +### Column Headers + +**v1 Supported Fields (Core Member Fields Only):** +- `first_name` / `Vorname` (required) +- `last_name` / `Nachname` (required) +- `email` / `E-Mail` (required) +- `phone_number` / `Telefon` (optional) +- `street` / `Straße` (optional) +- `postal_code` / `PLZ` / `Postleitzahl` (optional) +- `city` / `Stadt` (optional) + +**Member Field Header Mapping:** + +| Canonical Field | English Variants | German Variants | +|---|---|---| +| `first_name` | `first_name`, `firstname` | `Vorname`, `vorname` | +| `last_name` | `last_name`, `lastname`, `surname` | `Nachname`, `nachname`, `Familienname` | +| `email` | `email`, `e-mail`, `e_mail` | `E-Mail`, `e-mail`, `e_mail` | +| `phone_number` | `phone_number`, `phone`, `telephone` | `Telefon`, `telefon` | +| `street` | `street`, `address` | `Straße`, `strasse`, `Strasse` | +| `postal_code` | `postal_code`, `zip`, `postcode` | `PLZ`, `plz`, `Postleitzahl`, `postleitzahl` | +| `city` | `city`, `town` | `Stadt`, `stadt`, `Ort` | + +**Header Normalization (used consistently for both input headers AND mapping variants):** +- Trim whitespace +- Convert to lowercase +- Normalize Unicode: `ß` → `ss` (e.g., `Straße` → `strasse`) +- Replace hyphens/whitespace with underscores: `E-Mail` → `e_mail`, `phone number` → `phone_number` +- Collapse multiple underscores: `e__mail` → `e_mail` +- Case-insensitive matching + +**Unknown columns:** ignored (no error) + +**Required fields:** `first_name`, `last_name`, `email` + +### CSV Template Files + +**Location:** +- `priv/static/templates/member_import_en.csv` +- `priv/static/templates/member_import_de.csv` + +**Content:** +- Header row with required + common optional fields +- One example row +- Uses semicolon delimiter (`;`) +- UTF-8 encoding **with BOM** (Excel compatibility) + +**Template Access:** +- Templates are static files in `priv/static/templates/` +- Served at: + - `/templates/member_import_en.csv` + - `/templates/member_import_de.csv` +- In LiveView, link them using Phoenix static path helpers (e.g. `~p` or `Routes.static_path/2`, depending on Phoenix version). + +### File Limits + +- **Max file size:** 10 MB +- **Max rows:** 1,000 rows (excluding header) +- **Processing:** chunks of 200 (via LiveView messages) +- **Encoding:** UTF-8 (BOM handled) + +--- + +## Technical Design Notes + +### Architecture Overview + +``` +┌─────────────────┐ +│ LiveView UI │ (GlobalSettingsLive or component) +│ - Upload area │ +│ - Progress │ +│ - Results │ +└────────┬────────┘ + │ prepare + ▼ +┌─────────────────────────────┐ +│ Import Service │ (Mv.Membership.Import.MemberCSV) +│ - parse + map + limit checks│ -> returns import_state +│ - process_chunk(chunk) │ -> returns chunk results +└────────┬────────────────────┘ + │ create + ▼ +┌─────────────────┐ +│ Ash Resource │ (Mv.Membership.Member) +│ - Create │ +└─────────────────┘ +``` + +### Technology Stack + +- **Phoenix LiveView:** file upload via `allow_upload/3` +- **NimbleCSV:** CSV parsing (add explicit dependency if missing) +- **Ash Resource:** member creation via `Membership.create_member/1` +- **Gettext:** bilingual UI/error messages + +### Module Structure + +**New Modules:** +- `lib/mv/membership/import/member_csv.ex` - import orchestration + chunk processing +- `lib/mv/membership/import/csv_parser.ex` - delimiter detection + parsing + BOM handling +- `lib/mv/membership/import/header_mapper.ex` - normalization + header mapping + +**Modified Modules:** +- `lib/mv_web/live/global_settings_live.ex` - render import section, handle upload/events/messages + +### Data Flow + +1. **Upload:** LiveView receives file via `allow_upload` +2. **Consume:** `consume_uploaded_entries/3` reads file content +3. **Prepare:** `MemberCSV.prepare/2` + - Strip BOM + - Detect delimiter (header recognition) + - Parse header + rows + - Map headers to canonical fields + - Early abort if required headers missing + - Row count check + - Return `import_state` containing chunks and metadata +4. **Process:** LiveView drives chunk processing via `handle_info` + - For each chunk: validate + create + collect errors +5. **Results:** LiveView shows progress + final summary + +### Types & Key Consistency + +- **Raw CSV parsing:** returns headers as list of strings, and rows **with csv line numbers** +- **Header mapping:** operates on normalized strings; mapping table variants are normalized once +- **Ash attrs:** built as atom-keyed map (`%{first_name: ..., ...}`) + +### Error Model + +```elixir +%{ + csv_line_number: 5, # physical line number in the CSV file + field: :email, # optional + message: "is not a valid email" +} +``` + +### CSV Line Numbers (Important) + +To keep error reporting user-friendly and accurate, **row errors must reference the physical line number in the original file**, even if empty lines are skipped. + +**Design decision:** the parser returns rows as: + +```elixir +rows :: [{csv_line_number :: pos_integer(), row_map :: map()}] +``` + +Downstream logic must **not** recompute line numbers from row indexes. + +### Authorization + +**Enforcement points:** +1. **LiveView event level:** check admin permission in `handle_event("start_import", ...)` +2. **UI level:** render import section only for admin users +3. **Static templates:** public assets (no authorization needed) + +Use `Mv.Authorization.PermissionSets` (preferred) instead of hard-coded string checks where possible. + +### Safety Limits + +- File size enforced by `allow_upload` (`max_file_size`) +- Row count enforced in `MemberCSV.prepare/2` before processing starts +- Chunking is done via **LiveView `handle_info` loop** (sequential, cooperative scheduling) + +--- + +## Implementation Issues + +### Issue #1: CSV Specification & Static Template Files + +**Dependencies:** None + +**Goal:** Define CSV contract and add static templates. + +**Tasks:** +- [ ] Finalize header mapping variants +- [ ] Document normalization rules +- [ ] Document delimiter detection strategy +- [ ] Create templates in `priv/static/templates/` (UTF-8 with BOM) +- [ ] Document template URLs and how to link them from LiveView +- [ ] Document line number semantics (physical CSV line numbers) + +**Definition of Done:** +- [ ] Templates open cleanly in Excel/LibreOffice +- [ ] CSV spec section complete + +--- + +### Issue #2: Import Service Module Skeleton + +**Dependencies:** None + +**Goal:** Create service API and error types. + +**API (recommended):** +- `prepare/2` — parse + map + limit checks, returns import_state +- `process_chunk/3` — process one chunk (pure-ish), returns per-chunk results + +**Tasks:** +- [ ] Create `lib/mv/membership/import/member_csv.ex` +- [ ] Define public function: `prepare/2 (file_content, opts \\ [])` +- [ ] Define public function: `process_chunk/3 (chunk_rows_with_lines, column_map, opts \\ [])` +- [ ] Define error struct: `%MemberCSV.Error{csv_line_number: integer, field: atom | nil, message: String.t}` +- [ ] Document module + API + +--- + +### Issue #3: CSV Parsing + Delimiter Auto-Detection + BOM Handling + +**Dependencies:** Issue #2 + +**Goal:** Parse CSV robustly with correct delimiter detection and BOM handling. + +**Tasks:** +- [ ] Verify/add NimbleCSV dependency (`{:nimble_csv, "~> 1.0"}`) +- [ ] Create `lib/mv/membership/import/csv_parser.ex` +- [ ] Implement `strip_bom/1` and apply it **before** any header handling +- [ ] Handle `\r\n` and `\n` line endings (trim `\r` on header record) +- [ ] Detect delimiter via header recognition (try `;` and `,`) +- [ ] Parse CSV and return: + - `headers :: [String.t()]` + - `rows :: [{csv_line_number, [String.t()]}]` or directly `[{csv_line_number, row_map}]` +- [ ] Skip completely empty records (but preserve correct physical line numbers) +- [ ] Return `{:ok, headers, rows}` or `{:error, reason}` + +**Definition of Done:** +- [ ] BOM handling works (Excel exports) +- [ ] Delimiter detection works reliably +- [ ] Rows carry correct `csv_line_number` + +--- + +### Issue #4: Header Normalization + Per-Header Mapping (No Language Detection) + +**Dependencies:** Issue #3 + +**Goal:** Map each header individually to canonical fields (normalized comparison). + +**Tasks:** +- [ ] Create `lib/mv/membership/import/header_mapper.ex` +- [ ] Implement `normalize_header/1` +- [ ] Normalize mapping variants once and compare normalized strings +- [ ] Build `column_map` (canonical field -> column index) +- [ ] **Early abort if required headers missing** (`first_name`, `last_name`, `email`) +- [ ] Ignore unknown columns + +**Definition of Done:** +- [ ] English/German headers map correctly +- [ ] Missing required columns fails fast + +--- + +### Issue #5: Validation (Required Fields) + Error Formatting + +**Dependencies:** Issue #4 + +**Goal:** Validate each row and return structured, translatable errors. + +**Tasks:** +- [ ] Implement `validate_row/3 (row_map, csv_line_number, opts)` +- [ ] Required field presence (`first_name`, `last_name`, `email`) +- [ ] Email format validation (EctoCommons.EmailValidator) +- [ ] Trim values before validation +- [ ] Gettext-backed error messages + +--- + +### Issue #6: Persistence via Ash Create + Per-Row Error Capture (Chunked Processing) + +**Dependencies:** Issue #5 + +**Goal:** Create members and capture errors per row with correct CSV line numbers. + +**Tasks:** +- [ ] Implement `process_chunk/3` in service: + - Input: `[{csv_line_number, row_map}]` + - Validate + create sequentially + - Collect counts + first 50 errors (per import overall; LiveView enforces cap across chunks) +- [ ] Implement Ash error formatter helper: + - Convert `Ash.Error.Invalid` into `%MemberCSV.Error{}` + - Prefer field-level errors where possible (attach `field` atom) + - Handle unique email constraint error as user-friendly message +- [ ] Map row_map to Ash attrs (`%{first_name: ..., ...}`) + +**Important:** **Do not recompute line numbers** in this layer—use the ones provided by the parser. + +--- + +### Issue #7: Admin Global Settings LiveView UI (Upload + Start Import + Results + Template Links) + +**Dependencies:** Issue #6 + +**Goal:** UI section with upload, progress, results, and template links. + +**Tasks:** +- [ ] Render import section only for admins +- [ ] Configure `allow_upload/3`: + - `.csv` only, `max_entries: 1`, `max_file_size: 10MB`, `auto_upload: false` +- [ ] `handle_event("start_import", ...)`: + - Admin permission check + - Consume upload -> read file content + - Call `MemberCSV.prepare/2` + - Store `import_state` in assigns (chunks + column_map + metadata) + - Initialize progress assigns + - `send(self(), {:process_chunk, 0})` +- [ ] `handle_info({:process_chunk, idx}, socket)`: + - Fetch chunk from `import_state` + - Call `MemberCSV.process_chunk/3` + - Merge counts/errors into progress assigns (cap errors at 50 overall) + - Schedule next chunk (or finish and show results) +- [ ] Results UI: + - Success count + - Failure count + - Error list (line number + message + field) + +**Template links:** +- Link `/templates/member_import_en.csv` and `/templates/member_import_de.csv` via Phoenix static path helpers. + +--- + +### Issue #8: Authorization + Limits + +**Dependencies:** None (can be parallelized) + +**Goal:** Ensure admin-only access and enforce limits. + +**Tasks:** +- [ ] Admin check in start import event handler +- [ ] File size enforced in upload config +- [ ] Row limit enforced in `MemberCSV.prepare/2` (max_rows from config) +- [ ] Configuration: + ```elixir + config :mv, csv_import: [ + max_file_size_mb: 10, + max_rows: 1000 + ] + ``` + +--- + +### Issue #9: End-to-End LiveView Tests + Fixtures + +**Dependencies:** Issue #7 and #8 + +**Tasks:** +- [ ] Fixtures: + - valid EN/DE + - invalid + - too many rows (1,001) + - BOM + `;` delimiter fixture + - fixture with empty line(s) to validate correct line numbers +- [ ] LiveView tests: + - admin sees section, non-admin does not + - upload + start import + - success + error rendering + - row limit + file size errors + +--- + +### Issue #10: Documentation Polish (Inline Help Text + Docs) + +**Dependencies:** Issue #9 + +**Tasks:** +- [ ] UI help text + translations +- [ ] CHANGELOG entry +- [ ] Ensure moduledocs/docs + +--- + +### Issue #11: Custom Field Import (Optional - v1.1) + +**Dependencies:** Issue #10 +**Status:** Optional + +*(unchanged — intentionally deferred)* + +--- + +## Rollout & Risks + +### Rollout Strategy +- Dev → Staging → Production (with anonymized real-world CSV tests) + +### Risks & Mitigations + +| Risk | Impact | Likelihood | Mitigation | +|---|---:|---:|---| +| Large import timeout | High | Medium | 10 MB + 1,000 rows, chunking via `handle_info` | +| Encoding issues | Medium | Medium | BOM stripping, templates with BOM | +| Invalid CSV format | Medium | High | Clear errors + templates | +| Duplicate emails | Low | High | Ash constraint error -> user-friendly message | +| Performance (no background jobs) | Medium | Low | Small limits, sequential chunk processing | +| Admin access bypass | High | Low | Event-level auth + UI hiding | +| Data corruption | High | Low | Per-row validation + best-effort | + +--- + +## Appendix + +### Module File Structure + +``` +lib/ +├── mv/ +│ └── membership/ +│ └── import/ +│ ├── member_csv.ex # prepare + process_chunk +│ ├── csv_parser.ex # delimiter detection + parsing + BOM handling +│ └── header_mapper.ex # normalization + header mapping +└── mv_web/ + └── live/ + └── global_settings_live.ex # add import section + LV message loop + +priv/ +└── static/ + └── templates/ + ├── member_import_en.csv + └── member_import_de.csv + +test/ +├── mv/ +│ └── membership/ +│ └── import/ +│ ├── member_csv_test.exs +│ ├── csv_parser_test.exs +│ └── header_mapper_test.exs +└── fixtures/ + ├── member_import_en.csv + ├── member_import_de.csv + ├── member_import_invalid.csv + ├── member_import_large.csv + └── member_import_empty_lines.csv +``` + +### Example Usage (LiveView) + +```elixir +def handle_event("start_import", _params, socket) do + assert_admin!(socket.assigns.current_user) + + [{_name, content}] = + consume_uploaded_entries(socket, :csv_file, fn %{path: path}, _entry -> + {:ok, File.read!(path)} + end) + + case Mv.Membership.Import.MemberCSV.prepare(content) do + {:ok, import_state} -> + socket = + socket + |> assign(:import_state, import_state) + |> assign(:import_progress, %{processed: 0, inserted: 0, failed: 0, errors: []}) + |> assign(:importing?, true) + + send(self(), {:process_chunk, 0}) + {:noreply, socket} + + {:error, reason} -> + {:noreply, put_flash(socket, :error, reason)} + end +end + +def handle_info({:process_chunk, idx}, socket) do + %{chunks: chunks, column_map: column_map} = socket.assigns.import_state + + case Enum.at(chunks, idx) do + nil -> + {:noreply, assign(socket, importing?: false)} + + chunk_rows_with_lines -> + {:ok, chunk_result} = + Mv.Membership.Import.MemberCSV.process_chunk(chunk_rows_with_lines, column_map) + + socket = merge_progress(socket, chunk_result) # caps errors at 50 overall + + send(self(), {:process_chunk, idx + 1}) + {:noreply, socket} + end +end +``` + +--- + +**End of Implementation Plan** \ No newline at end of file diff --git a/docs/test-status-membership-fee-ui.md b/docs/test-status-membership-fee-ui.md new file mode 100644 index 0000000..63445fb --- /dev/null +++ b/docs/test-status-membership-fee-ui.md @@ -0,0 +1,137 @@ +# Test Status: Membership Fee UI Components + +**Date:** 2025-01-XX +**Status:** Tests Written - Implementation Complete + +## Übersicht + +Alle Tests für die Membership Fee UI-Komponenten wurden geschrieben. Die Tests sind TDD-konform geschrieben und sollten erfolgreich laufen, da die Implementation bereits vorhanden ist. + +## Test-Dateien + +### Helper Module Tests + +**Datei:** `test/mv_web/helpers/membership_fee_helpers_test.exs` +- ✅ format_currency/1 formats correctly +- ✅ format_interval/1 formats all interval types +- ✅ format_cycle_range/2 formats date ranges correctly +- ✅ get_last_completed_cycle/2 returns correct cycle +- ✅ get_current_cycle/2 returns correct cycle +- ✅ status_color/1 returns correct color classes +- ✅ status_icon/1 returns correct icon names + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +**Datei:** `test/mv_web/member_live/index/membership_fee_status_test.exs` +- ✅ load_cycles_for_members/2 efficiently loads cycles +- ✅ get_cycle_status_for_member/2 returns correct status +- ✅ format_cycle_status_badge/1 returns correct badge + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +### Member List View Tests + +**Datei:** `test/mv_web/member_live/index_membership_fee_status_test.exs` +- ✅ Status column displays correctly +- ✅ Shows last completed cycle status by default +- ✅ Toggle switches to current cycle view +- ✅ Color coding for paid/unpaid/suspended +- ✅ Filter "Unpaid in last cycle" works +- ✅ Filter "Unpaid in current cycle" works +- ✅ Handles members without cycles gracefully +- ✅ Loads cycles efficiently without N+1 queries + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +### Member Detail View Tests + +**Datei:** `test/mv_web/member_live/show_membership_fees_test.exs` +- ✅ Cycles table displays all cycles +- ✅ Table columns show correct data +- ✅ Membership fee type dropdown shows only same-interval types +- ✅ Warning displayed if different interval selected +- ✅ Status change actions work (mark as paid/suspended/unpaid) +- ✅ Cycle regeneration works +- ✅ Handles members without membership fee type gracefully + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +### Membership Fee Types Admin Tests + +**Datei:** `test/mv_web/live/membership_fee_type_live/index_test.exs` +- ✅ List displays all types with correct data +- ✅ Member count column shows correct count +- ✅ Create button navigates to form +- ✅ Edit button per row navigates to edit form +- ✅ Delete button disabled if type is in use +- ✅ Delete button works if type is not in use +- ✅ Only admin can access + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +**Datei:** `test/mv_web/live/membership_fee_type_live/form_test.exs` +- ✅ Create form works +- ✅ Edit form loads existing type data +- ✅ Interval field editable on create +- ✅ Interval field grayed out on edit +- ✅ Amount change warning displays on edit +- ✅ Amount change warning shows correct affected member count +- ✅ Amount change can be confirmed +- ✅ Amount change can be cancelled +- ✅ Validation errors display correctly +- ✅ Only admin can access + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +### Member Form Tests + +**Datei:** `test/mv_web/member_live/form_membership_fee_type_test.exs` +- ✅ Membership fee type dropdown displays in form +- ✅ Shows available types +- ✅ Filters to same interval types if member has type +- ✅ Warning displayed if different interval selected +- ✅ Warning cleared if same interval selected +- ✅ Form saves with selected membership fee type +- ✅ New members get default membership fee type + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +### Integration Tests + +**Datei:** `test/mv_web/member_live/membership_fee_integration_test.exs` +- ✅ End-to-end: Create type → Assign to member → View cycles → Change status +- ✅ End-to-end: Change member type → Cycles regenerate +- ✅ End-to-end: Update settings → New members get default type +- ✅ End-to-end: Delete cycle → Confirmation → Cycle deleted +- ✅ End-to-end: Edit cycle amount → Modal → Amount updated + +**Status:** Alle Tests sollten erfolgreich sein (Implementation vorhanden) + +## Test-Ausführung + +Alle Tests können mit folgenden Befehlen ausgeführt werden: + +```bash +# Alle Tests +mix test + +# Nur Membership Fee Tests +mix test test/mv_web/helpers/membership_fee_helpers_test.exs +mix test test/mv_web/member_live/ +mix test test/mv_web/live/membership_fee_type_live/ + +# Mit Coverage +mix test --cover +``` + +## Bekannte Probleme + +Keine bekannten Probleme. Alle Tests sollten erfolgreich laufen, da die Implementation bereits vorhanden ist. + +## Nächste Schritte + +1. ✅ Tests geschrieben +2. ⏳ Tests ausführen und verifizieren +3. ⏳ Eventuelle Anpassungen basierend auf Test-Ergebnissen +4. ⏳ Code-Review durchführen + diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 787b1d1..1d6d96e 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -39,6 +39,7 @@ defmodule Mv.Membership.Member do require Ash.Query import Ash.Expr + require Logger # Module constants @member_search_limit 10 @@ -73,6 +74,9 @@ defmodule Mv.Membership.Member do create :create_member do primary? true + + # Note: Custom validation function cannot be done atomically (queries DB for required custom fields) + # In Ash 3.0, require_atomic? is not available for create actions, but the validation will still work # Custom field values can be created along with member argument :custom_field_values, {:array, :map} # Allow user to be passed as argument for relationship management @@ -102,6 +106,9 @@ defmodule Mv.Membership.Member do where [changing(:user)] end + # Auto-assign default membership fee type if not explicitly set + change Mv.Membership.Member.Changes.SetDefaultMembershipFeeType + # Auto-calculate membership_fee_start_date if not manually set # Requires both join_date and membership_fee_type_id to be present change Mv.MembershipFees.Changes.SetMembershipFeeStartDate @@ -110,54 +117,18 @@ defmodule Mv.Membership.Member do # Only runs if membership_fee_type_id is set # Note: Cycle generation runs asynchronously to not block the action, # but in test environment it runs synchronously for DB sandbox compatibility - change after_action(fn _changeset, member, _context -> - if member.membership_fee_type_id && member.join_date do - if Application.get_env(:mv, :sql_sandbox, false) do - # Run synchronously in test environment for DB sandbox compatibility - # Use skip_lock?: true to avoid nested transactions (after_action runs within action transaction) - # Return notifications to Ash so they are sent after commit - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( - member.id, - today: Date.utc_today(), - skip_lock?: true - ) do - {:ok, _cycles, notifications} -> - {:ok, member, notifications} - - {:error, reason} -> - require Logger - - Logger.warning( - "Failed to generate cycles for member #{member.id}: #{inspect(reason)}" - ) - - {:ok, member} + change after_transaction(fn _changeset, result, _context -> + case result do + {:ok, member} -> + if member.membership_fee_type_id && member.join_date do + handle_cycle_generation(member) end - else - # Run asynchronously in other environments - # Send notifications explicitly since they cannot be returned via after_action - Task.start(fn -> - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id) do - {:ok, _cycles, notifications} -> - # Send notifications manually for async case - if Enum.any?(notifications) do - Ash.Notifier.notify(notifications) - end - {:error, reason} -> - require Logger - - Logger.warning( - "Failed to generate cycles for member #{member.id}: #{inspect(reason)}" - ) - end - end) - - {:ok, member} - end - else - {:ok, member} + {:error, _} -> + :ok end + + result end) end @@ -239,6 +210,29 @@ defmodule Mv.Membership.Member do {:ok, member} end end) + + # Trigger cycle regeneration when join_date or exit_date changes + # Regenerates cycles based on new dates + # Note: Cycle generation runs synchronously in test environment, asynchronously in production + # CycleGenerator uses advisory locks and transactions internally to prevent race conditions + # If both join_date and exit_date are changed simultaneously, this hook runs only once + # (Ash ensures each after_transaction hook runs once per action, regardless of how many attributes changed) + change after_transaction(fn changeset, result, _context -> + case result do + {:ok, member} -> + join_date_changed = Ash.Changeset.changing_attribute?(changeset, :join_date) + exit_date_changed = Ash.Changeset.changing_attribute?(changeset, :exit_date) + + if (join_date_changed || exit_date_changed) && member.membership_fee_type_id do + handle_cycle_generation(member) + end + + {:error, _} -> + :ok + end + + result + end) end # Action to handle fuzzy search on specific fields @@ -392,7 +386,7 @@ defmodule Mv.Membership.Member do end end - # Join date not in the future + # Join date not in future validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0), where: [present(:join_date)], message: "cannot be in the future" @@ -427,6 +421,32 @@ defmodule Mv.Membership.Member do {:error, field: :email, message: "is not a valid email"} end end + + # Validate required custom fields + validate fn changeset, _ -> + provided_values = provided_custom_field_values(changeset) + + case Mv.Membership.list_required_custom_fields() do + {:ok, required_custom_fields} -> + missing_fields = missing_required_fields(required_custom_fields, provided_values) + + if Enum.empty?(missing_fields) do + :ok + else + build_custom_field_validation_error(missing_fields) + end + + {:error, error} -> + Logger.error( + "Failed to load custom fields for validation: #{inspect(error)}. Required field validation cannot be performed." + ) + + {:error, + field: :custom_field_values, + message: + "Unable to validate required custom fields. Please try again or contact support."} + end + end end attributes do @@ -454,10 +474,6 @@ defmodule Mv.Membership.Member do constraints min_length: 5, max_length: 254 end - attribute :paid, :boolean do - allow_nil? true - end - attribute :phone_number, :string do allow_nil? true end @@ -868,6 +884,90 @@ defmodule Mv.Membership.Member do end end + # Handles cycle generation for a member, choosing sync or async execution + # based on environment (test vs production) + # This function encapsulates the common logic for cycle generation + # to avoid code duplication across different hooks + defp handle_cycle_generation(member) do + if Mv.Config.sql_sandbox?() do + handle_cycle_generation_sync(member) + else + handle_cycle_generation_async(member) + end + end + + # Runs cycle generation synchronously (for test environment) + defp handle_cycle_generation_sync(member) do + require Logger + + case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( + member.id, + today: Date.utc_today() + ) do + {:ok, cycles, notifications} -> + send_notifications_if_any(notifications) + log_cycle_generation_success(member, cycles, notifications, sync: true) + + {:error, reason} -> + log_cycle_generation_error(member, reason, sync: true) + end + end + + # Runs cycle generation asynchronously (for production environment) + defp handle_cycle_generation_async(member) do + Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> + case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id) do + {:ok, cycles, notifications} -> + send_notifications_if_any(notifications) + log_cycle_generation_success(member, cycles, notifications, sync: false) + + {:error, reason} -> + log_cycle_generation_error(member, reason, sync: false) + end + end) + end + + # Sends notifications if any are present + defp send_notifications_if_any(notifications) do + if Enum.any?(notifications) do + Ash.Notifier.notify(notifications) + end + end + + # Logs successful cycle generation + defp log_cycle_generation_success(member, cycles, notifications, sync: sync?) do + require Logger + + sync_label = if sync?, do: "", else: " (async)" + + Logger.debug( + "Successfully generated cycles for member#{sync_label}", + member_id: member.id, + cycles_count: length(cycles), + notifications_count: length(notifications) + ) + end + + # Logs cycle generation errors + defp log_cycle_generation_error(member, reason, sync: sync?) do + require Logger + + sync_label = if sync?, do: "", else: " (async)" + + Logger.error( + "Failed to generate cycles for member#{sync_label}", + member_id: member.id, + member_email: member.email, + error: inspect(reason), + error_type: error_type(reason) + ) + end + + # Helper to extract error type for structured logging + defp error_type(%{__struct__: struct_name}), do: struct_name + defp error_type(error) when is_atom(error), do: error + defp error_type(_), do: :unknown + # Normalizes visibility config map keys from strings to atoms. # JSONB in PostgreSQL converts atom keys to string keys when storing. defp normalize_visibility_config(config) when is_map(config) do @@ -1062,4 +1162,127 @@ defmodule Mv.Membership.Member do query end end + + # Extracts provided custom field values from changeset + # Handles both create (from argument) and update (from existing data) scenarios + defp provided_custom_field_values(changeset) do + custom_field_values_arg = Ash.Changeset.get_argument(changeset, :custom_field_values) + + if is_nil(custom_field_values_arg) do + extract_existing_values(changeset.data) + else + extract_argument_values(custom_field_values_arg) + end + end + + # Extracts custom field values from existing member data (update scenario) + defp extract_existing_values(member_data) do + case Ash.load(member_data, :custom_field_values) do + {:ok, %{custom_field_values: existing_values}} -> + Enum.reduce(existing_values, %{}, &extract_value_from_cfv/2) + + _ -> + %{} + end + end + + # Extracts value from a CustomFieldValue struct + defp extract_value_from_cfv(cfv, acc) do + value = extract_union_value(cfv.value) + Map.put(acc, cfv.custom_field_id, value) + end + + # Extracts value from union type (map or direct value) + defp extract_union_value(value) when is_map(value), do: Map.get(value, :value) + defp extract_union_value(value), do: value + + # Extracts custom field values from provided argument (create/update scenario) + defp extract_argument_values(custom_field_values_arg) do + Enum.reduce(custom_field_values_arg, %{}, &extract_value_from_arg/2) + end + + # Extracts value from argument map + defp extract_value_from_arg(cfv, acc) do + custom_field_id = Map.get(cfv, "custom_field_id") + value_map = Map.get(cfv, "value", %{}) + actual_value = extract_value_from_map(value_map) + Map.put(acc, custom_field_id, actual_value) + end + + # Extracts value from map, supporting both "value" and "_union_value" keys + # Also handles Ash.Union structs (which have atom keys :value and :type) + # Uses cond instead of || to preserve false values + defp extract_value_from_map(value_map) do + cond do + # Handle Ash.Union struct - check if it's a struct with __struct__ == Ash.Union + match?({:ok, Ash.Union}, Map.fetch(value_map, :__struct__)) -> + Map.get(value_map, :value) + + # Handle map with string keys + Map.has_key?(value_map, "value") -> + Map.get(value_map, "value") + + Map.has_key?(value_map, "_union_value") -> + Map.get(value_map, "_union_value") + + # Handle map with atom keys + Map.has_key?(value_map, :value) -> + Map.get(value_map, :value) + + true -> + nil + end + end + + # Finds which required custom fields are missing from provided values + defp missing_required_fields(required_custom_fields, provided_values) do + Enum.filter(required_custom_fields, fn cf -> + value = Map.get(provided_values, cf.id) + not value_present?(value, cf.value_type) + end) + end + + # Builds validation error message for missing required custom fields + defp build_custom_field_validation_error(missing_fields) do + # Sort missing fields alphabetically for consistent error messages + sorted_missing_fields = Enum.sort_by(missing_fields, & &1.name) + missing_names = Enum.map_join(sorted_missing_fields, ", ", & &1.name) + + {:error, + field: :custom_field_values, + message: + Gettext.dgettext(MvWeb.Gettext, "default", "Required custom fields missing: %{fields}", + fields: missing_names + )} + end + + # Helper function to check if a value is present for a given custom field type + # Boolean: false is valid, only nil is invalid + # String: nil or empty strings are invalid + # Integer: nil or empty strings are invalid, 0 is valid + # Date: nil or empty strings are invalid + # Email: nil or empty strings are invalid + defp value_present?(nil, _type), do: false + + defp value_present?(value, :boolean), do: not is_nil(value) + + defp value_present?(value, :string), do: is_binary(value) and String.trim(value) != "" + + defp value_present?(value, :integer) when is_integer(value), do: true + + defp value_present?(value, :integer) when is_binary(value), do: String.trim(value) != "" + + defp value_present?(_value, :integer), do: false + + defp value_present?(value, :date) when is_struct(value, Date), do: true + + defp value_present?(value, :date) when is_binary(value), do: String.trim(value) != "" + + defp value_present?(_value, :date), do: false + + defp value_present?(value, :email) when is_binary(value), do: String.trim(value) != "" + + defp value_present?(_value, :email), do: false + + defp value_present?(_value, _type), do: false end diff --git a/lib/membership/member/changes/set_default_membership_fee_type.ex b/lib/membership/member/changes/set_default_membership_fee_type.ex new file mode 100644 index 0000000..8b75ed7 --- /dev/null +++ b/lib/membership/member/changes/set_default_membership_fee_type.ex @@ -0,0 +1,42 @@ +defmodule Mv.Membership.Member.Changes.SetDefaultMembershipFeeType do + @moduledoc """ + Ash change that automatically assigns the default membership fee type to new members + if no membership_fee_type_id is explicitly provided. + + This change reads the default_membership_fee_type_id from global settings and + assigns it to the member if membership_fee_type_id is nil. + """ + use Ash.Resource.Change + + @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t() + def change(changeset, _opts, _context) do + # Only set default if membership_fee_type_id is not already set + current_type_id = Ash.Changeset.get_attribute(changeset, :membership_fee_type_id) + + if is_nil(current_type_id) do + apply_default_membership_fee_type(changeset) + else + changeset + end + end + + defp apply_default_membership_fee_type(changeset) do + case Mv.Membership.get_settings() do + {:ok, settings} -> + if settings.default_membership_fee_type_id do + Ash.Changeset.force_change_attribute( + changeset, + :membership_fee_type_id, + settings.default_membership_fee_type_id + ) + else + changeset + end + + {:error, _error} -> + # If settings can't be loaded, continue without default + # This prevents member creation from failing if settings are misconfigured + changeset + end + end +end diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index f5a708b..4917c7c 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -21,6 +21,9 @@ defmodule Mv.Membership do use Ash.Domain, extensions: [AshAdmin.Domain, AshPhoenix] + require Ash.Query + import Ash.Expr + admin do show? true end @@ -125,6 +128,29 @@ defmodule Mv.Membership do |> Ash.update(domain: __MODULE__) end + @doc """ + Lists only required custom fields. + + This is an optimized version that filters at the database level instead of + loading all custom fields and filtering in memory. + + ## Returns + + - `{:ok, required_custom_fields}` - List of required custom fields + - `{:error, error}` - Error reading custom fields + + ## Examples + + iex> {:ok, required_fields} = Mv.Membership.list_required_custom_fields() + iex> Enum.all?(required_fields, & &1.required) + true + """ + def list_required_custom_fields do + Mv.Membership.CustomField + |> Ash.Query.filter(expr(required == true)) + |> Ash.read(domain: __MODULE__) + end + @doc """ Updates the member field visibility configuration. diff --git a/lib/membership_fees/membership_fee_cycle.ex b/lib/membership_fees/membership_fee_cycle.ex index b437ead..4d9c8b7 100644 --- a/lib/membership_fees/membership_fee_cycle.ex +++ b/lib/membership_fees/membership_fee_cycle.ex @@ -49,7 +49,7 @@ defmodule Mv.MembershipFees.MembershipFeeCycle do update :update do primary? true - accept [:status, :notes] + accept [:status, :notes, :amount] end update :mark_as_paid do diff --git a/lib/mv/application.ex b/lib/mv/application.ex index b77107e..09eefce 100644 --- a/lib/mv/application.ex +++ b/lib/mv/application.ex @@ -10,6 +10,7 @@ defmodule Mv.Application do children = [ MvWeb.Telemetry, Mv.Repo, + {Task.Supervisor, name: Mv.TaskSupervisor}, {DNSCluster, query: Application.get_env(:mv, :dns_cluster_query) || :ignore}, {Phoenix.PubSub, name: Mv.PubSub}, {AshAuthentication.Supervisor, otp_app: :my}, diff --git a/lib/mv/config.ex b/lib/mv/config.ex new file mode 100644 index 0000000..5e6ba90 --- /dev/null +++ b/lib/mv/config.ex @@ -0,0 +1,24 @@ +defmodule Mv.Config do + @moduledoc """ + Configuration helper functions for the application. + + Provides centralized access to configuration values to avoid + magic strings/atoms scattered throughout the codebase. + """ + + @doc """ + Returns whether SQL sandbox mode is enabled. + + SQL sandbox mode is typically enabled in test environments + to allow concurrent database access in tests. + + ## Returns + + - `true` if SQL sandbox is enabled + - `false` otherwise + """ + @spec sql_sandbox?() :: boolean() + def sql_sandbox? do + Application.get_env(:mv, :sql_sandbox, false) + end +end diff --git a/lib/mv/constants.ex b/lib/mv/constants.ex index 843ad2b..c81dbd6 100644 --- a/lib/mv/constants.ex +++ b/lib/mv/constants.ex @@ -7,7 +7,6 @@ defmodule Mv.Constants do :first_name, :last_name, :email, - :paid, :phone_number, :join_date, :exit_date, diff --git a/lib/mv/membership_fees/calendar_cycles.ex b/lib/mv/membership_fees/calendar_cycles.ex index 8a4ef24..9bc3afa 100644 --- a/lib/mv/membership_fees/calendar_cycles.ex +++ b/lib/mv/membership_fees/calendar_cycles.ex @@ -299,11 +299,15 @@ defmodule Mv.MembershipFees.CalendarCycles do end defp quarterly_cycle_end(cycle_start) do - case cycle_start.month do - 1 -> Date.new!(cycle_start.year, 3, 31) - 4 -> Date.new!(cycle_start.year, 6, 30) - 7 -> Date.new!(cycle_start.year, 9, 30) - 10 -> Date.new!(cycle_start.year, 12, 31) + # Ensure cycle_start is aligned to quarter boundary + # This handles cases where cycle_start might not be at the correct quarter start (e.g., month 12) + aligned_start = quarterly_cycle_start(cycle_start) + + case aligned_start.month do + 1 -> Date.new!(aligned_start.year, 3, 31) + 4 -> Date.new!(aligned_start.year, 6, 30) + 7 -> Date.new!(aligned_start.year, 9, 30) + 10 -> Date.new!(aligned_start.year, 12, 31) end end @@ -313,9 +317,13 @@ defmodule Mv.MembershipFees.CalendarCycles do end defp half_yearly_cycle_end(cycle_start) do - case cycle_start.month do - 1 -> Date.new!(cycle_start.year, 6, 30) - 7 -> Date.new!(cycle_start.year, 12, 31) + # Ensure cycle_start is aligned to half-year boundary + # This handles cases where cycle_start might not be at the correct half-year start (e.g., month 10) + aligned_start = half_yearly_cycle_start(cycle_start) + + case aligned_start.month do + 1 -> Date.new!(aligned_start.year, 6, 30) + 7 -> Date.new!(aligned_start.year, 12, 31) end end diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index feb7b53..7d6c798 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -379,25 +379,34 @@ defmodule Mv.MembershipFees.CycleGenerator do status: :unpaid } - case Ash.create(MembershipFeeCycle, attrs, return_notifications?: true) do - {:ok, cycle, notifications} when is_list(notifications) -> - {:ok, cycle, notifications} - - {:ok, cycle} -> - {:ok, cycle, []} - - {:error, reason} -> - {:error, {cycle_start, reason}} - end + handle_cycle_creation_result( + Ash.create(MembershipFeeCycle, attrs, return_notifications?: true), + cycle_start + ) end) - {successes, errors} = Enum.split_with(results, &match?({:ok, _, _}, &1)) + {successes, skips, errors} = + Enum.reduce(results, {[], [], []}, fn + {:ok, cycle, notifications}, {successes, skips, errors} -> + {[{:ok, cycle, notifications} | successes], skips, errors} + + {:skip, cycle_start}, {successes, skips, errors} -> + {successes, [cycle_start | skips], errors} + + {:error, error}, {successes, skips, errors} -> + {successes, skips, [error | errors]} + end) all_notifications = Enum.flat_map(successes, fn {:ok, _cycle, notifications} -> notifications end) if Enum.empty?(errors) do successful_cycles = Enum.map(successes, fn {:ok, cycle, _notifications} -> cycle end) + + if Enum.any?(skips) do + Logger.debug("Skipped #{length(skips)} cycles that already exist for member #{member_id}") + end + {:ok, successful_cycles, all_notifications} else Logger.warning("Some cycles failed to create: #{inspect(errors)}") @@ -407,4 +416,45 @@ defmodule Mv.MembershipFees.CycleGenerator do {:error, {:partial_failure, errors}} end end + + defp handle_cycle_creation_result({:ok, cycle, notifications}, _cycle_start) + when is_list(notifications) do + {:ok, cycle, notifications} + end + + defp handle_cycle_creation_result({:ok, cycle}, _cycle_start) do + {:ok, cycle, []} + end + + defp handle_cycle_creation_result( + {:error, + %Ash.Error.Invalid{ + errors: [ + %Ash.Error.Changes.InvalidAttribute{ + private_vars: %{constraint: constraint, constraint_type: :unique} + } + ] + }} = error, + cycle_start + ) do + # Cycle already exists (unique constraint violation) - skip it silently + # This makes the function idempotent and prevents errors on server restart + handle_unique_constraint_violation(constraint, cycle_start, error) + end + + defp handle_cycle_creation_result({:error, reason}, cycle_start) do + {:error, {cycle_start, reason}} + end + + defp handle_unique_constraint_violation( + "membership_fee_cycles_unique_cycle_per_member_index", + cycle_start, + _error + ) do + {:skip, cycle_start} + end + + defp handle_unique_constraint_violation(_constraint, cycle_start, error) do + {:error, {cycle_start, error}} + end end diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index a1020ef..ccec5a5 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -333,7 +333,8 @@ defmodule MvWeb.CoreComponents do attr :error_class, :string, default: nil, doc: "the input error class to use over defaults" attr :rest, :global, - include: ~w(accept autocomplete capture cols disabled form list max maxlength min minlength + include: + ~w(accept autocomplete aria-required capture cols disabled form list max maxlength min minlength multiple pattern placeholder readonly required rows size step) def input(%{field: %Phoenix.HTML.FormField{} = field} = assigns) do @@ -353,6 +354,24 @@ defmodule MvWeb.CoreComponents do Phoenix.HTML.Form.normalize_value("checkbox", assigns[:value]) end) + # For checkboxes, we don't use HTML required attribute (means "must be checked") + # Instead, we use aria-required for screen readers (WCAG 2.1, Success Criterion 3.3.2) + # Extract required from rest and remove it, but keep aria-required if provided + rest = assigns.rest || %{} + is_required = Map.get(rest, :required, false) + aria_required = Map.get(rest, :aria_required, if(is_required, do: "true", else: nil)) + + # Remove required from rest (we don't want HTML required on checkbox) + rest_without_required = Map.delete(rest, :required) + # Ensure aria-required is set if field is required + rest_final = + if aria_required, + do: Map.put(rest_without_required, :aria_required, aria_required), + else: rest_without_required + + assigns = assign(assigns, :rest, rest_final) + assigns = assign(assigns, :is_required, is_required) + ~H"""
diff --git a/lib/mv_web/components/layouts/navbar.ex b/lib/mv_web/components/layouts/navbar.ex index c2e28d6..adc3444 100644 --- a/lib/mv_web/components/layouts/navbar.ex +++ b/lib/mv_web/components/layouts/navbar.ex @@ -32,7 +32,9 @@ defmodule MvWeb.Layouts.Navbar do
{gettext("Contributions")}