From 3d6a8d8b6e7c5cfeafbdffbde5b8b6b26eeb3e2b Mon Sep 17 00:00:00 2001 From: buzzromain <18685603+buzzromain@users.noreply.github.com> Date: Wed, 26 Aug 2026 07:01:12 +0200 Subject: [PATCH] feat(budgets): move money between envelopes in one gesture (#3164) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(budgets): carry a category's unspent budget into the next month A budget category resets to zero every month, so anything non-monthly (annual insurance, a holiday fund, car servicing) has no place to accumulate. Two columns on budget_categories turn a category into a real envelope: `rollover_enabled`, opt-in per category and off by default, and `rolled_over_amount`, the surplus carried in from the previous month. rolled_over(n) = rollover_enabled ? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1)) : 0 v1 floors at zero: only a surplus carries, never an overspend. The amount is materialized, not derived. March depends on February which depends on January, so computing it on read would walk the whole chain on every budget render. Budget::RolloverCalculator recomputes it in a single forward pass and writes once via upsert_all, from Budget.find_or_bootstrap and from BudgetCategoriesController#update -- allocations and the toggle being the only inputs. No Transaction hook: a past month's actuals can change after the fact, and the page load is a fine moment to catch up. Scope kept deliberately narrow. `Budget#budgeted_spending`, `#allocated_spending` and `#available_to_allocate` are untouched -- the top of the budget page still answers "I planned to spend X, I've allocated Y". The carry is per-envelope information, surfaced as `Budget#total_rolled_over` and never folded into those totals. What the carry does change is consumption: `available_to_spend`, `percent_of_budget_spent` and `budgeted?` all count it, or a category funded entirely by rollover would read as unbudgeted and get an alert pill while it still had money left. `display_budgeted_spending` stays the month's allocation alone -- the card shows the two figures side by side. Details worth knowing: - A parent's carry is net of its ring-fenced subcategories'. A parent's allocation already contains theirs and its actuals already contain their spending; those subcategories carry their own surplus, so counting the parent's raw leftover would roll the same money over twice. - Chains never mix: household with household, a member's personal budgets with their own. A missing month is a gap the carry crosses, not a month budgeted at zero. - The carry stops at a currency change. sync_budget_categories stamps categories with family.currency at sync time while a budget freezes its own at creation, so the guard is on budget_category.currency -- the unit the amount is actually denominated in. - upsert_all writes with `update_only`, so a concurrent request that moves an allocation between our read and our write doesn't get it clobbered by the stale value we loaded. - copy_from! copies the toggle, never the amount. Cost for families that never turn it on: one EXISTS query per budget page load, measured, including on the reports page which also bootstraps a budget. With rollover on, the walk starts at the first month that uses it rather than at the two-year history bound. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z * fix(budgets): pin the household rollover chain to a viewer-independent scope Addresses review feedback on #3143. The household budget (user_id NULL) has no owner to scope actuals by, and `IncomeStatement` falls back to `Current.user` when nobody says otherwise. The calculator therefore computed one shared `rolled_over_amount` through whichever member happened to load the page, and each viewer overwrote the other's number -- last one wins, and a member could infer spending in accounts they cannot see. `Budget#income_statement_accounts` can now be overridden, and the calculator pins the household chain to the whole family so the shared row holds one number. Personal chains are untouched: they already scope to their owner's accounts and were always deterministic. `copy_from!` runs after `find_or_bootstrap` has already recomputed the chain, so copying `rollover_enabled` left the target sitting on a zero carry until the next page load. It now recomputes before its transaction commits. The toggle tooltip described the wrong direction. `incoming_carry` checks the flag of the month being computed, so the toggle governs what that month *receives* from the previous one, not what it sends forward. Reworded in English and French. The concurrency regression test now drives its concurrent write through `Budget#budget_category_actual_spending`, a public seam, instead of stubbing a private method of the calculator from another class's test suite. Each guard was confirmed load-bearing by reverting it and watching its test fail. bin/rails test: 6939 runs, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z * fix(budgets): let the rollover choice stand instead of resetting each month `rollover_enabled` lives on budget_categories, one row per (budget, category), so a month created by `find_or_bootstrap` was born with the flag off. Switching rollover on for Vacations in January and simply opening February dropped January's surplus on the floor -- the user had to re-arm the toggle every month, or go through "copy from previous budget". The feature's headline case, a category funded 50/month accumulating over a year, did not work as shipped. New rows now inherit the flag from the last initialized budget of the same owner, the same chain the carry itself walks. Turning the toggle off on a given month still overrides it from there on, so the per-month escape hatch survives. The flag stays on budget_categories rather than moving to Category, which is where comparable products (Monarch, Copilot, Lunch Money) put it. Categories here are family-wide while budgets are per owner, so a category-level flag would force one member's rollover choice onto everyone's personal budget and onto the household budget. budget_categories is the only table carrying both the category and the owner. A regression test covers that isolation. Naming follows the same products: the toggle reads "Rollover", the noun, not "Roll over", the verb -- which also matches `rollover_enabled` and the calculator. Both tooltips now describe the property rather than a direction ("keep this category's unspent money from one month to the next"). The previous wording named the direction the flag actually gates, incoming, which is accurate but the opposite of the mental model every comparable product installs; describing the property is true under either reading. The French card string switched to "+%{amount} de report" so it no longer has to agree in number with a currency noun it cannot see. bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed load-bearing by removing it and watching its tests fail. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z * fix(budgets): make a rollover opt-out stop the money in both directions `incoming_carry` gates what a month receives, but `leftover_for` computed what it sends regardless of the toggle. So switching rollover off for one month and back on the next handed the opted-out month's whole allocation to the month after: the surplus the user meant to forfeit reappeared a month later. Reproduced at 100, where 0 was expected. The outgoing carry is now gated on the same flag, which also skips the actuals lookup for opted-out rows. "Off" now means this envelope does not roll over, in either direction -- the reading the standing toggle and the tooltip both promise. Found by CodeRabbit on #3143. It only became wrong with the standing-choice inheritance in 2b1cff5a: while the flag was per-month, "off" plausibly meant "do not accept", and the previous month's surplus reaching a re-armed month was defensible. Once the flag reads as a property of the envelope, it isn't. bin/rails test: 6943 runs, 0 failures. Confirmed load-bearing by removing the guard and watching the new three-month test fail. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z * fix(budgets): serialize rollover recomputes for a chain with an advisory lock `recompute!` reads the whole chain into memory, walks it, then upserts. Nothing made that atomic: two overlapping recomputes for the same (family, owner) chain could both load it, and the one that started first could land its now-stale `rolled_over_amount` on top of the other's. `update_only` keeps an upsert off allocations, but the carry is the very column this writes, so nothing protected it. The wrong value survived until the next page load recomputed it. The read-then-write now runs inside a transaction holding `pg_advisory_xact_lock` keyed on the chain, and the walk was extracted so the guard is legible. The cheap `first_relevant_budget_date` check still runs first and unlocked, so families that never enabled rollover pay one query and never contend; the date is re-read under the lock because the chain may have moved while waiting. The key names the (family, owner) pair, so a household recompute and a member's personal recompute don't queue behind each other. This reverses the spec's "no advisory lock" guidance, at the request of an upstream maintainer reviewing #3143. On the test: under transactional fixtures a second connection cannot see the data, so a true two-connection interleaving test isn't practical here. The regression test asserts what is observable in-process -- the lock is taken, it is taken before the write, and two chains produce different keys. Removing `lock_chain!` makes it fail. bin/rails test: 6944 runs, 0 failures. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z * feat(api): expose the rollover toggle and carried amount on budget categories `available_to_spend` started counting the carry in this branch, so an API client could receive a category budgeted at 500 with 700 available and nothing in the payload to account for the difference. The two fields that explain it are now serialized. `rollover_enabled` ships with the stored fields, so the summary rendered by the index action carries it. `rolled_over_amount` sits with the derived amounts behind `include_derived_amounts`, next to the `available_to_spend` it accounts for -- the index deliberately omits both, unchanged. Schemas updated in spec/swagger_helper.rb (BudgetCategory and BudgetCategorySummary), docs regenerated with rswag, and behavioural coverage added to the Minitest controller test: the show action returns the toggle and the carry, and the index returns the toggle without the derived amount. Note on docs/api/openapi.yaml: 64 of the 72 added lines are not from this change. The committed file had drifted from what rswag generates -- specs for the merchant CSV import and transfer source fees had been added without regenerating -- and the mandated `rake rswag:specs:swaggerize` picks them up. Verified by regenerating on a clean tree, where those 64 lines appear on their own. Hand-trimming them back out would leave the generated file not matching its generator, so they are included; happy to split them into their own commit if a maintainer prefers. bin/rails test: 6945 runs, 0 failures. ruby test/support/verify_api_endpoint_consistency.rb: OK. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z * feat(budgets): move money between envelopes in one gesture Overspending one category and covering it from another meant editing two allocations by hand, with no atomicity: the budget could sit over-allocated between the two saves, and a failure left it there. `BudgetCategory.move_allocation!` does both sides in one transaction. Deliberately no new table — v1 stores the resulting allocations and keeps no history of the move itself. Refused, each with its own localized message: an amount at or below zero, more than the source has, two categories from different budgets, a category and itself, "Uncategorized" (synthesized on read, it has no row), and — the one that is not obvious — a category and its own direct parent or child. `sync_parent_budgeted_spending!` rebuilds a parent from the sum of its children plus its reserve, so money moved across that boundary would be re-derived away and the total would not be conserved. Lock order is the delicate part. `update_budgeted_spending!` locks its own row and, for a subcategory, its parent, so two simultaneous moves in opposite directions could each hold what the other needs. Every row the operation will touch — both ends and their parents — is locked up front by ascending id. The rollover chain is recomputed by the caller AFTER the move commits, never inside it. `Budget::RolloverCalculator` takes a transaction-scoped advisory lock, and taking it while these row locks are held would invert the order `#update` already established: one request holding rows and waiting for the advisory lock, another holding the advisory lock and waiting for those rows. A model test pins that `move_allocation!` never recomputes on its own. The recompute is not optional. A move is neutral for `Budget#allocated_spending`, but not for the carry: `leftover_for` is budgeted + rolled_over − actual, so moving money changes what both envelopes hand to the next month. UI is one native `` shared by the page rather than one per row, opened from a discreet button on each envelope that has something to give. The Stimulus controller has 6 targets and disables the options the server would refuse anyway, so an impossible move is never offered. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(budgets): use the design-system dialog, and stop a parent lending its children's money Addresses review feedback on #3164. **The move dialog was hand-rolled.** `DS::Dialog` already exists and already carries focus trapping, Escape, click-outside, focus restore and the design-system chrome; rewriting those by hand is how they end up subtly wrong, and the guidelines say to reach for the primitive first. It keeps the one-dialog-for-the-page shape — the list holds dozens of rows and a per-row dialog would be dozens of copies of the same markup — via `auto_open: false` and `disable_frame: true`. **It also stayed open after a successful move,** still showing the previous source and amount. It now closes on `turbo:submit-end`, and only when Turbo reports success: closing on submit alone would hide the reason a move was refused. **Submit was enabled with nowhere to send.** A lone envelope, or one whose only peers are its own parent and children, offered a button whose only outcome was a server error. The form now says so and disables itself. **A parent could send away its children's money.** `budgeted_spending` on a parent already contains its individually funded subcategories' allocations, so comparing against the gross figure let a move spend what a child had ring-fenced. The parent dropped below the sum of its children, and the next edit to any child rebuilt it — the money appeared to teleport back. The movable amount for a parent is now its own reserve. `test "moving the whole allocation is allowed, moving one cent more is not"` moved a parent's gross amount and passed: it encoded that bug. It now uses a leaf as its source, where "the whole allocation" is the whole of it, and the parent boundary gets its own pair of tests. **A negative carry could be written.** The calculator floors it at zero but writes through `upsert_all`, and a negative `rolled_over_amount` would quietly subtract from `available_to_spend`. Now a CHECK constraint, verified by replaying the migration on a throwaway database. bin/rails test: 6967 runs, 28020 assertions, 0 failures. RuboCop, erb_lint, Brakeman and biome clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(budgets): carry the rollover choice into months already open, and mend the API schema Addresses the remaining review feedback on #3164. **Enabling rollover skipped months that already existed.** Inheritance runs when `sync_budget_categories` creates a missing row, so it only ever reaches months that do not exist yet. A user who opened March, then went back to January and switched rollover on, left March sitting at `false` — created before the choice was made, so it had nothing to inherit — and the chain died there. The toggle is a standing choice about the envelope, which is what `inherited_rollover_flags` already says: "turning it off on a given month still overrides it from there on." Applying the choice forward closes the hole without a tri-state column. Later months take the most recent decision, which is the one the user just made; earlier months keep theirs. **`rollover_enabled` was emitted but not required.** The shared partial always sends it in both list and detail responses. Added to the `required` list of `BudgetCategorySummary` and `BudgetCategory` in `spec/swagger_helper.rb`, then regenerated. **`type: file` is not valid OpenAPI 3.0.3.** A Swagger 2.0 leftover in the merchants import spec, which generated clients that send nothing the controller can read. It surfaced now because this branch is the first to regenerate `openapi.yaml` since it was written — `origin/main` has no occurrence of it. Spelled as a string with `format: binary` instead. Regeneration produced a four-line diff, so the checked-in document was already in sync otherwise. bin/rails test: 6969 runs, 28023 assertions, 0 failures. RuboCop and Brakeman clean; 324 rswag examples pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(budgets): stop the API serving a carry the web pages would have refreshed Addresses the remaining P1 on #3164, and its duplicate on #3143. The objection was that nothing recomputes when a sync, an edit or a recategorisation changes spending in an earlier month. On the web that is by design and measured: every surface showing the carry goes through `Budget.find_or_bootstrap`, so it recomputes on the way in, and the alternative — recomputing on every transaction write — buys nothing a page load does not already give. The API is the case that argument does not cover, and the review was right about it. `Api::V1::BudgetCategoriesController` reads `rolled_over_amount` straight off the column, so it was the one surface that could serve a stale carry indefinitely, until somebody happened to open the budget page. It now recomputes the chains it is about to read. A read that writes is a smell, but it is the same bargain the budget page already makes, applied to the surface that was missed: the walk is per family, and the calculator's leading EXISTS makes it a single query that writes nothing for a family that never turned rollover on. Also from review: the move dialog's amount field allowed `min: 0` while `move_allocation!` rejects zero as non-positive. Browser validation now matches the server contract, at the currency step. bin/rails test: 6970 runs, 28025 assertions, 0 failures. Confirmed load-bearing by removing the callback and watching the new API test fail. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * Fix budget rollover schema delta * Remove duplicate rollover test class --------- Signed-off-by: Juan José Mata Co-authored-by: Claude Opus 5 Co-authored-by: Juan José Mata Co-authored-by: sure-admin --- .../api/v1/budget_categories_controller.rb | 22 ++ .../budget_categories_controller.rb | 45 +++- .../controllers/budget_move_controller.js | 79 +++++++ app/models/budget_category.rb | 124 +++++++++++ .../_budget_category_form.html.erb | 20 +- .../budget_categories/_move_dialog.html.erb | 80 +++++++ app/views/budget_categories/index.html.erb | 4 +- .../budget_categories/move.turbo_stream.erb | 29 +++ config/locales/views/budgets/en.yml | 18 ++ config/locales/views/budgets/fr.yml | 18 ++ config/routes.rb | 4 +- ...00000_add_rollover_to_budget_categories.rb | 8 + db/schema.rb | 1 + docs/api/openapi.yaml | 2 + spec/swagger_helper.rb | 4 +- .../v1/budget_categories_controller_test.rb | 25 +++ .../budget_categories_controller_test.rb | 106 +++++++++ test/models/budget_category_test.rb | 210 ++++++++++++++++++ 18 files changed, 792 insertions(+), 7 deletions(-) create mode 100644 app/javascript/controllers/budget_move_controller.js create mode 100644 app/views/budget_categories/_move_dialog.html.erb create mode 100644 app/views/budget_categories/move.turbo_stream.erb diff --git a/app/controllers/api/v1/budget_categories_controller.rb b/app/controllers/api/v1/budget_categories_controller.rb index 3ab776b74..7790a55b6 100644 --- a/app/controllers/api/v1/budget_categories_controller.rb +++ b/app/controllers/api/v1/budget_categories_controller.rb @@ -4,6 +4,7 @@ class Api::V1::BudgetCategoriesController < Api::V1::BaseController include Pagy::Backend before_action :ensure_read_scope + before_action :refresh_rollover_chains before_action :set_budget_category, only: :show def index @@ -36,6 +37,27 @@ class Api::V1::BudgetCategoriesController < Api::V1::BaseController authorize_scope!(:read) end + # `rolled_over_amount` is materialized, and the web pages that show it + # recompute on the way in — every one of them goes through + # Budget.find_or_bootstrap. This endpoint reads the column straight, so + # without this it is the one surface that can serve a carry left stale by + # a sync or a recategorisation touching an earlier month. + # + # A read that writes is a smell, but the alternative is recomputing on + # every transaction change, which is the cost this design deliberately + # refused: the chain is walked per family, and for a family that never + # turned rollover on the calculator's leading EXISTS makes it one query + # that writes nothing. Same bargain the budget page already makes, applied + # to the surface that was missed. + def refresh_rollover_chains + visible_owner_ids.each do |owner_id| + Budget::RolloverCalculator.new( + family: current_resource_owner.family, + user: owner_id && User.find_by(id: owner_id) + ).recompute! + end + end + def budget_categories_scope BudgetCategory .joins(:budget, :category) diff --git a/app/controllers/budget_categories_controller.rb b/app/controllers/budget_categories_controller.rb index d4e3cb9ad..0e83464d1 100644 --- a/app/controllers/budget_categories_controller.rb +++ b/app/controllers/budget_categories_controller.rb @@ -2,7 +2,7 @@ class BudgetCategoriesController < ApplicationController include BudgetOwnership before_action :set_budget - before_action :ensure_budget_editable!, only: %i[index update] + before_action :ensure_budget_editable!, only: %i[index update move] def index @budget_categories = @budget.budget_categories.includes(:category) @@ -34,7 +34,12 @@ class BudgetCategoriesController < ApplicationController def update @budget_category = @budget.budget_categories.find(params[:id]) - @budget_category.update!(rollover_enabled: rollover_enabled_param) unless rollover_enabled_param.nil? + unless rollover_enabled_param.nil? + @budget_category.update!(rollover_enabled: rollover_enabled_param) + # A month the user opened before making this choice was created with the + # flag off and had nothing to inherit, so the chain stopped there. + @budget_category.propagate_rollover_choice_forward! + end @budget_category.update_budgeted_spending!(budgeted_spending_param) # Allocations and the rollover toggle both feed the chain, so recompute @@ -52,7 +57,43 @@ class BudgetCategoriesController < ApplicationController render :index, status: :unprocessable_entity end + # Shifts allocation from one envelope to another in one gesture. The + # recompute deliberately runs AFTER move_allocation! has committed, never + # inside it: the calculator takes an advisory lock, and taking it while the + # move still holds its row locks would invert the lock order #update + # already established and deadlock two concurrent moves. Once per move — + # the calculator rereads the whole chain either way. + def move + @from = @budget.budget_categories.find(params[:from_id]) + @to = @budget.budget_categories.find(params[:to_id]) + + BudgetCategory.move_allocation!(from: @from, to: @to, amount: move_amount_param) + Budget::RolloverCalculator.new(family: @budget.family, user: @budget.user).recompute! + + @budget.reload + flash.now[:notice] = t(".success") + respond_to do |format| + format.turbo_stream + format.html { redirect_to budget_budget_categories_path(@budget, **budget_owner_query), notice: t(".success") } + end + rescue BudgetCategory::InvalidMove => e + flash.now[:alert] = e.message + respond_to do |format| + format.turbo_stream { render turbo_stream: flash_notification_stream_items, status: :unprocessable_entity } + format.html do + @budget_categories = @budget.budget_categories.includes(:category) + render :index, layout: "wizard", status: :unprocessable_entity + end + end + end + private + # A blank or non-numeric amount is a zero move, which move_allocation! + # refuses with the localized "enter an amount greater than zero". + def move_amount_param + params.require(:budget_category_move).permit(:amount).fetch(:amount, nil).to_d + end + def rollover_enabled_param permitted = params.require(:budget_category).permit(:rollover_enabled) return nil unless permitted.key?(:rollover_enabled) diff --git a/app/javascript/controllers/budget_move_controller.js b/app/javascript/controllers/budget_move_controller.js new file mode 100644 index 000000000..07dd4592e --- /dev/null +++ b/app/javascript/controllers/budget_move_controller.js @@ -0,0 +1,79 @@ +import { Controller } from "@hotwired/stimulus" + +// Drives the single move dialog shared by every category row on the budget +// allocation page. One dialog for the page rather than one per row: the list +// can hold dozens of categories, and they would all be identical but for the +// source. +// +// The dialog itself is a DS::Dialog, so focus trapping, Escape, click-outside +// and focus restore are its job, not this controller's. What is left here is +// what the component cannot know: which row opened it, and where the money is +// allowed to go. +// +// Options that the server would refuse anyway are disabled rather than left +// selectable — a category cannot send money to itself, nor to its own parent +// or child, because the parent's allocation is derived from its children's. +export default class extends Controller { + static targets = [ + "dialog", + "fromId", + "fromName", + "available", + "toSelect", + "amount", + "submit", + "noDestination", + ] + + open({ params }) { + this.fromIdTarget.value = params.fromId + this.fromNameTarget.textContent = params.fromName + this.availableTarget.textContent = params.available + this.amountTarget.value = "" + + this.#refreshOptions(String(params.fromId), String(params.categoryId), String(params.parentId || "")) + this.dialogTarget.showModal() + this.amountTarget.focus() + } + + close() { + this.#dialogController()?.close() ?? this.dialogTarget.close() + } + + // Closing on submit alone would hide the reason a move was refused. Only a + // response Turbo considers successful ends the interaction. + submitEnd(event) { + if (event.detail?.success) this.close() + } + + #refreshOptions(fromId, categoryId, parentId) { + let firstEnabled = null + + for (const option of this.toSelectTarget.options) { + const optionCategoryId = option.dataset.categoryId + const optionParentId = option.dataset.parentId || "" + + option.disabled = + option.value === fromId || + optionCategoryId === parentId || + optionParentId === categoryId + + if (!option.disabled && firstEnabled === null) firstEnabled = option + } + + if (firstEnabled) this.toSelectTarget.value = firstEnabled.value + + // A lone envelope, or one whose only peers are its own parent and + // children, has nowhere to send money. Leaving submit enabled offers a + // button whose only outcome is a server error. + const hasDestination = firstEnabled !== null + this.submitTarget.disabled = !hasDestination + this.toSelectTarget.disabled = !hasDestination + this.amountTarget.disabled = !hasDestination + this.noDestinationTarget.classList.toggle("hidden", hasDestination) + } + + #dialogController() { + return this.application.getControllerForElementAndIdentifier(this.dialogTarget, "DS--dialog") + } +} diff --git a/app/models/budget_category.rb b/app/models/budget_category.rb index ff0dd87e1..790273be0 100644 --- a/app/models/budget_category.rb +++ b/app/models/budget_category.rb @@ -36,6 +36,97 @@ class BudgetCategory < ApplicationRecord category: nil, ) end + + # Moves `amount` of allocation from one envelope to another in a single + # step — YNAB's "roll with the punches". Deliberately keeps no history: + # v1 stores the resulting allocations, nothing else. + # + # ⚠️ Does NOT recompute the rollover chain, on purpose. The caller must + # run Budget::RolloverCalculator AFTER this returns, never inside it: + # the calculator takes a transaction-scoped advisory lock, and taking it + # while these row locks are held inverts the lock order every other + # caller uses (update_budgeted_spending! commits before the calculator + # runs). Two concurrent moves would then deadlock — one holding rows and + # waiting for the advisory lock, the other holding the advisory lock and + # waiting for those rows. + def move_allocation!(from:, to:, amount:) + amount = amount.to_d + validate_move!(from: from, to: to, amount: amount) + + transaction do + # Deterministic lock order — the critical detail of this operation. + # update_budgeted_spending! locks its own row and, for a + # subcategory, its parent. Two simultaneous moves in opposite + # directions would each hold what the other wants, so every row this + # touches is locked up front, by ascending id. + where(id: lock_ids_for_move(from, to)).order(:id).lock.to_a + + from.reload + to.reload + + # Re-checked under the lock: the balance read before it may be stale. + raise InvalidMove.new(:insufficient_funds) if amount > movable_from(from) + + from.update_budgeted_spending!((from[:budgeted_spending] || 0) - amount) + to.update_budgeted_spending!((to[:budgeted_spending] || 0) + amount) + end + + [ from.reload, to.reload ] + end + + private + def validate_move!(from:, to:, amount:) + raise InvalidMove.new(:non_positive_amount) unless amount.positive? + # Checked before the budget comparison: "Uncategorized" is synthesized + # on read and carries no budget_id, so it would otherwise be reported + # as belonging to a different budget — true, but not the reason. + raise InvalidMove.new(:uncategorized) if [ from, to ].any? { |bc| bc[:category_id].nil? || !bc.persisted? } + raise InvalidMove.new(:different_budgets) unless from.budget_id == to.budget_id + raise InvalidMove.new(:same_category) if from.id == to.id + raise InvalidMove.new(:parent_child) if direct_lineage?(from, to) + raise InvalidMove.new(:insufficient_funds) if amount > movable_from(from) + end + + # What a category can actually send away. For a leaf that is its whole + # allocation; for a parent it is only its own reserve, because + # `budgeted_spending` on a parent ALREADY CONTAINS its individually + # funded subcategories' allocations (sync_parent_budgeted_spending! + # keeps it at children + reserve). + # + # Comparing against the gross figure let a move spend money that is + # already ring-fenced by a child, leaving the parent below the sum of + # its children — and the next edit to any child rebuilt the parent back + # up, silently undoing the move. The money appeared to teleport back. + def movable_from(budget_category) + gross = budget_category[:budgeted_spending] || 0 + return gross if budget_category.subcategory? + + ring_fenced = budget_category.subcategories + .reject(&:inherits_parent_budget?) + .sum { |child| child[:budgeted_spending] || 0 } + + [ gross - ring_fenced, 0 ].max + end + + # sync_parent_budgeted_spending! rebuilds a parent from the sum of its + # children plus its own reserve, so money moved between a parent and + # its direct child would be re-derived away and the "sum is conserved" + # invariant would not hold. Refuse the move rather than special-case it. + def direct_lineage?(from, to) + from[:category_id] == to.category.parent_id || to[:category_id] == from.category.parent_id + end + + # from, to, and whichever parents update_budgeted_spending! will touch. + def lock_ids_for_move(from, to) + parent_category_ids = [ from, to ].filter_map { |bc| bc.category.parent_id } + parent_ids = if parent_category_ids.any? + from.budget.budget_categories.where(category_id: parent_category_ids).pluck(:id) + else + [] + end + + ([ from.id, to.id ] + parent_ids).uniq + end end def initialized? @@ -54,6 +145,27 @@ class BudgetCategory < ApplicationRecord budget.budget_category_actual_spending(self) end + # The toggle is a standing choice about the envelope, and the comment on + # Budget#inherited_rollover_flags already says so: "turning it off on a given + # month still overrides it from there on." + # + # Inheritance at row creation only covers months that do not exist yet. A + # user who opened March, then went back to January and switched rollover on, + # left March sitting at `false` — created before the choice was made, so it + # never had one to inherit — and the chain died there. Applying the choice + # forward closes that hole without a tri-state column: later months carry the + # most recent decision, which is the one the user just made. + def propagate_rollover_choice_forward! + later = BudgetCategory + .joins(:budget) + .where(category_id: category_id) + .where(budgets: { family_id: budget.family_id, user_id: budget.user_id }) + .where("budgets.start_date > ?", budget.start_date) + .where.not(rollover_enabled: rollover_enabled) + + later.update_all(rollover_enabled: rollover_enabled, updated_at: Time.current) + end + def update_budgeted_spending!(new_budgeted_spending) self.class.transaction do lock! @@ -65,6 +177,18 @@ class BudgetCategory < ApplicationRecord end end + # Raised by move_allocation! when the requested move is not one the budget + # can represent. Carries an i18n key rather than a sentence so the + # controller renders it localized. + class InvalidMove < StandardError + attr_reader :reason + + def initialize(reason) + @reason = reason + super(I18n.t("budget_categories.move.errors.#{reason}")) + end + end + def avg_monthly_expense budget.category_avg_monthly_expense(category) end diff --git a/app/views/budget_categories/_budget_category_form.html.erb b/app/views/budget_categories/_budget_category_form.html.erb index 80e243d6d..6784e0133 100644 --- a/app/views/budget_categories/_budget_category_form.html.erb +++ b/app/views/budget_categories/_budget_category_form.html.erb @@ -11,7 +11,25 @@

<%= t("budget_categories.budget_category_form.monthly_average", amount: budget_category.median_monthly_expense_money.format(precision: 0)) %>

-
+
+ <%# Outside the allocation form on purpose: a button inside it would + submit the amount field on click. Hidden for an envelope with nothing + to give — an inheriting subcategory, or one left at zero. %> + <% if budget_category[:budgeted_spending].to_d.positive? %> + + <% end %> + <%= form_with model: [budget_category.budget, budget_category], url: budget_budget_category_path(budget_category.budget, budget_category, **budget_owner_query), data: { controller: "auto-submit-form preserve-focus" } do |f| %>
"> diff --git a/app/views/budget_categories/_move_dialog.html.erb b/app/views/budget_categories/_move_dialog.html.erb new file mode 100644 index 000000000..71c4891fa --- /dev/null +++ b/app/views/budget_categories/_move_dialog.html.erb @@ -0,0 +1,80 @@ +<%# locals: (budget:, budget_categories:) %> + +<%# One dialog for the whole page — every row's button fills it in. The list can + hold dozens of categories and they would all be identical but for the + source, so a per-row dialog would be dozens of copies of the same markup. + + Rendered with DS::Dialog rather than a bare : the component already + carries focus trapping, Escape, click-outside, focus restore on close and + the design-system chrome, and hand-rolling those is how they end up subtly + wrong. `auto_open: false` because this one is opened by a row button rather + than by landing in the modal frame, and `disable_frame: true` because it + lives on the page instead of inside that frame. %> +<%= render DS::Dialog.new( + auto_open: false, + disable_frame: true, + width: "sm", + data: { budget_move_target: "dialog" } + ) do |dialog| %> + <% dialog.with_header(title: t("budget_categories.move.dialog_title")) %> + + <% dialog.with_body do %> + <%# `turbo:submit-end` rather than a blind close on submit: a rejected move + (amount above the source's own allocation, say) re-renders the form with + its error, and closing the dialog would hide the reason. %> + <%= form_with url: move_budget_budget_categories_path(budget, **budget_owner_query), + method: :post, + class: "space-y-4", + data: { action: "turbo:submit-end->budget-move#submitEnd" } do |f| %> + <%= hidden_field_tag "from_id", nil, data: { budget_move_target: "fromId" } %> + +

+ + · + +

+ +
+ <%= label_tag "budget_category_move_amount", t("budget_categories.move.amount_label"), class: "text-sm text-secondary" %> + <%= number_field_tag "budget_category_move[amount]", nil, + id: "budget_category_move_amount", + step: Money::Currency.new(budget.currency).step, + min: Money::Currency.new(budget.currency).step, + required: true, + autocomplete: "off", + class: "form-field__input w-full text-right tabular-nums privacy-sensitive", + data: { budget_move_target: "amount" } %> +
+ +
+ <%= label_tag "to_id", t("budget_categories.move.to_label"), class: "text-sm text-secondary" %> + <%= select_tag "to_id", + safe_join(budget_categories.map { |bc| + tag.option(bc.category.display_name, + value: bc.id, + data: { category_id: bc.category_id, parent_id: bc.category.parent_id }) + }), + class: "form-field__input w-full", + data: { budget_move_target: "toSelect" } %> +
+ + <%# Shown when the source has nowhere to send money: the only other + envelopes are its own parent or children, which the server refuses + because a parent's allocation is derived from its children's. Saying + so beats an enabled button that can only fail. %> + + +
+ <%= render DS::Button.new(text: t("budget_categories.move.cancel"), + variant: "secondary", + type: "button", + data: { action: "budget-move#close" }) %> + <%= render DS::Button.new(text: t("budget_categories.move.submit"), + type: "submit", + data: { budget_move_target: "submit" }) %> +
+ <% end %> + <% end %> +<% end %> diff --git a/app/views/budget_categories/index.html.erb b/app/views/budget_categories/index.html.erb index 3730b74d7..52e1b9363 100644 --- a/app/views/budget_categories/index.html.erb +++ b/app/views/budget_categories/index.html.erb @@ -20,7 +20,7 @@ <%= render "budget_categories/no_categories" %>
<% else %> -
+
<%= render "budget_categories/allocation_progress", budget: @budget %>
@@ -46,6 +46,8 @@
<%= render "budget_categories/confirm_button", budget: @budget %> + + <%= render "budget_categories/move_dialog", budget: @budget, budget_categories: @budget_categories %>
<% end %>
diff --git a/app/views/budget_categories/move.turbo_stream.erb b/app/views/budget_categories/move.turbo_stream.erb new file mode 100644 index 000000000..4622e3669 --- /dev/null +++ b/app/views/budget_categories/move.turbo_stream.erb @@ -0,0 +1,29 @@ +<%= flash_notification_stream_items %> + +<%= turbo_stream.replace dom_id(@budget, :allocation_progress), partial: "budget_categories/allocation_progress", locals: { budget: @budget } %> + +<%= turbo_stream.replace dom_id(@budget, :uncategorized_budget_category_form), partial: "budget_categories/uncategorized_budget_category_form", locals: { budget: @budget } %> + +<%= turbo_stream.replace dom_id(@budget, :confirm_button), partial: "budget_categories/confirm_button", locals: { budget: @budget } %> + +<%# Both ends of the move, plus whatever their allocation drags along: a + subcategory pulls its parent and siblings, a parent pushes down to its + children. Re-rendering the same row twice (from and to can be siblings) + is harmless — the last replace wins with identical markup. %> +<% [ @from, @to ].each do |budget_category| %> + <%= turbo_stream.replace dom_id(budget_category, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: budget_category } %> + + <% if budget_category.subcategory? %> + <% if (parent_budget_category = budget_category.parent_budget_category) %> + <%= turbo_stream.replace dom_id(parent_budget_category, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: parent_budget_category } %> + <% end %> + + <% budget_category.siblings.each do |sibling| %> + <%= turbo_stream.replace dom_id(sibling, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: sibling } %> + <% end %> + <% else %> + <% budget_category.subcategories.each do |subcategory| %> + <%= turbo_stream.replace dom_id(subcategory, :form), partial: "budget_categories/budget_category_form", locals: { budget_category: subcategory } %> + <% end %> + <% end %> +<% end %> diff --git a/config/locales/views/budgets/en.yml b/config/locales/views/budgets/en.yml index da77e6f9a..63d1a0589 100644 --- a/config/locales/views/budgets/en.yml +++ b/config/locales/views/budgets/en.yml @@ -82,6 +82,24 @@ en: shared_title: Leave empty to share parent's budget confirm_button: confirm: "Confirm" + move: + button_title: Move money from this category + cancel: Cancel + dialog_title: Move money + no_destination: This is the only envelope money can move between right now — a category cannot send to its own parent or child. + submit: Move + amount_label: Amount + available: "%{amount} available" + from_label: From + to_label: To + success: Money moved. + errors: + different_budgets: Both categories must belong to the same budget. + insufficient_funds: That is more than this category has allocated. + non_positive_amount: Enter an amount greater than zero. + parent_child: Money cannot move between a category and its own subcategory — adjust the subcategory directly. + same_category: Pick a different category to move the money to. + uncategorized: Uncategorized is not a real envelope, so money cannot move in or out of it. no_categories: oops: "Oops!" no_categories_message: "You have not created or assigned any expense categories to your transactions yet." diff --git a/config/locales/views/budgets/fr.yml b/config/locales/views/budgets/fr.yml index fc921a9c4..594e7782b 100644 --- a/config/locales/views/budgets/fr.yml +++ b/config/locales/views/budgets/fr.yml @@ -19,6 +19,24 @@ fr: index: description: Ajustez les budgets des catégories pour fixer des limites de dépenses. Les fonds non alloués seront automatiquement attribués comme non classés. title: Modifiez vos budgets de catégorie + move: + amount_label: Montant + available: "%{amount} disponible" + button_title: Déplacer de l'argent depuis cette catégorie + cancel: Annuler + dialog_title: Déplacer de l'argent + no_destination: C'est la seule enveloppe disponible pour l'instant — une catégorie ne peut pas envoyer vers son propre parent ni vers sa sous-catégorie. + errors: + different_budgets: Les deux catégories doivent appartenir au même budget. + insufficient_funds: C'est plus que ce que cette catégorie a alloué. + non_positive_amount: Indiquez un montant supérieur à zéro. + parent_child: L'argent ne peut pas circuler entre une catégorie et sa propre sous-catégorie — ajustez directement la sous-catégorie. + same_category: Choisissez une autre catégorie de destination. + uncategorized: "« Non classé » n'est pas une vraie enveloppe : l'argent ne peut ni y entrer ni en sortir." + from_label: Depuis + submit: Déplacer + success: Argent déplacé. + to_label: Vers no_categories: new_category: Nouvelle catégorie no_categories_message: Vous n'avez pas encore créé ou attribué de catégories de dépenses à vos transactions. diff --git a/config/routes.rb b/config/routes.rb index d4bec8082..44352608b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -415,7 +415,9 @@ Rails.application.routes.draw do post :copy_previous, on: :member get :picker, on: :collection - resources :budget_categories, only: %i[index show update] + resources :budget_categories, only: %i[index show update] do + post :move, on: :collection + end end resources :goals do diff --git a/db/migrate/20260823000000_add_rollover_to_budget_categories.rb b/db/migrate/20260823000000_add_rollover_to_budget_categories.rb index 3d61afe96..015f3ca61 100644 --- a/db/migrate/20260823000000_add_rollover_to_budget_categories.rb +++ b/db/migrate/20260823000000_add_rollover_to_budget_categories.rb @@ -2,5 +2,13 @@ class AddRolloverToBudgetCategories < ActiveRecord::Migration[7.2] def change add_column :budget_categories, :rollover_enabled, :boolean, null: false, default: false add_column :budget_categories, :rolled_over_amount, :decimal, precision: 19, scale: 4, null: false, default: 0 + + # The calculator floors the carry at zero, but it writes through + # `upsert_all` and nothing else stops a direct write. A negative carry + # would quietly SUBTRACT from `available_to_spend` — an envelope that + # shrinks for no visible reason. Enforced in the database because that is + # the one door every writer goes through. + add_check_constraint :budget_categories, "rolled_over_amount >= 0", + name: "chk_budget_categories_rolled_over_amount_non_negative" end end diff --git a/db/schema.rb b/db/schema.rb index 6eb5d2ccc..f20402a76 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -372,6 +372,7 @@ ActiveRecord::Schema[7.2].define(version: 2026_08_23_000000) do t.index ["budget_id", "category_id"], name: "index_budget_categories_on_budget_id_and_category_id", unique: true t.index ["budget_id"], name: "index_budget_categories_on_budget_id" t.index ["category_id"], name: "index_budget_categories_on_category_id" + t.check_constraint "rolled_over_amount >= 0::numeric", name: "chk_budget_categories_rolled_over_amount_non_negative" end create_table "budget_shares", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 18de97bec..913c65ed0 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -666,6 +666,7 @@ components: - currency - subcategory - inherits_parent_budget + - rollover_enabled - category - created_at - updated_at @@ -727,6 +728,7 @@ components: - currency - subcategory - inherits_parent_budget + - rollover_enabled - category - created_at - updated_at diff --git a/spec/swagger_helper.rb b/spec/swagger_helper.rb index b43f9ffd2..f1d5e7ed9 100644 --- a/spec/swagger_helper.rb +++ b/spec/swagger_helper.rb @@ -391,7 +391,7 @@ RSpec.configure do |config| }, BudgetCategorySummary: { type: :object, - required: %w[id budget_id currency subcategory inherits_parent_budget category created_at updated_at], + required: %w[id budget_id currency subcategory inherits_parent_budget rollover_enabled category created_at updated_at], properties: { id: { type: :string, format: :uuid }, budget_id: { type: :string, format: :uuid }, @@ -420,7 +420,7 @@ RSpec.configure do |config| }, BudgetCategory: { type: :object, - required: %w[id budget_id currency subcategory inherits_parent_budget category created_at updated_at], + required: %w[id budget_id currency subcategory inherits_parent_budget rollover_enabled category created_at updated_at], properties: { id: { type: :string, format: :uuid }, budget_id: { type: :string, format: :uuid }, diff --git a/test/controllers/api/v1/budget_categories_controller_test.rb b/test/controllers/api/v1/budget_categories_controller_test.rb index 9b230c4f2..67a1190b4 100644 --- a/test/controllers/api/v1/budget_categories_controller_test.rb +++ b/test/controllers/api/v1/budget_categories_controller_test.rb @@ -213,4 +213,29 @@ class Api::V1::BudgetCategoriesControllerTest < ActionDispatch::IntegrationTest ensure api_key_without_read&.destroy end + + # Every web surface that shows the carry recomputes on the way in, through + # Budget.find_or_bootstrap. This endpoint reads the materialized column + # straight, so it was the one place a carry left stale by a sync or a + # recategorisation could still be served. + test "index refreshes a stale carry rather than serving it" do + earlier = Budget.find_or_bootstrap(@family, start_date: 3.months.ago.to_date, user: nil) + earlier.update!(budgeted_spending: 3_000, expected_income: 5_000) + earlier.budget_categories.find_by!(category: @category) + .update!(budgeted_spending: 500, rollover_enabled: true) + + later = Budget.find_or_bootstrap(@family, start_date: 2.months.ago.to_date, user: nil) + later.update!(budgeted_spending: 3_000, expected_income: 5_000) + later.budget_categories.find_by!(category: @category) + .update!(budgeted_spending: 500, rollover_enabled: true) + + # Simulate what a sync does: rewrite the stored carry behind the app's + # back, the way a changed past month would leave it. + later.budget_categories.find_by!(category: @category).update_column(:rolled_over_amount, 0) + + get api_v1_budget_categories_url, headers: api_headers(@api_key) + + assert_response :success + assert_equal 500, later.budget_categories.find_by!(category: @category).reload[:rolled_over_amount] + end end diff --git a/test/controllers/budget_categories_controller_test.rb b/test/controllers/budget_categories_controller_test.rb index cb4b55d1c..77812c5d8 100644 --- a/test/controllers/budget_categories_controller_test.rb +++ b/test/controllers/budget_categories_controller_test.rb @@ -220,6 +220,95 @@ class BudgetCategoriesControllerTest < ActionDispatch::IntegrationTest assert_includes @response.body, "MORTGAGE_REPRO_OUTFLOW", "loan_payment outflow remains visible (kind is not BUDGET_EXCLUDED)" end + + # --- move (Lot A2) --- + + test "move shifts allocation between two envelopes and leaves the total alone" do + source = @budget.budget_categories.find_by(category: @parent_category) + other = Category.create!(name: "Transport controller test", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other, budgeted_spending: 100, currency: @budget.currency) + source.update_budgeted_spending!(400) + before = @budget.reload.allocated_spending + + post move_budget_budget_categories_path(@budget), + params: { from_id: source.id, to_id: destination.id, budget_category_move: { amount: "150" } }, + as: :turbo_stream + + assert_response :success + assert_equal 250, source.reload.budgeted_spending.to_i + assert_equal 250, destination.reload.budgeted_spending.to_i + assert_equal before, @budget.reload.allocated_spending + end + + test "move refuses an amount the source does not have and says why" do + source = @budget.budget_categories.find_by(category: @parent_category) + other = Category.create!(name: "Transport controller test", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other, budgeted_spending: 0, currency: @budget.currency) + source.update_budgeted_spending!(100) + + post move_budget_budget_categories_path(@budget), + params: { from_id: source.id, to_id: destination.id, budget_category_move: { amount: "500" } }, + as: :turbo_stream + + assert_response :unprocessable_entity + assert_equal 100, source.reload.budgeted_spending.to_i + assert_equal 0, destination.reload.budgeted_spending.to_i + end + + test "move refuses a parent to subcategory transfer" do + parent = @budget.budget_categories.find_by(category: @parent_category) + child = @budget.budget_categories.find_by(category: @electric_category) + child.update_budgeted_spending!(50) + before_parent = parent.reload.budgeted_spending + + post move_budget_budget_categories_path(@budget), + params: { from_id: parent.id, to_id: child.id, budget_category_move: { amount: "10" } }, + as: :turbo_stream + + assert_response :unprocessable_entity + assert_equal before_parent, parent.reload.budgeted_spending + end + + # A move changes what each envelope has left, so it changes what the next + # month inherits — the chain has to be rebuilt, exactly as an allocation + # edit does. + # + # Twice, not once, and that is worth pinning: `set_budget` resolves the + # budget through `Budget.find_or_bootstrap`, which already recomputes on + # every request to this controller, and the action then recomputes after + # the move commits. `#update` has carried the same double cost since the + # rollover lot landed. The action itself must call it exactly once — the + # count moving to three would mean a second call crept into #move. + test "move recomputes the rollover chain, once of its own accord" do + source = @budget.budget_categories.find_by(category: @parent_category) + other = Category.create!(name: "Transport controller test", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other, budgeted_spending: 0, currency: @budget.currency) + source.update_budgeted_spending!(300) + + Budget::RolloverCalculator.any_instance.expects(:recompute!).twice + + post move_budget_budget_categories_path(@budget), + params: { from_id: source.id, to_id: destination.id, budget_category_move: { amount: "100" } }, + as: :turbo_stream + + assert_response :success + end + + test "a category from another budget cannot be reached through move" do + source = @budget.budget_categories.find_by(category: @parent_category) + source.update_budgeted_spending!(300) + other_family = families(:empty) + other_budget = Budget.find_or_bootstrap(other_family, start_date: Date.current.beginning_of_month) + foreign_category = other_family.categories.create!(name: "Foreign", color: "#e99537") + foreign = BudgetCategory.create!(budget: other_budget, category: foreign_category, budgeted_spending: 0, currency: other_budget.currency) + + post move_budget_budget_categories_path(@budget), + params: { from_id: source.id, to_id: foreign.id, budget_category_move: { amount: "10" } }, + as: :turbo_stream + + assert_response :not_found + assert_equal 0, foreign.reload.budgeted_spending.to_i + end end class BudgetCategoriesControllerSharingTest < ActionDispatch::IntegrationTest @@ -254,4 +343,21 @@ class BudgetCategoriesControllerSharingTest < ActionDispatch::IntegrationTest assert_response :success assert_equal 250.0, budget_category.reload.budgeted_spending.to_f end + + test "a read_only viewer cannot move money on the owner's budget" do + BudgetShare.create!(owner: @owner, viewer: @viewer, permission: "read_only") + categories = @owner_budget.budget_categories.to_a + source = categories.first + source.update_budgeted_spending!(200) + destination = @family.categories.create!(name: "Transport", color: "#e99537") + target = BudgetCategory.create!(budget: @owner_budget, category: destination, budgeted_spending: 0, currency: @owner_budget.currency) + sign_in @viewer + + post move_budget_budget_categories_path(@owner_budget, owner: @owner.id), + params: { from_id: source.id, to_id: target.id, budget_category_move: { amount: "50" } }, + as: :turbo_stream + + assert_response :not_found + assert_equal 200, source.reload.budgeted_spending.to_i + end end diff --git a/test/models/budget_category_test.rb b/test/models/budget_category_test.rb index a0eed36f4..3031f0586 100644 --- a/test/models/budget_category_test.rb +++ b/test/models/budget_category_test.rb @@ -271,6 +271,187 @@ class BudgetCategoryTest < ActiveSupport::TestCase assert_equal 14, suggestion[:days_remaining] end end + + # --- move_allocation! (Lot A2) --- + + test "moving money between two top-level envelopes conserves the total" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 200, currency: "USD") + before = @budget.reload.allocated_spending + + BudgetCategory.move_allocation!(from: @parent_budget_category, to: destination, amount: 50) + + assert_equal 950, @parent_budget_category.reload.budgeted_spending + assert_equal 250, destination.reload.budgeted_spending + assert_equal before, @budget.reload.allocated_spending, "allocated_spending must be invariant" + end + + # Deliberately a leaf as the source: only there does "the whole allocation" + # mean the whole of it. A parent's figure already contains its individually + # funded children's, so its boundary is its own reserve — covered separately + # below. This test used to move a parent's gross amount and pass, which is + # exactly the money-teleports-back bug. + test "moving the whole allocation is allowed, moving one cent more is not" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + source = @subcategory_with_limit_bc.reload + whole = source.budgeted_spending + + assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: source, to: destination, amount: whole + 0.01) + end + + BudgetCategory.move_allocation!(from: source, to: destination, amount: whole) + assert_equal 0, source.reload.budgeted_spending + assert_equal whole, destination.reload.budgeted_spending + end + + test "an amount larger than the source allocation is refused" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: @parent_budget_category, to: destination, amount: 1001) + end + + assert_equal :insufficient_funds, error.reason + assert_equal 1000, @parent_budget_category.reload.budgeted_spending + end + + test "a zero or negative amount is refused" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + + [ 0, -50 ].each do |amount| + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: @parent_budget_category, to: destination, amount: amount) + end + assert_equal :non_positive_amount, error.reason + end + end + + test "categories from two different budgets cannot exchange money" do + other_budget = Budget.create!( + family: @family, + start_date: @budget.start_date - 1.month, + end_date: @budget.start_date - 1.day, + currency: @budget.currency + ) + foreign_category = Category.create!(name: "Test Foreign #{Time.now.to_f}", family: @family, color: "#e99537") + foreign = BudgetCategory.create!(budget: other_budget, category: foreign_category, budgeted_spending: 100, currency: other_budget.currency) + + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: @parent_budget_category, to: foreign, amount: 10) + end + + assert_equal :different_budgets, error.reason + end + + test "a category cannot move money to itself" do + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: @parent_budget_category, to: @parent_budget_category, amount: 10) + end + + assert_equal :same_category, error.reason + end + + # sync_parent_budgeted_spending! rebuilds a parent from its children, so a + # parent <-> child move would be re-derived away. + test "money cannot move between a parent and its own subcategory, in either direction" do + [ [ @parent_budget_category, @subcategory_with_limit_bc ], + [ @subcategory_with_limit_bc, @parent_budget_category ] ].each do |from, to| + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: from, to: to, amount: 50) + end + assert_equal :parent_child, error.reason + end + end + + test "Uncategorized can neither give nor receive" do + [ [ BudgetCategory.uncategorized, @parent_budget_category ], + [ @parent_budget_category, BudgetCategory.uncategorized ] ].each do |from, to| + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: from, to: to, amount: 10) + end + assert_equal :uncategorized, error.reason + end + end + + # A subcategory's allocation is folded into its parent's, so moving money + # out of one must pull the parent down by the same amount and leave the + # budget total untouched. + test "a move out of a subcategory keeps its parent consistent" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + before = @budget.reload.allocated_spending + + BudgetCategory.move_allocation!(from: @subcategory_with_limit_bc, to: destination, amount: 100) + + assert_equal 200, @subcategory_with_limit_bc.reload.budgeted_spending + assert_equal 100, destination.reload.budgeted_spending + assert_equal 900, @parent_budget_category.reload.budgeted_spending, + "the parent must absorb its subcategory's decrease" + assert_equal before, @budget.reload.allocated_spending, "allocated_spending must be invariant" + end + + # A parent's budgeted_spending already contains its individually funded + # subcategories', so treating the gross figure as movable let a move spend + # money a child had ring-fenced. The parent dropped below the sum of its + # children, and the next edit to any child rebuilt it — the money appeared + # to teleport back. + test "a parent can only send away its own reserve, not its children's money" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + parent_gross = @parent_budget_category.reload.budgeted_spending + ring_fenced = @subcategory_with_limit_bc.reload.budgeted_spending + reserve = parent_gross - ring_fenced + + assert_operator ring_fenced, :>, 0, "fixture should ring-fence part of the parent" + + error = assert_raises(BudgetCategory::InvalidMove) do + BudgetCategory.move_allocation!(from: @parent_budget_category, to: destination, amount: reserve + 1) + end + assert_equal :insufficient_funds, error.reason + + assert_equal parent_gross, @parent_budget_category.reload.budgeted_spending + end + + test "a parent may still send away every penny of its own reserve" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + reserve = @parent_budget_category.reload.budgeted_spending - @subcategory_with_limit_bc.reload.budgeted_spending + before_total = @budget.reload.allocated_spending + + BudgetCategory.move_allocation!(from: @parent_budget_category, to: destination, amount: reserve) + + assert_equal reserve, destination.reload.budgeted_spending + assert_equal before_total, @budget.reload.allocated_spending, "allocated_spending must be invariant" + end + + test "a move between two subcategories of the same parent leaves the parent alone" do + before_parent = @parent_budget_category.reload.budgeted_spending + before_total = @budget.reload.allocated_spending + @subcategory_inheriting_bc.update_budgeted_spending!(100) + + BudgetCategory.move_allocation!(from: @subcategory_with_limit_bc, to: @subcategory_inheriting_bc, amount: 50) + + assert_equal 250, @subcategory_with_limit_bc.reload.budgeted_spending + assert_equal 150, @subcategory_inheriting_bc.reload.budgeted_spending + assert_equal before_parent + 100, @parent_budget_category.reload.budgeted_spending + assert_equal before_total + 100, @budget.reload.allocated_spending + end + + # The rollover chain is the caller's job, never the move's: taking the + # calculator's advisory lock while these row locks are held would invert + # the lock order and deadlock two concurrent moves. + test "move_allocation! does not recompute the rollover chain itself" do + other_parent = Category.create!(name: "Test Transport #{Time.now.to_f}", family: @family, color: "#e99537") + destination = BudgetCategory.create!(budget: @budget, category: other_parent, budgeted_spending: 0, currency: "USD") + + Budget::RolloverCalculator.any_instance.expects(:recompute!).never + + BudgetCategory.move_allocation!(from: @parent_budget_category, to: destination, amount: 10) + end end class BudgetCategoryRolloverTest < ActiveSupport::TestCase @@ -688,6 +869,35 @@ class BudgetCategoryRolloverTest < ActiveSupport::TestCase assert_not ann_second.budget_categories.find_by!(category: @category).rollover_enabled? end + # Inheritance at row creation only covers months that do not exist yet. A + # month opened BEFORE the user made the choice was created with the flag off + # and had nothing to inherit, so the chain died there. + test "switching rollover on reaches months that were already open" do + first = initialized_budget(2.months.ago) + later = initialized_budget(1.month.ago) + allocate(first, 100, rollover: false) + allocate(later, 100, rollover: false) + + budget_category_for(first).update!(rollover_enabled: true) + budget_category_for(first).propagate_rollover_choice_forward! + + assert budget_category_for(later).reload.rollover_enabled? + end + + test "switching it off reaches them too, and never runs backwards" do + first = initialized_budget(2.months.ago) + middle = initialized_budget(1.month.ago) + allocate(first, 100) + allocate(middle, 100) + + budget_category_for(middle).update!(rollover_enabled: false) + budget_category_for(middle).propagate_rollover_choice_forward! + + assert_not budget_category_for(middle).reload.rollover_enabled? + assert budget_category_for(first).reload.rollover_enabled?, + "an earlier month keeps the choice it was given" + end + private def capture_sql statements = []