diff --git a/.changeset/24751-remind-notified-per-person.md b/.changeset/24751-remind-notified-per-person.md new file mode 100644 index 00000000000..1e73fa3ca8e --- /dev/null +++ b/.changeset/24751-remind-notified-per-person.md @@ -0,0 +1,11 @@ +--- +'@objectstack/plugin-approvals': patch +--- + +fix(plugin-approvals): a reminder's `notified` counts each approver address once, not once per slate slot + +Clause-②: no + +`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. + +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. diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 3382971460c..ea15dca7233 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -5482,9 +5482,19 @@ export class ApprovalService implements IApprovalService { // approver: a reminder names no channel, messaging's default materializes // it in the in-app inbox, and the inbox row carries the title, the body and // the deep link below, never links. The deep link is the approver's way in. + // + // [#24751] Each address once — the opening's rule (`openedOn`). A slate + // lists a person once per group they resolved into (per_group tagging), and + // `pending_approvers` keeps every one of those slots until the first vote, + // so one id can stand there twice. Messaging delivers that person ONE + // reminder (both emits carry the same dedup key), so counting per slot + // answered a `notified` the inbox contradicts. `notified` is the number of + // reminders handed over: one per distinct address. A user id and the same + // account's email are two addresses here, exactly as at the opening — two + // slots the person takes separately, and two reminders delivered. const subject = this.nameSubjectRecord(raw.object_name, raw.record_id); let notified = 0; - const concrete = pending.filter(a => a && !a.includes(':')); + const concrete = new Set(pending.filter(a => a && !a.includes(':'))); const literals = pending.filter(a => a && a.includes(':')); for (const approver of concrete) { notified += await this.notify({ diff --git a/packages/plugins/plugin-approvals/src/remind-notified-per-person.integration.test.ts b/packages/plugins/plugin-approvals/src/remind-notified-per-person.integration.test.ts new file mode 100644 index 00000000000..3dc9f2789ce --- /dev/null +++ b/packages/plugins/plugin-approvals/src/remind-notified-per-person.integration.test.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A reminder's `notified` counts the reminders it hands over — one per + * distinct approver address, never one per slate slot (#24751). + * + * ## The defect + * + * 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 + * one user placed in two groups stands on the stored slate twice. The opening + * fan-out told that person once (`openedOn`, a Set); `remind()` looped over + * the raw slots and added each `notify()` result, so `POST …/remind` answered + * `notified: 2` while messaging stored ONE reminder for them — the second emit + * carries the same dedup key. The console shows that number to the submitter + * ("Reminder sent to N approver(s)"). + * + * ## What this pins + * + * Driven through the real `ApprovalService` on a real `ObjectQL` over + * `SqlDriver` (better-sqlite3, in-memory, real DDL), with the real + * `AutomationEngine` running each flow and the real approval node opening the + * request, and the messaging surface recording every emit it is handed: + * + * - one approver listed in two groups: the stored slate really lists them + * twice (the scene), the reminder hands messaging ONE reminder for them and + * answers `notified: 1`; + * - the same holds for an approver authored as an email in both groups; + * - control: two distinct approvers get a reminder each, `notified: 2`; + * - a `position:*` literal the slate keeps beside a person adds nothing — no + * emit, no count — while the person beside it is still reminded; + * - an account's id and its email are two addresses, at the reminder exactly + * as at the opening: two reminders, `notified: 2`; + * - on every slate, the reminder reaches exactly the addresses the opening's + * `approval.requested` reached. + * + * Unchanged by the fix and held here too: each reminder's topic, deep link and + * per-approver dedup key, and the one `remind` audit row per reminder. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { AutomationEngine } from '@objectstack/service-automation'; +import type { EngineQueryOptions } from '@objectstack/spec/data'; +import { ApprovalService } from './approval-service.js'; +import { ApprovalsServicePlugin } from './approvals-plugin.js'; +import { registerApprovalNode } from './approval-node.js'; + +const SYSTEM = { isSystem: true, positions: [], permissions: [] } as any; +const asUser = (userId: string) => ({ userId, positions: [], permissions: [] }) as any; +const silent = { info: () => {}, warn: () => {}, error: () => {}, debug: () => {} } as any; + +const ORG = 'org_remind'; +const ALICE_EMAIL = 'alice@example.com'; + +const USERS = [ + { id: 'u_sub', name: 'Sam Submitter', email: 'sam@example.com' }, + { id: 'u_alice', name: 'Alice', email: ALICE_EMAIL }, + { id: 'u_bob', name: 'Bob', email: 'bob@example.com' }, +]; + +const sysUser = { + name: 'sys_user', + label: 'User', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + email: { name: 'email', label: 'Email', type: 'text' as const }, + }, +}; +const deal = { + name: 'crm_deal', + label: 'Deal', + fields: { + id: { name: 'id', label: 'Id', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + }, +}; + +/** The plugin's own object definitions, as its manifest registers them. */ +async function packageObjects(): Promise { + let manifest: any; + const ctx: any = { + getService: (name: string) => { + if (name === 'manifest') return { register: (m: any) => { manifest = m; } }; + throw new Error(`no service '${name}'`); + }, + logger: silent, + }; + await new ApprovalsServicePlugin().init(ctx); + return manifest.objects; +} + +/** A `user` approver in a named group of a `per_group` slate. */ +const inGroup = (value: string, group: string) => ({ type: 'user', value, group }); + +/** One `per_group` approval step between start and two ends. */ +function perGroup(name: string, approvers: Array>): any { + return { + name, label: name, type: 'autolaunched', + nodes: [ + { id: 'start', type: 'start', label: 'Start' }, + { id: 'review', type: 'approval', label: 'Review', config: { approvers, behavior: 'per_group' } }, + { id: 'ok', type: 'end', label: 'Approved' }, + { id: 'no', type: 'end', label: 'Rejected' }, + ], + edges: [ + { id: 'e1', source: 'start', target: 'review' }, + { id: 'e2', source: 'review', target: 'ok', label: 'approve' }, + { id: 'e3', source: 'review', target: 'no', label: 'reject' }, + ], + }; +} + +/** The slates under test — each its own flow, so each request is its own reminder window. */ +const SLATES: Record>> = { + twice_by_id: [inGroup('u_alice', 'first'), inGroup('u_alice', 'second')], + twice_by_email: [inGroup(ALICE_EMAIL, 'first'), inGroup(ALICE_EMAIL, 'second')], + two_people: [inGroup('u_alice', 'first'), inGroup('u_bob', 'second')], + // `position:nobody_holds` stays on the slate as its `type:value` literal. + person_and_literal: [inGroup('u_alice', 'first'), { type: 'position', value: 'nobody_holds', group: 'second' }], + id_and_email: [inGroup('u_alice', 'first'), inGroup(ALICE_EMAIL, 'second')], +}; + +interface Emitted { + topic: string; + audience: string[]; + dedupKey?: string; + source?: { object: string; id: string }; + payload?: Record; +} + +describe('a reminder counts the reminders it hands over, one per distinct approver address', () => { + let engine: ObjectQL; + let automation: AutomationEngine; + let svc: ApprovalService; + let now = Date.parse('2026-10-20T09:00:00.000Z'); + const emits: Emitted[] = []; + + /** Start `flow` on a fresh deal as the submitter; answer the request it opened. */ + const submit = async (flow: string): Promise => { + const record = { id: `D_${flow}`, title: flow, organization_id: ORG }; + await engine.insert('crm_deal', record, { context: SYSTEM } as any); + const run = await automation.execute(flow, { object: 'crm_deal', record, userId: 'u_sub' } as any); + expect(run.status, `${flow} paused on its approval step`).toBe('paused'); + const rows: any[] = await engine.find('sys_approval_request', { + where: { record_id: record.id, status: 'pending' }, context: SYSTEM, + } satisfies EngineQueryOptions); + expect(rows.length, `one pending request on ${record.id}`).toBe(1); + return rows[0]; + }; + /** The audiences of one topic's emits about one request, in emit order. */ + const audiencesOf = (topic: string, requestId: string) => + emits.filter((e) => e.topic === topic && e.source?.id === requestId).map((e) => e.audience); + /** + * Submit on `flow`, then send its reminder as the submitter. Every slate also + * holds the reminder to the opening's rule: it reaches exactly the addresses + * `approval.requested` reached (none of these slates has an out-of-office hop). + */ + const remindOn = async (flow: string) => { + const req = await submit(flow); + const stored = String(req.pending_approvers ?? '').split(',').filter(Boolean); + const out = await svc.remind(req.id, { actorId: 'u_sub' }, asUser('u_sub')); + const reminders = audiencesOf('approval.reminder', req.id); + const opened = audiencesOf('approval.requested', req.id); + expect(opened.length, `${flow}: the opening told someone`).toBeGreaterThan(0); + // Soft, so a broken fan-out still reaches the case's own `notified` assertion. + expect.soft(reminders, `${flow}: the reminder reached whom the opening reached`).toEqual(opened); + return { req, stored, notified: out.notified, reminders }; + }; + + beforeAll(async () => { + engine = new ObjectQL(); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true, + }), true); + await engine.init(); + for (const def of [sysUser, deal, ...(await packageObjects())]) { + engine.registry.registerObject(def as any, 'approvals-test', 'approvals-test'); + } + await engine.syncSchemas(); + for (const u of USERS) await engine.insert('sys_user', u, { context: SYSTEM } as any); + + automation = new AutomationEngine(silent); + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(now += 1000) }, + logger: silent, + }); + svc.attachAutomation(automation as any); + svc.attachMessaging({ async emit(input) { emits.push(input as Emitted); } }); + registerApprovalNode(automation as any, svc, silent); + + for (const [name, approvers] of Object.entries(SLATES)) automation.registerFlow(name, perGroup(name, approvers)); + }); + + afterAll(async () => { + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('an approver the slate lists in two groups is handed one reminder and counted once', async () => { + const { req, stored, notified, reminders } = await remindOn('twice_by_id'); + // The scene: the stored slate really lists her twice, so a count of 1 is the rule, not an empty slate. + expect(stored).toEqual(['u_alice', 'u_alice']); + + expect(notified).toBe(1); + expect(reminders).toEqual([['u_alice']]); + + // Unchanged: the reminder itself — topic, deep link, per-approver dedup key — and its one audit row. + const [reminder] = emits.filter((e) => e.topic === 'approval.reminder' && e.source?.id === req.id); + expect(reminder.payload?.actionUrl).toBe(`/system/approvals?request=${encodeURIComponent(req.id)}`); + expect(reminder.dedupKey).toMatch(new RegExp(`^approval-remind-${req.id}-.+-u_alice$`)); + const audit: any[] = await engine.find('sys_approval_action', { + where: { request_id: req.id, action: 'remind' }, context: SYSTEM, + } satisfies EngineQueryOptions); + expect(audit.map((a) => a.actor_id)).toEqual(['u_sub']); + }); + + it('an approver authored as the same email in two groups is handed one reminder and counted once', async () => { + const { stored, notified, reminders } = await remindOn('twice_by_email'); + expect(stored).toEqual([ALICE_EMAIL, ALICE_EMAIL]); + expect(notified).toBe(1); + expect(reminders).toEqual([[ALICE_EMAIL]]); + }); + + it('control: two distinct approvers are each handed a reminder, and both are counted', async () => { + const { stored, notified, reminders } = await remindOn('two_people'); + expect(stored).toEqual(['u_alice', 'u_bob']); + expect(notified).toBe(2); + expect(reminders).toEqual([['u_alice'], ['u_bob']]); + }); + + it('a position:* literal on the slate adds nothing, and the person beside it is still reminded', async () => { + const { stored, notified, reminders } = await remindOn('person_and_literal'); + expect(stored).toEqual(['u_alice', 'position:nobody_holds']); + // The literal names no one: it adds nothing to the count, and no emit carries it. + expect(notified).toBe(1); + expect(reminders).toEqual([['u_alice']]); + }); + + it("an account's id and its email are two addresses: two reminders, as at the opening", async () => { + const { stored, notified, reminders } = await remindOn('id_and_email'); + expect(stored).toEqual(['u_alice', ALICE_EMAIL]); + expect(notified).toBe(2); + expect(reminders).toEqual([['u_alice'], [ALICE_EMAIL]]); + }); +});