From bf81cfa74bf9cf5288b9f1acc3da01985be44995 Mon Sep 17 00:00:00 2001 From: os-zhuang Date: Thu, 11 Jun 2026 21:08:54 +0500 Subject: [PATCH] =?UTF-8?q?feat(approvals):=20display=20contract=20v2=20?= =?UTF-8?q?=E2=80=94=20no=20raw=20identifiers=20anywhere?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A business reviewer of the revamped inbox still met raw ids in three places: lookup foreign keys inside the payload summary (e.g. an account shown as dpOfPMy7cbeEL1jk), user-id approvers in the waiting-on chips, and actor ids in the audit timeline. Extend the enrichment pass: - payload_display — lookup/master_detail fields in the snapshot resolve to the referenced record's display title (schema `reference`, batched one query per referenced object). - pending_approver_names — user-id approvers resolve via sys_user (id or email); role: literals stay as-is, already readable. - object_label — the target object's schema label rides the row. - listActions rows carry actor_name so the timeline never shows an id. Co-Authored-By: Claude Fable 5 --- .../src/approval-service.test.ts | 31 ++++ .../plugin-approvals/src/approval-service.ts | 163 ++++++++++++++---- .../spec/src/contracts/approval-service.ts | 15 ++ 3 files changed, 180 insertions(+), 29 deletions(-) diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 065a2e4850..b1191c471c 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -376,6 +376,37 @@ describe('ApprovalService (node era)', () => { expect(rows[0].record_title).toBe('Snapshot Title'); }); + it('enrichment resolves lookup foreign keys in the payload to record titles', async () => { + (engine as any).getSchema = (name: string) => + name === 'opportunity' + ? { label: 'Opportunity', fields: { name: {}, account: { type: 'lookup', reference: 'account' } } } + : name === 'account' ? { label: 'Account', fields: { name: {} } } : undefined; + engine._tables['opportunity'] = [{ id: 'opp1', name: 'Acme Renewal', account: 'acc1' }]; + engine._tables['account'] = [{ id: 'acc1', name: 'Acme Corp' }]; + await svc.openNodeRequest(openInput(['u9'], { record: { id: 'opp1', name: 'Acme Renewal', account: 'acc1' } }), CTX); + const rows = await svc.listRequests({ status: 'pending' }, SYS); + expect(rows[0].object_label).toBe('Opportunity'); + expect(rows[0].payload_display).toEqual({ account: 'Acme Corp' }); + }); + + it('enrichment maps user-id approvers to display names', async () => { + engine._tables['sys_user'] = [{ id: 'u9', name: 'Grace Hopper', email: 'grace@example.com' }]; + await svc.openNodeRequest(openInput(['u9']), CTX); + const rows = await svc.listRequests({ status: 'pending' }, SYS); + expect(rows[0].pending_approver_names).toEqual({ u9: 'Grace Hopper' }); + }); + + it('listActions resolves actor display names', async () => { + engine._tables['sys_user'] = [ + { id: 'u1', name: 'Ada Lovelace', email: 'ada@example.com' }, + { id: 'u9', name: 'Grace Hopper', email: 'grace@example.com' }, + ]; + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS); + const actions = await svc.listActions(req.id, SYS); + expect(actions.map(a => (a as any).actor_name)).toEqual(['Ada Lovelace', 'Grace Hopper']); + }); + it('enrichment resolves an email submitter via sys_user.email', async () => { engine._tables['sys_user'] = [{ id: 'u7', name: 'Grace Hopper', email: 'grace@example.com' }]; await svc.openNodeRequest(openInput(['u9'], { submitterId: 'grace@example.com' }), CTX); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index 784fc14fd7..19189028b3 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -593,15 +593,66 @@ export class ApprovalService implements IApprovalService { } /** - * Attach inbox display fields (`record_title`, `submitter_name`) to rows. - * Batched: one query per distinct target object plus one `sys_user` lookup. - * Best-effort — a deleted record falls back to the payload snapshot, and a - * lookup failure leaves the field unset rather than failing the list. + * Batch-resolve `sys_user` display names for identifiers that may be user + * ids or emails. Best-effort — failures leave entries unresolved. + */ + private async resolveUserNames(identifiers: Array): Promise> { + const names = new Map(); + const targets = Array.from(new Set(identifiers.filter(Boolean))) as string[]; + if (!targets.length) return names; + try { + const users = await this.engine.find('sys_user', { + where: { id: { $in: targets } }, fields: ['id', 'name', 'email'], + limit: targets.length, context: SYSTEM_CTX, + }); + for (const u of (users ?? []) as any[]) { + if (u?.id && (u.name || u.email)) names.set(String(u.id), String(u.name ?? u.email)); + } + } catch { /* best-effort */ } + const unresolvedEmails = targets.filter(t => !names.has(t) && t.includes('@')); + if (unresolvedEmails.length) { + try { + const users = await this.engine.find('sys_user', { + where: { email: { $in: unresolvedEmails } }, fields: ['email', 'name'], + limit: unresolvedEmails.length, context: SYSTEM_CTX, + }); + for (const u of (users ?? []) as any[]) { + if (u?.email && u.name) names.set(String(u.email), String(u.name)); + } + } catch { /* best-effort */ } + } + return names; + } + + /** Lookup-typed fields (key + referenced object) of an object's schema. */ + private resolveLookupFields(object: string): Array<{ key: string; reference: string }> { + try { + const schema: any = (this.engine as any).getSchema?.(object); + const fields = schema?.fields ?? {}; + const out: Array<{ key: string; reference: string }> = []; + for (const [key, f] of Object.entries(fields)) { + if ((f?.type === 'lookup' || f?.type === 'master_detail') && f?.reference) { + out.push({ key, reference: String(f.reference) }); + } + } + return out; + } catch { return []; } + } + + /** + * Attach inbox display fields to rows so clients never render a raw + * identifier: `record_title`, `submitter_name`, `object_label`, + * `pending_approver_names` (user-id approvers), and `payload_display` + * (lookup foreign keys in the snapshot → referenced record titles). + * Batched: one query per distinct object (target + referenced) plus one + * `sys_user` lookup. Best-effort — a deleted record falls back to the + * payload snapshot, and any failure leaves the field unset rather than + * failing the list. */ private async enrichRows(rows: ApprovalRequestRow[]): Promise { if (!rows.length) return; - // Record titles, batched per object. + // Record titles + object labels, batched per object. const byObject = new Map>(); for (const r of rows) { if (!r.object_name || !r.record_id) continue; @@ -610,7 +661,12 @@ export class ApprovalService implements IApprovalService { set.add(r.record_id); } const titles = new Map(); + const objectLabels = new Map(); for (const [object, idSet] of byObject) { + try { + const schema: any = (this.engine as any).getSchema?.(object); + if (schema?.label) objectLabels.set(object, String(schema.label)); + } catch { /* label optional */ } const ids = Array.from(idSet); const displayField = this.resolveDisplayField(object); try { @@ -619,44 +675,83 @@ export class ApprovalService implements IApprovalService { }); for (const rec of (recs ?? []) as any[]) { const title = ApprovalService.pickTitle(rec, displayField); - if (rec?.id && title) titles.set(`${object}${rec.id}`, title); + if (rec?.id && title) titles.set(`${object} ${rec.id}`, title); } } catch { /* object may be unregistered — payload fallback below */ } } - // Submitter display names — submitter_id may be a user id or an email. - const submitters = Array.from(new Set(rows.map(r => r.submitter_id).filter(Boolean))) as string[]; - const names = new Map(); - if (submitters.length) { + // Lookup foreign keys inside payload snapshots → referenced record titles. + const lookupFieldsByObject = new Map>(); + for (const object of byObject.keys()) { + const lookups = this.resolveLookupFields(object); + if (lookups.length) lookupFieldsByObject.set(object, lookups); + } + const refIds = new Map>(); + for (const r of rows) { + const lookups = lookupFieldsByObject.get(r.object_name); + const payload: any = r.payload; + if (!lookups || !payload || typeof payload !== 'object') continue; + for (const { key, reference } of lookups) { + const v = payload[key]; + if (v == null || typeof v === 'object' || !String(v).trim()) continue; + let set = refIds.get(reference); + if (!set) { set = new Set(); refIds.set(reference, set); } + set.add(String(v)); + } + } + const refTitles = new Map(); + for (const [object, idSet] of refIds) { + const ids = Array.from(idSet); + const displayField = this.resolveDisplayField(object); try { - const users = await this.engine.find('sys_user', { - where: { id: { $in: submitters } }, fields: ['id', 'name', 'email'], - limit: submitters.length, context: SYSTEM_CTX, + const recs = await this.engine.find(object, { + where: { id: { $in: ids } }, limit: ids.length, context: SYSTEM_CTX, }); - for (const u of (users ?? []) as any[]) { - if (u?.id && (u.name || u.email)) names.set(String(u.id), String(u.name ?? u.email)); + for (const rec of (recs ?? []) as any[]) { + const title = ApprovalService.pickTitle(rec, displayField); + if (rec?.id && title) refTitles.set(`${object} ${rec.id}`, title); } - } catch { /* best-effort */ } - const unresolvedEmails = submitters.filter(s => !names.has(s) && s.includes('@')); - if (unresolvedEmails.length) { - try { - const users = await this.engine.find('sys_user', { - where: { email: { $in: unresolvedEmails } }, fields: ['email', 'name'], - limit: unresolvedEmails.length, context: SYSTEM_CTX, - }); - for (const u of (users ?? []) as any[]) { - if (u?.email && u.name) names.set(String(u.email), String(u.name)); - } - } catch { /* best-effort */ } + } catch { /* referenced object unreadable — leave unresolved */ } + } + + // Display names for submitters AND user-id approvers in one lookup. + // `role:` (and other `type:value` literals) are already readable. + const userIdentifiers: Array = []; + for (const r of rows) { + userIdentifiers.push(r.submitter_id); + for (const a of r.pending_approvers ?? []) { + if (a && !a.includes(':')) userIdentifiers.push(a); } } + const names = await this.resolveUserNames(userIdentifiers); for (const r of rows as any[]) { - const title = titles.get(`${r.object_name}${r.record_id}`) + const title = titles.get(`${r.object_name} ${r.record_id}`) ?? ApprovalService.pickTitle(r.payload, undefined); if (title) r.record_title = title; const name = r.submitter_id ? names.get(String(r.submitter_id)) : undefined; if (name) r.submitter_name = name; + const label = objectLabels.get(r.object_name); + if (label) r.object_label = label; + + const approverNames: Record = {}; + for (const a of r.pending_approvers ?? []) { + const n = names.get(String(a)); + if (n) approverNames[a] = n; + } + if (Object.keys(approverNames).length) r.pending_approver_names = approverNames; + + const lookups = lookupFieldsByObject.get(r.object_name); + if (lookups && r.payload && typeof r.payload === 'object') { + const display: Record = {}; + for (const { key, reference } of lookups) { + const v = (r.payload as any)[key]; + if (v == null) continue; + const t = refTitles.get(`${reference} ${String(v)}`); + if (t) display[key] = t; + } + if (Object.keys(display).length) r.payload_display = display; + } } } @@ -741,6 +836,16 @@ export class ApprovalService implements IApprovalService { orderBy: [{ field: 'created_at', direction: 'asc' }], context: SYSTEM_CTX, }); - return Array.isArray(rows) ? rows.map(rowFromAction) : []; + const actions = Array.isArray(rows) ? rows.map(rowFromAction) : []; + // Timeline display: resolve actor ids to names so the audit trail never + // shows a raw identifier. Role/team literals are already readable. + const names = await this.resolveUserNames( + actions.map(a => a.actor_id).filter(id => id && !id.includes(':')), + ); + for (const a of actions as any[]) { + const n = a.actor_id ? names.get(String(a.actor_id)) : undefined; + if (n) a.actor_name = n; + } + return actions; } } diff --git a/packages/spec/src/contracts/approval-service.ts b/packages/spec/src/contracts/approval-service.ts index 9054d316fe..e8d6d3a63b 100644 --- a/packages/spec/src/contracts/approval-service.ts +++ b/packages/spec/src/contracts/approval-service.ts @@ -57,6 +57,19 @@ export interface ApprovalRequestRow { record_title?: string; /** Display name of the submitter (`sys_user.name`), when resolvable. */ submitter_name?: string; + /** Schema label of the target object (e.g. "Project" for `showcase_project`). */ + object_label?: string; + /** + * Display names for user-id entries in `pending_approvers` + * (id → `sys_user.name`). Emails and `role:` entries are not mapped — + * they are already human-readable. + */ + pending_approver_names?: Record; + /** + * Display values for lookup fields in `payload` (field key → referenced + * record's display name), so inbox summaries never show foreign-key ids. + */ + payload_display?: Record; } /** Audit row. */ @@ -69,6 +82,8 @@ export interface ApprovalActionRow { actor_id?: string; comment?: string; created_at?: string; + /** Display name of the actor (`sys_user.name`), when resolvable. */ + actor_name?: string; } /** Input for a decision on an approval request. */