fix(content-drive): field rendering and filtering issues (#36795) - #37074
Draft
zJaaal wants to merge 13 commits into
Draft
fix(content-drive): field rendering and filtering issues (#36795)#37074zJaaal wants to merge 13 commits into
zJaaal wants to merge 13 commits into
Conversation
"No language selected" was never a neutral state: the backend omits the language term entirely, so every language version of a contentlet came back as its own row. The store now resolves the environment's default language (by the `defaultLanguage` flag — not id 1, not the first entry) and seeds it into the `languageId` filter whenever nothing is selected: cold load, a URL without a language, clear-all, or a Back/Forward restore. Clearing the filter re-selects that default rather than leaving it unset. Three consequences worth calling out: - A language selection no longer hides folders. Folders have no language, so a locale filter (which picks a *version* of content) must not tear down the structure being navigated — and with a default always selected, the old rule would have hidden every folder in the drive. - The popstate guard now seeds the restored filters before comparing. Otherwise the seed became a history trap: the write-back pushes the seeded URL, Back returns to the language-less one, re-hydration re-seeds, and the same entry is pushed again — the user could never Back out of the portlet. The comparison is also key-order independent now, since the seed appends `languageId` last. - The first search waits for the default to resolve, so the portlet no longer fires one search without a language (a flash of duplicated rows) and a second one with it. A failed languages request degrades to the previous behaviour instead of hanging in LOADING. The Locale chip reads the language list from the store instead of fetching it itself, so the seed and the chip share one request, and it hides its remove X while only the default is selected — there is nothing to remove when clearing re-selects the same value. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Content Drive "Add New" selector listed system Content Types (Host, Forms, Dot Favorite Page) — none of which a user should be creating from the drive. The backend already supported excluding them (#36072), and the palette store even carried a comment claiming they were excluded server-side, but the query param was never sent. The endpoint defaults to *including* system types for backward compatibility, so it has to be explicit. `system` is now an optional param on `getAllContentTypes` and is sent only when specified, so callers that never asked about system types keep their current behaviour. It is set to `false` in the page-agnostic branch alone, which is the branch the Content Drive pickers use — UVE's own palette tabs and the favorites panel are deliberately untouched, and a test pins that. Verified against a running instance: the request returns 37 content types by default and 34 with `system=false`, dropping exactly `dotFavoritePage`, `forms` and `Host`. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clearing the filter left the narrowed result set in place: `onPanelHide` blanked the visible text but kept `contentTypes`, `currentPage` and `canLoadMore`, and the chip's X cleared only the selection. Reopening the panel therefore still showed just the previously searched content type. The reopen lazy load could not recover on its own — after a search that fits in one page `canLoadMore` is false, so the guard in `onLazyLoad` drops the event and no request is made. When it happened to be true it was worse: page 2 of the *unfiltered* list got appended to the narrowed array, giving a mixed list with no page 1. `onFocusChange` already had the correct recipe, so it is now extracted as `#resetContentTypeSearch` and shared. Blanking without refetching is not an option: PrimeNG does not render its virtual scroller for an empty list, so no lazy load would ever fire to repopulate it — the reset and the page-1 fetch have to ship together. - `onPanelHide` resets only when a term was actually active, so opening and closing the panel without searching still costs no request. - `onClearAll` (the X) resets unconditionally and returns focus to "all content": the X reads as "clear this filter entirely", so no part of it should be left applied. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`openEditByIdentifier` looked up `+identifier:X +working:true` with a limit of 1 and no language term, then took the first hit. An identifier exists once per language, each version with its own inode, so the version returned was whichever the index happened to rank first — a shared `?editContent=` link could open a language the user was not looking at. The lookup is now pinned to the drive's active Locale filter, so the resolved version matches the row the link was shared from. It falls back to the environment default (only reachable before the store has seeded the filter) and, when no language is known at all, to no term — which is the previous behaviour, so an unresolved language can never make the link stop working. This is the Content Drive half of a wider pre-existing problem with opening content in a non-default language. The rest sits in UVE and is not touched here: `editEmaGuard` defaults `language_id` to the literal `'1'` rather than the environment default, and the UVE shell renders an empty body for any page-load failure other than 404/403/401, so a failure looks like "nothing opened". Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Radio or Select field with Data Type True/False types its column `boolean`, and the per-type width for that type was an override rather than a floor: the column stayed pinned at 7rem however long its header was. Because "Show in List" forces `indexed`, such a column is effectively always sortable, so its header renders `whitespace-nowrap` with a sort icon under `table-layout: fixed` and no overflow clipping — and a longer label spilled into the neighbouring header. The width now resolves to whichever of the fixed and header-derived widths is wider, so a short label keeps the compact 7rem and a long one gets the room it needs. It stays a single length rather than a CSS `max()` because these widths are also summed into the table's `min-width: calc(100% + …)`. Fixed-type columns are still never measured from their *content*: they render formatted text or an icon, so the raw value length is irrelevant. Also in that cell: the icon gets `align-middle` (a 20px inline-block in a 14px text line otherwise sits off the baseline every sibling column shares) and its test id becomes per-field, since a static one produced duplicates whenever a type had two boolean columns. Test coverage only ever exercised `Checkbox` for the boolean column type, which is why this shipped; Radio and Select cases are now covered, plus a long-header width case. Also corrects the deep-link language term added in 5851c3c to use an explicit `OR`. A whitespace group (`languageId:(1 2)`) is not an implicit OR here — verified against a running instance, it fails the whole query and returns `resultsSize: -1`, i.e. no rows and no error. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A True/False Radio or Select authored as `True|1` / `False|0` showed BOTH options selected and only ever sent `false`, whichever one was clicked. The cause is a single expression: `castSingleSelectableValue` matched only the literal `'true'`, so `'1'` and `'0'` both cast to `false` and the two options ended up sharing one value. A listbox marks every option matching the model, and the emitted value is that shared one — every reported symptom follows from there. dotCMS actively invites this authoring style: the Radio field's own help text gives `True|1 False|0` as the example, `SelectableValuesField.check()` accepts `1`/`0`, `y`/`n`, `t`/`f` and `on`/`off`, and the product ships `Host.runDashboard` as `Yes|1 / No|0`. The cast now mirrors the token set the backend coerces through commons-lang `BooleanUtils.toBoolean` on save, so the whole family is handled rather than just the reported pair. Verified against a running instance: saving such a field with `"1"` stores a real boolean `true`, and `"0"` stores `false` — so `true`/`false` is what a filter has to send, and casting `1` to false was simply wrong. Only the mapping of these tokens changes; anything that cast to `true` before still does. The full edit-content suite (112 suites, 2233 tests) passes untouched — this also fixes the same latent defect in the content editor's own rendering of such fields. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A Checkbox field with one option rendered its chip titled with that
option's own label. That reads well only when the label happens to be a
phrase ("Include in Site Map"); for a field `active` whose single option is
`yes`, the chip was titled "yes" — the option's value, saying nothing about
which field was being filtered. The chip now always shows the field's name.
The option text moves to where it belongs, the options themselves, and both
labels are derived from the field rather than hardcoded: the positive one is
the option's own label (or its value when the option carries none, as the
classic `|true` checkbox does), and the negative one wraps that same text in
a new `content-drive.field-filter.binary.exclude` message. The words
"true"/"false" described a user-authored `yes` or `accept` no better than
any other value.
So `active|yes` now offers `yes` / `Exclude "yes"` under an "Active" chip,
and `|true` stays generic with `true` / `Exclude "true"`.
The values sent are unchanged — still `true`/`false`, meaning "has this
value" / "does not" — so nothing about the request shape moves here.
Refs #36795
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two field filters could never return a row, for two different reasons.
**A True/False Radio or Select could not be queried at all.** Such a field
is mapped in Elasticsearch as a `boolean`, but `FieldHandlerRegistry` keys
its handlers on the field *type* alone, so Radio/Select went to the TEXT
handler and its contains-style `+(f:*v* f_dotraw:*v*)`. A wildcard against
a boolean-mapped field is rejected outright, and since these queries are not
lenient the rejection fails the WHOLE query — which surfaces as an empty
result set with no error, so the filter looks like it simply found nothing.
Verified against a running instance: `f:*true*` returns `resultsSize: -1`
(a failed query) while the exact terms `f:true`, `f:false`, `f:1` and `f:0`
all return rows — the literal does not matter for a boolean-mapped field,
only the absence of the wildcard. So handlers are now selected by DATA TYPE
first: a BOOL field routes to a new `BooleanFieldStrategy` emitting an exact
term. Checkbox fields are deliberately unaffected — they only accept TEXT
and LONG_TEXT, so one is never boolean-mapped even when its option value
happens to be the word "true".
**A single-option checkbox could not be filtered either way.** It stores its
OPTION VALUE when ticked ("yes", "accept", "true") and nothing at all when
unticked, so asking for the literal "true" finds nothing unless the option
value happens to be that word, and "false" can never match — an unticked box
leaves no token to match against. Both halves now resolve from the field's
own option value: ticked contains it, unticked is the negation of the same
clause, which is the only way to express an absence.
Verified on the demo data: `Blog.sitemap` filtered false returned 0 rows,
while `-(Blog.sitemap:*true* …)` returns the 1 blog that is not ticked.
Integration coverage adds both shapes — a `Yes|1 / No|0` BOOL Radio and an
`active` checkbox whose only option is `yes` — to a suite already registered
in MainSuite3a. Not run locally: the harness would need its own Postgres and
Elasticsearch on ports the dev instance is using.
Refs #36795
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
19 tasks
github-actions
Bot
added
Area : Backend
Contributor
|
Claude finished @zJaaal's task in 4m 19s —— View job Review
I read the backend strategy/handler changes, New Issues
Notes (non-blocking, no change requested)
Everything else — the · |
zJaaal
added
the
PR: docker image
Constraining the lookup with `+languageId:` was wrong: a query pinned to a language the content has no version in returns nothing, and the caller treats "nothing" as "do not open". So on an environment whose default is not English, a shared `?editContent=` link to English-only content stopped opening anything at all — worse than the arbitrary-language problem it was meant to fix. Verified against a running instance: for an English-only Blog, `+identifier:<id> +working:true` returns its one version, while the same query plus `+languageId:2` returns zero rows. The query goes back to being language-agnostic and the preference is applied to the results: the version matching the drive's active Locale filter wins, then the environment default, then whatever exists. That keeps the link working in every case, costs no extra request, and drops the Lucene grouping this had to get right. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Guessing which language version a `?editContent=` link should reopen was weak in exactly the case it targeted. `openEditByIdentifier` runs from the shell's constructor, while the store's languages request is still in flight — so the Locale filter is usually not seeded yet and `defaultLanguageId` is often still undefined, leaving the preference to fall through to "whatever the index returned first". The data was always available at the point the panel opens: a row click knows its own `languageId`. It is now recorded on the panel request and written to the URL as `editContentLang`, so the link names the exact version instead of the content alone, and the resolver prefers it above everything else. The previous ordering (active filter → environment default → first available) remains for links written before the language was recorded. Kept as a separate param rather than folded into `editContent`, so the popstate guard's "is this the same content" comparison stays untouched. It is cleared whenever the panel closes or a `new` panel is open. Also addresses PR review feedback on `BooleanFieldStrategy`: `Boolean.parseBoolean` maps everything that is not the literal "true" to false, so a caller filtering a True/False field by the db-style value its own options are authored with (`1`, `yes`, `on`) would have silently got the OPPOSITE result set. The Content Drive normalizes before that point, but the generic content-search endpoint routes through the same strategy with raw values. It now accepts the same token set the frontend cast does, which is what dotCMS coerces on save. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ub.com/dotCMS/core into issue-36795-content-drive-filter-fixes
…uses The token list added in the previous commit duplicated knowledge that already has an owner: `FieldHandlerStrategyFactory.booleanStrategy` coerces these values on save with commons-lang `BooleanUtils.toBoolean`. The strategy now calls that directly, so a filter can never drift from what the contentlet actually stored, and there is one definition instead of two. Refs #36795 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
🐳 PR Docker test imageLatest build for commit docker pull dotcms/dotcms-test:pr-37074-issue-36795-content-drive-filter-fixes
docker pull dotcms/dotcms-test:pr-37074-issue-36795-content-drive-filter-fixes_05845bc |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the Content Drive field-rendering and filtering defects in #36795 — the four in the original write-up plus two more found while working on it, and one drive-side language fix.
Each bug is its own commit, so they can be read (or dropped) independently.
e399c1e0d44904bcfalse88271fce(frontend) +759c9aee(backend)8950605759c9aee66ebe0dd477256bc?editContent=deep link resolved an arbitrary language5851c3c5,f5b9f4ac,90792e0cThe two that turned out to be bigger than the write-up
Bug 3 was not just a frontend cast. A True/False field is mapped in Elasticsearch as a
boolean, but Radio/Select filters were routed to the text handler and its contains-style+(f:*v* f_dotraw:*v*). A wildcard against a boolean-mapped field is rejected, and because these queries are not lenient the rejection fails the whole query — which surfaces as an empty result set with no error, so the filter looks like it simply found nothing. Handlers are now selected by data type first, and a BOOL field gets an exact term.Worth noting this is the authoring style dotCMS itself recommends: the Radio field's help text gives
True|1 False|0as the example,SelectableValuesField.check()accepts1/0,y/n,on/off, and the product shipsHost.runDashboardasYes|1 / No|0. So the filter has never worked for the form users are told to use.Bug 4b affects every single-option checkbox, not just non-boolean ones. A checkbox stores its option value when ticked and nothing at all when unticked, so no match query can find the unticked ones —
Blog.sitemapis broken onfalsetoo, not only a field likeactive|yes. "Unticked" has to be expressed as a negation, which the filter pipeline could not produce.Verified against a running instance
Measurements that drove the fixes, all on demo data:
f:*true*on a BOOL field →resultsSize: -1(a failed query);f:true,f:false,f:1,f:0all return rows. The literal is irrelevant for a boolean-mapped field — only the wildcard breaks it.boolRadio=true→ 0 rows.Blog.sitemap: 5 blogs indexsitemap='true', 1 has no value indexed. Filteringfalse→ 0 rows today;-(Blog.sitemap:*true* …)→ the 1 unticked blog.system=falseon the content-type endpoint → 37 → 34 types, dropping exactlydotFavoritePage,forms,Host."1"stores a real booleantrue— sotrue/falseis what a filter must send.The deep link now names the version, not just the content
An identifier has one inode per language, so
?editContent=<identifier>alone does not say which version to reopen. The first attempt constrained the lookup with+languageId:, which was worse than the problem: a query pinned to a language the content has no version in returns nothing, and the caller treats "nothing" as "do not open" — so on a Spanish-default environment a link to English-only content stopped opening anything (verified:+identifier:<id> +working:true→ 1 row, the same query+languageId:2→ 0).The language is now recorded when the panel opens (a row click already knows it) and written to the URL as
editContentLang, so the link reopens the exact version. The lookup is language-agnostic again and the preference is applied to the results, which also matters becauseopenEditByIdentifierruns from the shell's constructor while the store's languages request is still in flight — the Locale filter is usually not seeded at that moment, so guessing from it was mostly inert.Behaviour changes worth a reviewer's attention
filters=URL param. Clearing the Locale filter re-selects the default rather than leaving it unset, and the chip hides its remove X while only the default is selected (nothing to remove).active|yesnow readsyes/Exclude "yes"under an "Active" chip. One new i18n key,content-drive.field-filter.binary.exclude.editContentLangis a new query param, written besideeditContentand cleared with it. Kept separate rather than folded intoeditContentso the popstate guard's "is this the same content" comparison stays untouched.libs/edit-content, so it also fixes the same latent defect in the content editor's own rendering of such fields. Only the mapping of db-style tokens changes; anything that cast totruebefore still does, and the full edit-content suite (112 suites, 2233 tests) passes untouched.Tests
Frontend, all green:
Backend:
dotcms-coreanddotcms-integrationcompile.ContentDriveFieldFilterTestgains aYes|1 / No|0BOOL Radio case and anactive|yessingle-option checkbox case (both directions). Not run locally — the harness needs its own Postgres and Elasticsearch on ports the dev instance was using, so it is left to CI.Two things for other teams, not fixed here
editEmaGuarddefaultslanguage_idto the literal'1'rather than the environment default, and the UVE shell renders an empty body for any page-load failure other than 404/403/401 — so a failure reads as "nothing opened". Separately, file-based containers/templates are resolved in the default language (ContainerFactoryImpl,TemplateFactoryImplhardcodegetDefaultLanguage()), so switching the default breaks them when theircontainer.vtlFileAsset exists only in the old one."1"persiststruein the database but indexesfalse: the save path coerces via commons-langBooleanUtils.toBoolean(which accepts1) while the indexing path does not. Reproduced on demo data; content saved through the editor is unaffected.How to verify
Frontend:
pnpm nx run-many -t test -p portlets-content-drive content-drive-ui edit-content data-access edit-ema-uiBackend:
./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=ContentDriveFieldFilterTestManually, on an environment with more than one language: the Locale chip should arrive pre-selected with the default language (not id 1, not the first one listed), folders should still be listed, and Back should still leave the portlet. Then a True/False Radio field marked User Searchable should filter, and a single-option checkbox should return the ticked rows for its positive option and the un-ticked ones for
Exclude ….🤖 Generated with Claude Code
Review feedback addressed
BooleanFieldStrategyoriginally usedBoolean.parseBoolean, which maps anything that is not the literal"true"tofalse— so a caller filtering a True/False field by the db-style value its own options are authored with (1,yes,on) would silently have got the opposite result set. The Content Drive normalizes before that point, but the generic content-search endpoint routes through the same strategy with raw values. It now coerces with commons-langBooleanUtils.toBoolean— the very functionFieldHandlerStrategyFactory.booleanStrategyuses to store these values — so the filter cannot drift from what the contentlet holds, and there is one definition rather than a duplicated token list.This PR fixes: #36795