From 2f83f35bcc4c87805136b94d1ba30fff2e9f51b6 Mon Sep 17 00:00:00 2001 From: Moritz Date: Mon, 15 Dec 2025 13:12:17 +0100 Subject: [PATCH] fix: address code review points for cycle regeneration 1. Fix critical notifications bug 2. Fix today inconsistency 3. Add advisory lock around deletion 4. Improve helper function documentation 5. Improve error message UX --- lib/membership/member.ex | 52 ++++++++++++++++--- .../changes/validate_same_interval.ex | 5 +- lib/mv/membership_fees/cycle_generator.ex | 39 ++++++++++---- 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 8e67570..234fc6f 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -588,8 +588,14 @@ defmodule Mv.Membership.Member do def show_in_overview?(_), do: true # Helper functions for cycle status calculations + # + # These functions expect membership_fee_cycles to be loaded with membership_fee_type + # preloaded. The calculations explicitly load this relationship, but if called + # directly, ensure membership_fee_type is loaded or the functions will return + # nil/[] when membership_fee_type is missing. @doc false + @spec get_current_cycle(Member.t()) :: MembershipFeeCycle.t() | nil def get_current_cycle(member) do today = Date.utc_today() @@ -619,6 +625,7 @@ defmodule Mv.Membership.Member do end @doc false + @spec get_last_completed_cycle(Member.t()) :: MembershipFeeCycle.t() | nil def get_last_completed_cycle(member) do today = Date.utc_today() @@ -664,6 +671,7 @@ defmodule Mv.Membership.Member do end @doc false + @spec get_overdue_cycles(Member.t()) :: [MembershipFeeCycle.t()] def get_overdue_cycles(member) do today = Date.utc_today() @@ -695,10 +703,40 @@ defmodule Mv.Membership.Member do # Regenerates cycles when membership fee type changes # Deletes future unpaid cycles and regenerates them with the new type/amount + # Uses advisory lock to prevent concurrent modifications defp regenerate_cycles_on_type_change(member) do require Ash.Query + alias Mv.Repo today = Date.utc_today() + lock_key = :erlang.phash2(member.id) + + # Use advisory lock to prevent concurrent deletion and regeneration + # This ensures atomicity when multiple updates happen simultaneously + if Repo.in_transaction?() do + # Already in transaction: use advisory lock directly + Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) + do_regenerate_cycles_on_type_change(member, today) + else + # Not in transaction: start new transaction with advisory lock + Repo.transaction(fn -> + Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key]) + + case do_regenerate_cycles_on_type_change(member, today) do + :ok -> :ok + {:error, reason} -> Repo.rollback(reason) + end + end) + |> case do + {:ok, result} -> result + {:error, reason} -> {:error, reason} + end + end + end + + # Performs the actual cycle deletion and regeneration + defp do_regenerate_cycles_on_type_change(member, today) do + require Ash.Query # Find all unpaid cycles for this member # We need to check cycle_end for each cycle using its own interval @@ -711,7 +749,7 @@ defmodule Mv.Membership.Member 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) + delete_and_regenerate_cycles(cycles_to_delete, member.id, today) {:error, reason} -> {:error, reason} @@ -736,13 +774,14 @@ defmodule Mv.Membership.Member do end # Deletes future cycles and regenerates them with the new type/amount - defp delete_and_regenerate_cycles(cycles_to_delete, member_id) do + # Passes today to ensure consistent date across deletion and regeneration + defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today) do if Enum.empty?(cycles_to_delete) do # No cycles to delete, just regenerate - regenerate_cycles(member_id) + regenerate_cycles(member_id, today) else case delete_cycles(cycles_to_delete) do - :ok -> regenerate_cycles(member_id) + :ok -> regenerate_cycles(member_id, today) {:error, reason} -> {:error, reason} end end @@ -764,8 +803,9 @@ defmodule Mv.Membership.Member do # Regenerates cycles with new type/amount # CycleGenerator detects if already in transaction and uses advisory lock accordingly - defp regenerate_cycles(member_id) do - case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member_id) do + # Passes today to ensure consistent date across deletion and regeneration + defp regenerate_cycles(member_id, today) do + case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member_id, today: today) do {:ok, _cycles} -> :ok {:error, reason} -> {:error, reason} end diff --git a/lib/membership_fees/changes/validate_same_interval.ex b/lib/membership_fees/changes/validate_same_interval.ex index e41441d..8c1efb4 100644 --- a/lib/membership_fees/changes/validate_same_interval.ex +++ b/lib/membership_fees/changes/validate_same_interval.ex @@ -116,7 +116,10 @@ defmodule Mv.MembershipFees.Changes.ValidateSameInterval do # Add validation error when types cannot be loaded defp add_type_validation_error(changeset, _reason) do - message = "Could not validate membership fee type intervals: type not found" + message = + "Could not validate membership fee type intervals. " <> + "The current or new membership fee type no longer exists. " <> + "This may indicate a data consistency issue." Ash.Changeset.add_error( changeset, diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index 7601380..f2539c0 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -216,14 +216,16 @@ defmodule Mv.MembershipFees.CycleGenerator do end # Execute function within transaction and return normalized result + # When not in transaction, create_cycles returns {:ok, cycles, notifications} + # When in transaction, create_cycles returns {:ok, cycles} (notifications handled by Ash) defp execute_within_transaction(fun) do case fun.() do {:ok, result, notifications} when is_list(notifications) -> - # Return result and notifications separately + # Return result and notifications separately (not in transaction case) {result, notifications} {:ok, result} -> - # Handle case where no notifications were returned (backward compatibility) + # In transaction case: notifications handled by Ash automatically {result, []} {:error, reason} -> @@ -232,9 +234,11 @@ defmodule Mv.MembershipFees.CycleGenerator do end # Normalize function result to consistent format + # When in transaction, create_cycles returns {:ok, cycles} (notifications handled by Ash) + # When not in transaction, create_cycles returns {:ok, cycles, notifications} defp normalize_fun_result({:ok, result, _notifications}) do - # Notifications will be sent after the outer transaction commits - # Return in same format as non-transaction case for consistency + # This case should not occur when in transaction (create_cycles handles it) + # But handle it for safety {:ok, result} end @@ -384,6 +388,10 @@ defmodule Mv.MembershipFees.CycleGenerator do end defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do + # If already in a transaction, let Ash handle notifications automatically + # Otherwise, return notifications to send them after transaction commits + return_notifications? = not Repo.in_transaction?() + results = Enum.map(cycle_starts, fn cycle_start -> attrs = %{ @@ -394,10 +402,15 @@ defmodule Mv.MembershipFees.CycleGenerator do status: :unpaid } - # Return notifications to avoid warnings when creating within a transaction - case Ash.create(MembershipFeeCycle, attrs, return_notifications?: true) do - {:ok, cycle, notifications} -> {:ok, cycle, notifications} - {:error, reason} -> {:error, {cycle_start, reason}} + case Ash.create(MembershipFeeCycle, attrs, return_notifications?: return_notifications?) do + {:ok, cycle, notifications} when is_list(notifications) -> + {:ok, cycle, notifications} + + {:ok, cycle} -> + {:ok, cycle, []} + + {:error, reason} -> + {:error, {cycle_start, reason}} end end) @@ -408,8 +421,14 @@ defmodule Mv.MembershipFees.CycleGenerator do if Enum.empty?(errors) do successful_cycles = Enum.map(successes, fn {:ok, cycle, _notifications} -> cycle end) - # Return cycles and notifications to be sent after transaction commits - {:ok, successful_cycles, all_notifications} + + if return_notifications? do + # Return cycles and notifications to be sent after transaction commits + {:ok, successful_cycles, all_notifications} + else + # Notifications are handled automatically by Ash when in transaction + {:ok, successful_cycles} + end else Logger.warning("Some cycles failed to create: #{inspect(errors)}") # Return partial failure with errors