fix(family): include HSA depository accounts in tax-advantaged exclusion

[galuis116]

Summary

Family#tax_advantaged_account_ids in app/models/family.rb:243-263 is the ID set the budget engine uses to exclude tax-advantaged account activity from income / expense / cashflow totals. PR #724 originated this method and explicitly listed HSA as an in-scope account type ("401k, IRA, HSA, Roth IRA, etc."), but the implementation only joined investments and cryptos. Depository::SUBTYPES["hsa"] already exists (app/models/depository.rb:9) and Plaid routes depository.hsa accounts to Depository (not Investment) via PlaidAccount::TypeMappable (app/models/plaid_account/type_mappable.rb:35), so a real-world Plaid-linked HSA cash account was silently absent from the filter and HSA contributions / withdrawals showed up in household expense totals — exactly the noise PR #724 was designed to suppress.

The symmetric gap on Account#tax_advantaged? (in the TaxTreatable concern) returned false for HSA depositories because Depository#respond_to?(:tax_treatment) was false.

Fixes #2003.

Fix

• app/models/depository.rb — add TAX_ADVANTAGED_SUBTYPES = %w[hsa].freeze constant + a tax_treatment instance method. Mirrors Investment#tax_treatment (SUBTYPES.dig(subtype, :tax_treatment) || :taxable) and the cryptos.tax_treatment enum. The TaxTreatable concern picks the new method up via its existing respond_to? check, so Account#tax_advantaged? flips to true for HSA depositories without touching the concern.
• app/models/family.rb — add a private tax_advantaged_depository_account_ids method that joins depositories and filters by Depository::TAX_ADVANTAGED_SUBTYPES, mirroring the existing investment_ids / crypto_ids extraction style. Append it to the union in tax_advantaged_account_ids. Memoization preserved.

Behavior change scope

HSA depositories now exit the budget engine via the same path as 401k / IRA / Roth IRA — IncomeStatement::Totals, FamilyStats, CategoryStats, and Transaction::Search consume the corrected ID set transparently.

Non-HSA depositories (Checking, Savings, CD, Money Market) now report tax_treatment: :taxable (previously nil). Account#taxable? continues to return true for them via the existing tax_treatment == :taxable clause in TaxTreatable — no expense-total change, no UI change.

Tests

• test/models/account_test.rb — rewrite the existing tax_treatment returns nil for non-investment accounts test (it was implicitly testing the bug) into two tests: one asserting :taxable for non-HSA depositories, and a new sibling asserting nil for accountables that genuinely lack tax_treatment (using a CreditCard). Add a new tax_advantaged? returns true for HSA depository accounts test.
• test/models/income_statement_test.rb — new test family.tax_advantaged_account_ids includes HSA depository accounts and excludes non-HSA depositories asserting both inclusion and exclusion paths against an HSA depository and a savings depository created on the fixture family.

Files changed

• app/models/depository.rb (+15)
• app/models/family.rb (+13 / -1)
• test/models/account_test.rb (+41 / -5)
• test/models/income_statement_test.rb (+31)

No schema migration, no controller change, no provider integration change, no view edit, no API contract change.

Related

