Skip to content

Commit 5b19a12

Browse files
os-zhuangclaude
andcommitted
feat(approvals): approver join table — exact pushdown for approver-filtered pagination (#1745)
sys_approval_approver holds one row per (pending request, approver identity); the service mirrors every pending_approvers change into it and clears rows when a request leaves pending. listRequests / countRequests resolve approver filters through the index and push status arrays down as $in, so pages and totals are correct at any table size — the 500-row bounded-scan residual is gone. rebuildApproverIndex() backfills idempotently at plugin start. Closes #1745 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 748e3eb commit 5b19a12

8 files changed

Lines changed: 414 additions & 57 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@objectstack/plugin-approvals": minor
3+
---
4+
5+
Approver join table — the #1745 follow-up that makes approver-filtered pagination exact. New `sys_approval_approver` object holds one row per (pending request, approver identity); the service mirrors every `pending_approvers` change into it (open / decide / recall / send-back / reassign / SLA-escalate) and clears the rows when a request leaves `pending`, so the table tracks the live work queue, not the append-only history. `listRequests` / `countRequests` now resolve approver filters through this index (`$in` on indexed equality instead of a per-row CSV scan) and push status arrays down as `$in` — every filter is engine-side, so the page window and totals are correct at any table size; the old 500-row bounded-scan residual is gone. `rebuildApproverIndex()` rebuilds the index from the CSV source of truth, and runs idempotently at plugin start to backfill rows written before the index existed.

packages/plugins/plugin-approvals/src/approval-service.test.ts

Lines changed: 117 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,7 @@ describe('ApprovalService (node era)', () => {
611611
expect(await svc.countRequests({ q: 'Deal 2' }, SYS)).toBe(1);
612612
});
613613

614-
it('listRequests: approver queries window in memory AFTER exact-match filtering', async () => {
614+
it('listRequests: approver queries resolve via the index and window engine-side', async () => {
615615
await openMany(4); // approver u9 on all
616616
await svc.openNodeRequest(openInput(['someone-else'], {
617617
recordId: 'oppX', record: { id: 'oppX', name: 'Other' },
@@ -622,6 +622,122 @@ describe('ApprovalService (node era)', () => {
622622
expect(await svc.countRequests({ approverId: 'u9' }, SYS)).toBe(4);
623623
});
624624

625+
it('listRequests/countRequests: status arrays push down as $in', async () => {
626+
await openMany(3);
627+
const all = await svc.listRequests({ status: 'pending' }, SYS);
628+
await svc.decideNode(all[0].id, { decision: 'approve', actorId: 'u9' }, SYS);
629+
await svc.decideNode(all[1].id, { decision: 'reject', actorId: 'u9' }, SYS);
630+
const done = await svc.listRequests({ status: ['approved', 'rejected'] }, SYS);
631+
expect(done.map(r => r.status).sort()).toEqual(['approved', 'rejected']);
632+
expect(await svc.countRequests({ status: ['approved', 'rejected'] }, SYS)).toBe(2);
633+
expect(await svc.countRequests({ status: ['recalled'] }, SYS)).toBe(0);
634+
});
635+
636+
// ── pending-approver index (#1745 join table) ───────────────────
637+
638+
const indexRows = () => (engine._tables['sys_approval_approver'] ?? [])
639+
.map(r => ({ request_id: r.request_id, approver: r.approver }));
640+
641+
it('openNodeRequest mirrors every approver identity into the index', async () => {
642+
const req = await svc.openNodeRequest(openInput(['u9', 'ada@example.com', 'role:finance']), CTX);
643+
expect(indexRows()).toEqual([
644+
{ request_id: req.id, approver: 'u9' },
645+
{ request_id: req.id, approver: 'ada@example.com' },
646+
{ request_id: req.id, approver: 'role:finance' },
647+
]);
648+
});
649+
650+
it('decide and recall clear the request\'s index rows', async () => {
651+
const a = await svc.openNodeRequest(openInput(['u9']), CTX);
652+
await svc.decideNode(a.id, { decision: 'approve', actorId: 'u9' }, SYS);
653+
expect(indexRows()).toHaveLength(0);
654+
655+
const b = await svc.openNodeRequest(openInput(['u9'], { recordId: 'opp2', record: { id: 'opp2' } }), CTX);
656+
await svc.recall(b.id, { actorId: 'u1' }, CTX);
657+
expect(indexRows()).toHaveLength(0);
658+
});
659+
660+
it('unanimous partial approval shrinks the index to the still-pending set', async () => {
661+
const req = await svc.openNodeRequest(openInput(['u1', 'u2'], {}, { behavior: 'unanimous' }), CTX);
662+
await svc.decideNode(req.id, { decision: 'approve', actorId: 'u1' }, SYS);
663+
expect(indexRows()).toEqual([{ request_id: req.id, approver: 'u2' }]);
664+
});
665+
666+
it('reassign and SLA-reassign rewrite the index rows', async () => {
667+
const req = await svc.openNodeRequest(
668+
openInput(['u9', 'u2'], {}, { escalation: { timeoutHours: 1, action: 'reassign', escalateTo: 'boss', notifySubmitter: false } }), CTX,
669+
);
670+
await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, SYS);
671+
expect(indexRows().map(r => r.approver).sort()).toEqual(['u2', 'u7']);
672+
673+
makeOverdue(req.id);
674+
await svc.runEscalations();
675+
expect(indexRows()).toEqual([{ request_id: req.id, approver: 'boss' }]);
676+
});
677+
678+
it('approver-filtered pages stay correct past the old 500-row scan window', async () => {
679+
// 30 u9 requests are the OLDEST rows, buried under 510 newer non-matching
680+
// ones — the pre-#1745 bounded scan (limit 500, newest-first) could never
681+
// reach them. Seeded directly: 540 openNodeRequest round-trips are noise.
682+
const reqs = (engine._tables['sys_approval_request'] ??= []);
683+
const idx = (engine._tables['sys_approval_approver'] ??= []);
684+
const ts = (i: number) => new Date(baseTime + i * 1000).toISOString();
685+
for (let i = 0; i < 30; i++) {
686+
reqs.push({
687+
id: `match_${i}`, process_name: 'flow:f', object_name: 'o', record_id: `m${i}`,
688+
status: 'pending', pending_approvers: 'u9', created_at: ts(i), updated_at: ts(i),
689+
});
690+
idx.push({ id: `aapr_m${i}`, request_id: `match_${i}`, approver: 'u9', organization_id: null, created_at: ts(i) });
691+
}
692+
for (let i = 0; i < 510; i++) {
693+
reqs.push({
694+
id: `noise_${i}`, process_name: 'flow:f', object_name: 'o', record_id: `n${i}`,
695+
status: 'pending', pending_approvers: 'someone-else', created_at: ts(100 + i), updated_at: ts(100 + i),
696+
});
697+
idx.push({ id: `aapr_n${i}`, request_id: `noise_${i}`, approver: 'someone-else', organization_id: null, created_at: ts(100 + i) });
698+
}
699+
700+
expect(await svc.countRequests({ approverId: 'u9' }, SYS)).toBe(30);
701+
const page = await svc.listRequests({ approverId: 'u9', limit: 10, offset: 20 }, SYS);
702+
// Newest-first within the matches: offset 20 of 30 → match_9 … match_0.
703+
expect(page.map(r => r.id)).toEqual(Array.from({ length: 10 }, (_, k) => `match_${9 - k}`));
704+
expect(page.every(r => r.pending_approvers?.includes('u9'))).toBe(true);
705+
});
706+
707+
it('rebuildApproverIndex backfills legacy rows, drops orphans + stale entries, and is idempotent', async () => {
708+
const reqs = (engine._tables['sys_approval_request'] ??= []);
709+
const idx = (engine._tables['sys_approval_approver'] ??= []);
710+
const ts = new Date(baseTime).toISOString();
711+
// Legacy pending row written before the index existed.
712+
reqs.push({
713+
id: 'legacy_1', process_name: 'flow:f', object_name: 'o', record_id: 'r1',
714+
status: 'pending', pending_approvers: 'u1,u2', created_at: ts, updated_at: ts,
715+
});
716+
// Completed row whose index rows were never cleaned (orphan).
717+
reqs.push({
718+
id: 'done_1', process_name: 'flow:f', object_name: 'o', record_id: 'r2',
719+
status: 'approved', pending_approvers: null, created_at: ts, updated_at: ts,
720+
});
721+
idx.push({ id: 'aapr_orphan', request_id: 'done_1', approver: 'u3', organization_id: null, created_at: ts });
722+
// Pending row whose index drifted (holds an approver no longer in the CSV).
723+
reqs.push({
724+
id: 'drift_1', process_name: 'flow:f', object_name: 'o', record_id: 'r3',
725+
status: 'pending', pending_approvers: 'u5', created_at: ts, updated_at: ts,
726+
});
727+
idx.push({ id: 'aapr_stale', request_id: 'drift_1', approver: 'u4', organization_id: null, created_at: ts });
728+
729+
const out = await svc.rebuildApproverIndex();
730+
expect(out).toEqual({ requests: 2, inserted: 3, deleted: 2 }); // +u1 +u2 +u5 / -orphan -stale
731+
expect(indexRows().sort((a, b) => a.approver.localeCompare(b.approver))).toEqual([
732+
{ request_id: 'legacy_1', approver: 'u1' },
733+
{ request_id: 'legacy_1', approver: 'u2' },
734+
{ request_id: 'drift_1', approver: 'u5' },
735+
]);
736+
737+
const again = await svc.rebuildApproverIndex();
738+
expect(again).toEqual({ requests: 2, inserted: 0, deleted: 0 });
739+
});
740+
625741
// ── SLA escalation (ADR-0042) ───────────────────────────────────
626742

627743
function makeOverdue(reqId: string) {

0 commit comments

Comments
 (0)