diff --git a/.drone.yml b/.drone.yml index e207ca0..52d48cd 100644 --- a/.drone.yml +++ b/.drone.yml @@ -166,7 +166,7 @@ environment: steps: - name: renovate - image: renovate/renovate:42.81 + image: renovate/renovate:42.80 environment: RENOVATE_CONFIG_FILE: "renovate_backend_config.js" RENOVATE_TOKEN: diff --git a/Justfile b/Justfile index c68c473..b835cf4 100644 --- a/Justfile +++ b/Justfile @@ -32,8 +32,6 @@ lint: mix format --check-formatted mix compile --warnings-as-errors mix credo - # Check that all German translations are filled (UI must be in German) - @bash -c 'for file in priv/gettext/de/LC_MESSAGES/*.po; do awk "/^msgid \"\"$/{header=1; next} /^msgid /{header=0} /^msgstr \"\"$/ && !header{print FILENAME\":\"NR\": \" \$0; exit 1}" "$file" || exit 1; done' mix gettext.extract --check-up-to-date audit: @@ -118,4 +116,4 @@ init-prod-secrets: # Start production environment with Docker Compose start-prod: init-prod-secrets - docker compose -f docker-compose.prod.yml up -d + docker compose -f docker-compose.prod.yml up -d \ No newline at end of file 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/docs/csv-member-import-v1.md b/docs/csv-member-import-v1.md index bc8f99f..2bdbe69 100644 --- a/docs/csv-member-import-v1.md +++ b/docs/csv-member-import-v1.md @@ -191,26 +191,6 @@ A **basic CSV member import feature** that allows administrators to upload a 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). -**Example Usage in LiveView Templates:** - -```heex - -<.link href={~p"/templates/member_import_en.csv"} download> - <%= gettext("Download English Template") %> - - -<.link href={~p"/templates/member_import_de.csv"} download> - <%= gettext("Download German Template") %> - - - -<.link href={Routes.static_path(MvWeb.Endpoint, "/templates/member_import_en.csv")} download> - <%= gettext("Download English Template") %> - -``` - -**Note:** The `templates` directory must be included in `MvWeb.static_paths()` (configured in `lib/mv_web.ex`) for the files to be served. - ### File Limits - **Max file size:** 10 MB diff --git a/docs/roles-and-permissions-architecture.md b/docs/roles-and-permissions-architecture.md index 8c89af7..b44604b 100644 --- a/docs/roles-and-permissions-architecture.md +++ b/docs/roles-and-permissions-architecture.md @@ -110,8 +110,8 @@ Control access to LiveView pages: Three scope levels for permissions: - **:own** - Only records where `record.id == user.id` (for User resource) - **:linked** - Only records linked to user via relationships - - Member: `id == user.member_id` (User.member_id → Member.id, inverse relationship) - - CustomFieldValue: `member_id == user.member_id` (traverses Member → User relationship) + - Member: `member.user_id == user.id` + - CustomFieldValue: `custom_field_value.member.user_id == user.id` - **:all** - All records, no filtering **6. Special Cases** @@ -714,8 +714,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) - - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) + - Member: member.user_id == actor.id + - CustomFieldValue: custom_field_value.member.user_id == actor.id (traverses relationship!) ## Error Handling @@ -799,14 +799,12 @@ defmodule Mv.Authorization.Checks.HasPermission do defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # User.member_id → Member.id (inverse relationship) - # Filter: member.id == actor.member_id - {:filter, expr(id == ^actor.member_id)} + # Member.user_id == actor.id (direct relationship) + {:filter, expr(user_id == ^actor.id)} "CustomFieldValue" -> - # CustomFieldValue.member_id → Member.id → User.member_id - # Filter: custom_field_value.member_id == actor.member_id - {:filter, expr(member_id == ^actor.member_id)} + # CustomFieldValue.member.user_id == actor.id (traverse through member!) + {:filter, expr(member.user_id == ^actor.id)} _ -> # Fallback for other resources: try direct user_id @@ -920,7 +918,7 @@ end **Location:** `lib/mv/membership/member.ex` -**Special Case:** Users can always READ their linked member (where `id == user.member_id`). +**Special Case:** Users can always access their linked member (where `member.user_id == user.id`). ```elixir defmodule Mv.Membership.Member do @@ -980,10 +978,10 @@ defmodule Mv.Membership.CustomFieldValue do policies do # SPECIAL CASE: Users can access custom field values of their linked member - # Note: This uses member_id relationship (CustomFieldValue.member_id → Member.id → User.member_id) + # Note: This traverses the member relationship! policy action_type([:read, :update]) do description "Users can access custom field values of their linked member" - authorize_if expr(member_id == ^actor(:member_id)) + authorize_if expr(member.user_id == ^actor(:id)) end # GENERAL: Check permissions from role diff --git a/docs/roles-and-permissions-overview.md b/docs/roles-and-permissions-overview.md index 61aef9c..86e7273 100644 --- a/docs/roles-and-permissions-overview.md +++ b/docs/roles-and-permissions-overview.md @@ -294,9 +294,7 @@ Each Permission Set contains: **:own** - Only records where id == actor.id - Example: User can read their own User record -**:linked** - Only records linked to actor via relationships -- Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) -- CustomFieldValue: `member_id == actor.member_id` (traverses Member → User relationship) +**:linked** - Only records where user_id == actor.id - Example: User can read Member linked to their account **:all** - All records without restriction diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 41e02b1..d2ea07d 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -34,12 +34,10 @@ defmodule Mv.Membership.Member do """ use Ash.Resource, domain: Mv.Membership, - data_layer: AshPostgres.DataLayer, - authorizers: [Ash.Policy.Authorizer] + data_layer: AshPostgres.DataLayer require Ash.Query import Ash.Expr - alias Mv.Helpers require Logger alias Mv.Membership.Helpers.VisibilityConfig @@ -120,12 +118,11 @@ 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 -> + change after_transaction(fn _changeset, result, _context -> case result do {:ok, member} -> if member.membership_fee_type_id && member.join_date do - actor = Map.get(changeset.context, :actor) - handle_cycle_generation(member, actor: actor) + handle_cycle_generation(member) end {:error, _} -> @@ -196,9 +193,7 @@ defmodule Mv.Membership.Member do Ash.Changeset.changing_attribute?(changeset, :membership_fee_type_id) if fee_type_changed && member.membership_fee_type_id && member.join_date do - actor = Map.get(changeset.context, :actor) - - case regenerate_cycles_on_type_change(member, actor: actor) do + case regenerate_cycles_on_type_change(member) do {:ok, notifications} -> # Return notifications to Ash - they will be sent automatically after commit {:ok, member, notifications} @@ -230,8 +225,7 @@ defmodule Mv.Membership.Member do exit_date_changed = Ash.Changeset.changing_attribute?(changeset, :exit_date) if (join_date_changed || exit_date_changed) && member.membership_fee_type_id do - actor = Map.get(changeset.context, :actor) - handle_cycle_generation(member, actor: actor) + handle_cycle_generation(member) end {:error, _} -> @@ -302,41 +296,6 @@ defmodule Mv.Membership.Member do end end - # Authorization Policies - # Order matters: Most specific policies first, then general permission check - policies do - # SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY) - # In test: All operations allowed (for test fixtures) - # In production/dev: ALL operations denied without actor (fail-closed for security) - # NoActor.check uses compile-time environment detection to prevent security issues - bypass action_type([:create, :read, :update, :destroy]) do - description "Allow system operations without actor (test environment only)" - authorize_if Mv.Authorization.Checks.NoActor - end - - # SPECIAL CASE: Users can always READ their linked member - # This allows users with ANY permission set to read their own linked member - # Check using the inverse relationship: User.member_id → Member.id - bypass action_type(:read) do - description "Users can always read member linked to their account" - authorize_if expr(id == ^actor(:member_id)) - end - - # GENERAL: Check permissions from user's role - # HasPermission handles update permissions correctly: - # - :own_data → can update linked member (scope :linked) - # - :read_only → cannot update any member (no update permission) - # - :normal_user → can update all members (scope :all) - # - :admin → can update all members (scope :all) - policy action_type([:read, :create, :update, :destroy]) do - description "Check permissions from user's role and permission set" - authorize_if Mv.Authorization.Checks.HasPermission - end - - # DEFAULT: Forbid if no policy matched - # Ash implicitly forbids if no policy authorized - end - @doc """ Filters members list based on email match priority. @@ -404,13 +363,8 @@ defmodule Mv.Membership.Member do user_id = user_arg[:id] current_member_id = changeset.data.id - # Get actor from changeset context for authorization - # If no actor is present, this will fail in production (fail-closed) - actor = Map.get(changeset.context || %{}, :actor) - # Check the current state of the user in the database - # Pass actor to ensure proper authorization (User might have policies in future) - case Ash.get(Mv.Accounts.User, user_id, actor: actor) do + case Ash.get(Mv.Accounts.User, user_id) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok @@ -788,37 +742,33 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} where notifications are collected # to be sent after transaction commits @doc false - def regenerate_cycles_on_type_change(member, opts \\ []) do + def regenerate_cycles_on_type_change(member) do today = Date.utc_today() lock_key = :erlang.phash2(member.id) - actor = Keyword.get(opts, :actor) # Use advisory lock to prevent concurrent deletion and regeneration # This ensures atomicity when multiple updates happen simultaneously if Mv.Repo.in_transaction?() do - regenerate_cycles_in_transaction(member, today, lock_key, actor: actor) + regenerate_cycles_in_transaction(member, today, lock_key) else - regenerate_cycles_new_transaction(member, today, lock_key, actor: actor) + regenerate_cycles_new_transaction(member, today, lock_key) end end # Already in transaction: use advisory lock directly # Returns {:ok, notifications} - notifications should be returned to after_action hook - defp regenerate_cycles_in_transaction(member, today, lock_key, opts) do - actor = Keyword.get(opts, :actor) + defp regenerate_cycles_in_transaction(member, today, lock_key) do Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) + do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) end # Not in transaction: start new transaction with advisory lock # Returns {:ok, notifications} - notifications should be sent by caller (e.g., via after_action) - defp regenerate_cycles_new_transaction(member, today, lock_key, opts) do - actor = Keyword.get(opts, :actor) - + defp regenerate_cycles_new_transaction(member, today, lock_key) do Mv.Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Mv.Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true, actor: actor) do + case do_regenerate_cycles_on_type_change(member, today, skip_lock?: true) do {:ok, notifications} -> # Return notifications - they will be sent by the caller notifications @@ -840,7 +790,6 @@ defmodule Mv.Membership.Member do require Ash.Query skip_lock? = Keyword.get(opts, :skip_lock?, false) - actor = Keyword.get(opts, :actor) # Find all unpaid cycles for this member # We need to check cycle_end for each cycle using its own interval @@ -850,21 +799,10 @@ defmodule Mv.Membership.Member do |> Ash.Query.filter(status == :unpaid) |> Ash.Query.load([:membership_fee_type]) - result = - if actor do - Ash.read(all_unpaid_cycles_query, actor: actor) - else - Ash.read(all_unpaid_cycles_query) - end - - case result do + case Ash.read(all_unpaid_cycles_query) do {:ok, all_unpaid_cycles} -> cycles_to_delete = filter_future_cycles(all_unpaid_cycles, today) - - delete_and_regenerate_cycles(cycles_to_delete, member.id, today, - skip_lock?: skip_lock?, - actor: actor - ) + delete_and_regenerate_cycles(cycles_to_delete, member.id, today, skip_lock?: skip_lock?) {:error, reason} -> {:error, reason} @@ -893,14 +831,13 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} or {:error, reason} defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) - actor = Keyword.get(opts, :actor) if Enum.empty?(cycles_to_delete) do # No cycles to delete, just regenerate - regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) + regenerate_cycles(member_id, today, skip_lock?: skip_lock?) else case delete_cycles(cycles_to_delete) do - :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?, actor: actor) + :ok -> regenerate_cycles(member_id, today, skip_lock?: skip_lock?) {:error, reason} -> {:error, reason} end end @@ -926,13 +863,11 @@ defmodule Mv.Membership.Member do # Returns {:ok, notifications} - notifications should be returned to after_action hook defp regenerate_cycles(member_id, today, opts) do skip_lock? = Keyword.get(opts, :skip_lock?, false) - actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member_id, today: today, - skip_lock?: skip_lock?, - actor: actor + skip_lock?: skip_lock? ) do {:ok, _cycles, notifications} when is_list(notifications) -> {:ok, notifications} @@ -946,25 +881,21 @@ defmodule Mv.Membership.Member do # 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, opts) do - actor = Keyword.get(opts, :actor) - + defp handle_cycle_generation(member) do if Mv.Config.sql_sandbox?() do - handle_cycle_generation_sync(member, actor: actor) + handle_cycle_generation_sync(member) else - handle_cycle_generation_async(member, actor: actor) + handle_cycle_generation_async(member) end end # Runs cycle generation synchronously (for test environment) - defp handle_cycle_generation_sync(member, opts) do + defp handle_cycle_generation_sync(member) do require Logger - actor = Keyword.get(opts, :actor) case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member( member.id, - today: Date.utc_today(), - actor: actor + today: Date.utc_today() ) do {:ok, cycles, notifications} -> send_notifications_if_any(notifications) @@ -976,11 +907,9 @@ defmodule Mv.Membership.Member do end # Runs cycle generation asynchronously (for production environment) - defp handle_cycle_generation_async(member, opts) do - actor = Keyword.get(opts, :actor) - + defp handle_cycle_generation_async(member) do Task.Supervisor.async_nolink(Mv.TaskSupervisor, fn -> - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member.id, actor: actor) do + 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) @@ -1209,18 +1138,15 @@ defmodule Mv.Membership.Member 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, changeset) + 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, changeset) do - actor = Map.get(changeset.context, :actor) - opts = Helpers.ash_actor_opts(actor) - - case Ash.load(member_data, :custom_field_values, opts) do + 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) diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 18ffe5b..345d6e4 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -21,8 +21,8 @@ defmodule Mv.Authorization.Checks.HasPermission do - **:all** - Authorizes without filtering (returns all records) - **:own** - Filters to records where record.id == actor.id - **:linked** - Filters based on resource type: - - Member: `id == actor.member_id` (User.member_id → Member.id, inverse relationship) - - CustomFieldValue: `member_id == actor.member_id` (CustomFieldValue.member_id → Member.id → User.member_id) + - Member: member.user.id == actor.id (via has_one :user relationship) + - CustomFieldValue: custom_field_value.member.user.id == actor.id (traverses member → user relationship!) ## Error Handling @@ -59,7 +59,6 @@ defmodule Mv.Authorization.Checks.HasPermission do def strict_check(actor, authorizer, _opts) do resource = authorizer.resource action = get_action_from_authorizer(authorizer) - record = get_record_from_authorizer(authorizer) cond do is_nil(actor) -> @@ -77,12 +76,12 @@ defmodule Mv.Authorization.Checks.HasPermission do {:ok, false} true -> - strict_check_with_permissions(actor, resource, action, record) + strict_check_with_permissions(actor, resource, action) end end # Helper function to reduce nesting depth - defp strict_check_with_permissions(actor, resource, action, record) do + defp strict_check_with_permissions(actor, resource, action) do with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- actor, {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), permissions <- PermissionSets.get_permissions(ps_atom), @@ -94,15 +93,9 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> - {:ok, true} - - {:filter, filter_expr} -> - # For strict_check on single records, evaluate the filter against the record - evaluate_filter_for_strict_check(filter_expr, actor, record, resource_name) - - false -> - {:ok, false} + :authorized -> {:ok, true} + {:filter, _} -> {:ok, :unknown} + false -> {:ok, false} end else %{role: nil} -> @@ -129,17 +122,9 @@ defmodule Mv.Authorization.Checks.HasPermission do action = get_action_from_authorizer(authorizer) cond do - is_nil(actor) -> - # No actor - deny access (fail-closed) - # Return filter that never matches (expr(false) = match none) - deny_filter() - - is_nil(action) -> - # Cannot determine action - deny access (fail-closed) - deny_filter() - - true -> - auto_filter_with_permissions(actor, resource, action) + is_nil(actor) -> nil + is_nil(action) -> nil + true -> auto_filter_with_permissions(actor, resource, action) end end @@ -156,97 +141,21 @@ defmodule Mv.Authorization.Checks.HasPermission do actor, resource_name ) do - :authorized -> - # :all scope - allow all records (no filter) - # Return empty keyword list (no filtering) - [] - - {:filter, filter_expr} -> - # :linked or :own scope - apply filter - # filter_expr is a keyword list from expr(...), return it directly - filter_expr - - false -> - # No permission - deny access (fail-closed) - deny_filter() + :authorized -> nil + {:filter, filter_expr} -> filter_expr + false -> nil end else - _ -> - # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) - deny_filter() - end - end - - # Helper function to return a filter that never matches (deny all records) - # Used when authorization should be denied (fail-closed) - # - # Using `expr(false)` avoids depending on the primary key being named `:id`. - # This is more robust than [id: {:in, []}] which assumes the primary key is `:id`. - defp deny_filter do - expr(false) - end - - # Helper to extract action type from authorizer - # CRITICAL: Must use action_type, not action.name! - # Action types: :create, :read, :update, :destroy - # Action names: :create_member, :update_member, etc. - # PermissionSets uses action types, not action names - # - # Prefer authorizer.action.type (stable API) over authorizer.subject (varies by context) - defp get_action_from_authorizer(authorizer) do - # Primary: Use authorizer.action.type (stable API) - case Map.get(authorizer, :action) do - %{type: action_type} when action_type in [:create, :read, :update, :destroy] -> - action_type - - _ -> - # Fallback: Try authorizer.subject (for compatibility with different Ash versions/contexts) - case Map.get(authorizer, :subject) do - %{action_type: action_type} when action_type in [:create, :read, :update, :destroy] -> - action_type - - %{action: %{type: action_type}} - when action_type in [:create, :read, :update, :destroy] -> - action_type - - _ -> - nil - end - end - end - - # Helper to extract record from authorizer for strict_check - defp get_record_from_authorizer(authorizer) do - case authorizer.subject do - %{data: data} when not is_nil(data) -> data _ -> nil end end - # Evaluate filter expression for strict_check on single records - # For :linked scope with Member resource: id == actor.member_id - defp evaluate_filter_for_strict_check(_filter_expr, actor, record, resource_name) do - case {resource_name, record} do - {"Member", %{id: member_id}} when not is_nil(member_id) -> - # Check if this member's ID matches the actor's member_id - if member_id == actor.member_id do - {:ok, true} - else - {:ok, false} - end - - {"CustomFieldValue", %{member_id: cfv_member_id}} when not is_nil(cfv_member_id) -> - # Check if this CFV's member_id matches the actor's member_id - if cfv_member_id == actor.member_id do - {:ok, true} - else - {:ok, false} - end - - _ -> - # For other cases or when record is not available, return :unknown - # This will cause Ash to use auto_filter instead - {:ok, :unknown} + # Helper to extract action from authorizer + defp get_action_from_authorizer(authorizer) do + case authorizer.subject do + %{action: %{name: action}} -> action + %{action: action} when is_atom(action) -> action + _ -> nil end end @@ -281,24 +190,21 @@ defmodule Mv.Authorization.Checks.HasPermission do end # Scope: linked - Filter based on user relationship (resource-specific!) - # IMPORTANT: Understand the relationship direction! - # - User belongs_to :member (User.member_id → Member.id) - # - Member has_one :user (inverse, no FK on Member) + # Uses Ash relationships: Member has_one :user, CustomFieldValue belongs_to :member defp apply_scope(:linked, actor, resource_name) do case resource_name do "Member" -> - # User.member_id → Member.id (inverse relationship) - # Filter: member.id == actor.member_id - {:filter, expr(id == ^actor.member_id)} + # Member has_one :user → filter by user.id == actor.id + {:filter, expr(user.id == ^actor.id)} "CustomFieldValue" -> - # CustomFieldValue.member_id → Member.id → User.member_id - # Filter: custom_field_value.member_id == actor.member_id - {:filter, expr(member_id == ^actor.member_id)} + # CustomFieldValue belongs_to :member → member has_one :user + # Traverse: custom_field_value.member.user.id == actor.id + {:filter, expr(member.user.id == ^actor.id)} _ -> - # Fallback for other resources - {:filter, expr(user_id == ^actor.id)} + # Fallback for other resources: try user relationship first, then user_id + {:filter, expr(user.id == ^actor.id or user_id == ^actor.id)} end end diff --git a/lib/mv/authorization/checks/no_actor.ex b/lib/mv/authorization/checks/no_actor.ex deleted file mode 100644 index f5eebb7..0000000 --- a/lib/mv/authorization/checks/no_actor.ex +++ /dev/null @@ -1,74 +0,0 @@ -defmodule Mv.Authorization.Checks.NoActor do - @moduledoc """ - Custom Ash Policy Check that allows actions when no actor is present. - - **IMPORTANT:** This check ONLY works in test environment for security reasons. - In production/dev, ALL operations without an actor are denied. - - ## Security Note - - This check uses compile-time environment detection to prevent accidental - security issues in production. In production, ALL operations (including :create - and :read) will be denied if no actor is present. - - For seeds and system operations in production, use an admin actor instead: - - admin_user = get_admin_user() - Ash.create!(resource, attrs, actor: admin_user) - - ## Usage in Policies - - policies do - # Allow system operations without actor (TEST ENVIRONMENT ONLY) - # In test: All operations allowed - # In production: ALL operations denied (fail-closed) - bypass action_type([:create, :read, :update, :destroy]) do - authorize_if NoActor - end - - # Check permissions when actor is present - policy action_type([:read, :create, :update, :destroy]) do - authorize_if HasPermission - end - end - - ## Behavior - - - In test environment: Returns `true` when actor is nil (allows all operations) - - In production/dev: Returns `false` when actor is nil (denies all operations - fail-closed) - - Returns `false` when actor is present (delegates to other policies) - """ - - use Ash.Policy.SimpleCheck - - # Compile-time check: Only allow no-actor bypass in test environment - @allow_no_actor_bypass Mix.env() == :test - # Alternative (if you want to control via config): - # @allow_no_actor_bypass Application.compile_env(:mv, :allow_no_actor_bypass, false) - - @impl true - def describe(_opts) do - if @allow_no_actor_bypass do - "allows actions when no actor is present (test environment only)" - else - "denies all actions when no actor is present (production/dev - fail-closed)" - end - end - - @impl true - def match?(nil, _context, _opts) do - # Actor is nil - if @allow_no_actor_bypass do - # Test environment: Allow all operations - true - else - # Production/dev: Deny all operations (fail-closed for security) - false - end - end - - def match?(_actor, _context, _opts) do - # Actor is present - don't match (let other policies decide) - false - end -end diff --git a/lib/mv/email_sync/changes/sync_member_email_to_user.ex b/lib/mv/email_sync/changes/sync_member_email_to_user.ex index 0c0d8f7..48c7955 100644 --- a/lib/mv/email_sync/changes/sync_member_email_to_user.ex +++ b/lib/mv/email_sync/changes/sync_member_email_to_user.ex @@ -41,10 +41,8 @@ defmodule Mv.EmailSync.Changes.SyncMemberEmailToUser do Ash.Changeset.around_transaction(changeset, fn cs, callback -> result = callback.(cs) - actor = Map.get(changeset.context, :actor) - with {:ok, member} <- Helpers.extract_record(result), - linked_user <- Loader.get_linked_user(member, actor) do + linked_user <- Loader.get_linked_user(member) do Helpers.sync_email_to_linked_record(result, linked_user, new_email) else _ -> result diff --git a/lib/mv/email_sync/changes/sync_user_email_to_member.ex b/lib/mv/email_sync/changes/sync_user_email_to_member.ex index 54829a4..7148067 100644 --- a/lib/mv/email_sync/changes/sync_user_email_to_member.ex +++ b/lib/mv/email_sync/changes/sync_user_email_to_member.ex @@ -33,17 +33,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do if Map.get(context, :syncing_email, false) do changeset else - # Ensure actor is in changeset context - get it from context if available - actor = Map.get(changeset.context, :actor) || Map.get(context, :actor) - - changeset_with_actor = - if actor && !Map.has_key?(changeset.context, :actor) do - Ash.Changeset.put_context(changeset, :actor, actor) - else - changeset - end - - sync_email(changeset_with_actor) + sync_email(changeset) end end @@ -52,7 +42,7 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do result = callback.(cs) with {:ok, record} <- Helpers.extract_record(result), - {:ok, user, member} <- get_user_and_member(record, cs) do + {:ok, user, member} <- get_user_and_member(record) do # When called from Member-side, we need to update the member in the result # When called from User-side, we update the linked member in DB only case record do @@ -71,19 +61,15 @@ defmodule Mv.EmailSync.Changes.SyncUserEmailToMember do end # Retrieves user and member - works for both resource types - defp get_user_and_member(%Mv.Accounts.User{} = user, changeset) do - actor = Map.get(changeset.context, :actor) - - case Loader.get_linked_member(user, actor) do + defp get_user_and_member(%Mv.Accounts.User{} = user) do + case Loader.get_linked_member(user) do nil -> {:error, :no_member} member -> {:ok, user, member} end end - defp get_user_and_member(%Mv.Membership.Member{} = member, changeset) do - actor = Map.get(changeset.context, :actor) - - case Loader.load_linked_user!(member, actor) do + defp get_user_and_member(%Mv.Membership.Member{} = member) do + case Loader.load_linked_user!(member) do {:ok, user} -> {:ok, user, member} error -> error end diff --git a/lib/mv/email_sync/loader.ex b/lib/mv/email_sync/loader.ex index 91927fb..ecb1038 100644 --- a/lib/mv/email_sync/loader.ex +++ b/lib/mv/email_sync/loader.ex @@ -2,30 +2,15 @@ defmodule Mv.EmailSync.Loader do @moduledoc """ Helper functions for loading linked records in email synchronization. Centralizes the logic for retrieving related User/Member entities. - - ## Authorization - - This module runs systemically and accepts optional actor parameters. - When called from hooks/changes, actor is extracted from changeset context. - When called directly, actor should be provided for proper authorization. - - All functions accept an optional `actor` parameter that is passed to Ash operations - to ensure proper authorization checks are performed. """ - alias Mv.Helpers @doc """ Loads the member linked to a user, returns nil if not linked or on error. - - Accepts optional actor for authorization. """ - def get_linked_member(user, actor \\ nil) - def get_linked_member(%{member_id: nil}, _actor), do: nil + def get_linked_member(%{member_id: nil}), do: nil - def get_linked_member(%{member_id: id}, actor) do - opts = Helpers.ash_actor_opts(actor) - - case Ash.get(Mv.Membership.Member, id, opts) do + def get_linked_member(%{member_id: id}) do + case Ash.get(Mv.Membership.Member, id) do {:ok, member} -> member {:error, _} -> nil end @@ -33,13 +18,9 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returns nil if not linked or on error. - - Accepts optional actor for authorization. """ - def get_linked_user(member, actor \\ nil) do - opts = Helpers.ash_actor_opts(actor) - - case Ash.load(member, :user, opts) do + def get_linked_user(member) do + case Ash.load(member, :user) do {:ok, %{user: user}} -> user {:error, _} -> nil end @@ -48,13 +29,9 @@ defmodule Mv.EmailSync.Loader do @doc """ Loads the user linked to a member, returning an error tuple if not linked. Useful when a link is required for the operation. - - Accepts optional actor for authorization. """ - def load_linked_user!(member, actor \\ nil) do - opts = Helpers.ash_actor_opts(actor) - - case Ash.load(member, :user, opts) do + def load_linked_user!(member) do + case Ash.load(member, :user) do {:ok, %{user: user}} when not is_nil(user) -> {:ok, user} {:ok, _} -> {:error, :no_linked_user} {:error, _} = error -> error diff --git a/lib/mv/helpers.ex b/lib/mv/helpers.ex deleted file mode 100644 index e20db67..0000000 --- a/lib/mv/helpers.ex +++ /dev/null @@ -1,27 +0,0 @@ -defmodule Mv.Helpers do - @moduledoc """ - Shared helper functions used across the Mv application. - - Provides utilities that are not specific to a single domain or layer. - """ - - @doc """ - Converts an actor to Ash options list for authorization. - Returns empty list if actor is nil. - - This helper ensures consistent actor handling across all Ash operations - in the application, whether called from LiveViews, changes, validations, - or other contexts. - - ## Examples - - opts = ash_actor_opts(actor) - Ash.read(query, opts) - - opts = ash_actor_opts(nil) - # => [] - """ - @spec ash_actor_opts(Mv.Accounts.User.t() | nil) :: keyword() - def ash_actor_opts(nil), do: [] - def ash_actor_opts(actor) when not is_nil(actor), do: [actor: actor] -end diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index e1bbc4e..a14bc0b 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -8,7 +8,6 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation - alias Mv.Helpers @doc """ Validates email uniqueness across linked Member-User pairs. @@ -30,8 +29,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do def validate(changeset, _opts, _context) do email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - actor = Map.get(changeset.context || %{}, :actor) - linked_user_id = get_linked_user_id(changeset.data, actor) + linked_user_id = get_linked_user_id(changeset.data) is_linked? = not is_nil(linked_user_id) # Only validate if member is already linked AND email is changing @@ -40,21 +38,19 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do if should_validate? do new_email = Ash.Changeset.get_attribute(changeset, :email) - check_email_uniqueness(new_email, linked_user_id, actor) + check_email_uniqueness(new_email, linked_user_id) else :ok end end - defp check_email_uniqueness(email, exclude_user_id, actor) do + defp check_email_uniqueness(email, exclude_user_id) do query = Mv.Accounts.User |> Ash.Query.filter(email == ^email) |> maybe_exclude_id(exclude_user_id) - opts = Helpers.ash_actor_opts(actor) - - case Ash.read(query, opts) do + case Ash.read(query) do {:ok, []} -> :ok @@ -69,10 +65,8 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do defp maybe_exclude_id(query, nil), do: query defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) - defp get_linked_user_id(member_data, actor) do - opts = Helpers.ash_actor_opts(actor) - - case Ash.load(member_data, :user, opts) do + defp get_linked_user_id(member_data) do + case Ash.load(member_data, :user) do {:ok, %{user: %{id: id}}} -> id _ -> nil end diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index 0d17dd3..7d6c798 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -28,15 +28,6 @@ defmodule Mv.MembershipFees.CycleGenerator do Uses PostgreSQL advisory locks to prevent race conditions when generating cycles for the same member concurrently. - ## Authorization - - This module runs systemically and accepts optional actor parameters. - When called from hooks/changes, actor is extracted from changeset context. - When called directly, actor should be provided for proper authorization. - - All functions accept an optional `actor` parameter in the `opts` keyword list - that is passed to Ash operations to ensure proper authorization checks are performed. - ## Examples # Generate cycles for a single member @@ -86,9 +77,7 @@ defmodule Mv.MembershipFees.CycleGenerator do def generate_cycles_for_member(member_or_id, opts \\ []) def generate_cycles_for_member(member_id, opts) when is_binary(member_id) do - actor = Keyword.get(opts, :actor) - - case load_member(member_id, actor: actor) do + case load_member(member_id) do {:ok, member} -> generate_cycles_for_member(member, opts) {:error, reason} -> {:error, reason} end @@ -98,27 +87,25 @@ defmodule Mv.MembershipFees.CycleGenerator do today = Keyword.get(opts, :today, Date.utc_today()) skip_lock? = Keyword.get(opts, :skip_lock?, false) - do_generate_cycles_with_lock(member, today, skip_lock?, opts) + do_generate_cycles_with_lock(member, today, skip_lock?) end # Generate cycles with lock handling # Returns {:ok, cycles, notifications} - notifications are never sent here, # they should be returned to the caller (e.g., via after_action hook) - defp do_generate_cycles_with_lock(member, today, true = _skip_lock?, opts) do + defp do_generate_cycles_with_lock(member, today, true = _skip_lock?) do # Lock already set by caller (e.g., regenerate_cycles_on_type_change) # Just generate cycles without additional locking - actor = Keyword.get(opts, :actor) - do_generate_cycles(member, today, actor: actor) + do_generate_cycles(member, today) end - defp do_generate_cycles_with_lock(member, today, false, opts) do + defp do_generate_cycles_with_lock(member, today, false) do lock_key = :erlang.phash2(member.id) - actor = Keyword.get(opts, :actor) Repo.transaction(fn -> Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) - case do_generate_cycles(member, today, actor: actor) do + case do_generate_cycles(member, today) do {:ok, cycles, notifications} -> # Return cycles and notifications - do NOT send notifications here # They will be sent by the caller (e.g., via after_action hook) @@ -235,33 +222,21 @@ defmodule Mv.MembershipFees.CycleGenerator do # Private functions - defp load_member(member_id, opts) do - actor = Keyword.get(opts, :actor) - - query = - Member - |> Ash.Query.filter(id == ^member_id) - |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) - - result = - if actor do - Ash.read_one(query, actor: actor) - else - Ash.read_one(query) - end - - case result do + defp load_member(member_id) do + Member + |> Ash.Query.filter(id == ^member_id) + |> Ash.Query.load([:membership_fee_type, :membership_fee_cycles]) + |> Ash.read_one() + |> case do {:ok, nil} -> {:error, :member_not_found} {:ok, member} -> {:ok, member} {:error, reason} -> {:error, reason} end end - defp do_generate_cycles(member, today, opts) do - actor = Keyword.get(opts, :actor) - + defp do_generate_cycles(member, today) do # Reload member with relationships to ensure fresh data - case load_member(member.id, actor: actor) do + case load_member(member.id) do {:ok, member} -> cond do is_nil(member.membership_fee_type_id) -> @@ -271,7 +246,7 @@ defmodule Mv.MembershipFees.CycleGenerator do {:error, :no_join_date} true -> - generate_missing_cycles(member, today, actor: actor) + generate_missing_cycles(member, today) end {:error, reason} -> @@ -279,8 +254,7 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp generate_missing_cycles(member, today, opts) do - actor = Keyword.get(opts, :actor) + defp generate_missing_cycles(member, today) do fee_type = member.membership_fee_type interval = fee_type.interval amount = fee_type.amount @@ -296,7 +270,7 @@ defmodule Mv.MembershipFees.CycleGenerator do # Only generate if start_date <= end_date if start_date && Date.compare(start_date, end_date) != :gt do cycle_starts = generate_cycle_starts(start_date, end_date, interval) - create_cycles(cycle_starts, member.id, fee_type.id, amount, actor: actor) + create_cycles(cycle_starts, member.id, fee_type.id, amount) else {:ok, [], []} end @@ -391,8 +365,7 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp create_cycles(cycle_starts, member_id, fee_type_id, amount, opts) do - actor = Keyword.get(opts, :actor) + defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do # Always use return_notifications?: true to collect notifications # Notifications will be returned to the caller, who is responsible for # sending them (e.g., via after_action hook returning {:ok, result, notifications}) @@ -407,7 +380,7 @@ defmodule Mv.MembershipFees.CycleGenerator do } handle_cycle_creation_result( - Ash.create(MembershipFeeCycle, attrs, return_notifications?: true, actor: actor), + Ash.create(MembershipFeeCycle, attrs, return_notifications?: true), cycle_start ) end) diff --git a/lib/mv_web.ex b/lib/mv_web.ex index 08a3c23..8589be1 100644 --- a/lib/mv_web.ex +++ b/lib/mv_web.ex @@ -17,7 +17,7 @@ defmodule MvWeb do those modules here. """ - def static_paths, do: ~w(assets fonts images favicon.ico robots.txt templates) + def static_paths, do: ~w(assets fonts images favicon.ico robots.txt) def router do quote do diff --git a/lib/mv_web/live/custom_field_live/form_component.ex b/lib/mv_web/live/custom_field_live/form_component.ex index 54ef4a7..71bdd03 100644 --- a/lib/mv_web/live/custom_field_live/form_component.ex +++ b/lib/mv_web/live/custom_field_live/form_component.ex @@ -91,9 +91,7 @@ defmodule MvWeb.CustomFieldLive.FormComponent do @impl true def handle_event("save", %{"custom_field" => custom_field_params}, socket) do - actor = MvWeb.LiveHelpers.current_actor(socket) - - case MvWeb.LiveHelpers.submit_form(socket.assigns.form, custom_field_params, actor) do + case AshPhoenix.Form.submit(socket.assigns.form, params: custom_field_params) do {:ok, custom_field} -> action = case socket.assigns.form.source.type do diff --git a/lib/mv_web/live/custom_field_value_live/form.ex b/lib/mv_web/live/custom_field_value_live/form.ex index 599ce1d..4db2bed 100644 --- a/lib/mv_web/live/custom_field_value_live/form.ex +++ b/lib/mv_web/live/custom_field_value_live/form.ex @@ -32,9 +32,6 @@ defmodule MvWeb.CustomFieldValueLive.Form do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] - @impl true def render(assigns) do ~H""" @@ -175,9 +172,8 @@ defmodule MvWeb.CustomFieldValueLive.Form do page_title = action <> " " <> "Custom field value" # Load all CustomFields and Members for the selection fields - actor = current_actor(socket) - custom_fields = Ash.read!(Mv.Membership.CustomField, actor: actor) - members = Ash.read!(Mv.Membership.Member, actor: actor) + custom_fields = Ash.read!(Mv.Membership.CustomField) + members = Ash.read!(Mv.Membership.Member) {:ok, socket @@ -228,9 +224,7 @@ defmodule MvWeb.CustomFieldValueLive.Form do custom_field_value_params end - actor = current_actor(socket) - - case submit_form(socket.assigns.form, updated_params, actor) do + case AshPhoenix.Form.submit(socket.assigns.form, params: updated_params) do {:ok, custom_field_value} -> notify_parent({:saved, custom_field_value}) diff --git a/lib/mv_web/live/custom_field_value_live/index.ex b/lib/mv_web/live/custom_field_value_live/index.ex index eea578e..b52fd96 100644 --- a/lib/mv_web/live/custom_field_value_live/index.ex +++ b/lib/mv_web/live/custom_field_value_live/index.ex @@ -23,9 +23,6 @@ defmodule MvWeb.CustomFieldValueLive.Index do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - import MvWeb.LiveHelpers, only: [current_actor: 1] - @impl true def render(assigns) do ~H""" @@ -73,85 +70,17 @@ defmodule MvWeb.CustomFieldValueLive.Index do @impl true def mount(_params, _session, socket) do - actor = current_actor(socket) - - # Early return if no actor (prevents exceptions in unauthenticated tests) - if is_nil(actor) do - {:ok, - socket - |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, [])} - else - case Ash.read(Mv.Membership.CustomFieldValue, actor: actor) do - {:ok, custom_field_values} -> - {:ok, - socket - |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, custom_field_values)} - - {:error, %Ash.Error.Forbidden{}} -> - {:ok, - socket - |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, []) - |> put_flash(:error, gettext("You do not have permission to view custom field values"))} - - {:error, error} -> - {:ok, - socket - |> assign(:page_title, "Listing Custom field values") - |> stream(:custom_field_values, []) - |> put_flash(:error, format_error(error))} - end - end + {:ok, + socket + |> assign(:page_title, "Listing Custom field values") + |> stream(:custom_field_values, Ash.read!(Mv.Membership.CustomFieldValue))} end @impl true def handle_event("delete", %{"id" => id}, socket) do - actor = MvWeb.LiveHelpers.current_actor(socket) + custom_field_value = Ash.get!(Mv.Membership.CustomFieldValue, id) + Ash.destroy!(custom_field_value) - case Ash.get(Mv.Membership.CustomFieldValue, id, actor: actor) do - {:ok, custom_field_value} -> - case Ash.destroy(custom_field_value, actor: actor) do - :ok -> - {:noreply, - socket - |> stream_delete(:custom_field_values, custom_field_value) - |> put_flash(:info, gettext("Custom field value deleted successfully"))} - - {:error, %Ash.Error.Forbidden{}} -> - {:noreply, - put_flash( - socket, - :error, - gettext("You do not have permission to delete this custom field value") - )} - - {:error, error} -> - {:noreply, put_flash(socket, :error, format_error(error))} - end - - {:error, %Ash.Error.Query.NotFound{}} -> - {:noreply, put_flash(socket, :error, gettext("Custom field value not found"))} - - {:error, %Ash.Error.Forbidden{} = _error} -> - {:noreply, - put_flash( - socket, - :error, - gettext("You do not have permission to access this custom field value") - )} - - {:error, error} -> - {:noreply, put_flash(socket, :error, format_error(error))} - end - end - - defp format_error(%Ash.Error.Invalid{errors: errors}) do - Enum.map_join(errors, ", ", fn %{message: message} -> message end) - end - - defp format_error(error) do - inspect(error) + {:noreply, stream_delete(socket, :custom_field_values, custom_field_value)} end end diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 97fd81e..a4332d3 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -90,9 +90,7 @@ defmodule MvWeb.GlobalSettingsLive do @impl true def handle_event("save", %{"setting" => setting_params}, socket) do - actor = MvWeb.LiveHelpers.current_actor(socket) - - case MvWeb.LiveHelpers.submit_form(socket.assigns.form, setting_params, actor) do + case AshPhoenix.Form.submit(socket.assigns.form, params: setting_params) do {:ok, _updated_settings} -> # Reload settings from database to ensure all dependent data is updated {:ok, fresh_settings} = Membership.get_settings() diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 3c68b21..0a05e1f 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -21,10 +21,6 @@ defmodule MvWeb.MemberLive.Form do """ use MvWeb, :live_view - on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} - - import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] - alias Mv.MembershipFees alias Mv.MembershipFees.MembershipFeeType alias MvWeb.Helpers.MembershipFeeHelpers @@ -176,7 +172,7 @@ defmodule MvWeb.MemberLive.Form do