Skip to content

Commit 9a06f0b

Browse files
fix(plugin-approvals): a reminder's notified counts each approver address once, not once per slate slot (#24855)
Fixes #24751 Clause-②: no ## What changed `ApprovalService.remind()` (`POST /api/v1/approvals/requests/:id/remind`) now de-duplicates the slate's concrete addresses before its per-approver loop, the rule the opening fan-out already applies (`openedOn`, a `Set`). `notified` is the number of reminders handed to messaging, one per distinct approver address. The code change is one line, `const concrete = new Set(pending.filter(...))`, plus the comment that states the rule. Unchanged: who is reminded; the reminder's topic, payload, deep link and per-approver dedup key; the throttle and the submitter check; the `remind` audit row; the shared nudge for `position:*` literals. `notify()` and the opening fan-out are untouched. No `packages/spec` edit: `IApprovalService.remind` says "notify every pending approver" and does not define `notified`. ## The premise, measured before the fix Measured on a composed stack: `bootStack` with the approvals source at `5fcc794972`, the real `MessagingServicePlugin` (inline fan-out) and the REST door. The fixture is a `per_group` flow whose two groups each read one approver from a record field. The measurement file was temporary and is not in this PR. | slate, two groups | stored `pending_approvers` | opening `approval.requested` events | remind `notified` | reminder events / inbox rows | |---|---|---|---|---| | same user id twice | `id,id` | 1 | **2** | 1 / 1 | | same email twice | `email,email` | 1 | **2** | 1 / 1 | | a user id + that account's email | `id,email` | 2, both delivered to the one user | 2 | 2 / 2, both to the one user | | two people | `a,b` | 2 | 2 | 2 / 2 | The duplicate is real, and it is reachable from the public door. `openNodeRequest` stores `approvers.join(',')`, and `expandApprovers` pushes one entry per group membership. The slate stays like that from the opening until the first vote, because a mid-tally vote rebuilds `pending_approvers` from `Object.keys(__approverGroups)`, which is already de-duplicated. Real producers, by reading and not re-measured here: the showcase seed grants the dev admin `manager` / `finance` / `legal` / `exec`. So `showcase_invoice_signoff` (`unanimous`: finance + legal) opens on that admin twice, and `showcase_committee_quorum` (manager / finance / legal) opens on them three times. ## A user id and the same account's email stay two addresses Measured above: messaging's `RecipientResolver` resolves an email-shaped address to its `sys_user` id. The two emits carry different dedup keys, so both land, and the one person gets two inbox rows, at the opening and at the reminder alike. This PR keeps that as two addresses and two reminders, with `notified: 2`. Read along the decision frame's four axes: - **Real business need.** The repeated-id shape has real producers (above). An id plus the same person's email requires an author to write both spellings for one person, and no example app or seed in this tree does. Folding them would serve a speculative shape. - **Long-term soundness.** A fold needs an email-to-id directory read inside `remind()`, and that would be a second address equivalence. `approver-address.ts` forbids writing one beside its own. A fold at the reminder alone would also make the reminder disagree with the opening fan-out, which the brief puts out of scope here. Exact-address de-duplication is the rule the opening already applies, so one rule now holds at both doors. - **Preventing AI authoring mistakes.** Neither option tightens or loosens a contract. Keeping one rule at both doors leaves nothing to learn twice. - **No scope creep at the startup stage.** The exact-address `Set` is the smallest change that makes the count agree with what messaging delivers. In every measured shape, `notified` now equals the reminders delivered. The slate holds two slots for that person (`heldSlot` takes the id first, and the email slot needs its own decision), so two reminders is also what the slate asks of them. ## Pin `packages/plugins/plugin-approvals/src/remind-notified-per-person.integration.test.ts` drives the real `ApprovalService` and the real `AutomationEngine` running each flow, with the real approval node opening each request. The stack is a real `ObjectQL` over `SqlDriver` (better-sqlite3, in-memory, real DDL), and messaging records every emit it is handed. The shape is the one `approval-decision-outcome-notification.integration.test.ts` uses. - one approver in two groups: the stored slate lists them twice (the scene), `notified` is 1, and one reminder is handed over. Also checked: the deep link, the per-approver dedup key, and one `remind` audit row by the submitter; - the same email in two groups: `notified` 1, one reminder; - control, two distinct approvers: `notified` 2, two reminders; - `position:nobody_holds` beside a person: the literal adds nothing and is carried by no emit, and the person is still reminded; - an id plus that account's email: two addresses, `notified` 2, two reminders; - every case also asserts, softly, that the reminder reached exactly the addresses the opening's `approval.requested` reached. On "one reminder row is stored": real messaging stores ONE reminder for the twice-listed person both before and after this fix (the second emit is a dedup hit, as the table shows), so a stored-row count cannot tell the fix from the defect. The pin asserts the boundary the fix actually moves instead: the emits handed to messaging, and `notified`. ## Ablation (one-time proof, not kept) The mutation was run through `scripts/ablation-replace.mjs` in wrap mode, under the shared lock, with an absolute-path EXIT/INT/TERM trap. It replaced `const concrete = new Set(pending.filter(a => a && !a.includes(':')));` with the plain `pending.filter(...)`, on committed head `5a3150311d`. The fix's subject resolves by relative import to `src/`, so no `dist/` rebuild was involved. - landed: anchor 1 → 0, blob `ea15dca72333` → `30e19d945490`; - red as expected: `Tests 2 failed | 3 passed (5)`. The twice-by-id and twice-by-email cases each failed `expected 2 to be 1` on `notified`, plus the soft opening cross-check (two emits against the opening's one). The two-people, literal and id-plus-email controls stayed green; - restored: blob after restore `ea15dca72333` == the blob at `HEAD`, and `git diff HEAD` is empty. ## Verification Run on head `f61fb763ef`, with `origin/main` `41646a5788` merged in, after the repository build (`turbo run build --filter=!@objectstack/docs`, 76/76 tasks): - `pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2`: `Test Files 89 passed (89)`, `Tests 1169 passed (1169)`; - `pnpm --filter @objectstack/plugin-approvals typecheck`: exit 0. `check:test-typecheck: OK` (8 files / 324 errors / 27 signatures held in the ledger, unchanged), and the new test file is in the `tsconfig.test.json` program (`--listFilesOnly`); - `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` derived 70 commands from this diff. All 70 ran on this head, each exit code captured before any pipe: 70 × exit 0. `--ran`: `70 derived, 70 run, 0 NOT-MEASURED, 0 UNRUN`. On an earlier head, two of them (`check:dual-build-cjs-loads`, `check:i18n`) first refused with exit 3 (PREREQUISITE NOT MET: packages outside this closure had no `dist/`). They went green after the full build, and green again on this head. Every family CI runs is read from this PR's check-runs. ## Acceptance notes - **A user id and the same account's email on one slate get two reminders and two "Approval requested" notices**, both at the opening and at the reminder. This is a recorded boundary, not a defect claim: the slate really holds two slots for that person, and no producer in this tree writes both spellings for one person. Not filed. Carrier: none. - **The stored-row half of the card's reproduction does not discriminate.** Real messaging already stored one reminder; the defect was only in the count. See Pin above. Related, and already merged on its side: objectstack-ai/objectui#12781. That change stopped the console's own double count of the same reminder. This PR corrects the framework count the console displays. --- _Generated by [Claude Code](https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 950f949 commit 9a06f0b

