Repository navigation
fix(plugin-approvals): a reminder's notified counts each approver address once, not once per slate slot - #24855
Conversation
…ress 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>
…e reaches its notified assertion Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude <noreply@anthropic.com>
…e emitted audiences Claude-Session: https://claude.ai/code/session_011TdZ8GjixNZFLz1xJD9zzK Co-authored-by: Claude <noreply@anthropic.com>
…mind-notified-per-person
…mind-notified-per-person
📓 Docs Drift CheckNothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 1 changed package(s)), so this run has no opinion about the docs. What this run could not see
Coarse fallback — 6 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
席位核对 · ACCEPT ·
|
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, aSet).notifiedis 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
remindaudit row; the shared nudge forposition:*literals.notify()and the opening fan-out are untouched. Nopackages/specedit:IApprovalService.remindsays "notify every pending approver" and does not definenotified.The premise, measured before the fix
Measured on a composed stack:
bootStackwith the approvals source at5fcc794972, the realMessagingServicePlugin(inline fan-out) and the REST door. The fixture is aper_groupflow whose two groups each read one approver from a record field. The measurement file was temporary and is not in this PR.pending_approversapproval.requestedeventsnotifiedid,idemail,emailid,emaila,bThe duplicate is real, and it is reachable from the public door.
openNodeRequeststoresapprovers.join(','), andexpandApproverspushes one entry per group membership. The slate stays like that from the opening until the first vote, because a mid-tally vote rebuildspending_approversfromObject.keys(__approverGroups), which is already de-duplicated. Real producers, by reading and not re-measured here: the showcase seed grants the dev adminmanager/finance/legal/exec. Soshowcase_invoice_signoff(unanimous: finance + legal) opens on that admin twice, andshowcase_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
RecipientResolverresolves an email-shaped address to itssys_userid. 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, withnotified: 2. Read along the decision frame's four axes:remind(), and that would be a second address equivalence.approver-address.tsforbids 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.Setis the smallest change that makes the count agree with what messaging delivers. In every measured shape,notifiednow equals the reminders delivered.The slate holds two slots for that person (
heldSlottakes 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.tsdrives the realApprovalServiceand the realAutomationEnginerunning each flow, with the real approval node opening each request. The stack is a realObjectQLoverSqlDriver(better-sqlite3, in-memory, real DDL), and messaging records every emit it is handed. The shape is the oneapproval-decision-outcome-notification.integration.test.tsuses.notifiedis 1, and one reminder is handed over. Also checked: the deep link, the per-approver dedup key, and oneremindaudit row by the submitter;notified1, one reminder;notified2, two reminders;position:nobody_holdsbeside a person: the literal adds nothing and is carried by no emit, and the person is still reminded;notified2, two reminders;approval.requestedreached.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.mjsin wrap mode, under the shared lock, with an absolute-path EXIT/INT/TERM trap. It replacedconst concrete = new Set(pending.filter(a => a && !a.includes(':')));with the plainpending.filter(...), on committed head5a3150311d. The fix's subject resolves by relative import tosrc/, so nodist/rebuild was involved.ea15dca72333→30e19d945490;Tests 2 failed | 3 passed (5). The twice-by-id and twice-by-email cases each failedexpected 2 to be 1onnotified, plus the soft opening cross-check (two emits against the opening's one). The two-people, literal and id-plus-email controls stayed green;ea15dca72333== the blob atHEAD, andgit diff HEADis empty.Verification
Run on head
f61fb763ef, withorigin/main41646a5788merged 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 thetsconfig.test.jsonprogram (--listFilesOnly);node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstackderived 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 nodist/). 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
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