Commit Graph

7 Commits

Author SHA1 Message Date
Víctor Falcón 2f00a56272
fix(budgets): re-derive budget membership when labels are attached without a model event (#787)
## Problem

Since #781 a catch-all budget ("Not budgeted") yields to any budget that
tracks a
transaction by category **or** label in a period covering its date.
Reassignment is
driven by `TransactionCreated` / `TransactionUpdated`, whose listener is
`AssignTransactionToBudget`.

Pivot writes and query-builder mass updates fire no model event. So
every path that
attaches a label without a real `save()` left the transaction assigned
to whatever it
was before — typically stuck in the catch-all and missing from the
budget that tracks
its label.

In production, 44 expenses carrying one label never entered their label
budget:
`label_transaction.created_at` is ~10 minutes later than
`transactions.updated_at` on
each of them, i.e. the listener ran before the label existed and nothing
ran after.

## The paths that were broken

| Path | Write | Event |
|---|---|---|
| `AutomationRuleService::applyActions` | `saveQuietly()` +
`syncWithoutDetaching` | none |
| `AutomationRuleService::applyRuleActionsToTransactions` |
`LabelTransaction::insertOrIgnore` + mass category `update()` | none |
| `Mcp\Tools\LabelTransaction` | `syncWithoutDetaching` / `detach` |
none |
| `TransactionController::bulkUpdate` | mass category `update()` +
`labels()->sync()` | none |

The last one was found during review and is the surface users hit most —
the
bulk-actions bar in the transactions table. Its `syncLabels()` helper
does
`sync()` then `save()`, but those models are loaded before the mass
update and
nothing dirties them, so `Model::save()` skips `performUpdate()` and
fires nothing.
The docblock claiming the `save()` bumped `updated_at` was wrong and is
corrected.

## The fix

A dedicated `ReassignTransactionsToBudgets` job, dispatched from each
path, rather
than re-broadcasting `TransactionUpdated` — which would also re-run the
automation
rules that dispatched it. The job takes ids, not models, so a retry
never works from
a stale payload.

Batching, so no path queues one job per row:

- the bulk rule apply dispatches per batch of 500 ids (the AI suggestion
path is the
  one caller that can hand it an unbounded list)
- `ReEvaluateTransactionRulesJob` loops `applyRules()` over the user's
whole history,
so `applyRules()` takes `reassignBudgets` and that job batches one job
per chunk
- `TransactionController::bulkUpdate` dispatches once for the whole
selection

Notifications are suppressed (`notify: false`) everywhere the change is
an
administrative edit rather than new spending: bulk rule applies,
re-evaluating rules
over history, the bulk-actions bar, and MCP relabelling. Otherwise
removing a label
would announce a months-old expense as new in the catch-all budget. The
single
transaction matched by a rule at creation time keeps notifications on.
Suppressing
them leaves `close_to_limit_notified` / `over_limit_notified` unclaimed,
so the next
genuine transaction into that budget still alerts.

A note never changes which budget counts a transaction, so a note-only
rule no longer
earns a reassignment on either path.

`applyRuleActionsToTransactions` was doing category, note and label work
inline; the
three branches are extracted so the added dispatch keeps the method
under the
repo's complexity-10 gate.

## Deploy step

This stops the state from going stale again, it does not repair what is
already
stale. After deploy:

```
php artisan budgets:reassign-labeled [--user=<email>] [--dry-run]
```

## Tests

`tests/Feature/LabelBudgetReassignmentTest.php` covers all four paths
plus the
re-evaluate batching, each asserting the transaction actually leaves the
catch-all
and lands in the label budget. All seven fail on `main` and pass here.
`AutomationRuleApplicationTest` now pins that the bulk apply queues
exactly one
reassignment job and that it is silent.

## QA

Verified in the browser against the running app with a throwaway
account: a
catch-all budget and a label budget over the same period, four expenses
starting in
the catch-all.

- Selecting all four in the transactions table and applying the label
moves
**Not budgeted $540.00 → $0.00** and **Miami 26 $0.00 → $540.00**,
confirmed in
  `budget_transactions`.
- "Remove all labels" hands them back to the catch-all.

## Demo

<!-- PLACEHOLDER: drag the QA video here -->


https://github.com/user-attachments/assets/5a135257-42cf-42a7-a66a-3f76bf773e0c



## Follow-ups (not in this PR)

- Soft-deleting a label leaves its budget still counting the
transactions; the
  catch-all never takes them back.
- `CategoryTree::deleteSubtree` mass-nulls `category_id` with no event,
so cascade
  category deletion leaves transactions in their old category budget.
- `BudgetService::create` only adds rows for a new budget's historical
transactions;
it never removes their catch-all rows, so creating a label budget next
to an
  existing catch-all double-counts until something else reassigns them.
2026-08-12 12:48:47 +02:00
Víctor Falcón d55c3f41df
fix(security): scope job-status endpoints to owner + feature-area fixes (#627)
## Summary

Starts from a security finding on the background-job status endpoints
and folds in the most important issues surfaced by a follow-up review of
the same feature area (categorization backfill, bulk rule re-evaluation,
automation-rule apply).

Each change is its own commit.

## Changes

### 1. `fix(security)` — scope job-status cache keys to the owning user
The categorization, bulk re-evaluation, and apply status endpoints
looked jobs up by a bare job UUID with no user scoping. Any
authenticated user who obtained another user's job id could poll its
progress payload. Cache keys now include the owning user's id, so a
status request keyed by the polling user's id resolves only that user's
own jobs — a mismatched owner falls through to the existing 404. No
ownership store or extra lookup. Cross-user isolation tests added for
all three endpoints.

### 2. `fix(automation-rules)` — re-check `only_uncategorized` at apply
time
The apply flow cached a match snapshot for up to 15 min (keyed only by
`rule.updated_at`) and applied the rule's category to every id in it
without re-checking eligibility. A transaction categorized *after* the
snapshot (by the user, a sibling rule, or a concurrent AI backfill) was
silently re-categorized and stamped `category_source=Rule`. Now
re-filtered through `shouldSkipForOnlyUncategorized()` at apply time;
skipped rows are no longer counted as changed.

### 3. `perf(automation-rules)` — memoize rule set per user in
`applyRules()`
Bulk re-evaluation calls `applyRules()` once per transaction, and each
call re-queried the user's whole rule set + labels — an N+1 scaling with
transaction count (the sibling apply job already loaded rules once).
Memoized per user for the service instance's lifetime (resolved fresh
per job, so rules created mid-run are intentionally not seen).
Query-count test asserts one `automation_rules` query regardless of
transaction count.

### 4. `fix(transactions)` — stop polling when the job never starts
The re-evaluate and apply pollers rescheduled on any non-terminal
status, including `pending`. If the queue worker is down the job never
runs, `failed()` never fires, and the client polled for the full
hour-long TTL with a stuck spinner. Now gives up after 30 consecutive
`pending` ticks (~30s), mirroring the guard the AI-categorization poller
already has. Long `processing` runs are unaffected.

### 5. `test(automation-rules)` — cover apply job execution and failure
branches
`ApplySingleAutomationRuleJob` was only asserted to be *pushed*; its
`handle()` body and `failed()` branch had no coverage, and
`ReEvaluateTransactionRulesJob::failed()` was untested too. Added direct
`handle()`/`failed()` tests pinning the progress-cache payloads.

## Deferred follow-ups (surfaced by review, not in this PR)

- **Consolidate the three frontend pollers onto `usePollJobStatus`** —
they hand-roll `setTimeout` loops and don't tear down on unmount (can
`setState`/`onClose` after the dialog closes). The hook already exists
and is unmount-safe; routing all three through it would also make the
`pending` cap unit-testable. The cap in change #4 currently mirrors the
already-shipped AI poller and is not separately unit-tested.
- **`noteAlreadyPresent()` uses a substring match**
(`AutomationRuleService`) — a rule note that is a substring of an
existing note is silently not appended. Should compare note lines
exactly.
- **No dedup/lock on concurrent apply/re-evaluate jobs** — note appends
are a non-atomic read-then-write, so two concurrent jobs can duplicate a
note. Category/label writes are already idempotent.
- **Transient 404 → false "failed"** — a mid-run cache
eviction/TTL-expiry (or the deploy window of change #1, where an
in-flight job's pre-deploy key is orphaned) makes the poller surface a
false failure. Retry a few times on transient errors before giving up.
- **`matches()` pagination** advances `next_offset` by fetched rows, not
page-window size — deleted ids can stall infinite scroll before reaching
`total`.
- **"Apply to N" count / encrypted-skip counts** can be stale or
under-report vs what's actually processed.
- **Job-trait convention drift** between the three jobs (modern
`Queueable` vs legacy trait stack).

## Testing
- `vendor/bin/pint --test` — pass
- `bun run format` / `bun run lint` — pass (one pre-existing unrelated
warning in `chart.tsx`)
- Affected Pest suites (apply, re-evaluate, evaluation, rule,
categorization, apply-rule-suggestions) — 94 tests pass
2026-07-03 14:49:32 +02:00
Víctor Falcón 10da06ed84
feat(transactions): add counterparty fields (#440)
## Summary
- add creditor/debtor fields to transactions with raw_data backfill
- store counterparties on bank sync and CSV/XLS imports
- add creditor/debtor filters plus hidden table columns

## Tests
- php artisan test --compact tests/Feature/LocalizationTest.php
tests/Feature/OpenBanking/TransactionSyncServiceTest.php
tests/Feature/TransactionFilterTest.php
- npm test -- resources/js/lib/file-parser.test.ts --run
- vendor/bin/pint --dirty --format agent

Note: `npm run types` still has pre-existing unrelated errors; no
creditor/debtor/import-related errors remained in filtered output.

## Screenshot
<img width="1241" height="676" alt="8odYWtFcvUM"
src="https://github.com/user-attachments/assets/55653485-d588-4beb-9e6a-5c7c81ba7cf8"
/>
2026-05-27 16:20:55 +02:00
Víctor Falcón 606093d311
fix: batch automation rule application (#435)
## Sentry
- Issue: PHP-LARAVEL-2C
- URL: https://whisper-money.sentry.io/issues/122336983/

## Root cause
Applying an automation rule to multiple transactions used
per-transaction `saveQuietly()` and `syncWithoutDetaching()`. Sentry
detected repeated transaction updates and pivot lookups/inserts on
`/settings/automation-rules/{automationRule}/apply`.

## Fix
- Added batched automation rule application for transaction collections.
- Bulk updates category changes in one query.
- Bulk inserts missing label pivots with `insertOrIgnore`.
- Reused batching from sync controller path and queued job chunks.

## Verification
- `vendor/bin/pint --dirty --format agent`
- `php artisan test --compact
tests/Feature/AutomationRuleApplicationTest.php`
2026-05-26 10:45:37 +02:00
Víctor Falcón 9772cfc37c
fix(automation): avoid skipping rule matches (#433)
## Summary
- replace ordered automation match scanning from chunkById to chunk so
sorted rows are not skipped
- make regression fixture deterministic across chunk boundaries

## Tests
- vendor/bin/pint --dirty --format agent
- php artisan test --compact
tests/Feature/AutomationRuleApplicationTest.php --filter='matches
endpoint avoids repeated relationship queries'
- php artisan test --compact
tests/Feature/AutomationRuleApplicationTest.php
2026-05-26 08:26:20 +02:00
Víctor Falcón fd67cf7c72
fix(automation): avoid rule preview n+1 (#431)
## Sentry issue
- PHP-LARAVEL-2F: https://whisper-money.sentry.io/issues/122581787/

## Root cause
- Automation rule match preview eagerly loaded account, bank, category,
and labels for every 500-transaction chunk, even for rules that only
inspect description fields.
- Sentry flagged repeated account/bank eager-load queries across chunks
as an N+1 pattern on
`/settings/automation-rules/{automationRule}/matches`.

## Fix
- Detect variables used by an automation rule and eager load only
relationships required for evaluation.
- Keep label eager loading only when label-only skip logic needs it.
- Avoid lazy loading unused relationships while preserving full data
shape for rule evaluation.
- Update the Sentry prompt to use the `sentry` CLI workflow.

## Verification
- `vendor/bin/pint --dirty --format agent`
- `php artisan test --compact
tests/Feature/AutomationRuleApplicationTest.php`
- `php artisan test --compact
tests/Feature/AutomationRuleEvaluationTest.php
tests/Feature/AutomationRuleApplicationTest.php`
2026-05-26 08:02:46 +02:00
Víctor Falcón 9d7a91dcd0
Apply automation rules to existing transactions (#413)
## Summary
- add apply-to-existing-transactions flow for automation rules
- preview matching transactions with uncategorized filter and infinite
scroll
- apply rule actions sync or via queued job with progress polling
- fix duplicate preview rows and repeat apply prompt when labels change

## Tests
- vendor/bin/pint --dirty --format agent
- npm test -- --run
resources/js/components/automation-rules/post-save-apply-rule-prompt.test.ts
resources/js/components/automation-rules/apply-automation-rule-flow.test.ts
- php artisan test --compact
tests/Feature/AutomationRuleApplicationTest.php

## Video

https://github.com/user-attachments/assets/e74f5e58-e582-4fb6-b6d4-2702398804b7
2026-05-22 08:36:18 +02:00