feat(up): flag internal transfers and round-ups as funds_movement

[threatsurfer]

Summary

Up sets relationships.transferAccount on any movement between a user's own accounts (including round-ups swept into a Saver). flatten_transaction currently drops it, so these import as ordinary income/expense and distort budgets, cashflow, and the income statement. This PR imports them as funds_movement (Sure's budget-excluded transfer kind).

Why it matters (real data)

I run an independent Up integration on my own data (~7yr, ~26k txns, 9 accounts incl. a 2Up joint account and Savers). In a recent 536-txn snapshot, 299 (56%) had a non-null transferAccount (e.g. a -$500 'Transfer to 2Up Spending'). Across full history they number in the thousands. All currently land as income/expense.

Relationship to auto_match_transfers!

Complementary, not a replacement. Two-sided transfers between linked accounts are still paired into a Transfer (the matcher filters on amount/date/currency, not on kind). One-sided movements (counterpart not linked) and round-ups cannot be paired by the matcher; honouring transferAccount is the only thing that classifies these correctly.

Changes

1. Provider::Up#flatten_transaction lifts transfer_account_id from relationships.transferAccount.data.id.
2. UpEntry::Processor imports transfers as funds_movement and stores transfer_account_id in extra under the up key.
3. Account::ProviderImportAdapter#import_transaction gains an optional kind: param; an explicit provider kind wins over account-type auto-detection and is applied after the protection check, so user re-categorisations survive re-sync.

Scope / design

Kept to marking. A deterministic Transfer record from the transferAccount id is a sensible follow-up PR. Went with a shared-adapter kind: param (mirrors investment_activity_label:, respects user edits); happy to switch to an Up-contained approach if you prefer.

Tests

Processor: transferAccount maps to funds_movement; an ordinary transaction stays standard. Provider: flatten extracts transfer_account_id (nil when absent).

Summary by CodeRabbit

• New Features

  • Internal fund transfers between accounts are now detected and classified as funds_movement.
  • The counterpart transfer account is captured and stored on affected transactions for improved tracking.

• Bug Fixes

  • Transaction “kind” assignment now follows account-specific rules (e.g., loan accounts won’t accept an import kind that conflicts with their expected payment kind).

• Tests

  • Added coverage for transfer-account relationship flattening and for the updated transaction kind behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b28527ce-20d8-4346-82bd-c3e61e6420e2

📥 Commits

Reviewing files that changed from the base of the PR and between 4a8a3fa and a31160e.

📒 Files selected for processing (3)

• app/models/account/provider_import_adapter.rb
• test/models/account/provider_import_adapter_test.rb
• test/models/provider/up_test.rb

🚧 Files skipped from review as they are similar to previous changes (1)

• test/models/provider/up_test.rb

---

📝 Walkthrough

Walkthrough

Provider::Up#flatten_transaction now extracts transfer_account_id from relationships.transferAccount.data.id. UpEntry::Processor derives funds_movement from that field, passes kind: into import_transaction, and stores the transfer account id in extra_metadata. ProviderImportAdapter#import_transaction accepts the new keyword and uses it as a fallback behind auto-detection.

Changes

Up Internal Transfer Detection

Layer / File(s)	Summary
Provider::Up: flatten transferAccount relationship app/models/provider/up.rb, test/models/provider/up_test.rb	flatten_transaction reads relationships.transferAccount.data.id and adds transfer_account_id to the returned hash. Test stubs two JSON:API records to assert correct flattening including the nil case.
ProviderImportAdapter: accept explicit kind param app/models/account/provider_import_adapter.rb, test/models/account/provider_import_adapter_test.rb	import_transaction gains kind: nil and keeps account-type/activity-label detection authoritative, with provider kind used only when auto-detection is blank. Tests cover depository and loan account precedence.
UpEntry::Processor: derive and forward kind app/models/up_entry/processor.rb, test/models/up_entry/processor_test.rb	Processor adds transfer_account_id and kind helpers; process passes kind: kind to the adapter; extra_metadata stores transfer_account_id under the "up" key. Tests assert funds_movement kind and metadata when transfer_account_id is present, and standard kind with no metadata when absent.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hop, hop—through transfer trails,
A burrowed kind and metadata sails.
One little id from the Up stream flows,
And funds_movement blooms where the transfer shows.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main Up-provider change: internal transfers and round-ups are flagged as funds_movement.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[superagent-security]

Superagent didn't find any vulnerabilities or security issues in this PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4a8a3fa2de

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve Up loan repayments as loan_payment

When an Up HOME_LOAN account is linked (UpAccount::UP_ACCOUNT_TYPE_MAP maps HOME_LOAN to Loan), repayments from another Up account can also carry transferAccount. Because UpEntry::Processor#kind now passes funds_movement and this line skips the existing account.accountable_type == "Loan" && amount.negative? branch below, those repayments are reclassified from budgeted loan_payment to budget-excluded funds_movement. Keep the loan account-type classification ahead of the provider transfer override, or only pass the provider kind for depository accounts.

Useful? React with 👍 / 👎.

[jjmata]

Good approach and well-reasoned. A few things to verify or watch:

Kind precedence vs. protection check – The description says the explicit kind: param is "applied after the protection check, so user re-categorisations survive re-sync." Looking at the diff, auto_kind = kind.presence is set before the nil-guard block, but I can't see the downstream protection-check code in this diff. Please confirm the protection gate (entry.transaction.kind_changed_by_user? or equivalent) still gates auto_kind even when it's set from the provider. If a user manually re-categorised a transfer as "standard" and then a re-sync runs, we'd want that to survive.

One-sided movements and future pairing – The PR description acknowledges auto_match_transfers! still filters on amount/date/currency, so funds_movement entries tagged with a transfer_account_id could still be paired if both accounts are linked. The transfer_account_id stored in extra["up"] is a solid hook for a follow-up that builds the Transfer record deterministically rather than relying on the probabilistic matcher. Worth filing as a follow-up issue so it doesn't get lost.

Test: FakeResponse stub – The new test stubs Provider::Up.get with a lambda that ignores the query: keyword. If get_account_transactions ever passes query params (pagination, date filters), the stub silently swallows them. For the purpose of this test that's fine, but a note in the stub would help future readers understand the omission is intentional.

---

Generated by Claude Code

[threatsurfer]

Thanks @jjmata, really helpful review.

Protection check (point 1). Confirmed. import_transaction runs the gate up front: determine_skip_reason returns user_modified / excluded / import_locked, and the method does record_skip(entry, skip_reason); return entry before the kind block executes. So a manually re-categorised transfer (which sets user_modified) is skipped on the next sync and the manual kind survives; the provider kind: is only applied on the non-skipped path. Happy to add an explicit regression test for that interaction if useful.

Loan accounts (Codex's catch). Good one. Since UP_ACCOUNT_TYPE_MAP maps HOME_LOAN to a Loan account, a repayment carrying transferAccount would have been reclassified from loan_payment to funds_movement. Pushed a fix (a31160e): the provider kind: is now a fallback and the activity-label / account-type branches take precedence, so loan and credit-card repayments keep their budgeted classification. Added a regression test asserting a loan repayment stays loan_payment.

Deterministic pairing (point 2). Agree. The transfer_account_id stored in extra is the hook for building the Transfer straight from Up rather than leaning on the matcher; I'll open a follow-up issue so it does not get lost.

Stub note (point 3). Added a comment clarifying the query: omission is intentional.