Skip to content
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
51eb140
fix(transactions): include one-time in reports, keep out of budget me…
glorydavid03023 May 26, 2026
ba88d96
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 May 26, 2026
0afb039
fix(transactions): address PR review feedback for one-time budget sco…
glorydavid03023 May 27, 2026
8c667a9
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 May 27, 2026
b0ef1dc
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 May 27, 2026
1801a2f
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 May 29, 2026
c7235c1
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 May 30, 2026
8d58a2f
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 May 31, 2026
53a124f
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jun 1, 2026
24a3ab8
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jun 1, 2026
d937faa
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jun 2, 2026
b5b44b2
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jun 2, 2026
5f6399a
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jun 3, 2026
666710c
fix(transactions): wire period-total memoization and align test expec…
glorydavid03023 Jun 3, 2026
6535d8e
ci: re-trigger checks (test_system Docker Hub pull timeout)
glorydavid03023 Jun 3, 2026
c48b7fc
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jun 3, 2026
58c1d6f
Merge branch 'main' into fix/one-time-transactions-in-reports
glorydavid03023 Jul 3, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions app/controllers/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -357,13 +357,13 @@ def build_trends_data(income_statement:)

def build_transactions_breakdown
# Base query: all transactions in the period
# Exclude transfers, one-time, and CC payments (matching income_statement logic)
# Exclude transfers and CC payments (matching income_statement report logic)
transactions = Transaction
.joins(:entry)
.joins(entry: :account)
.where(accounts: { family_id: Current.family.id, status: [ "draft", "active" ] })
.where(entries: { entryable_type: "Transaction", excluded: false, date: @period.date_range })
.where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS)
.where.not(kind: Transaction::REPORT_EXCLUDED_KINDS)
.includes(entry: :account, category: :parent)

# Apply filters (includes finance account scoping)
Expand Down Expand Up @@ -661,13 +661,13 @@ def apply_entry_filters(scope)

def build_transactions_breakdown_for_export
# Get flat transactions list (not grouped) for export
# Exclude transfers, one-time, and CC payments (matching income_statement logic)
# Exclude transfers and CC payments (matching income_statement report logic)
transactions = Transaction
.joins(:entry)
.joins(entry: :account)
.where(accounts: { family_id: Current.family.id, status: [ "draft", "active" ] })
.where(entries: { entryable_type: "Transaction", excluded: false, date: @period.date_range })
.where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS)
.where.not(kind: Transaction::REPORT_EXCLUDED_KINDS)
.includes(entry: :account, category: [])

transactions = apply_transaction_filters(transactions)
Expand Down Expand Up @@ -698,13 +698,13 @@ def build_monthly_breakdown_for_export
end

# Get all transactions in the period
# Exclude transfers, one-time, and CC payments (matching income_statement logic)
# Exclude transfers and CC payments (matching income_statement report logic)
transactions = Transaction
.joins(:entry)
.joins(entry: :account)
.where(accounts: { family_id: Current.family.id, status: [ "draft", "active" ] })
.where(entries: { entryable_type: "Transaction", excluded: false, date: @period.date_range })
.where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS)
.where.not(kind: Transaction::REPORT_EXCLUDED_KINDS)
.includes(entry: :account, category: [])

transactions = apply_transaction_filters(transactions)
Expand Down
8 changes: 4 additions & 4 deletions app/models/budget.rb
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ def estimated_income
end

def actual_income
family.income_statement.income_totals(period: self.period).total
income_totals.total
end

def actual_income_percent
Expand All @@ -313,15 +313,15 @@ def income_statement
end

def net_totals
@net_totals ||= income_statement.net_category_totals(period: period)
@net_totals ||= income_statement.net_category_totals(period: period, for_budget: true)
end

def expense_totals
@expense_totals ||= income_statement.expense_totals(period: period)
@expense_totals ||= income_statement.expense_totals(period: period, for_budget: true)
end

