-
Notifications
You must be signed in to change notification settings - Fork 292
fix(transactions): include one-time in reports, keep out of budget medians #1995
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
51eb140
ba88d96
0afb039
8c667a9
b0ef1dc
1801a2f
c7235c1
8d58a2f
53a124f
24a3ab8
d937faa
b5b44b2
5f6399a
666710c
6535d8e
c48b7fc
58c1d6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
# line 294
def actual_income
family.income_statement.income_totals(period: self.period).total
end
Concrete effect: Fix — route through the private memoized helper so it matches def actual_income
income_totals.total
endAlso worth adding a |
||
| end | ||
|
|
||
| def expense_totals_by_category | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leftover from my last review:
Suggested change
|
||||||||
| @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) { | ||||||||
|
|
@@ -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, | ||||||||
|
|
@@ -141,9 +143,14 @@ def period_cache_key(period) | |||||||
| [ period.start_date, period.end_date ] | ||||||||
| end | ||||||||
|
|
||||||||
| def build_period_total(classification:, period:) | ||||||||
| def build_period_total(classification:, period:, for_budget: false) | ||||||||
| excluded_kinds = for_budget ? Transaction::BUDGET_EXCLUDED_KINDS : Transaction::REPORT_EXCLUDED_KINDS | ||||||||
| # Exclude pending transactions from budget calculations | ||||||||
| totals = totals_query(transactions_scope: family.transactions.visible.excluding_pending.in_period(period), date_range: period.date_range).select { |t| t.classification == classification } | ||||||||
| totals = totals_query( | ||||||||
| transactions_scope: family.transactions.visible.excluding_pending.in_period(period), | ||||||||
| date_range: period.date_range, | ||||||||
| excluded_kinds: excluded_kinds | ||||||||
| ).select { |t| t.classification == classification } | ||||||||
| classification_total = totals.sum(&:total) | ||||||||
|
|
||||||||
| uncategorized_category = family.categories.uncategorized | ||||||||
|
|
@@ -208,12 +215,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 | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.