diff --git a/.drone.yml b/.drone.yml index 06db32b..8c7f325 100644 --- a/.drone.yml +++ b/.drone.yml @@ -4,7 +4,7 @@ name: check services: - name: postgres - image: docker.io/library/postgres:18.1 + image: docker.io/library/postgres:17.7 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:18.1 + image: docker.io/library/postgres:17.7 commands: # Wait for postgres to become available - | @@ -166,7 +166,7 @@ environment: steps: - name: renovate - image: renovate/renovate:42.71 + image: renovate/renovate:42.44 environment: RENOVATE_CONFIG_FILE: "renovate_backend_config.js" RENOVATE_TOKEN: diff --git a/.gitignore b/.gitignore index 058543c..9517a21 100644 --- a/.gitignore +++ b/.gitignore @@ -44,4 +44,3 @@ npm-debug.log # Docker secrets directory (generated by `just init-secrets`) /secrets/ -notes.md diff --git a/.tool-versions b/.tool-versions index 275206c..489262a 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1,3 +1,3 @@ elixir 1.18.3-otp-27 erlang 27.3.4 -just 1.46.0 +just 1.45.0 diff --git a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md b/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md deleted file mode 100644 index eebf419..0000000 --- a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md +++ /dev/null @@ -1,318 +0,0 @@ ---- -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 5fcfcf5..053fc19 100644 --- a/config/config.exs +++ b/config/config.exs @@ -95,16 +95,7 @@ config :tailwind, # Configures Elixir's Logger config :logger, :default_formatter, format: "$time $metadata[$level] $message\n", - metadata: [ - :request_id, - :user_id, - :member_id, - :member_email, - :error, - :error_type, - :cycles_count, - :notifications_count - ] + metadata: [:request_id] # 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 1ed863a..5b35e10 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -33,7 +33,7 @@ services: restart: unless-stopped db-prod: - image: postgres:18.1-alpine + image: postgres:17.7-alpine container_name: mv-prod-db environment: POSTGRES_USER: postgres diff --git a/docker-compose.yml b/docker-compose.yml index 8621603..feff34c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -4,7 +4,7 @@ networks: services: db: - image: postgres:18.1-alpine + image: postgres:17.7-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.4 + image: ghcr.io/sebadob/rauthy:0.33.1 environment: - LOCAL_TEST=true - SMTP_URL=mailcrab diff --git a/docs/test-status-membership-fee-ui.md b/docs/test-status-membership-fee-ui.md deleted file mode 100644 index 63445fb..0000000 --- a/docs/test-status-membership-fee-ui.md +++ /dev/null @@ -1,137 +0,0 @@ -# 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 1d6d96e..787b1d1 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -39,7 +39,6 @@ defmodule Mv.Membership.Member do require Ash.Query import Ash.Expr - require Logger # Module constants @member_search_limit 10 @@ -74,9 +73,6 @@ 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 @@ -106,9 +102,6 @@ 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 @@ -117,18 +110,54 @@ 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_transaction(fn _changeset, result, _context -> - case result do - {:ok, member} -> - if member.membership_fee_type_id && member.join_date do - handle_cycle_generation(member) + 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} 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, _} -> - :ok + {:error, reason} -> + require Logger + + Logger.warning( + "Failed to generate cycles for member #{member.id}: #{inspect(reason)}" + ) + end + end) + + {:ok, member} + end + else + {:ok, member} end - - result end) end @@ -210,29 +239,6 @@ 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 @@ -386,7 +392,7 @@ defmodule Mv.Membership.Member do end end - # Join date not in future + # Join date not in the future validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0), where: [present(:join_date)], message: "cannot be in the future" @@ -421,32 +427,6 @@ 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 @@ -474,6 +454,10 @@ 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 @@ -884,90 +868,6 @@ 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 @@ -1162,127 +1062,4 @@ 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 deleted file mode 100644 index 8b75ed7..0000000 --- a/lib/membership/member/changes/set_default_membership_fee_type.ex +++ /dev/null @@ -1,42 +0,0 @@ -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 4917c7c..f5a708b 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -21,9 +21,6 @@ defmodule Mv.Membership do use Ash.Domain, extensions: [AshAdmin.Domain, AshPhoenix] - require Ash.Query - import Ash.Expr - admin do show? true end @@ -128,29 +125,6 @@ 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 4d9c8b7..b437ead 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, :amount] + accept [:status, :notes] end update :mark_as_paid do diff --git a/lib/mv/application.ex b/lib/mv/application.ex index 09eefce..b77107e 100644 --- a/lib/mv/application.ex +++ b/lib/mv/application.ex @@ -10,7 +10,6 @@ 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 deleted file mode 100644 index 5e6ba90..0000000 --- a/lib/mv/config.ex +++ /dev/null @@ -1,24 +0,0 @@ -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 c81dbd6..843ad2b 100644 --- a/lib/mv/constants.ex +++ b/lib/mv/constants.ex @@ -7,6 +7,7 @@ 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 9bc3afa..8a4ef24 100644 --- a/lib/mv/membership_fees/calendar_cycles.ex +++ b/lib/mv/membership_fees/calendar_cycles.ex @@ -299,15 +299,11 @@ defmodule Mv.MembershipFees.CalendarCycles do end defp quarterly_cycle_end(cycle_start) do - # 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) + 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) end end @@ -317,13 +313,9 @@ defmodule Mv.MembershipFees.CalendarCycles do end defp half_yearly_cycle_end(cycle_start) do - # 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) + case cycle_start.month do + 1 -> Date.new!(cycle_start.year, 6, 30) + 7 -> Date.new!(cycle_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 7d6c798..feb7b53 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -379,34 +379,25 @@ defmodule Mv.MembershipFees.CycleGenerator do status: :unpaid } - handle_cycle_creation_result( - Ash.create(MembershipFeeCycle, attrs, return_notifications?: true), - cycle_start - ) + 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 end) - {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) + {successes, errors} = Enum.split_with(results, &match?({:ok, _, _}, &1)) 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)}") @@ -416,45 +407,4 @@ 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 ccec5a5..a1020ef 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -333,8 +333,7 @@ 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 aria-required capture cols disabled form list max maxlength min minlength + include: ~w(accept autocomplete 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 @@ -354,24 +353,6 @@ 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 adc3444..c2e28d6 100644 --- a/lib/mv_web/components/layouts/navbar.ex +++ b/lib/mv_web/components/layouts/navbar.ex @@ -32,9 +32,7 @@ defmodule MvWeb.Layouts.Navbar do
{gettext("Contributions")}