def income_totals
@income_totals ||= income_statement.income_totals(period: period)
@income_totals ||= income_statement.income_totals(period: period, for_budget: true)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

@gariasf gariasf May 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Budget#actual_income bypasses this private helper — asymmetric with actual_spending / estimated_income.

# line 294
def actual_income
  family.income_statement.income_totals(period: self.period).total
end

actual_spending (line 230) goes through net_totalsfor_budget: true and correctly excludes one-time. actual_income calls the public income_totals directly, hits the new default for_budget: falseREPORT_EXCLUDED_KINDSone-time income is now included.

Concrete effect: estimated_income is the median (no one-time). A one-time bonus inflates actual_income, actual_income_percent, remaining_expected_income, surplus_percent. Same-magnitude one-time expense does not show in actual_spending. This is the same defect the PR fixes — just on the income side.

Fix — route through the private memoized helper so it matches actual_spending:

def actual_income
  income_totals.total
end

Also worth adding a Budget-level test pinning both actual_spending and actual_income against a kind: "one_time" transaction. Would catch this and future drift.

end

def expense_totals_by_category
Expand Down
60 changes: 38 additions & 22 deletions app/models/income_statement.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,28 +29,30 @@ def totals(transactions_scope: nil, date_range:)
)
end

def expense_totals(period: Period.current_month)
def expense_totals(period: Period.current_month, for_budget: false)
# Memoized per instance so callers that also invoke `net_category_totals`
key = period_cache_key(period)
Comment on lines 33 to 34

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover from my last review: key is assigned but never used (the memo builds its own cache_key below), and the comment above it ends mid-sentence.

Suggested change
# Memoized per instance so callers that also invoke `net_category_totals`
key = period_cache_key(period)
# Memoized per instance so repeat callers (e.g. net_category_totals) reuse the same build

@expense_totals_by_period ||= {}
return @expense_totals_by_period[key] if @expense_totals_by_period.key?(key)
@expense_totals_by_period[key] = build_period_total(classification: "expense", period: period)
cache_key = [ period_cache_key(period), for_budget ]
@expense_totals_by_period[cache_key] ||=
build_period_total(classification: "expense", period: period, for_budget: for_budget)
end

def income_totals(period: Period.current_month)
key = period_cache_key(period)
def income_totals(period: Period.current_month, for_budget: false)
@income_totals_by_period ||= {}
return @income_totals_by_period[key] if @income_totals_by_period.key?(key)
@income_totals_by_period[key] = build_period_total(classification: "income", period: period)
cache_key = [ period_cache_key(period), for_budget ]
@income_totals_by_period[cache_key] ||=
build_period_total(classification: "income", period: period, for_budget: for_budget)
end

def net_category_totals(period: Period.current_month)
key = period_cache_key(period)
def net_category_totals(period: Period.current_month, for_budget: false)
@net_category_totals_by_period ||= {}
return @net_category_totals_by_period[key] if @net_category_totals_by_period.key?(key)
cache_key = [ period_cache_key(period), for_budget ]
cached = @net_category_totals_by_period[cache_key]
return cached if cached

expense = expense_totals(period: period)
income = income_totals(period: period)
expense = expense_totals(period: period, for_budget: for_budget)
income = income_totals(period: period, for_budget: for_budget)

