Skip to content

Commit b75129f

Browse files
fix(plugin-approvals): an approval request record title reads the flow label (process_label), falling back to process_name (#24085)
Fixes #24007 Clause-②: no The request's computed record title reads the flow's label in place of `flow:NAME`. No export, type, accept set or served column moves. ## What changed - **`sys_approval_request.display_title`**, the object's `nameField`, is now `record.process_label != null ? record.process_label + ' · ' + record.record_id : record.process_name + ' · ' + record.record_id`. - A request opened by a flow labelled "Discount Sign-off" is titled `Discount Sign-off · opp_45`. - A row with no stored label keeps `flow:deal_desk · opp_42`, never `null · …`. - The guard is the idiom other platform objects already use (`sys-approval-action.object.ts`, `sys-automation-run.object.ts`, `sys-member.object.ts`). - **The field comment** that said no null guard is needed now says why one is. - **`titleFormat` keeps `'{process_name} · {record_id}'`** (see below). The comment above `nameField` says why, and names the renderer that reads it first. - **`sys-approval-display-title.test.ts`**: the request case is now two pins, and the pairing is restated (below). - **One `patch` changeset** for `@objectstack/plugin-approvals`. No field description changed, so the translation bundles are not regenerated, and Acceptance 4 does not apply. `check:i18n` reading: see Gates. ## Premise, measured first (Acceptance 2's question) On `origin/main` `b6805ac0f6`: - `display_title` read `record.process_name + ' · ' + record.record_id`. - `titleFormat` was `'{process_name} · {record_id}'`. - `process_label` is `required: false`. Can `process_label` still be null after the open write and the boot fill? **Yes.** Measured on a real `ObjectQL` over `SqlDriver` (better-sqlite3, in memory), with a throwaway probe that was not committed: - **A failed fill run leaves every unfilled row null.** `backfillApprovalDisplayLabels` throws on a failed read. In the probe, a `find` that refuses gives `threw: "datasource refused"`, and the row stays `process_label: null`. - `approvals-plugin.ts` logs that at `warn` and retries on the next boot. Its own message says the cells "stay blank until a later boot fills them". - The fill also returns without running when the approval service is absent (`if (!svc) return`). - It runs on `kernel:ready`, so rows are readable before it finishes. - **A row the chain cannot label stays null on every boot.** Take a request whose `process_name` humanizes to nothing (`flow:`) and whose config snapshot has no `__flowLabel`. The engine accepts that row: `required` refuses only `''`. - The first fill wrote 1 of the 2 seeded rows (`requestsFilled: 1`). The `flow:` row kept `process_label: null`. - A second fill wrote 0 rows. - **The open write** stores `labelOrMachineName(flowLabel, processName) ?? null`. That is null only when `flow:` plus `flowName ?? nodeId` humanizes to nothing: `labelOrMachineName(undefined, 'flow:')` is `undefined`. So `'{process_label} · {record_id}'` is not safe, and `titleFormat` keeps `{process_name}`. ## Which renderers read `titleFormat` first - **The global search palette: `searchAll` in `packages/metadata-protocol/src/protocol.ts`.** Its `renderTitle` tries `titleFormat` before `nameField`. - `sys_approval_request` is in its sweep. `canServeApiOperation(enable, 'search')` is true, and the auto-default search fields include `process_name` and `record_id`. - Measured through `searchAll` on this branch at `1afade209a`: a row labelled "Discount Sign-off" gets the palette title `flow:deal_desk · opp_45`. Its `nameField` value is `Discount Sign-off · opp_45`. - Measured on `main` with the template swapped to `{process_label}`: a row with no label gets the palette title `· opp_43`, with no source at all. - **objectui's title readers rank `nameField` above `titleFormat`** (ADR-0079 order), read at objectui `daeb77f`. That covers `@object-ui/core` `getRecordDisplayName`, the record page H1 in `containers.tsx`, and `LookupField`. - So the record page's H1 and breadcrumb, and the Action History list's `request_id` cell, read `display_title`. ## The pairing, restated `sys-approval-display-title.test.ts` held the formula and `titleFormat` to the same text. They now name different columns, so the file asserts: - **With no label stored,** both draw `{process_name} · {record_id}`. This is the old assertion, kept, plus a check that `process_label` is null on the row. - **With a label stored,** the formula draws `{process_label} · {record_id}`, and `resolveRecordTitle` agrees. `titleFormat` draws `{process_name} · {record_id}`. ## Tests All runs went through `scripts/pm/os-verify-lock.sh`. Each verdict below is the lock's own `VERDICT command-exit` line. - **The pin file at `1afade209a`:** `pnpm --filter @objectstack/plugin-approvals exec vitest run --maxWorkers=2 src/sys-approval-display-title.test.ts` gives `Tests 6 passed (6)`, exit 0. - **The whole package at the final head `ff2cf67c91`**, after merging `origin/main` `8133cc7805`, which brought #24064's `approval-service.ts` change into this package: - `vitest run --maxWorkers=2` gives `Test Files 81 passed (81)` and `Tests 1105 passed (1105)`. - Then `pnpm --filter @objectstack/plugin-approvals run typecheck` runs `tsc --noEmit` on both configs, then `check:test-typecheck: OK`, with 27 pinned signatures held. - Exit 0 for the whole chain. - **The same package run before the merge, at `1afade209a`:** 81 files and 1104 tests passed. Typecheck exit 0. ## Ablation (one-shot, nothing committed) The test subject is imported by relative path (`./sys-approval-request.object.js`), so vitest reads `src/`. No `dist/` is involved, and no rebuild or dist preflight applies. Each leg went through `scripts/ablation-replace.mjs` in wrap mode, with a shell `trap` restore on top. Every restore was proven: the blob equals the HEAD blob `8ac394bf6ba2`, and `git diff HEAD` is empty. - **Leg A: drop the label branch.** The formula goes back to `record.process_name + ' · ' + record.record_id`. Anchor 1 → 0, blob `8ac394bf6ba2` → `69cfd16dcea2`. - The authored-label pin goes red: `expected 'flow:deal_desk · opp_45' to be 'Discount Sign-off · opp_45'`, 1 failed and 5 passed. - **Leg B: drop the null guard** (`record.process_label != null ? ` becomes `true ? `). Anchor 1 → 0, replacement 0 → 1, blob → `a5aee4f1c4ed`. - The null-fallback pin goes red: `expected null to be 'flow:deal_desk · opp_42'`, 1 failed and 5 passed. - The direction differs from the card's "not `null · …`" framing. CEL refuses `null + string`, so the unguarded formula yields a null title, not the text `null · …`. Either way the pin catches it. - **A first leg-A attempt was a no-op.** Its replacement text already occurred inside the anchor, so `ablation-replace` refused it (replacement count 1 → 1) and restored, with no test run. It was redone with `--delete` on the label branch. ## Gates **Local set.** `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` at the final head `ff2cf67c91` derives 65 commands. All 65 were run there, each exit captured before any pipe, and all 65 exited 0. **Reconciliation.** `--ran` (each line records `:: exit N`) reports `65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN`, and the zero is derived from the recorded codes. **Verdict lines worth naming:** - `check-i18n-bundles: OK (10 package(s) — all bundles in sync …)`, with `plugins/plugin-approvals in sync (7 bundle(s))`. - `check-adr-0087-registration: this PR adds no declared-breaking changeset`. - `No empty-frontmatter changeset introduced by this diff`. - `check-nul-bytes: OK`. - `check:type-check-debt … none above its recorded number`. - `check:cross-package-test-inputs OK`. - `check-test-source-alias OK`. **A first union at `1afade209a` had two exit-3 results.** `check:i18n` and `check:dual-build-cjs-loads` reported `PREREQUISITE NOT MET`, meaning nothing was measured. Both were measured green in the final union, after the prerequisite build. **Not run locally (CI's runs):** `test:repo`, the type-check lanes, the path-scheduled CI jobs and the roster families the derivation names. No exported symbol was renamed or removed, so `check-platform-checklist` was not owed. `origin/main` moved to `8a83cebc15` after the merge. That is one TSDoc-only commit, in `packages/spec/src/contracts/http-server.ts`, touching nothing this PR reads, so it was not merged in. ## Acceptance notes - **The palette still shows the machine source.** The global search palette ranks `titleFormat` above `nameField`, which the spec's own `titleFormat` description contradicts ("an explicit nameField now takes precedence", `packages/spec/src/data/object.zod.ts`). So on this branch the palette still titles a labelled request `flow:deal_desk · opp_45`. That is the renderer named above, and the cost of Acceptance 2's second arm. It is reported to the seat as a finding, not fixed here: `metadata-protocol` is outside this card's surface. - **An underscore-only flow name stores `''` as `process_label`.** `prettifyMachineName` returns `''`, not `undefined`, for such a name (`flow:_`); `FlowSchema.name`'s pattern admits `_`. With an empty authored label, the open write stores `''`, which the new `!= null` guard reads as a label. Measured on the chain function only (`labelOrMachineName('', 'flow:_')` is `''`). No real producer names a flow that way. Carrier: none. - **The changeset says "action history list".** The Action History list's `request_id` cell draws this title through the lookup's target `nameField`. That was read from the declaration, not measured on screen. --- _Generated by [Claude Code](https://claude.ai/code/session_018VqJPGA1NDNSCZsnecsChP)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 051aea6 commit b75129f

3 files changed

Lines changed: 60 additions & 10 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/plugin-approvals': patch
3+
---
4+
5+
Clause-②: no
6+
7+
fix(plugin-approvals): an approval request's record title reads the flow's label, not `flow:NAME`
8+
9+
- **`sys_approval_request.display_title` reads `process_label`.** The title is now the flow's label and the record id (`Discount Sign-off · opp_45`), the same label the request lists show. A row with no label stored falls back to `process_name` (`flow:deal_desk · opp_42`), so the title never reads `null · …`. Every surface that draws the request's title through `nameField` shows it: the record page's heading and breadcrumb, and the request cell of the action history list.
10+
- **`titleFormat` still names `{process_name}`.** A placeholder template cannot fall back, and `process_label` can be null after the boot fill (a row it could not label, or a failed fill run). A renderer that reads `titleFormat` ahead of `nameField`, such as the global search palette, still shows the machine source.
11+
12+
A patch: one formula on one platform object's computed title. No export, type, accepted value or stored column changes.

‎packages/plugins/plugin-approvals/src/sys-approval-display-title.test.ts‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,14 @@
2222
*
2323
* Where a source column is nullable, a row without it is titled by the other
2424
* column rather than failing to evaluate.
25+
*
26+
* `sys_approval_request` is the one object whose formula and `titleFormat`
27+
* name different columns (#24007): the formula's flow half is the flow's label
28+
* (`process_label`, nullable), with `process_name` as its fallback, while
29+
* `titleFormat` keeps `{process_name}` because a template cannot express that
30+
* fallback. So for that object point 2 is asserted where they agree — a row
31+
* with no stored label, where both draw `process_name` — and a row with a
32+
* label pins the formula's own text and the template's difference from it.
2533
*/
2634

2735
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
@@ -90,7 +98,26 @@ describe('[#20015] approval objects resolve a real record title under ADR-0079 o
9098
}
9199
});
92100

