spec(content-drive): Title / All Fields search scope + literal-text search terms (#37479, #37532) - #37518
spec(content-drive): Title / All Fields search scope + literal-text search terms (#37479, #37532)#37518KevinDavilaDotCMS wants to merge 36 commits into
Conversation
…h box Spec-Kit PR 1 for #37479. Carries spec.md alone. Settles the four decisions the issue left open: All Content stays the default, Title mode matches the contentlet title only, the scope lives in the URL rather than a user preference, and sorting is untouched. Four premises verified against main and corrected in the spec: - Nothing sorts by score today — the default is modDate:desc on both sides, and the only trace of score sorting is a stale comment. Open decision 4 is void. - Folders and links never reach Elasticsearch; they are already matched on name only, in both scopes, so the selector governs the contentlet clause alone. - buildPureESQuery is unreachable under the shipped heuristic, so it stays out of scope. - A scope written into the filters on every selection would light up "Clear all" on an unfiltered drive, because hasNonDefaultFilters counts every key but two. Hence FR-021: the scope counts as filter state only while it differs from the default. Refs #37479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
One naming ask, because our two specs are about to collide in prose and then in the same request object. This one introduces a scope that says which fields a search reads. #37487 introduces one that says where in the site you are browsing: All, the site root, or System Host. Bare "scope" now names either, and a reader landing on either spec cannot tell which is meant. Worse, both end up as fields on the same Content Drive search request, so the ambiguity outlives the specs. I have already renamed mine to spell browse scope everywhere, with a note in Key Entities pointing at yours so the two cannot be conflated. Could you do the matching half here: say search scope in the prose rather than plain "scope", and name the field Then the pair reads unambiguously wherever they meet: Unrelated to the naming, and only because it touches the same sentence I was reading: your spec's "All Content" label is the one I flagged above as really meaning all fields. If that wording changes, the field values change with it, so it is worth settling before the name goes on the wire. Comment written by Claude on @zJaaal's behalf. |
|
One more on the contract, following the naming note above: I think the search scope belongs inside
To be clear that this is not a general "scopes go in filters" rule: the browse scope in #37487 stays at the top level, for two reasons. It qualifies Related, since it came up while looking at this: we also considered letting the path alone carry the browse scope, with no field at all, mirroring how the Content Drive URL now encodes it in one value. It does not survive contact with this endpoint, and for a reason that matters to your spec too: Worth flagging so nobody invokes consistency in either direction: Comment written by Claude on @zJaaal's behalf. |
ihoffmann-dot
left a comment
ihoffmann-dot
left a comment
There was a problem hiding this comment.
Mid: possible conflict with ADR-0018 (Database-First Search for Content Drive)
ADR-0018's routing table states explicitly:
Title → DB ∪ Index (
contentlet.titlecolumn ∪ index records)
This union exists specifically to preserve read-your-writes: a just-saved/renamed item that the index hasn't caught up to yet must still be findable via the DB title column.
FR-008/FR-010 define Title scope purely as a narrower Elasticsearch query (drop catchall, drop the leading wildcard, gate on title). Nothing in the FR/SC list references consulting the contentlet.title DB column, and no acceptance scenario or success criterion covers "a document created/renamed just before search, not yet indexed, searched by title."
The ADR does note the DB ∪ Index union is gated on a separate title-persistence effort and may not be fully implemented in the current codebase either, so this may already be a pre-existing gap rather than something this feature introduces. It's worth an explicit call before /speckit-plan
No other findings. No other ADRs in platform-adrs were found that this spec contradicts.
…, ADR-0018 Four asks from @zJaaal and one from @ihoffmann-dot, all settled in a new Review Decisions table (rows 5-8) so PR 1's approval records them. Renamed the wide option "All Content" -> All Fields (values TITLE / ALL_FIELDS). "All Content" described a set of content, which is what #37426's browse scope All genuinely means; this scope widens which *fields* are read, not which content is searched. The issue's original wording stays recorded verbatim in Input. Named the concept "search scope" throughout and the wire field filters.searchScope, inside the existing filters object rather than at the top level. AbstractQueryFilters is {text, filterFolders} today and filterFolders' own Javadoc says "when text is provided" — both members qualify the text search, which is exactly what the search scope does. FR-025 makes a scope with no text a contract error rather than a rule to remember. The browse scope stays top level for the opposite reasons. FR-017 now names the Asset Picker as the caller the optional-field requirement protects, with its actual call site, so a future change to the default has to confront it by name. FR-024 pins the change to the text-search branch and names the three other doors into the same listing (WebAssetHelper, BrowserAjax, DotCMSMacroWebAPI) that would widen the blast radius from two callers to six. SC-008 measures it. ADR-0018's Title -> DB ∪ Index routing gets its own section and an explicit deferral rather than silence. The ADR defers its own union: it states contentlet.title is "not reliably populated" and that fixing that is a separate issue. Verified that no text search consults the column today — Premise Correction 5 shows ContentDriveHelper:180-184 sets useElasticsearchFiltering(true) unconditionally when text is present, so the SQL ILIKE text path is unreachable for Content Drive and contentlet matching is index-only in both scopes. Title scope inherits the existing index-lag exposure rather than creating it, and FR-026 keeps the union additive for when the gated work lands. Also adds FR-022 (the control must explain what each option matches — two labels do not carry the distinction on their own). Refs #37479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KevinDavilaDotCMS
changed the title
|
@zJaaal — naming and placement both taken, in full. Naming. "Search scope" everywhere in prose, never bare "scope", and Placement — Your asymmetry argument is what convinced me the two placements are not inconsistent: the search scope should be cleared by "Clear all" — that is FR-020 — whereas a browse scope living in Noted on @ihoffmann-dot — ADR-0018 gets its own section and an explicit call rather than silence. Short version: deferred, with reasons, and I think the evidence is a little stronger than "possibly a pre-existing gap".
There is also a new edge case covering the scenario you said nothing covered — an item saved or renamed moments before the search, not yet indexed — stating plainly that it is missing in both scopes, as it is today. Premise Correction 5 was worth the trip on its own: it rules out a second, SQL-shaped text match that a Title scope could have silently failed to narrow, which would have been a real hole in FR-010. Pushed as |
…terms #37532 (High, customer ticket 39185) reports that content type field filters in the Content Search portlet return "no results found" for values containing Lucene query-syntax characters. Verified that the same defect lives in Content Drive's own search box, in the exact clause FR-010 rewrites, so the two issues are resolved together. Premise Correction 6 records what GlobalSearchAttributeStrategy actually does. Escaping is applied to its final clause only (:46-48); the mandatory gate at :37-38 is built from the raw value, so a term like "ABC (XETRA: DB)" yields invalid query_string syntax. Its private SPECIAL_CHARS_TO_ESCAPE regex (:20) is missing "/", which is #37532's fifth acceptance criterion verbatim, while LuceneQueryUtils.escape — vendor-neutral, documented, already used by TextFieldStrategy — covers the full reserved set. The split at :40-45 has no empty-token filter, so consecutive separators emit a term-less title:^5 clause. And a query that fails to parse is swallowed at BrowserAPIImpl:893-895, returning an empty set, which is why the user is told their content does not exist. Nothing sanitizes upstream: ContentDriveHelper:183 passes text() raw. The parallel field-filter path is already correct — TextFieldStrategy escapes and filters empty tokens — so #37532's Content Drive field-filter criterion is verification, not implementation. FR-030 and SC-012 pin it. FR-009 and SC-002 had to yield. Escaping the gate changes All Fields results for affected terms, which contradicts the no-regression promise as approved, so the carve-out is written down and bounded rather than smuggled in: reserved characters and consecutive separators only, every other term unchanged, and the tests that change enumerated in the PR. Adds User Story 4 (P1, listed fourth), FR-027 to FR-031 and SC-009 to SC-012. FR-031 keeps the fix in the shared strategy rather than a Content-Drive-only branch, so the Search portlet and the Relationships dialog stop mis-parsing reserved characters too. What this deliberately does not do is stated in Legacy Considerations and in "Why #37532 lands here": the Content Search portlet keeps its current behaviour. #37532 itself directs the improved behaviour to Content Drive rather than to the legacy construction, but the customer on ticket 39185 is using the portlet today, so closing #37532 on this work is the issue owner's call and is flagged rather than assumed. Refs #37479, #37532 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KevinDavilaDotCMS
changed the title
Scope change: #37532 folded in — please re-reviewPushed #37532 (High, customer ticket 39185) reports that field filters return "no results found" for values containing Lucene query-syntax characters — The same defect is in Content Drive's search box, in the exact clause FR-010 rewrites:
Nothing sanitizes upstream — The issue also asks for this directly: "Content Drive is intended to replace the Content Search portlet, so the improved field-level search behaviour should land there." What changed in the spec
Two things I want you to push back on if you disagree1. FR-009 and SC-002 had to yield. Escaping the gate changes All Fields results for affected terms. That contradicts the no-regression promise you approved, so rather than smuggle it in I wrote the carve-out down and bounded it: reserved characters and consecutive separators only, every other term byte-identical, and the tests that change enumerated in the PR. If you'd rather the escaping ride in its own PR to keep FR-009 absolute, say so — it's a defensible call, it just costs a second trip through this code. 2. The Content Search portlet is NOT fixed. Also worth noting: the fix lands in One thing that was already fine: Content Drive's field filters escape correctly today via |
Resolves the Content Drive half of #37532 (High, customer ticket 39185): filtering on a value containing ":", "(" or "/" returned "No results found" for content the user was looking at. GlobalSearchAttributeStrategy escaped only its final clause, so the mandatory gate — the clause that decides whether a document matches at all — was built from raw user input. A term like "ABC Bank (XETRA: DB)" produced a query Elasticsearch could not parse; the failure was logged and discarded, and the caller received an empty result indistinguishable from a genuine miss. Three defects, one cause: - The gate carried the raw term. Now the term is escaped once, up front, and every clause uses the escaped value. The "*" wildcards the strategy appends are added after escaping so they stay live. - The private SPECIAL_CHARS_TO_ESCAPE regex omitted "/" entirely. Replaced with LuceneQueryUtils.escape — the helper TextFieldStrategy already uses, so the two strategies no longer disagree about the reserved set, and a character walk rather than a regex, which ADR-0009 calls for ahead of the ES→OpenSearch move. - Consecutive separators emitted a term-less "title:^5" clause. Empty tokens are now dropped, matching TextFieldStrategy. A query that fails to execute is no longer reported as a search that found nothing — but only for callers that ask. BrowserQuery gains surfaceQueryFailures, off by default, and Content Drive is the only caller that opts in. The assets REST API, the legacy admin browser and the Velocity viewtool keep receiving today's empty result. The failure is still logged either way. Also lays the contract groundwork for #37479: SearchScope, and filters.searchScope defaulting to ALL_FIELDS so a request omitting it is processed exactly as before. Nothing reads the scope yet. Evidence, against real PostgreSQL and OpenSearch: - ContentDriveLiteralTextSearchTest 3/3. The ticket 39185 headline is found when searched verbatim, all 19 reserved characters are findable by their own text, and field filters match literally. The first two failed before this change — that Red is what proved the defect reported against the Content Search portlet also reaches Content Drive. - 21 unit assertions green. GlobalSearchAttributeStrategyBaselineTest pins the boundary: two terms without reserved characters produce byte-identical queries, and the three that changed carry their before/after in-line. Those three are the complete list of pre-existing expectations this commit changes. The Content Search portlet's own ContentletAjax path is deliberately untouched — #37532 directs the improved behaviour to Content Drive rather than to the legacy construction. Refs #37532, #37479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes FR-029 for #37532. The backend now distinguishes "the query failed" from "nothing matched"; this puts that on screen. The store already set status ERROR on a failed search, but nothing rendered it: the grid simply went empty. That is the defect the customer reported — content they were looking at appeared not to exist. A banner now sits above the listing when the last search failed, with a retry. Above rather than instead of: replacing the grid would hide the thing the message explains, and it would also make the listing unreachable to component queries, which broke seven existing specs on the first attempt. Also removes a stale comment in the store claiming score-descending sorting for title search. Nothing has sorted by score since #36688 — the default is modDate on both sides — so the comment described behaviour the code does not have (progressive enhancement; spec Premise Correction 1). Four specs cover the banner: it appears, it stays alongside the listing it explains, it carries role="alert", and the retry re-runs the search. All 162 specs in the shell suite pass. Refs #37532, #37479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements the backend half of #37479. filters.searchScope now selects which fields a term is matched against: ALL_FIELDS (the default, and today's behaviour) or TITLE. The Title clause is a sibling of GlobalSearchAttributeStrategy, not a mode inside it. That strategy also serves the Search portlet and the Relationships dialog through the Lucene Query Builder service, and neither asked for a narrower query. It deliberately uses neither catchall nor a leading wildcard in its mandatory gate: catchall aggregates every field, which is the breadth the scope exists to avoid, and title_dotraw is a keyword field where *term* scans every distinct raw title while term* is a prefix seek. #36688 removed a leading wildcard for that reason. Known trade-off, signed off: dropping it also drops mid-token matching, so "1004" will not find IMG_1004.jpeg in Title scope. All Fields keeps it (#36791). Also reverts the failure-raising added in the previous commit. Two reasons found while building on it: - With FR-027's escaping in place, no user input can break the query, so what remained was infrastructure failure. The only way to force one through the public API was a 2000-word term hitting the clause ceiling. - It broke testMalformedDateBoundIsSafe, which asserts that a Lucene injection attempt is escaped, matches nothing, and produces no 500. Raising the failure told an attacker their probe had landed. The front end keeps surfacing the failures it can observe as an error banner, so an unexplained empty grid is still not what a failed search looks like. #37532's UI criterion is therefore partially met, and spec.md's FR-029 needs amending and re-approval before PR 2 — recorded in tasks.md rather than left implicit. 42 integration assertions green across the four Content Drive classes: the scope narrows as specified, an omitted scope is indistinguishable from ALL_FIELDS, folder matching is identical in both scopes, a scope without text is refused, and the customer fix and security guarantee both still hold. Refs #37479, #37532 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… box Completes the user-facing half of #37479. The search box now carries a two-option control saying which fields the term is matched against, and the placeholder follows it so the box states what it will do before the user types again. The control is composed in Content Drive's own store-aware wrapper rather than in the shared DotSearchInputComponent. That component is deliberately presentational so the AssetPicker can reuse it without a store, and composing here means the AssetPicker cannot acquire the control by accident — its 12 spec files pass unmodified, which is the assertion rather than a side effect. The scope is stored under its own filter key, never under `title`. That key holds the search TERM, and a scope whose value is 'TITLE' sitting beside a filter named `title` is a collision waiting to happen. It is written into the filter state only while it differs from the default, and deleted when it returns to it. Not cosmetic: hasNonDefaultFilters counts every filter key but two, and that signal shows the chip bar's "Clear all" — writing the key on every selection would offer "Clear all" on a drive with nothing filtered at all. Mirrors how setGlobalSearch already deletes its own key when the term empties. Re-selecting the active scope is ignored: the results cannot change, and patchFilters would reset the user to page 1 for nothing. On the wire, searchScope is sent only when a term is present and the scope is not the default, so a request from any other caller is byte-identical to what it was before this field existed. PrimeNG usage validated against the component API: pTooltip is a directive on a wrapper rather than a SelectButton input, and the control is named with a plain aria-label — ariaLabelledBy takes an element id, not text, which the first attempt got wrong. 30 of 30 portlet spec files green, including 9 new ones covering the control, the placeholder, the no-op re-selection, the accessible name and the explanation. Refs #37479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…palette Three problems visible as soon as the control rendered on a real page. The width moved. p-select sizes itself to its selected label, so "Title" and "All Fields" produced two different widths — and since the input took whatever space was left, choosing a scope resized the text field under the user's cursor. The addon is now a fixed 140px, so neither the white field nor the group as a whole ever moves. Neither label was readable. Both rendered truncated — "T..." and "All ..." — because the select was sized by a layout that had not left it room. The fixed width fixes this too; 140px is what the filter chips below already use and it fits the longer label whole. The colours belonged to the default input theme rather than to this page. The border and label colour now match dot-chip-filter's own border-slate-200 and text-slate-600, so the search box and the chip row under it read as one family. 140px is not an invented number: the chips carry min-w-[140px] for the same reason, having solved the same jitter first. Taken as a fixed width rather than a minimum, because a minimum still lets the content push it. 30 of 30 portlet spec files green. Refs #37479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntend standards My changes broke two of them, and the second is why the last round of CSS had no visible effect anyway. The search input component carried an inline template and an inline `styles` block. ANGULAR_STANDARDS requires logic, template and styles in separate files; the template now lives in its own .html and the styles block is gone entirely rather than moved to a .scss. Gone rather than moved because the styling belongs in PrimeNG's PassThrough. `pt` is the component's supported way into its own internals, so the addon becomes transparent and the label gets its padding without any stylesheet and without ::ng-deep reaching past a component's API into its markup. STYLING_STANDARDS puts PrimeNG and Tailwind first and custom CSS last; this had it backwards. The failed-search banner had the same problem in a smaller way: a hand-rolled div with a heading, a paragraph and a button, in a shell that already imports MessageModule and already uses `p-message` with `pt` three lines further up. It is now a p-message with severity="error", which brings the styling, the icon and the semantics. Its heading key is dropped from Language.properties — the severity says what the heading was saying. No .scss file was added, no ::ng-deep survives in the working tree, and neither touched component carries an inline template or styles. 30 of 30 portlet spec files green. Refs #37479, #37532 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d `!`
Read PrimeNG's own compiled CSS to find the real cause rather than
guessing again. Two separate bugs, both hiding behind the same symptom
(two disconnected boxes with a visible gap):
1. PrimeNG injects its component styles at runtime, after Tailwind's
compiled stylesheet has already loaded. A plain Tailwind class and a
PrimeNG rule targeting the same element (e.g. `.p-select { background:
...; border: ... }`) have equal specificity, and on a tie the
later-inserted stylesheet wins — PrimeNG's. Every override in this
component now carries Tailwind v4's `!` (important) modifier, which is
the exact pattern PrimeNG's own inputgroup+select demo uses
(`border-none! shadow-none! bg-transparent!` in their docs).
2. The addon side (`p-inputgroup-addon`) is a direct child of
`p-inputgroup` and gets its own connecting rules for free — top/bottom
border, right border and right-side rounding, all correct by default,
confirmed straight from @primeuix/styles' inputgroup CSS. The search
INPUT side never got the matching treatment, because `dot-search-input`
is a wrapper component sitting between `p-inputgroup` and the actual
`<input>`, and PrimeNG's connecting CSS only rewires DIRECT
`.p-iconfield`/`.p-component` children — the wrapper breaks that chain,
so the input kept its own default border and full rounding on every
side, rendering as its own separate box no matter what was applied to
`p-inputgroup` itself.
The second bug needed an actual capability, not a workaround: added an
optional `inputClass` to the shared `DotSearchInputComponent` so a host
that nests it in an input group can flatten its connecting edge from
outside. Empty by default, so every other consumer (AssetPicker
included, five usages) is unaffected — verified all still pass.
Also switched from the `pTooltip` directive to Select's own `tooltip` /
`tooltipPosition` inputs, dropping the now-unused TooltipModule import;
Select implements them with the same underlying Tooltip directive, so the
existing directive-instance assertion still holds.
390+ tests green: 30 portlet spec files, 13 ui spec files (394 tests,
covering the AssetPicker and the shared search input), both libraries
typecheck clean.
Refs #37479
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ecipe Threw out the `p-select` + Tailwind `!important` approach entirely and rebuilt on https://v21.primeng.org/inputgroup#button, the pattern PrimeNG documents for exactly this: a trigger inside an input-group addon. The trigger is now a plain `pButton`, directly inside `p-inputgroup-addon` with no wrapper and no override classes. Confirmed straight from @primeuix/styles' own inputgroup CSS that this combination is self-styling: `.p-inputgroupaddon:has(.p-button)` zeroes the addon's padding and `.p-inputgroupaddon .p-button` zeroes the button's radius, which is exactly why the official demo needs no custom CSS either. No `!important` was ever going to be necessary here — the double-boxed look came from fighting a component (`p-select`) that isn't part of that recipe, not from insufficient specificity. The trigger opens a `p-popover` + `p-listbox`, styled with `CHIP_FILTER_POPOVER_PT` / `CHIP_FILTER_LISTBOX_PT` / `CHIP_FILTER_SCROLL_HEIGHT` — the exact constants a sibling in this same portlet (`dot-content-drive-workflow-filter`) already uses for its own dropdown. The panel now matches every other filter in the toolbar instead of inventing its own look. The one real gap — the search input's connecting edge, since its host component sits between `p-inputgroup` and the actual `<input>` and breaks PrimeNG's structural CSS — is closed without `!important` or a stylesheet. `DotSearchInputComponent` gains an optional `inputDt` input, forwarded as `[dt]` onto the real `<input pInputText>`. A design-token override sets the CSS custom property the component's own stylesheet already reads, so it applies by redefinition rather than by winning a specificity fight — which is why no `!` is needed there either. The override value itself reuses `{form.field.border.radius}`, the same token reference the active theme's own preset uses internally, rather than a hardcoded pixel guess. Empty by default; the five other consumers of the shared component are unaffected. 1398 tests green in the portlet, 394 in `ui` (13 files, the AssetPicker included), both libraries typecheck clean. Zero `!important`, zero extra wrapper divs, zero custom stylesheets. Refs #37479 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…he input
Two mismatches, both traced to a specific cause rather than patched by
guessing.
The button resized with its label because a p-button has no width rule
of its own — width is purely content-driven, so "Title" and "All Fields"
produced two different button sizes and the whole group visibly moved.
Fixed with a plain Tailwind `w-[140px]`, the same width the filter chips
below already use; there is nothing in Button's own stylesheet to
compete with on this property, so no override mechanic was needed at all.
The border colours never matched because Button and InputText draw from
different token families in this theme. `button.secondary.border.color`
resolves to `{surface.700}`; `inputtext.border.color` resolves to
`{form.field.border.color}`. Confirmed straight from the Lara preset
source, not guessed from how the colours looked. `TRIGGER_DT` repoints
the button's border at the same token the input already uses, so the two
stay identical if the theme itself changes rather than silently drifting
apart again.
Also truncates the label and shrinks the chevron so the longer "All
Fields" text doesn't wrap or push the icon around inside the now-fixed
width.
1398 tests still green across all 30 portlet spec files. Zero
`!important`, zero extra divs — the width is Tailwind because it's
layout, the border is `dt` because it's a PrimeNG token PrimeNG itself
needed to be told about.
Refs #37479
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous fix only repointed the border and made things worse: this
theme's `button.secondary` sets `borderColor: {surface.100}`, identical
to its own `background: {surface.100}` — the default secondary button's
border is invisible by design, matching its own fill. Overriding only the
border color left a visibly grey-filled button sitting next to a white
field, a bigger mismatch than the one being fixed.
`TRIGGER_DT` now repoints all three: background, border colour and text
colour, each to the exact token InputText already uses
(`form.field.background`, `form.field.border.color`, `form.field.color`).
Button and InputText are simply different PrimeNG components with
separate token families end to end, not a single misaligned property, so
partial repointing was always going to leave a mismatch somewhere.
1398 tests still green, typecheck clean.
Refs #37479
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e placeholder
Replaces the manually-applied `border-none!` with `[pButtonPT]`'s
`root.style`. `pButtonPT`'s value reaches the host through the
`[style]`/`[class]` HOST BINDINGS `Bind` (the directive backing `pButton`)
already declares — the same mechanism as any `[style]` binding, applied
as a real inline style — so it wins the cascade over PrimeNG's own
injected `.p-button-secondary` rule unconditionally, the same guarantee
`!important` gave without reaching for it.
Also freezes the placeholder. It no longer describes the active scope:
the `[placeholder]` binding is removed entirely so the shared
`DotSearchInputComponent`'s own default ("Search") always applies,
regardless of Title vs All Fields. The `$placeholder` computed is deleted
rather than left unused.
Spec note: this narrows FR-003 ("The search input's placeholder MUST
describe the active scope"), decided directly against the approved spec
rather than derived from it. Recorded here as a marker; the amendment
itself belongs in tasks.md alongside the FR-029 one already pending
re-approval before PR 2.
1398 tests still green, typecheck clean, zero `!important` in the
component's own code (two mentions left are prose, inside a comment
explaining why one is no longer needed).
Refs #37479
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ases (T036) BrowserAPITest gains one test pinning FR-024/SC-008 by construction: `ContentDriveHelper` is the only caller in the codebase that ever sets `useElasticsearchFiltering`, so a BrowserQuery built the way the other three callers (WebAssetHelper, BrowserAjax, DotCMSMacroWebAPI) build it — text filter set, that flag left at its default — is asserted to opt out of ES filtering on its own and still find a reserved-character title via the ordinary SQL path, unaffected by the Title-scope or literal-text changes. ContentDriveResource's Postman collection gains a "Search Scope Tests" folder: C-1 (omitted scope behaves like ALL_FIELDS), C-2 (explicit ALL_FIELDS matches C-1), C-3 (TITLE excludes a body-only match), C-4 (unrecognized value rejected, message names it), and two cases for "no text" rather than one. Splitting C-5 into 5a/5b is a real finding, not tidying: `text` is a required attribute on the `@Value.Immutable` QueryFilters, so a request whose JSON omits the `text` key never reaches ContentDriveHelper's own FR-025 check at all — Jackson's deserialization rejects it first, with a generic "required attributes not set [text]" message that never mentions `searchScope`. Only `text: ""` (present but empty) reaches the FR-025-specific message. Both are legitimate 400s; the wording differs by route, so testing only the message assumed by the first draft would have asserted something Jackson's own error never says. Verified against the running endpoint directly (both return 400, with the expected wording each) before writing the fix, not guessed. The corresponding Java integration test's Javadoc is extended to record why it only covers the ContentDriveHelper-direct route: it constructs QueryFilters through the builder, bypassing Jackson entirely, so the omitted-key case is deliberately left to the endpoint layer that actually parses raw JSON. 46 integration assertions still green across the four Content Drive test classes (ContentDriveSearchScopeTest, ContentDriveLiteralTextSearchTest, ContentDriveKeywordSearchTest, ContentDriveFieldFilterTest), plus the new BrowserAPITest case. Postman collection verified with 26/27 assertions passing on the first run (the one failure was the C-5 wording gap fixed here) and the corrected C-5a/C-5b verified directly against the running endpoint. Refs #37479, #37532 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two requirements narrowed during implementation, both struck through in place and replaced with the amended wording, dated, and reasoned — never silently edited. FR-029 (narrowed 2026-09-14): the stronger "every query failure surfaces as an error" was implemented, then reverted. It cost a security guarantee — ContentDriveFieldFilterTest#testMalformedDateBoundIsSafe requires a Lucene-injection attempt to be escaped, match nothing, and produce no 500, and raising query failures turned that into an error response instead. What ships: the front end surfaces failures it can itself observe; BrowserAPIImpl's internal failures stay logged-only, as before this feature. FR-003 (narrowed 2026-09-15): the placeholder no longer describes the active scope. Direct instruction from the issue owner during UI review, not a defect — FR-003 as written was fully implementable and had been implemented and tested. The placeholder is fixed to the shared search box's own default in both scopes. Header updated to flag both amendments and that re-approval is required before PR 2 — the spec was approved on PR #37518 at 1ce8cdd, which predates both. Full narrative for each amendment already lived in tasks.md as it was decided; this is the spec.md side of the same record, so the approved document reflects what actually shipped rather than diverging from it silently. Refs #37479, #37532 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…2 carve-out)
Found while running the quickstart's Level 3 (T068): a preexisting test,
`test_buildBaseESQuery_withDifferentFilterCombinations`, asserted that an
unescaped "&" survived into the generated query verbatim
(`result.contains("test & special")`). "&" is a Lucene query_string
reserved character, and that assertion was pinning the exact defect
#37532 reports — just with a different symbol than the customer's
":"/"("/"/" . It is precisely SC-002's enumerated carve-out: an ordinary
term stays byte-identical, a term with a reserved character does not.
Now asserts the escaped form (`test \& special`) and explicitly asserts
the raw form no longer survives, with the change reasoned in a comment
rather than silently edited.
This is the entry SC-002 and the plan's "enumerate in the PR" requirement
ask for — the one preexisting test outside the four Content Drive classes
whose expectations this feature changed. All four Content Drive classes
plus this one are green (88 assertions total).
Refs #37479, #37532
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…g field-filter sweep /speckit-converge (2026-09-15) found three gaps, none blocking a P1 story or violating the constitution — two spec self-consistency issues and one test-rigor gap. All three closed here (T073-T075). T073: User Story 1's Acceptance Scenario 1 still read "the placeholder describes an all-fields search" — the pre-amendment behaviour FR-003 no longer requires. Corrected in place, struck through with the reason, matching how the FR-level amendments were already recorded rather than silently edited. FR-003's own amendment note also mis-cited this as "acceptance scenario 2"; it's scenario 1, fixed. T074: User Story 4's Acceptance Scenario 5 and SC-011 both still stated an unqualified "a search request that fails to execute" / "100% of attempts" surfaces an error, without FR-029's amendment carve-out — only front-end-observable failures (network/server errors reaching the browser) are covered; BrowserAPIImpl's internal execution failures stay logged-only, exactly as before this feature. Both narrowed to match, same strikethrough-with-reason treatment. T075: FR-030/SC-012 promise field filters get "the same reserved-set coverage as SC-010," but the two existing tests (ContentDriveFieldFilterTest#testMalformedDateBoundIsSafe, ContentDriveLiteralTextSearchTest#fieldFilterValue_withReservedCharacters_matchesLiterally) each check one fixed string with a handful of reserved characters, not an exhaustive sweep. Added textFieldFilter_matchesEveryReservedCharacterLiterally, mirroring the search-box sweep: one seeded contentlet per character in the full reserved set, found via userSearchable on a Text field. 27/27 green, including the new test — confirms the field-filter path was already correct, the gap was in coverage rather than behaviour. Refs #37479, #37532 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The trigger's tooltip named both scopes at once, which read oddly for a control that only ever shows one of them. Splitting it into a per-option tooltip in the panel lets each option's own explanation stand on its own, and satisfies FR-022 without requiring a specific location for it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…search scope handling Updated the search input component to utilize a predefined input style object, improving maintainability. Enhanced the store logic to ensure that the search scope is cleared when the search term is emptied, preventing stale state issues. Added tests to verify the correct behavior of search scope management, particularly for cases involving hyphenated terms and query syntax characters. Refs #37479, #37532
- Fix the scope selector silently dropping to All Fields: p-listbox toggles (metaKeySelection=false) and emits null when re-clicking the already-active option, which slipped past the "already active" guard and got written into filters as a real scope change, lighting up "Clear all" on an unfiltered drive with no way back from the panel. - Scope the #37532 literal-text escaping fix to Content Drive only. GlobalSearchAttributeStrategy is reverted to its pre-PR behavior (byte-identical), since it also backs the Search portlet's and the Relationships dialog's global search — this matches what spec.md's "Why #37532 lands here" section already said the Content Search portlet would keep. Content Drive gets its own BrowserAPIImpl#buildAllFieldsScopedQuery instead, forked rather than shared, with the same escaping. - Add <, >, = to the Title-scope reserved-character split set so a term like "Sales > 2024" isn't misread by Elasticsearch as a range query. - Default dot-search-input's inputDt to undefined instead of {}, so PrimeNG doesn't load an unused scoped theme + change listener for every consumer that passes no override (AssetPicker included). - Resolve the search-scope option labels eagerly instead of piping them in the item template: PrimeNG's own aria-label on each option reads the raw option.label, which was the untranslated i18n key. Also add aria-expanded to the scope trigger. - Remove the dead ADDON_STYLE field and its stale 15-line comment describing styling that nothing binds. - Soften comments in processSingleESQuery/processMultipleESQueries that overstated the error banner's coverage: a failure inside those methods still returns HTTP 200 with a partial result, which the banner cannot see. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ords title_dotraw is the whole raw title as one keyword term, so a prefix match against it can only succeed when the fragment is a prefix of the ENTIRE title — true, at best, for the first word of a multi-word search term, and never for the ones after it. The old per-word clause carried title_dotraw on every word regardless, paying for a prefix search over a near-one-term-per-document keyword dictionary on words where it could never contribute a match, against title's much smaller per-word vocabulary. Only the first fragment now keeps the title/title_dotraw OR-clause; every later word is title-only. Match set is unchanged (the dropped alternative was structurally unable to match past the first word); this is the follow-up suggested against the PR #37554 SC-003 finding that Title scope measured slower than All Fields, contrary to FR-010's rationale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Updated the DotContentDriveSearchInputComponent to ensure the trigger label and chevron are consistently positioned at the edges of the button, improving user experience when switching scopes. Adjusted the styling to utilize a predefined object for better maintainability. Additionally, modified the DotSearchInputComponent to maintain icon visibility above the input field during focus, addressing z-index issues within input groups. Added tests to verify these layout changes and ensure proper functionality. Refs #37479, #37532
… testing Added a new `ContentDriveSearchScope` component to manage search scope selection between 'Title' and 'All Fields'. Implemented methods for opening the scope panel, selecting options, and verifying active states. Enhanced the `ContentDrivePage` class with methods to capture search payloads and ensure no searches occur during specific actions. Created comprehensive tests for the new functionality, ensuring proper behavior and user experience when interacting with the search scope. Refs #37479
|
Claude finished @zJaaal's task in 5m 8s —— View job Code ReviewNo issues found. Reviewed the search-scope + literal-text changes across backend and frontend:
Every edge case probed resolved to intentional, documented behavior — corroborated by |
github-actions
Bot
added
Area : Backend
…ndling Modified the `onScopeChange` method in `DotContentDriveSearchInputComponent` to accept a nullable `DotContentDriveSearchScope`. Updated the `getFilterValue` method in `dot-content-drive.store.ts` to return a type of `string | string[] | undefined`, enhancing type safety and handling of filter values. Refs #37479
…yload matching Updated the method in to accept an optional parameter for more precise validation of drive search payloads. This allows for better handling of search requests during specific actions, ensuring that only relevant searches are flagged as violations. Additionally, improved test cases to reflect these changes and ensure accurate behavior during search scope interactions. Refs #37479
3 participants
Spec-Kit PR 1 of 2. Carries
spec.mdalone. Review it as a spec, not as code: is this the right problem, scoped right, with criteria a reviewer can tell "done" from? Approval here — not merge — unblocks/speckit-plan.Refs #37479
What it specifies
A scope dropdown beside the Content Drive search input, with two options — Title and All Content — so an author who knows the name of what they want can say so, instead of getting back every document whose body or Story Block happens to mention the word. It is also the cheap query path: the all-content gate is
+(catchall:<value>*^10 OR title_dotraw:*<value>*^2), an all-fields aggregate plus a leading wildcard, and in the drive a broad match multiplies DB hydration and permission filtering downstream, not just index time.The four open decisions, settled
The issue marks these as needing a call before implementation. Settled by the issue owner on 2026-09-11; approving this PR is the record of that sign-off.
titleonlytitlein step with the file name.Premises checked against
main— four correctionsThese are the part most worth a second pair of eyes, because three of them shrink the work and the fourth adds a rule the issue's file list has no place for.
Nothing sorts by score today. Decision 4 asks to confirm score-desc holds in Title mode. There is no score sort:
DEFAULT_SORTismodDate:DESC(shared/constants.ts:51), the store sends it unconditionally (dot-content-drive.store.ts:157), and the server default isSORT_BY = "modDate"(AbstractDriveRequestForm.java:81). The only trace is a stale comment atdot-content-drive.store.ts:481sitting above code that does nothing of the kind. → Sorting out of scope; comment corrected in passing.Folders and links are already matched on name only, in both scopes. They never reach Elasticsearch — loaded from the DB and narrowed in Java by substring (
BrowserAPIImpl.java:3026for folders,:2908-2913for links). → The selector governs the contentlet clause alone; folder/link matching must not change.buildPureESQueryis unreachable for the drive under shipped configuration.doPureESQueryruns only whenBROWSE_API_HEURISTIC_TYPE=PURE_ES; the default isHYBRID_SINGLE_CHUNKED_QUERY_ES(BrowserAPIImpl.java:701-709). → Explicitly out of scope rather than changed unverified.A scope stored as a filter would offer "Clear all" on an unfiltered drive. The issue routes the scope through filter state — which is what carries it into the URL — but stops there. That state also feeds the chip bar, and
hasNonDefaultFilters(utils/functions.ts:334-355) counts every key butsharedAssetsandlanguageIdas a non-default filter, which is exactly what shows "Clear all" (dot-filter-bar.component.html:7). Picking All Content — the default — would light it up on a drive with nothing filtered. → FR-021: the scope counts as filter state only while it differs from the default, mirroring how the search term already deletes its own key when it goes empty.Scope boundaries
buildPureESQuery; the all-content strategy shared with the Search portlet and the Relationships dialog, which keeps serving them unmodified — Title is a sibling path, not a branch inside it; the Asset Picker, which reuses the same search box and must stay untouched unless it opts in; sorting; any DB, index-mapping or content-model change.UI
Deliberately thin. The issue's ASCII diagram is the only design input — no mock image exists — so the spec fixes which components are on screen (input, scope dropdown beside it, placeholder that follows the scope) and stops there. No dimensions, spacing or layout rules.
What a reviewer should push back on
fileName/metadata.namewill bite in practice — the spec names the gap (a file whose title was edited away from its name) and calls widening it a follow-up rather than a silent change.🤖 Generated with Claude Code