# Use a stable key for each category: id for persisted, invariant token for synthetic
cat_key = ->(ct) {
Expand Down Expand Up @@ -98,7 +100,7 @@ def net_category_totals(period: Period.current_month)
CategoryTotal.new(category: r[:category], total: r[:total], currency: family.currency, weight: weight)
end

@net_category_totals_by_period[key] = NetCategoryTotals.new(
@net_category_totals_by_period[cache_key] = NetCategoryTotals.new(
net_expense_categories: net_expense_categories,
net_income_categories: net_income_categories,
total_net_expense: total_net_expense,
Expand Down Expand Up @@ -141,9 +143,9 @@ def period_cache_key(period)
[ period.start_date, period.end_date ]
end

def build_period_total(classification:, period:)
# Exclude pending transactions from budget calculations
totals = totals_for_period(period).select { |t| t.classification == classification }
def build_period_total(classification:, period:, for_budget: false)
# Income and expense for the same period+scope share a single totals fetch
totals = totals_for_period(period, for_budget: for_budget).select { |t| t.classification == classification }
classification_total = totals.sum(&:total)

uncategorized_category = family.categories.uncategorized
Expand Down Expand Up @@ -186,12 +188,16 @@ def build_period_total(classification:, period:)
)
end

def totals_for_period(period)
def totals_for_period(period, for_budget: false)
excluded_kinds = for_budget ? Transaction::BUDGET_EXCLUDED_KINDS : Transaction::REPORT_EXCLUDED_KINDS
@totals_for_period ||= {}
@totals_for_period[period_cache_key(period)] ||=
cache_key = [ period_cache_key(period), for_budget ]
# Exclude pending transactions from budget/report calculations
@totals_for_period[cache_key] ||=
totals_query(
transactions_scope: family.transactions.visible.excluding_pending.in_period(period),
date_range: period.date_range
date_range: period.date_range,
excluded_kinds: excluded_kinds
)
end

Expand All @@ -217,12 +223,22 @@ def included_account_ids_hash
@included_account_ids_hash ||= included_account_ids ? Digest::MD5.hexdigest(included_account_ids.sort.join(",")) : nil
end

def totals_query(transactions_scope:, date_range:)
def totals_query(transactions_scope:, date_range:, excluded_kinds: Transaction::REPORT_EXCLUDED_KINDS)
sql_hash = Digest::MD5.hexdigest(transactions_scope.to_sql)
kinds_key = excluded_kinds.join(",")

Rails.cache.fetch([
"income_statement", "totals_query", "v2", family.id, user&.id, included_account_ids_hash, sql_hash, date_range.begin, date_range.end, family.entries_cache_version
]) { Totals.new(family, transactions_scope: transactions_scope, date_range: date_range, included_account_ids: included_account_ids).call }
"income_statement", "totals_query", "v3", family.id, user&.id, included_account_ids_hash, sql_hash,
date_range.begin, date_range.end, kinds_key, family.entries_cache_version
]) {
Totals.new(
family,
transactions_scope: transactions_scope,
date_range: date_range,
included_account_ids: included_account_ids,
excluded_kinds: excluded_kinds
).call
}
end

def monetizable_currency
Expand Down
12 changes: 7 additions & 5 deletions app/models/income_statement/totals.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
class IncomeStatement::Totals
def initialize(family, transactions_scope:, date_range:, include_trades: true, included_account_ids: nil)
def initialize(family, transactions_scope:, date_range:, include_trades: true, included_account_ids: nil,
excluded_kinds: Transaction::REPORT_EXCLUDED_KINDS)
@family = family
@transactions_scope = transactions_scope
@date_range = date_range
@include_trades = include_trades
@included_account_ids = included_account_ids
@excluded_kinds = excluded_kinds