• PR Exclude tax-advantaged account activity from budget & add provider data quality warnings #724 — origin of Family#tax_advantaged_account_ids. Body listed HSA in scope; the implementation handled Investment + Crypto branches but missed Depository HSA. This PR completes what Exclude tax-advantaged account activity from budget & add provider data quality warnings #724 announced.
• PR Add tax treatment classification for investment accounts with international support #693 — introduced the Investment::SUBTYPES.tax_treatment metadata that PR Exclude tax-advantaged account activity from budget & add provider data quality warnings #724 keys off. Same shape that Depository was missing.
• Open PR fix(reports): exclude investment_contribution from expense totals (#1750) #1999 — fix(reports): exclude investment_contribution from expense totals (#1750) — adjacent in spirit, different code path (modifies BUDGET_EXCLUDED_KINDS and investment_contribution Transaction kind handling, not tax_advantaged_account_ids). Test-file overlap on test/models/income_statement_test.rb only — different line regions, trivial rebase if needed.
• app/models/plaid_account/type_mappable.rb:35 / :68 — the upstream mapping that routes depository.hsa to Depository, making this bug fire for real Plaid users.

Summary by CodeRabbit

• New Features

  • HSA depository accounts are now treated as tax-advantaged across budget, cashflow, and income statement views, improving filtering and badge display.

• Tests

  • Added and updated tests to verify tax treatment for HSA and non‑HSA depository accounts and their effect on tax‑advantaged account listings.

[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: f2999415-a7a0-4aec-b876-32f973a5cdf3

📥 Commits

Reviewing files that changed from the base of the PR and between 8e5368d and 9b197d3.

📒 Files selected for processing (2)

• app/models/depository.rb
• test/models/account_test.rb

---

📝 Walkthrough

Walkthrough

This PR classifies HSA depositories as tax-advantaged by adding Depository::TAX_ADVANTAGED_SUBTYPES and a Depository#tax_treatment method, updates Family#tax_advantaged_account_ids to include depository IDs matching that list, and updates tests to cover HSA and non-HSA depository behavior.

Changes

HSA Depository Tax Treatment and Family Filtering

Layer / File(s)	Summary
Depository tax treatment model app/models/depository.rb	Adds TAX_ADVANTAGED_SUBTYPES = %w[hsa].freeze and tax_treatment which returns :tax_advantaged for subtypes in the list and nil otherwise.
Family tax-advantaged account filtering app/models/family.rb	tax_advantaged_account_ids now appends depository tax-advantaged IDs computed by new private helper tax_advantaged_depository_account_ids, joining depositories and filtering by Depository::TAX_ADVANTAGED_SUBTYPES.
Account tax treatment test updates test/models/account_test.rb	Adds/updates tests asserting non-HSA depositories return nil for tax_treatment/tax_treatment_label and remain taxable, and asserting HSA depositories return :tax_advantaged with tax_advantaged? true and taxable? false.
Family integration test for tax-advantaged depositories test/models/income_statement_test.rb	Adds a test that creates HSA and non-HSA depositories, clears memoized @tax_advantaged_account_ids, and asserts Family includes HSA depository account IDs and excludes non-HSA depositories.

Sequence Diagram

sequenceDiagram
  participant Family
  participant Accounts
  participant Depository
  Family->>Accounts: tax_advantaged_account_ids (joins depositories)
  Accounts->>Depository: where(subtype IN Depository::TAX_ADVANTAGED_SUBTYPES)
  Depository-->>Accounts: matching account ids
  Accounts-->>Family: return depository account ids

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• we-promise/sure#724: Original PR that introduced Family#tax_advantaged_account_ids; this extends its filtering to include HSA depositories.
• we-promise/sure#693: Introduced the tax_treatment framework on investment accounts; this PR applies the same pattern to Depository.

Suggested reviewers

• jjmata

Poem

🐰 A rabbit scurries through accounts with care,

Finds HSA depositories flagged fair and square.
Depository learned which ones to shield,
Family now knows which balances to yield.
Hops, munches, and celebrates the fix with flair 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 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 fix: adding HSA depository accounts to the tax-advantaged exclusion logic in Family.
Linked Issues check	✅ Passed	All objectives from issue #2003 are met: Depository.tax_treatment implemented, TAX_ADVANTAGED_SUBTYPES constant added, Family#tax_advantaged_account_ids extended with depository join, and tests added.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fix #2003: adding tax_treatment support to Depository, updating Family#tax_advantaged_account_ids, and adding relevant tests.

_{✏️ 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 and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8e5368da24

ℹ️ 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]

Avoid marking every depository account as taxable

For every non-HSA Depository (checking, savings, CD, money market), this now returns :taxable instead of nil. The account header renders the tax badge whenever account.tax_treatment.present? (app/views/accounts/show/_header.html.erb), so ordinary bank account pages will now show a “Taxable” pill even though tax treatment was previously only surfaced for account types that supported it. This affects the common non-HSA depository path; consider keeping non-HSA depositories nil (or gating the badge) while still treating hsa as tax-advantaged.

Useful? React with 👍 / 👎.

[jjmata]

Well-researched fix that closes a real gap — PR #724 listed HSA in scope but the implementation only covered investments and cryptos, and Plaid's depository.hsa routing made this silently wrong for real users.

A few observations:

Design is consistent with the existing pattern. Depository#tax_treatment mirrors Investment#tax_treatment and the cryptos.tax_treatment enum, and TaxTreatable's respond_to? check picks it up without touching the concern. The private tax_advantaged_depository_account_ids query mirrors the investment_ids/crypto_ids extraction style — easy to read and follow.

Memoization is preserved correctly. The new private method is called inside the @tax_advantaged_account_ids ||= begin...end block, so the extra query is incurred at most once per family object lifetime.

Tests cover both inclusion and exclusion paths, including the fixture checking account — the @family.instance_variable_set(:@tax_advantaged_account_ids, nil) cache-bust in the income statement test is the right way to handle this.

One small comment on comment verbosity: The private method has a 5-line explanatory comment. Per the project's convention (CLAUDE.md), comments should only appear when the WHY is non-obvious. The method name tax_advantaged_depository_account_ids plus the Depository::TAX_ADVANTAGED_SUBTYPES constant make the intent clear; the comment about "why extracted rather than inlined" is more of a PR-description thought than a code comment. Not a blocker.

No schema change, no API contract change, tests pass the inclusion/exclusion contract — looks good to me.

---

Generated by Claude Code

[luckyPipewrench]

The tax_advantaged_account_ids fix is correct and the tests are great. But there is a UI change.

Depository#tax_treatment now returns :taxable for non-HSA subtypes where it used to return nil. The header renders the tax badge on account.tax_treatment.present? (app/views/accounts/show/_header.html.erb:15), so every checking, savings, CD, and money-market account now shows a "Taxable" pill it never had before.

I traced the rest: that badge is the only unintended change. taxable? and tax_advantaged? have no callers in app/ or lib/, so the HSA exclusion itself is clean. Cleanest fix is to return nil for non-HSA depositories. That keeps hsa in the filter and drops the new pills on ordinary bank accounts.

Looks good otherwise.

[galuis116]

The tax_advantaged_account_ids fix is correct and the tests are great. But there is a UI change.

Depository#tax_treatment now returns :taxable for non-HSA subtypes where it used to return nil. The header renders the tax badge on account.tax_treatment.present? (app/views/accounts/show/_header.html.erb:15), so every checking, savings, CD, and money-market account now shows a "Taxable" pill it never had before.

I traced the rest: that badge is the only unintended change. taxable? and tax_advantaged? have no callers in app/ or lib/, so the HSA exclusion itself is clean. Cleanest fix is to return nil for non-HSA depositories. That keeps hsa in the filter and drops the new pills on ordinary bank accounts.

Looks good otherwise.

updated according to your feedback.