From af682026da38e551eaea97da74ea0aa1836359e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 6 Sep 2026 10:06:02 +0000 Subject: [PATCH] feat(lead): one duplicate banner per verdict, each with its own next step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/pages/lead_detail.page.ts` carried ONE `record:alert` for both duplicate verdicts. #1207 gated it on `duplicate_status == "suspected"`; #1289 widened it to every verdict the field carries. The widening was right, but a `record:alert` carries a single `visible` and a single title/body pair, and `pickLocalized` picks by LANGUAGE, not by row — so one component covering both verdicts had to word itself so it named NEITHER, or it would have mislabelled every lead in the other state. Since #1288 the two verdicts have opposite next steps: `suspected` warns and conversion PROCEEDS, `confirmed` is REFUSED outright by `refuse_confirmed_duplicate`. One sentence cannot state either without being false for the other, so it stated neither — the 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. On `confirmed` it was worse: nothing on the record said the Convert button would refuse them, so they learned it by pressing it. So the banner becomes two, one per verdict, in all four locales. `suspected` keeps `warning` and says to compare against the linked record first; `confirmed` ships at `error` — which `record-alert.tsx` maps to `role="alert"` / `aria-live="assertive"` rather than the polite `role="status"` every other level gets — and is the only place a rep is warned about the refusal before pressing Convert. 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 ids are their React keys, which is why the two ids differ. Both halves of the guard #1289 ruled for are intact. `has()` stays verbatim — this call site is FAIL-SOFT, so an unevaluable predicate SHOWS the banner, and an unguarded predicate aborts with `No such key` on every clean lead whose driver omits the column. The comparison beside it becomes the EQUALITY, which is strictly narrower than `!= null` and subsumes it: measured on the pinned engine, `null == "suspected"` is a clean `false`, not a fault, and the two spellings agree on every record shape a driver can produce. The same spelling already ships on this field one file over, in the conversion flow's `e21` / `e25` edges. A lead carrying a value neither option declares now raises NO banner, where the widened predicate raised the neutral one. That matches what the flow already does with such a row — `e22` Clean 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. The pins in `test/lead-duplicate-visibility.test.ts` move deliberately, three of them: the shape (one alert -> two, addressed by id, ids distinct), the predicate table (each banner true on exactly its own verdict, plus a new mutual-exclusion pin mirroring the flow's one-live-edge rule), and the copy rule, which INVERTS from "names neither verdict" to "names its own and never the other". The copy rule still reads both words out of the locale packs, so renaming an option re-aims the assertion instead of retiring it. Claude-Session: https://claude.ai/code/session_018xtjdpZFjgWh4Ad9Wcx68J Co-authored-by: Claude --- .../duplicate-banner-one-per-verdict.md | 45 +++ src/pages/lead_detail.page.ts | 203 +++++++++---- src/translations/en/app.ts | 3 +- src/translations/es-ES/app.ts | 3 +- src/translations/ja-JP/app.ts | 3 +- src/translations/zh-CN/app.ts | 3 +- test/lead-duplicate-visibility.test.ts | 281 ++++++++++++------ 7 files changed, 389 insertions(+), 152 deletions(-) create mode 100644 .changeset/duplicate-banner-one-per-verdict.md 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); } });