Skip to content

Commit 5938ec9

Browse files
committed
fix(plugin-approvals): a reminder's notified counts each approver address once, not once per slate slot
A per_group slate lists a person once per group they resolved into, and pending_approvers keeps every slot until the first vote. remind() looped over the raw slots, so a person in two groups was counted twice while messaging stored one reminder (both emits carry the same dedup key). De-duplicate the concrete addresses before the loop, the opening fan-out's openedOn rule. The literals branch, the per-approver dedup key, the payload, the throttle and the audit row are unchanged. Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5fcc794 commit 5938ec9

3 files changed

Lines changed: 269 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
@@ -5482,9 +5482,19 @@ export class ApprovalService implements IApprovalService {
54825482
// approver: a reminder names no channel, messaging's default materializes
54835483
// it in the in-app inbox, and the inbox row carries the title, the body and
54845484
// the deep link below, never links. The deep link is the approver's way in.
5485+
//
5486+
// [#24751] Each address once — the opening's rule (`openedOn`). A slate
5487+
// lists a person once per group they resolved into (per_group tagging), and
5488+
// `pending_approvers` keeps every one of those slots until the first vote,
5489+
// so one id can stand there twice. Messaging delivers that person ONE
5490+
// reminder (both emits carry the same dedup key), so counting per slot
5491+
// answered a `notified` the inbox contradicts. `notified` is the number of
5492+
// reminders handed over: one per distinct address. A user id and the same
5493+
// account's email are two addresses here, exactly as at the opening — two
5494+
// slots the person takes separately, and two reminders delivered.
54855495
const subject = this.nameSubjectRecord(raw.object_name, raw.record_id);
54865496
let notified = 0;
5487-
const concrete = pending.filter(a => a && !a.includes(':'));
5497+
const concrete = new Set(pending.filter(a => a && !a.includes(':')));
54885498
const literals = pending.filter(a => a && a.includes(':'));
54895499
for (const approver of concrete) {
54905500
notified += await this.notify({
Lines changed: 247 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,247 @@
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+
expect(reminders, `${flow}: the reminder reached whom the opening reached`).toEqual(opened);
169+
return { req, stored, notified: out.notified, reminders };
170+
};
171+
172+
beforeAll(async () => {
173+
engine = new ObjectQL();
174+
engine.registerDriver(new SqlDriver({
175+
client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true,
176+
}), true);
177+
await engine.init();
178+
for (const def of [sysUser, deal, ...(await packageObjects())]) {
179+
engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test');
180+
}
181+
await engine.syncSchemas();
182+
for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any);
183+
184+
automation = new AutomationEngine(silent);
185+
svc = new ApprovalService({
186+
engine: engine as any,
187+
clock: { now: () => new Date(now += 1000) },
188+
logger: silent,
189+
});
190+
svc.attachAutomation(automation as any);
191+
svc.attachMessaging({ async emit(input) { emits.push(input as Emitted); } });
192+
registerApprovalNode(automation as any, svc, silent);
193+
194+
for (const [name, approvers] of Object.entries(SLATES)) automation.registerFlow(name, perGroup(name, approvers));
195+
});
196+
197+
afterAll(async () => {
198+
try { await engine?.destroy(); } catch { /* noop */ }
199+
});
200+
201+
it('an approver the slate lists in two groups is handed one reminder and counted once', async () => {
202+
const { req, stored, notified, reminders } = await remindOn('twice_by_id');
203+
// The scene: the stored slate really lists her twice, so a count of 1 is the rule, not an empty slate.
204+
expect(stored).toEqual(['u_alice', 'u_alice']);
205+
206+
expect(reminders).toEqual([['u_alice']]);
207+
expect(notified).toBe(1);
208+
209+
// Unchanged: the reminder itself — topic, deep link, per-approver dedup key — and its one audit row.
210+
const [reminder] = emits.filter((e) => e.topic === 'approval.reminder' && e.source?.id === req.id);
211+
expect(reminder.payload?.actionUrl).toBe(`/system/approvals?request=${encodeURIComponent(req.id)}`);
212+
expect(reminder.dedupKey).toMatch(new RegExp(`^approval-remind-${req.id}-.+-u_alice$`));
213+
const audit: any[] = await engine.find('sys_approval_action', {
214+
where: { request_id: req.id, action: 'remind' }, context: SYSTEM,
215+
} satisfies EngineQueryOptions);
216+
expect(audit.map((a) => a.actor_id)).toEqual(['u_sub']);
217+
});
218+
219+
it('an approver authored as the same email in two groups is handed one reminder and counted once', async () => {
220+
const { stored, notified, reminders } = await remindOn('twice_by_email');
221+
expect(stored).toEqual([ALICE_EMAIL, ALICE_EMAIL]);
222+
expect(reminders).toEqual([[ALICE_EMAIL]]);
223+
expect(notified).toBe(1);
224+
});
225+
226+
it('control: two distinct approvers are each handed a reminder, and both are counted', async () => {
227+
const { stored, notified, reminders } = await remindOn('two_people');
228+
expect(stored).toEqual(['u_alice', 'u_bob']);
229+
expect(reminders).toEqual([['u_alice'], ['u_bob']]);
230+
expect(notified).toBe(2);
231+
});
232+
233+
it('a position:* literal on the slate adds nothing, and the person beside it is still reminded', async () => {
234+
const { stored, notified, reminders } = await remindOn('person_and_literal');
235+
expect(stored).toEqual(['u_alice', 'position:nobody_holds']);
236+
// The literal names no one: no emit carries it, and it adds nothing to the count.
237+
expect(reminders).toEqual([['u_alice']]);
238+
expect(notified).toBe(1);
239+
});
240+
241+
it("an account's id and its email are two addresses: two reminders, as at the opening", async () => {
242+
const { stored, notified, reminders } = await remindOn('id_and_email');
243+
expect(stored).toEqual(['u_alice', ALICE_EMAIL]);
244+
expect(reminders).toEqual([['u_alice'], [ALICE_EMAIL]]);
245+
expect(notified).toBe(2);
246+
});
247+
});

0 commit comments

Comments
 (0)