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
This commit is contained in:
parent
eb31455e60
commit
d55c3f41df
|
|
@ -37,7 +37,7 @@ class StartCategorizationBackfill
|
|||
$jobId = (string) Str::uuid();
|
||||
|
||||
Cache::put(
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId),
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId),
|
||||
['status' => 'pending', 'processed' => 0, 'total' => $total, 'applied' => 0],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ namespace App\Http\Controllers\Ai;
|
|||
use App\Http\Controllers\Controller;
|
||||
use App\Jobs\CategorizeUncategorizedTransactionsJob;
|
||||
use Illuminate\Http\JsonResponse;
|
||||
use Illuminate\Http\Request;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
|
||||
class CategorizationController extends Controller
|
||||
|
|
@ -12,9 +13,9 @@ class CategorizationController extends Controller
|
|||
/**
|
||||
* Return current progress for a consent-triggered categorization backfill.
|
||||
*/
|
||||
public function status(string $jobId): JsonResponse
|
||||
public function status(Request $request, string $jobId): JsonResponse
|
||||
{
|
||||
$progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($request->user()->id, $jobId));
|
||||
|
||||
if ($progress === null) {
|
||||
return response()->json(['message' => 'Job not found.'], 404);
|
||||
|
|
|
|||
|
|
@ -47,7 +47,7 @@ class ReEvaluateTransactionRulesController extends Controller
|
|||
|
||||
// Set initial pending state so the first poll returns something meaningful
|
||||
Cache::put(
|
||||
ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId),
|
||||
ReEvaluateTransactionRulesJob::cacheKeyForJobId($user->id, $jobId),
|
||||
['status' => 'pending', 'processed' => 0, 'total' => 0, 'updated' => 0],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
|
@ -64,7 +64,7 @@ class ReEvaluateTransactionRulesController extends Controller
|
|||
*/
|
||||
public function status(Request $request, string $jobId): JsonResponse
|
||||
{
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId);
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($request->user()->id, $jobId);
|
||||
$progress = Cache::get($cacheKey);
|
||||
|
||||
if ($progress === null) {
|
||||
|
|
|
|||
|
|
@ -103,7 +103,7 @@ class AutomationRuleApplicationController extends Controller
|
|||
->with(['account.bank', 'category', 'labels'])
|
||||
->get();
|
||||
|
||||
$changed = $service->applyRuleActionsToTransactions($transactions, $automationRule);
|
||||
$changed = $service->applyRuleActionsToTransactions($transactions, $automationRule, $onlyUncategorized);
|
||||
|
||||
$applied = $transactions->count();
|
||||
|
||||
|
|
@ -121,12 +121,12 @@ class AutomationRuleApplicationController extends Controller
|
|||
$jobId = (string) Str::uuid();
|
||||
|
||||
Cache::put(
|
||||
ApplySingleAutomationRuleJob::cacheKeyForJobId($jobId),
|
||||
ApplySingleAutomationRuleJob::cacheKeyForJobId($automationRule->user_id, $jobId),
|
||||
['status' => 'pending', 'processed' => 0, 'total' => $total, 'applied' => 0, 'updated' => 0],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
||||
ApplySingleAutomationRuleJob::dispatch($automationRule, $jobId, $matchingIds);
|
||||
ApplySingleAutomationRuleJob::dispatch($automationRule, $jobId, $matchingIds, $onlyUncategorized);
|
||||
|
||||
$this->forgetMatchesCache($automationRule, $onlyUncategorized);
|
||||
|
||||
|
|
@ -141,7 +141,7 @@ class AutomationRuleApplicationController extends Controller
|
|||
*/
|
||||
public function status(Request $request, string $jobId): JsonResponse
|
||||
{
|
||||
$progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($request->user()->id, $jobId));
|
||||
|
||||
if ($progress === null) {
|
||||
return response()->json(['message' => 'Job not found.'], 404);
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ class ApplySingleAutomationRuleJob implements ShouldQueue
|
|||
public AutomationRule $rule,
|
||||
public string $jobId,
|
||||
public array $transactionIds,
|
||||
public bool $onlyUncategorized = false,
|
||||
) {}
|
||||
|
||||
public function handle(AutomationRuleService $service): void
|
||||
|
|
@ -46,7 +47,7 @@ class ApplySingleAutomationRuleJob implements ShouldQueue
|
|||
->whereNull('description_iv')
|
||||
->with(['account.bank', 'category', 'labels'])
|
||||
->chunkById(100, function ($transactions) use ($service, $rule, $total, &$processed, &$applied, &$changed) {
|
||||
$changed += $service->applyRuleActionsToTransactions($transactions, $rule);
|
||||
$changed += $service->applyRuleActionsToTransactions($transactions, $rule, $this->onlyUncategorized);
|
||||
$applied += $transactions->count();
|
||||
$processed += $transactions->count();
|
||||
|
||||
|
|
@ -69,14 +70,14 @@ class ApplySingleAutomationRuleJob implements ShouldQueue
|
|||
);
|
||||
}
|
||||
|
||||
public static function cacheKeyForJobId(string $jobId): string
|
||||
public static function cacheKeyForJobId(string $userId, string $jobId): string
|
||||
{
|
||||
return "apply_automation_rule_job_{$jobId}";
|
||||
return "apply_automation_rule_job_{$userId}_{$jobId}";
|
||||
}
|
||||
|
||||
private function cacheKey(): string
|
||||
{
|
||||
return self::cacheKeyForJobId($this->jobId);
|
||||
return self::cacheKeyForJobId($this->rule->user_id, $this->jobId);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -41,9 +41,9 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue
|
|||
return (string) config('ai_categorization.queue');
|
||||
}
|
||||
|
||||
public static function cacheKeyForJobId(string $jobId): string
|
||||
public static function cacheKeyForJobId(string $userId, string $jobId): string
|
||||
{
|
||||
return "categorize_transactions_job_{$jobId}";
|
||||
return "categorize_transactions_job_{$userId}_{$jobId}";
|
||||
}
|
||||
|
||||
public function handle(AiCategorizationGate $gate, AiCategorizer $categorizer): void
|
||||
|
|
@ -68,7 +68,7 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue
|
|||
*/
|
||||
public function failed(?Throwable $exception): void
|
||||
{
|
||||
$progress = Cache::get(self::cacheKeyForJobId($this->jobId), [
|
||||
$progress = Cache::get(self::cacheKeyForJobId($this->user->id, $this->jobId), [
|
||||
'processed' => 0,
|
||||
'total' => 0,
|
||||
'applied' => 0,
|
||||
|
|
@ -87,7 +87,7 @@ class CategorizeUncategorizedTransactionsJob implements ShouldQueue
|
|||
*/
|
||||
private function updateProgress(string $status, int $processed, int $total, int $applied): void
|
||||
{
|
||||
Cache::put(self::cacheKeyForJobId($this->jobId), [
|
||||
Cache::put(self::cacheKeyForJobId($this->user->id, $this->jobId), [
|
||||
'status' => $status,
|
||||
'processed' => $processed,
|
||||
'total' => $total,
|
||||
|
|
|
|||
|
|
@ -83,14 +83,14 @@ class ReEvaluateTransactionRulesJob implements ShouldQueue
|
|||
);
|
||||
}
|
||||
|
||||
public static function cacheKeyForJobId(string $jobId): string
|
||||
public static function cacheKeyForJobId(string $userId, string $jobId): string
|
||||
{
|
||||
return "re_evaluate_rules_job_{$jobId}";
|
||||
return "re_evaluate_rules_job_{$userId}_{$jobId}";
|
||||
}
|
||||
|
||||
private function cacheKey(): string
|
||||
{
|
||||
return self::cacheKeyForJobId($this->jobId);
|
||||
return self::cacheKeyForJobId($this->user->id, $this->jobId);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -13,17 +13,25 @@ use JWadhams\JsonLogic;
|
|||
|
||||
class AutomationRuleService
|
||||
{
|
||||
/**
|
||||
* Per-user rule cache, memoized for the lifetime of this service instance.
|
||||
*
|
||||
* Bulk re-evaluation calls applyRules() once per transaction; without this
|
||||
* every transaction re-queried the user's full rule set (an N+1). A service
|
||||
* instance is short-lived (resolved fresh per job/request), so rules created
|
||||
* after it is built are never seen mid-run — which is the intended behaviour.
|
||||
*
|
||||
* @var array<string, EloquentCollection<int, AutomationRule>>
|
||||
*/
|
||||
private array $rulesByUser = [];
|
||||
|
||||
public function applyRules(Transaction $transaction): void
|
||||
{
|
||||
if ($transaction->description_iv !== null) {
|
||||
return;
|
||||
}
|
||||
|
||||
$rules = AutomationRule::query()
|
||||
->where('user_id', $transaction->user_id)
|
||||
->with('labels')
|
||||
->orderBy('priority')
|
||||
->get();
|
||||
$rules = $this->rulesForUser($transaction->user_id);
|
||||
|
||||
if ($rules->isEmpty()) {
|
||||
return;
|
||||
|
|
@ -61,9 +69,17 @@ class AutomationRuleService
|
|||
}
|
||||
|
||||
/**
|
||||
* Apply a rule's actions to a set of transactions.
|
||||
*
|
||||
* When $onlyUncategorized is true the set is re-filtered here, at apply
|
||||
* time, rather than trusting the caller's (possibly stale) match snapshot.
|
||||
* A transaction that was categorized after the snapshot was taken — by the
|
||||
* user, another rule, or the AI backfill — is skipped so the rule never
|
||||
* silently overwrites a category the user did not ask it to touch.
|
||||
*
|
||||
* @param EloquentCollection<int, Transaction> $transactions
|
||||
*/
|
||||
public function applyRuleActionsToTransactions(EloquentCollection $transactions, AutomationRule $rule): int
|
||||
public function applyRuleActionsToTransactions(EloquentCollection $transactions, AutomationRule $rule, bool $onlyUncategorized = false): int
|
||||
{
|
||||
if ($transactions->isEmpty()) {
|
||||
return 0;
|
||||
|
|
@ -72,6 +88,16 @@ class AutomationRuleService
|
|||
$rule->loadMissing('labels');
|
||||
$transactions->loadMissing('labels');
|
||||
|
||||
if ($onlyUncategorized) {
|
||||
$transactions = $transactions->reject(
|
||||
fn (Transaction $transaction): bool => $this->shouldSkipForOnlyUncategorized($rule, $transaction),
|
||||
);
|
||||
|
||||
if ($transactions->isEmpty()) {
|
||||
return 0;
|
||||
}
|
||||
}
|
||||
|
||||
$changedTransactionIds = [];
|
||||
|
||||
if ($rule->action_category_id !== null) {
|
||||
|
|
@ -221,6 +247,18 @@ class AutomationRuleService
|
|||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @return EloquentCollection<int, AutomationRule>
|
||||
*/
|
||||
private function rulesForUser(string $userId): EloquentCollection
|
||||
{
|
||||
return $this->rulesByUser[$userId] ??= AutomationRule::query()
|
||||
->where('user_id', $userId)
|
||||
->with('labels')
|
||||
->orderBy('priority')
|
||||
->get();
|
||||
}
|
||||
|
||||
/**
|
||||
* @param Collection<int, AutomationRule> $rules
|
||||
* @param array<string, mixed> $transactionData
|
||||
|
|
|
|||
|
|
@ -178,6 +178,7 @@ export function ApplyAutomationRuleFlow({
|
|||
const pollStatus = useCallback(
|
||||
async (jobId: string) => {
|
||||
const url = statusRoute(jobId).url;
|
||||
let pendingTicks = 0;
|
||||
const poll = async (): Promise<void> => {
|
||||
const res = await fetch(url, {
|
||||
headers: { Accept: 'application/json' },
|
||||
|
|
@ -202,6 +203,13 @@ export function ApplyAutomationRuleFlow({
|
|||
setApplying(false);
|
||||
return;
|
||||
}
|
||||
// The job never started (e.g. no queue worker running) — give
|
||||
// up instead of polling forever.
|
||||
if (data.status === 'pending' && ++pendingTicks > 30) {
|
||||
toast.error(__('Failed to apply rule to transactions.'));
|
||||
setApplying(false);
|
||||
return;
|
||||
}
|
||||
setTimeout(() => {
|
||||
void poll();
|
||||
}, 1000);
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@ export function useReEvaluateAllTransactions() {
|
|||
const jobId = bulkResponse.data.job_id;
|
||||
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
let pendingTicks = 0;
|
||||
const poll = async () => {
|
||||
try {
|
||||
const statusResponse = await axios.get<{
|
||||
|
|
@ -47,6 +48,13 @@ export function useReEvaluateAllTransactions() {
|
|||
resolve();
|
||||
} else if (status === 'failed') {
|
||||
reject(new Error('Job failed'));
|
||||
} else if (
|
||||
status === 'pending' &&
|
||||
++pendingTicks > 30
|
||||
) {
|
||||
// The job never started (e.g. no queue worker
|
||||
// running) — give up instead of polling forever.
|
||||
reject(new Error('Job did not start'));
|
||||
} else {
|
||||
setTimeout(poll, 1000);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -940,6 +940,7 @@ export default function Transactions({
|
|||
const jobId = bulkResponse.data.job_id;
|
||||
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
let pendingTicks = 0;
|
||||
const poll = async () => {
|
||||
try {
|
||||
const statusResponse = await axios.get<{
|
||||
|
|
@ -969,6 +970,13 @@ export default function Transactions({
|
|||
resolve();
|
||||
} else if (status === 'failed') {
|
||||
reject(new Error('Job failed'));
|
||||
} else if (
|
||||
status === 'pending' &&
|
||||
++pendingTicks > 30
|
||||
) {
|
||||
// The job never started (e.g. no queue worker
|
||||
// running) — give up instead of polling forever.
|
||||
reject(new Error('Job did not start'));
|
||||
} else {
|
||||
setTimeout(poll, 1000);
|
||||
}
|
||||
|
|
@ -1472,7 +1480,7 @@ export default function Transactions({
|
|||
handleDismissAiConsent
|
||||
}
|
||||
disabled={aiConsentSaving}
|
||||
className="opacity-20 hover:opacity-100 transition-all duration-300"
|
||||
className="opacity-20 transition-all duration-300 hover:opacity-100"
|
||||
variant="ghost"
|
||||
size="icon"
|
||||
aria-label={__('Dismiss')}
|
||||
|
|
|
|||
|
|
@ -85,7 +85,7 @@ it('does not dispatch a backfill when AI categorization is disabled', function (
|
|||
it('returns categorization progress from the status endpoint', function () {
|
||||
$user = User::factory()->create();
|
||||
Cache::put(
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId('job-123'),
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, 'job-123'),
|
||||
['status' => 'processing', 'processed' => 1, 'total' => 4, 'applied' => 1],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
|
@ -102,6 +102,20 @@ it('returns 404 from the status endpoint for an unknown job', function () {
|
|||
->assertNotFound();
|
||||
});
|
||||
|
||||
it('does not leak another user\'s categorization progress', function () {
|
||||
$owner = User::factory()->create();
|
||||
Cache::put(
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($owner->id, 'job-123'),
|
||||
['status' => 'processing', 'processed' => 1, 'total' => 4, 'applied' => 1],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
||||
$otherUser = User::factory()->create();
|
||||
|
||||
actingAs($otherUser)->getJson(route('ai.categorization.status', 'job-123'))
|
||||
->assertNotFound();
|
||||
});
|
||||
|
||||
it('records progress while categorizing the uncategorized transactions', function () {
|
||||
$user = User::factory()->create();
|
||||
$user->recordAiConsent();
|
||||
|
|
@ -139,7 +153,7 @@ it('records progress while categorizing the uncategorized transactions', functio
|
|||
$jobId = 'job-run-1';
|
||||
app()->call([new CategorizeUncategorizedTransactionsJob($user, $jobId), 'handle']);
|
||||
|
||||
$progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId));
|
||||
|
||||
expect($progress['status'])->toBe('done')
|
||||
->and($progress['total'])->toBe(2)
|
||||
|
|
@ -151,7 +165,7 @@ it('marks the cache as failed and preserves counts when the job fails', function
|
|||
$user = User::factory()->create();
|
||||
$jobId = 'failed-job';
|
||||
Cache::put(
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId),
|
||||
CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId),
|
||||
['status' => 'processing', 'processed' => 3, 'total' => 10, 'applied' => 2],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
|
@ -159,7 +173,7 @@ it('marks the cache as failed and preserves counts when the job fails', function
|
|||
(new CategorizeUncategorizedTransactionsJob($user, $jobId))
|
||||
->failed(new RuntimeException('boom'));
|
||||
|
||||
$progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(CategorizeUncategorizedTransactionsJob::cacheKeyForJobId($user->id, $jobId));
|
||||
|
||||
expect($progress['status'])->toBe('failed')
|
||||
->and($progress['processed'])->toBe(3)
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ use App\Models\Category;
|
|||
use App\Models\Label;
|
||||
use App\Models\Transaction;
|
||||
use App\Models\User;
|
||||
use App\Services\AutomationRuleService;
|
||||
use Illuminate\Database\Eloquent\Factories\Sequence;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
|
@ -289,6 +290,56 @@ test('apply endpoint queues a job when matches exceed threshold', function () {
|
|||
Queue::assertPushed(ApplySingleAutomationRuleJob::class);
|
||||
});
|
||||
|
||||
test('apply job applies the rule and records done progress', function () {
|
||||
$t1 = Transaction::factory()->enableBanking()->create([
|
||||
'user_id' => $this->user->id,
|
||||
'account_id' => $this->account->id,
|
||||
'description' => 'Grocery Store',
|
||||
'amount' => -5000,
|
||||
'category_id' => null,
|
||||
]);
|
||||
$t2 = Transaction::factory()->enableBanking()->create([
|
||||
'user_id' => $this->user->id,
|
||||
'account_id' => $this->account->id,
|
||||
'description' => 'Grocery Store',
|
||||
'amount' => -3000,
|
||||
'category_id' => null,
|
||||
]);
|
||||
|
||||
$jobId = 'apply-run-1';
|
||||
(new ApplySingleAutomationRuleJob($this->rule, $jobId, [$t1->id, $t2->id]))
|
||||
->handle(app(AutomationRuleService::class));
|
||||
|
||||
expect($t1->fresh()->category_id)->toBe($this->category->id)
|
||||
->and($t2->fresh()->category_id)->toBe($this->category->id);
|
||||
|
||||
$progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
expect($progress['status'])->toBe('done')
|
||||
->and($progress['total'])->toBe(2)
|
||||
->and($progress['processed'])->toBe(2)
|
||||
->and($progress['applied'])->toBe(2)
|
||||
->and($progress['updated'])->toBe(2);
|
||||
});
|
||||
|
||||
test('apply job marks cache as failed and preserves counts', function () {
|
||||
$jobId = 'apply-failed';
|
||||
Cache::put(
|
||||
ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId),
|
||||
['status' => 'processing', 'processed' => 3, 'total' => 10, 'applied' => 3, 'updated' => 2],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
||||
(new ApplySingleAutomationRuleJob($this->rule, $jobId, ['x', 'y']))
|
||||
->failed(new RuntimeException('boom'));
|
||||
|
||||
$progress = Cache::get(ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
expect($progress['status'])->toBe('failed')
|
||||
->and($progress['processed'])->toBe(3)
|
||||
->and($progress['total'])->toBe(10)
|
||||
->and($progress['applied'])->toBe(3)
|
||||
->and($progress['updated'])->toBe(2);
|
||||
});
|
||||
|
||||
test('apply endpoint returns done with zero matches when no transactions match', function () {
|
||||
Transaction::factory()->enableBanking()->create([
|
||||
'user_id' => $this->user->id,
|
||||
|
|
@ -318,6 +369,59 @@ test('cannot apply rule belonging to another user', function () {
|
|||
->assertForbidden();
|
||||
});
|
||||
|
||||
test('cannot poll apply job status belonging to another user', function () {
|
||||
$jobId = 'apply-job-1';
|
||||
Cache::put(
|
||||
ApplySingleAutomationRuleJob::cacheKeyForJobId($this->user->id, $jobId),
|
||||
['status' => 'processing', 'processed' => 1, 'total' => 4],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
||||
$this->actingAs($this->user)
|
||||
->getJson(route('automation-rules.apply.status', $jobId))
|
||||
->assertOk()
|
||||
->assertJsonPath('status', 'processing');
|
||||
|
||||
$otherUser = User::factory()->onboarded()->create();
|
||||
|
||||
$this->actingAs($otherUser)
|
||||
->getJson(route('automation-rules.apply.status', $jobId))
|
||||
->assertNotFound();
|
||||
});
|
||||
|
||||
test('apply re-checks only_uncategorized at apply time and skips newly categorized transactions', function () {
|
||||
$otherCategory = Category::factory()->create(['user_id' => $this->user->id]);
|
||||
|
||||
// Both matched the rule when the snapshot was taken, but this one has since
|
||||
// been categorized (by the user, another rule, or the AI backfill).
|
||||
$nowCategorized = Transaction::factory()->enableBanking()->create([
|
||||
'user_id' => $this->user->id,
|
||||
'account_id' => $this->account->id,
|
||||
'description' => 'Grocery Store',
|
||||
'amount' => -5000,
|
||||
'category_id' => $otherCategory->id,
|
||||
]);
|
||||
|
||||
$stillUncategorized = Transaction::factory()->enableBanking()->create([
|
||||
'user_id' => $this->user->id,
|
||||
'account_id' => $this->account->id,
|
||||
'description' => 'Grocery Store',
|
||||
'amount' => -3000,
|
||||
'category_id' => null,
|
||||
]);
|
||||
|
||||
$transactions = Transaction::query()
|
||||
->whereIn('id', [$nowCategorized->id, $stillUncategorized->id])
|
||||
->get();
|
||||
|
||||
$changed = app(AutomationRuleService::class)
|
||||
->applyRuleActionsToTransactions($transactions, $this->rule, onlyUncategorized: true);
|
||||
|
||||
expect($changed)->toBe(1);
|
||||
expect($nowCategorized->fresh()->category_id)->toBe($otherCategory->id);
|
||||
expect($stillUncategorized->fresh()->category_id)->toBe($this->category->id);
|
||||
});
|
||||
|
||||
test('label-only rule applies when only_uncategorized is true', function () {
|
||||
$labelOnlyRule = AutomationRule::factory()->create([
|
||||
'user_id' => $this->user->id,
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ use App\Models\Transaction;
|
|||
use App\Models\User;
|
||||
use App\Services\AutomationRuleService;
|
||||
use Illuminate\Support\Facades\Cache;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
use Illuminate\Support\Facades\Queue;
|
||||
|
||||
beforeEach(function () {
|
||||
|
|
@ -213,7 +214,7 @@ test('bulk endpoint sets initial pending status in cache', function () {
|
|||
->postJson(route('transactions.re-evaluate-rules.bulk'));
|
||||
|
||||
$jobId = $response->json('job_id');
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId);
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId);
|
||||
|
||||
expect(Cache::get($cacheKey))->toMatchArray(['status' => 'pending']);
|
||||
});
|
||||
|
|
@ -224,7 +225,7 @@ test('bulk endpoint sets initial pending status in cache', function () {
|
|||
|
||||
test('status endpoint returns progress from cache', function () {
|
||||
$jobId = 'test-job-id';
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId);
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId);
|
||||
|
||||
Cache::put($cacheKey, [
|
||||
'status' => 'processing',
|
||||
|
|
@ -250,6 +251,19 @@ test('status endpoint returns 404 for unknown job', function () {
|
|||
->assertNotFound();
|
||||
});
|
||||
|
||||
test('status endpoint does not leak another user\'s job progress', function () {
|
||||
$jobId = 'owned-by-someone-else';
|
||||
$cacheKey = ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId);
|
||||
|
||||
Cache::put($cacheKey, ['status' => 'processing', 'processed' => 5, 'total' => 50, 'updated' => 1], now()->addHour());
|
||||
|
||||
$otherUser = User::factory()->onboarded()->create();
|
||||
|
||||
$this->actingAs($otherUser)
|
||||
->getJson(route('transactions.re-evaluate-rules.status', $jobId))
|
||||
->assertNotFound();
|
||||
});
|
||||
|
||||
// ──────────────────────────────────────────────
|
||||
// Job execution
|
||||
// ──────────────────────────────────────────────
|
||||
|
|
@ -287,12 +301,60 @@ test('job applies rules to non-encrypted transactions and tracks progress', func
|
|||
expect($matchingTransaction->fresh()->category_id)->toBe($this->category->id);
|
||||
expect($nonMatchingTransaction->fresh()->category_id)->toBeNull();
|
||||
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
expect($progress['status'])->toBe('done');
|
||||
expect($progress['processed'])->toBe(2);
|
||||
expect($progress['updated'])->toBe(1);
|
||||
});
|
||||
|
||||
test('job queries automation rules once regardless of transaction count', function () {
|
||||
// Create transactions BEFORE the rule so the creation listener has no rules to apply
|
||||
Transaction::factory()->enableBanking()->count(5)->create([
|
||||
'user_id' => $this->user->id,
|
||||
'account_id' => $this->account->id,
|
||||
'description' => 'Grocery Store',
|
||||
'amount' => -5000,
|
||||
'category_id' => null,
|
||||
]);
|
||||
|
||||
AutomationRule::factory()->create([
|
||||
'user_id' => $this->user->id,
|
||||
'priority' => 1,
|
||||
'rules_json' => ['in' => ['grocery', ['var' => 'description']]],
|
||||
'action_category_id' => $this->category->id,
|
||||
]);
|
||||
|
||||
$ruleQueries = 0;
|
||||
DB::listen(function ($query) use (&$ruleQueries): void {
|
||||
if (str_contains($query->sql, 'automation_rules')) {
|
||||
$ruleQueries++;
|
||||
}
|
||||
});
|
||||
|
||||
$jobId = 'test-job-'.uniqid();
|
||||
(new ReEvaluateTransactionRulesJob($this->user, $jobId))->handle(app(AutomationRuleService::class));
|
||||
|
||||
expect($ruleQueries)->toBe(1);
|
||||
});
|
||||
|
||||
test('job marks cache as failed and preserves counts', function () {
|
||||
$jobId = 'reeval-failed';
|
||||
Cache::put(
|
||||
ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId),
|
||||
['status' => 'processing', 'processed' => 4, 'total' => 20, 'updated' => 2],
|
||||
now()->addHour(),
|
||||
);
|
||||
|
||||
(new ReEvaluateTransactionRulesJob($this->user, $jobId))
|
||||
->failed(new RuntimeException('boom'));
|
||||
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
expect($progress['status'])->toBe('failed')
|
||||
->and($progress['processed'])->toBe(4)
|
||||
->and($progress['total'])->toBe(20)
|
||||
->and($progress['updated'])->toBe(2);
|
||||
});
|
||||
|
||||
test('job skips encrypted transactions', function () {
|
||||
AutomationRule::factory()->create([
|
||||
'user_id' => $this->user->id,
|
||||
|
|
@ -314,7 +376,7 @@ test('job skips encrypted transactions', function () {
|
|||
$job = new ReEvaluateTransactionRulesJob($this->user, $jobId);
|
||||
$job->handle(app(AutomationRuleService::class));
|
||||
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
// 0 processed because the encrypted transaction is excluded from the query
|
||||
expect($progress['processed'])->toBe(0);
|
||||
expect($progress['updated'])->toBe(0);
|
||||
|
|
@ -352,7 +414,7 @@ test('job only processes provided transaction_ids', function () {
|
|||
expect($t1->fresh()->category_id)->toBe($this->category->id);
|
||||
expect($t2->fresh()->category_id)->toBeNull();
|
||||
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
expect($progress['processed'])->toBe(1);
|
||||
});
|
||||
|
||||
|
|
@ -412,7 +474,7 @@ test('job applies rules to transactions matching filters', function () {
|
|||
expect($matchingTransaction->fresh()->category_id)->toBe($this->category->id);
|
||||
expect($outsideRangeTransaction->fresh()->category_id)->toBeNull();
|
||||
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($jobId));
|
||||
$progress = Cache::get(ReEvaluateTransactionRulesJob::cacheKeyForJobId($this->user->id, $jobId));
|
||||
expect($progress['status'])->toBe('done');
|
||||
expect($progress['processed'])->toBe(1);
|
||||
expect($progress['updated'])->toBe(1);
|
||||
|
|
|
|||
Loading…
Reference in New Issue