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 = []