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/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