QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-04-19 12:04:08 +00:00
Code Issues Packages Projects Releases Wiki Activity

Update PR #1214 review: approve, suggest .classification refactor as follow-up

Browse Source 
Replace hardcoded liability type lists with Accountable's .classification
method across all sync providers in a separate PR.

https://claude.ai/code/session_013wBVMnAdt2h4wHAvSiy3ih
This commit is contained in:
Claude
2026-03-17 17:27:31 +00:00
parent 7880b4f926
commit 7425606e68
1 changed files with 16 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
pr_review_1214.md
View File

@@ -4,18 +4,25 @@
Fixes #868 where Discover checking/savings accounts had inverted balances because the mapper incorrectly inferred them as liabilities. The fix prioritizes the user's manually selected account type over mapper inference.
### Assessment: Approve with minor suggestion
### Assessment: Approve
The core logic change is correct. When `account.accountable_type` is present, use the linked type's liability classification; otherwise fall back to mapper inference. This properly trusts user-selected account types.
### Issue: `OtherLiability` missing from liability checks
Line 55 and 66 only check `["CreditCard", "Loan"]` but `Accountable::TYPES` includes `OtherLiability`. Suggest:
```ruby
LIABILITY_TYPES = %w[CreditCard Loan OtherLiability].freeze
```
### Tests look good
The two new tests properly verify both directions: depository overrides mapper (positive balance preserved) and credit card overrides mapper (balance inverted). The old "mislinked" test is correctly replaced.
### Follow-up suggestion: Replace hardcoded liability type lists with `.classification`
All sync providers (SimpleFIN, Plaid, Lunchflow, Enable Banking, Mercury) hardcode `["CreditCard", "Loan"]` for liability detection, which misses `OtherLiability`. Rather than expanding the list, the existing `Accountable` concern already provides `classification`:
```ruby
# Current (fragile — must update when new liability types are added):
is_linked_liability = ["CreditCard", "Loan"].include?(account.accountable_type)
# Suggested (self-maintaining — uses the domain model):
is_linked_liability = account.accountable_type.present? &&
account.accountable_type.constantize.classification == "liability"
```
This is a cross-provider refactor and out of scope for this PR. Recommend a follow-up PR to address it across all 5 providers.