validate_date_range!
end
Expand Down Expand Up @@ -73,7 +75,7 @@ def transactions_only_query_sql
er.from_currency = ae.currency AND
er.to_currency = :target_currency
)
WHERE at.kind NOT IN (#{budget_excluded_kinds_sql})
WHERE at.kind NOT IN (#{excluded_kinds_sql})
AND ae.excluded = false
AND a.family_id = :family_id
AND a.status IN ('draft', 'active')
Expand Down Expand Up @@ -101,7 +103,7 @@ def transactions_subquery_sql
er.from_currency = ae.currency AND
er.to_currency = :target_currency
)
WHERE at.kind NOT IN (#{budget_excluded_kinds_sql})
WHERE at.kind NOT IN (#{excluded_kinds_sql})
AND (
at.investment_activity_label IS NULL
OR at.investment_activity_label NOT IN ('Transfer', 'Sweep In', 'Sweep Out', 'Exchange')
Expand Down Expand Up @@ -159,8 +161,8 @@ def include_finance_accounts_sql
"AND a.id IN (:included_account_ids)"
end

def budget_excluded_kinds_sql
@budget_excluded_kinds_sql ||= Transaction::BUDGET_EXCLUDED_KINDS.map { |k| "'#{k}'" }.join(", ")
def excluded_kinds_sql
@excluded_kinds_sql ||= @excluded_kinds.map { |k| ActiveRecord::Base.connection.quote(k.to_s) }.join(", ")
end

def validate_date_range!
Expand Down
9 changes: 6 additions & 3 deletions app/models/transaction.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,18 +71,21 @@ def exchange_rate_must_be_valid
funds_movement: "funds_movement", # Movement of funds between accounts, excluded from budget analytics
cc_payment: "cc_payment", # A CC payment, excluded from budget analytics (CC payments offset the sum of expense transactions)
loan_payment: "loan_payment", # A payment to a Loan account, treated as an expense in budgets
one_time: "one_time", # A one-time expense/income, excluded from budget analytics
one_time: "one_time", # A one-time expense/income; included in historical reports, excluded from budget medians
investment_contribution: "investment_contribution" # Transfer to investment/crypto account, treated as an expense in budgets
}

# All kinds where money moves between accounts (transfer? returns true).
# Used for search filters, rule conditions, and UI display.
TRANSFER_KINDS = %w[funds_movement cc_payment loan_payment investment_contribution].freeze

# Kinds excluded from budget/income-statement analytics.
# Kinds excluded from historical reports and period totals (transfers / CC payments only).
REPORT_EXCLUDED_KINDS = %w[funds_movement cc_payment].freeze

# Kinds excluded from budget medians, burn-rate stats, and budget category actuals.
# loan_payment and investment_contribution are intentionally NOT here —
# they represent real cash outflow from a budgeting perspective.
BUDGET_EXCLUDED_KINDS = %w[funds_movement one_time cc_payment].freeze
BUDGET_EXCLUDED_KINDS = (REPORT_EXCLUDED_KINDS + %w[one_time]).freeze

# All valid investment activity labels (for UI dropdown)
ACTIVITY_LABELS = [
Expand Down
2 changes: 1 addition & 1 deletion config/locales/views/transactions/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ en:
activity_type: Activity Type
activity_type_description: Type of investment activity (Buy, Sell, Dividend, etc.). Auto-detected or set manually.
one_time_title: One-time %{type}
one_time_description: One-time transactions will be excluded from certain budgeting calculations and reports to help you see what's really important.
one_time_description: One-time transactions appear in historical reports and totals but are excluded from budget medians and recurring spending estimates.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The English copy is now correct, but the other locales (es, ca, de, hu, pl, and friends) still say one-time transactions are "excluded from certain budgeting calculations and reports", which describes the pre-PR behavior and is now wrong for non-English users. Worth updating them in the same pass, per the repo's i18n guideline of keeping locale files in sync with changed copy.

convert_to_trade_title: Convert to Security Trade
convert_to_trade_description: Convert this transaction into a Buy or Sell trade with security details for portfolio tracking.
convert_to_trade_button: Convert to Trade
Expand Down
4 changes: 2 additions & 2 deletions test/controllers/pages_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@ class PagesControllerTest < ActionDispatch::IntegrationTest
)

income_statement.expects(:build_period_total)
.with(classification: "expense", period: kind_of(Period))
.with(classification: "expense", period: kind_of(Period), for_budget: false)
.once
.returns(fake_expense_period_total)

income_statement.expects(:build_period_total)
.with(classification: "income", period: kind_of(Period))
.with(classification: "income", period: kind_of(Period), for_budget: false)
.once
.returns(fake_income_period_total)

Expand Down
51 changes: 51 additions & 0 deletions test/models/budget_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -453,4 +453,55 @@ class BudgetTest < ActiveSupport::TestCase
# Other Investments synthetic categories previously caused this to return 0
assert spending >= 75, "Uncategorized actual spending should include the $75 transaction, got #{spending}"
end

test "actual_spending and actual_income exclude one-time transactions" do
family = families(:empty)
account = family.accounts.create!(
name: "Checking",
currency: family.currency,
balance: 5000,
accountable: Depository.new
)
income_category = family.categories.create!(name: "Income")
expense_category = family.categories.create!(name: "Groceries")

Entry.create!(
account: account,
entryable: Transaction.create!(category: expense_category, kind: "standard"),
date: Date.current,
name: "Groceries",
amount: 100,
currency: family.currency
)
Entry.create!(
account: account,
entryable: Transaction.create!(category: expense_category, kind: "one_time"),
date: Date.current,
name: "One-time expense",
amount: 250,
currency: family.currency
)
Entry.create!(
account: account,
entryable: Transaction.create!(category: income_category, kind: "standard"),
date: Date.current,
name: "Paycheck",
amount: -500,
currency: family.currency
)
Entry.create!(
account: account,
entryable: Transaction.create!(category: income_category, kind: "one_time"),
date: Date.current,
name: "One-time bonus",
amount: -300,
currency: family.currency
)

budget = Budget.find_or_bootstrap(family, start_date: Date.current.beginning_of_month)
budget = Budget.find(budget.id)

assert_equal 100, budget.actual_spending
assert_equal 500, budget.actual_income
end
end
24 changes: 17 additions & 7 deletions test/models/income_statement_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ class IncomeStatementTest < ActiveSupport::TestCase
expense_period_total = IncomeStatement::PeriodTotal.new("expense", 900, @family.currency, [])
income_period_total = IncomeStatement::PeriodTotal.new("income", 1000, @family.currency, [])

income_statement.expects(:build_period_total).with(classification: "expense", period: period).once.returns(expense_period_total)
income_statement.expects(:build_period_total).with(classification: "income", period: period).once.returns(income_period_total)
income_statement.expects(:build_period_total).with(classification: "expense", period: period, for_budget: false).once.returns(expense_period_total)
income_statement.expects(:build_period_total).with(classification: "income", period: period, for_budget: false).once.returns(income_period_total)

income_statement.net_category_totals(period: period)
income_statement.expense_totals(period: period)
Expand Down Expand Up @@ -195,17 +195,27 @@ class IncomeStatementTest < ActiveSupport::TestCase
assert_equal Money.new(1900, @family.currency), totals.expense_money # 900 + 1000
end

test "excludes one-time transactions from income statement calculations" do
# Create a one-time transaction
test "includes one-time transactions in income statement totals and reports" do
create_transaction(account: @checking_account, amount: 250, category: @groceries_category, kind: "one_time")

income_statement = IncomeStatement.new(@family)
totals = income_statement.totals(date_range: Period.last_30_days.date_range)

# NOW WORKING: Excludes one-time transactions correctly after refactoring
assert_equal 4, totals.transactions_count # Only original 4 transactions
assert_equal 5, totals.transactions_count
assert_equal Money.new(1000, @family.currency), totals.income_money
assert_equal Money.new(900, @family.currency), totals.expense_money
assert_equal Money.new(1150, @family.currency), totals.expense_money

expense_totals = income_statement.expense_totals(period: Period.last_30_days)
assert_equal 200 + 300 + 400 + 250, expense_totals.total
end

test "excludes one-time transactions from budget-scoped period totals" do
create_transaction(account: @checking_account, amount: 250, category: @groceries_category, kind: "one_time")

income_statement = IncomeStatement.new(@family)
expense_totals = income_statement.expense_totals(period: Period.last_30_days, for_budget: true)

assert_equal 200 + 300 + 400, expense_totals.total
end

test "excludes payment transactions from income statement calculations" do
Expand Down
Loading