diff --git a/lib/membership_fees/cycle_end_backfill.ex b/lib/membership_fees/cycle_end_backfill.ex new file mode 100644 index 00000000..1d739dad --- /dev/null +++ b/lib/membership_fees/cycle_end_backfill.ex @@ -0,0 +1,53 @@ +defmodule Mv.MembershipFees.CycleEndBackfill do + @moduledoc """ + One-time backfill for the denormalized `cycle_end` column on + `membership_fee_cycles`. + + For every cycle whose `cycle_end` is still `NULL`, sets it to + `CalendarCycles.calculate_cycle_end(cycle_start, interval)` using the cycle's + own fee-type interval. Idempotent: rows that already have a `cycle_end` are + left untouched, so it is safe to re-run. + + Invoked from the `cycle_end` migration. Kept as a module function (rather than + inline migration SQL) so the exact calendar arithmetic is reused from + `CalendarCycles` and can be tested directly. + """ + + import Ecto.Query + + alias Mv.MembershipFees.CalendarCycles + alias Mv.Repo + + @doc """ + Backfills `cycle_end` for all cycles missing it. Returns `{:ok, updated_count}`. + """ + @spec run() :: {:ok, non_neg_integer()} + def run do + rows = + from(c in "membership_fee_cycles", + join: t in "membership_fee_types", + on: c.membership_fee_type_id == t.id, + where: is_nil(c.cycle_end), + select: {c.id, c.cycle_start, t.interval} + ) + |> Repo.all() + + count = + Enum.reduce(rows, 0, fn {id, cycle_start, interval}, acc -> + cycle_end = CalendarCycles.calculate_cycle_end(cycle_start, normalize_interval(interval)) + + from(c in "membership_fee_cycles", where: c.id == ^id) + |> Repo.update_all(set: [cycle_end: cycle_end]) + + acc + 1 + end) + + {:ok, count} + end + + # The raw query returns the enum column as a string. + defp normalize_interval(interval) when is_atom(interval), do: interval + + defp normalize_interval(interval) when is_binary(interval), + do: String.to_existing_atom(interval) +end diff --git a/lib/membership_fees/membership_fee_cycle.ex b/lib/membership_fees/membership_fee_cycle.ex index cd628871..cb881938 100644 --- a/lib/membership_fees/membership_fee_cycle.ex +++ b/lib/membership_fees/membership_fee_cycle.ex @@ -13,7 +13,9 @@ defmodule Mv.MembershipFees.MembershipFeeCycle do - `notes` - Optional notes for this cycle ## Design Decisions - - **No cycle_end field**: Calculated from cycle_start + interval (from fee type) + - **Denormalized cycle_end**: Set at create from cycle_start + interval (fee type). + Because the fee type interval and the cycle_start are immutable after create, + cycle_end never drifts, so it can be filtered/sorted in SQL. - **Amount stored per cycle**: Preserves historical amounts when fee type changes - **Calendar-aligned cycles**: All cycles start on calendar boundaries @@ -46,6 +48,14 @@ defmodule Mv.MembershipFees.MembershipFeeCycle do create :create do primary? true accept [:cycle_start, :amount, :status, :notes, :member_id, :membership_fee_type_id] + + argument :interval, :atom do + allow_nil? true + + description "Optional pre-resolved fee-type interval; when given, SetCycleEnd skips the fee-type lookup" + end + + change Mv.MembershipFees.MembershipFeeCycle.Changes.SetCycleEnd end update :update do @@ -106,6 +116,13 @@ defmodule Mv.MembershipFees.MembershipFeeCycle do description "Start date of the billing cycle" end + attribute :cycle_end, :date do + allow_nil? false + public? true + + description "Denormalized end date of the cycle (cycle_start + interval), set at create" + end + attribute :amount, :decimal do allow_nil? false public? true diff --git a/lib/membership_fees/membership_fee_cycle/changes/set_cycle_end.ex b/lib/membership_fees/membership_fee_cycle/changes/set_cycle_end.ex new file mode 100644 index 00000000..296f952c --- /dev/null +++ b/lib/membership_fees/membership_fee_cycle/changes/set_cycle_end.ex @@ -0,0 +1,68 @@ +defmodule Mv.MembershipFees.MembershipFeeCycle.Changes.SetCycleEnd do + @moduledoc """ + Ash change that denormalizes `cycle_end` on a membership fee cycle at create time. + + `cycle_end` is computed as `CalendarCycles.calculate_cycle_end(cycle_start, interval)`, + where `interval` is read from the cycle's `membership_fee_type`. Because a fee type's + `interval` is immutable and the cycle's `cycle_start` is never updated, the stored + `cycle_end` is fixed at create time and never drifts. + + When the caller already knows the interval (e.g. the bulk cycle generator, which + resolves the fee type once for a whole batch), it may pass it via the `:interval` + create argument; the change then uses that value and issues no fee-type query, + avoiding an N+1 lookup across a batch of cycles. + + Otherwise the fee-type interval is read with `authorize?: false`: it is an internal + lookup of immutable reference data needed to populate a derived column, not a + user-facing read, and cycle creation is itself a systemic operation. Reading it under + the caller's actor is unreliable here (the actor is not always present in the change + context, e.g. when a changeset is built with `for_create/3` and the actor is only + supplied to `Ash.create/2`). + + When `cycle_start` or `membership_fee_type_id` are missing the changeset is returned + unchanged so the normal `allow_nil?` validations surface the error. + """ + use Ash.Resource.Change + + alias Mv.MembershipFees.CalendarCycles + alias Mv.MembershipFees.MembershipFeeType + + @impl true + def change(changeset, _opts, _context) do + with {:ok, cycle_start} <- fetch(changeset, :cycle_start), + {:ok, fee_type_id} <- fetch(changeset, :membership_fee_type_id), + {:ok, interval} <- resolve_interval(changeset, fee_type_id) do + cycle_end = CalendarCycles.calculate_cycle_end(cycle_start, interval) + Ash.Changeset.force_change_attribute(changeset, :cycle_end, cycle_end) + else + _ -> changeset + end + end + + defp fetch(changeset, field) do + case Ash.Changeset.fetch_change(changeset, field) do + {:ok, value} when not is_nil(value) -> + {:ok, value} + + _ -> + case Map.get(changeset.data, field) do + nil -> :error + value -> {:ok, value} + end + end + end + + defp resolve_interval(changeset, fee_type_id) do + case Ash.Changeset.get_argument(changeset, :interval) do + nil -> fetch_interval(fee_type_id) + interval -> {:ok, interval} + end + end + + defp fetch_interval(fee_type_id) do + case Ash.get(MembershipFeeType, fee_type_id, authorize?: false) do + {:ok, %{interval: interval}} -> {:ok, interval} + _ -> :error + end + end +end diff --git a/lib/mv/membership_fees/cycle_generator.ex b/lib/mv/membership_fees/cycle_generator.ex index a6c2d300..8bd76d5e 100644 --- a/lib/mv/membership_fees/cycle_generator.ex +++ b/lib/mv/membership_fees/cycle_generator.ex @@ -305,7 +305,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, opts) + create_cycles(cycle_starts, member.id, fee_type.id, amount, interval, opts) else {:ok, [], []} end @@ -400,7 +400,7 @@ defmodule Mv.MembershipFees.CycleGenerator do end end - defp create_cycles(cycle_starts, member_id, fee_type_id, amount, opts) do + defp create_cycles(cycle_starts, member_id, fee_type_id, amount, interval, opts) do actor = get_actor(opts) create_opts = Helpers.ash_actor_opts(actor) @@ -414,7 +414,10 @@ defmodule Mv.MembershipFees.CycleGenerator do member_id: member_id, membership_fee_type_id: fee_type_id, amount: amount, - status: :unpaid + status: :unpaid, + # Pass the already-resolved interval so SetCycleEnd does not re-fetch the + # fee type for every cycle in the batch (avoids an N+1 lookup). + interval: interval } handle_cycle_creation_result( diff --git a/priv/repo/migrations/20260630163238_add_cycle_end_to_membership_fee_cycles.exs b/priv/repo/migrations/20260630163238_add_cycle_end_to_membership_fee_cycles.exs new file mode 100644 index 00000000..5cabc9a5 --- /dev/null +++ b/priv/repo/migrations/20260630163238_add_cycle_end_to_membership_fee_cycles.exs @@ -0,0 +1,32 @@ +defmodule Mv.Repo.Migrations.AddCycleEndToMembershipFeeCycles do + @moduledoc """ + Adds the denormalized `cycle_end` column to `membership_fee_cycles`. + + The column is added nullable, backfilled from `cycle_start + interval` for every + existing row (via `Mv.MembershipFees.CycleEndBackfill`), and only then made + `NOT NULL`. Going forward the column is populated at create time by the + `SetCycleEnd` change. + """ + + use Ecto.Migration + + def up do + alter table(:membership_fee_cycles) do + add :cycle_end, :date + end + + flush() + + {:ok, _count} = Mv.MembershipFees.CycleEndBackfill.run() + + alter table(:membership_fee_cycles) do + modify :cycle_end, :date, null: false + end + end + + def down do + alter table(:membership_fee_cycles) do + remove :cycle_end + end + end +end diff --git a/priv/resource_snapshots/repo/membership_fee_cycles/20260630163240.json b/priv/resource_snapshots/repo/membership_fee_cycles/20260630163240.json new file mode 100644 index 00000000..e2646f6e --- /dev/null +++ b/priv/resource_snapshots/repo/membership_fee_cycles/20260630163240.json @@ -0,0 +1,173 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"uuid_generate_v7()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "cycle_start", + "type": "date" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "cycle_end", + "type": "date" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": 2, + "size": null, + "source": "amount", + "type": "decimal" + }, + { + "allow_nil?": false, + "default": "\"unpaid\"", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "status", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "notes", + "type": "text" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "membership_fee_cycles_member_id_fkey", + "on_delete": null, + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "members" + }, + "scale": null, + "size": null, + "source": "member_id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "membership_fee_cycles_membership_fee_type_id_fkey", + "on_delete": null, + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "membership_fee_types" + }, + "scale": null, + "size": null, + "source": "membership_fee_type_id", + "type": "uuid" + } + ], + "base_filter": null, + "check_constraints": [], + "create_table_options": null, + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "455B6D49C804301DF13F950318644BF28B491535A94F4ABF7805AA1A53287401", + "identities": [ + { + "all_tenants?": false, + "base_filter": null, + "index_name": "membership_fee_cycles_unique_cycle_per_member_index", + "keys": [ + { + "type": "atom", + "value": "member_id" + }, + { + "type": "atom", + "value": "cycle_start" + } + ], + "name": "unique_cycle_per_member", + "nils_distinct?": true, + "where": null + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.Mv.Repo", + "schema": null, + "table": "membership_fee_cycles" +} \ No newline at end of file diff --git a/test/membership_fees/cycle_end_test.exs b/test/membership_fees/cycle_end_test.exs new file mode 100644 index 00000000..23e6ed59 --- /dev/null +++ b/test/membership_fees/cycle_end_test.exs @@ -0,0 +1,98 @@ +defmodule Mv.MembershipFees.CycleEndTest do + @moduledoc """ + Tests for the denormalized `cycle_end` column on membership fee cycles: + the create-time change that sets it, and the one-time backfill helper. + """ + use Mv.DataCase, async: false + + import Mv.Fixtures, only: [create_fee_type: 2, member_fixture_with_actor: 2] + + alias Mv.MembershipFees.CalendarCycles + alias Mv.MembershipFees.CycleEndBackfill + alias Mv.MembershipFees.MembershipFeeCycle + + setup do + %{actor: Mv.Helpers.SystemActor.get_system_actor()} + end + + defp create_member(fee_type, actor) do + member_fixture_with_actor(%{membership_fee_type_id: fee_type.id}, actor) + end + + describe "create sets cycle_end" do + for interval <- [:monthly, :quarterly, :half_yearly, :yearly] do + test "create sets cycle_end from cycle_start + #{interval} interval", %{actor: actor} do + interval = unquote(interval) + fee_type = create_fee_type(%{interval: interval}, actor) + member = create_member(fee_type, actor) + cycle_start = ~D[2024-02-01] + + cycle = + MembershipFeeCycle + |> Ash.Changeset.for_create(:create, %{ + cycle_start: cycle_start, + amount: Decimal.new("50.00"), + member_id: member.id, + membership_fee_type_id: fee_type.id + }) + |> Ash.create!(actor: actor) + + assert cycle.cycle_end == CalendarCycles.calculate_cycle_end(cycle_start, interval) + end + end + end + + describe "create with interval argument" do + test "uses the passed interval and skips the fee-type lookup", %{actor: actor} do + # Fee type carries :yearly, but the caller passes :monthly explicitly. + # If the interval argument is honored (no per-row fee-type lookup), the + # stored cycle_end reflects the argument, not the fee type's interval. + fee_type = create_fee_type(%{interval: :yearly}, actor) + member = create_member(fee_type, actor) + cycle_start = ~D[2024-02-01] + + cycle = + MembershipFeeCycle + |> Ash.Changeset.for_create(:create, %{ + cycle_start: cycle_start, + amount: Decimal.new("50.00"), + member_id: member.id, + membership_fee_type_id: fee_type.id, + interval: :monthly + }) + |> Ash.create!(actor: actor) + + assert cycle.cycle_end == CalendarCycles.calculate_cycle_end(cycle_start, :monthly) + end + end + + describe "backfill" do + test "fills cycle_end for pre-existing rows matching calculate_cycle_end/2", %{actor: actor} do + fee_type = create_fee_type(%{interval: :quarterly}, actor) + member = create_member(fee_type, actor) + + cycle = + MembershipFeeCycle + |> Ash.Changeset.for_create(:create, %{ + cycle_start: ~D[2024-04-01], + amount: Decimal.new("50.00"), + member_id: member.id, + membership_fee_type_id: fee_type.id + }) + |> Ash.create!(actor: actor) + + # Simulate a pre-existing row that predates the cycle_end column. + Repo.query!("ALTER TABLE membership_fee_cycles ALTER COLUMN cycle_end DROP NOT NULL") + + Repo.query!("UPDATE membership_fee_cycles SET cycle_end = NULL WHERE id = $1", [ + Ecto.UUID.dump!(cycle.id) + ]) + + assert {:ok, count} = CycleEndBackfill.run() + assert count >= 1 + + reloaded = Ash.get!(MembershipFeeCycle, cycle.id, actor: actor) + assert reloaded.cycle_end == CalendarCycles.calculate_cycle_end(~D[2024-04-01], :quarterly) + end + end +end