refactor: reduce complexity of with_advisory_lock function

Split the complex with_advisory_lock function into smaller, focused
functions to improve readability and reduce cyclomatic complexity:

- with_advisory_lock_in_transaction: Handles case when already in transaction
- with_advisory_lock_new_transaction: Handles case when starting new transaction
- execute_within_transaction: Executes function within transaction
- normalize_fun_result: Normalizes function results to consistent format
- handle_transaction_result: Handles transaction results and sends notifications

This fixes Credo warnings:
- Function body nested too deep (was 3, now max 2)
- Function too complex (was 11, now max 9)

lib/membership/member.ex

@@ -588,14 +588,8 @@ 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()
 
@@ -625,7 +619,6 @@ 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()
 
@@ -671,7 +664,6 @@ 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()
 
@@ -703,52 +695,10 @@ 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
-    alias Mv.Repo
+    require Ash.Query
 
     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
-      regenerate_cycles_in_transaction(member, today, lock_key)
-    else
-      regenerate_cycles_new_transaction(member, today, lock_key)
-    end
-  end
-
-  # Already in transaction: use advisory lock directly
-  defp regenerate_cycles_in_transaction(member, today, lock_key) do
-    alias Mv.Repo
-
-    Ecto.Adapters.SQL.query!(Repo, "SELECT pg_advisory_xact_lock($1)", [lock_key])
-    do_regenerate_cycles_on_type_change(member, today)
-  end
-
-  # Not in transaction: start new transaction with advisory lock
-  defp regenerate_cycles_new_transaction(member, today, lock_key) do
-    alias Mv.Repo
-
-    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)
-    |> handle_transaction_result()
-  end
-
-  # Handle transaction result
-  defp handle_transaction_result({:ok, result}), do: result
-  defp handle_transaction_result({:error, reason}), do: {:error, reason}
-
-  # 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
@@ -761,7 +711,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, today)
+        delete_and_regenerate_cycles(cycles_to_delete, member.id)
 
       {:error, reason} ->
         {:error, reason}
@@ -786,14 +736,13 @@ defmodule Mv.Membership.Member do
   end
 
   # Deletes future cycles and regenerates them with the new type/amount
-  # Passes today to ensure consistent date across deletion and regeneration
-  defp delete_and_regenerate_cycles(cycles_to_delete, member_id, today) do
+  defp delete_and_regenerate_cycles(cycles_to_delete, member_id) do
     if Enum.empty?(cycles_to_delete) do
       # No cycles to delete, just regenerate
-      regenerate_cycles(member_id, today)
+      regenerate_cycles(member_id)
     else
       case delete_cycles(cycles_to_delete) do
-        :ok -> regenerate_cycles(member_id, today)
+        :ok -> regenerate_cycles(member_id)
         {:error, reason} -> {:error, reason}
       end
     end
@@ -815,9 +764,8 @@ defmodule Mv.Membership.Member do
 
   # Regenerates cycles with new type/amount
   # CycleGenerator detects if already in transaction and uses advisory lock accordingly
-  # 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
+  defp regenerate_cycles(member_id) do
+    case Mv.MembershipFees.CycleGenerator.generate_cycles_for_member(member_id) do
       {:ok, _cycles} -> :ok
       {:error, reason} -> {:error, reason}
     end

lib/membership_fees/changes/validate_same_interval.ex

@@ -116,10 +116,7 @@ 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. " <>
-        "The current or new membership fee type no longer exists. " <>
-        "This may indicate a data consistency issue."
+    message = "Could not validate membership fee type intervals: type not found"
 
     Ash.Changeset.add_error(
       changeset,

lib/mv/membership_fees/cycle_generator.ex

@@ -216,16 +216,14 @@ defmodule Mv.MembershipFees.CycleGenerator do
   end
 
   # Execute function within transaction and return normalized result
-  # When in transaction, create_cycles returns {:ok, cycles, notifications}
-  # When not in transaction, create_cycles returns {:ok, cycles}
   defp execute_within_transaction(fun) do
     case fun.() do
       {:ok, result, notifications} when is_list(notifications) ->
-        # In transaction case: return result and notifications separately
+        # Return result and notifications separately
         {result, notifications}
 
       {:ok, result} ->
-        # Not in transaction case: notifications handled by Ash automatically
+        # Handle case where no notifications were returned (backward compatibility)
         {result, []}
 
       {:error, reason} ->
@@ -234,10 +232,8 @@ defmodule Mv.MembershipFees.CycleGenerator do
   end
 
   # Normalize function result to consistent format
-  # When in transaction, create_cycles returns {:ok, cycles, notifications}
-  # When not in transaction, create_cycles returns {:ok, cycles}
   defp normalize_fun_result({:ok, result, _notifications}) do
-    # In transaction case: notifications will be sent after outer transaction commits
+    # Notifications will be sent after the outer transaction commits
     # Return in same format as non-transaction case for consistency
     {:ok, result}
   end
@@ -388,11 +384,6 @@ defmodule Mv.MembershipFees.CycleGenerator do
   end
 
   defp create_cycles(cycle_starts, member_id, fee_type_id, amount) do
-    # Always return notifications when in a transaction (required by Ash)
-    # When not in transaction, Ash handles notifications automatically
-    # When in transaction, we must return notifications and send them after commit
-    return_notifications? = Repo.in_transaction?()
-
     results =
       Enum.map(cycle_starts, fn cycle_start ->
         attrs = %{
@@ -403,15 +394,10 @@ defmodule Mv.MembershipFees.CycleGenerator do
           status: :unpaid
         }
 
-        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}}
+        # 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}}
         end
       end)
 
@@ -422,14 +408,8 @@ defmodule Mv.MembershipFees.CycleGenerator do
 
     if Enum.empty?(errors) do
       successful_cycles = Enum.map(successes, fn {:ok, cycle, _notifications} -> cycle end)
-
-      if return_notifications? do
       # Return cycles and notifications to be sent after transaction commits
       {:ok, successful_cycles, all_notifications}
-      else
-        # Not in transaction: Ash handles notifications automatically
-        {:ok, successful_cycles}
-      end
     else
       Logger.warning("Some cycles failed to create: #{inspect(errors)}")
       # Return partial failure with errors