From 9a493d3261ba5a443ffec2043c1fbc29d4f045dd Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 19:11:56 +0000 Subject: [PATCH 1/2] fix(i18n): stop the Review Queue tab claiming a 180-day window its filter never applied (#769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four locale packs named `crm_knowledge_article.stale_articles` "Stale (>180d)" / 过期 (>180 天) / "Obsoletos (>180d)" / 古い (>180日) over a filter reading only `status = published`. The tab therefore returned EVERY published article — one reviewed minutes ago included, merely sorted last — under a heading promising a six-month cut. Same defect class as #730, and as there the lie lived only in the translated half: the metadata label ("Review Queue · Oldest First") promised an ordering throughout, and the `last_reviewed_at asc` sort delivers it. The window was measured before being ruled out, not assumed impossible. On the pinned 17.0.0-rc.2 `{180_days_ago}` resolves on the read path and lands on the START of the calendar day 180 days back, so `less_than` excludes that whole day — the #3777 day-boundary convention, and the right sense for "> 180d". But `$lt` matches neither null nor absent values, so the window deletes never-reviewed articles, which are a review queue's most overdue population. The honest condition is a DISJUNCTION ("older than the window OR never reviewed"), and a view `filter` is a flat, strict array of {field, operator, value} rules combined with AND: no `or`, no nesting, no logic key. Spelled the only way the grammar allows, the two rules AND together and the tab returns zero rows. The engine itself answers correctly with `$or` — the limit is the view-authoring grammar, not the data layer. So the fix is the name, not the filter. Which rows the tab returns is unchanged, and no emptyState is authored because no new empty state exists (the #746 reason does not apply here). The house-rule guard grew a second claim family. It read only current-calendar-period phrases and so was blind to "> 180d": run against these four labels verbatim it passes 17/17. It now also reads parameterised day windows in all four locales, and carries a positive control — the deleted labels, fed through the shipped filter, must be reported as a breach — so the family cannot pass vacuously now that its only instances are gone. A new runtime block pins every measurement above against a real engine. Fixes #769 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VHrPAGEgFDoHjphqYG4BMa --- .../stale-articles-review-queue-label.md | 29 ++ src/translations/en.ts | 2 +- src/translations/es-ES.ts | 2 +- src/translations/ja-JP.ts | 2 +- src/translations/zh-CN.ts | 2 +- src/views/knowledge_article.view.ts | 81 ++-- test/forecast-current-quarter-view.test.ts | 404 +++++++++++++++++- 7 files changed, 473 insertions(+), 49 deletions(-) create mode 100644 .changeset/stale-articles-review-queue-label.md diff --git a/.changeset/stale-articles-review-queue-label.md b/.changeset/stale-articles-review-queue-label.md new file mode 100644 index 000000000..99abc064b --- /dev/null +++ b/.changeset/stale-articles-review-queue-label.md @@ -0,0 +1,29 @@ +--- +"hotcrm": patch +--- + +Knowledge base: the Review Queue tab no longer claims a 180-day window it never applied. + +In English, Chinese, Spanish and Japanese the knowledge-article review tab was named +"Stale (>180d)" (过期 (>180 天) / Obsoletos (>180d) / 古い (>180日)), but its filter +selected only `status = published`. The tab therefore returned **every** published +article — including one reviewed minutes ago, merely sorted to the bottom — under a +heading that promised a six-month cut. Anyone who read the tab as a worklist was +reading a list of the whole knowledge base. + +The four names now match the view's own metadata label, "Review Queue · Oldest First", +which is what the view actually does: every published article, least-recently-reviewed +first. Which rows the tab returns is unchanged. + +The 180-day window was measured before being ruled out rather than assumed impossible. +The date macro does resolve on the read path now, and lands on the start of the calendar +day 180 days back — but a comparison against it does not match articles that have never +been reviewed at all, and those are exactly the rows a review queue most needs to show. +Saying "older than 180 days **or** never reviewed" needs a disjunction, and a view filter +is a flat list of conditions combined with AND, so the honest window cannot currently be +written. Whether this queue should become a cut is now a separate, open product question. + +The house-rule guard that catches this defect class (a view name promising a time scope +its filter does not express) previously recognised only current-calendar-period phrases +like "This Quarter", so it could not see "> 180d". It now also reads parameterised day +windows in all four locales. diff --git a/src/translations/en.ts b/src/translations/en.ts index a260474d8..fed78c3e7 100644 --- a/src/translations/en.ts +++ b/src/translations/en.ts @@ -263,7 +263,7 @@ export const en: TranslationData = { all_articles: { label: 'All Articles' }, published_articles: { label: 'Published' }, my_drafts: { label: 'My Drafts' }, - stale_articles: { label: 'Stale (>180d)' }, + stale_articles: { label: 'Review Queue · Oldest First' }, }, _sections: { basic: { label: 'Article Information' }, diff --git a/src/translations/es-ES.ts b/src/translations/es-ES.ts index 27d5a3858..906ba3b0c 100644 --- a/src/translations/es-ES.ts +++ b/src/translations/es-ES.ts @@ -282,7 +282,7 @@ export const esES: TranslationData = { all_articles: { label: 'Todos los artículos' }, published_articles: { label: 'Publicados' }, my_drafts: { label: 'Mis borradores' }, - stale_articles: { label: 'Obsoletos (>180d)' }, + stale_articles: { label: 'Cola de revisión · Más antiguos primero' }, }, _sections: { basic: { label: 'Información del Artículo' }, diff --git a/src/translations/ja-JP.ts b/src/translations/ja-JP.ts index 90bb01961..df27acaf3 100644 --- a/src/translations/ja-JP.ts +++ b/src/translations/ja-JP.ts @@ -290,7 +290,7 @@ export const jaJP: TranslationData = { all_articles: { label: 'すべての記事' }, published_articles: { label: '公開済み' }, my_drafts: { label: '自分の下書き' }, - stale_articles: { label: '古い (>180日)' }, + stale_articles: { label: 'レビューキュー · 古い順' }, }, _sections: { basic: { label: '記事情報' }, diff --git a/src/translations/zh-CN.ts b/src/translations/zh-CN.ts index f7fa94e10..18fc922c6 100644 --- a/src/translations/zh-CN.ts +++ b/src/translations/zh-CN.ts @@ -256,7 +256,7 @@ export const zhCN: TranslationData = { all_articles: { label: '全部文章' }, published_articles: { label: '已发布' }, my_drafts: { label: '我的草稿' }, - stale_articles: { label: '过期 (>180 天)' }, + stale_articles: { label: '复核队列 · 最久未复核在前' }, }, _sections: { basic: { label: '文章信息' }, diff --git a/src/views/knowledge_article.view.ts b/src/views/knowledge_article.view.ts index a5c0aaa7f..721232d40 100644 --- a/src/views/knowledge_article.view.ts +++ b/src/views/knowledge_article.view.ts @@ -5,9 +5,13 @@ import { defineView } from '@objectstack/spec/ui'; /** * Knowledge Article Views * - * • grid — agent-facing article queue (status / category / audience) + * • grid — agent-facing article queue (status / category / audience) * • published — public articles only - * • stale — published > 180 days without review + * • stale — every published article, least-recently-reviewed first + * + * The third one is a RANKING, not a cut: `stale_articles` is the metadata + * item's name, not a claim about which rows come back. See its own note below + * for why it stays a ranking (#769). */ export const KnowledgeArticleViews = defineView({ list: { @@ -73,32 +77,57 @@ export const KnowledgeArticleViews = defineView({ stale_articles: { name: 'stale_articles', type: 'grid', - // Operator-only filter + oldest-review-first sort, and the reason - // recorded here has expired (#744). + // Operator-only filter + least-recently-reviewed-first sort. This view + // is a RANKING and the label says so; it deliberately applies no + // 180-day cut. Both halves of that sentence were settled by measurement + // (#769), so the reasoning is recorded here rather than re-derived. + // + // 1. The old note's premise expired (#744, #773). It read: the list data + // path does NOT resolve date macros, so `last_reviewed_at < + // '{180_days_ago}'` compared the literal string, and since '2026-…' < + // '{…' is lexicographically true it matched EVERY published article. + // True on @objectstack 16.1.0; false from 17.0.0-rc.0, when + // `resolveFilterTokens()` was wired into the ObjectQL read path + // (objectql #3582) ahead of the middleware chain — + // `find`/`findOne`/`count`/`aggregate`, saved-view filters included. + // Measured on the pinned 17.0.0-rc.2: `{180_days_ago}` resolves to + // the START of the calendar day 180 days ago (00:00:00.000Z), so as + // an EXCLUSIVE upper bound under `less_than` it excludes that whole + // day — the #3777 day-boundary convention, and the right sense for a + // label reading "> 180d". + // + // 2. Expressible is not the same as expressible WITHOUT LOSS. A review + // queue's most overdue population is the never-reviewed one, and + // `last_reviewed_at` is nullable: a published article can carry no + // review timestamp at all. Measured on the same engine, `$lt` does + // not match null or absent values, so a bare window silently deletes + // exactly those rows. The honest condition is "earlier than + // {180_days_ago} OR empty" — and a view `filter` cannot say that. It + // is a FLAT, strict array of `{field, operator, value}` rules + // (`ViewFilterRuleSchema`) combined with AND; there is no `or`, no + // nesting, no logic key. Spelled as the two rules the grammar does + // allow, the window and the emptiness test AND together and the view + // returns ZERO rows. (The engine itself has `$or` and answers the + // question correctly — this is a view-authoring limit, not an engine + // limit.) // - // It read: the list data path does NOT resolve date macros, so - // `last_reviewed_at < '{180_days_ago}'` compared the literal string, and - // since '2026-…' < '{…' is lexicographically true it matched EVERY - // published article — including ones reviewed minutes ago (verified - // against the running console). That was measured on @objectstack 16.1.0 - // and stopped being true at 17.0.0-rc.0, when `resolveFilterTokens()` was - // wired into the ObjectQL read path (objectql #3582) ahead of the - // middleware chain — `find`/`findOne`/`count`/`aggregate`, saved-view - // filters included. `{180_days_ago}` matches the parameterised token - // grammar `DATE_MACRO_PARAM_RE`, so on the pinned 17.0.0-rc.2 it reaches - // the driver as a resolved instant and the lexicographic inversion above - // is unreachable on this path. `test/forecast-current-quarter-view.test.ts` - // pins that seam against a real engine for the view filters that use it. + // So the label is what changed in #769, not the filter: the four locale + // packs had translated this view as 'Stale (>180d)' / '过期 (>180 天)' / + // 'Obsoletos (>180d)' / '古い (>180日)', promising a window the filter + // never expressed — the #730 defect class, living (as it did there) only + // in the translated half. They now render the metadata label faithfully, + // and every user-visible name for this view again promises exactly the + // ordering it delivers. No `emptyState` is authored because the change + // introduces no new empty state: "no published articles" is the same + // zero this tab has always had, unlike `closing_this_quarter` (#746), + // where scoping made "empty" newly reachable. // - // So the window is expressible now, and this view still does not express - // it — which matters more here than it does for `stale_opportunities`, - // because the four locale labels DO promise one ('Stale (>180d)' and its - // translations) while the metadata label promises only an ordering. That - // gap is #769: adding the bound is a behaviour change with two things to - // measure first (`last_reviewed_at` is `Field.datetime`, so a bare - // calendar-day comparand carries the #3777 upper-bound convention, and - // never-reviewed articles have no value at all), so it is filed rather - // than done here. Nothing below was touched. + // `test/forecast-current-quarter-view.test.ts` owns the house rule and + // pins all of the above against a real engine, including the day-window + // claim vocabulary that would fail this view the day a label reclaims a + // window. Whether the queue SHOULD become a 180-day cut is a product + // question that needs the null population handled first — filed + // separately, not decided by a translator. label: 'Review Queue · Oldest First', data: { provider: 'object', object: 'crm_knowledge_article' }, columns: ['article_number', 'title', 'category', 'owner_id', 'last_reviewed_at'], diff --git a/test/forecast-current-quarter-view.test.ts b/test/forecast-current-quarter-view.test.ts index 38c7e1bbb..755bb1ca4 100644 --- a/test/forecast-current-quarter-view.test.ts +++ b/test/forecast-current-quarter-view.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { ObjectQL } from '@objectstack/objectql'; import { InMemoryDriver } from '@objectstack/driver-memory'; +import { ViewFilterRuleSchema } from '@objectstack/spec/ui'; import stack from '../objectstack.config'; /** @@ -62,6 +63,16 @@ import stack from '../objectstack.config'; * arbitrary day inside the quarter. A range has a failure mode an equality * does not: lose one bound and the list widens back out while still looking * scoped, so that block subtracts each bound separately (#743). + * + * ### A third claim shape, and the one case the rule resolves the OTHER way + * + * The file's name predates its scope: it is the house rule's home, and the + * rule is not about forecasts. #769 added the third claim family — a + * PARAMETERISED DAY WINDOW ("> 180d"), which the calendar-period vocabulary + * could not see — and, unlike #730 and #743, it was fixed by correcting the + * NAME rather than the filter. The last runtime block records why, on + * measurement rather than on preference: the honest condition needs a + * disjunction, and a view `filter` has no way to write one. */ type AnyRec = Record; @@ -104,6 +115,52 @@ const CURRENT_PERIOD_CLAIMS: Array<{ period: string; token: string; phrases: Reg }, ]; +/** + * Phrases that claim a PARAMETERISED DAY WINDOW — "nothing here has been + * touched for N days" — and, derived from the N the label itself names, the + * `{N_days_ago}` token a filter must carry to honour the claim. + * + * A second family rather than more rows in the list above, because the claim + * is PARAMETERISED: the scope is not one of four fixed calendar words, it is + * whatever number the label prints, and the token that would honour it is + * computed from that number. `CURRENT_PERIOD_CLAIMS` cannot express that + * without one entry per N. + * + * It exists because the vocabulary above could not see #769. Four locale packs + * named `crm_knowledge_article.stale_articles` 'Stale (>180d)', '过期 (>180 + * 天)', 'Obsoletos (>180d)' and '古い (>180日)' over a filter reading only + * `status = published`, so the tab returned EVERY published article — one + * reviewed minutes ago included, merely sorted last. The metadata label + * ('Review Queue · Oldest First') was honest throughout: exactly as in #730, + * the lie lived only in the translated half, and "this/current quarter" does + * not match "> 180 days". + * + * Each pattern captures the day count in group 1, and every one of them + * requires an explicit DAY unit (`d` / `days` / `días` / `天` / `日`), so + * "Top 10", "Q4" and an SLA heading reading "> 4h" claim no window. + */ +const DAY_WINDOW_PATTERNS: RegExp[] = [ + /[>＞]\s*(\d+)\s*d\b/i, // Stale (>180d) · Obsoletos (>180d) + /[>＞]\s*(\d+)\s*days?\b/i, // Stale (>180 days) + /[>＞]\s*(\d+)\s*天/, // 过期 (>180 天) + /[>＞]\s*(\d+)\s*日/, // 古い (>180日) + /older than\s*(\d+)\s*days?\b/i, + /over\s*(\d+)\s*days?\b/i, + /más de\s*(\d+)\s*días?\b/i, + /(\d+)\s*天(?:以上|未|没)/, // 180 天未复核 + /(\d+)\s*日(?:以上|超)/, // 180日以上 +]; + +/** The day counts a single label claims, deduped. `[]` when it claims none. */ +const dayWindowClaims = (label: string): number[] => { + const found = new Set(); + for (const pattern of DAY_WINDOW_PATTERNS) { + const m = label.match(pattern); + if (m) found.add(Number(m[1])); + } + return [...found].sort((a, b) => a - b); +}; + /** * KNOWN DEBT — surfaces in breach of the rule above, with the issue that owns * the fix. NOT a suppression list: each entry is asserted to be STILL in @@ -153,6 +210,54 @@ const filterTokens = (view: AnyRec): string[] => .map((rule: AnyRec) => rule?.value) .filter((v: unknown): v is string => typeof v === 'string' && /^\{.+\}$/.test(v)); +/** + * Every breach ONE view is in, given every name it can be read under and the + * `{token}`s its filter carries. + * + * A pure function of (labels, tokens) rather than of a view, so the guard can + * be run against a HYPOTHETICAL label set — which is what proves it is not + * decoration. Both claim families are fixed by removing their only instances, + * so on the day each fix lands the derivation over the shipped stack returns + * `[]` and would pass for a shipped-empty reason. Feeding it the labels the fix + * DELETED is the positive control (see the #769 block below). + */ +const breachesOf = (id: string, labels: Array<[string, string]>, tokens: string[]): string[] => { + const out: string[] = []; + const named = (claiming: Array<[string, string]>) => + claiming.map(([where, label]) => `${where}:"${label}"`).join(', '); + + // Family 1 — a named CURRENT calendar period (#730, #743). + for (const claim of CURRENT_PERIOD_CLAIMS) { + const claiming = labels.filter(([, label]) => claim.phrases.some((p) => p.test(label))); + if (claiming.length === 0) continue; + if (tokens.includes(claim.token)) continue; + out.push( + `${id} is labelled ${named(claiming)} ` + + `but its filter carries no ${claim.token} — it returns every ${claim.period}, ` + + `not the current one (filter tokens: ${tokens.join(', ') || 'none'})`, + ); + } + + // Family 2 — a parameterised day window (#769). + const byDays = new Map>(); + for (const [where, label] of labels) { + for (const days of dayWindowClaims(label)) { + byDays.set(days, [...(byDays.get(days) ?? []), [where, label]]); + } + } + for (const [days, claiming] of [...byDays.entries()].sort((a, b) => a[0] - b[0])) { + const token = `{${days}_days_ago}`; + if (tokens.includes(token)) continue; + out.push( + `${id} is labelled ${named(claiming)} ` + + `but its filter carries no ${token} — it returns rows of every age, not the ` + + `ones untouched for ${days} days (filter tokens: ${tokens.join(', ') || 'none'})`, + ); + } + + return out; +}; + describe('no view label promises a time scope its filter does not express (#730)', () => { it('the locale bundles this guard reads are actually loaded', () => { // Without this, a bundle rename would silently reduce the guard to the @@ -170,25 +275,9 @@ describe('no view label promises a time scope its filter does not express (#730) expect(claims.map((c) => `${c.object}.${c.name}`)).toContain('crm_forecast.this_quarter_forecasts'); }); - /** Every (view, claimed period) pair whose filter does not pin that period. */ - const breaches = (): string[] => { - const out: string[] = []; - for (const { object, name, view } of listViews) { - const labels = namesOf(object, name, view); - const tokens = filterTokens(view); - for (const claim of CURRENT_PERIOD_CLAIMS) { - const claiming = labels.filter(([, label]) => claim.phrases.some((p) => p.test(label))); - if (claiming.length === 0) continue; - if (tokens.includes(claim.token)) continue; - out.push( - `${object}.${name} is labelled ${claiming.map(([l, v]) => `${l}:"${v}"`).join(', ')} ` - + `but its filter carries no ${claim.token} — it returns every ${claim.period}, ` - + `not the current one (filter tokens: ${tokens.join(', ') || 'none'})`, - ); - } - } - return out; - }; + /** Every (view, claim) pair, in either family, that the filter does not honour. */ + const breaches = (): string[] => listViews.flatMap(({ object, name, view }) => + breachesOf(`${object}.${name}`, namesOf(object, name, view), filterTokens(view))); it('every current-period label is backed by a filter that pins that period', () => { const bad = breaches().filter((line) => !Object.keys(KNOWN_DEBT).some((k) => line.startsWith(`${k} `))); @@ -210,6 +299,72 @@ describe('no view label promises a time scope its filter does not express (#730) ).toEqual([]); }); + // ── The day-window family, and the probe that keeps it from being decoration ── + + /** + * The four locale labels #769 deleted, verbatim. They are the only instances + * the day-window family has ever had, and the fix removed them — so without + * this control the family would ship green over an empty set and nobody + * would find out it never matched anything. + */ + const RETIRED_180D_LABELS: Array<[string, string]> = [ + ['en', 'Stale (>180d)'], + ['zh-CN', '过期 (>180 天)'], + ['es-ES', 'Obsoletos (>180d)'], + ['ja-JP', '古い (>180日)'], + ]; + + it('the day-window vocabulary reads a window out of every label #769 deleted', () => { + for (const [locale, label] of RETIRED_180D_LABELS) { + expect(dayWindowClaims(label), `${locale}: ${label}`).toEqual([180]); + } + }); + + it('re-installing those labels over the shipped filter is reported as a breach', () => { + // The subtraction, run rather than asserted from memory — and run against + // the filter `stale_articles` ACTUALLY ships, not a mock of it. Restore the + // deleted names and the guard must name the missing token; that is the + // whole claim this family makes. + const stale = listViews.find((v) => v.name === 'stale_articles')!; + const lines = breachesOf( + 'crm_knowledge_article.stale_articles', + [['metadata', stale.view.label], ...RETIRED_180D_LABELS], + filterTokens(stale.view), + ); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain('{180_days_ago}'); + expect(lines[0]).toContain('en:"Stale (>180d)"'); + expect(lines[0]).toContain('ja-JP:"古い (>180日)"'); + // The metadata label was the honest half in #769 and must not be blamed. + expect(lines[0]).not.toContain('metadata:'); + }); + + it('no name this stack actually ships claims a day window', () => { + const claiming = listViews.flatMap(({ object, name, view }) => + namesOf(object, name, view) + .filter(([, label]) => dayWindowClaims(label).length > 0) + .map(([where, label]) => `${object}.${name} ${where}:"${label}"`)); + expect( + claiming, + 'a name claims an N-day window. Either carry {N_days_ago} in the filter, or ' + + `drop the window from the name:\n ${claiming.join('\n ')}`, + ).toEqual([]); + }); + + it('the vocabulary does not fire on names that merely contain a number', () => { + // False positives would be worse than the gap: they push authors to delete + // honest labels. A count, a quarter name and an hour-scoped SLA all name + // no day window. + for (const label of [ + 'Top 10 Articles', 'Q4 Pipeline', 'SLA > 4h', 'Tier 1 Accounts', + '2026 Renewals', '第 3 四半期', 'Review Queue · Oldest First', + '复核队列 · 最久未复核在前', 'Cola de revisión · Más antiguos primero', + 'レビューキュー · 古い順', + ]) { + expect(dayWindowClaims(label), label).toEqual([]); + } + }); + it('the quarter view pins BOTH halves of the period key', () => { // `period_start` alone merges the quarter row with the MONTH row that opens // the same quarter (Q3 and July both start on the 1st); `period` alone is @@ -608,3 +763,214 @@ describe('closing_this_quarter returns this quarter only, on the real engine (#7 expect(missing, `untranslated empty-state copy:\n ${missing.join('\n ')}`).toEqual([]); }); }); + +// ─── Runtime: why the third case is fixed at the NAME, not the filter (#769) ─ + +/** + * `crm_knowledge_article.stale_articles` is the day-window family's origin, and + * the one surface under this rule where the honest fix was to correct the NAME. + * The two before it were not a precedent for that: `this_quarter_forecasts` and + * `closing_this_quarter` carried the time claim in their METADATA label — the + * authored contract itself promised a quarter — so the filter had to deliver + * one. Here the metadata label says 'Review Queue · Oldest First' and promises + * only an ordering, which the `last_reviewed_at asc` sort delivers exactly; the + * "> 180d" window existed nowhere but the four locale packs. + * + * That alone would not settle it — a promise four translators independently + * wrote is worth taking seriously as product intent. What settles it is that + * the honest window is NOT EXPRESSIBLE in a view filter, measured here rather + * than argued: + * + * 1. The macro resolves, and to the START of the calendar day 180 days ago, + * so `less_than` excludes that whole day (#3777's day-boundary convention + * on a `datetime` comparand) — the right sense for "> 180d". + * 2. `$lt` does not match null or absent values, so the window DELETES + * never-reviewed articles — the rows a review queue most needs. + * 3. The honest condition is therefore a DISJUNCTION, "older than the window + * OR never reviewed", and a view `filter` is a flat, strict array of + * `{field, operator, value}` rules combined with AND. Written the only way + * the grammar allows, it returns ZERO rows. + * 4. The engine itself answers the question correctly with `$or`. The limit + * is the view-authoring grammar, not the data layer — which is why this is + * recorded as a measurement with an issue behind it, not as a workaround. + * + * If a future spec gives view filters a disjunction, delete this block's fourth + * assertion, add the window, and give the tab an `emptyState` (the #746 rule: + * scoping makes "empty" newly reachable). + */ +describe('stale_articles: the 180-day window, measured on the real engine (#769)', () => { + let ql: Awaited>; + let api: AnyRec; + const view = listViews.find((v) => v.name === 'stale_articles')!.view; + + // Fixtures are pinned at module load; the boundary probes sit on the UTC day + // 180 days back, computed the same way the resolver does. + const DAY_MS = 86_400_000; + const nowMs = Date.now(); + const daysAgo = (d: number) => new Date(nowMs - d * DAY_MS).toISOString(); + const day180 = new Date(nowMs - 180 * DAY_MS); + const day180StartMs = Date.UTC(day180.getUTCFullYear(), day180.getUTCMonth(), day180.getUTCDate()); + const day180Start = new Date(day180StartMs).toISOString(); + + /** name → what the row's `last_reviewed_at` holds. */ + const fixtures: Array<{ title: string; status: string; last_reviewed_at?: string | null }> = [ + { title: 'reviewed-today', status: 'published', last_reviewed_at: daysAgo(0) }, + { title: 'reviewed-179d', status: 'published', last_reviewed_at: daysAgo(179) }, + { title: 'boundary-day-opens', status: 'published', last_reviewed_at: day180Start }, + { title: 'boundary-day-closes', status: 'published', last_reviewed_at: new Date(day180StartMs + DAY_MS - 60_000).toISOString() }, + { title: 'reviewed-181d', status: 'published', last_reviewed_at: daysAgo(181) }, + { title: 'reviewed-400d', status: 'published', last_reviewed_at: daysAgo(400) }, + { title: 'never-reviewed-null', status: 'published', last_reviewed_at: null }, + { title: 'never-reviewed-absent', status: 'published' }, + // Ancient, but not published — keeps the status half of the filter honest. + { title: 'draft-ancient', status: 'draft', last_reviewed_at: daysAgo(900) }, + ]; + + /** The window rule as it WOULD be authored, in canonical operator spelling. */ + const WINDOW_RULE = { field: 'last_reviewed_at', operator: 'less_than', value: '{180_days_ago}' }; + + const titles = (rows: AnyRec[]) => rows.map((r) => r.title).sort(); + + beforeAll(async () => { + ql = await ObjectQL.create({ + datasources: { default: new InMemoryDriver({ persistence: false }) }, + objects: { + crm_knowledge_article: { + name: 'crm_knowledge_article', + fields: { + id: { type: 'text' }, + title: { type: 'text' }, + status: { type: 'text' }, + // `datetime`, NOT `date` — the property assertion 1 rests on. + last_reviewed_at: { type: 'datetime' }, + }, + }, + } as never, + }); + api = ql.createContext({ isSystem: true, userId: 'usr_1', tenantId: 'org_1' } as never); + for (const f of fixtures) await api.object('crm_knowledge_article').insert(f); + }); + + afterAll(async () => { + await ql?.close(); + }); + + it('the shipped filter is the status test alone, and the label promises only an order', () => { + expect((view.filter ?? []).map((r: AnyRec) => [r.field, r.operator, r.value])).toEqual([ + ['status', 'equals', 'published'], + ]); + expect(view.sort).toEqual([{ field: 'last_reviewed_at', order: 'asc' }]); + expect(dayWindowClaims(view.label)).toEqual([]); + }); + + it('the fixture holds rows on both sides of the window AND rows with no value', async () => { + // Premise of everything below: on an all-stale or all-stamped table these + // assertions would pass without the window or the null case doing anything. + const all: AnyRec[] = await api.object('crm_knowledge_article').find({ where: {} }); + expect(all.length).toBe(fixtures.length); + expect(all.filter((r) => r.last_reviewed_at == null).length).toBe(2); + }); + + it('{180_days_ago} resolves, and lands on the START of the day 180 days back', async () => { + // Not zero rows (the macro shipped unresolved) and not every row (the + // #769 defect): the same three-way pin the blocks above make, on a + // parameterised token. The second assertion is the day-boundary one — the + // macro is compared against the equivalent literal rather than described. + const macro: AnyRec[] = await api.object('crm_knowledge_article').find({ + where: { last_reviewed_at: { $lt: '{180_days_ago}' } }, + }); + expect(macro.length).toBeGreaterThan(0); + expect(macro.length).toBeLessThan(fixtures.length); + expect(titles(macro)).toEqual(['draft-ancient', 'reviewed-181d', 'reviewed-400d']); + + const literal: AnyRec[] = await api.object('crm_knowledge_article').find({ + where: { last_reviewed_at: { $lt: day180Start } }, + }); + expect(titles(literal)).toEqual(titles(macro)); + }); + + it('as an exclusive upper bound it excludes the whole 180-days-ago day', async () => { + // The three-state boundary: 179d is fresh, every hour of the 180th day is + // fresh, 181d is stale. `last_reviewed_at` is `Field.datetime`, so a row + // reviewed at 23:59 on the boundary day would fall INTO a window that used + // `less_than_or_equal` against a day-start comparand — a whole day of + // articles flipping side on the operator alone. + const stale = titles(await api.object('crm_knowledge_article').find({ + where: { status: 'published', last_reviewed_at: { $lt: '{180_days_ago}' } }, + })); + expect(stale).not.toContain('reviewed-179d'); + expect(stale).not.toContain('boundary-day-opens'); + expect(stale).not.toContain('boundary-day-closes'); + expect(stale).toContain('reviewed-181d'); + }); + + it('the window DELETES never-reviewed articles, which today’s filter returns', async () => { + // The measurement that decided #769. Both spellings of "no value" — an + // explicit null and an absent key — fall out of a `$lt`, and a review queue + // that hides never-reviewed articles hides its most overdue population. + const shipped: AnyRec[] = await api.object('crm_knowledge_article').find({ + where: whereFromViewFilter(view.filter), + }); + expect(titles(shipped)).toContain('never-reviewed-null'); + expect(titles(shipped)).toContain('never-reviewed-absent'); + + const windowed: AnyRec[] = await api.object('crm_knowledge_article').find({ + where: whereFromViewFilter([...(view.filter ?? []), WINDOW_RULE]), + }); + expect(titles(windowed)).toEqual(['reviewed-181d', 'reviewed-400d']); + expect(titles(windowed)).not.toContain('never-reviewed-null'); + expect(titles(windowed)).not.toContain('never-reviewed-absent'); + }); + + it('the disjunction is not an authorable filter rule — `or` is not a key', async () => { + // A rule is `{field, operator, value}` and the object is STRICT, so there + // is no `or`, no nesting and no logic key to hang a second branch on. This + // asserts the KEY is not an authoring surface (unrecognized_keys), which is + // the fact in question — the operator vocabulary itself is fine. + const canonical = ViewFilterRuleSchema.safeParse(WINDOW_RULE); + expect(canonical.success, 'the window rule itself is authorable').toBe(true); + + const disjunction = ViewFilterRuleSchema.safeParse({ + ...WINDOW_RULE, + or: [{ field: 'last_reviewed_at', operator: 'is_empty' }], + }); + expect(disjunction.success).toBe(false); + expect(disjunction.error!.issues.map((i) => i.code)).toContain('unrecognized_keys'); + }); + + it('and spelled the only way the grammar allows, it returns nothing at all', async () => { + // Two rules on one field AND together. `last_reviewed_at` cannot be both + // older than the window and empty, so the tab would go blank — the failure + // that looks exactly like "nothing needs review". + const anded: AnyRec[] = await api.object('crm_knowledge_article').find({ + where: { last_reviewed_at: { $lt: '{180_days_ago}', $null: true } }, + }); + expect(anded).toEqual([]); + }); + + it('the ENGINE answers the question correctly — the limit is the view grammar', async () => { + // Stated positively so the constraint above cannot be misread as "the data + // layer cannot do this". It can; a view just has no way to ask. + const rows: AnyRec[] = await api.object('crm_knowledge_article').find({ + where: { + status: 'published', + $or: [ + { last_reviewed_at: { $lt: '{180_days_ago}' } }, + { last_reviewed_at: { $null: true } }, + ], + }, + }); + expect(titles(rows)).toEqual([ + 'never-reviewed-absent', 'never-reviewed-null', 'reviewed-181d', 'reviewed-400d', + ]); + }); + + it('a token outside the macro grammar is rejected, not silently unresolved', async () => { + // The parameterised family is a GRAMMAR, not a fixed list: {180_days_ago} + // and {181_days_ago} both resolve. A misspelling throws and names the fix, + // so the 16.1.0 silent-empty mode is unreachable on this path too. + await expect( + api.object('crm_knowledge_article').find({ where: { last_reviewed_at: { $lt: '{180_days_agoo}' } } }), + ).rejects.toThrow(/Unresolvable filter placeholder/); + }); +}); From 32dffb55b0ef3431e0813bdbf32d72b384c080b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 19:21:23 +0000 Subject: [PATCH 2/2] docs(comments): name the write path that actually leaves a published article unreviewed (#769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The note added in the previous commit said `last_reviewed_at` is nullable without saying how a published article reaches that state, and the measurement behind it was taken on a bare `ObjectQL.create()` — which does NOT register the engine's `sys_fetch_previous_update` builtin. Re-measured with a faithful replica of that builtin bound (priority 5, beforeUpdate, object '*'), the first reading was wrong in one direction and right in another: single-row update prev-fetch sets previous, hook sees previous.status "published" and re-stamps — a null write is overwritten multi:true update input.id is undefined, so the builtin fetches nothing, the hook sees no status and stamps nothing — a bulk write puts null straight onto published rows, and a bulk edit never marks them reviewed So the never-reviewed population is not produced by ordinary editing; it is produced by bulk loads and mass edits — which is how an org imports an existing knowledge base. That makes the window's loss worse than "rare and pathological", not better: the rows it would hide are the freshly imported ones, which no human has ever reviewed. Comment only. The runtime block's assertions are about `$lt`, null and the filter grammar, none of which depend on hooks, so they are unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VHrPAGEgFDoHjphqYG4BMa --- src/views/knowledge_article.view.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/views/knowledge_article.view.ts b/src/views/knowledge_article.view.ts index 721232d40..ed384bf8b 100644 --- a/src/views/knowledge_article.view.ts +++ b/src/views/knowledge_article.view.ts @@ -98,10 +98,18 @@ export const KnowledgeArticleViews = defineView({ // // 2. Expressible is not the same as expressible WITHOUT LOSS. A review // queue's most overdue population is the never-reviewed one, and - // `last_reviewed_at` is nullable: a published article can carry no - // review timestamp at all. Measured on the same engine, `$lt` does - // not match null or absent values, so a bare window silently deletes - // exactly those rows. The honest condition is "earlier than + // `last_reviewed_at` is nullable in exactly the case that matters. + // On the SINGLE-record path `knowledge_article_publish_timestamps` + // keeps it stamped — the engine's `sys_fetch_previous_update` builtin + // hands the hook a `previous`, so publishing and every later edit + // re-stamp. On the BULK path (`multi: true`) there is no + // `input.id`, that builtin cannot fetch anything, the hook sees no + // status at all and stamps nothing — so a bulk load or mass edit, + // which is how an org imports an existing knowledge base, leaves + // published articles with no review timestamp (measured; filed + // separately). `$lt` matches neither null nor an absent key, so a + // bare window deletes precisely those rows — the imported ones, the + // least reviewed of all. The honest condition is "earlier than // {180_days_ago} OR empty" — and a view `filter` cannot say that. It // is a FLAT, strict array of `{field, operator, value}` rules // (`ViewFilterRuleSchema`) combined with AND; there is no `or`, no