fix: convert after_action to after_transaction for cycle generation

Replace after_action hooks with after_transaction to ensure async tasks only run after successful commit. Extract common cycle generation logic into handle_cycle_generation/2 to reduce duplication. Add structured error logging with context.

lib/membership/member.ex

@@ -113,54 +113,18 @@ 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_action(fn _changeset, member, _context ->
+      change after_transaction(fn _changeset, result, _context ->
-               if member.membership_fee_type_id && member.join_date do
+               case result do
-                 if Application.get_env(:mv, :sql_sandbox, false) do
+                 {:ok, member} ->
-                   # Run synchronously in test environment for DB sandbox compatibility
+                   if member.membership_fee_type_id && member.join_date do
-                   # Use skip_lock?: true to avoid nested transactions (after_action runs within action transaction)
+                     handle_cycle_generation(member)
-                   # 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, reason} ->
+                 {:error, _} ->
-                         require Logger
+                   :ok
-
-                         Logger.warning(
-                           "Failed to generate cycles for member #{member.id}: #{inspect(reason)}"
-                         )
-                     end
-                   end)
-
-                   {:ok, member}
-                 end
-               else
-                 {:ok, member}
                end
+
+               result
              end)
     end
 
@@ -247,57 +211,23 @@ defmodule Mv.Membership.Member do
       # 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
-      change after_action(fn changeset, member, _context ->
+      # If both join_date and exit_date are changed simultaneously, this hook runs only once
-               join_date_changed = Ash.Changeset.changing_attribute?(changeset, :join_date)
+      # (Ash ensures each after_transaction hook runs once per action, regardless of how many attributes changed)
-               exit_date_changed = Ash.Changeset.changing_attribute?(changeset, :exit_date)
+      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
+                   if (join_date_changed || exit_date_changed) && member.membership_fee_type_id do
-                 if Application.get_env(:mv, :sql_sandbox, false) do
+                     handle_cycle_generation(member)
-                   # 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 regenerate 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, reason} ->
+                 {:error, _} ->
-                         require Logger
+                   :ok
-
-                         Logger.warning(
-                           "Failed to regenerate cycles for member #{member.id}: #{inspect(reason)}"
-                         )
-                     end
-                   end)
-
-                   {:ok, member}
-                 end
-               else
-                 {:ok, member}
                end
+
+               result
              end)
     end
 
@@ -452,6 +382,11 @@ defmodule Mv.Membership.Member do
       end
     end
 
+    # Join date not in future
+    validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0),
+      where: [present(:join_date)],
+      message: "cannot be in the future"
+
     # Exit date not before join date
     validate compare(:exit_date, greater_than: :join_date),
       where: [present([:join_date, :exit_date])],
@@ -919,6 +854,90 @@ 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