From 4a8a3fa2de091a904d40d06f981ea04f63341d82 Mon Sep 17 00:00:00 2001 From: Gavin Matthews Date: Mon, 22 Jun 2026 23:14:29 +0000 Subject: [PATCH 1/2] feat(up): flag internal transfers and round-ups as funds_movement Up populates relationships.transferAccount on transactions that move money between the user's own accounts (including round-ups swept into a Saver), but flatten_transaction dropped it, so these imported as ordinary income/expense and distorted budgets and cashflow. - Provider::Up#flatten_transaction: lift transfer_account_id from relationships.transferAccount.data.id. - UpEntry::Processor: import transfers as funds_movement and persist transfer_account_id in extra["up"]. - Account::ProviderImportAdapter#import_transaction: optional kind: param; an explicit provider kind takes precedence over account-type auto-detection and is applied after the sync-protection check, so user re-categorisations survive re-sync. Complementary to Family#auto_match_transfers!: two-sided transfers between linked accounts are still paired into a Transfer (the matcher does not filter on kind); one-sided movements and round-ups, which the matcher cannot pair, are the cases this fixes. Co-Authored-By: Claude Opus 4.8 --- app/models/account/provider_import_adapter.rb | 29 ++++++++------ app/models/provider/up.rb | 8 +++- app/models/up_entry/processor.rb | 18 +++++++++ test/models/provider/up_test.rb | 38 +++++++++++++++++++ test/models/up_entry/processor_test.rb | 38 +++++++++++++++++++ 5 files changed, 118 insertions(+), 13 deletions(-) diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index 61a095e324..743e69046a 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -26,7 +26,7 @@ def reset_skipped_entries! # @param extra [Hash, nil] Optional provider-specific metadata to merge into transaction.extra # @param investment_activity_label [String, nil] Optional activity type label (e.g., "Buy", "Dividend") # @return [Entry] The created or updated entry - def import_transaction(external_id:, amount:, currency:, date:, name:, source:, category_id: nil, merchant: nil, notes: nil, pending_transaction_id: nil, extra: nil, investment_activity_label: nil) + def import_transaction(external_id:, amount:, currency:, date:, name:, source:, category_id: nil, kind: nil, merchant: nil, notes: nil, pending_transaction_id: nil, extra: nil, investment_activity_label: nil) raise ArgumentError, "external_id is required" if external_id.blank? raise ArgumentError, "source is required" if source.blank? @@ -208,18 +208,23 @@ def import_transaction(external_id:, amount:, currency:, date:, name:, source:, detected_label = detect_activity_label(name, amount) end - # Auto-set kind for internal movements and contributions - auto_kind = nil + # Determine the transaction kind. An explicit kind supplied by the provider takes + # precedence over the account-type auto-detection below: a provider such as Up that + # flags internal transfers and round-ups (via relationships.transferAccount) has + # authoritative knowledge that the movement is a transfer, so we honour it directly. + auto_kind = kind.presence auto_category = nil - if Transaction::INTERNAL_MOVEMENT_LABELS.include?(detected_label) - auto_kind = "funds_movement" - elsif detected_label == "Contribution" - auto_kind = "investment_contribution" - auto_category = account.family.investment_contributions_category - elsif account.accountable_type == "Loan" && amount.negative? - auto_kind = "loan_payment" - elsif account.accountable_type == "CreditCard" && amount.negative? - auto_kind = "cc_payment" + if auto_kind.nil? + if Transaction::INTERNAL_MOVEMENT_LABELS.include?(detected_label) + auto_kind = "funds_movement" + elsif detected_label == "Contribution" + auto_kind = "investment_contribution" + auto_category = account.family.investment_contributions_category + elsif account.accountable_type == "Loan" && amount.negative? + auto_kind = "loan_payment" + elsif account.accountable_type == "CreditCard" && amount.negative? + auto_kind = "cc_payment" + end end # Set investment activity label, kind, and category if detected diff --git a/app/models/provider/up.rb b/app/models/provider/up.rb index 1346b03c84..9b0f843b36 100644 --- a/app/models/provider/up.rb +++ b/app/models/provider/up.rb @@ -92,6 +92,11 @@ def flatten_account(resource) # Flattens a JSON:API transaction resource, lifting attributes to the top level and # extracting the related account/category ids from relationships. + # + # transfer_account_id is the other side of an internal money movement: Up populates + # relationships.transferAccount on any transaction that moves funds between the + # user's own accounts (including round-ups swept into a Saver). It is nil for + # ordinary income/expense. def flatten_transaction(resource) data = resource.with_indifferent_access attributes = data[:attributes].is_a?(Hash) ? data[:attributes] : {} @@ -99,7 +104,8 @@ def flatten_transaction(resource) attributes.merge( id: data[:id], account_id: data.dig(:relationships, :account, :data, :id), - category_id: data.dig(:relationships, :category, :data, :id) + category_id: data.dig(:relationships, :category, :data, :id), + transfer_account_id: data.dig(:relationships, :transferAccount, :data, :id) ).with_indifferent_access end diff --git a/app/models/up_entry/processor.rb b/app/models/up_entry/processor.rb index 52d6c760cc..59e9b9e890 100644 --- a/app/models/up_entry/processor.rb +++ b/app/models/up_entry/processor.rb @@ -53,6 +53,7 @@ def process date: date, name: name, source: "up", + kind: kind, merchant: merchant, notes: notes, extra: extra_metadata @@ -98,6 +99,22 @@ def name data[:description].presence || I18n.t("transactions.unknown_name") end + # The id of the other account in an internal money movement, if any (see + # Provider::Up#flatten_transaction). Present for transfers between the user's own + # accounts and for round-ups swept into a Saver; nil for ordinary income/expense. + def transfer_account_id + data[:transfer_account_id].presence + end + + # Mark internal movements as funds_movement so they are excluded from income, + # expense, and budget analytics. Two-sided transfers between two linked accounts are + # additionally paired into a Transfer by Family#auto_match_transfers!; one-sided moves + # (counterpart not linked in Sure) and round-ups rely on this flag, since the matcher + # has no opposing entry to pair them with. + def kind + transfer_account_id ? "funds_movement" : nil + end + # Optional user-entered message attached to the transaction. def notes data[:message].presence @@ -170,6 +187,7 @@ def extra_metadata "pending" => pending?, "status" => data[:status], "category_id" => data[:category_id], + "transfer_account_id" => transfer_account_id, "raw_text" => data[:rawText], "fx_from" => foreign_amount_data[:currencyCode], "fx_amount" => foreign_amount_data[:value] diff --git a/test/models/provider/up_test.rb b/test/models/provider/up_test.rb index 332ffc0513..3ac1c97d49 100644 --- a/test/models/provider/up_test.rb +++ b/test/models/provider/up_test.rb @@ -138,6 +138,44 @@ class Provider::UpTest < ActiveSupport::TestCase end end + test "flattens transaction relationships including transferAccount" do + response = FakeResponse.new( + code: 200, + message: "OK", + body: { + data: [ + { + type: "transactions", id: "tx_xfer", + attributes: { status: "SETTLED", description: "Transfer to Savings" }, + relationships: { + account: { data: { id: "acc_123" } }, + category: { data: nil }, + transferAccount: { data: { id: "acc_saver" } } + } + }, + { + type: "transactions", id: "tx_plain", + attributes: { status: "SETTLED", description: "Coffee" }, + relationships: { + account: { data: { id: "acc_123" } }, + category: { data: { id: "restaurants-and-cafes" } }, + transferAccount: { data: nil } + } + } + ], + links: { prev: nil, next: nil } + }.to_json + ) + + Provider::Up.stub(:get, ->(_url, headers:, query: nil) { response }) do + transactions = Provider::Up.new("up-access-token").get_account_transactions(account_id: "acc_123") + + assert_equal "acc_saver", transactions.first[:transfer_account_id] + assert_equal "restaurants-and-cafes", transactions.second[:category_id] + assert_nil transactions.second[:transfer_account_id] + end + end + test "raises typed errors for unauthorized responses" do response = FakeResponse.new(code: 401, message: "Unauthorized", body: "{}") diff --git a/test/models/up_entry/processor_test.rb b/test/models/up_entry/processor_test.rb index 45ba6228c3..d7d07c4048 100644 --- a/test/models/up_entry/processor_test.rb +++ b/test/models/up_entry/processor_test.rb @@ -113,4 +113,42 @@ class UpEntry::ProcessorTest < ActiveSupport::TestCase assert_equal BigDecimal("-2500.0"), entry.amount end + + test "marks internal transfers (transferAccount present) as funds_movement" do + entry = UpEntry::Processor.new( + { + id: "tx_transfer_1", + account_id: "acc_123", + status: "SETTLED", + description: "Transfer to 2Up Spending", + amount: { currencyCode: "AUD", value: "-500.00", valueInBaseUnits: -50000 }, + settledAt: "2026-01-22T00:00:00+11:00", + createdAt: "2026-01-22T00:00:00+11:00", + transfer_account_id: "acc_other" + }, + up_account: @up_account + ).process + + transaction = entry.entryable + assert_equal "funds_movement", transaction.kind + assert_equal "acc_other", transaction.extra.dig("up", "transfer_account_id") + end + + test "ordinary transactions (no transferAccount) keep the standard kind" do + entry = UpEntry::Processor.new( + { + id: "tx_standard_1", + account_id: "acc_123", + status: "SETTLED", + description: "Coffee Shop", + amount: { currencyCode: "AUD", value: "-4.50", valueInBaseUnits: -450 }, + settledAt: "2026-01-22T00:00:00+11:00", + createdAt: "2026-01-22T00:00:00+11:00" + }, + up_account: @up_account + ).process + + assert_equal "standard", entry.entryable.kind + assert_nil entry.entryable.extra.dig("up", "transfer_account_id") + end end From a31160ed459bf27884d0e5dd365d08f2ba31c667 Mon Sep 17 00:00:00 2001 From: Gavin Matthews Date: Thu, 25 Jun 2026 06:15:22 +0000 Subject: [PATCH 2/2] fix(up): account-type kind wins over provider transfer hint Codex review caught that Up HOME_LOAN accounts map to a Loan account, so a repayment carrying transferAccount would be reclassified from loan_payment to funds_movement (budget-excluded). Make the provider kind: a fallback: activity-label and account-type classification now take precedence, so loan_payment and cc_payment survive. Adds a regression test (loan repayment stays loan_payment), a depository-applies test, and a note that the up_test stub ignores query: intentionally. Co-Authored-By: Claude Opus 4.8 --- app/models/account/provider_import_adapter.rb | 34 ++++++++++--------- .../account/provider_import_adapter_test.rb | 31 +++++++++++++++++ test/models/provider/up_test.rb | 3 ++ 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index 743e69046a..483c20bc52 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -208,24 +208,26 @@ def import_transaction(external_id:, amount:, currency:, date:, name:, source:, detected_label = detect_activity_label(name, amount) end - # Determine the transaction kind. An explicit kind supplied by the provider takes - # precedence over the account-type auto-detection below: a provider such as Up that - # flags internal transfers and round-ups (via relationships.transferAccount) has - # authoritative knowledge that the movement is a transfer, so we honour it directly. - auto_kind = kind.presence + # Determine the transaction kind. Activity-label and account-type classification + # take precedence; an explicit kind supplied by the provider is used as a fallback + # for the standard case. A provider such as Up flags internal transfers and + # round-ups (via relationships.transferAccount) and passes funds_movement, but a + # repayment imported onto a linked Loan/CreditCard account must stay + # loan_payment/cc_payment (a budgeted expense) rather than being reclassified, so + # the account-type branches below win over the provider hint. + auto_kind = nil auto_category = nil - if auto_kind.nil? - if Transaction::INTERNAL_MOVEMENT_LABELS.include?(detected_label) - auto_kind = "funds_movement" - elsif detected_label == "Contribution" - auto_kind = "investment_contribution" - auto_category = account.family.investment_contributions_category - elsif account.accountable_type == "Loan" && amount.negative? - auto_kind = "loan_payment" - elsif account.accountable_type == "CreditCard" && amount.negative? - auto_kind = "cc_payment" - end + if Transaction::INTERNAL_MOVEMENT_LABELS.include?(detected_label) + auto_kind = "funds_movement" + elsif detected_label == "Contribution" + auto_kind = "investment_contribution" + auto_category = account.family.investment_contributions_category + elsif account.accountable_type == "Loan" && amount.negative? + auto_kind = "loan_payment" + elsif account.accountable_type == "CreditCard" && amount.negative? + auto_kind = "cc_payment" end + auto_kind ||= kind.presence # Set investment activity label, kind, and category if detected if entry.entryable.is_a?(Transaction) diff --git a/test/models/account/provider_import_adapter_test.rb b/test/models/account/provider_import_adapter_test.rb index 29ab62fb89..6927ad30af 100644 --- a/test/models/account/provider_import_adapter_test.rb +++ b/test/models/account/provider_import_adapter_test.rb @@ -55,6 +55,37 @@ class Account::ProviderImportAdapterTest < ActiveSupport::TestCase end end + test "applies an explicit provider kind on a depository account" do + entry = @adapter.import_transaction( + external_id: "up_transfer_1", + amount: -50.00, + currency: "USD", + date: Date.today, + name: "Transfer to Savings", + source: "up", + kind: "funds_movement" + ) + + assert_equal "funds_movement", entry.transaction.kind + end + + test "account-type kind takes precedence over an explicit provider kind" do + loan_adapter = Account::ProviderImportAdapter.new(accounts(:loan)) + + entry = loan_adapter.import_transaction( + external_id: "up_loan_repayment_1", + amount: -200.00, + currency: "USD", + date: Date.today, + name: "Home Loan Repayment", + source: "up", + kind: "funds_movement" + ) + + assert_equal "loan_payment", entry.transaction.kind, + "a repayment on a Loan account must stay loan_payment, not the provider's funds_movement" + end + test "updates existing transaction instead of creating duplicate" do # Create initial transaction entry = @adapter.import_transaction( diff --git a/test/models/provider/up_test.rb b/test/models/provider/up_test.rb index 3ac1c97d49..297c0866c4 100644 --- a/test/models/provider/up_test.rb +++ b/test/models/provider/up_test.rb @@ -167,6 +167,9 @@ class Provider::UpTest < ActiveSupport::TestCase }.to_json ) + # The stub deliberately ignores the query: keyword: this test exercises only + # response flattening, not the request params (pagination/date filters), which + # are covered by the pagination tests above. Provider::Up.stub(:get, ->(_url, headers:, query: nil) { response }) do transactions = Provider::Up.new("up-access-token").get_account_transactions(account_id: "acc_123")