3 files changed

Lines changed: 270 additions & 1 deletion

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
fix(plugin-approvals): a reminder's `notified` counts each approver address once, not once per slate slot
6+
7+
Clause-②: no
8+
9+
`POST /api/v1/approvals/requests/:id/remind` (`ApprovalService.remind()`) answers `notified` as the number of reminders it handed to messaging: one per distinct approver address on the pending slate. A `per_group` slate lists a person once per group they resolved into, and `pending_approvers` keeps every one of those slots until the first vote, so a person placed in two groups was counted twice while messaging delivered them one reminder. They are now reminded once and counted once, the rule the opening `approval.requested` notice already applies.
10+
11+
A user id and the same account's email are still two addresses: the person holds two slots and receives two reminders, exactly as at the opening. Unchanged: who is reminded, each reminder's topic, payload, deep link and dedup key, the throttle, the submitter check, the `remind` audit row, and `position:*`-style literals, which still notify no one and add nothing to the count.

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5477,9 +5477,19 @@ export class ApprovalService implements IApprovalService {
54775477
// approver: a reminder names no channel, messaging's default materializes
54785478
// it in the in-app inbox, and the inbox row carries the title, the body and
54795479
// the deep link below, never links. The deep link is the approver's way in.
5480+
//
5481+
// [#24751] Each address once — the opening's rule (`openedOn`). A slate
5482+
// lists a person once per group they resolved into (per_group tagging), and
5483+
// `pending_approvers` keeps every one of those slots until the first vote,
5484+
// so one id can stand there twice. Messaging delivers that person ONE
5485+
// reminder (both emits carry the same dedup key), so counting per slot
5486+
// answered a `notified` the inbox contradicts. `notified` is the number of
5487+
// reminders handed over: one per distinct address. A user id and the same
5488+
// account's email are two addresses here, exactly as at the opening — two
5489+
// slots the person takes separately, and two reminders delivered.
54805490
const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
54815491
let notified = 0;
5482-
const concrete = pending.filter(a => a && !a.includes(':'));
5492+
const concrete = new Set(pending.filter(a => a && !a.includes(':')));
54835493
const literals = pending.filter(a => a && a.includes(':'));
54845494
for (const approver of concrete) {
54855495
notified += await this.notify({
Lines changed: 248 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,248 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* A reminder's `notified` counts the reminders it hands over — one per
5+
* distinct approver address, never one per slate slot (#24751).
6+
*
7+
* ## The defect
8+
*
9+
* A `per_group` slate lists a person once per group they resolved into, and
10+
* `pending_approvers` keeps every one of those slots until the first vote. So
11+
* one user placed in two groups stands on the stored slate twice. The opening
12+
* fan-out told that person once (`openedOn`, a Set); `remind()` looped over
13+
* the raw slots and added each `notify()` result, so `POST …/remind` answered
14+
* `notified: 2` while messaging stored ONE reminder for them — the second emit
15+
* carries the same dedup key. The console shows that number to the submitter
16+
* ("Reminder sent to N approver(s)").
17+
*
18+
* ## What this pins
19+
*
20+
* Driven through the real `ApprovalService` on a real `ObjectQL` over
21+
* `SqlDriver` (better-sqlite3, in-memory, real DDL), with the real
22+
* `AutomationEngine` running each flow and the real approval node opening the
23+
* request, and the messaging surface recording every emit it is handed:
24+
*
25+
* - one approver listed in two groups: the stored slate really lists them
26+
* twice (the scene), the reminder hands messaging ONE reminder for them and
27+
* answers `notified: 1`;
28+
* - the same holds for an approver authored as an email in both groups;
29+
* - control: two distinct approvers get a reminder each, `notified: 2`;
30+
* - a `position:*` literal the slate keeps beside a person adds nothing — no
31+
* emit, no count — while the person beside it is still reminded;
32+
* - an account's id and its email are two addresses, at the reminder exactly
33+
* as at the opening: two reminders, `notified: 2`;
34+
* - on every slate, the reminder reaches exactly the addresses the opening's
35+
* `approval.requested` reached.
36+
*
37+
* Unchanged by the fix and held here too: each reminder's topic, deep link and
38+
* per-approver dedup key, and the one `remind` audit row per reminder.
39+
*/
40+
41+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
42+
import { ObjectQL } from '@objectstack/objectql';
43+
import { SqlDriver } from '@objectstack/driver-sql';
44+
import { AutomationEngine } from '@objectstack/service-automation';
45+
import type { EngineQueryOptions } from '@objectstack/spec/data';
46+
import { ApprovalService } from './approval-service.js';
47+
import { ApprovalsServicePlugin } from './approvals-plugin.js';
48+
import { registerApprovalNode } from './approval-node.js';
49+
50+
const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any;
51+
const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any;
52+
const silent = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} } as any;
53+
54+
const ORG = 'org_remind';
55+
const ALICE_EMAIL = 'alice@example.com';
56+
57+
const USERS = [
58+
{ id: 'u_sub', name: 'Sam Submitter', email: 'sam@example.com' },
59+
{ id: 'u_alice', name: 'Alice', email: ALICE_EMAIL },
60+
{ id: 'u_bob', name: 'Bob', email: 'bob@example.com' },
61+
];
62+
63+
const sysUser = {
64+
name: 'sys_user',
65+
label: 'User',
66+
fields: {
67+
id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
68+
name: { name: 'name', label: 'Name', type: 'text' as const },
69+
email: { name: 'email', label: 'Email', type: 'text' as const },
70+
},
71+
};
72+
const deal = {
73+
name: 'crm_deal',
74+
label: 'Deal',
75+
fields: {
76+
id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true },
77+
title: { name: 'title', label: 'Title', type: 'text' as const },
78+
},
79+
};
80+
81+
/** The plugin's own object definitions, as its manifest registers them. */
82+
async function packageObjects(): Promise<any[]> {
83+
let manifest: any;
84+
const ctx: any = {
85+
getService: (name: string) => {
86+
if (name === 'manifest') return { register: (m: any) => { manifest = m; } };
87+
throw new Error(`no service '${name}'`);
88+
},
89+
logger: silent,
90+
};
91+
await new ApprovalsServicePlugin().init(ctx);
92+
return manifest.objects;
93+
}
94+
95+
/** A `user` approver in a named group of a `per_group` slate. */
96+
const inGroup = (value: string, group: string) => ({ type: 'user', value, group });
97+
98+
/** One `per_group` approval step between start and two ends. */
99+
function perGroup(name: string, approvers: Array<Record<string, unknown>>): any {
100+
return {
101+
name, label: name, type: 'autolaunched',
102+
nodes: [
103+
{ id: 'start', type: 'start', label: 'Start' },
104+
{ id: 'review', type: 'approval', label: 'Review', config: { approvers, behavior: 'per_group' } },
105+
{ id: 'ok', type: 'end', label: 'Approved' },
106+
{ id: 'no', type: 'end', label: 'Rejected' },
107+
],
108+
edges: [
109+
{ id: 'e1', source: 'start', target: 'review' },
110+
{ id: 'e2', source: 'review', target: 'ok', label: 'approve' },
111+
{ id: 'e3', source: 'review', target: 'no', label: 'reject' },
112+
],
113+
};
114+
}
115+
116+
/** The slates under test — each its own flow, so each request is its own reminder window. */
117+
const SLATES: Record<string, Array<Record<string, unknown>>> = {
118+
twice_by_id: [inGroup('u_alice', 'first'), inGroup('u_alice', 'second')],
119+
twice_by_email: [inGroup(ALICE_EMAIL, 'first'), inGroup(ALICE_EMAIL, 'second')],
120+
two_people: [inGroup('u_alice', 'first'), inGroup('u_bob', 'second')],
121+
// `position:nobody_holds` stays on the slate as its `type:value` literal.
122+
person_and_literal: [inGroup('u_alice', 'first'), { type: 'position', value: 'nobody_holds', group: 'second' }],
123+
id_and_email: [inGroup('u_alice', 'first'), inGroup(ALICE_EMAIL, 'second')],
124+
};
125+
126+
interface Emitted {
127+
topic: string;
128+
audience: string[];
129+
dedupKey?: string;
130+
source?: { object: string; id: string };
131+
payload?: Record<string, unknown>;
132+
}
133+
134+
describe('a reminder counts the reminders it hands over, one per distinct approver address', () => {
135+
let engine: ObjectQL;
136+
let automation: AutomationEngine;
137+
let svc: ApprovalService;
138+
let now = Date.parse('2026-10-20T09:00:00.000Z');
139+
const emits: Emitted[] = [];
140+
141+
/** Start `flow` on a fresh deal as the submitter; answer the request it opened. */
142+
const submit = async (flow: string): Promise<any> => {
143+
const record = { id: `D_${flow}`, title: flow, organization_id: ORG };
144+
await engine.insert('crm_deal', record, { context: SYSTEM } as any);
145+
const run = await automation.execute(flow, { object: 'crm_deal', record, userId: 'u_sub' } as any);
146+
expect(run.status, `${flow} paused on its approval step`).toBe('paused');
147+
const rows: any[] = await engine.find('sys_approval_request', {
148+
where: { record_id: record.id, status: 'pending' }, context: SYSTEM,
149+
} satisfies EngineQueryOptions);
150+
expect(rows.length, `one pending request on ${record.id}`).toBe(1);
151+
return rows[0];
152+
};
153+
/** The audiences of one topic's emits about one request, in emit order. */
154+
const audiencesOf = (topic: string, requestId: string) =>
155+
emits.filter((e) => e.topic === topic && e.source?.id === requestId).map((e) => e.audience);
156+
/**
157+
* Submit on `flow`, then send its reminder as the submitter. Every slate also
158+
* holds the reminder to the opening's rule: it reaches exactly the addresses
159+
* `approval.requested` reached (none of these slates has an out-of-office hop).
160+
*/
161+
const remindOn = async (flow: string) => {
162+
const req = await submit(flow);
163+
const stored = String(req.pending_approvers ?? '').split(',').filter(Boolean);
164+
const out = await svc.remind(req.id, { actorId: 'u_sub' }, asUser('u_sub'));
165+
const reminders = audiencesOf('approval.reminder', req.id);
166+
const opened = audiencesOf('approval.requested', req.id);
167+
expect(opened.length, `${flow}: the opening told someone`).toBeGreaterThan(0);
168+
// Soft, so a broken fan-out still reaches the case's own `notified` assertion.
169+
expect.soft(reminders, `${flow}: the reminder reached whom the opening reached`).toEqual(opened);
170+
return { req, stored, notified: out.notified, reminders };
171+
};
172+
173+
beforeAll(async () => {
174+
engine = new ObjectQL();
175+
engine.registerDriver(new SqlDriver({
176+
client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true,
177+
}), true);
178+
await engine.init();
179+
for (const def of [sysUser, deal, ...(await packageObjects())]) {
180+
engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test');
181+
}
182+
await engine.syncSchemas();
183+
for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any);
184+
185+
automation = new AutomationEngine(silent);
186+
svc = new ApprovalService({
187+
engine: engine as any,
188+
clock: { now: () => new Date(now += 1000) },
189+
logger: silent,
190+
});
191+
svc.attachAutomation(automation as any);
192+
svc.attachMessaging({ async emit(input) { emits.push(input as Emitted); } });
193+
registerApprovalNode(automation as any, svc, silent);
194+
195+
for (const [name, approvers] of Object.entries(SLATES)) automation.registerFlow(name, perGroup(name, approvers));
196+
});
197+
198+
afterAll(async () => {
199+
try { await engine?.destroy(); } catch { /* noop */ }
200+
});
201+
202+
it('an approver the slate lists in two groups is handed one reminder and counted once', async () => {
203+
const { req, stored, notified, reminders } = await remindOn('twice_by_id');
204+
// The scene: the stored slate really lists her twice, so a count of 1 is the rule, not an empty slate.
205+
expect(stored).toEqual(['u_alice', 'u_alice']);
206+
207+
expect(notified).toBe(1);
208+
expect(reminders).toEqual([['u_alice']]);
209+
210+
// Unchanged: the reminder itself — topic, deep link, per-approver dedup key — and its one audit row.
211+
const [reminder] = emits.filter((e) => e.topic === 'approval.reminder' && e.source?.id === req.id);
212+
expect(reminder.payload?.actionUrl).toBe(`/system/approvals?request=${encodeURIComponent(req.id)}`);
213+
expect(reminder.dedupKey).toMatch(new RegExp(`^approval-remind-${req.id}-.+-u_alice$`));
214+
const audit: any[] = await engine.find('sys_approval_action', {
215+
where: { request_id: req.id, action: 'remind' }, context: SYSTEM,
216+
} satisfies EngineQueryOptions);
217+
expect(audit.map((a) => a.actor_id)).toEqual(['u_sub']);
218+
});
219+
220+
it('an approver authored as the same email in two groups is handed one reminder and counted once', async () => {
221+
const { stored, notified, reminders } = await remindOn('twice_by_email');
222+
expect(stored).toEqual([ALICE_EMAIL, ALICE_EMAIL]);
223+
expect(notified).toBe(1);
224+
expect(reminders).toEqual([[ALICE_EMAIL]]);
225+
});
226+
227+
it('control: two distinct approvers are each handed a reminder, and both are counted', async () => {
228+
const { stored, notified, reminders } = await remindOn('two_people');
229+
expect(stored).toEqual(['u_alice', 'u_bob']);
230+
expect(notified).toBe(2);
231+
expect(reminders).toEqual([['u_alice'], ['u_bob']]);
232+
});
233+
234+
it('a position:* literal on the slate adds nothing, and the person beside it is still reminded', async () => {
235+
const { stored, notified, reminders } = await remindOn('person_and_literal');
236+
expect(stored).toEqual(['u_alice', 'position:nobody_holds']);
237+
// The literal names no one: it adds nothing to the count, and no emit carries it.
238+
expect(notified).toBe(1);
239+
expect(reminders).toEqual([['u_alice']]);
240+
});
241+
242+
it("an account's id and its email are two addresses: two reminders, as at the opening", async () => {
243+
const { stored, notified, reminders } = await remindOn('id_and_email');
244+
expect(stored).toEqual(['u_alice', ALICE_EMAIL]);
245+
expect(notified).toBe(2);
246+
expect(reminders).toEqual([['u_alice'], [ALICE_EMAIL]]);
247+
});
248+
});

0 commit comments

Comments
 (0)