fix(sync): store EnableBanking credit card debt as balance instead of available credit#2459
fix(sync): store EnableBanking credit card debt as balance instead of available credit#2459mxafi wants to merge 3 commits into
Conversation
… available credit Previously, the EnableBanking processor forcibly overrode the primary account balance for credit cards to be the available credit (credit_limit - debt) rather than the actual outstanding debt. Since credit cards are modeled as Liability accounts, this caused the balance sheet (net worth) to treat available credit mathematically as a debt. This PR aligns the EnableBanking processor with the Plaid and SimpleFIN processors by storing the absolute debt as the account balance, while tracking the available credit via accountable metadata. Fixes we-promise#2458
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesCredit Card Balance Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/models/enable_banking_account/processor.rb (1)
44-47: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale comment contradicts the new behavior and should be updated.
The comment states that
available_credit"overrides the 'balance' variable" and instructs maintainers "Do not revert this behavior." However, the fix specifically removes that override—balanceis now set to the absolute debt, not available credit. Leaving this comment risks a future maintainer reintroducing the bug, thinking they are restoring intended behavior.Update or remove the comment to reflect that balance now represents the outstanding debt, with available credit tracked separately in accountable metadata.
📝 Suggested comment update
- # For liability accounts, ensure balance sign is correct. - # DELIBERATE UX DECISION: For CreditCards, we display the available credit (credit_limit - outstanding debt) - # rather than the raw outstanding debt. Do not revert this behavior, as future maintainers should understand - # users expect to see how much credit they have left rather than their debt balance. - # The 'available_credit' calculation overrides the 'balance' variable. + # For liability accounts, ensure balance sign is correct (positive = debt owed). + # CreditCard balance stores the absolute outstanding debt, matching Plaid/SimpleFIN behavior. + # available_credit is tracked separately in accountable metadata for display purposes, + # keeping it mathematically inert for net-worth calculations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/enable_banking_account/processor.rb` around lines 44 - 47, The comment block describing the CreditCards balance calculation behavior is outdated and contradicts the current implementation. The comment currently states that available_credit overrides the balance variable and instructs maintainers not to revert this behavior, but the fix has changed balance to represent the absolute outstanding debt instead. Update this comment to accurately reflect that balance now contains the outstanding debt amount and available credit is tracked separately through accountable metadata, ensuring future maintainers understand the current intended behavior and are less likely to inadvertently revert the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/models/enable_banking_account/processor.rb`:
- Around line 44-47: The comment block describing the CreditCards balance
calculation behavior is outdated and contradicts the current implementation. The
comment currently states that available_credit overrides the balance variable
and instructs maintainers not to revert this behavior, but the fix has changed
balance to represent the absolute outstanding debt instead. Update this comment
to accurately reflect that balance now contains the outstanding debt amount and
available credit is tracked separately through accountable metadata, ensuring
future maintainers understand the current intended behavior and are less likely
to inadvertently revert the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03ebc599-d20a-44df-a9e4-1046acd560fe
📒 Files selected for processing (2)
app/models/enable_banking_account/processor.rbtest/models/enable_banking_account/processor_test.rb
Updates the inline processor comments to reflect the new behavior introduced by the previous commit, clarifying that outstanding debt is stored sequentially as the primary balance rather than the UX available credit overriding it.
jjmata
left a comment
There was a problem hiding this comment.
The net-worth correctness argument is solid — storing outstanding debt makes the liability calculation accurate.
A couple of things worth flagging:
Breaking display change for existing users — Anyone with an EnableBanking CreditCard account currently sees "available credit" as their balance. After this PR merges, the same field shows "outstanding debt." That's arguably more correct financially, but it will surprise users. Worth calling out in release notes or a migration notice.
Removed "else" branch — The old fallback path (no credit_limit, no stored available_credit) left balance as the raw API value. The new code always balance.abs-es before the conditionals, so that case becomes abs(raw_balance). That's probably fine but the deleted comment "Fallback: no credit_limit from API — display raw outstanding balance" was documenting a deliberate choice. A brief comment on the new balance = balance.abs line saying why it's unconditional (even without a limit) would help the next reader.
available = credit_limit - balance — In the new code, balance has already been abs-ed, so this computes limit - abs_debt, which is the available amount. That's correct. Just making sure the reader notices the ordering dependency: balance = balance.abs must run before the subtraction. No bug, but the code is order-sensitive in a non-obvious way.
Generated by Claude Code
|
@jjmata Thanks for the review!
I will push a quick follow-up commit to maybe refactor the abs logic a bit and add inline comments clarifying:
I'll get that commit pushed up today or tomorrow! |
…EnableBanking processor Refactors the debt polarity assignments to clarify why liability balances are strictly parsed as absolute positive numbers. Replaces the implicit ordering dependency between the '.abs' conversion and the 'available_credit' math with an explicit 'outstanding_debt' variable to prevent regression by future maintainers.
Fixes #2458
Description
Previously, the EnableBanking processor forcibly overrode the primary account balance for credit cards to be the available credit (
credit_limit - debt) rather than the actual outstanding debt.Because credit cards are correctly classified as Liability accounts dynamically within the application, this caused the balance sheet to incorrectly treat the user's available credit mathematically as a debt in their net worth calculation.
This PR aligns the EnableBanking processor with the existing Plaid and SimpleFIN processors by:
Summary by CodeRabbit