mirror of
https://github.com/we-promise/sure.git
synced 2026-09-05 14:51:15 +00:00
bf5ceff26926b63a07b0ae4ac1ecccdddbbf7c8b
1079
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Brandon
|
bf5ceff269 |
Fix flaky sign_out teardown in six test suites (#3208)
Six suites (passkey, MFA, SnapTrade, categorize, onboarding and the Active Storage authorization integration tests) share a sign_out helper that deletes the user's sessions through the controller, one HTTP request per session, iterating in unspecified order. The moment the loop deletes the session the test itself is signed in with, every later request in the loop is unauthenticated and silently deletes nothing, so whichever sessions happen to sort after it survive. The sessions fixture belongs to the same user these suites use, so a surviving fixture row then fails every assertion that expects the user to have no sessions. Row order usually favors the fixture, which is why the suites usually pass. Under parallel CI they fail a few times a week, always in this file family, always with the fixture session as the leftover. Forcing newest-first order reproduces it deterministically on current main: ten of the fifteen passkey tests fail. Teardown hygiene is not the behavior under test, so the helpers now destroy the sessions directly, which no order can break. All six suites run green three times in a row. |
||
|
Tim Katz
|
2fdb9ee175 |
Plaid: add accounts to an existing connection (#3199)
* feat: add accounts to existing Plaid items * fix: harden Plaid account addition * fix: guard Plaid follow-up sync retries * fix: use debug log for Plaid retry exhaustion |
||
|
Juan José Mata
|
2abce5f5b7 |
Verify PDF support with a synthetic health probe (#3191)
* Add synthetic PDF health checks * Require exact marker in PDF health probes * Report PDF health paths separately * Refactor application code * Remove unrelated schema dump changes * Simplify synthetic PDF validation --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
0xmarty_andMarty
|
79a175806e |
Fix SimpleFIN balances-only liability signs (#3187)
* Fix SimpleFIN balances-only liability signs * Fix SimpleFIN balances-only liability edge cases * Fix importer test helper indentation --------- Co-authored-by: Marty <marty0x@github.com> |
||
|
|
4930aacdb8 |
feat: create merchant inline from transaction detail (#3106)
* feat: create merchant inline from transaction detail Add a searchable "create or select" merchant combobox (DS::MerchantSelect, mirroring the existing DS::TagSelect pattern) so a new FamilyMerchant can be created directly from the transaction detail and new-transaction forms, instead of requiring a trip to Settings > Merchants first. FamilyMerchantsController#create now also responds to JSON so the combobox can create-and-select a merchant without a full page reload. * fix: address PR review feedback on merchant inline creation - Handle Turbo validation failures in FamilyMerchantsController#create (missing format.turbo_stream branch raised ActionController::UnknownFormat) - Guard connect() so disabled merchant selectors (no menu target) don't throw - Select the exact match (or block form submission) on Enter when the create row is hidden - Catch network/parse failures in createMerchant and show an error - Localize the fallback "could not create merchant" error message - Move the create button/error text outside the listbox and add aria-controls for accessibility - Align create-option avatar size with the selected-value display Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: keep created merchant option inside the listbox, move logo URL logic to component - Add a dedicated listbox target and insert newly created merchant options inside it (previously landed outside role="listbox" after the create button was moved out in the prior review-fix commit) - Move selected-merchant logo URL transformation out of the template into DS::MerchantSelect#selected_merchant_logo_url Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address maintainer review feedback on merchant inline creation - Drop the explicit format.turbo_stream branch in the merchant creation failure path: respond_to's turbo_stream matching forces the response Content-Type to text/vnd.turbo-stream.html before the block runs, so render :new, formats: [:html] only changed template lookup, not the Content-Type. Turbo's client then received a turbo-stream response with no <turbo-stream> tags and silently did nothing. Leaving turbo_stream undeclared lets Rails negotiate down to format.html, which renders :new with the correct text/html type. - Add truncate/shrink-0 classes to the merchant option partial's name and avatar spans so they match the selected-value display once updateSelectionDisplay clones them into the trigger button. - Add a regression test simulating a Turbo form submission (Accept: turbo-stream, text/html) against a duplicate merchant name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
74bb980271 |
feat(goals): a reserve measured in months of spending, not a fixed sum (#3180)
* feat(goals): a reserve measured in months of spending, not a fixed sum "Six months of expenses" is the way people actually describe an emergency fund, and it is a moving number: what covered six months last January does not cover six months today. A reserve pinned to a figure typed once drifts quietly out of date, and the drift always runs the wrong way — the bar reads full while the cover shrinks. `target_amount` stays the single source of truth, rewritten monthly by RefreshMaintainedGoalTargetsJob. That is the whole architectural decision here. An `effective_target_amount` would have been the obvious shape and the wrong one: `remaining_amount`, `progress_percent`, `Goal.summary_for`, the ring, the card and every future caller would each have had to learn which target to read. None of them change. The job refuses to write more often than it writes, on purpose: - a family with no spending history yet computes a floor of zero, which would both violate the `target_amount > 0` constraint and read to the user as "your reserve is complete". The previous target stands. - a figure identical to the current one is not rewritten, so a reserve does not collect a fresh updated_at every month for nothing. - a write that fails validation leaves the target alone and is recorded through DebugLogEntry, not just the application log: a reserve frozen at a stale floor is invisible to the user, who has no reason to suspect the number stopped moving. The job reads the family's spending, not a member's view. IncomeStatement falls back to Current.user when nobody says otherwise, which in a background job is nobody — so the scope is the whole family, and the number is the same whoever is looking. That is deliberate, and matches how the rollover chain had to be pinned. `target_months` is refused outside a months-mode reserve rather than tolerated: a number nothing reads would sit there looking meaningful, and the job would skip it for reasons no one could see. schema.rb is hand-edited again — verified against a real migration on a throwaway database, structures identical. The dumper on this Rails version also rewrites every check-constraint cast, so the new constraint is written in the file's existing style rather than the dumper's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): pin the reserve calculation to the family, and get it right on day one Review of the previous commit raised two things, and they are the same thing seen from either end. `IncomeStatement.new(family)` looked family-wide but was only so by accident. Its constructor falls back to `Current.user`, and eligible_accounts narrows to that user's accounts when one is present. The single caller was a background job, where nobody is current — so the figure was correct for the reason that it happened to be computed nowhere else. `target_amount` belongs to the whole family: derived from a viewer's slice of the accounts, it would have started moving with whoever last triggered it. This is the same fallback that made the budget rollover carry depend on its reader. The account scope is now passed explicitly, so the calculation is safe whatever calls it. That mattered immediately, because the second point required a new caller. A reserve created as "6 months of expenses" had no floor computed until the 1st of the following month: the user chose the mode, guessed an amount, and lived with a wrong target for up to a month. The feature's first impression was its least convincing moment. The floor is now computed on creation, and whenever the mode or the number of months changes — but never on an unrelated save, so the monthly job keeps owning the cadence and renaming a goal cannot silently move a financial figure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): keep a months-based floor derived, and in the right currency Review on #3180. The median comes back in FAMILY currency and `target_amount` is stored in the GOAL's, so a EUR reserve in a USD family read a 3,000 dollar floor as 3,000 euros — and rewrote it that way every month, silently. Converted now, and when there is no rate for the day the previous target stands: the same safe failure the method already took for a family with no spending history, because a stale floor beats a wrong one. The callback also skipped a target_amount edit, so the form could persist an arbitrary figure under a "six months of expenses" label until the next monthly refresh. It runs on that edit now and overwrites it — and when there is nothing to derive from, restores what the reserve already had rather than accepting the typed figure. The form marks the field read-only in that mode. The model does not depend on it, but a field that silently discards what you type is worse than one you cannot type into. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
03b783b139 |
feat(budgets): show what is actually free, beside the plan (#3179)
* feat(budgets): show what is actually free, beside the plan The budget has never consulted an account balance. `budgeted_spending` and `expected_income` are numbers the user typed, and every figure on the page derives from them — a forecast, checked against reality after the fact. It answers "what did I plan to spend" and cannot answer "what do I actually have". Three methods answer the second question: `available_cash`, `earmarked_for_goals`, and `free_cash`. They appear in their own panel, below the plan and outside it. That separation is the whole design, not a layout choice. Folding cash into the allocation arithmetic turns the budget into a different product — YNAB's, where you distribute money you hold rather than money you expect — and a page showing "expected income 3,000" beside "really free 1,600" leaves the reader unsure which number drives the split. `allocated_spending` and `available_to_allocate` keep their exact meaning; a test asserts none of them moves. **The subtraction has to be over the same accounts as the sum.** `Goal::FUNDABLE_ACCOUNT_TYPES` includes Investment, so a goal can be backed by a brokerage account that `available_cash` never counted. Subtracting that earmark would show a "really free" figure too low, or negative, with nothing on the page to explain it. `earmarked_for_goals` is therefore restricted to `cash_accounts`, and `Goal#backing_within` exists to ask that question. It reads through the shared pool rather than summing `allocated_amount`, because a whole-account link reserves no fixed slice: summed naively it counts as zero while actually claiming the remainder. Scoped like `#transactions` — a personal budget sees its owner's accounts, the household one what the viewer can see. A figure labelled "available" has to mean available to the person reading it. Behind the preview flag, because goals are: a panel that subtracts what they claim, and links to them, would otherwise point at a page the reader cannot open and explain a subtraction they cannot inspect. bin/rails test: 7083 runs, 28500 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(budgets): make the cash panel work in more than one currency Review on #3179. Three defects, all of them mine, all in the same seam. `ExchangeRate.find_rate` does not exist. Every multi-currency family opening the budget page hit a NoMethodError before the panel rendered. `find_or_fetch_rate` is the lookup the rest of the app uses. `earmarked_for_goals` summed each goal's backing in the goal's own currency and subtracted it from an `available_cash` that had been converted. A fully earmarked EUR 1,000 account in a USD budget read as 1,200 available, 1,000 earmarked and 200 free — when none of it is free. The French keys landed under `budget_categories` instead of `budgets`, so the partial's `t(".heading")` found nothing and French readers got the English fallback. A missing rate leaves the amount as it stands rather than raising: a panel wrong by the spread beats the whole budget page failing to render. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
f0333c026e |
feat(goals): surface money that left a goal's accounts unexplained (#3177)
* feat(goals): surface money that left a goal's accounts unexplained Goals never read transactions. `current_balance` is a stock summed from account balances, so an outflow reaches a goal only as a smaller number, with nothing saying which goal it belonged to. `consume!` closes that gap, but only for a user who thinks to declare it — and the whole difficulty is that they have no reason to think of it. `Goal::WithdrawalDetector` surfaces the outflows nothing has claimed, and the goal page offers them: *if any of this was spent on Trip, say so*. One click records it with the transaction as evidence. This is the pull half of what `GoalPledge` does for money coming in. A pledge asks first and matches later; here there is nothing to promise, so the outflow is surfaced after the fact and attributed — or not. **Anchored on the transaction, not declared.** `consume!` now takes one and stamps `extra["goal"]["consumed_goal_id"]`, the same namespace the pledges write into. That is what makes attribution idempotent: replaying it cannot credit a goal twice for one spend, and the stamp happens inside the consumption's own transaction so a refusal rolls the whole thing back. **Sign matters more than it reads.** In Sure an inflow carries a NEGATIVE amount, so the detector selects the positive side. Reading it the other way round would have offered to attribute the user's deposits as spending, and the mistake would look right in a diff. A test pins it. **A reserve is excluded.** It is drawn down and refilled, not spent, and asking someone to attribute a withdrawal from one invites them to erase the very shortfall it exists to report. Known limitation, unchanged by this: `GoalPledge::Reconciler` only runs on provider imports, never on a hand-entered transaction. This detector reads entries directly and so has no such gap, but the two halves are not symmetric and that is worth knowing. bin/rails test: 7100 runs, 28540 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): only offer an outflow the goal could still have spent Review on #3177. The panel offered outflows for completed and archived goals. Those have handed their accounts back, so a later transaction on one is not evidence about this goal — attributing it writes spending into a history that is already closed. The detector now returns nothing for a released goal; `consume!` refuses these too, but the panel should not ask in the first place. Provisional transactions were offered as well. A pending charge can be reversed or replaced by its posted form, leaving the goal consumed for a transaction that no longer exists while the posted twin arrives unstamped and gets offered again. Filtered through the pending-provider SQL the rest of the app already uses. `thaw_completed_amount!` wiped `consumed_amount` unconditionally, so a goal that recorded a spend and was then archived straight from active lost that history on unarchive — and dropped its progress with it. Restarting is what clears the figure, and a direct archive never closed a lifecycle to restart from. Cleared now only when a frozen figure exists. The attribution button was a hand-rolled `button_to` with raw `btn` classes; it is `DS::Button` now, the same primitive the consumption dialog uses, so the two ways of recording a spend do not read as two features. Carried down from #3176 by rebase: the goal-level lock, the `:not_active` guard, and the success notice, which was blank on this path because the form posts only `transaction_id`. The resolved amount is formatted through `Money` and the account behind an attributed outflow now resolves through `accessible_accounts` like the named one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): keep a private backing account out of the outflow panel The same leak #3176 closed on the dialog, through a third door. A goal can be backed by an account private to another family member, and the panel listed its outflows — naming the account, what was spent on it and roughly its size to someone with no access to it. `WithdrawalDetector` takes `accounts:` now, and the controller passes the links narrowed to what the viewer may see. It defaults to every linked account for callers with no viewer to speak for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): use the shared separator key in the outflow panel DS Drift Patrol on #3177. The `·` between an outflow's date and its account was a bare literal. `shared.dot_separator` already exists and is already used three times in the goals views, so this was drift rather than a missing mechanism. Wrapped in `aria-hidden` like the existing uses: the separator is decorative, and a screen reader was reading it out between the two values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
b6029c1e28 |
feat(goals): show what each account still has room to earmark (#3166)
* feat(goals): show what each account still has room to earmark `Account#free_to_earmark` has existed, unused, since earmarks shipped — its own comment said the UI was a follow-up. This is that follow-up, and the wording is the substance of it. It does not say "over-allocated". `free_to_earmark` is negative for as long as the saving is unfinished, which is the normal condition of anyone with goals in progress: a 6,000 account backing two goals of 5,000 gives −4,000 and is a perfectly correct setup. A warning phrased as a fault would fire permanently and teach people to ignore it. The message states the consequence instead — the goals come to X for a balance of Y, so they progress pro rata — and is never styled as an error. The trap is the goal being edited. `goal_earmarked_total` counts every goal including that one, so reopening a goal that earmarks 5,000 on a 6,000 account shows 1,000 of headroom, and re-entering the same 5,000 trips a message about a setup the user has not touched. `earmarked_by_other_goals` excludes it, and only when it is persisted — a goal being created has nothing to exclude. The pool is read once per render and passed down, never per account: the form lists every fundable account the user can see. A test counts the query and fails at two. The Stimulus controller is its own, with 3 targets. goal_form_controller is at 10 against the 7 the project guidelines suggest, needs none of this state, and is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): read the typed amount strictly, and format it in the app's locale Addresses review feedback on #3166. `Number.parseFloat` accepts prefixes, so "500abc" became 500, and the bare comma-to-dot swap turned a thousands-separated "1,500" into 1.5. Either way the preview described an amount the user had not typed — and the second case is a habit from another locale, not a typo, so it would have gone unnoticed. The value now has to match a complete number before anything is computed. `Intl.NumberFormat(undefined, ...)` let the BROWSER pick the locale, so a French user on an English-locale browser read separators and symbol placement matching nothing else on the page. The amounts cannot be formatted server-side — they change with every keystroke — so the server passes `I18n.locale` and the client applies it. That puts the decision where the rest of the app's formatting already lives. bin/rails test: 6954 runs, 0 failures. RuboCop, erb_lint and biome clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): let the assistant create a second goal on a claimed account Review on #3166. The function always built whole-account links and had no way to express an earmark, so once exclusivity landed, asking for a second goal on an account another goal already claimed came back as a bare `validation_failed` — while the account list still advertised the account as available. A common request became an unexplained refusal. Three changes, and the list is the important one: it now says what is left on each account and which are claimed in full, because the assistant reasons from that list and had no way to know otherwise. `earmarks` is an optional map of account name to amount, so the assistant can reserve a slice rather than the whole balance. Accounts left out keep the previous behaviour and take whatever is spare. The refusal is named before the save — `account_claimed_in_full`, with the account names — so the assistant gets a reason it can act on and ask about, rather than a validation message it can only relay. Checked after the currency check, which is the more fundamental of the two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(goals): move the spend tests back out of the private section The merge of `main` into this branch landed #3176's tests between `count_pool_queries` and the helpers below it, inside the `private` section and at the wrong indentation. `ci / lint` has been failing on `Layout/IndentationConsistency` since. They still ran — `test` is a class method, so `private` does not hide them — which is why the unit job stayed green while lint went red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
buzzromainandClaude Opus 5
|
d7bf401cc7 |
fix(onchain-wallets): call transfers transfers, and drop the address swap (#3153)
* fix(onchain-wallets): call transfers transfers, and drop the address swap Three things reported from real use. **A transfer was announced as a purchase.** A movement was imported with `activity_label: "Buy"/"Sell"` and named "Buy 1.5 FAKE" — but coins arriving at an address were not bought there, and nothing here knows whether they were ever bought at all. The trade shape stays, because it is what carries quantity and cost basis in this ledger, but the label is now "Transfer" and the name is the one the movement already had while it was unpriced. The old wording also made the same event rename itself the day a price turned up for it. Worth pairing with the change to trades/_header.html.erb, which until now read the amount's sign and would still say "Buy" whatever the label. **Changing a tracked address is gone.** It repointed the rows at a new address while keeping their accounts, holdings and history — so trades reconstructed from address A stayed under an account presented as address B. Its own help text said so out loud: "The accounts, holdings and history stay as they are." Removing the address and adding the new one is not just simpler, it is the only one of the two that is honest, because it takes the old history with the old address. **The buttons follow the app's conventions now.** Actions that repeat per row belong in a menu here — accounts/_account.html.erb renders its own that way — not in a row of labelled buttons, which is what a provider panel does when it has a single connection to act on. So the per-address actions are a menu, the per-asset disconnect is icon-only, and the accounts-page card gains the actions menu every other provider card already had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(onchain-wallets): give a tracked asset its icon on the accounts page Account#logo_url asks its provider adapter for one, and ours answered nothing: there is no institution behind a self-custody wallet, and nothing attaches a file, so every tracked asset showed a blank where every other account shows an icon. Fixing Security#crypto_base_asset covered the holdings list, which reads the security directly — this is the other path, and it went through the adapter. Built from the symbol rather than looked up. The accounts page renders one of these per account, so resolving a Security each would be a query per row, and the symbol is all Brandfetch's crypto endpoint needs. It answers nil without a client id, which is the same nothing the page shows today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(onchain-wallets): follow the moved banner and the menu in the browser The system suite still expected the settings panel to carry the read-only reassurance, and to find "Review tokens" as a visible button. Both moved in the previous commit: the banner into the linking modal, where the question it answers is actually asked, and the per-address actions into a menu, as repeated row actions are rendered everywhere else in this app. Caught by CI rather than by me — the per-branch checks I ran covered `bin/rails test` and not `test:system`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(onchain-wallets): pin the reassurance to the frame it moved into The assertion proved the banner was somewhere on the page, which is exactly what the change does not claim: the point is where it lives. Now it asserts the text is absent from the settings panel and present inside the modal, so the test fails if the banner drifts back or never arrives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(onchain-wallets): rename movements imported before transfers had a name Review on #3153. Wallets synced before this change keep their `Buy`/`Sell` labels and their "Buy 1.5 shares of CRYPTO:BTC" wording, and nothing was rewriting them: `perform_sync` returns early when no address changed on chain, and the repair pass only ever looked at display-only `Transaction` rows. A cold address would have shown the old wording indefinitely — which for a wallet nobody touches is most of them. The repair now relabels this processor's own trades too, scoped to its `external_id` prefix and to `source: SOURCE` so a trade the user entered by hand is never renamed. It runs from `perform_post_sync`, which is the pass that already runs for every linked asset rather than only the changed ones. Idempotent, so a nightly sync does not rewrite the same rows forever. Separately: `Security.brandfetch_crypto_url` interpolated the symbol straight into a URL path, and `Onchain::AssetSymbol.canonical` only upcases and trims. An on-chain token can be called whatever its deployer chose, so a slash pointed the path elsewhere on the CDN and a hash pushed the client id into a fragment Brandfetch never sees. Guarded in the helper rather than at the call site — six callers reach it from four providers. Also from review: the icon test saves and restores `Setting.brand_fetch_client_id` instead of hard-coding nil in its `ensure`, which was erasing whatever the suite had configured. The "one query" claim in a repair test's name was never asserted, and this change adds a second query. Renamed to what it actually checks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * test(onchain): drop the last change_address test with its feature #3182 added a `change_address` test while this branch was removing the feature it exercises. The rebase kept both, leaving a test calling a route this branch deletes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
613964529a |
feat(goals): let a goal be spent without looking like it fell behind (#3176)
* feat(goals): let a goal be spent without looking like it fell behind Coming home from the holiday a goal paid for dropped it from 100% to 20%. The money went where it was meant to go, and the app read that as failure. The only way back was to edit the target — falsifying what the user had actually set out to save. `consumed_amount` records what was spent ON the thing the goal was for, and progress reads `(backing + consumed) / target`. Spending the money is no longer indistinguishable from losing it. **The part that is easy to miss.** `consume!` also shrinks the earmark on the account by the same amount. Without that, money the user has already spent stays reserved and keeps its share away from every sibling goal — the exact double-counting the exclusivity rules exist to prevent, arriving through the back door. A test pins it through the pro-rata haircut, where the effect is visible: a sibling's backing grows as the spent share is released. **Kept separate from `completed_amount`.** That one freezes the BACKING at closure; folding consumption into it would count the same money twice on a goal partly spent and then closed. A test asserts each side is counted once. **A reserve refuses consumption outright.** It is drawn down and refilled, not spent, and recording a withdrawal as consumption would erase the shortfall the reserve exists to report. `account:` may be omitted only when the goal has one link — with several, guessing would silently pick a side. The controller refuses an account id that resolves to nothing rather than falling back to nil, which on a single-link goal would have recorded the spend against an account the user never named. The write is its own action rather than a verb branch inside `consume`: HEAD routes like GET but `request.get?` is false for it, so a branch would send a HEAD request down the write path. Brakeman caught that. bin/rails test: 7088 runs, 28510 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): make a spend either happen entirely or not at all Review on #3176 found the recorded spend and the released earmark could drift apart, and that nothing stopped the two figures being edited out of agreement afterwards. The lock was on the link, not the goal. `consumed_amount` lives on the goal, so two concurrent requests locking only their own links both read the same old value, both passed the target check, and both added to it. The whole check-and-write now runs under `with_lock` on the goal. Consuming more than the chosen link held was silently clamped: the link released what it had while `consumed_amount` took the full figure, so money counted as spent stayed reserved against every sibling goal. It is refused now — `:exceeds_earmark` — rather than half-applied. A dialog left open in another tab could still post to a goal that had since been completed or archived; `:not_active` closes that. Two validations stop the pair being separated after the fact: a goal that has recorded a spend cannot become a reserve (reserves refuse consumption, so the figure would count toward progress on an object whose model treats spending as a shortfall), and the target cannot be lowered below what was already spent. Consuming cleared the columns and left the memos standing, so an instance that had already read its backing kept reporting the pre-spend figure. Progress holding steady is the feature — the earmark shrinks by what consumption grows by — which is exactly what hid the stale backing. Also from review: `accessible_accounts` rather than the whole family for the account picker, `DS::Select` rather than a bare `select_tag`, the flash amount through `Money#format`, and the French label for the menu entry, which I had left untranslated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * refactor(goals): reuse the cache reset the class already owns Review pointed out `reset_state_dependent_caches!` exists for exactly this, and that hand-rolling a second ivar list was the wrong shape. It was also wrong in substance: mine omitted `@pooled_allocations`, and consuming shrinks a link's allocation, which is precisely what the pool is computed from. One list, kept in one place, stays right when a memo is added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * docs(goals): put four comments back on the methods they describe Rebases had stranded them: a paragraph about clearing memos on an AASM transition, two about what reopening does to a frozen figure, and one about `reload` leaving memos standing had all piled up in front of `consumption_link_for`, which does none of those things. The last is dropped rather than moved — its explanation now sits at the call site in `consume!`, where the reset actually happens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): keep a private backing account out of the consume dialog A goal can be backed by an account the viewer is not allowed to see, and the first pass only guarded the named-account path. Two ways round it remained. The dialog listed every link, so it named private accounts outright. And with `account_id` left blank the model picked the sole link on its own, without anyone having checked the viewer could reach it — so a direct POST reduced a private account's earmark, the figures moving afterwards saying roughly how much was in it. The controller now derives the eligible links from `Current.user.accessible_accounts`, the dialog renders those, and a blank id resolves only to a sole *eligible* account. With none the request is refused; with several it stays nil and the model asks, as before. Naming the account explicitly matters even when the goal has several links: the one the viewer can reach is not necessarily the one the model would have picked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
90203139bf |
feat(goals): tell the user when a reserve has dropped below its level (#3175)
* feat(goals): reserves you maintain, not goals you finish An emergency fund is not a goal you reach and close — it is a level you hold, and every withdrawal is a shortfall to make good. Sure treated it like anything else: at 100% it offered to close it, which would release the very money being set aside; a withdrawal dropped the bar with no sign that anything was owed. `kind` (added by the lifecycle lot without behavior) now means something. A maintained goal is `funded` or `depleted`, never `reached` — sitting at its floor is a steady state, not an achievement to file away. `complete` is refused by an AASM guard rather than merely hidden, so no path can release a reserve's earmark. Two ordering traps, both of which would have made a drained reserve invisible: `ACTIVE_DISPLAY_STATUS_RANK` falls back to 4 for any status it does not know, so an unranked `:depleted` would sort a drained emergency fund below everything else — the exact opposite of what it means. It ranks alongside `:behind` now, and `:funded` sorts near the end with the goals that need nothing. `behind_pace?` excludes reserves. `monthly_target_amount` and `pace` both derive from `target_date`, which a reserve does not have, so "save X/month to catch up" would be advice about a deadline that does not exist. The form leads with the choice, since it changes what the rest of it means, and hides the target date for a reserve rather than disabling it — a hidden field cannot submit a stale value that would then drive a pace. The card states the shortfall, which is exactly `remaining_amount`. The panel that offers a one-off its closing action tells a reserve it is intact and offers nothing, because there is nothing to do. Scope: fixed targets only. Targets expressed in months of expenses, the monthly refresh job, and the depletion insight are the next two PRs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): let a reserve behave like one everywhere it is shown Addresses review feedback on #3167. The kind selector never hid the target date. `data-controller="goal-kind"` sat on the selector div while its `dateField` target is a sibling, so `dateFieldTargets` came back empty and picking "Reserve to maintain" left the deadline on screen and submittable. The controller moves to the form wrapper, which encloses both. Hiding a field is not enforcement, so the model now clears `target_date` for a maintained goal. Normalising rather than rejecting: the field is hidden, and an error about something the user cannot see is not actionable. A date could only arrive through a conversion or a crafted request, and either way a stored deadline would drive a pace the reserve does not have. A completed goal could be switched to `maintained` from the edit form. It then sat in a released state — one that has handed its earmark back — while the show page promised its money stays reserved, and `complete` for reserves is refused precisely to prevent that state. `kind` is now locked while released: reopen first. Reserves counted against the "goals on track" tile. Their statuses are `funded`/`depleted`, which match none of the exclusions in `tracked_total`, so they could never reach the numerator and a family with one reserve read "0 of 1 on track" for a goal working exactly as intended. Two more places still spoke of pace to something that has none. `pace_line` is suppressed for reserves on the card, and a depleted reserve gets its own panel before the projection card — the projection's summary, catch-up line and colour are all built from a deadline. What a drained reserve needs is the number the projection cannot show: how much is missing from the floor. The French celebration copy read "Votre réserve est à son niveau", which never says which level. Each guard was confirmed load-bearing by removing it and watching its test fail. bin/rails test: 6962 runs, 28003 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Left open deliberately: extracting the show page's lifecycle panel into a ViewComponent. The guideline behind it is right, but the refactor is wider than this round of fixes and belongs on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): finish teaching the status consumers about reserves Second round of review feedback on #3167: two consumers still had no branch for the reserve statuses. `ProgressRingComponent#percent_text_class` styled only `:reached` as success, so a funded reserve — a floor the user is holding exactly as intended — fell back to the neutral colour and read as unfinished. `status_callout_context` had no `:depleted` branch, so a drained reserve showed no callout at all: the one status that most deserves a line of explanation was the only one saying nothing. It now names the shortfall. `:funded` deliberately keeps no callout — a reserve at its level has nothing to report, and the celebration panel already says so. A test pins that, so the silence reads as a decision rather than another missing branch. bin/rails test: 6964 runs, 28008 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): lock the kind on the state the goal is actually in Addresses review feedback on #3167, on the guard that landed in c1c7f4f7. `kind_locked_while_released` read the in-memory `state`, so a single write setting `state: "active"` alongside the new kind saw the goal as already reopened and waved it through. The end state looks legitimate — active and maintained — which is why the hole is easy to miss. It is not: the direct write skipped the `reopen` transition, and with it `thaw_completed_amount!`. `completed_amount` survived, so `current_balance` returned that frozen snapshot forever on a live reserve. Reopening has to be its own gesture, because it is the gesture that thaws. Now reads `state_in_database`, with a regression test on the combined write asserting both that it is refused and that the frozen amount is untouched. Confirmed load-bearing by reading the attribute again and watching it fail. bin/rails test: 6969 runs, 28019 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): send an empty reserve to the shortfall panel, not the empty state Addresses the last review thread on #3167. The `maintained?` branch sat after the zero-balance/zero-pace one, so a brand-new reserve matched the generic "make your first transfer" card. I had put it there on purpose, thinking a reserve with nothing in it wanted the first-transfer nudge. The review is right that it does not: it is still a reserve short of its floor, and the shortfall panel says so with the saved, target and missing amounts, where the generic card says none of them. Ordering it after also meant evaluating `pace` on a goal that has no pace to evaluate. bin/rails test: 6965 runs, 0 failures. RuboCop and erb_lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): stop a paused goal outranking a reserve that is whole Review on #3175. `:funded` and paused both ranked 3 in `active_display_sort`, so the tie broke on name and a paused goal called "Alpha" sat above a reserve called "Zeta" that was fully funded — the list saying the paused one wanted attention more. Paused now ranks behind every status, which is what the comment above the table already claimed. The seven panels on the goal page were hand-rolled repetitions of `DS::Card`'s exact shell, two of them adjacent and identical. They render through the primitive now, so their surface styling cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * refactor(goals): move the lifecycle panel decision out of the template Review on #3167 and #3180. Which panel a goal gets is a lifecycle question with five answers, and the template worked it out inline from `completed?`, `maintained?`, `one_off?`, `status` and `may_complete?` — five predicates deep in ERB where the ordering between them was load-bearing and nothing said so. `Goals::LifecyclePanelComponent` answers it in Ruby and the template renders the answer. The markup moves across unchanged, keys made absolute because a relative `t(".x")` in a component resolves against the component's own path rather than the page these strings belong to. The order is now stated once, where it can be read and tested: `:reserve_shortfall` before `:empty`, because a brand-new reserve sits at zero balance and zero pace and the generic "make your first transfer" card would otherwise swallow it. Closing from the panel now confirms, as the header menu already did. Completing releases the goal's earmarked money, and the panel offered that in one click. Both go through `goal_complete_confirm` rather than building the wording twice — two copies drifting apart is how one ends up describing the wrong consequence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): refresh the pace suggestion when the deadline is cleared Assigning `input.value = ""` fires no event, so `goal-form#suggestedChanged` never ran: selecting "Reserve to maintain" cleared the date but left the monthly pace suggestion on screen, derived from a deadline the goal no longer has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): let a depleted reserve look as urgent as it is Review on #3179 and #3180. `Goal#needs_attention?` names the pair of statuses that mean "this one wants looking at" — a goal off its pace and a reserve below its floor. Three places were spelling that out and the Plan hub's progress bar had fallen behind, so a depleted reserve got a neutral bar an inch from its own amber status pill: the same goal reported as needing attention and not. `projection_summary` told a funded reserve it had "hit the target, no projection needed". A reserve holds a level; there is no finish line to project toward and no target to have hit. It does not reach that panel today — the shortfall and celebration panels catch it first — but the method reads as the single source of truth for that subtitle and should not hand a caller a one-off's wording. The legend swatches are bordered spans now rather than inline SVG, and the label takes `text-xs` instead of an arbitrary 11px. The projection swatch keeps the chart's own colour variables in an inline style rather than `border-success` / `border-warning`: the chart hard-codes green-600 and yellow-600, and a legend whose colour does not match the line it describes is worse than the markup it would save. The Plan-card test counts warning bars rather than matching one. A fixture goal is already off its pace, so the markup is on the page either way and a presence check passed without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * feat(goals): tell the user when a reserve has dropped below its level A maintained reserve exists to hold a floor. Nothing said when it stopped holding it: the goal page shows the shortfall, but only to someone who thought to look, and a reserve is precisely the thing you stop looking at once it is full. That silence is the gap this closes. High priority, unlike most of this feed. `IdleCashGenerator` is a nudge about money doing nothing; this is the opposite — a floor the user deliberately set is no longer there. It is also the one signal a reserve can produce that a one-off goal cannot, which is what makes it worth a generator of its own. Three deliberate limits: - **Active reserves only.** A paused one is shelved on purpose, and `behind_pace?` already excludes paused goals for the same reason. Nagging about a goal someone put down is noise. - **The dedup key rotates monthly.** A reserve can sit short for weeks while it is rebuilt, and re-raising the same shortfall every night trains people to dismiss the feed. - **Two at a time, worst shortfall first.** A family running four drained reserves has one problem, not four. Loaded through `Goal.prepared_for` so the family-wide pooled allocations are read once: asking each reserve for its status reaches `current_balance`, and without that injection every one of them would re-read the whole pool. bin/rails test: 7079 runs, 28491 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(insights): stop an insight naming something that has been renamed Review on #3175: renaming a depleted reserve leaves the stored title naming the old one for the rest of the month, because the name is not part of the metadata that drives a refresh. Rather than making the name material, the same-signal branch now refreshes the title alongside the facts. The title is built from I18n and the generator's own data — the model writes the body, not this — so keeping it current costs nothing, and a rename is not a reason to resurface an insight the user has already read. The body still says the old name until the numbers move. Forcing an LLM rewrite on every rename is the wrong trade, and making the name material would do that *and* re-nag the user. This is generic to every generator whose title embeds a name, not only the reserve one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(insights): refresh the title on reactivation too A gap in the previous commit: the expired-and-returned branch resurfaces an insight without touching its title, so a subject renamed while the insight was expired came back naming the old one. Same reasoning as the same-signal branch — the title is I18n plus the generator's own data, not the model's prose, so keeping it current costs nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
2c0d262cf6 |
fix(onchain): a deleted account leaves no wallet row behind (#3182)
* fix(onchain): a deleted account leaves no wallet row behind Deleting a Sure account left its on-chain tracking row in place. The row had a callback to follow its account link into the grave, but the guard that stopped it destroying itself could not tell apart the two ways that link dies — by the row, or by the account — and so caught both. What survived was worse than untidy. The row synced nothing and showed nowhere, yet it still answered "yes" to "is this address already tracked?" and still held the asset's slot in the partial unique index. The address became unusable: adding it again was refused as a duplicate of something the user could not see, and moving another address onto it raised a database error. So three places learn the difference between a row that tracks an account and one that merely exists: the guard now skips only its own row's destruction, the family's linked check asks for a live link, and the linker and address change let a dead row make way instead of colliding with it. No migration: a row already orphaned on a running instance is absorbed the next time that address is tracked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013nN1GZi7Dv3d7yZUozw3Yo * fix(onchain): stop an orphan row passing itself off as a tracked asset Review on #3182. Both findings are the same root: a row left behind by a deleted account has no Account and no AccountProvider, but nothing downstream was asking. `revise` counted any matching row as tracked, orphans included, so ticking an asset whose account had been deleted did nothing at all — no Account created, the row still orphaned, and the screen still reporting the asset as tracked. It now looks only at rows that are actually linked, and at the ones that survived the removal pass. `link` cleared only the rows matching the assets picked in that submit, so relinking a subset left the rest as orphans. They still show up in `grouped_accounts` and token review as tracked, and — before the fix above — they also stopped `revise` from ever rebuilding them. Reclaiming an address now clears every row it left behind. A live row is untouched, with a test that fails if the sweep widens. One existing test had to change its premise rather than its assertion. It built two unlinked rows and asserted the surviving row object was the same one; unlinked rows are orphans, so revising now rebuilds them and the survivor is a new row. Linking them first is what a tracked asset actually is, and the assertion then measures the removal it was written for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
6d7ca3584c |
feat(goals): reserves you maintain, not goals you finish (#3167)
* feat(goals): reserves you maintain, not goals you finish An emergency fund is not a goal you reach and close — it is a level you hold, and every withdrawal is a shortfall to make good. Sure treated it like anything else: at 100% it offered to close it, which would release the very money being set aside; a withdrawal dropped the bar with no sign that anything was owed. `kind` (added by the lifecycle lot without behavior) now means something. A maintained goal is `funded` or `depleted`, never `reached` — sitting at its floor is a steady state, not an achievement to file away. `complete` is refused by an AASM guard rather than merely hidden, so no path can release a reserve's earmark. Two ordering traps, both of which would have made a drained reserve invisible: `ACTIVE_DISPLAY_STATUS_RANK` falls back to 4 for any status it does not know, so an unranked `:depleted` would sort a drained emergency fund below everything else — the exact opposite of what it means. It ranks alongside `:behind` now, and `:funded` sorts near the end with the goals that need nothing. `behind_pace?` excludes reserves. `monthly_target_amount` and `pace` both derive from `target_date`, which a reserve does not have, so "save X/month to catch up" would be advice about a deadline that does not exist. The form leads with the choice, since it changes what the rest of it means, and hides the target date for a reserve rather than disabling it — a hidden field cannot submit a stale value that would then drive a pace. The card states the shortfall, which is exactly `remaining_amount`. The panel that offers a one-off its closing action tells a reserve it is intact and offers nothing, because there is nothing to do. Scope: fixed targets only. Targets expressed in months of expenses, the monthly refresh job, and the depletion insight are the next two PRs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 * fix(goals): let a reserve behave like one everywhere it is shown Addresses review feedback on #3167. The kind selector never hid the target date. `data-controller="goal-kind"` sat on the selector div while its `dateField` target is a sibling, so `dateFieldTargets` came back empty and picking "Reserve to maintain" left the deadline on screen and submittable. The controller moves to the form wrapper, which encloses both. Hiding a field is not enforcement, so the model now clears `target_date` for a maintained goal. Normalising rather than rejecting: the field is hidden, and an error about something the user cannot see is not actionable. A date could only arrive through a conversion or a crafted request, and either way a stored deadline would drive a pace the reserve does not have. A completed goal could be switched to `maintained` from the edit form. It then sat in a released state — one that has handed its earmark back — while the show page promised its money stays reserved, and `complete` for reserves is refused precisely to prevent that state. `kind` is now locked while released: reopen first. Reserves counted against the "goals on track" tile. Their statuses are `funded`/`depleted`, which match none of the exclusions in `tracked_total`, so they could never reach the numerator and a family with one reserve read "0 of 1 on track" for a goal working exactly as intended. Two more places still spoke of pace to something that has none. `pace_line` is suppressed for reserves on the card, and a depleted reserve gets its own panel before the projection card — the projection's summary, catch-up line and colour are all built from a deadline. What a drained reserve needs is the number the projection cannot show: how much is missing from the floor. The French celebration copy read "Votre réserve est à son niveau", which never says which level. Each guard was confirmed load-bearing by removing it and watching its test fail. bin/rails test: 6962 runs, 28003 assertions, 0 failures. RuboCop, erb_lint and Brakeman clean. Left open deliberately: extracting the show page's lifecycle panel into a ViewComponent. The guideline behind it is right, but the refactor is wider than this round of fixes and belongs on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): finish teaching the status consumers about reserves Second round of review feedback on #3167: two consumers still had no branch for the reserve statuses. `ProgressRingComponent#percent_text_class` styled only `:reached` as success, so a funded reserve — a floor the user is holding exactly as intended — fell back to the neutral colour and read as unfinished. `status_callout_context` had no `:depleted` branch, so a drained reserve showed no callout at all: the one status that most deserves a line of explanation was the only one saying nothing. It now names the shortfall. `:funded` deliberately keeps no callout — a reserve at its level has nothing to report, and the celebration panel already says so. A test pins that, so the silence reads as a decision rather than another missing branch. bin/rails test: 6964 runs, 28008 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): lock the kind on the state the goal is actually in Addresses review feedback on #3167, on the guard that landed in c1c7f4f7. `kind_locked_while_released` read the in-memory `state`, so a single write setting `state: "active"` alongside the new kind saw the goal as already reopened and waved it through. The end state looks legitimate — active and maintained — which is why the hole is easy to miss. It is not: the direct write skipped the `reopen` transition, and with it `thaw_completed_amount!`. `completed_amount` survived, so `current_balance` returned that frozen snapshot forever on a live reserve. Reopening has to be its own gesture, because it is the gesture that thaws. Now reads `state_in_database`, with a regression test on the combined write asserting both that it is refused and that the frozen amount is untouched. Confirmed load-bearing by reading the attribute again and watching it fail. bin/rails test: 6969 runs, 28019 assertions, 0 failures. RuboCop and Brakeman clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): send an empty reserve to the shortfall panel, not the empty state Addresses the last review thread on #3167. The `maintained?` branch sat after the zero-balance/zero-pace one, so a brand-new reserve matched the generic "make your first transfer" card. I had put it there on purpose, thinking a reserve with nothing in it wanted the first-transfer nudge. The review is right that it does not: it is still a reserve short of its floor, and the shortfall panel says so with the saved, target and missing amounts, where the generic card says none of them. Ordering it after also meant evaluating `pace` on a goal that has no pace to evaluate. bin/rails test: 6965 runs, 0 failures. RuboCop and erb_lint clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): stop a paused goal outranking a reserve that is whole Review on #3175. `:funded` and paused both ranked 3 in `active_display_sort`, so the tie broke on name and a paused goal called "Alpha" sat above a reserve called "Zeta" that was fully funded — the list saying the paused one wanted attention more. Paused now ranks behind every status, which is what the comment above the table already claimed. The seven panels on the goal page were hand-rolled repetitions of `DS::Card`'s exact shell, two of them adjacent and identical. They render through the primitive now, so their surface styling cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * refactor(goals): move the lifecycle panel decision out of the template Review on #3167 and #3180. Which panel a goal gets is a lifecycle question with five answers, and the template worked it out inline from `completed?`, `maintained?`, `one_off?`, `status` and `may_complete?` — five predicates deep in ERB where the ordering between them was load-bearing and nothing said so. `Goals::LifecyclePanelComponent` answers it in Ruby and the template renders the answer. The markup moves across unchanged, keys made absolute because a relative `t(".x")` in a component resolves against the component's own path rather than the page these strings belong to. The order is now stated once, where it can be read and tested: `:reserve_shortfall` before `:empty`, because a brand-new reserve sits at zero balance and zero pace and the generic "make your first transfer" card would otherwise swallow it. Closing from the panel now confirms, as the header menu already did. Completing releases the goal's earmarked money, and the panel offered that in one click. Both go through `goal_complete_confirm` rather than building the wording twice — two copies drifting apart is how one ends up describing the wrong consequence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): refresh the pace suggestion when the deadline is cleared Assigning `input.value = ""` fires no event, so `goal-form#suggestedChanged` never ran: selecting "Reserve to maintain" cleared the date but left the monthly pace suggestion on screen, derived from a deadline the goal no longer has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye * fix(goals): let a depleted reserve look as urgent as it is Review on #3179 and #3180. `Goal#needs_attention?` names the pair of statuses that mean "this one wants looking at" — a goal off its pace and a reserve below its floor. Three places were spelling that out and the Plan hub's progress bar had fallen behind, so a depleted reserve got a neutral bar an inch from its own amber status pill: the same goal reported as needing attention and not. `projection_summary` told a funded reserve it had "hit the target, no projection needed". A reserve holds a level; there is no finish line to project toward and no target to have hit. It does not reach that panel today — the shortfall and celebration panels catch it first — but the method reads as the single source of truth for that subtitle and should not hand a caller a one-off's wording. The legend swatches are bordered spans now rather than inline SVG, and the label takes `text-xs` instead of an arbitrary 11px. The projection swatch keeps the chart's own colour variables in an inline style rather than `border-success` / `border-warning`: the chart hard-codes green-600 and yellow-600, and a legend whose colour does not match the line it describes is worse than the markup it would save. The Plan-card test counts warning bars rather than matching one. A fixture goal is already off its pace, so the markup is on the page either way and a presence check passed without the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
eb498cfa0c |
Feature/category hierarchy and account search (#2845)
* Add parent/child category hierarchy to all category selects; add search to account select
Category selection consistency:
- DS::Select (shared component powering the main transaction form,
transaction edit, bulk-update, and transfer category pickers) now
indents subcategories with a corner-down-right icon, matching the
existing transaction-row category dropdown.
- Feed DS::Select-based category pickers with Category.alphabetically_by_hierarchy
(parent name, then parent-before-children, then own name) so children
render directly under their parent.
- Added Category::Group.select_options, a shared helper producing
parent-then-child ordered options (with an indent marker) for plain
HTML <select> elements. Used by:
- Rule builder category condition/action selects
- Bulk 'categorize transactions' select
- CSV/QIF import category mapping select
- Grouped the splits category combobox and the transaction search
category filter checklist the same way, both with the
corner-down-right indent icon used elsewhere.
Account selection:
- Added searchable: true to the account select in the new/edit
transaction form, matching the category and merchant selects next
to it.
* Fix SyntaxError: 'for' is a Ruby reserved keyword
Category::Group.select_options called for(categories) as a bare method
call, but Ruby parses a bare 'for' as the start of a for..in loop
statement, not a method invocation. Qualify it as self.for(categories)
to call the class method explicitly.
Verified with 'ruby -c' on all touched .rb files and ERB.new(...).src
on all touched .erb files.
* Add test coverage for category hierarchy and account search
Model-level:
- Category::GroupTest (new): for() grouping and the new select_options
helper (order + indent labels).
- CategoryTest: alphabetically_by_hierarchy scope ordering.
- Rule::ConditionFilter::TransactionCategoryTest (new)
- Rule::ActionExecutor::SetTransactionCategoryTest (new)
- Import::CategoryMappingTest (new): grouping + 'Add as new category'
still prepends correctly.
Controller/integration-level (asserting actual rendered HTML order):
- SplitsControllerTest: category combobox data-value ordering.
- Transactions::CategorizesControllerTest: bulk-categorize <select>
option ordering.
- TransactionsControllerTest:
- search filter checkbox ordering (q[categories][])
- new-transaction DS::Select category ordering (via trigger id +
ancestor traversal)
- new-transaction account select renders a search box
All new/modified test files verified with 'ruby -c' (syntax) and
cross-checked fixture names, family scoping, route helpers, and field
names against the actual fixtures/routes/views. Ruby/Bundler network
access to rubygems.org is unavailable in this sandbox, so the suite
itself has not been executed — run 'bin/rails test' before merging.
* Align with design-sure conventions: keep domain logic in component, not template
Per .cursor/rules/view_conventions.mdc ('keep domain logic out of the
views'), the parent/child hierarchy check for DS::Select items belongs
in the component class, not inline in the ERB template. DS::Select
already has this exact pattern for other per-item derived properties
(color_for, icon_for, logo_for) — added child? alongside them and
updated the template to call it instead of computing it inline.
Added test/components/DS/select_test.rb (ViewComponent::TestCase,
no rendering needed) covering child? directly: subcategory objects,
root-category objects, non-hierarchical objects (merchants), and the
include_blank placeholder item.
Also did a broader pass against the design-sure .cursor/rules to confirm
the rest of this branch's changes already comply:
- Uses Current.family (never current_family) throughout
- Uses the icon() helper exclusively, never lucide_icon directly
- No new/hardcoded colors; only existing semantic Tailwind tokens
already used elsewhere in these same files
- No changes to sure-design-system.css / application.css
- Extended existing components/partials rather than creating new ones
where one already existed (view_conventions.mdc component-vs-partial
guidance)
- Test additions stay in Minitest + fixtures, avoid system tests,
and test query-method output directly (testing.mdc)
* Address CodeRabbit review: sort category groups, tighten test assertion, move grouping out of view
* Address review: fix arrow leak in rule summaries, filter panel alignment, simplify splits ordering
* fix(pages): set breadcrumbs for changelog and feedback pages (#2889)
* fix(app): set breadcrumbs for changelog and feedback pages
* feat(test): add test to assert breadcrumbs
* fix(test): remove changes
* feat(app): update breadcrumbs to use semantic nav element
* feat(test): add breadcrumb assertions to changelog and feedback pages
* fix(app): replace breadcrumb nav element with div containing data-breadcrumbs attribute
* fix ci failures
* resolved failures
* Regenerate schema.rb from migrations
* Fix test
* Remove schema dump noise
---------
Signed-off-by: Shibu M <23173570+DataEnginr@users.noreply.github.com>
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Claude <claude@anthropic.com>
Co-authored-by: Kenrick Tandrian <60643640+KenTandrian@users.noreply.github.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
|
||
|
Sure Admin (bot)
|
3a9c8baf81 |
Warn before provider setup without encryption keys (#3007)
* Warn before provider setup without encryption keys * Cover provider encryption warning states * Warn on inline provider credential forms * Avoid duplicate provider encryption warnings * Add German passkey translations * Standardize provider encryption warnings |
||
|
Alessandro Pogliaghi
|
d18d853c8e | fix(lunchflow): keep balances current during rate limits (#3129) | ||
|
|
8f9529fbe9 |
feat(goals): a reached goal lets go of the money it was holding (#3165)
* fix(goals): stop two goals from each claiming the same account in full
A GoalAccount with a NULL `allocated_amount` means "dedicate the whole
balance". Two of them on one account each claimed all of it, so the money
was counted twice:
Livret A, 6,000 precaution 6,000 vacances 6,000
progress: 100% progress: 100%
`Goal#backing_share_for` cannot catch this. Its pro-rata haircut only
scales FIXED earmarks, and an unallocated link contributes `nil.to_d` —
zero — to `others_fixed`, so the two links never see each other. The
invariant "shares never sum past the balance" held for every earmark
except the one that claims everything.
Enforce it at the door: GoalAccount now refuses a second whole-balance
link on an account another non-archived goal already claims in full, and
asks for an amount instead. The scope matches
`Goal.pooled_allocations_for` — archived goals are excluded from the
backing math, so they do not block; completed goals still hold their
money, so they do.
Rows written before this guard stay readable and editable. Autosave
revalidates every loaded goal_account on `goal.save`, so validating
untouched links would make a goal that merely holds a legacy overlap
impossible to rename. Only a new link, or one whose amount is being
cleared onto a contested account, is checked.
The goal fixtures encoded exactly the forbidden state — three goals
claiming `depository` in full — so tests that built a fourth whole
claim now use accounts of their own. `build_goal` mirrors the old
balance, leaving every KPI figure unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4
* fix(goals): keep a restored goal from re-claiming an account in full
Addresses review feedback on #3160, raised independently on #3165, #3166 and
#3167 — one bug seen four times, because those branches stack.
Archiving a goal releases the accounts it claimed in full, so another goal can
legitimately claim one while it is away. Restoring it then put two
whole-account links back on the same account and reopened the double-counting
`whole_account_link_must_be_exclusive` closes: that check only fires when a
link is written, and a state change writes none.
A validation rather than an AASM guard. `may_fire_event?` stays true, the save
fails, and GoalsController#perform_transition! already surfaces
`errors.full_messages` — the user reads which goal holds the account instead of
a generic "can't do that in this state".
The conflict lookup now lives in one place, `Goal#whole_account_conflicts_on`,
read both by the door (writing a link) and by the restore, so the two cannot
drift into disagreeing about which goals still hold their money. The
`new_record? || will_save_change_to_allocated_amount?` bound stays on the
validation rather than moving into the shared lookup: it exists so a goal
merely holding a legacy overlap can still be renamed.
`Goal::RELEASED_STATES` replaces the repeated "archived" literal. The old
comment already said this scope had to move with the pool's; the constant makes
that true rather than hoped for, and the restore guard will follow the day the
set grows.
Restores from a released state are guarded; `resume` from `paused` is not. A
paused goal never let go of its accounts, so nothing can legitimately have
claimed one meanwhile, and blocking it would strand a user on a goal they
merely shelved.
Two things this surfaced in the test data:
- The fixtures had three goals each claiming `depository` in full — the exact
state the rule forbids. `test "AASM transitions"` failed on it, a true
positive. Two of them now take a fixed 1,000 slice.
- `current_balance sums linked account balances` asserted the gross balance,
which only held because of that overlap. A whole-account link takes what is
left after other goals' fixed earmarks; the test now says so, and computes it
from the data rather than a constant.
Each guard was confirmed load-bearing by removing it and watching its test
fail. bin/rails test: 6936 runs, 27911 assertions, 0 failures. RuboCop,
erb_lint and Brakeman clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): treat moving a whole-account link as the fresh claim it is
Addresses review feedback on #3160.
The exclusivity check was bounded to `new_record? ||
will_save_change_to_allocated_amount?`. A persisted whole-account row whose
`account_id` or `goal_id` changes is neither, so moving one landed it on an
account nobody had checked — the same double-counting hole a restore opened,
through a different door.
The bound is widened rather than dropped. It exists because `Goal has_many
:goal_accounts, autosave: true` revalidates every loaded child on `goal.save`,
so an unguarded check makes a goal that merely holds a legacy overlap
impossible to rename. That reason still holds for every row along for the ride;
it does not hold for a row being moved. A test pins both faces.
bin/rails test: 6939 runs, 27916 assertions, 0 failures. RuboCop and Brakeman
clean. Confirmed load-bearing by narrowing the bound back and watching the move
test fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): stop a link that is changing goals from conflicting with itself
Addresses review feedback on #3160, on the widening that landed in
|
||
|
|
c3cfdf03b2 |
fix(rules): validate Rule::Condition type registry + normalize legacy values (#1908)
* fix(rules): validate Rule::Condition type registry + normalize legacy 'name' values Closes #1229. /rules crashes with `ActionView::Template::Error (Unsupported condition type: name)` when a Rule::Condition row has a condition_type the registry doesn't know about. Reporter found two cases in the wild: - "name" — invalid, crashes the rules index because the view path (`_rule.html.erb` → `displayed_condition.filter.label`) ends up in `Rule::Registry#get_filter!` which raises. - "transaction_details" — actually a valid filter that searches the `transactions.extra` JSONB metadata field (see existing tests in test/models/rule/condition_test.rb). Reporter assumed it was a transaction-name synonym; it isn't. This PR leaves it alone. Changes: - Rule::Condition gains a SUPPORTED_CONDITION_TYPES registry constant matching Rule::Registry::TransactionResource#condition_filters plus "compound", an inclusion validation against it, and a before_validation callback that maps the one known legacy alias ("name" -> "transaction_name") so saving an existing rule fixes itself. - Rule::Condition#filter now rescues UnsupportedConditionError and returns a Rule::ConditionFilter::Unsupported placeholder. The placeholder labels itself "Unsupported (<key>)" (i18n) and its #apply returns `scope.none`, so any stale row that survives the migration stops matching rather than silently matching everything. - A one-shot migration normalizes existing "name" rows. Single UPDATE statement — rule_conditions is a small per-family table. Tests added in test/models/rule/condition_test.rb cover the inclusion validation, the normalization callback, and the graceful-render path (uses update_columns to simulate a row written before the validation existed — that's the exact codepath that crashes /rules today). * test(rules): add system test for unsupported condition_type render + raise on migration down - test/system/rules_test.rb: visit /rules with a row whose condition_type has been update_columns'd to "name" — assert page renders and shows the "Unsupported (name)" label instead of raising. - db/migrate/...normalize_rule_condition_types.rb: replace the empty #down with `raise ActiveRecord::IrreversibleMigration` to match the repo's data-migration convention (see e.g. 20260219190000_scope_*). * chore(rules): log unsupported condition + cross-check supported types Address maintainer review on #1908: - Log a Rails.logger.warn (with rule_id and condition_type) from Rule::ConditionFilter::Unsupported#apply so silent zero-match rules are traceable when debugging. - Add a comment above Rule::Condition::SUPPORTED_CONDITION_TYPES and a test that cross-checks it against the registry's filter keys, so drift between the two surfaces as a failing test rather than a confusing validation error. * refactor(rules): derive supported condition types from registry --------- Co-authored-by: John Baillie <johnbaillie2007@gmail.com> Co-authored-by: Khaostica <256858950+Khaostica@users.noreply.github.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
buzzromainandClaude Opus 5
|
7a86ee47c5 |
fix(goals): stop two goals from each claiming the same account in full (#3160)
* fix(goals): stop two goals from each claiming the same account in full
A GoalAccount with a NULL `allocated_amount` means "dedicate the whole
balance". Two of them on one account each claimed all of it, so the money
was counted twice:
Livret A, 6,000 precaution 6,000 vacances 6,000
progress: 100% progress: 100%
`Goal#backing_share_for` cannot catch this. Its pro-rata haircut only
scales FIXED earmarks, and an unallocated link contributes `nil.to_d` —
zero — to `others_fixed`, so the two links never see each other. The
invariant "shares never sum past the balance" held for every earmark
except the one that claims everything.
Enforce it at the door: GoalAccount now refuses a second whole-balance
link on an account another non-archived goal already claims in full, and
asks for an amount instead. The scope matches
`Goal.pooled_allocations_for` — archived goals are excluded from the
backing math, so they do not block; completed goals still hold their
money, so they do.
Rows written before this guard stay readable and editable. Autosave
revalidates every loaded goal_account on `goal.save`, so validating
untouched links would make a goal that merely holds a legacy overlap
impossible to rename. Only a new link, or one whose amount is being
cleared onto a contested account, is checked.
The goal fixtures encoded exactly the forbidden state — three goals
claiming `depository` in full — so tests that built a fourth whole
claim now use accounts of their own. `build_goal` mirrors the old
balance, leaving every KPI figure unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4
* fix(goals): keep a restored goal from re-claiming an account in full
Addresses review feedback on #3160, raised independently on #3165, #3166 and
#3167 — one bug seen four times, because those branches stack.
Archiving a goal releases the accounts it claimed in full, so another goal can
legitimately claim one while it is away. Restoring it then put two
whole-account links back on the same account and reopened the double-counting
`whole_account_link_must_be_exclusive` closes: that check only fires when a
link is written, and a state change writes none.
A validation rather than an AASM guard. `may_fire_event?` stays true, the save
fails, and GoalsController#perform_transition! already surfaces
`errors.full_messages` — the user reads which goal holds the account instead of
a generic "can't do that in this state".
The conflict lookup now lives in one place, `Goal#whole_account_conflicts_on`,
read both by the door (writing a link) and by the restore, so the two cannot
drift into disagreeing about which goals still hold their money. The
`new_record? || will_save_change_to_allocated_amount?` bound stays on the
validation rather than moving into the shared lookup: it exists so a goal
merely holding a legacy overlap can still be renamed.
`Goal::RELEASED_STATES` replaces the repeated "archived" literal. The old
comment already said this scope had to move with the pool's; the constant makes
that true rather than hoped for, and the restore guard will follow the day the
set grows.
Restores from a released state are guarded; `resume` from `paused` is not. A
paused goal never let go of its accounts, so nothing can legitimately have
claimed one meanwhile, and blocking it would strand a user on a goal they
merely shelved.
Two things this surfaced in the test data:
- The fixtures had three goals each claiming `depository` in full — the exact
state the rule forbids. `test "AASM transitions"` failed on it, a true
positive. Two of them now take a fixed 1,000 slice.
- `current_balance sums linked account balances` asserted the gross balance,
which only held because of that overlap. A whole-account link takes what is
left after other goals' fixed earmarks; the test now says so, and computes it
from the data rather than a constant.
Each guard was confirmed load-bearing by removing it and watching its test
fail. bin/rails test: 6936 runs, 27911 assertions, 0 failures. RuboCop,
erb_lint and Brakeman clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): treat moving a whole-account link as the fresh claim it is
Addresses review feedback on #3160.
The exclusivity check was bounded to `new_record? ||
will_save_change_to_allocated_amount?`. A persisted whole-account row whose
`account_id` or `goal_id` changes is neither, so moving one landed it on an
account nobody had checked — the same double-counting hole a restore opened,
through a different door.
The bound is widened rather than dropped. It exists because `Goal has_many
:goal_accounts, autosave: true` revalidates every loaded child on `goal.save`,
so an unguarded check makes a goal that merely holds a legacy overlap
impossible to rename. That reason still holds for every row along for the ride;
it does not hold for a row being moved. A test pins both faces.
bin/rails test: 6939 runs, 27916 assertions, 0 failures. RuboCop and Brakeman
clean. Confirmed load-bearing by narrowing the bound back and watching the move
test fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016GTNba5qE5NwzaHzbp27ye
* fix(goals): stop a link that is changing goals from conflicting with itself
Addresses review feedback on #3160, on the widening that landed in
|
||
|
buzzromainandClaude Opus 5
|
ed28fab807 |
fix(trades): let the header call a trade what the importer recorded (#3150)
The header derived its wording from the amount's sign and ignored investment_activity_label, except for Dividend and Interest. So every other labelled trade was announced as a buy or a sell: a Questrade journal, which records "Transfer", opened as a purchase it never was. The label now wins when this view has wording for it, and the sign remains the fallback for a trade with no label, or one carrying a label this view does not name. `I18n.exists?` guards that second case, so an unknown label degrades to buy/sell rather than rendering a missing-translation string. Wording added for the rest of Trade::ACTIVITY_LABELS. English only: fallbacks are enabled application-wide, so the other seventeen locales inherit it rather than raising. Not fixed by this, in case the title suggests otherwise: Kraken records its Contribution and Withdrawal labels on Transactions, not Trades, so it never reaches this view — its labels already display through the quick-edit badge. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
|
3d6a8d8b6e |
feat(budgets): move money between envelopes in one gesture (#3164)
* feat(budgets): carry a category's unspent budget into the next month
A budget category resets to zero every month, so anything non-monthly
(annual insurance, a holiday fund, car servicing) has no place to
accumulate. Two columns on budget_categories turn a category into a real
envelope: `rollover_enabled`, opt-in per category and off by default, and
`rolled_over_amount`, the surplus carried in from the previous month.
rolled_over(n) = rollover_enabled
? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1))
: 0
v1 floors at zero: only a surplus carries, never an overspend.
The amount is materialized, not derived. March depends on February which
depends on January, so computing it on read would walk the whole chain on
every budget render. Budget::RolloverCalculator recomputes it in a single
forward pass and writes once via upsert_all, from Budget.find_or_bootstrap
and from BudgetCategoriesController#update -- allocations and the toggle
being the only inputs. No Transaction hook: a past month's actuals can
change after the fact, and the page load is a fine moment to catch up.
Scope kept deliberately narrow. `Budget#budgeted_spending`,
`#allocated_spending` and `#available_to_allocate` are untouched -- the top
of the budget page still answers "I planned to spend X, I've allocated Y".
The carry is per-envelope information, surfaced as `Budget#total_rolled_over`
and never folded into those totals.
What the carry does change is consumption: `available_to_spend`,
`percent_of_budget_spent` and `budgeted?` all count it, or a category funded
entirely by rollover would read as unbudgeted and get an alert pill while it
still had money left. `display_budgeted_spending` stays the month's
allocation alone -- the card shows the two figures side by side.
Details worth knowing:
- A parent's carry is net of its ring-fenced subcategories'. A parent's
allocation already contains theirs and its actuals already contain their
spending; those subcategories carry their own surplus, so counting the
parent's raw leftover would roll the same money over twice.
- Chains never mix: household with household, a member's personal budgets
with their own. A missing month is a gap the carry crosses, not a month
budgeted at zero.
- The carry stops at a currency change. sync_budget_categories stamps
categories with family.currency at sync time while a budget freezes its
own at creation, so the guard is on budget_category.currency -- the unit
the amount is actually denominated in.
- upsert_all writes with `update_only`, so a concurrent request that moves
an allocation between our read and our write doesn't get it clobbered by
the stale value we loaded.
- copy_from! copies the toggle, never the amount.
Cost for families that never turn it on: one EXISTS query per budget page
load, measured, including on the reports page which also bootstraps a
budget. With rollover on, the walk starts at the first month that uses it
rather than at the two-year history bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): pin the household rollover chain to a viewer-independent scope
Addresses review feedback on #3143.
The household budget (user_id NULL) has no owner to scope actuals by, and
`IncomeStatement` falls back to `Current.user` when nobody says otherwise.
The calculator therefore computed one shared `rolled_over_amount` through
whichever member happened to load the page, and each viewer overwrote the
other's number -- last one wins, and a member could infer spending in
accounts they cannot see. `Budget#income_statement_accounts` can now be
overridden, and the calculator pins the household chain to the whole
family so the shared row holds one number. Personal chains are untouched:
they already scope to their owner's accounts and were always deterministic.
`copy_from!` runs after `find_or_bootstrap` has already recomputed the
chain, so copying `rollover_enabled` left the target sitting on a zero carry
until the next page load. It now recomputes before its transaction commits.
The toggle tooltip described the wrong direction. `incoming_carry` checks
the flag of the month being computed, so the toggle governs what that month
*receives* from the previous one, not what it sends forward. Reworded in
English and French.
The concurrency regression test now drives its concurrent write through
`Budget#budget_category_actual_spending`, a public seam, instead of stubbing
a private method of the calculator from another class's test suite.
Each guard was confirmed load-bearing by reverting it and watching its test
fail. bin/rails test: 6939 runs, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): let the rollover choice stand instead of resetting each month
`rollover_enabled` lives on budget_categories, one row per (budget,
category), so a month created by `find_or_bootstrap` was born with the flag
off. Switching rollover on for Vacations in January and simply opening
February dropped January's surplus on the floor -- the user had to re-arm
the toggle every month, or go through "copy from previous budget". The
feature's headline case, a category funded 50/month accumulating over a
year, did not work as shipped.
New rows now inherit the flag from the last initialized budget of the same
owner, the same chain the carry itself walks. Turning the toggle off on a
given month still overrides it from there on, so the per-month escape hatch
survives.
The flag stays on budget_categories rather than moving to Category, which is
where comparable products (Monarch, Copilot, Lunch Money) put it. Categories
here are family-wide while budgets are per owner, so a category-level flag
would force one member's rollover choice onto everyone's personal budget and
onto the household budget. budget_categories is the only table carrying both
the category and the owner. A regression test covers that isolation.
Naming follows the same products: the toggle reads "Rollover", the noun, not
"Roll over", the verb -- which also matches `rollover_enabled` and the
calculator. Both tooltips now describe the property rather than a direction
("keep this category's unspent money from one month to the next"). The
previous wording named the direction the flag actually gates, incoming,
which is accurate but the opposite of the mental model every comparable
product installs; describing the property is true under either reading. The
French card string switched to "+%{amount} de report" so it no longer has to
agree in number with a currency noun it cannot see.
bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed
load-bearing by removing it and watching its tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): make a rollover opt-out stop the money in both directions
`incoming_carry` gates what a month receives, but `leftover_for` computed
what it sends regardless of the toggle. So switching rollover off for one
month and back on the next handed the opted-out month's whole allocation to
the month after: the surplus the user meant to forfeit reappeared a month
later. Reproduced at 100, where 0 was expected.
The outgoing carry is now gated on the same flag, which also skips the
actuals lookup for opted-out rows. "Off" now means this envelope does not
roll over, in either direction -- the reading the standing toggle and the
tooltip both promise.
Found by CodeRabbit on #3143. It only became wrong with the standing-choice
inheritance in
|
||
|
buzzromainandClaude Opus 5
|
1fddb4d97c |
feat(budgets): carry a category's unspent budget into the next month (#3143)
* feat(budgets): carry a category's unspent budget into the next month
A budget category resets to zero every month, so anything non-monthly
(annual insurance, a holiday fund, car servicing) has no place to
accumulate. Two columns on budget_categories turn a category into a real
envelope: `rollover_enabled`, opt-in per category and off by default, and
`rolled_over_amount`, the surplus carried in from the previous month.
rolled_over(n) = rollover_enabled
? max(0, budgeted(n-1) + rolled_over(n-1) - actual(n-1))
: 0
v1 floors at zero: only a surplus carries, never an overspend.
The amount is materialized, not derived. March depends on February which
depends on January, so computing it on read would walk the whole chain on
every budget render. Budget::RolloverCalculator recomputes it in a single
forward pass and writes once via upsert_all, from Budget.find_or_bootstrap
and from BudgetCategoriesController#update -- allocations and the toggle
being the only inputs. No Transaction hook: a past month's actuals can
change after the fact, and the page load is a fine moment to catch up.
Scope kept deliberately narrow. `Budget#budgeted_spending`,
`#allocated_spending` and `#available_to_allocate` are untouched -- the top
of the budget page still answers "I planned to spend X, I've allocated Y".
The carry is per-envelope information, surfaced as `Budget#total_rolled_over`
and never folded into those totals.
What the carry does change is consumption: `available_to_spend`,
`percent_of_budget_spent` and `budgeted?` all count it, or a category funded
entirely by rollover would read as unbudgeted and get an alert pill while it
still had money left. `display_budgeted_spending` stays the month's
allocation alone -- the card shows the two figures side by side.
Details worth knowing:
- A parent's carry is net of its ring-fenced subcategories'. A parent's
allocation already contains theirs and its actuals already contain their
spending; those subcategories carry their own surplus, so counting the
parent's raw leftover would roll the same money over twice.
- Chains never mix: household with household, a member's personal budgets
with their own. A missing month is a gap the carry crosses, not a month
budgeted at zero.
- The carry stops at a currency change. sync_budget_categories stamps
categories with family.currency at sync time while a budget freezes its
own at creation, so the guard is on budget_category.currency -- the unit
the amount is actually denominated in.
- upsert_all writes with `update_only`, so a concurrent request that moves
an allocation between our read and our write doesn't get it clobbered by
the stale value we loaded.
- copy_from! copies the toggle, never the amount.
Cost for families that never turn it on: one EXISTS query per budget page
load, measured, including on the reports page which also bootstraps a
budget. With rollover on, the walk starts at the first month that uses it
rather than at the two-year history bound.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): pin the household rollover chain to a viewer-independent scope
Addresses review feedback on #3143.
The household budget (user_id NULL) has no owner to scope actuals by, and
`IncomeStatement` falls back to `Current.user` when nobody says otherwise.
The calculator therefore computed one shared `rolled_over_amount` through
whichever member happened to load the page, and each viewer overwrote the
other's number -- last one wins, and a member could infer spending in
accounts they cannot see. `Budget#income_statement_accounts` can now be
overridden, and the calculator pins the household chain to the whole
family so the shared row holds one number. Personal chains are untouched:
they already scope to their owner's accounts and were always deterministic.
`copy_from!` runs after `find_or_bootstrap` has already recomputed the
chain, so copying `rollover_enabled` left the target sitting on a zero carry
until the next page load. It now recomputes before its transaction commits.
The toggle tooltip described the wrong direction. `incoming_carry` checks
the flag of the month being computed, so the toggle governs what that month
*receives* from the previous one, not what it sends forward. Reworded in
English and French.
The concurrency regression test now drives its concurrent write through
`Budget#budget_category_actual_spending`, a public seam, instead of stubbing
a private method of the calculator from another class's test suite.
Each guard was confirmed load-bearing by reverting it and watching its test
fail. bin/rails test: 6939 runs, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): let the rollover choice stand instead of resetting each month
`rollover_enabled` lives on budget_categories, one row per (budget,
category), so a month created by `find_or_bootstrap` was born with the flag
off. Switching rollover on for Vacations in January and simply opening
February dropped January's surplus on the floor -- the user had to re-arm
the toggle every month, or go through "copy from previous budget". The
feature's headline case, a category funded 50/month accumulating over a
year, did not work as shipped.
New rows now inherit the flag from the last initialized budget of the same
owner, the same chain the carry itself walks. Turning the toggle off on a
given month still overrides it from there on, so the per-month escape hatch
survives.
The flag stays on budget_categories rather than moving to Category, which is
where comparable products (Monarch, Copilot, Lunch Money) put it. Categories
here are family-wide while budgets are per owner, so a category-level flag
would force one member's rollover choice onto everyone's personal budget and
onto the household budget. budget_categories is the only table carrying both
the category and the owner. A regression test covers that isolation.
Naming follows the same products: the toggle reads "Rollover", the noun, not
"Roll over", the verb -- which also matches `rollover_enabled` and the
calculator. Both tooltips now describe the property rather than a direction
("keep this category's unspent money from one month to the next"). The
previous wording named the direction the flag actually gates, incoming,
which is accurate but the opposite of the mental model every comparable
product installs; describing the property is true under either reading. The
French card string switched to "+%{amount} de report" so it no longer has to
agree in number with a currency noun it cannot see.
bin/rails test: 6942 runs, 0 failures. The inheritance was confirmed
load-bearing by removing it and watching its tests fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CyD26wsXjsYfpTgAGL1n1Z
* fix(budgets): make a rollover opt-out stop the money in both directions
`incoming_carry` gates what a month receives, but `leftover_for` computed
what it sends regardless of the toggle. So switching rollover off for one
month and back on the next handed the opted-out month's whole allocation to
the month after: the surplus the user meant to forfeit reappeared a month
later. Reproduced at 100, where 0 was expected.
The outgoing carry is now gated on the same flag, which also skips the
actuals lookup for opted-out rows. "Off" now means this envelope does not
roll over, in either direction -- the reading the standing toggle and the
tooltip both promise.
Found by CodeRabbit on #3143. It only became wrong with the standing-choice
inheritance in
|
||
|
Sure Admin (bot)
|
3bdfe9521e |
Fix AI health check for OpenAI-compatible endpoints (#3184)
* Fix AI health probe for OpenAI-compatible endpoints * Stabilize accounts sync system test * Address AI health probe review feedback |
||
|
Juan José Mata
|
b438a1c8dd | Improve chat tracking and system test assertions | ||
|
colinedwardwood
|
b1df16b0a7 |
Allow API transaction create to opt into sync protection (user_modified) (#3162)
* Allow API transaction create to opt into sync protection (user_modified) The transactions API has no way to mark a newly-created transaction user_modified, which is the only thing that protects an entry from a later provider sync (Plaid/SimpleFin/etc.) silently overwriting its category or name - Account::ProviderImportAdapter#import_transaction claims any entry matching on date/amount/currency with no external_id yet, then enriches unlocked fields from the sync payload. This matters for any API client that owns writes into an account also linked to a bank-sync provider: without a way to protect its own entries, the client's data can be silently overwritten the first time the linked provider happens to sync a matching transaction. Adds an optional `user_modified` param to POST /api/v1/transactions, reusing the existing Entry#mark_user_modified! (added for #1977, so far only wired into the merchant merge/convert/unlink flows) rather than mass-assigning the column directly. Exposes user_modified in the transaction JSON response, matching how external_id/source already are. Scoped to create only, matching the concrete need; happy to extend to update in a follow-up if that's wanted too. * fix: mark entry user_modified before enqueueing account sync sync_account_later enqueued the background sync job before mark_user_modified! ran, leaving a window where a fast-running job could read and overwrite the entry before the protection flag was set. Move the mark_user_modified! call ahead of the sync enqueue so the flag is always in place first. |
||
|
|
3a50846cc7 |
feat(mcp): support Streamable HTTP transport (protocol 2025-06-18) (#2631)
* feat(mcp): support Streamable HTTP transport (protocol 2025-06-18) Bumps the MCP protocol version from 2025-03-26 to 2025-06-18 to support clients using the Streamable HTTP transport, notably Bifrost v1.6.3+. Changes: - Add after_action hook to set Mcp-Protocol-Version header on all responses - Generate and return a sessionId in the initialize response - Set Mcp-Session-Id header on subsequent responses Without these headers, Bifrost's MCP client retries the initialize handshake 5 times and ultimately fails with a context deadline exceeded error, even though every JSON-RPC message is processed correctly. * test: update mcp protocol expectations * fix(mcp): negotiate protocol versions * fix(mcp): align transport error responses --------- Co-authored-by: secretsound <secretsound@users.noreply.github.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
Juan José MataandClaude Opus 5
|
d6462f5fc9 |
feat(snaptrade): add device-flow OAuth alongside the browser redirect (#3126)
* feat(snaptrade): add device-flow OAuth alongside the browser redirect SnapTrade could only be connected through the authorization-code + PKCE flow, which needs a confidential OAuth client: SNAPTRADE_OAUTH_CLIENT_SECRET and a redirect URI registered on the OAuth app. A deployment that cannot register one had no path at all. Add the device grant (RFC 8628) as a second way to obtain the same token, so people can pick the flow that suits their deployment. Both grants end at SnaptradeItem#apply_oauth_tokens!, so a device-authorized item is indistinguishable from a redirect-authorized one from there on -- same Bearer data calls, refresh, revocation and sync. Nothing about existing authorized items changes: no schema change, no migration, and the PKCE path is untouched. - Provider::Snaptrade gains start_device_authorization and poll_device_token, with endpoints read from SnapTrade's OAuth metadata document (cached). - oauth_configured? now means "some flow is available" (public client id), which is what gates syncing and the provider panel; the new authorization_code_configured? gates the redirect flow specifically. - Token and revocation requests authenticate as a public client when no secret is configured -- client_id in the body instead of HTTP Basic. Without this a device-authorized item would authorize fine and then fail at its first token rotation. - The settings panel offers both when both are available; every other entry point picks one through SnaptradeItemsHelper#snaptrade_authorize_path. - The device page carries a failed attempt's code back into the form, so "not confirmed yet" is a retry rather than a restart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): keep the provider panel's setup-step keys and cover both flows Two test_unit failures from the panel change. The setup steps were reordered and their keys renamed, which orphaned the translations twelve locales already had for them and broke the test asserting `oauth_setup_step_3`. The rename bought nothing: reword the steps in place instead, leaving the callback URL on step 2 where the interpolation lives. The panel tests stubbed `oauth_configured?`, which no longer decides which buttons render -- that is now `authorization_code_configured?`. Stub both, so the "configured" cases test the deployment they name, and add the device-only case that was previously unreachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): address device-flow review findings Two real bugs from the bot reviews, plus consistency work. The completion form posts into the `drawer` frame so errors re-render in place, but a successful redirect was then followed as a frame navigation. Both destinations carry the layout's empty `drawer` frame, so Turbo swapped that in and merely closed the dialog: the notice was lost and `return_to=setup_accounts` never advanced. Success now breaks out with a redirect stream action, the same mechanism holdings and categorizes already use, while errors keep rendering in the drawer. RFC 8628 §3.1 requires a confidential client to authenticate its device authorization request, and the panel offers the device code on deployments that configured a secret. That request now carries the same client authentication as the token request. Token endpoint resolution is now shared by all three grants, since whatever issued a token has to be what refreshes it. It reads the discovery document only when already cached and never fetches it, so the browser flow keeps working off the constant it has always used -- no new network call on refresh and no new way for an existing authorized item to fail. Also: the drawer no longer asks the provider whether it is configured, the controller tells it; and the test helpers restore the previous OAuth config rather than clearing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): reject a device authorization response that cannot drive the flow A 2xx missing device_code, user_code or a verification URI was passed straight to the drawer, which then rendered a blank code and a link to nowhere -- a dead end the user could only abandon. Every one of those fields is load-bearing, and a response without them is partial or schema-changed, so fail with a message instead. Same reasoning as the results-array check in get_positions. verification_uri_complete substitutes for verification_uri when present, since the drawer prefers it for the link anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): filter device-flow codes from request logs complete_oauth_device_flow receives the device code as a request parameter, and none of the existing filter_parameters patterns is a substring of "device_code" -- ParameterFilter matches on substrings, and "token", "_key", "secret", "code_verifier" and "code_challenge" all miss it. So Rails' default "Processing by ... Parameters: {...}" line was writing it in plaintext. That matters more here than ordinary log hygiene: the device code is the only capability check on redemption. Unlike the redirect flow's state, nothing binds a device code to the family that requested it, so anyone who can read the logs could redeem another family's in-flight authorization into their own item and pick up a token for that family's brokerage data. Adds :device_code, :user_code and :verification_uri_complete (which embeds the user code) to the filter list, with a regression test in the style of the existing Sophtron credential-filtering test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): bind a pending device authorization to its session The device code was posted back from the drawer as a form field, so the request body was the only thing deciding which item a pending authorization redeemed into. Nothing tied a code to the family that asked for it -- the guarantee `state` gives the redirect flow -- so a code recovered from anywhere could be redeemed into an item belonging to someone else, handing them a token for the victim's brokerage data. Hold the pending authorization in the session instead, where oauth_callback already keeps its code_verifier and state: - start_oauth_device_flow records the code, what the page displays, the family, the item and the return_to context under :snaptrade_device_flow. - complete_oauth_device_flow reads the code from there and refuses unless the flow was started by this session for this family and this item. A device_code parameter is no longer read at all, so there is no longer a way to inject one. - return_to and accountable_type come from the session too, so completion needs nothing from the form to find its way back. The code now never reaches the browser, which also makes the previous commit's log filtering a second line of defence rather than the only one. A failed attempt keeps the code only while it is still redeemable: expired_token and access_denied clear it so the page offers a fresh start, while a transient failure leaves it in place to retry. expires_in and interval are no longer carried anywhere, since nothing ever read them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk * fix(snaptrade): use one token endpoint for every grant poll_device_token resolved the token endpoint from the cached discovery document while exchange_code and refresh_tokens used TOKEN_URL, so which URL a device-issued token was refreshed at depended on whether the 12h metadata cache was still warm. If the discovered endpoint ever differed from the constant, a device-authorized item would work until the cache lapsed and then fail its first rotation -- and fail invisibly, since a refresh failure marks the connection requires_update. Resolve it by removing the choice rather than by making refresh depend on discovery. RFC 8628 §3.4 redeems a device code at the authorization server's token endpoint, the same one the authorization code grant uses: there is one token endpoint, not one per grant, and nothing to keep in sync between issuing a token and refreshing it. TOKEN_URL is also the endpoint the browser flow has been using in production, so it is the one with evidence behind it. Discovery is still consulted, but only for device_authorization_endpoint, which has no hardcoded equivalent. This also keeps refresh free of any network dependency it did not already have: reintroducing discovery there would have put a fetch, with retries and backoff, in front of every token rotation on items that never needed one. Also restore the previous OAuth configuration in the missing-client-id test instead of leaving the client id nil, which made it order-dependent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X3f2UyefTKJrgvNjPnhMRk --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
0aa43de10a |
Add experimental Swift-native Sure Insights app (#3134)
* Add Swift-native Sure app * Fix push subscriptions schema for CI * Address native app review feedback * Address remaining native app review feedback * Use Flutter app logo for native icon * Honor insight notification preferences and locale --------- Co-authored-by: Juan Jose Mata <2v8shcb6pz@privaterelay.appleid.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
Sure Admin (bot)andJuan José Mata
|
311f06e404 |
Fix budget cache invalidation after transaction deletion (#2808)
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> |
||
|
|
3e79de5b64 |
perf: memoize Family#balance_sheet/investment_statement, cache transactions-index side queries (#3058)
* perf: memoize Family#balance_sheet/investment_statement, cache transactions-index side queries The account sidebar renders on every page (mobile + desktop, 3 tabs each) and calls Family#balance_sheet multiple times per render; neither it nor Family#investment_statement/InvestmentStatement#current_holdings were memoized, so each call rebuilt the underlying query from scratch. Memoize both per-user (family sharing means different users must not share a cached BalanceSheet/InvestmentStatement). Also cache TransactionsController#index's uncategorized_count and projected_recurring lookups, which run unconditionally on every request regardless of whether the underlying data changed, using the same entries_cache_version-keyed pattern already used elsewhere in the codebase (e.g. Transaction::Search#totals). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address PR #3058 review feedback on cache invalidation and query reuse - Invalidate transactions-index caches when the current user's AccountShare access changes, not just on entries/recurring updates (CodeRabbit/Codex flagged revoked users could see stale data for up to a day). - Use full-precision timestamps instead of to_i in the cache keys so same-second updates aren't missed. - Reuse the already-memoized investment_account_ids in InvestmentStatement#current_holdings instead of an extra any? query. - Assert the rendered response instead of a controller instance variable in the uncategorized_count test, per CodeRabbit nitpick. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: assert rendered response, not implementation details, in PR #3058 tests Two CodeRabbit nitpicks from the second review round: the uncategorized-count cache-reuse assertion matched a scope name that never appears in generated SQL (making it vacuous), and the recurring-cache revocation test read the controller's private @projected_recurring ivar instead of the rendered page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: bust transactions-index caches on entry/recurring deletion and account status change jjmata's PR review flagged two invalidation gaps: hard-deleting an uncategorized entry or recurring transaction left the previous max updated_at unchanged (cache never busted), and toggling an account's active status doesn't touch entries/AccountShare at all. Fold in counts (like account_share_version already did) and a new Family#accounts_status_version, and move the version helpers onto Family/Current per the "fat models, skinny controllers" nit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include merchant version in projected_recurring cache key Editing or deleting a FamilyMerchant doesn't touch recurring_transactions, so the cached projected-recurring list (rendered with merchant name/logo, expires_in: 1.day) could show stale merchant data for up to a day. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(transactions): version projected-recurring cache by referenced merchants, not just FamilyMerchant recurring_transactions.merchant_id can point at a shared ProviderMerchant (recurring detection copies transaction.merchant_id), not just a family-owned FamilyMerchant. merchants_version only tracked Family#merchants (FamilyMerchant), so a ProviderMerchant update (e.g. ProviderMerchant::Enhancer setting name/logo) left the cached projected recurring list stale for up to a day. Replace Family#merchants_version with #recurring_transaction_merchants_version, scoped to the merchant records actually referenced by the family's recurring transactions (both FamilyMerchant and ProviderMerchant), and bump the cache key version. Also fix a flaky test assertion that matched all recurring_transactions-table queries instead of the actual projection query, since computing the cache key itself still runs small COUNT/MAX queries against that table on a cache hit. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> |
||
|
|
00d35993eb |
Exclude pending transactions from balances (#2897)
* Exclude pending transactions from balances * Fix balance regression assertions * Add pending balance review regressions * Fix merge conflict commit --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: Juan José Mata <jjmata@jjmata.com> |
||
|
Jeffandjeffrey701
|
12eb9e15fb |
fix(lunchflow): mark sync unhealthy when importer reports fetch failures (#1796) (#1873)
* fix(lunchflow): mark sync unhealthy when importer reports fetch failures (#1796) `LunchflowItem::Syncer#perform_sync` called `lunchflow_item.import_latest_lunchflow_data` but threw the result away and then ran `collect_health_stats(sync, errors: nil)`. The importer already catches per-account 429/500 fetch errors, bumps a `transactions_failed` counter, and returns `success: false` — but the syncer never inspected the return value, so the parent sync was marked completed/green even when zero transactions were imported because every fetch had been rate-limited. Capture the importer result and translate any `accounts_failed` / `transactions_failed` / `error` fields into the `{ message:, category: }` error shape `collect_health_stats` expects. The exception-path `rescue` branch is unchanged. Closes #1796 * test(lunchflow): i18n the new health messages + add syncer invariant tests (#1796) Two pieces of follow-up feedback: - @coderabbitai + @JSONbored: the three new operator-facing strings should go through I18n.t. Add keys under provider_warnings.lunchflow_* (matching the existing provider_warnings.limited_investment_data shape) and use Rails pluralization for the count-bearing entries. Other locales follow the repo's normal translation flow. - @jjmata + @JSONbored: add tests for the invariants. New LunchflowItem::SyncerTest covers: * successful import → sync healthy * accounts_failed positive → sync unhealthy with localized message * transactions_failed positive → sync unhealthy with localized message * both counters positive → both error entries recorded in order * sync raises → sync_error category + reraise (existing rescue branch) @jjmata also asked to confirm the importer contract: LunchflowItem::Importer#import returns 'success: accounts_failed == 0 && transactions_failed == 0' (see importer.rb), so the early 'return [] if import_result[:success]' guard is safe — success is never true while either counter is positive. --------- Co-authored-by: jeffrey701 <jeffrey701@users.noreply.github.com> |
||
|
Juan José Mata
|
fd6f4ff078 |
Add live AI checks to system health (#3155)
* Add live AI checks to system health Give super admins a dedicated AI status view with bounded liveness probes for LLMs, vector stores, pgvector, and embedding endpoints. Record sanitized failures in both the system debug log and Rails logger, and document the recommended local configuration.\n\nCloses #3145 * Fix AI health CI checks * Address AI health review feedback * Correct Ollama model preload guidance * Distinguish OpenAI-compatible providers * Make Ollama startup readiness explicit * Recognize Cloudflare AI endpoints |
||
|
buzzromainandClaude Opus 5
|
c26b16d36f |
chore(goals): drop copy-pasted duplicates and an unused lock-key helper (#3159)
`Account#goal_earmarked_total` and `Account#free_to_earmark` were each defined twice, byte for byte, with no `private` boundary or singleton class between the blocks — a copy-paste artifact where the second definition silently overwrote the first. Keep one copy of each. `Goal.advisory_lock_key_for` had no caller anywhere in app/; the only reference was a test asserting the dead helper was deterministic. Remove both. No behavior change. Claude-Session: https://claude.ai/code/session_01DJ1npaGEHr6t2HW1rYZdt4 Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
buzzromainandClaude Opus 5
|
721fc394ec |
fix(securities): give prefixed crypto tickers their logo back (#3151)
Security#crypto_base_asset only stripped a fiat suffix — "BTCUSD" gave "BTC" — so it answered nil for the "CRYPTO:BTC" form, which is the form every crypto integration writes: the on-chain wallets, Kraken, CoinStats and Binance all store the prefix. display_logo_url feeds that nil to the crypto branch, so none of those securities carried a logo at all. Provider::BinancePublic already parses every shape it accepts — the pair form, the prefixed pair, the bare base asset and the USD stablecoins — so this delegates instead of growing a second parser next to it. The parsing moves to a class method for that; the private instance method stays as a delegator, so its own callers and their tests are untouched. Verified across the four documented forms: CRYPTO:BTC and BTCUSD both give BTC, CRYPTO:ETH gives ETH, CRYPTO:USDT gives USDT. A logo still needs BRAND_FETCH_CLIENT_ID configured — this fixes the parsing, not the hosting requirement. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
GFRandClaude Sonnet 5
|
0252787dc0 |
fix(enable-banking): use real merchant instead of POS terminal line for name (#2968)
* fix(enable-banking): use real merchant instead of POS terminal line for name
Some ASPSPs (e.g. BankDirekt/Raiffeisen in Austria) return
remittance_information as a multi-element array where the first line is a
generic card terminal descriptor (\"POS 45,13 AT D6 31.07. 10:27\")
and a later line holds the real merchant. EnableBankingEntry::Processor
always used the first array element, so transaction names showed the
terminal string instead of the merchant.
primary_remittance_information now skips lines that look like a technical
terminal booking (POS/ATM + amount, or a trailing date+time stamp) and
prefers the first descriptive line, falling back to the original element
when nothing better is available. It also strips known small-merchant
payment-processor prefixes (SumUp, Square, iZettle, PayPal) from the
selected line.
Fixes #2935
Disclosure: this fix was written by Claude Code, verified against the
reporter's real (decrypted) Enable Banking payload and against test-stack
Rails test / RuboCop / Brakeman runs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(enable-banking): require both technical-line signals together
Address CodeRabbit/Codex review feedback on #2968: the POS/ATM+amount
prefix and the trailing date+time suffix were OR'd, so either alone
could misclassify a legitimate line as technical. A line embedding the
merchant right after the amount (e.g. "POS 45,13 BILLA DANKT ...") or
a legitimate descriptor that happens to end in a timestamp (e.g.
"Invoice paid 31.07. 10:27") would have been wrongly skipped.
Both signals are now required together in a single anchored pattern,
matching every real technical line observed in production while no
longer misclassifying either of the scenarios above.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(enable-banking): tighten date-stamp regex and clean up the merchant line
Addresses two review threads on this PR:
1. jjmata (PR review): technical_remittance_line?'s date-stamp check required
exactly 2-digit day/month (\d{2}[./]\d{2}), so an un-padded date ("1.07."
instead of "01.07.") wasn't recognized as technical and the line would
resurface as the transaction name -- reproducing the original #2935 bug for
that date shape. Now accepts 1-2 digits for both.
2. john-frandsen (issue #2935 comment): suggested cleaning up the merchant line
further (e.g. "BILLA DANKT 0007114 SIEGENDORF 7011" -> "Billa"). Checked
point 1 (structured remittance fields) against Enable Banking's own API
docs -- no such field exists there, not applicable. Points 2/3/5 already
match current behavior. Point 4 (loyalty-marker cleanup) implemented as two
layers:
- Primary: match the line against merchants the family already knows
(Family#known_merchant_names) -- self-maintaining, no pattern-guessing,
and now also assigns the transaction's merchant when matched (previously
out of scope for blank-counterparty EB transactions). Case-insensitive,
regex-escaped, longest-match-wins, with a minimum length guard against
spurious short-name matches.
- Fallback (no known merchant yet): remove only the "DANKT"/"DANKE"
thank-you marker word itself, not a directional truncation -- the marker
can precede or follow the merchant name depending on phrasing ("X DANKT"
vs. "DANKE ... bei X"), so truncating at it risked deleting the real
merchant name in one of the two phrasings.
Verified against the full test suite, RuboCop, and Brakeman on a test-stack
Rails instance (0 RuboCop offenses, 0 Brakeman warnings; full-suite failures
present on that instance are pre-existing/environmental and unrelated to
these files).
Disclosure: this fix (investigation, implementation, and tests) was written
by Claude Code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(enable-banking): address CodeRabbit/Codex review on merchant-line cleanup
Follow-up to
|
||
|
buzzromainandClaude Opus 5
|
0d0003b032 |
fix(onchain-wallets): show linked wallets on the accounts page, and drop the tracking row on unlink (#3136)
Two gaps in #3081 after it merged, both raised there by @jjmata's review of #2191 — credit to that PR for identifying them. **Linked wallets were invisible on /accounts.** The feature creates real Sure accounts and they count towards net worth, but the accounts page never showed them: `Account.manual` excludes anything carrying a provider link, and unlike the twenty other providers there was no on-chain section to claim them. A family whose only connection was a wallet saw the empty state on the one page meant to list their accounts. The controller now loads the items, the view renders them, and the empty-state condition counts them. The card is rendered by name rather than as a collection, because the model's default partial slot is already the provider settings row; renaming it would be the tidier convention but costs eight locale files of churn for a follow-up fix. **A generic unlink left the tracking row behind.** The dedicated disconnect flow is careful, but `AccountsController#unlink` destroys the AccountProvider directly, and an OnchainWalletAccount holds that link rather than being held by it. Orphaned, it stops syncing — the syncer only reads linked rows — while its partial unique index still holds the (item, chain, address, asset) slot, so linking that same asset again would collide with a row nothing displays. It is destroyed with its link now, following the CoinStats precedent in the same model, with a guard against the recursion that precedent lacks. The regression test is the one asked for explicitly: the Sure account and its holdings survive as manual while the tracking row and the provider link go. **The card only renders accounts the viewer may see.** Found in review of this branch. An item is surfaced as soon as ONE of its accounts is accessible, so rendering them all showed a member given access to one wallet account the names and balances of the others. Reproduced before fixing, with two real addresses under one item and a member shared into only one: the unshared account's name appeared on /accounts. Non-admins now get the accessible subset and the address count derives from it; admins keep the whole item, which is the rule visible_provider_items already applies. That pattern is not specific to this card — six existing providers pass `item.accounts` unfiltered to the same partial, which filters nothing. In the same reproduction the unshared name appeared twice, once from a CoinStats card over the same accounts. Raised separately for the maintainers; only this card is changed here. Both figures are computed on the item off the preloaded associations and prepared per card by the controller, the way `_coinstats_sync_stats_map` already is: a first attempt did it in the template with a `linked` scope, which opened a fresh relation and cost a query per row. Measured on /accounts with 1 then 5 wallet items: 52->79 queries became 50->69, so 6.75 per extra item became 4.75. Verified beyond the suite: a real Bitcoin address linked, then both flows driven over real HTTP — `GET /accounts` renders the card, and `DELETE /accounts/:id/unlink` leaves the account behind reporting `manual: true`. Integration parity was checked rather than assumed: on-chain now appears everywhere CoinStats does, the nightly family sync picks it up through its own reflection over `*_items` associations, and it is already exposed to the mobile client via /api/v1/accounts and to /api/v1/provider_connections. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Sure Admin (bot)
|
81c613f64a |
Improve async AI rule error reporting (#2271)
* Improve async AI rule error reporting * fix(rule-runs): preserve failures on job completion * fix(rule-runs): deduplicate failure messages |
||
|
Josh
|
ca66346dc5 |
feat: add safe admin user removal (#3131)
* feat: add safe admin user removal * fix: address user removal review findings * fix: close remaining user removal review gaps * fix: handle deleted users during session creation * fix: fail closed when session creation fails * fix: reject token issuance for inactive users |
||
|
|
735b62d9c7 |
Track self-custody wallets natively: Bitcoin, EVM and Solana (#3081)
* feat(onchain-wallets): foundation for self-custody wallet tracking
Adds the schema, models and the normalised contract every chain will
produce, with no chain implemented yet.
The central constraint of a multi-chain integration is that `case chain`
must not spread through the importer, processor, controller and views.
So a chain adapter's only job is to turn an address into an
Onchain::Snapshot — a list of Onchain::Assets and Onchain::Movements —
and Onchain::Chains is the single source of truth for which chains exist,
how their addresses are validated, what their native asset is, and which
adapter to instantiate. Everything downstream is written once.
Two tables:
- onchain_wallet_items: the family-level connection. Keyless by
default; the only credential is an optional Etherscan key, encrypted
via `encrypts`.
- onchain_wallet_accounts: one row per asset, per address, per chain.
Uniqueness uses three partial unique indexes, one per asset kind, because
the identity of an asset depends on its kind: a native coin is identified
by its address alone while a token is identified by its contract/mint. A
single index over all columns would treat two native rows with a NULL
contract_address as distinct and let duplicates through — the model test
proves the rejection comes from the database by saving with
`validate: false`.
The schema also leaves room for extended-key (xpub) wallets later: an HD
wallet is a set of derived addresses under one item, which the current
uniqueness key already allows, so adding it needs no destructive change.
db/schema.rb is hand-edited to add only the two new tables: regenerating
it with the Rails version now in the Gemfile reorders every column in the
file, which is out of scope for this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): chain-agnostic importer, processor and syncer
The whole pipeline is written once here and never branches on chain: it
consumes Onchain::Snapshots, so a chain is only ever a registry key and an
adapter.
Importer: refreshes every tracked address and records a digest of what it
saw (quantity plus movements, no timestamps) on each row. It deliberately
never creates rows — real wallets are full of spam airdrops, so a newly
seen token becomes trackable only when the user ticks it. An asset that
disappears from a wallet goes to zero rather than going stale.
Syncer: reprocesses only the rows whose digest changed. Two consecutive
syncs of an idle wallet write nothing at all — no row updates, no
holdings, no entries, and no queued account syncs. Both the importer and
syncer tests for that were checked against the pre-fix behaviour: with the
content_hash guard removed they fail.
Processor: writes the holding, the account balance and the movements.
Movements materialise two ways. When that day's price is known, a signed
trade (positive = Buy, negative = Sell) so cost basis and the value chart
reconstruct back to acquisition. Otherwise a display-only entry with
amount 0 and excluded: true, raw movement preserved in `extra` — visible,
but not inventing a value that would distort the account's history. Prices
are matched on the exact day for trades, because valuing a two-year-old
transfer at today's price would fabricate a cost basis.
Onchain::SecurityResolver binds a "CRYPTO:<SYMBOL>" ticker straight to the
crypto price provider instead of going through provider search, where
"USDC" can come back as a EUR-quoted pair and then need FX to repair. It
reuses an existing Security for the ticker whatever its MIC, so one asset
never splits into two records. Symbol normalisation for bridged
stablecoins and the price backfill follow in the next commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): canonical asset symbols and price backfill
Two things stood between a linked wallet and a correct valuation.
Bridged and wrapped variants. The same dollar is USDC on Ethereum, USDC.e
on Arbitrum and USDbC on Base; the same ether is ETH natively and WETH
once wrapped. Left alone each variant becomes its own Security, so one
gets priced and the others sit at zero, and the same asset held on two
chains reports two different values. Onchain::AssetSymbol maps the
variants that are redeemable 1:1 onto the canonical asset, so pricing them
as that asset is exact rather than approximate.
Missing price history. A Security created at link time has none, so on the
first sync every movement would fall back to a display-only entry and the
cost basis would never reconstruct — the feature would look broken exactly
when the user first looks at it. The processor now backfills the window in
one batched provider call rather than one call per movement date, includes
today so a wallet whose movements all predate the sync window still gets a
current valuation, and treats a provider failure as a logged warning: the
holding and the quantity are still written.
All of it is a no-op when no crypto price provider is enabled, which is
the case the settings panel has to warn about before linking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): Bitcoin adapter
Bitcoin has no account balances: an address owns unspent outputs, so the
balance is everything ever paid to it minus everything spent from it, and
mempool totals count — a broadcast-but-unconfirmed spend has already left
the wallet as far as its owner is concerned. Movements are the net effect
of each transaction on the address, so a self-transfer nets to zero and
produces no entry.
All three address formats are accepted (Base58 P2PKH/P2SH, bech32
segwit, bech32m taproot) with the character-set exclusions each encoding
actually has, and a malformed address is rejected before any request is
made — the test relies on WebMock failing the run if a request escapes.
Single address, not extended keys. A Bitcoin wallet is normally an HD
wallet: one xpub derives thousands of addresses and change goes to derived
ones, so tracking a single address under-reports such a wallet. Extended
keys would need BIP32 derivation (a dependency this codebase does not
want) or a descriptor-indexing backend. The limitation is stated in the
adapter, will be stated in the linking UI, and an xpub is rejected as an
address rather than silently treated as one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): EVM adapter for six networks, two backends
One adapter class serves every EVM network: a network's identity — label,
native coin, explorer URL, whether a family-supplied Etherscan key applies
— is data in the chain registry, so adding a network is an entry there
rather than a branch here.
The unit tracked is the (chain, address) couple, carried by the unique
index. A 0x address is valid on all six networks and holds different
balances on each, so detection asks each candidate network whether the
address is worth tracking there. That probe is exactly one request and
never reads paginated history: Blockscout's address summary carries the
coin balance and the token/transfer flags together, which is also why a
wallet holding only ERC-20 tokens with zero native balance is still found.
An explorer being down means "not detected here", not an error the user has
to interpret — a dead indexer must not break linking.
Two interchangeable backends behind Provider::EvmExplorer: keyless
Blockscout by default for every network, and Etherscan when the family
configured a key on a network the registry enables it for (today Ethereum
only), where a key buys nothing but a higher rate limit. Etherscan
deliberately does not implement the activity probe: its one-request answer
is the native balance, which reports "nothing here" for a token-only
wallet, so detection stays on the indexer that can answer correctly.
Zero-balance token rows are dropped — real wallets are full of spam dust —
while the native asset is always reported, even at zero, because a wallet
that spent everything still has a history worth keeping.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): Solana adapter
A Solana wallet does not hold its tokens. Each SPL token sits in its own
token account, owned by the wallet but addressed separately, so balances
come from enumerating those accounts across both token programs rather
than from reading the wallet address — and because one wallet can own
several accounts for the same mint, they are summed into one position.
Emptied token accounts are left behind on chain by design and are dropped.
RPC gives mints, not metadata. Well-known mints get their real symbol;
anything else is labelled with its mint in a form that deliberately cannot
pass for a ticker, so security resolution declines it and the asset is
tracked by quantity rather than priced as some unrelated coin that happens
to share a name.
Activity is one request. Bitcoin's Base58 addresses fall inside Solana's
address shape, so the two are told apart by asking the node: it rejects a
non-32-byte key, which reads as "not here" rather than an error.
The Snapshot it returns has the same shape as Bitcoin's and the EVM
adapter's — asserted in the test — so nothing downstream changed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): settings panel and linking flow
Linking is three steps: paste an address, confirm which network, choose
what to track.
The network step exists because address formats are not unique to a chain.
Candidates come from the address shape, and when there is more than one each
is asked whether the address is worth tracking there — one bounded request
each. When several answer yes, or none does, the user picks from a list that
marks which ones showed activity. Silently keeping the first match would
link the wrong network and surface later as a sync bug.
The token step imports nothing that was not ticked. Assets whose symbol a
price provider can quote are pre-checked; spam airdrops, whose "symbol" is
usually an advertisement, are listed unticked and can still be tracked by
quantity. Quantities and metadata are re-read from the chain when the
selection is applied, so a tampered selection can only change which assets
are tracked, never what they claim to hold. Previewing an address creates
no connection record, so an abandoned flow leaves nothing behind.
The panel and both modal steps carry a price-provider warning: with no
crypto market data enabled every wallet is valued at zero, which users
report as a broken sync rather than a missing setting, so it is said before
linking and links to where to fix it. The Bitcoin single-address limit and
"never enter a seed phrase" are stated in the linking UI too.
Errors are separated by kind. A rate limit or an unreachable explorer gets
its own localized message. Anything else is a bug: the user sees a generic
message and the class and message go to DebugLogEntry, never into the
response — asserted in the tests, which also check the exception text does
not appear in the body.
Adapters now translate their data source's errors into Onchain::Chains
errors, so the controller rescues two chain-agnostic types instead of
carrying a list of every explorer's error classes — which would have put
per-chain knowledge back into the controller.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): manage, review tokens, change address, disconnect
Four actions, because two would make the token choice irrevocable.
Review tokens reopens the selection screen with the address left alone —
deliberately without an address field. Without it the only way to untick a
token would be to change the address, which is a different operation
entirely. Assets the chain no longer reports stay listed and ticked, so
they can be dropped once they are gone.
Disconnect one asset is a per-row action with a button next to every asset.
A destroy route no view calls is dead code: the feature does not exist
until it has a button, so the test asserts one form per asset rather than
one per wallet.
Change address updates the existing rows instead of recreating them, so the
accounts, holdings, entries and balance history all survive — verified by
asserting a pre-existing balance is still there afterwards, and checked
against the recreate-instead-of-update behaviour, which fails it. The
content digest is cleared so the next sync reprocesses even if the new
address happens to hold the same amount, and an account still carrying its
generated name is renamed to match.
Disconnect wallet drops every asset at one address and leaves other
addresses alone.
Disconnecting never destroys an account: the provider link goes, holdings
are detached, and what the user can see stays as a manual account that
stops updating — the same contract every other provider's unlink has here.
The duplicate-address guard covers initial linking as well as address
changes. Without it, re-linking an already-tracked address would become the
unofficial way to add a token, quietly creating a second set of rows for
the same wallet; both guards are checked against that pre-fix behaviour.
Every lookup and mutation is scoped through Current.family, including the
per-row disconnect, which cannot reach a row belonging to another
connection.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(onchain-wallets): hosting guide for self-custody wallet tracking
docs/hosting/onchain-wallets.md covers what the feature reads, which
endpoint serves each network and how to point it at your own instance, the
optional Etherscan key and why it is optional, the per-sync request cost and
where history is capped, and the management actions.
Two things get stated plainly because they generate the support traffic:
prices come from a separate market data setting, so with no crypto-capable
provider enabled every wallet is tracked by quantity and valued at zero; and
Bitcoin is one address at a time, which under-reports an HD wallet whose
funds are spread across derived addresses.
Every locale key the feature uses is checked to resolve, including the
pluralised ones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): keep EVM balances on the keyless indexer when a key is set
Configuring an Etherscan key moved every read onto Etherscan, including
balances. That was wrong: Etherscan has no free endpoint that enumerates an
address's tokens, so its token balances are summed from transfer history —
which cannot see a rebasing token's current balance, and silently
under-reports any wallet whose history exceeds the page cap. A user adding a
key to fix a rate limit would have quietly traded it for wrong balances.
The two reads are now separated by what each backend can actually answer.
Balances and activity detection always go to the keyless indexer, whose
address summary answers both in one request — so adding a key buys nothing
there and cannot cost anything either. History, the paginated and
rate-limited half, is where a key helps and is the only thing it changes.
Provider::Etherscan drops its balance methods and refuses token_balances
with the reason, rather than offering an approximation that reads as a fact.
The chain-agnostic error mapping now accepts several error families per role,
since one snapshot can involve both backends.
Checked against the pre-fix behaviour: with balances routed through the
keyed backend, the new test reports 1 USDC held instead of the 7 the indexer
sees, and Etherscan raises on the balance call.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): name verified SPL tokens so they can be priced
Solana RPC returns mints, not names, so every SPL token outside the
hard-coded handful showed up as "SPL:abcd…wxyz" — which security resolution
correctly declines, leaving the asset tracked by quantity and valued at zero.
For a wallet holding anything beyond USDC that was most of its value.
Names now come from Jupiter's keyless token search, asked once per snapshot
for every mint at once, cached 24 hours per mint.
Only mints the list reports as *verified* are trusted. Anyone can mint a
token calling itself USDC; naming an unverified one would hand it the real
dollar's price and value dust at thousands. Unverified and unknown mints keep
the placeholder, and the test for that asserts the spam token is not named
USDC. Misses are cached as well, because spam wallets hold many mints that
will still be unvouched-for tomorrow.
Metadata is a naming nicety, not the wallet: a list that is down or rate
limiting degrades to placeholders instead of failing the snapshot, and the
hard-coded mints stay as an offline floor. Assets and movements resolve from
the same lookup, so a token cannot be called two different things within one
snapshot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): enable crypto pricing in one click, and log zero valuations
The warning said what was wrong and where to fix it, but still made the user
navigate to another settings page and pick the right provider out of a list.
On a self-hosted instance an admin can now fix it from the warning itself:
the crypto provider is appended to the enabled securities providers, leaving
the others alone — enabling crypto prices must not turn off whatever prices
the user's equities, and the test fails if it does. On a managed instance the
providers are the operator's setting, so the button is not offered and the
action refuses.
The other half is diagnosis. Until now a holding valued at zero for this
reason looked exactly like a holding whose price simply had not been fetched
yet: nothing recorded it, so support had to infer it from a screenshot. The
processor now records it via DebugLogEntry — but only for this cause, since a
price merely missing for today is ordinary and already covered by the
backfill.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): upgrade display-only movements once their price is known
A wallet linked before market data covered its history got the worst of both
worlds: its transfers landed as zero-amount excluded entries, and nothing ever
brought them back. The syncer only reprocesses assets whose on-chain state
changed, and a two-year-old transfer never changes — so the cost basis stayed
broken for exactly the wallets that were linked earliest.
perform_post_sync now runs a repair over every linked asset, not just the ones
that moved: what changed is the price history, which no chain read can report.
It reads prices from the database only, makes no network call, and does
nothing when there is nothing to upgrade.
An upgraded entry keeps its external_id, so it is the same transfer rather
than a duplicate. The entry is destroyed and rewritten because an Entry cannot
change entryable type in place and import_trade refuses an id already held by
a Transaction. Entries from other providers are never touched — matched by
source and by this asset's own id prefix.
Checked against the pre-fix behaviour: with perform_post_sync empty again, the
syncer test fails.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat(onchain-wallets): make history truncation visible and its depth configurable
History was capped silently. A wallet with more transfers than one sync reads
looked exactly like a wallet whose history was fully imported — the only trace
was a Rails log line on Bitcoin, and nothing at all on the EVM and Solana
paths. A user reconciling their cost basis had no way to tell the difference
between "this is all there was" and "we stopped reading".
Each source now reports whether it stopped on the budget or on the end of the
history. That travels on the Snapshot, so it stays chain-agnostic; the flag is
recorded on the affected rows, the manage screen says the history is
incomplete for that address, and the importer records it once per address per
sync — only when something changed, so an idle wallet with deep history does
not log the same line every night. The message also states what is not
affected: balances come from an address summary, never from history.
The depth itself is a hosting decision, not a property of a chain, so it moves
out of the providers into Onchain::HistoryBudget and is settable with
ONCHAIN_HISTORY_MAX_PAGES (default 10, clamped to 200). Adapters inject it, so
the providers stay unaware of the policy and the budget is testable without
touching the environment.
What this does not do is resume where it stopped. Reading older history across
syncs needs a per-wallet cursor and changes what the content digest means —
which is what guarantees an idle wallet writes nothing — so it is a change of
its own rather than a rider on this one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): read Blockscout token balances the way the API accepts
Verified against the live API: /api/v2/addresses/{address}/token-balances
rejects a `type` filter with HTTP 422 — the filter exists on token-transfers,
not here. Every EVM snapshot was therefore failing in production and
surfacing as "the explorer could not be reached", while the tests passed
because the stub encoded the same wrong assumption. No unit test can catch a
mistaken API contract; only asking the API can.
Without the filter the endpoint answers with every token standard the address
holds — ERC-20, ERC-721, ERC-1155, ERC-404 — so ERC-20 is now selected
client-side. An NFT row carries value "1" and no decimals, so it would have
been imported as a fungible balance of one token, priced by whatever its
symbol happened to resemble.
The tests now stub the endpoint the way it really answers and assert we ask
without a query, so re-adding the filter fails the run. Each token's
market-cap signal is carried through for the next commit, which has to decide
what to do about an address holding thousands of tokens.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): bound how many tokens one address can surface
Probing the live APIs turned up the other thing the stubs were hiding: real
addresses are airdrop dumping grounds. A well-known Ethereum address returns
7,924 ERC-20 balances (3.2 MB), and a comparable Solana one 2,801 token
accounts. Nothing bounded either. That meant a review screen with thousands of
rows nobody can use, and on Solana — where names are looked up in batches of
50 — around 56 extra requests per sync just to label an airdrop dump.
One read now surfaces at most 200 tokens per address
(ONCHAIN_MAX_TOKENS_PER_ADDRESS, clamped to 5,000). The native coin is never
affected, and anything already tracked keeps syncing regardless of the cap.
What survives the cap has to be both sensible and stable. On EVM the tokens
are ranked by the market cap the indexer already reports, so real assets stay
and airdrops fall off the end. Solana RPC gives no such signal, so the order
is the mint address: arbitrary, but identical between two reads of an unchanged
address — an unstable order would reshuffle the wallet, change the content
digest, and rewrite holdings every night. There is a test for that on both
chains. On Solana the cap is applied before metadata lookup, so it bounds the
requests as well as the rows.
The cap is not silent: it is recorded on the affected rows, stated in the token
review screen and in Manage wallets, and reported alongside history truncation
in the debug log with the limit that applied.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(onchain-wallets): walk the linking and management flow in a browser
The linking flow stacks three Turbo frame navigations — the provider drawer,
the linking modal on top of it, then the token review in that same modal —
and no controller test exercises any of that. Driving it in a real browser
found two things worth keeping:
The panel is reached two different ways. With nothing linked yet it is a card
under "Available" that opens the drawer; once a wallet exists the provider
moves to "Your connections", where the panel renders inline inside a
disclosure and there is no link to click at all. Only the first path had ever
been exercised.
Both paths now are, along with the parts of the flow that only exist in a
browser: assets arriving pre-ticked or not according to whether they can be
priced, review tokens reopening the selection with no address field present,
and disconnecting one asset leaving its account behind. One test per flow —
the branching cases stay in the controller test, where they cost a fraction
of the time.
Verified visually at each step: the warning banner, the Bitcoin
single-address note, the three-asset review with the spam token unticked, the
four management actions, and the accounts landing under Crypto with the wallet
subtype.
Full system suite green the documented way (DISABLE_PARALLELIZATION=true):
94 runs, 385 assertions, 0 failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): report a timed-out data source as unreachable
Reading a real Solana wallet turned up the hole: the public RPC timed out,
Net::ReadTimeout escaped every layer untranslated, and the user was told
something had gone wrong with Sure — the generic message reserved for our own
bugs — with a DebugLogEntry filed as if it were one. A public endpoint being
slow is the most ordinary failure this feature has.
All five clients now translate transport failures into their own ApiError:
timeouts, refused or reset connections, unreachable hosts, TLS errors, and a
response that is not JSON. The adapters already map a provider's own errors
onto Onchain::Chains::UnreachableError, so a timeout now reads as "the public
explorer could not be reached", the message that tells the user to retry,
while genuine bugs keep the generic one. During chain detection it goes back
to meaning "not detected here", so a slow explorer still cannot break linking.
The translated message carries only the error class, never the original
message, because a transport error's message contains the full URL and these
get logged.
Checked against the pre-fix behaviour on Bitcoin: with the translation
removed, the timeout test fails with the raw exception instead of the chain
error.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): name on-chain trades after the asset, not "shares"
Running a real Bitcoin address through the app showed every imported transfer
as "Buy 0.000003 shares of CRYPTO:BTC". That name comes from the shared trade
helper, which is written for equities; a wallet does not hold shares, and the
internal ticker is not what the user calls the coin. Trades are now named
"Buy 0.000003 BTC", through i18n like the display-only entries already were.
Also drops the sync status_text calls. Sync has no such attribute in this
schema, so every one of them was a guarded no-op and the four locale strings
they referenced could never render — dead weight copied from another
provider's syncer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): keep balances when a source refuses history
Reading real wallets on the free Solana endpoint exposed two problems, one of
which I introduced.
The budget. Making history depth configurable scaled Solana's transaction cap
from 25 to 250, and on Solana one transaction is one RPC call rather than one
page — so the same nominal depth became an order of magnitude more expensive
and a sync went from seconds to minutes. The per-transaction budget is now its
own constant, scaled proportionally off the page knob so one setting still
moves both, with a test that pins it far below the paginated row count.
The bigger one: a failure while reading history threw away the balances too.
A balance is one bounded request and is what a wallet fundamentally is; history
is paginated, far more expensive, and the first thing a throttled endpoint
refuses. On the free Solana endpoint, which routinely throttles getTransaction,
that meant a wallet showed nothing at all rather than showing what it holds —
permanently, not transiently.
History is now best effort: when the data source refuses it, the balances are
still recorded and the history is marked incomplete, which the manage screen
already surfaces. Anything that is not the data source failing still raises, so
a bug here cannot be swallowed. Verified live: the same Solana wallet that
failed entirely now reports 52.06 SOL and its SPL tokens with the history
flagged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): pre-tick what is worth something, not what parses as a ticker
Linking a real airdropped address showed 200 of its 201 assets arriving
pre-ticked, one click away from 200 accounts — the opposite of what reviewing
tokens before import is for. The pre-tick rule was "the symbol looks like a
ticker", and airdrops use perfectly plausible short symbols (0XBTC, 4CHAN), so
it selected nearly everything.
Assets now carry whether the data source treats them as notable, and only those
are pre-ticked. Two attempts at that signal, decided by measuring the real
address rather than guessing:
- Blockscout's `reputation` is "ok" for all 6,669 tokens it holds. Useless.
- Market-cap presence looks strong on the full list (365 of 6,669) but is
useless after the cap, which already ranks by market cap — hence every
surfaced token having one.
- Holding value discriminates: of the 365 priced tokens, 112 are worth more
than a dollar.
So on EVM networks a token is notable when the indexer can price it and the
holding is worth more than a dollar; on Solana when the verified token list
vouches for the mint; the native coin always. Pre-ticked count on that address
drops from 200 to 72, and everything else stays one click away.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): warn when nothing can convert USD prices into the family currency
A family whose currency is not USD needs two settings, not one, and only the
first was covered. The single provider that prices bare crypto symbols quotes in
USD, so valuing a wallet in EUR needs an exchange rate on top — and Sure's
default exchange rate provider requires an API key, so a self-hosted install
without one has no FX at all.
Tested against a real address in a EUR family: every wallet came out at zero,
with 275 transfers recorded as unpriced, and nothing anywhere said why. That is
the same support ticket the crypto-provider banner exists to prevent, arriving
through the other door.
Onchain::Pricing answers "can an on-chain asset be valued in this currency, and
if not, why" with the two reasons separately. The linking UI states whichever
applies — naming the currency for the FX one, and pointing at Frankfurter, which
needs no key — and a USD family never sees that warning because it needs no
conversion. The processor records the reasons when a holding lands at zero, so
support sees "exchange_rate" rather than guessing.
Verified live afterwards: with EXCHANGE_RATE_PROVIDER=frankfurter, the same EUR
family values the wallet at 416,198,342.25 EUR at 0.86363 USD/EUR, with all 275
transfers priced in EUR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): let a movement become a trade after being display-only
Syncing a real wallet twice — once before its prices were reachable, once after
— raised ArgumentError from the shared importer: an Entry cannot change
entryable type in place, and the display-only Transaction already held the
external_id the trade needed.
That is the ordinary case, not an edge one. A wallet linked before market data
covers its history gets display-only entries on the first sync, and the first
sync that can price them dies. Worse, it dies inside perform_sync, so the repair
pass that exists precisely to upgrade those entries — and which runs in
perform_post_sync, afterwards — was never reached. The account stayed stuck.
Both paths now go through one writer that discards a stale display-only entry
before the trade takes over its identity, so the entry keeps being the same
transfer rather than becoming a duplicate. Checked against the pre-fix
behaviour: without the discard, the new test raises the original ArgumentError.
Found by running a EUR family through two syncs on real data, which is the only
way the two price states occur in order.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): write a trade and drop its display-only entry atomically
Replacing a display-only entry with the trade it became is one change, but it
was two writes outside a transaction. A failure between them left the account
with neither: the transfer disappeared until some later sync happened to rewrite
it, which for an idle wallet could be never.
The repair pass already wrapped this; the sync path did not, and that is the one
that runs first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): tell apart two transfers of one token in one transaction
A token transfer was identified by its transaction hash and contract, which are
not unique together: a swap router or a batch payout routinely emits several
transfers of the same token involving the same address within one transaction.
The second overwrote the first, so a transfer disappeared from the account
without a trace.
Both EVM backends report the log index — the field that makes an event unique
inside a transaction — and neither was using it. It is now part of the
identifier, with the contract kept only as a fallback for an instance that does
not report one.
Reported by review on #3081; confirmed against the live Blockscout payload,
which carries log_index. The test fails against the previous identifier.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): canonicalise an address per chain before anything keys off it
Addresses were only stripped, never canonicalised, and what counts as the same
address differs by chain — so the duplicate guard could be walked straight past
and one Bitcoin case was worse than a duplicate.
On EVM, hex is hex: 0xABC… and 0xabc… are one wallet, but they linked as two,
each with its own accounts and holdings for the same balance.
On Bitcoin, bech32 is case-insensitive and canonically lowercase, and the API
reports outputs that way. An uppercase bech32 address passed validation, gave a
correct balance from the address summary, and matched no output at all — so the
wallet silently had zero movements and no cost basis, with nothing anywhere
saying why.
Canonicalisation is now the adapter's answer, since only the chain knows whether
case carries identity: EVM and bech32 fold, Base58 and Solana are left exactly
as given. The controller applies it as soon as the chain is known and before the
duplicate check, and on an address change too.
Reported by review on #3081. Both tests fail without the folding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): stop folding the case of Solana mints
Contract identifiers were downcased everywhere on the assumption that a contract
address is hex. That holds for ERC-20 but not for an SPL mint, which is a Base58
public key where case is part of the value: the stored mint was an unusable copy
of the real one, and two distinct mints could collide once folded into the same
string — one wallet's balance landing on the other's row.
Whether case carries identity is a property of the token kind, so it is now
answered in one place and applied consistently: by the asset's identity key, by
the column, and by the two comparisons in Onchain::Snapshot. A Movement no longer
folds anything on its own, because a movement does not know its asset's kind.
That also removes the duplication review flagged: the asset key was derived in
three places and only one of them folded, so the review screen and the linker
could disagree about what identifies an asset. There is one definition now,
OnchainWalletAccount#asset_key.
Reported by review on #3081. The test fails with the unconditional fold.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): isolate a failing address, and let a late symbol land
Two problems in the importer, both reported by review on #3081.
One unreachable address took the whole connection down with it: the loop over a
family's addresses had no rescue, so a Bitcoin explorer being throttled left an
untouched Ethereum wallet unsynced as well. Each address is now recorded and
skipped on its own — a row we failed to read keeps its previous quantity rather
than being zeroed, since we did not learn that it holds nothing — and only a
connection whose every address failed is reported as a failed sync rather than a
quiet success.
The content digest covered quantity and movements but not the metadata written
alongside them, so a Solana mint that later gained a real symbol from the token
list produced the same digest as before: the row was never rewritten and its
placeholder label became permanent. That silently undid the token-naming work.
The digest now covers everything the update writes.
Both tests fail against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(onchain-wallets): use DS::Button for the wallet actions
Five hand-built button_to controls carried their own utility-class strings for
what DS::Button already does — sync, disconnect a connection, stop tracking one
asset, disconnect a wallet, enable crypto prices. They drifted from the design
system on size, hover and destructive treatment, and the confirm prompts were
wired by hand.
The browser test walks the two that matter, so the behaviour is unchanged; this
is the styling and the confirm handling moving to the component that owns them.
Reported by review on #3081, along with the raw `bg-amber-600` on the provider
badge — left as it is, deliberately: all 23 other providers set a raw palette
class for their badge, there is no functional token for a brand colour, and
changing one entry would make it the only different one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): four smaller findings from the review on #3081
**A failed read no longer leaves an empty connection.** `link_wallet` created the
family's connection before fetching the address, so an explorer being down left a
connection with no wallets showing in the panel as connected. Reading needs no
saved connection — which is exactly why previewing uses an unsaved one — so it is
now created only once the read succeeds.
**Assets that could not be tracked are named instead of blamed on the user.**
`success?` is `created.positive?`, and the only failure message was "pick at least
one asset to track" — so a user who ticked three assets and hit three failed
creates was told to tick something, sending them back to tick the same three.
The linker already collected which assets failed; the message now says so, and a
partial failure is reported alongside what did get tracked instead of being
dropped silently. Same fix in the token revision action, which had the same shape.
**A malformed stored amount costs its movement, not the asset.** `BigDecimal()`
on a stored payload raises, and one unparseable row would fail the whole asset's
processing — including the repair pass. These payloads are written by this code,
so it takes a row that survived a format change, but the containment is two lines.
**No dead-end link.** The warning offered "open market data settings" to
everyone, while that page is gated to self-hosted admins — the same gate the
enable button already had. Both controls are now behind it, and the warning text
stands on its own for everyone else.
Each has a test that fails against the previous behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): reject non-finite amounts and quantities
The guard added an hour ago for malformed stored amounts was incomplete for the
very case it was written for: BigDecimal parses "NaN", "Infinity" and
"-Infinity", and none of them is zero, so all three sailed through
`parse_amount` into trade materialisation. Verified rather than assumed — the
test fails without the check with PG::NumericValueOutOfRange, so the infinity
reached the insert.
Fixed at both layers that write numbers. Movements: a non-finite amount skips its
movement, like any other unparseable one. Quantities: `normalize` treats
non-finite as unknown and writes zero, because Postgres numeric stores NaN
happily and one NaN quantity would turn every total that reads it into NaN.
Reported by review on #3081.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): derive the content digest from what actually gets written
Third time the digest missed a field it was supposed to cover: first the symbol,
so a Solana placeholder that gained a real name was never rewritten; now the
truncation flags, so a wallet whose history became complete — or started being
capped — kept showing the old completeness in the UI, since the early return
fires before `extra` is touched.
Rather than add a third field to a hand-picked list, the digest is now taken from
exactly the attribute hash that is about to be written. The two cannot drift
apart, which is what kept going wrong.
That needs two things to hold. Movements are sorted before the payload is built,
so the order a source happened to list them in is not mistaken for a change. And
the hash is canonicalised before hashing, because jsonb does not preserve key
order: `extra` written as {history, assets} comes back as {assets, history}, and
hashing that raw made an idle wallet's digest flip every other sync — the tests
caught it, and it is now covered by one asserting that reordered movements are
not a change.
Reported by review on #3081. Both halves checked against the pre-fix behaviour:
without the flags two tests fail, without the canonicalisation three.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(onchain-wallets): state the pricing coverage limit, and that DeFi is unseen
The hosting guide explained how to configure prices but never said what the
crypto provider actually covers. It quotes by symbol, and a symbol is not a
token's identity — measured on a real Ethereum address, two of its ten largest
token positions were quoted and the other eight, including holdings worth
roughly $406k, $141k and $74k, showed zero with their quantities tracked
correctly.
That reads as a broken sync unless it is written down, so it is now the first
limitation in the list, with the practical rule a user needs: a zero next to a
token you know is worth something means the provider does not list it, not that
the balance is wrong. Troubleshooting gains the matching entry, separating it
from the two configuration causes that produce a zero across every wallet.
Also records that DeFi positions — staked ETH, LP tokens, lending, Solana stake
accounts — are invisible, which was missing from the list entirely. A wallet
holding most of its value in a staking protocol reports a fraction of it.
Docs only; no code touched, so the suite was not re-run — the last run on this
tree was green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): do not zero an asset the token cap never reached
A tracked asset missing from a snapshot was always read as "the wallet no
longer holds it" and set to zero. That is right for a complete read, and wrong
for a capped one: at most 200 tokens per address are surfaced, so a tracked
token can be absent simply because the read stopped before reaching it. Its
balance was then wiped — a real holding removed from the user's net worth, with
a holding of zero written and the account balance reduced to match.
It is reachable: on Solana the surfaced set is ordered by mint address, which is
arbitrary, so a genuinely held USDC position on an airdropped wallet can fall
outside the cap and be zeroed on the next sync.
Absence of evidence is not evidence of absence — the same distinction this code
already makes for an address it could not read, where the row keeps its previous
quantity. When the snapshot is capped and the asset is not in it, the row now
keeps what was last known and only its completeness flags are refreshed.
Reported by review on #3081. The test fails against the previous behaviour, and
the "an asset that disappeared is set to zero" case still passes: a complete read
that no longer lists an asset still zeroes it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(onchain-wallets): rename the local `token` variables the secret scan trips on
Pipelock failed the PR with eleven "Credential in URL (high)" findings, all on
the same shape: a local variable named `token` being assigned. `token = ...`
is what a leaked credential looks like to a scanner, and the rule is a
reasonable one to keep.
Renamed rather than excluded. Excluding paths or adding `# pipelock:ignore`
would have kept the pattern and blunted the check for everyone; the new names
are at least as clear — `token_data` for the raw hash from an indexer,
`metadata` for the resolved symbol/name pair, `token_asset` for an
Onchain::Asset in tests.
Verified with the scanner itself rather than by inference: the same pipelock
2.8.0 the workflow pins, run locally over the branch diff, now reports "No
secrets found in diff". It also caught one site the CI log had truncated away
(the system test), which is why the local run was worth setting up.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): bound detection latency, and three review findings
Detection runs on the request thread but asked each candidate chain with a
sync's patience: a 30s timeout, three retries and exponential backoff, per
chain. A 0x address is a candidate on six networks, so one rate-limited
explorer could hold the page for minutes. Detection now reads with its own
budget - one short attempt, no retry, ONCHAIN_DETECTION_TIMEOUT to raise it -
while syncs keep the patient one. A chain that cannot answer in time is
reported as "no activity", which is the screen an ambiguous answer already
produces.
Also from review:
- A full page of Solana signatures is now reported as incomplete history.
The adapter passes no cursor, so a page that fills means the read stopped
short of the address's history; counting only against the transaction
budget called that complete.
- A reused CRYPTO: security with no price provider is bound to the crypto
one. A blank provider falls back to whichever is enabled first, and only
the crypto provider quotes a bare coin symbol, so the holding valued at
zero and read as a broken sync. A provider another integration chose is
left alone: the CRYPTO: prefix is shared with the exchange integrations.
- The chain select's label reached the form builder as an HTML attribute
instead of a label option, so no <label> was associated with the field.
Each regression test was checked against the pre-fix code: the two detection
tests fail with four requests instead of one, and the truncation test fails
by reporting complete history.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): let the table refuse a token row with no contract
The partial unique indexes key a token on its contract address, and NULLs
are distinct to Postgres, so a token row that reached the table without one
would slip past its index and duplicate freely. The model already refuses
it, but a direct write does not go through the model, and this repo puts
simple guarantees like this in the database.
Added to the existing migration rather than a new one: the table is created
by this branch, so the constraint belongs with it, and this leaves the
schema version untouched.
The regression test writes with validate: false, which is how the sibling
tests prove a guarantee comes from the table rather than from the model
above it. Without the constraint it fails with "expected but nothing was
raised".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(onchain-wallets): detect a token-only Solana wallet, and pin asset_kind
Two findings from the review of the previous commits.
Detection read only the wallet's lamports, so a wallet emptied of SOL but
still holding SPL tokens answered "nothing here". Each token account carries
its own rent, so an empty wallet address is not an empty wallet. The token
accounts are asked only once the balance comes back zero, so the ordinary
case still costs the one request this probe is meant to be, and accounts
left behind empty do not count as activity.
The narrow blast radius is worth stating: has_activity? only runs when an
address matches more than one chain, and when no candidate answers the user
is asked to choose rather than turned away. So this was a worse screen, not
a rejected wallet.
Separately, the check constraint accepted any asset_kind that carried a
contract address. Each partial unique index names its kind, so a row with
any other one is keyed by nothing and duplicates freely. Adding a token kind
already means adding its index here, so pinning the three in the table adds
no coupling that the indexes did not already have.
Both regression tests fail on the previous code. The third test - emptied
token accounts are not activity - passes either way by design: it guards the
new branch rather than testing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(onchain-wallets): cover the asset_kind constraint that shipped without it
The constraint landed in
|
||
|
|
6d6dac44f3 |
fix(i18n): remove duplicate locale keys and guard with a regression test (#2789)
* fix(i18n): remove duplicate locale keys that shadow translations
YAML resolves duplicate keys by silently letting the last occurrence
win, so every duplicated key hides an earlier definition without any
warning:
- transactions/{en,de,nl,hu}.yml defined `transactions.merge_duplicate`
twice: once as a stale flat string ("Yes, merge them") and once as the
`success`/`failure` mapping the controllers actually read. The flat
string is dead either way (the show view reads
`transactions.show.merge_duplicate`), so drop it.
- securities/fr.yml listed five providers (tiingo, eodhd, alpha_vantage,
mfapi, binance_public) twice inside `securities.providers`.
- settings/api_keys/hu.yml defined
`settings.api_keys.created.usage_instructions_title` twice.
- imports/en.yml defined `imports.create.ndjson_uploaded` twice.
All removals keep the currently-winning value, so rendered output is
unchanged; this only makes the files match what I18n already loads.
Add a non-skipped I18nTest case that walks the raw Psych AST of every
file under config/locales and fails on same-level duplicate keys, so
shadowed translations can't sneak back in.
Fixes #1506
Fixes #1502
* Fix linter / merge errors
* fix(i18n): repair duplicate locale regression test
---------
Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: rsnetworkinginc <rsnetworkinginc@users.noreply.github.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Juan José Mata <jjmata@jjmata.com>
Co-authored-by: sure-admin <sure-admin@splashblot.com>
|
||
|
|
4e010493c7 |
feat(up): map Up category slugs to Sure categories on import (#2487)
* feat(up): map Up category slugs to Sure categories on import UpEntry::Processor captured Up's category slug into extra but never applied it, so Up transactions imported uncategorised even though the user had already tagged them in the Up app. Add UpAccount::Transactions::CategoryTaxonomy + CategoryMatcher, mirroring PlaidAccount::Transactions::CategoryMatcher: map Up's child category slugs onto the family's existing/default Sure categories by alias, and wire the matcher through UpAccount::Transactions::Processor into UpEntry::Processor. The category is applied via the adapter's enrich_attribute, so a category the user has set or locked is preserved on re-sync. High-confidence mappings only. Up-specific categories with no honest Sure default (Booze, Pets, Apps & Games, Life Admin, Technology, ...) intentionally stay uncategorised for the user's own rules / AI, since a wrong auto-category is worse than none. Adds a matcher unit test and processor wiring tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(up): match category slugs as strings in CategoryMatcher Up category ids are string slugs; compare them against the taxonomy keys as strings so the lookup does not depend on the keys being symbols. No behaviour change (the "slug": hash syntax already produces symbol keys that matched the symbolized input, covered by the matcher unit test), but it removes a subtle footgun and reads clearer. Flagged by the Codex review on the PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(up): make category import non-destructive; word-boundary the alias match Per review feedback: do not bootstrap Sure's default categories during a sync. family_categories now returns the family's existing categories without creating defaults, so a family that has none (deliberately cleared, or pre-onboarding) gets uncategorised transactions rather than having the full default set silently created. Matching resumes once the user sets up categories through the normal UI flow. Also word-boundary the "and" stripping in the matcher normalization so it strips only the standalone conjunction, not "and" inside a word (e.g. errand). Adds a processor test for the non-destructive guarantee. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Gavin Matthews <matthews.gav@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
super
|
1a5d04d527 |
feat(rules): add a Transaction tag condition filter (#2558)
* feat(rules): add a Transaction tag condition filter Rules could already set tags via the set_transaction_tags action but had no way to match transactions by an existing tag. Add a select-type transaction_tag condition filter (mirroring transaction_category) with the standard "Equal to" and "Is empty" operators, registered on the transaction rule resource so it surfaces in the rule builder automatically. Also remap the tag UUID<->name in family data export/import for the new condition, matching how the transaction_category/transaction_merchant conditions and the set_transaction_tags action are already handled, so tag-based rules survive an export/import round-trip. Closes #2557 * fix(rules): match transaction_tag via EXISTS so compound tag conditions work Addresses review feedback on #2558: the tag filter joined transactions.tags and predicated on tags.id, which broke two compound cases: - two ANDed tag conditions collapsed to `tags.id = a AND tags.id = b` on the same joined alias and could never match, even when the transaction had both tags; - OR / multi-tag matches returned a transaction once per tagging row, inflating counts and making rule actions iterate duplicate transactions. Use a correlated EXISTS subquery per condition instead. Each condition is independent (fixes the AND case) and no join is added, so rows are never multiplied (fixes the OR duplication) and prepare adds nothing, keeping branches structurally compatible inside a compound OR. Add tests for both. |
||
|
|
6439a731ab |
feat(self-hosting): surface Sidekiq-unhealthy nudge + admin system health page (#1906)
* feat(self-hosting): surface Sidekiq-unhealthy nudge + admin system health page (#1481) When the Sidekiq worker container isn't running — the most common Docker Compose misconfiguration in self-hosted setups — every background job silently never executes. Balance calculations, net-worth updates, and account syncs stall. The UI shows zeros and "No balance data available for this date" without explaining why (#1481, #1047). Per jjmata's resolution on the issue, this PR ships both halves of the fix in one pass: 1. A user-facing nudge banner that appears on every authenticated page when Sidekiq isn't processing jobs. Tells the user their data may be stale; doesn't pretend zeros are real. 2. An admin-only deep link from that banner into a new `/settings/admin/system_health` page (super-admin gated, matching the existing admin namespace contract) showing live Sidekiq state: process count, last heartbeat, max queue latency, job counters, and per-queue depth. ## What changed - New `SidekiqHealth` PORO (`app/models/sidekiq_health.rb`) eagerly loads ProcessSet + Queue + Stats in one pass and exposes `healthy?` plus a stable `reason` symbol (`:redis_unreachable`, `:no_worker_processes`, `:stale_heartbeat`, `:queue_backed_up`). Any Redis/Sidekiq failure during the eager load is caught and surfaced as `:redis_unreachable` so a degraded broker never crashes the layout. - `ApplicationController#current_sidekiq_health` memoizes a single instance per request via `helper_method` so the layout, banner partial, and any controller checks share one Redis round-trip. - New `app/views/shared/_sidekiq_health_banner.html.erb` rendered from `_htmldoc.html.erb` when `Current.user` is present and the health check is failing. Banner shows the user-facing message to everyone; the "View system health" CTA + reason detail are gated on `Current.user&.super_admin?`. - New `Admin::SystemHealthController#show` (inherits the existing `Admin::BaseController`, so super-admin gating is enforced for free) + view rendering status, counters, and per-queue breakdown. - Routes: `resource :system_health, only: :show` inside the existing `namespace :admin`. - Settings nav: new "System health" entry under the Advanced section, gated on `super_admin?` to match `sso_providers_label` and `users_label`. - i18n: new `shared.sidekiq_health_banner.*` keys (title, body, CTA, per-reason explanations) and a full `admin.system_health.show.*` namespace for the new admin page. English-only, matching how `ds.pill.*` and other DS keys are scoped. ## Why - jjmata: "Let's take both approaches ... a nudge about 'data unavailable' which hyperlinks to the admin UI if you are an admin only (not for other types of users) sounds like the best path forward. **Any takers for the PR?**" (#1481) - smurfpandey: "We can add a section in Settings for superadmins to see 'health' of the application/host." - The detection signal is conservative on purpose: - `PROCESS_HEARTBEAT_TIMEOUT = 2.minutes` tolerates deploy restarts and brief Redis blips without flapping. - `LATENCY_THRESHOLD = 5.minutes` is well above the sync-job tail under default `config/sidekiq.yml` concurrency. ## Validation This worktree runs on Windows without a local Ruby toolchain, so I could not run `bin/rubocop`, `bundle exec erb_lint`, `bin/brakeman`, or `bin/rails test` locally. CI will run the full matrix on the PR: - `lint` — `bin/rubocop -f github` - `lint_js` — `npm run lint` (no JS touched, should be green) - `scan_ruby` — `bin/brakeman --no-pager` - `scan_js` — `bin/importmap audit` - `test_unit` — `bin/rails test` (includes 7 new tests under `test/models/sidekiq_health_test.rb` and 4 new under `test/controllers/admin/system_health_controller_test.rb`) - `test_system` — `DISABLE_PARALLELIZATION=true bin/rails test:system` - `pipelock` — secret + agent-security diff scan Manual checks done in this worktree: - Re-read `CONTRIBUTING.md` and `.cursor/rules/project-conventions.mdc`. PORO under `app/models/` per Convention 2. No new gem dependency per Convention 1. Banner uses semantic tokens (`bg-warning/10`, `text-warning`) per the design-system rules. No `lucide_icon` direct call — uses the `icon` helper per CLAUDE.md. - Confirmed `Sidekiq::ProcessSet` / `Sidekiq::Queue` / `Sidekiq::Stats` are the same APIs Sidekiq 7+ exposes (we're on Sidekiq 8.x per the `Gemfile.lock` comment in `config/initializers/sidekiq.rb`). - Tests stub `Sidekiq::ProcessSet.new` / `Sidekiq::Queue.all` / `Sidekiq::Stats.new` so the suite doesn't need Redis populated. - The admin route lives inside the existing `namespace :admin` so `Admin::BaseController#require_super_admin!` enforces auth — no new authorization surface added. ## Notes - No public API endpoints, no rswag specs, no OpenAPI changes. - No migrations, no model changes outside the new PORO. - No background jobs touched. - English-only locale entry, mirroring the `ds.*` / `admin.invitations.*` precedent in this repo. Other locales fall back to English. - Detection thresholds are constants on `SidekiqHealth` so they're easy to tune from a follow-up PR if the defaults turn out to flap on any real-world deployment. - The banner positions itself at `top-20` (below the impersonation / super-admin bars) and uses `z-40` (below the `z-50` notification tray). Single-screen overlap with mobile flash toasts is acceptable for V1. Refs: #1481, #1047 * fix(self-hosting): address review on Sidekiq health PR (#1481) - `Admin::SystemHealthController#show` now reads from the request-memoized `current_sidekiq_health` instead of building a fresh `SidekiqHealth.new`, so the controller and the layout banner share one Redis round-trip. - `SidekiqHealth#reason` now treats `last_heartbeat_at.nil?` the same as a stale beat: a registered process that hasn't published a heartbeat is not "healthy". Previously the check short-circuited on the nil guard and silently fell through to the queue-latency branch. Added a unit test covering the `ProcessSet` entry with `"beat" => nil` case. - Settings nav: switched the "System health" entry's icon from `activity` to `heart-pulse` so it no longer duplicates the LLM Usage icon. - Routes: dropped the redundant `controller: "system_health"` option from the `resource :system_health` declaration — Rails infers `Admin::SystemHealthController` from the namespace, matching the style of the sibling `:sso_providers`, `:users`, `:invitations`, and `:families` admin resources. * fix(self-hosting): scope + cache Sidekiq health, admin-only banner (#1481) Addresses the second round of maintainer review on the Sidekiq health PR. - Skip the check entirely in managed mode. `current_sidekiq_health` returns `nil` unless `Rails.application.config.app_mode.self_hosted?`, so authenticated requests in managed deployments add zero Redis round-trips for this feature. - Cache the snapshot across requests via `SidekiqHealth.current` (Rails.cache, TTL `CACHE_TTL` = 60s default, env-overridable). The per-request memoization on `ApplicationController` is preserved on top, so even back-to-back self-hosted pages share one fetch. - Make thresholds operator-tunable. `PROCESS_HEARTBEAT_TIMEOUT`, `LATENCY_THRESHOLD`, and the new `CACHE_TTL` read from `SIDEKIQ_HEALTH_HEARTBEAT_TIMEOUT`, `SIDEKIQ_HEALTH_LATENCY_THRESHOLD`, and `SIDEKIQ_HEALTH_CACHE_TTL` env vars (seconds), with the previous values as defaults. Comments now explain the tuning rationale. - Gate the banner on `Current.user&.super_admin?` at the layout level rather than rendering a vague warning to family members who can't act on it. The partial no longer carries an internal admin check since the call site does it; non-admins see nothing. - Replace the hard-coded `top-20` offset with a computed offset based on which impersonation bars are visible (`top-4` / `top-20` / `top-36`) so the banner doesn't collide with the super-admin or approval bars when both are stacked above it. - `Admin::SystemHealthController#show` now bypasses the cache (`SidekiqHealth.expire_cache!` + `SidekiqHealth.new`) so an operator who just restarted the worker sees fresh state instead of a stale 60-second snapshot. Also lets the page render in managed mode where `current_sidekiq_health` is nil. - Tests: add coverage for `.current` cache reuse and `.expire_cache!` forcing a re-query, swapping `Rails.cache` to a MemoryStore since the test env defaults to `:null_store`. * fix(self-hosting): route singular resource + drop assert_same on cached snapshot (#1481) Two CI failures surfaced once the full pipeline ran on this branch for the first time (it was gated on contributor approval until d04b78e): - Admin system-health controller tests returned 404. Singular `resource :system_health` in `config/routes.rb` makes Rails infer `Admin::SystemHealthsController` (it pluralizes the controller name even for singular resources), but the controller file is named `system_health_controller.rb` / `Admin::SystemHealthController`. Restore the explicit `controller: "system_health"` override that the previous "address review" commit dropped on the (mistaken) premise that Rails would infer it from the namespace — the sibling admin routes all use plural `resources` so they round-trip cleanly, this one doesn't. Comment now spells the gotcha out so the next reviewer doesn't try to "simplify" it again. - `SidekiqHealthTest#test_current_memoizes_across_calls_inside_the_cache_TTL` used `assert_same` on the two returns from `SidekiqHealth.current`. `ActiveSupport::Cache::MemoryStore` defaults to `dup_values: true` and Marshals on read, so a cache hit returns an `==`-equal but `equal?`-different instance. Replace the identity check with the behavioral assertion we actually care about: re-stub `ProcessSet` to raise on the second call, then assert the second `current` return is still healthy (proving Redis was not re-queried). * fix(i18n): drop redundant inline default on system_health nav label (#1481) `system_health_label` is already defined in config/locales/views/settings/en.yml, so the inline `default: "System health"` was a hard-coded English string in the template (DS Drift Patrol Rule 5). Use the bare locale lookup like the sibling nav entries. --------- Co-authored-by: John Baillie <johnbaillie2007@gmail.com> Co-authored-by: Khaostica <256858950+Khaostica@users.noreply.github.com> |
||
|
|
5a2bf02b13 |
fix(simplefin): stop repair_stale_linkages from hijacking a live linkage on a same-name twin (#3116)
* fix(simplefin): stop repair_stale_linkages from hijacking a live linkage on a same-name twin repair_stale_linkages matched purely on case-insensitive display name, so two distinct upstream accounts sharing a name (e.g. two "CHECKING (0001)" accounts at the same institution) caused the unlinked twin to silently steal the linked account's AccountProvider, merge in its transactions, and overwrite its balance on every subsequent sync. Thread the upstream account_id set already computed during account discovery through to repair_stale_linkages so it only treats a linked account as stale when its account_id is actually absent upstream, and skip ambiguous multi-way name matches instead of picking the first one. Fixes #2852 * fix(simplefin): clear stale upstream_account_ids and log skipped repairs - Clear simplefin_item.upstream_account_ids at the start of each perform_account_discovery run so a later discovery that finds zero accounts can't reuse IDs from a prior run on the same SimplefinItem instance (CodeRabbit review finding). - Capture skipped stale-linkage repairs via DebugLogEntry so operators can see them in /settings/debug, not just the raw Rails log (Codex review finding). - Fix two pre-existing SimplefinAccount::Transactions::ProcessorInvestmentTest tests broken by the new upstream_account_ids nil-guard: they called process_accounts directly without going through the Importer, so they now set upstream_account_ids explicitly to simulate a legitimate "old account_id genuinely absent upstream" repair. - Add regression coverage for both fixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
pro3958andagentloop
|
fcb46be19f |
fix(transactions): re-render form when creating a transaction with no account (#2777)
TransactionsController#create looked up the account with accessible_accounts.find(params.dig(:entry, :account_id)). With no account selected the id is blank, find raises RecordNotFound, and StoreLocation's rescue_from turns that into head :not_found — the 404 on /transactions the reporter saw. Switch to find_by(id:) and, when it returns nil, rebuild the entry, run validation, and re-render :new with 422, matching the existing validation-failure branch. This covers a blank, missing, or invalid account_id, so the user gets the form back with errors instead of a dead button. Fixes #2566 Co-authored-by: agentloop <agentloop@localhost> |
||
|
|
b0ecb919b0 |
fix(lunchflow): refresh stored transaction on pending to posted (#2778)
* fix(lunchflow): refresh stored transaction on pending to posted LunchflowItem::Importer keyed stored raw transactions but treated them as immutable snapshots. When Lunchflow flipped a transaction from pending to posted under a stable ID, fetch_and_store_transactions skipped it as a duplicate and kept the stale pending snapshot. The processor then re-imported it with isPending:true, so ProviderImportAdapter#import_transaction never reached its pending-clearing branch and the entry stayed stuck with a "Pending" badge. Index stored transactions by key (the Lunchflow ID, or a content hash for the blank IDs Lunchflow returns for some pendings) and refresh the stored snapshot in place when the upstream payload actually changed, while still deduplicating by key to prevent unbounded growth. Fixes #2735 * fix(lunchflow): preserve identical same-response blank-ID transactions The stored snapshot was keyed with a Hash of key -> transaction, so two rows in one sync response that share a content hash (Lunchflow returns blank IDs for some pending transactions, and two genuinely distinct identical purchases hash the same) collapsed into a single entry. The second row hit the existing-key branch as though it were a duplicate, dropping a real transaction before LunchflowEntry::Processor could apply its collision suffix. Pool the existing snapshot into one bucket per key and match incoming rows one-for-one (shift), so same-response collisions each claim their own slot or count as new. This preserves every real transaction while still deduplicating across syncs (re-syncing the same pair stays at two, not four) and refreshing pending -> posted transitions. Adds a regression test for two identical blank-ID rows in the same response. --------- Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: pro3958 <pro3958@users.noreply.github.com> |
||
|
|
964817748e |
fix: keep excluded entries visible in account activity (#2772)
* fix: keep excluded entries visible in account activity AccountsController#show built the activity list with @account.entries.where(excluded: false), which hard-hid every excluded entry. Trades only appear in the account activity feed, not the global /transactions page, so once a trade was excluded from analytics there was no row to click and no way to toggle exclusion back off. The balance still counted the trade, so the row vanished from the list but remained in the balance tooltip. Use the excluding_split_parents scope instead, matching Transaction::Search. Excluded entries render greyed-out and can be re-included via the existing exclude toggle in the drawer; only split parents stay hidden. The account list is now consistent with both the global transactions list and the balance calculation. Fixes #2612 * Fix account activity test lint --------- Signed-off-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: Juan José Mata <juanjo.mata@gmail.com> Co-authored-by: sure-admin <sure-admin@splashblot.com> |
||
|
|
29c0a369d0 |
fix(transfers): isolate concurrent transfer match in a savepoint (#2769)
* fix(transfers): isolate concurrent transfer match in a savepoint auto_match_transfers! opens one Transfer.transaction and, per candidate, calls Transfer.find_or_create_by! while rescuing RecordNotUnique. The transfers table has a composite unique index on (inflow_transaction_id, outflow_transaction_id), so when two syncs of the same family run at once the losing insert raises the unique violation. On PostgreSQL a failed statement aborts the whole surrounding transaction. Rescuing the Ruby exception does not clear that state, so the next update! raises PG::InFailedSqlTransaction and every following candidate is dropped. Run the per-candidate insert in its own savepoint via Transfer.transaction(requires_new: true), extracted into a private find_or_create_transfer! helper. A lost race now rolls back only to the savepoint; the outer transaction stays healthy and the loop keeps matching. The same race surfacing through the uniqueness validation (RecordInvalid with :taken) is treated as already-created; any other validation failure is re-raised. Fixes #2471 * fix(transfers): only swallow the uniqueness race for the exact pair The rescue treated a :taken on inflow_transaction_id or outflow_transaction_id as proof that this candidate's transfer was created. But the uniqueness validations are per-column, so two same-amount candidates racing (one committing (inflow, outflow_a) while another tries (inflow, outflow_b)) raise :taken on inflow_transaction_id even though no Transfer exists for (inflow, outflow_b). The caller then marked outflow_b as matched with no Transfer behind it. Confirm the exact (inflow_transaction_id, outflow_transaction_id) row exists before accepting the race; otherwise return nil and skip the candidate. Non-:taken validation failures still re-raise. The RecordNotUnique path (composite index) already implies the exact pair — it now returns that row for the same reason. * test(transfers): assert matching continues past a skipped collision The concurrent-race test had no surviving candidate, so a regression that stopped processing after the skipped collision would still pass. Add a second, non-conflicting candidate and assert its transfer is created and both entries are marked. * test(transfers): pass insert! attributes as an explicit hash Ruby 3 treats insert!(inflow_transaction_id: ..., outflow_transaction_id: ...) as keyword arguments, so ActiveRecord's insert!(attributes) got zero positional args and raised ArgumentError (given 0, expected 1). Wrap the attributes in { } so they are the positional attributes hash. --------- Co-authored-by: agentloop <agentloop@localhost> Co-authored-by: pro3958 <pro3958@users.noreply.github.com> |