93-
it('sys_approval_request: the H1 is "{process_name} · {record_id}", not the id', async () => {
101+
it('sys_approval_request: with a flow label stored, the H1 is "{process_label} · {record_id}"', async () => {
102+
// An authored label that is not the humanized machine name ("Deal Desk"),
103+
// so the title can only have come from the stored label.
104+
const created = await engine.insert('sys_approval_request', {
105+
process_name: 'flow:deal_desk', process_label: 'Discount Sign-off',
106+
object_name: 'opportunity', record_id: 'opp_45', status: 'pending',
107+
}, SYS);
108+
expect(created.display_title).toBe('Discount Sign-off · opp_45');
109+
const row = await engine.findOne('sys_approval_request', { where: { id: created.id } }, SYS);
110+
expect(row).not.toBeNull();
111+
112+
expect(h1Of(SysApprovalRequest, row!)).toBe('Discount Sign-off · opp_45');
113+
expect(h1Of(SysApprovalRequest, row!)).not.toBe(row!.id);
114+
expect(resolveRecordTitle(SysApprovalRequest, storedOnly(row!))).toBe('Discount Sign-off · opp_45');
115+
// The pairing, restated: `titleFormat` keeps the machine source, so a
116+
// renderer that reads it first draws a different text for this row.
117+
expect(renderTitleFormat(SysApprovalRequest.titleFormat, row!)).toBe('flow:deal_desk · opp_45');
118+
});
119+
120+
it('sys_approval_request: with no flow label stored, the H1 is "{process_name} · {record_id}", never "null · …"', async () => {
94121
const created = await engine.insert('sys_approval_request', {
95122
process_name: 'flow:deal_desk', object_name: 'opportunity', record_id: 'opp_42',
96123
status: 'pending',
@@ -99,6 +126,7 @@ describe('[#20015] approval objects resolve a real record title under ADR-0079 o
99126
expect(created.display_title).toBe('flow:deal_desk · opp_42');
100127
const row = await engine.findOne('sys_approval_request', { where: { id: created.id } }, SYS);
101128
expect(row).not.toBeNull();
129+
expect(row!.process_label ?? null).toBeNull();
102130

103131
expect(h1Of(SysApprovalRequest, row!)).toBe('flow:deal_desk · opp_42');
104132
expect(h1Of(SysApprovalRequest, row!)).not.toBe(row!.id);

‎packages/plugins/plugin-approvals/src/sys-approval-request.object.ts‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -45,12 +45,19 @@ export const SysApprovalRequest = ObjectSchema.create({
4545
isSystem: true,
4646
managedBy: 'engine-owned',
4747
description: 'Live approval instance tracked per submission',
48-
// [ADR-0079] The record title is `display_title`, a text formula over the
49-
// same two columns `titleFormat` names. The pointer used to be `id`: once a
50-
// renderer honours ADR-0079's order (an explicit `nameField` wins over
51-
// `titleFormat`), that made the record page's H1 the raw id. `titleFormat`
52-
// stays for renderers that still read it first;
53-
// `sys-approval-display-title.test.ts` holds the two to the same text.
48+
// [ADR-0079] The record title is `display_title`, a text formula. The
49+
// pointer used to be `id`: once a renderer honours ADR-0079's order (an
50+
// explicit `nameField` wins over `titleFormat`), that made the record page's
51+
// H1 the raw id. The formula reads the flow's label (`process_label`) and
52+
// falls back to the machine source (`process_name`) where no label is
53+
// stored. `titleFormat` stays for renderers that still read it first — the
54+
// global search palette (`searchAll` in `@objectstack/metadata-protocol`)
55+
// ranks it above `nameField` — and keeps `{process_name}`: a placeholder
56+
// template cannot say "else", and `process_label` can be null after the boot
57+
// fill (a row the fill could not label, or a failed fill run), where a
58+
// `{process_label}` template draws a title with no source at all.
59+
// `sys-approval-display-title.test.ts` holds the two to the same text where
60+
// no label is stored, and pins where they differ.
5461
displayNameField: 'display_title',
5562
nameField: 'display_title', // [ADR-0079] canonical primary-title pointer (mirrors deprecated displayNameField)
5663
titleFormat: '{process_name} · {record_id}',
@@ -128,12 +135,15 @@ export const SysApprovalRequest = ObjectSchema.create({
128135
id: Field.text({ label: 'Request ID', required: true, readonly: true, group: 'System' }),
129136

130137
// [ADR-0079] The record title (`nameField` above). A formula is computed on
131-
// read and has no stored column. Both source columns are required, so the
132-
// expression needs no null guard.
138+
// read and has no stored column. Its flow half is the flow's label,
139+
// `process_label`, the same column the request lists show. That column is
140+
// nullable — a row the boot fill could not label, or one written before
141+
// the fill ran — so the expression guards it and falls back to
142+
// `process_name`, which is required, as is `record_id`.
133143
display_title: Field.formula({
134144
label: 'Title',
135145
returnType: 'text',
136-
expression: F`record.process_name + ' · ' + record.record_id`,
146+
expression: F`record.process_label != null ? record.process_label + ' · ' + record.record_id : record.process_name + ' · ' + record.record_id`,
137147
description: 'Record title: the request source and the record it is about (computed on read)',
138148
group: 'Target',
139149
}),

0 commit comments

Comments
 (0)