diff --git a/.changeset/duplicate-banner-one-per-verdict.md b/.changeset/duplicate-banner-one-per-verdict.md new file mode 100644 index 00000000..9d205c5d --- /dev/null +++ b/.changeset/duplicate-banner-one-per-verdict.md @@ -0,0 +1,45 @@ +--- +'hotcrm': patch +--- + +Split the lead detail page's duplicate banner into **one `record:alert` per +verdict**, so each one states the next step its verdict actually has. + +### One banner could not state either next step + +#1207 gated a single banner on `duplicate_status == "suspected"`; #1289 widened +it to every verdict the field carries. Widening was right, but a `record:alert` +carries one `visible` and one title/body pair, and `pickLocalized` picks by +LANGUAGE, not by row — so one component covering both verdicts had to choose +copy that named **neither**, or it would have mislabelled every lead in the +other state. + +Since #1288 the two verdicts have opposite next steps: + +| verdict | at conversion | what the rep should do | +|:--|:--|:--| +| `suspected` | warns, conversion **proceeds** | compare against the linked record, then convert or disqualify | +| `confirmed` | **refused** (`refuse_confirmed_duplicate`) | cannot convert — disqualify, naming the survivor | + +So the neutral banner announced that something was wrong without saying what to +do, and the rep had to scroll to the Duplicate Status chip to find out which +situation they were in. The `confirmed` case was worse than that: nothing on the +record said the Convert button would refuse them, so they learned it by pressing +it. + +### What ships + +Two components, two predicates, two next steps, in all four locales. The +`confirmed` banner is the only place a rep is warned about the refusal before +they press Convert, and it ships at `error` severity — which the renderer maps +to `role="alert"` / `aria-live="assertive"` rather than the polite `role= +"status"` every other level gets. `suspected` stays `warning`, because +conversion still goes through. + +A lead carrying a value neither option declares now raises **no** banner, where +the widened predicate raised the neutral one. That matches what the conversion +flow already does with such a row — its `e22` Clean edge converts it — so the +page and the flow now agree about the same lead. + +⛔ Nothing here changes what the app refuses. That was ruled by #1288 and +shipped by PR #1555; this card changes only what the record page tells the rep. diff --git a/src/pages/lead_detail.page.ts b/src/pages/lead_detail.page.ts index f7bb4f4a..a874f4e9 100644 --- a/src/pages/lead_detail.page.ts +++ b/src/pages/lead_detail.page.ts @@ -90,30 +90,47 @@ export const LeadDetailPage: Page = { ], }, }, - // Duplicate banner (#1207, widened to every verdict by #1289). + // Duplicate banners — ONE PER VERDICT (#1207 · widened by #1289 · + // split by #1628). // // `lead_duplicate_check` (lead.hook.ts, job 2) already writes // `duplicate_status: 'suspected'` and links the record the lead repeats // — the flag existed, and this page never read it. A rep opened a // flagged lead, saw a page identical to a clean one, and converted it - // into a second account, contact and opportunity. This banner and the + // into a second account, contact and opportunity. These banners and the // `duplicates` section on the Details tab are the record-page half of // that fix — the banner is the alarm, the section is the link to // compare against; the conversion screen carries the other half. // - // ## Why it covers `confirmed` too (#1289) + // ## Why there are TWO of them (#1628) // - // #1207 shipped this gated on `== "suspected"` alone, which left the - // STRONGER state silent: a `confirmed` duplicate is a person's verdict, - // and it got no banner at all. That narrowness was a ruling, not an - // oversight in the code, and #1289 is the ruling being corrected. + // #1207 shipped one banner gated on `== "suspected"`, which left the + // STRONGER state silent. #1289 widened the predicate to every verdict + // and, being one component, had to pick copy that named NEITHER state: + // a `record:alert` carries a single `visible` and a single title/body + // pair, and `pickLocalized` picks by LANGUAGE, not by row — so naming + // one verdict would have mislabelled every lead in the other. // - // Since #1288 the gap also changed shape. `confirmed` is now the state - // on which the conversion flow REFUSES outright - // (`lead-conversion.flow.ts`, `refuse_confirmed_duplicate`), so a - // silent banner meant the record page said nothing about the one fact - // that stops the rep's next click — they pressed Convert and met a - // refusal dialog with no warning on the record behind it. + // That was correct for one banner, and it is why one banner is not + // enough. Since #1288 the two verdicts have OPPOSITE next steps: + // + // suspected — the intake hook's guess. Conversion PROCEEDS; the rep + // should compare against the linked record first. + // confirmed — a reviewer's verdict. Conversion is REFUSED outright + // (`lead-conversion.flow.ts`, `refuse_confirmed_duplicate`). + // + // One sentence cannot state either without being false for the other, + // so it stated neither, and the rep had to scroll to the Duplicate + // Status chip to learn which situation they were in. A banner that + // announces something is wrong but not what to do is not doing the one + // job a banner has. Two components, two predicates, two next steps. + // + // ⚠️ Two sibling `record:alert` nodes really do BOTH render: a region + // renders as `components.map((node, i) => )` — read out of the shipped console bundle at the + // `.objectui-sha` pin — so each is mounted separately and evaluates its + // own `visible` against the same row. Their `id`s are their React keys, + // which is why the two ids differ rather than sharing one. // // ⚠️ `visible` is the ONE record component whose PROPS carry a real row // predicate: `record-alert.tsx` evaluates `properties.visible` through @@ -126,22 +143,34 @@ export const LeadDetailPage: Page = { // ⚠️ `has()` is load-bearing, and this surface is the WORST of the four // this repo measures (cf. `test/view-predicate-dialect.test.ts`): the // renderer's call site is FAIL-SOFT — an unevaluable predicate answers - // SHOWN. So a bare `record.duplicate_status != null` would abort with - // `No such key` on every clean lead whose driver omits the column + // SHOWN. So a bare `record.duplicate_status == "suspected"` would abort + // with `No such key` on every clean lead whose driver omits the column // (`driver-memory` / `driver-mongodb`; `driver-sql` returns it as null) // and put a duplicate warning on leads that are not duplicates. The // guard is what makes the predicate answer `false` instead of faulting. // Pinned on the real engine in `test/lead-duplicate-visibility.test.ts`. // - // ⛔ And the guard is NOT the whole predicate. "Any verdict the record - // actually carries" reads like a job for `has()` alone, and `has()` - // alone is wrong here: it is TRUE for a key that is PRESENT AND NULL, - // which is precisely what `driver-sql` hands back for a clean lead. - // Measured on this engine — `has(record.duplicate_status)` against - // `{ duplicate_status: null }` answers `{ ok: true, value: true }` — so - // dropping the comparison would reach the same cry-wolf banner as - // dropping the guard, just by the other road. `!= null` is what makes - // "set" mean set; both halves are pinned, shape by shape. + // ⛔ And the guard is NOT the whole predicate — `has()` ALONE is wrong + // here, in either shape. It is TRUE for a key that is PRESENT AND NULL, + // which is precisely what `driver-sql` hands back for a clean lead + // (measured: `has(record.duplicate_status)` against + // `{ duplicate_status: null }` answers `{ ok: true, value: true }`), so + // the guard needs a comparison beside it or the banner cries wolf on + // every clean lead. #1289, covering both verdicts at once, spelled that + // comparison `&& … != null`. A per-verdict banner spells it with the + // EQUALITY, which is strictly narrower and subsumes it: measured on + // this engine, `null == "suspected"` is a clean `false`, not a fault, + // and the two spellings agree on every record shape a driver can + // produce. Both halves of the shape #1289 ruled for are intact — the + // `has()` guard verbatim, and a comparison that makes "set" mean set — + // and the comparison got STRICTER, which is the point of the split. The + // same spelling already ships one file over, on this same field: the + // conversion flow's `e21` / `e25` edges (#1288) read + // `has(vars.leadRecord.duplicate_status) && … == "suspected"`. + // + // ⛔ Neither half may be simplified away. Both are pinned, shape by + // shape, including the reverse pin that the unguarded tail really does + // fault. // // `P` — an explicit `{ dialect: 'cel' }` envelope — is not decoration // either: `ExpressionEvaluator.evaluateCondition` routes ONLY the @@ -158,57 +187,103 @@ export const LeadDetailPage: Page = { // emptyText/submitLabel, so a plain-string `body` would ship English to // all four locales. Keeping both halves of one banner's copy in one // place beats splitting `title` into the locale packs. + // + // ⭐ Each banner's copy NAMES ITS OWN VERDICT, in the vocabulary the + // `duplicate_status` chip below publishes, and never the other one — + // pinned against the option labels read out of the locale packs in + // `test/lead-duplicate-visibility.test.ts`, so renaming an option + // re-aims the assertion rather than retiring it. { type: 'record:alert', - id: 'lead_duplicate_alert', - label: 'Duplicate Flagged', + id: 'lead_duplicate_alert_suspected', + label: 'Suspected Duplicate Alert', properties: { + // `warning`, not `error`: conversion still PROCEEDS on this + // verdict, and the renderer maps `error` to `role="alert"` / + // `aria-live="assertive"` — an interruption this state has not + // earned. severity: 'warning', - visible: P`has(record.duplicate_status) && record.duplicate_status != null`, + visible: P`has(record.duplicate_status) && record.duplicate_status == "suspected"`, + title: { + en: 'Suspected duplicate — compare before you convert', + 'zh-CN': '疑似重复——转换前请先比对', + 'ja-JP': '重複の疑い — 変換する前に照合してください', + 'es-ES': 'Duplicado Sospechoso: compare antes de convertir', + }, + body: { + en: + 'Intake matched this lead to a record this app already has. Duplicate ' + + 'Management below links that record — open it and compare. You can still ' + + 'convert this lead: this is the match intake guessed at, not a reviewer\'s ' + + 'decision. But if it is the same buyer, disqualify it instead, because ' + + 'converting creates a second account, contact and opportunity for them.', + 'zh-CN': + '录入时发现该线索与系统中已有记录匹配。下方「重复线索管理」中是它重复的那条记录，' + + '请先打开比对。该线索仍然可以转换：这是录入时的自动判断，不是审核人的结论。' + + '但若确属同一客户，请改为取消资格——转换会为同一客户再创建一套客户、联系人和商机。', + 'ja-JP': + '登録時に、このリードが既存レコードと一致しました。下の「重複管理」に重複先の' + + 'レコードがあります。まず開いて照合してください。このリードはまだ変換できます。' + + 'これは登録時の自動判定であり、担当者の結論ではありません。ただし同じ相手で' + + 'あれば、変換せず不適格にしてください。変換すると同じ相手に取引先・取引先' + + '責任者・商談がもう一組作成されます。', + 'es-ES': + 'La captura encontró que este prospecto coincide con un registro que ya ' + + 'existe. Gestión de Duplicados, más abajo, enlaza ese registro: ábralo y ' + + 'compárelo. Todavía puede convertir este prospecto, porque se trata de una ' + + 'coincidencia automática de la captura y no del veredicto de una persona. ' + + 'Pero si es el mismo comprador, descalifíquelo en su lugar: convertirlo ' + + 'crea una segunda cuenta, contacto y oportunidad para él.', + }, + }, + }, + { + type: 'record:alert', + id: 'lead_duplicate_alert_confirmed', + label: 'Confirmed Duplicate Alert', + properties: { + // `error`, and the level is the message: this is the state on which + // the app REFUSES the rep's next click, and the renderer gives + // `error` `role="alert"` / `aria-live="assertive"` rather than the + // polite `role="status"` every other level gets. ⛔ The severity is + // presentation only — what the app refuses was ruled by #1288 and + // shipped by PR #1555, and nothing here changes it. + severity: 'error', + visible: P`has(record.duplicate_status) && record.duplicate_status == "confirmed"`, title: { - // ⛔ The words are deliberately NOT the locale packs' - // `duplicate_status` option labels any more (#1289). One banner - // now covers two states with one title, and `pickLocalized` - // picks by LANGUAGE, not by row — so naming either verdict here - // would label the other one wrongly, and calling a reviewer's - // finished verdict a machine's suspicion is the one sentence - // this banner must not say. It names the FACT both states share - // and sends the rep to the chip below for the verdict itself. - // Pinned against the option labels in - // `test/lead-duplicate-visibility.test.ts`. - en: 'Marked as a duplicate', - 'zh-CN': '已标记为重复', - 'ja-JP': '重複としてマークされています', - 'es-ES': 'Marcado como duplicado', + en: 'Confirmed duplicate — conversion will be refused', + 'zh-CN': '已确认重复——转换将被拒绝', + 'ja-JP': '重複確定 — 変換は拒否されます', + 'es-ES': 'Duplicado Confirmado: la conversión será rechazada', }, body: { - // Same discipline as `title`: says WHAT is true of both states - // and points at the field that distinguishes them, rather than - // asserting one. "Intake flagged this lead", the #1207 wording, - // is a claim only `suspected` supports — `confirmed` is written - // by a person, not by the hook. + // ⭐ The only place a rep learns the Convert button will refuse + // them BEFORE they press it — until this banner, the refusal + // dialog was the first they heard of it. en: - 'This lead is marked as repeating a record this app already has. ' - + 'Duplicate Status below says whether that is an automatic match from ' - + 'intake or a reviewer\'s verdict, and Duplicate Management links the ' - + 'record it repeats — compare them before you convert, because converting ' - + 'creates a second account, contact and opportunity for the same buyer.', + 'A reviewer checked this lead and recorded that it repeats a record this app ' + + 'already has, so Convert Lead refuses it — this banner is the only warning ' + + 'you get before you press the button. Disqualify this lead instead, naming ' + + 'the surviving record from Duplicate Management below. If the verdict is ' + + 'wrong, a reviewer revises Duplicate Status; there is no override here.', 'zh-CN': - '该线索已被标记为与系统中已有记录重复。下方的「重复状态」会说明这是录入时的' - + '自动匹配还是审核人的判定，「重复线索管理」中是它重复的那条记录' - + '——转换前请先比对，转换会为同一客户再创建一套客户、联系人和商机。', + '审核人已核实该线索与系统中已有记录重复，因此「转换线索」会拒绝执行——' + + '本提示是你按下按钮前唯一的预警。请改为取消该线索的资格，并在下方' + + '「重复线索管理」中注明保留的那条记录。若判定有误，应由审核人修改' + + '「重复状态」，此处不提供强制转换的入口。', 'ja-JP': - 'このリードは既存レコードと重複するものとしてマークされています。' - + '下の「重複ステータス」が登録時の自動照合か担当者の判定かを示し、' - + '「重複管理」に重複先のレコードがあります。変換すると同じ相手に' - + '取引先・取引先責任者・商談がもう一組作成されるため、変換する前に比較してください。', + '担当者が確認し、このリードは既存レコードの重複であると記録されました。' + + 'そのため「リード変換」は拒否されます。この通知が、ボタンを押す前に得られる' + + '唯一の警告です。このリードは不適格にしたうえで、下の「重複管理」で残す' + + 'レコードを明記してください。判定が誤っている場合は担当者が「重複ステータス」' + + 'を修正します。ここに強制変換の手段はありません。', 'es-ES': - 'Este prospecto está marcado como duplicado de un registro que ya existe. ' - + 'El campo Estado del Duplicado indica si se trata de una coincidencia ' - + 'automática de la captura o del veredicto de una persona, y Gestión de ' - + 'Duplicados enlaza el registro que repite: compárelos antes de convertirlo, ' - + 'porque la conversión crea una segunda cuenta, contacto y oportunidad para ' - + 'el mismo comprador.', + 'Una persona verificó que este prospecto repite un registro que ya existe, ' + + 'por lo que Convertir Prospecto lo rechazará: este aviso es la única ' + + 'advertencia antes de pulsar el botón. Descalifique el prospecto e indique ' + + 'el registro que sobrevive desde Gestión de Duplicados, más abajo. Si el ' + + 'veredicto es incorrecto, una persona debe cambiar el Estado del Duplicado; ' + + 'aquí no hay forma de forzar la conversión.', }, }, }, diff --git a/src/translations/en/app.ts b/src/translations/en/app.ts index c6823adf..addfd1ce 100644 --- a/src/translations/en/app.ts +++ b/src/translations/en/app.ts @@ -182,7 +182,8 @@ export const appSurface: Omit = { title: '{first_name} {last_name}', subtitle: '{company}', components: { - lead_duplicate_alert: { label: 'Duplicate Flagged' }, + lead_duplicate_alert_confirmed: { label: 'Confirmed Duplicate Alert' }, + lead_duplicate_alert_suspected: { label: 'Suspected Duplicate Alert' }, lead_highlights: { label: 'Key Information' }, lead_path: { label: 'Lead Status Path' }, main_tabs: { label: 'Lead Information Tabs' }, diff --git a/src/translations/es-ES/app.ts b/src/translations/es-ES/app.ts index d1d5ca7e..cabb06a6 100644 --- a/src/translations/es-ES/app.ts +++ b/src/translations/es-ES/app.ts @@ -515,7 +515,8 @@ export const appSurface: Omit = { title: '{first_name} {last_name}', subtitle: '{company}', components: { - lead_duplicate_alert: { label: 'Aviso de duplicado' }, + lead_duplicate_alert_confirmed: { label: 'Aviso de duplicado confirmado' }, + lead_duplicate_alert_suspected: { label: 'Aviso de duplicado sospechoso' }, lead_highlights: { label: 'Información Clave' }, lead_path: { label: 'Progreso del Estado del Prospecto' }, main_tabs: { label: 'Pestañas de Información del Prospecto' }, diff --git a/src/translations/ja-JP/app.ts b/src/translations/ja-JP/app.ts index 31b693bf..19892fd3 100644 --- a/src/translations/ja-JP/app.ts +++ b/src/translations/ja-JP/app.ts @@ -517,7 +517,8 @@ export const appSurface: Omit = { title: '{first_name} {last_name}', subtitle: '{company}', components: { - lead_duplicate_alert: { label: '重複マークの警告' }, + lead_duplicate_alert_confirmed: { label: '重複確定の警告' }, + lead_duplicate_alert_suspected: { label: '重複の疑いの警告' }, lead_highlights: { label: '重要情報' }, lead_path: { label: 'リードステータスの進捗' }, main_tabs: { label: 'リード情報タブ' }, diff --git a/src/translations/zh-CN/app.ts b/src/translations/zh-CN/app.ts index 8b8e8cee..19cb7b3e 100644 --- a/src/translations/zh-CN/app.ts +++ b/src/translations/zh-CN/app.ts @@ -516,7 +516,8 @@ export const appSurface: Omit = { title: '{first_name} {last_name}', subtitle: '{company}', components: { - lead_duplicate_alert: { label: '重复标记提醒' }, + lead_duplicate_alert_confirmed: { label: '已确认重复提醒' }, + lead_duplicate_alert_suspected: { label: '疑似重复提醒' }, lead_highlights: { label: '关键信息' }, lead_path: { label: '线索状态进度' }, main_tabs: { label: '线索信息标签页' }, diff --git a/test/lead-duplicate-visibility.test.ts b/test/lead-duplicate-visibility.test.ts index ce35a99a..d4c6b8d2 100644 --- a/test/lead-duplicate-visibility.test.ts +++ b/test/lead-duplicate-visibility.test.ts @@ -28,7 +28,7 @@ import stack from '../objectstack.config'; * * | surface | carries | unevaluable predicate ⇒ | * | ------- | ------- | ----------------------- | - * | `lead_detail_page` `record:alert` | `properties.visible`, client CEL | FAIL-SOFT: banner SHOWN | + * | `lead_detail_page` `record:alert` ×2 | `properties.visible`, client CEL | FAIL-SOFT: banner SHOWN | * | `lead_conversion` edges `e21`/`e22`/`e25` | flow condition, server CEL | RUN FAILS | * * They fail in opposite directions and both are ugly: a fail-soft banner cries @@ -45,6 +45,18 @@ import stack from '../objectstack.config'; * `test/flow-condition-totality.test.ts` (record-change flow conditions) and * `test/view-predicate-dialect.test.ts` (view `visibleWhen`). This is the * fourth: a record PAGE component predicate. + * + * ## One banner per verdict (#1628) + * + * The record-page half is TWO `record:alert` components, not one. #1207 gated + * a single banner on `suspected`; #1289 widened it to every verdict and, being + * one component with one `visible` and one title/body pair, had to word it so + * it named NEITHER state. Since #1288 the two verdicts have opposite next + * steps — `suspected` warns and conversion proceeds, `confirmed` is refused — + * so the neutral wording told the rep something was wrong without telling them + * what to do. Splitting the component is what buys that back, and it moves + * three of the pins below: the shape, the predicates, and the copy rule, which + * INVERTS from "names neither verdict" to "names its own and never the other". */ const LOCALES = ['en', 'zh-CN', 'ja-JP', 'es-ES'] as const; @@ -66,7 +78,42 @@ function componentsOf(node: unknown, out: AnyRec[] = []): AnyRec[] { const leadPage: AnyRec | undefined = pages.find((p) => p.name === 'lead_detail_page'); const leadPageComponents = componentsOf(leadPage?.regions ?? []); -const duplicateAlert = leadPageComponents.find((c) => c.type === 'record:alert'); +const duplicateAlerts = leadPageComponents.filter((c) => c.type === 'record:alert'); + +/** + * One banner per verdict, addressed by the id the page gives it (#1628). + * + * The id is not decoration. A region renders as + * `components.map((node, i) => )` + * — read out of the shipped console bundle at the `.objectui-sha` pin — so two + * sibling `record:alert` nodes are two MOUNTED components, each evaluating its + * own `properties.visible` against the same row, and each node's `id` is its + * React key. That is what makes one-banner-per-verdict expressible at all, and + * it is why the ids must differ. + */ +const ALERT_IDS = { + suspected: 'lead_duplicate_alert_suspected', + confirmed: 'lead_duplicate_alert_confirmed', +} as const; + +type Verdict = keyof typeof ALERT_IDS; +const VERDICTS = Object.keys(ALERT_IDS) as Verdict[]; +const OTHER_VERDICT: Record = { + suspected: 'confirmed', + confirmed: 'suspected', +}; + +const alertFor = (verdict: Verdict): AnyRec | undefined => + duplicateAlerts.find((c) => c.id === ALERT_IDS[verdict]); + +/** Every record shape a driver can hand the renderer, plus the verdicts. */ +const RECORD_SHAPES: Array<[string, Rec]> = [ + ['a record with no keys at all (driver-memory / driver-mongodb)', {}], + ['a clean lead on driver-sql (present and null)', { duplicate_status: null }], + ['suspected', { duplicate_status: 'suspected' }], + ['confirmed', { duplicate_status: 'confirmed' }], + ['a value neither option declares', { duplicate_status: 'merged' }], +]; const leadFields = Object.keys( (objects.find((o) => o.name === 'crm_lead')?.fields ?? {}) as AnyRec, ); @@ -82,22 +129,56 @@ const leadFields = Object.keys( const evaluate = (source: string, record: Rec) => ExpressionEngine.evaluate({ dialect: 'cel', source }, { record }); -describe('lead record page — the duplicate banner', () => { - it('ships a warning-severity `record:alert` on the lead detail page', () => { +describe('lead record page — the duplicate banners, one per verdict', () => { + it('ships ONE `record:alert` per verdict, under distinct ids (#1628)', () => { expect(leadPage, 'lead_detail_page is not registered').toBeDefined(); - expect(duplicateAlert, 'the lead detail page carries no record:alert').toBeDefined(); - expect(duplicateAlert!.properties?.severity).toBe('warning'); + + // Two nodes rather than one widened node. Since #1288 the verdicts have + // OPPOSITE next steps — `suspected` warns and conversion proceeds, + // `confirmed` is refused outright — while one `record:alert` carries one + // `visible` and one title/body pair, resolved per LANGUAGE and not per + // row. So a single banner can state one next step or neither; #1289 + // rightly chose neither, and that is the gap this card closes. + expect( + duplicateAlerts.map((c) => c.id).sort(), + 'the lead page no longer carries exactly the two per-verdict banners', + ).toEqual([ALERT_IDS.confirmed, ALERT_IDS.suspected].sort()); + + // Distinct ids are load-bearing: the region renderer keys each child by + // `node.id`, so two siblings sharing one id would collide as React keys. + expect( + new Set(duplicateAlerts.map((c) => c.id)).size, + 'two sibling banners share one id — they would collide as React keys', + ).toBe(duplicateAlerts.length); }); - it('gates the banner with a CEL ENVELOPE, not a bare string', () => { + /** + * Severity is measured, not decorative. `RecordAlertProps` in + * `@objectstack/spec` documents that `error` renders `role="alert"` / + * `aria-live="assertive"` and every other level `role="status"` / polite, + * and `record-alert.tsx` implements exactly that at the pin. `confirmed` is + * the state on which the app REFUSES the rep's next click, so it is + * announced assertively; `suspected` stays a polite caution, because + * conversion still goes through. + */ + it.each([ + ['suspected', 'warning'], + ['confirmed', 'error'], + ] as Array<[Verdict, string]>)('the %s banner ships at `%s` severity', (verdict, severity) => { + const alert = alertFor(verdict); + expect(alert, `the lead detail page carries no ${verdict} banner`).toBeDefined(); + expect(alert!.properties?.severity).toBe(severity); + }); + + it.each(VERDICTS)('gates the %s banner with a CEL ENVELOPE, not a bare string', (verdict) => { // Not a style point. `ExpressionEvaluator.evaluateCondition` routes only an // explicit `{ dialect: 'cel' }` envelope to `@objectstack/formula`; a bare // string takes the legacy JS path, whose `FormulaFunctions` carries no CEL // `has()`. There the guard below would itself be the fault — and this call // site is fail-soft, so the banner would show on every lead. `P` from // `@objectstack/spec` is what produces the envelope. - const visible = duplicateAlert!.properties?.visible; - expect(visible, 'the banner has no visibility predicate — it would show on every lead') + const visible = alertFor(verdict)!.properties?.visible; + expect(visible, `the ${verdict} banner has no visibility predicate — it would show on every lead`) .toBeTruthy(); expect(typeof visible).toBe('object'); expect(visible.dialect).toBe('cel'); @@ -105,57 +186,79 @@ describe('lead record page — the duplicate banner', () => { expect(visible.source.trim()).not.toBe(''); }); - it('answers with a VERDICT on every record shape a driver can hand it', () => { - const source: string = duplicateAlert!.properties.visible.source; - - // 1. A brand-new / clean lead on a driver that omits absent columns. This - // is the shape that decides whether the banner is trustworthy: an abort - // here answers SHOWN, and a duplicate warning on a lead that is not a - // duplicate teaches reps to dismiss the banner they need. - expect(evaluate(source, {}), 'faults on a record with no keys at all') - .toEqual({ ok: true, value: false }); - - // 2. The same lead on `driver-sql`, which returns the column as null. - // `has()` is TRUE for a present-but-null key, so this shape is a - // different question from the one above, not a restatement of it — - // and since #1289 it is the shape that decides how the widening is - // SPELLED. `has(record.duplicate_status)` on its own is the obvious - // way to write "any value the record actually carries", and it is - // wrong: measured on this engine it answers TRUE here, which would put - // a duplicate banner on every clean lead `driver-sql` returns — the - // fail-soft cry-wolf this whole file exists to prevent, arrived at by - // a different road. The comparison beside the guard is what makes - // "set" mean set. - expect(evaluate(source, { duplicate_status: null })) - .toEqual({ ok: true, value: false }); - - // 3. The lead the card is about. - expect(evaluate(source, { duplicate_status: 'suspected' })) - .toEqual({ ok: true, value: true }); - - // 4. The boundary #1289 MOVED, and the line that was here to make the - // move deliberate did its job: this used to read `false`, with a - // comment saying widening to `confirmed` was a product call rather - // than a defect repair. The product call was taken — a human's - // `confirmed` verdict is STRONGER evidence than the machine's guess, - // and it was the one duplicate state the banner stayed silent on. - // Since #1288 it is also the state on which the app REFUSES to - // convert, so a rep who reaches Convert without a banner meets a - // refusal dialog with no warning on the record behind it. - expect(evaluate(source, { duplicate_status: 'confirmed' })) - .toEqual({ ok: true, value: true }); + /** + * Each banner answers with a VERDICT on every record shape a driver can hand + * it, and answers TRUE on exactly its own verdict. + * + * Row 2 is the one that decides how a per-verdict predicate is SPELLED. + * `has()` is TRUE for a present-but-null key — measured, and re-measured + * here — so `has(record.duplicate_status)` alone would put a duplicate + * banner on every clean lead `driver-sql` returns. #1289 answered that with + * `&& … != null`; a per-verdict banner answers it with the equality itself, + * which is strictly narrower: `null == "suspected"` is a clean `false` on + * this engine, not a fault. Both halves of the shape #1289 ruled for survive + * — the `has()` guard verbatim, and a comparison that makes "set" mean set — + * and the comparison got stricter, which is the whole point of the split. + * The same spelling already ships one file over, on this same field: the + * conversion flow's `e21` / `e25` edges (#1288) read + * `has(vars.leadRecord.duplicate_status) && … == "suspected"`. + * + * Row 5 is a behaviour change this card MAKES, deliberately. A value neither + * option declares used to raise the widened banner, while the conversion + * flow's `e22` Clean edge treats it as clean and converts it — the page and + * the flow disagreed about the same row. Two verdict-scoped predicates make + * the page agree with the flow: no banner, and conversion proceeds. + */ + it.each(VERDICTS)('the %s banner answers with a verdict on every record shape', (verdict) => { + const source: string = alertFor(verdict)!.properties.visible.source; + + for (const [label, record] of RECORD_SHAPES) { + const expected = record.duplicate_status === verdict; + expect( + evaluate(source, record), + `the ${verdict} banner misreads ${label}`, + ).toEqual({ ok: true, value: expected }); + } + }); + + it.each(RECORD_SHAPES)('at most one banner is ever shown — %s', (_label, record) => { + // The record-page twin of the flow's "exactly one live edge" pin. Two + // banners on one row would stack two contradictory next steps on the same + // lead; nothing structural prevents that, so it is measured. + const shown = VERDICTS.filter((v) => { + const result = evaluate(alertFor(v)!.properties.visible.source, record); + expect(result.ok, `the ${v} banner faulted — this surface is FAIL-SOFT, so it would SHOW`) + .toBe(true); + // `expect` does not narrow the union for tsc, so the discriminant is + // re-read here rather than asserted away. + return result.ok === true && result.value === true; + }); + expect( + shown.length, + `both banners are visible at once on this row: ${shown.join(' + ')}`, + ).toBeLessThanOrEqual(1); }); - it('the guard is load-bearing — the unguarded spelling really does fault', () => { + it.each(VERDICTS)('the %s banner\u2019s guard is load-bearing — the unguarded spelling really does fault', (verdict) => { // Reverse verification of the premise, pinned rather than assumed: if a // future engine starts answering `false` for an absent key, this flips and // the next reader is told the premise changed instead of finding a guard - // that protects nothing. The unguarded text is built from the shipped - // predicate's own comparison so the two cannot drift apart. - const source: string = duplicateAlert!.properties.visible.source; + // that protects nothing. + // + // The unguarded text is still BUILT FROM the shipped predicate's own + // comparison (`split('&&').pop()`) so the two cannot drift apart — the + // property #1289 gave this pin. Splitting the banner changed what that + // tail SAYS: it used to be `!= null`, and is now `== ""`. So the second leg has to ask for the banner's OWN verdict + // row rather than a fixed one — the unguarded `== "confirmed"` tail is + // correctly FALSE on a suspected lead, and asserting `true` there would + // pin the wrong claim. + const source: string = alertFor(verdict)!.properties.visible.source; const unguarded = source.split('&&').pop()!.trim(); expect(unguarded, 'the shipped predicate no longer ends in the comparison') .toContain('record.duplicate_status'); + expect(unguarded, 'the shipped predicate no longer compares against its own verdict') + .toContain(`"${verdict}"`); const faulted = evaluate(unguarded, {}); expect(faulted.ok, `\`${unguarded}\` answered on a keyless record — the guard is now decorative`) @@ -163,10 +266,10 @@ describe('lead record page — the duplicate banner', () => { // …and it is the ABSENT key that faults it, not the text: the same // predicate answers cleanly the moment the column is present. - expect(evaluate(unguarded, { duplicate_status: 'suspected' })).toEqual({ ok: true, value: true }); + expect(evaluate(unguarded, { duplicate_status: verdict })).toEqual({ ok: true, value: true }); }); - it('carries its copy in all four shipped locales', () => { + it.each(VERDICTS)('the %s banner carries its copy in all four shipped locales', (verdict) => { // `record:alert` resolves `title` / `body` through `pickLocalized(…, // language)`, so an inline `{ en, 'zh-CN', … }` map is a delivered // capability — and for `body` it is the ONLY channel: the i18n extractor's @@ -178,8 +281,8 @@ describe('lead record page — the duplicate banner', () => { .toEqual([...LOCALES].sort()); for (const key of ['title', 'body'] as const) { - const copy = duplicateAlert!.properties?.[key]; - expect(copy, `the banner has no ${key}`).toBeTruthy(); + const copy = alertFor(verdict)!.properties?.[key]; + expect(copy, `the ${verdict} banner has no ${key}`).toBeTruthy(); expect(typeof copy, `${key} is a bare string — three locales would read English`) .toBe('object'); for (const locale of LOCALES) { @@ -189,45 +292,55 @@ describe('lead record page — the duplicate banner', () => { } }); - it('describes the flag without asserting WHICH verdict it is (#1289)', () => { - // The half of the widening that is not a predicate. One banner now covers - // two states that mean different things — a machine's guess and a - // person's verdict — and it has ONE title and ONE body with no - // per-state channel: `record:alert` carries a single `visible`, and - // `pickLocalized` picks by LANGUAGE, not by row. So the copy may not - // assert either state, and the failure is silent and one-directional: - // widening the predicate while leaving the old words behind labels every - // `confirmed` lead "suspected" — telling a rep a reviewer's finished - // verdict is a machine's guess, which is the one sentence this banner - // must never say. + it.each(VERDICTS)('the %s banner names ITS OWN verdict, and never the other one (#1628)', (verdict) => { + // The copy half of the split, and it inverts the #1289 pin this replaces. + // + // While ONE banner covered both states it could assert neither: a single + // `visible` and a single title/body pair, picked by LANGUAGE and not by + // row, meant naming a verdict would mislabel every lead in the other + // state — telling a rep that a reviewer's finished verdict was a machine's + // guess. #1289's pin therefore forbade BOTH words on the one banner. + // + // With one banner per verdict that constraint reverses in one direction + // and hardens in the other. Each banner is now shown on exactly one state, + // so it MUST name that state — a banner whose whole job is "here is what + // to do next" has to say which situation it is talking about, and the + // vocabulary the rep can check it against is the `duplicate_status` chip + // below it. And it must still never name the OTHER verdict, for exactly + // the reason #1289 gave. // - // The forbidden words are READ FROM the locale packs rather than typed - // here, so this cannot drift from the option labels a rep actually sees - // on the `duplicate_status` chip below the banner: renaming an option - // re-aims the assertion instead of quietly retiring it. + // ⭐ Both words are still READ FROM the locale packs rather than typed + // here — the property #1289 built in, deliberately preserved: renaming an + // option re-aims both assertions instead of quietly retiring them. const packs = new Map(localePacks); expect([...packs.keys()].sort(), 'the locale packs no longer cover these four') .toEqual([...LOCALES].sort()); + const other = OTHER_VERDICT[verdict]; + for (const locale of LOCALES) { const options: AnyRec = packs.get(locale)?.objects?.crm_lead?.fields?.duplicate_status?.options ?? {}; - const verdicts = [options.suspected, options.confirmed].filter( - (w): w is string => typeof w === 'string' && w.trim() !== '', - ); - expect(verdicts.length, `${locale} has no duplicate_status option labels to check against`) - .toBe(2); + const own = options[verdict]; + const foreign = options[other]; + expect(typeof own, `${locale} has no \`${verdict}\` option label to check against`) + .toBe('string'); + expect(typeof foreign, `${locale} has no \`${other}\` option label to check against`) + .toBe('string'); const copy = [ - duplicateAlert!.properties?.title?.[locale], - duplicateAlert!.properties?.body?.[locale], + alertFor(verdict)!.properties?.title?.[locale], + alertFor(verdict)!.properties?.body?.[locale], ].join(' '); - for (const verdict of verdicts) { - expect( - copy, - `the ${locale} banner copy says "${verdict}" — it is shown on BOTH verdicts and may name neither`, - ).not.toContain(verdict); - } + + expect( + copy, + `the ${locale} ${verdict} banner never says "${own}" — the rep cannot tell which verdict it is reading`, + ).toContain(own); + expect( + copy, + `the ${locale} ${verdict} banner says "${foreign}" — it is shown only on ${verdict} and would mislabel the verdict`, + ).not.toContain(foreign); } });