diff --git a/app/controllers/reports_controller.rb b/app/controllers/reports_controller.rb index 8dfea80ee..d76a82c12 100644 --- a/app/controllers/reports_controller.rb +++ b/app/controllers/reports_controller.rb @@ -350,7 +350,7 @@ class ReportsController < ApplicationController .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: [ "funds_movement", "one_time", "cc_payment" ]) + .where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS) .includes(entry: :account, category: :parent) # Apply filters @@ -611,7 +611,7 @@ class ReportsController < ApplicationController .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: [ "funds_movement", "one_time", "cc_payment" ]) + .where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS) .includes(entry: :account, category: []) transactions = apply_transaction_filters(transactions) @@ -648,7 +648,7 @@ class ReportsController < ApplicationController .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: [ "funds_movement", "one_time", "cc_payment" ]) + .where.not(kind: Transaction::BUDGET_EXCLUDED_KINDS) .includes(entry: :account, category: []) transactions = apply_transaction_filters(transactions) diff --git a/app/controllers/transfer_matches_controller.rb b/app/controllers/transfer_matches_controller.rb index e07aa5614..ea9425039 100644 --- a/app/controllers/transfer_matches_controller.rb +++ b/app/controllers/transfer_matches_controller.rb @@ -49,6 +49,7 @@ class TransferMatchesController < ApplicationController currency: @entry.currency, date: @entry.date, name: "Transfer to #{@entry.amount.negative? ? @entry.account.name : target_account.name}", + user_modified: true, ) ) diff --git a/app/models/income_statement/category_stats.rb b/app/models/income_statement/category_stats.rb index 6f774722e..0476549e4 100644 --- a/app/models/income_statement/category_stats.rb +++ b/app/models/income_statement/category_stats.rb @@ -38,6 +38,10 @@ class IncomeStatement::CategoryStats params end + def budget_excluded_kinds_sql + @budget_excluded_kinds_sql ||= Transaction::BUDGET_EXCLUDED_KINDS.map { |k| "'#{k}'" }.join(", ") + end + def exclude_tax_advantaged_sql ids = @family.tax_advantaged_account_ids return "" if ids.empty? @@ -62,7 +66,7 @@ class IncomeStatement::CategoryStats er.to_currency = :target_currency ) WHERE a.family_id = :family_id - AND t.kind NOT IN ('funds_movement', 'one_time', 'cc_payment') + AND t.kind NOT IN (#{budget_excluded_kinds_sql}) AND ae.excluded = false AND (t.extra -> 'simplefin' ->> 'pending')::boolean IS DISTINCT FROM true AND (t.extra -> 'plaid' ->> 'pending')::boolean IS DISTINCT FROM true diff --git a/app/models/income_statement/family_stats.rb b/app/models/income_statement/family_stats.rb index 73959e9a3..8ce0e2e2d 100644 --- a/app/models/income_statement/family_stats.rb +++ b/app/models/income_statement/family_stats.rb @@ -37,6 +37,10 @@ class IncomeStatement::FamilyStats params end + def budget_excluded_kinds_sql + @budget_excluded_kinds_sql ||= Transaction::BUDGET_EXCLUDED_KINDS.map { |k| "'#{k}'" }.join(", ") + end + def exclude_tax_advantaged_sql ids = @family.tax_advantaged_account_ids return "" if ids.empty? @@ -59,7 +63,7 @@ class IncomeStatement::FamilyStats er.to_currency = :target_currency ) WHERE a.family_id = :family_id - AND t.kind NOT IN ('funds_movement', 'one_time', 'cc_payment') + AND t.kind NOT IN (#{budget_excluded_kinds_sql}) AND ae.excluded = false AND (t.extra -> 'simplefin' ->> 'pending')::boolean IS DISTINCT FROM true AND (t.extra -> 'plaid' ->> 'pending')::boolean IS DISTINCT FROM true diff --git a/app/models/income_statement/totals.rb b/app/models/income_statement/totals.rb index 028de5638..398bd1377 100644 --- a/app/models/income_statement/totals.rb +++ b/app/models/income_statement/totals.rb @@ -69,7 +69,7 @@ class IncomeStatement::Totals er.from_currency = ae.currency AND er.to_currency = :target_currency ) - WHERE at.kind NOT IN ('funds_movement', 'one_time', 'cc_payment') + WHERE at.kind NOT IN (#{budget_excluded_kinds_sql}) AND ae.excluded = false AND a.family_id = :family_id AND a.status IN ('draft', 'active') @@ -96,7 +96,7 @@ class IncomeStatement::Totals er.from_currency = ae.currency AND er.to_currency = :target_currency ) - WHERE at.kind NOT IN ('funds_movement', 'one_time', 'cc_payment') + WHERE at.kind NOT IN (#{budget_excluded_kinds_sql}) AND ( at.investment_activity_label IS NULL OR at.investment_activity_label NOT IN ('Transfer', 'Sweep In', 'Sweep Out', 'Exchange') @@ -144,6 +144,10 @@ class IncomeStatement::Totals "AND a.id NOT IN (:tax_advantaged_account_ids)" end + def budget_excluded_kinds_sql + @budget_excluded_kinds_sql ||= Transaction::BUDGET_EXCLUDED_KINDS.map { |k| "'#{k}'" }.join(", ") + end + def validate_date_range! unless @date_range.is_a?(Range) raise ArgumentError, "date_range must be a Range, got #{@date_range.class}" diff --git a/app/models/rule/action_executor/set_as_transfer_or_payment.rb b/app/models/rule/action_executor/set_as_transfer_or_payment.rb index d4e1939dc..9f3a79b12 100644 --- a/app/models/rule/action_executor/set_as_transfer_or_payment.rb +++ b/app/models/rule/action_executor/set_as_transfer_or_payment.rb @@ -46,6 +46,7 @@ class Rule::ActionExecutor::SetAsTransferOrPayment < Rule::ActionExecutor currency: entry.currency, date: entry.date, name: "#{target_account.liability? ? "Payment" : "Transfer"} #{entry.amount.negative? ? "to #{target_account.name}" : "from #{entry.account.name}"}", + user_modified: true, ) ) diff --git a/app/models/rule/condition_filter/transaction_type.rb b/app/models/rule/condition_filter/transaction_type.rb index 4c852d236..f63f278f8 100644 --- a/app/models/rule/condition_filter/transaction_type.rb +++ b/app/models/rule/condition_filter/transaction_type.rb @@ -1,7 +1,4 @@ class Rule::ConditionFilter::TransactionType < Rule::ConditionFilter - # Transfer kinds matching Transaction#transfer? method - TRANSFER_KINDS = %w[funds_movement cc_payment loan_payment].freeze - def type "select" end @@ -26,15 +23,13 @@ class Rule::ConditionFilter::TransactionType < Rule::ConditionFilter # Logic matches Transaction::Search#apply_type_filter for consistency case value when "income" - # Negative amounts, excluding transfers and investment_contribution scope.where("entries.amount < 0") - .where.not(kind: TRANSFER_KINDS + %w[investment_contribution]) + .where.not(kind: Transaction::TRANSFER_KINDS) when "expense" - # Positive amounts OR investment_contribution (regardless of sign), excluding transfers - scope.where("entries.amount >= 0 OR transactions.kind = 'investment_contribution'") - .where.not(kind: TRANSFER_KINDS) + scope.where("entries.amount >= 0") + .where.not(kind: Transaction::TRANSFER_KINDS) when "transfer" - scope.where(kind: TRANSFER_KINDS) + scope.where(kind: Transaction::TRANSFER_KINDS) else scope end diff --git a/app/models/transaction.rb b/app/models/transaction.rb index 4f94bb488..397703034 100644 --- a/app/models/transaction.rb +++ b/app/models/transaction.rb @@ -17,9 +17,18 @@ class Transaction < ApplicationRecord 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 - investment_contribution: "investment_contribution" # Transfer to investment/crypto account, excluded from budget analytics + 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. + # 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 + # All valid investment activity labels (for UI dropdown) ACTIVITY_LABELS = [ "Buy", "Sell", "Sweep In", "Sweep Out", "Dividend", "Reinvestment", @@ -54,7 +63,7 @@ class Transaction < ApplicationRecord # Overarching grouping method for all transfer-type transactions def transfer? - funds_movement? || cc_payment? || loan_payment? + TRANSFER_KINDS.include?(kind) end def set_category!(category) diff --git a/app/models/transaction/search.rb b/app/models/transaction/search.rb index fa51fb55d..3ea2a2391 100644 --- a/app/models/transaction/search.rb +++ b/app/models/transaction/search.rb @@ -57,8 +57,14 @@ class Transaction::Search result = scope .select( - "COALESCE(SUM(CASE WHEN transactions.kind = 'investment_contribution' THEN ABS(entries.amount * COALESCE(er.rate, 1)) WHEN entries.amount >= 0 AND transactions.kind NOT IN ('funds_movement', 'cc_payment') THEN ABS(entries.amount * COALESCE(er.rate, 1)) ELSE 0 END), 0) as expense_total", - "COALESCE(SUM(CASE WHEN entries.amount < 0 AND transactions.kind NOT IN ('funds_movement', 'cc_payment', 'investment_contribution') THEN ABS(entries.amount * COALESCE(er.rate, 1)) ELSE 0 END), 0) as income_total", + ActiveRecord::Base.sanitize_sql_array([ + "COALESCE(SUM(CASE WHEN entries.amount >= 0 AND transactions.kind NOT IN (?) THEN ABS(entries.amount * COALESCE(er.rate, 1)) ELSE 0 END), 0) as expense_total", + Transaction::TRANSFER_KINDS + ]), + ActiveRecord::Base.sanitize_sql_array([ + "COALESCE(SUM(CASE WHEN entries.amount < 0 AND transactions.kind NOT IN (?) THEN ABS(entries.amount * COALESCE(er.rate, 1)) ELSE 0 END), 0) as income_total", + Transaction::TRANSFER_KINDS + ]), "COUNT(entries.id) as transactions_count" ) .joins( @@ -110,14 +116,14 @@ class Transaction::Search # Get parent category IDs for the given category names parent_category_ids = family.categories.where(name: real_categories).pluck(:id) - uncategorized_condition = "(categories.id IS NULL AND transactions.kind NOT IN ('funds_movement', 'cc_payment'))" + uncategorized_condition = "categories.id IS NULL AND transactions.kind NOT IN (?)" # Build condition based on whether parent_category_ids is empty if parent_category_ids.empty? if include_uncategorized query = query.left_joins(:category).where( - "categories.name IN (?) OR #{uncategorized_condition}", - real_categories.presence || [] + "categories.name IN (?) OR (#{uncategorized_condition})", + real_categories.presence || [], Transaction::TRANSFER_KINDS ) else query = query.left_joins(:category).where(categories: { name: real_categories }) @@ -125,8 +131,8 @@ class Transaction::Search else if include_uncategorized query = query.left_joins(:category).where( - "categories.name IN (?) OR categories.parent_id IN (?) OR #{uncategorized_condition}", - real_categories, parent_category_ids + "categories.name IN (?) OR categories.parent_id IN (?) OR (#{uncategorized_condition})", + real_categories, parent_category_ids, Transaction::TRANSFER_KINDS ) else query = query.left_joins(:category).where( @@ -143,29 +149,22 @@ class Transaction::Search return query unless types.present? return query if types.sort == [ "expense", "income", "transfer" ] - transfer_condition = "transactions.kind IN ('funds_movement', 'cc_payment', 'loan_payment')" - # investment_contribution is always an expense regardless of amount sign - # (handles both manual outflows and provider-imported inflows like 401k contributions) - investment_contribution_condition = "transactions.kind = 'investment_contribution'" - expense_condition = "(entries.amount >= 0 OR #{investment_contribution_condition})" - income_condition = "(entries.amount <= 0 AND NOT #{investment_contribution_condition})" - - condition = case types.sort + case types.sort when [ "transfer" ] - transfer_condition + query.where(kind: Transaction::TRANSFER_KINDS) when [ "expense" ] - Arel.sql("#{expense_condition} AND NOT (#{transfer_condition})") + query.where("entries.amount >= 0").where.not(kind: Transaction::TRANSFER_KINDS) when [ "income" ] - Arel.sql("#{income_condition} AND NOT (#{transfer_condition})") + query.where("entries.amount < 0").where.not(kind: Transaction::TRANSFER_KINDS) when [ "expense", "transfer" ] - Arel.sql("#{expense_condition} OR #{transfer_condition}") + query.where("entries.amount >= 0 OR transactions.kind IN (?)", Transaction::TRANSFER_KINDS) when [ "income", "transfer" ] - Arel.sql("#{income_condition} OR #{transfer_condition}") + query.where("entries.amount < 0 OR transactions.kind IN (?)", Transaction::TRANSFER_KINDS) when [ "expense", "income" ] - Arel.sql("NOT (#{transfer_condition})") + query.where.not(kind: Transaction::TRANSFER_KINDS) + else + query end - - query.where(condition) end def apply_merchant_filter(query, merchants) diff --git a/test/controllers/transfer_matches_controller_test.rb b/test/controllers/transfer_matches_controller_test.rb index 51ba79a23..fa277659a 100644 --- a/test/controllers/transfer_matches_controller_test.rb +++ b/test/controllers/transfer_matches_controller_test.rb @@ -40,6 +40,22 @@ class TransferMatchesControllerTest < ActionDispatch::IntegrationTest assert_equal "Transfer created", flash[:notice] end + test "new transfer entry is protected from provider sync" do + outflow_entry = create_transaction(amount: 100, account: accounts(:depository)) + + post transaction_transfer_match_path(outflow_entry), params: { + transfer_match: { + method: "new", + target_account_id: accounts(:investment).id + } + } + + transfer = Transfer.order(created_at: :desc).first + new_entry = transfer.inflow_transaction.entry + + assert new_entry.user_modified?, "New transfer entry should be marked as user_modified to protect from provider sync" + end + test "assigns investment_contribution kind and category for investment destination" do # Outflow from depository (positive amount), target is investment outflow_entry = create_transaction(amount: 100, account: accounts(:depository)) diff --git a/test/models/rule/condition_test.rb b/test/models/rule/condition_test.rb index 774e77812..52ab40371 100644 --- a/test/models/rule/condition_test.rb +++ b/test/models/rule/condition_test.rb @@ -447,7 +447,7 @@ class Rule::ConditionTest < ActiveSupport::TestCase assert_not filtered.map(&:id).include?(transfer_entry.transaction.id) end - test "transaction_type expense includes investment_contribution regardless of amount sign" do + test "transaction_type transfer includes investment_contribution" do scope = @rule_scope # Create investment contribution with negative amount (inflow from provider) @@ -463,14 +463,27 @@ class Rule::ConditionTest < ActiveSupport::TestCase rule: @transaction_rule, condition_type: "transaction_type", operator: "=", - value: "expense" + value: "transfer" ) scope = condition.prepare(scope) filtered = condition.apply(scope) - # Should include investment_contribution even with negative amount + # investment_contribution is a transfer kind assert filtered.map(&:id).include?(contribution_entry.transaction.id) + + # Should NOT match expense filter + expense_condition = Rule::Condition.new( + rule: @transaction_rule, + condition_type: "transaction_type", + operator: "=", + value: "expense" + ) + + expense_scope = expense_condition.prepare(@rule_scope) + expense_filtered = expense_condition.apply(expense_scope) + + assert_not expense_filtered.map(&:id).include?(contribution_entry.transaction.id) end test "transaction_type income excludes investment_contribution" do diff --git a/test/models/transaction/search_test.rb b/test/models/transaction/search_test.rb index c933c368c..c6d817ce1 100644 --- a/test/models/transaction/search_test.rb +++ b/test/models/transaction/search_test.rb @@ -119,11 +119,10 @@ class Transaction::SearchTest < ActiveSupport::TestCase # Should include standard uncategorized transactions assert_includes uncategorized_ids, uncategorized_standard.entryable.id - # Should include loan_payment since it's treated specially in category logic - assert_includes uncategorized_ids, uncategorized_loan_payment.entryable.id - # Should exclude transfer transactions even if uncategorized + # Should exclude all transfer kinds (TRANSFER_KINDS) even if uncategorized assert_not_includes uncategorized_ids, uncategorized_transfer.entryable.id + assert_not_includes uncategorized_ids, uncategorized_loan_payment.entryable.id end test "filtering for only Uncategorized returns only uncategorized transactions" do @@ -237,10 +236,50 @@ class Transaction::SearchTest < ActiveSupport::TestCase # Test that the relation builds from family.transactions correctly assert_equal @family.transactions.joins(entry: :account).where( - "entries.amount >= 0 AND NOT (transactions.kind IN ('funds_movement', 'cc_payment', 'loan_payment'))" + "entries.amount >= 0 AND NOT (transactions.kind IN (?))", Transaction::TRANSFER_KINDS ).count, results.count end + test "transfer filter includes investment_contribution transactions" do + investment_contribution = create_transaction( + account: @checking_account, + amount: 500, + kind: "investment_contribution" + ) + + funds_movement = create_transaction( + account: @checking_account, + amount: 200, + kind: "funds_movement" + ) + + search = Transaction::Search.new(@family, filters: { types: [ "transfer" ] }) + result_ids = search.transactions_scope.pluck(:id) + + assert_includes result_ids, investment_contribution.entryable.id + assert_includes result_ids, funds_movement.entryable.id + end + + test "expense filter excludes investment_contribution transactions" do + investment_contribution = create_transaction( + account: @checking_account, + amount: 500, + kind: "investment_contribution" + ) + + standard_expense = create_transaction( + account: @checking_account, + amount: 100, + kind: "standard" + ) + + search = Transaction::Search.new(@family, filters: { types: [ "expense" ] }) + result_ids = search.transactions_scope.pluck(:id) + + assert_not_includes result_ids, investment_contribution.entryable.id + assert_includes result_ids, standard_expense.entryable.id + end + test "family-based API requires family parameter" do assert_raises(NoMethodError) do search = Transaction::Search.new({ types: [ "expense" ] }) diff --git a/test/models/transaction_test.rb b/test/models/transaction_test.rb index f4a93e74d..17018f778 100644 --- a/test/models/transaction_test.rb +++ b/test/models/transaction_test.rb @@ -32,6 +32,17 @@ class TransactionTest < ActiveSupport::TestCase assert transaction.investment_contribution? end + test "TRANSFER_KINDS constant matches transfer? method" do + Transaction::TRANSFER_KINDS.each do |kind| + assert Transaction.new(kind: kind).transfer?, "#{kind} should be a transfer kind" + end + + non_transfer_kinds = Transaction.kinds.keys - Transaction::TRANSFER_KINDS + non_transfer_kinds.each do |kind| + assert_not Transaction.new(kind: kind).transfer?, "#{kind} should NOT be a transfer kind" + end + end + test "all transaction kinds are valid" do valid_kinds = %w[standard funds_movement cc_payment loan_payment one_time investment_contribution] diff --git a/test/models/transfer/creator_test.rb b/test/models/transfer/creator_test.rb index 4f1a2a163..a88a33a94 100644 --- a/test/models/transfer/creator_test.rb +++ b/test/models/transfer/creator_test.rb @@ -28,6 +28,7 @@ class Transfer::CreatorTest < ActiveSupport::TestCase # Verify outflow transaction is marked as investment_contribution outflow = transfer.outflow_transaction assert_equal "investment_contribution", outflow.kind + assert outflow.transfer?, "investment_contribution should be recognized as a transfer" assert_equal @amount, outflow.entry.amount assert_equal @source_account.currency, outflow.entry.currency assert_equal "Transfer to #{@destination_account.name}", outflow.entry.name @@ -36,6 +37,7 @@ class Transfer::CreatorTest < ActiveSupport::TestCase # Verify inflow transaction (always funds_movement) inflow = transfer.inflow_transaction assert_equal "funds_movement", inflow.kind + assert inflow.transfer?, "funds_movement should be recognized as a transfer" assert_equal(@amount * -1, inflow.entry.amount) assert_equal @destination_account.currency, inflow.entry.currency assert_equal "Transfer from #{@source_account.name}", inflow.entry.name