Repository navigation
Commit 544b8d7
fix(plugin-approvals): the out-of-office notifications name the absent approver and the delegate by display name (#24146)
Fixes #24065
Clause-②: no
## What changed
The out-of-office fan-out in `openNodeRequest` now names both people by
display name in its two notification bodies. Path:
`packages/plugins/plugin-approvals/src/approval-service.ts`, line
numbers at this head.
- `approval.ooo_substituted` (`:3640`) goes to the delegate and names
the absent approver. `You are covering an approval on Deal D_ooo while
u_ooo is out of office.` now reads `… while Olive Away is out of
office.`
- `approval.ooo_skipped` (`:3652`) goes to the absent approver and names
the delegate. `An approval on Deal D_ooo was routed to u_bob while you
are out of office.` now reads `… was routed to Bob while you are out of
office.`
Both names come from `resolveUserNames` (`:6680`), the plugin's one
person-label chain. The approvals door uses the same resolver for
`pending_approver_names` and for the names on the timeline.
- The fan-out calls it once, at `:3622`, before the loop, with both
people (`sub.from` and `sub.to`) of every hop.
- The chain removes duplicates, so an A → B → C chain is one `sys_user`
read for three people.
- With no substitution, the call has no targets and makes no read:
`readUserNames` returns before its `find`.
No resolver was added. `resolveUserNames` itself, its fallback and its
field-level-security note are unchanged. The record half
(`nameSubjectRecord`, from #23978) is untouched.
### What the body shows when there is no name, and whether that can be
an email
`sub.from` and `sub.to` are the hop's `delegator_id` and `delegate_id`
(`applyOooDelegation`). Both are `sys_user` lookups, so the stored value
is a user id, never an email. For each person, the body shows:
| what the chain reads | the body names the person as |
|:--|:--|
| an account row with a `name` | the name (`Olive Away`) |
| an account row with no `name` but with an `email` | **the email
address.** `readUserNames` labels such a row with `u.name ?? u.email`.
This is the same label the approvals door already serves for that
account, as `pending_approver_names` and on the timeline.
`sys_user.name` is declared `required: true`
(`packages/platform-objects/src/identity/sys-user.object.ts`), so only a
row written without a name reaches this case. |
| no account row for the id, or a failed `sys_user` read | the stored
user id, which is the text the body carries today. This is the rule the
spec's `ApprovalActionRow` states for every reader of this chain: when
no name is returned, "render the address". |
Each body goes to one party of the hop's delegation and names the other
party.
A body is a **stored** copy of the name: messaging stores it and the
inbox serves it. The `resolveUserNames` docblock warns, under "STORED
COPIES", that a later narrowing of names by `sys_user` field-level
security does not reach the copies stored in rows. It lists two columns,
and these two bodies are a third copy. The comment at the call site says
so. Adding the bodies to the docblock's list is outside this claim's
file surface, so it is an open question in the report (see Acceptance
notes).
## The audit comment keeps the ids, by design
The `ooo_substitute` row's `comment` (`:3629`, for example `u_ooo →
u_bob — leave`) is unchanged. These read it:
- **The approvals door.** `listActions` serves the comment as stored. It
resolves names per read for `actor_id`, `reassign_from` and
`reassign_to`. This row has no person in any of those fields (`actor_id`
is null).
- **The console.** The request timeline renders `a.comment` verbatim, in
quotes: `ApprovalsInboxPage.tsx` and `RecordApprovalsPanel.tsx` at the
console pin `f8267df93a`.
- **The list views.** `sys_approval_action`'s `recent` and `all_actions`
list views show the column, and an export of the object carries it.
- **The platform checklist.** The OOO item in
`docs/qa/platform-checklist/areas/approvals.json` verifies the row by
its comment reading `A → B`, the two identities.
This comment is the request's audit trail of identities. An id stays
true when a user is renamed. A name written into this row would be one
more copy that a later field-level-security narrowing cannot reach.
Where the platform wanted a hand-off's parties to be readable, it chose
structured fields with names resolved at read time: `reassign_from` /
`reassign_to` and their `*_name` companions replaced the `"FROM → TO"`
text that `reassign` used to write into its comment (spec
`ApprovalActionRow`). A readable OOO timeline line is a job for that
shape, not for this comment (see Acceptance notes).
## Premise check (`origin/main` `27d7c2aa1d`)
The premise holds.
- The bodies at `:3624` and `:3636` interpolated `sub.from` and `sub.to`
as raw ids.
- The audit comment is at `:3613`, and `resolveUserNames` is at `:6664`.
- `notify()` passes `payload.body` to `messaging.emit` unchanged. The
red run below shows the bodies as messaging received them.
## Pin
`approval-notification-body.integration.test.ts` is the battery that
drives every call site that notifies. It runs the real `ApprovalService`
over a real `ObjectQL` on `SqlDriver` (better-sqlite3, in memory), and
records what reaches the messaging surface. This PR adds one request and
two tests:
- **Request `ooo_gone`** is the control. Its approver `u_gone` has no
`sys_user` row and an active delegation to `u_alice`. So in one fan-out,
one person's name resolves and the other's cannot.
- **Test: both OOO bodies name people by display name.**
- It asserts the person's name in each body: `Olive Away` in
`ooo_substituted@ooo`, `Bob` in `ooo_skipped@ooo`, and `Alice` in
`ooo_skipped@ooo_gone`. It also asserts that the matching id is absent.
It does not assert the rest of the sentence.
- It then sweeps every OOO body the battery sent for any of the
battery's user ids.
- **Test: the fallback.** The `ooo_substituted@ooo_gone` body names
`u_gone`, so the slot holds the id, not `undefined` and not an empty
string.
The battery's coverage list gains the two `ooo_gone` sites.
Readings:
- **Red against the unfixed source** (test-only commit `225281a720`).
One test failed and six passed. The three offenders were the bodies as
sent: `You are covering an approval on Deal D_ooo while u_ooo is out of
office.`, `An approval on Deal D_ooo was routed to u_bob while you are
out of office.` and `An approval on Deal D_ooo_gone was routed to
u_alice while you are out of office.` The fallback pin was green before
the fix, because the id was the whole text then.
- **Green** at `26ca62ac57`: the battery passes 7/7.
- **Ablation 1** (`scripts/ablation-replace.mjs`, WRAP mode):
`oooPerson` returns the bare id, which puts the old text back in both
bodies. The anchor went from 1 hit to 0 and the blob from `05d5b39c94e8`
to `269142866144`. The name pin went red with exactly the three
offenders above, and the other six tests stayed green. The restore was
proven: the blob is `05d5b39c94e8`, equal to HEAD, and `git diff HEAD`
is empty.
- **Ablation 2**: `oooPerson` returns the chain's answer with no
fallback. The blob went from `05d5b39c94e8` to `c88096bc793b`. The
fallback pin went red with `You are covering an approval on Deal
D_ooo_gone while undefined is out of office.`, and the other six tests
stayed green. The restore was proven: the blob is `05d5b39c94e8`, equal
to HEAD, and `git diff HEAD` is empty.
- The test imports the subject relatively from `src/`, so neither
ablation has a `dist/` leg.
## Tests and gates (head `de3023f846`, after merging `origin/main`
`462d02f00a`)
- **Package tests:** `pnpm --filter @objectstack/plugin-approvals exec
vitest run --maxWorkers=2` passed 81 files / 1107 tests.
- **Typecheck:** `pnpm --filter @objectstack/plugin-approvals typecheck`
exits 0. `check:test-typecheck` holds the ledger unchanged at 8 files /
324 errors.
- **Derived gates:** `node scripts/pm/dispatch-gates.mjs --commands
--repo objectstack-ai/objectstack` derived 67 commands. I ran all 67 and
recorded each exit before any pipe. 65 exited 0 on the first pass.
- Two exited 3 (`PREREQUISITE NOT MET`): `pnpm
check:dual-build-cjs-loads` (some package had no `dist/`) and `pnpm
check:i18n` (the CLI build closure was missing).
- I built both prerequisites with the commands each gate prints. Turbo
answered from cache: 61/61 cached for the i18n closure, and 75/76 for
the whole workspace.
- On re-run, both exit 0. `check:i18n` reports all 10 packages' bundles
in sync. `check:dual-build-cjs-loads` reports 111 require entry points
across 69 packages load.
- **`dispatch-gates --ran`** over the final record: 67 derived, 67 run,
0 NOT-MEASURED, 0 UNRUN, exit 0. Over the first-pass record, it reports
65 run and 2 NOT-MEASURED, derived from the two exit 3 codes.
- **Changeset level axis**, measured offline: `node
scripts/check-changeset-no-major.mjs --base origin/main --event` with
this body exits 0 (`patch` under `Clause-②: no`).
- **CI-run families:** this PR's check-runs are the reading.
## Acceptance notes
- **The `resolveUserNames` "STORED COPIES" docblock** lists
`pending_approver_labels` and `acted_as_label` as the stored copies that
a field-level-security narrowing must follow. These two bodies are now a
third. The call site records this, and the docblock does not. The claim
excluded `resolveUserNames` itself, so the one-line addition is left to
the seat.
- **The OOO row on the console timeline.** At the console pin, the
timeline maps no label for `ooo_substitute`, so it shows the raw kind
`ooo_substitute`, followed by the comment `"u_ooo → u_bob — leave"`.
This is a reading of the source only, not measured in a running console.
The structured-parties shape that `reassign` uses is the fix pattern
that preserves the audit comment. Carrier: none.
- **A delegation chain.** In an A → B → C chain, B is told they are
covering for A, although B is also away and the slot went on to C. That
is the existing per-hop design, and this PR does not change it.
---
_Generated by [Claude
Code](https://claude.ai/code/session_018VqJPGA1NDNSCZsnecsChP)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 265ef64 commit 544b8d7
3 files changed
Lines changed: 95 additions & 4 deletions
File tree
- .changeset
- packages/plugins/plugin-approvals/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
52 | 62 | | |
53 | 63 | | |
54 | 64 | | |
| |||
80 | 90 | | |
81 | 91 | | |
82 | 92 | | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
83 | 100 | | |
84 | 101 | | |
85 | 102 | | |
| |||
238 | 255 | | |
239 | 256 | | |
240 | 257 | | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
241 | 267 | | |
242 | 268 | | |
243 | 269 | | |
| |||
255 | 281 | | |
256 | 282 | | |
257 | 283 | | |
258 | | - | |
259 | | - | |
| 284 | + | |
| 285 | + | |
260 | 286 | | |
261 | 287 | | |
262 | 288 | | |
| |||
266 | 292 | | |
267 | 293 | | |
268 | 294 | | |
| 295 | + | |
| 296 | + | |
269 | 297 | | |
270 | 298 | | |
271 | 299 | | |
| |||
327 | 355 | | |
328 | 356 | | |
329 | 357 | | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
330 | 396 | | |
331 | 397 | | |
332 | 398 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3605 | 3605 | | |
3606 | 3606 | | |
3607 | 3607 | | |
| 3608 | + | |
| 3609 | + | |
| 3610 | + | |
| 3611 | + | |
| 3612 | + | |
| 3613 | + | |
| 3614 | + | |
| 3615 | + | |
| 3616 | + | |
| 3617 | + | |
| 3618 | + | |
| 3619 | + | |
| 3620 | + | |
| 3621 | + | |
| 3622 | + | |
| 3623 | + | |
3608 | 3624 | | |
3609 | 3625 | | |
3610 | 3626 | | |
| |||
3621 | 3637 | | |
3622 | 3638 | | |
3623 | 3639 | | |
3624 | | - | |
| 3640 | + | |
3625 | 3641 | | |
3626 | 3642 | | |
3627 | 3643 | | |
| |||
3633 | 3649 | | |
3634 | 3650 | | |
3635 | 3651 | | |
3636 | | - | |
| 3652 | + | |
3637 | 3653 | | |
3638 | 3654 | | |
3639 | 3655 | | |
| |||
0 commit comments