feat(content-drive): Status filter (Archived, Unpublished, Locked), three regression fixes, and a Refresh role gate (#37066)

[zJaaal]

Implementation PR, stacked on #37170 (the approved spec) — stack #37217. Review #37170 first; this diff is the feature only.

Resolves #37066.

Ready for a Docker image. Backend + frontend are complete and the unit/Jest suites are green. The integration tests have not been run — see Verification below. Manual testing against a build is genuinely useful here.

What it does

An optional status array on POST /api/v1/drive/search, plus a multiselect chip in the Content Drive toolbar. Selected statuses combine with OR — checking more boxes returns more content, matching the content-type and locale filters beside it.

[shared assets] [content type] [workflow] [status] [locale]

The five things most worth reviewing

1. One OR-ed group, with the archived baseline OUTSIDE it.

  and cvi.deleted = false                                        -- baseline, unless ARCHIVED selected
  and ( cvi.live_inode is null or cvi.locked_by is not null )    -- the selected statuses

Folding the baseline into the group would make [UNPUBLISHED, LOCKED] read (deleted = false or …) and match nearly every row — a filter that silently stops filtering.

2. Lucene needs an explicit group. + means REQUIRED, so +deleted:true +live:false is an AND. buildPureESQuery emits +(deleted:true OR live:false). Caught by @nollymar in review on #37170; it follows the convention already in that same method (+(conhost:… OR conhost:SYSTEM_HOST)).

3. An empty selection emits nothing at all. Not an empty group — and ( ) is a SQL syntax error and +() is invalid Lucene. This is the default path: every drive search that exists today sends no status, so an unfiltered request stays byte-identical.

4. showWorking now covers ARCHIVED/UNPUBLISHED. Neither state has a live version, so without this the query joins live_inode and returns nothing — silently, with no error. LOCKED doesn't need it; a locked item may well be live.

5. ARCHIVED suppresses the archive-step reconciliation the same way showArchived does, so it can't contradict appendWorkflowQuery's per-branch cvi.deleted.

A real bug the tests caught

parseStatuses originally built its 400 message with String.format and handed the result to BadRequestException. But HttpStatusCodeException runs String.format over the message again — so status: ["50%"] raised UnknownFormatConversionException and surfaced as a 500 instead of a 400. User input was reaching a format string. The value now travels as a format argument, with a regression test.

The same test also showed BadRequestException.getMessage() returns "HTTP 400 Bad Request"; the useful text is in the error-message header, which is what a client actually sees.

Regressions found while testing, fixed here

Three Content Drive issues surfaced during manual testing of this filter. Carried in rather than deferred, at @zJaaal's call, so the portlet ships whole.

1. A loading dialog could not be closed. The folder dialog's footer sat inside @if ($formReady()), so while the form loaded there was no Cancel button and no way out but the backdrop. The footer is now outside the guard and Cancel always renders; only the submit button waits on the form.

2. The toolbar lost its bottom border. Traced to border-none! on the toolbar, added to strip p-toolbar's default border on the other three sides. Narrowed to border-x-0! border-t-0!, which keeps the bottom edge that separates the toolbar from the table. Scoped to Content Drive only.

3. The context menu was one undifferentiated list. It now reads as three named groups:

ACTIONS                 <- caption
Edit Content
Lock
Push Publish
Add to Bundle
────────────────
WORKFLOWS               <- caption
Assign Workflow
Save
Save / Publish
────────────────
Archive                 <- separator alone, no caption

A folder gets the same treatment: an Actions caption, and Delete held apart below a separator. There the caption is unshifted after the fact rather than pushed first — every entry on a folder is permission-gated, so that is the only point where the group is known to be non-empty.

On the caption itself: p-contextMenu has no group-label class at all — its style map is item, separator, submenu, submenuIcon, and it renders items as a flyout. PrimeNG's inline group label (p-menu-submenu-label) lives on p-menu. So the caption is a regular item wearing that class, made inert by disabled (PrimeNG's isValidItem skips disabled items, keeping it out of keyboard navigation) plus pointer-events-none (it would otherwise take a click and close the menu). Three utilities cancel what the surrounding styles impose: p-0! so it aligns on the item padding instead of being inset twice, pointer-events-none, and text-inherit on the content wrapper, since .p-contextmenu-item-content sets color on a descendant and would win.

Switching wholesale to p-menu for its native grouped model was considered and rejected: p-menu popup calls absolutePosition(container, target) and anchors to an element, while p-contextMenu positions at event.pageX/pageY. A right-click menu opening at the row instead of the cursor is a worse regression than a faked caption row.

The destructive split reads hasArchiveActionlet || hasDeleteActionlet || hasDestroyActionlet — the action's actual sub-actionlets, never its name, so a scheme's "Retire this blog" or "Purge" lands there too. A parameterized test names the actions nothing like Archive or Delete precisely to hold that line. A folder's Delete gets the same separator; its gate is strictly narrower than Edit Folder's, so it can never lead the menu.

Finer grouping than this is not possible from the data: the API exposes no actionlet class names, no category and no tag, order is a within-scheme sort index, and icon is admin-authored free text. Anything more would be guesswork that breaks on custom schemes.

Frontend notes

• The whole row is clickable, label included. First cut bound [ngModel] on the listbox without (ngModelChange), so only the checkbox responded. Regression test clicks the label specifically.
• decodeByFilterKey needs the explicit status entry. Without it a lone status:ARCHIVED decodes to the string 'ARCHIVED' — whose .length is 8, so every ?.status?.length guard misreads it and the filter looks fine until someone selects exactly one status.
• Placement: after Workflow, before Locale. Content type and workflow stay adjacent because the workflow filter derives its scheme list from the content-type selection — the row's only real dependency.
• archived: false pin removed from the store request (FR-019); the endpoint already defaults it, and pinning it would contradict an Archived selection.

Screen.Recording.2026-08-26.at.10.24.34.AM.mov

Two questions reviewers keep asking

Answering both here rather than in the source — the code comments already carry their weight.

Why a new ContentStatus enum, when WorkflowState already has these names?

com.dotmarketing.portlets.workflows.model.WorkflowState is NEW, LOCKED, UNLOCKED, PUBLISHED, UNPUBLISHED, ARCHIVED, LISTING, EDITING — all three of ours, same spelling. It looks like a duplicate and isn't:

• It is a different concept. Its own javadoc: the show_on set deciding whether a workflow action renders. That is why it also carries LISTING and EDITING — view contexts, not states a contentlet can be in, with nothing to resolve against in a query. Reusing it would put status: ["EDITING"] in the public API contract.
• Its parsing contradicts ours. WorkflowState.toSet catches any exception and returns an empty set, so one bad value silently drops the whole filter and returns a wider result than the caller asked for. This filter deliberately returns a 400 instead — the behaviour the tests and this PR's review cycle settled on.
• It would couple the API surface to workflow internals. Adding a show_on value would silently widen /v1/drive/search's accepted input. We implement three states; that enum has eight and grows for unrelated reasons.

Same words, different job.

Why does decodeFilters drop every empty array, not just status?

Raised by the automated review, and fair: the behaviour is general while the comment reads status-specific. The general rule is intended.

Every consumer reads these through ?.length, so undefined and [] already mean the same thing — there is no behavioural difference to preserve. Storing the key would re-encode a bare status: into the URL for the next decode to trip over. status is simply what surfaced it: sanitizing status:BOGUS is the first decode that can legitimately produce an empty array.

Verification

Suite	Result
ContentDriveHelperStatusTest (unit)	✅ 8/8
Frontend (portlets-content-drive, full)	✅ 1304/1304
Lint	✅ clean
tsc --noEmit	✅ no new errors (35 pre-existing, none in touched files)
ContentDriveStatusFilterTest (integration, 16 cases)	✅ 16/16 in CI
ContentDriveWorkflowArchiveStepTest (regression guard)	✅ green after b09fb47 — see below

The integration class covers each status alone, every pair, all three, the empty default, the never-shrinks property and the archive-step regression. Registered in MainSuite3a.

MainSuite 3a has now run, and it caught one thing — a bad assertion of mine, not a code defect. 1 failure out of 803, deterministic across all three retries: testUnpublishedStatusWithArchiveStepExcludesArchivedContent claimed UNPUBLISHED + an archive-target step must exclude archived content. False — an archive-target step makes appendWorkflowQuery admit cvi.deleted = true rows in that branch by itself, with no status involved (testMixedFilterScopesArchivedToArchiveBranch pins exactly that and passes), and ContentletAPI.archive unpublishes so the row satisfies live_inode is null too. Both clauses match; returning it is correct.

Replaced in b09fb47 by two tests rather than one: …StillReturnsArchivedContent asserts what actually holds, and …KeepsTheGlobalArchivedBaseline is the real complement, using a normal step where nothing lifts the baseline. Strictly stronger than what it replaced — it still guards the baseline and now documents the interaction that misled me.

The fix itself was not verified locally: com.dotcms.tika-api's pom in the local repo is unflattened, so :dotcms-integration fails dependency resolution before any test runs. CI is the verification.

PURE_ES parity holds for ARCHIVED and LOCKED, and deliberately not for UNPUBLISHED — see below.

Two things I won't paper over

The TDD gates for US2–US5 are not satisfied. I implemented all three SQL disjuncts in one switch rather than story-by-story, because splitting them leaves states where selecting LOCKED parses fine and filters on nothing. Their tests were therefore written against working code — characterization, not true Red. US1's gates were honored properly (Red confirmed, then implementation).

UNPUBLISHED means something slightly different under PURE_ES, and we accepted that. It means no live version exists — a question about the content, not about one version. The index stores live per version, so a published item with newer unpublished edits has a working document carrying live:false, which the index query matches and cvi.live_inode is null does not. There is no index-side fix short of redefining the status per-version, which would degrade the default path to match a limitation of a strategy nobody runs (PURE_ES is opt-in and is not set in any config file in this repo). ADR-0018 already routes structural predicates to the database for exactly this reason and states PURE_ES forfeits that guarantee. Recorded as FR-009a in the spec, with FR-009 and SC-005 narrowed to match, and a predicate-comparison table in contracts/. ARCHIVED and LOCKED are unaffected.

openapi.yaml is unchanged, deliberately. /v1/drive has zero entries in the generated spec, unlike /v1/folder (8), /v1/browser (4) and /v1/workflow (45) — ContentDriveResource is excluded from OpenAPI generation entirely. The @Operation text was updated and is good source documentation, but it does not reach the yaml. Pre-existing gap; flagging rather than working around it, and worth its own ticket.

Unrelated polish: Refresh is now role-gated in the UI (#36845)

Not part of the status-filter spec. Carried here rather than held back, so it is in the demo build.

QA on POST /api/v1/content/_bulkrefresh turned up that a non-admin sees Refresh in Quick Actions,
fires it, and gets a 403. That was a deliberate call at the time: BulkRefreshHelper.canRefresh
reads as Power User OR Administrator, and the browser cannot tell whether someone is a Power User,
so gating on isAdmin looked like it would hide the action from people entitled to it.

It does not, because the Power User half never fires. It resolves the role key "CMS Power User",
no role ships with that key, loadRoleByKey answers null, and RoleFactoryImpl returns false for a
null role. What survives is doesUserHaveRole(user, loadCMSAdminRole()), which is character-for-character
User.isAdmin(), which is what already feeds currentUserIsAdmin in the store. Client and server
evaluate the same expression, so the row can predict the refusal rather than discover it.

Disabled, not hidden — the same treatment Push Publish gets with no environment configured. The row
keeps saying the capability exists; the tooltip says who it is for.

	Before	After
Non-admin	Row enabled, fires, 403	Row disabled, tooltip names the requirement
Admin	Row enabled, fires	Unchanged

Follows the existing gate shape exactly: requiresAdmin on the action def, missingAdminRole computed
in getQuickActions, added to the [disabled] binding, the quickActionHint tooltip and the
onSelectQuickAction guard. Three new getQuickActions tests plus three at component level; five
existing Refresh tests now set the admin precondition, since the behaviour they cover starts after the
gate.

Known limit, left alone: an install that hand-created a role keyed "CMS Power User" would let a
non-admin holder through server-side while this row stays shut. dotCMS ships no such role. The clean fix
is to drop the dead branch from canRefresh so the gate is admin-only by definition, but that is a
change to a server-side authorization path and does not belong in a demo-eve commit. Worth its own ticket.

Also touched

Renamed the status:published placeholder in existing decodeFilters tests to owner:jane. They used status as a made-up key to exercise unknown-key fallback, and it is now a real key. Changing their expectations to arrays instead would have forced them green while destroying what they test.

Checklist

• Tests — unit + integration + Jest/Spectator. Integration not yet executed locally (see Verification)
• Translations — four content-drive.status-filter.* keys in Language.properties, plus content-drive.action-center.requires-admin for the Refresh gate
• Security Implications Contemplated — input is a closed enum, validated and rejected with a 400, never interpolated into SQL. The format-string bug above was found and fixed. Permission filtering untouched, so the filter grants no new visibility

🤖 Generated with Claude Code

[claude]

Claude finished @zJaaal's task in 2m 16s —— View job

---

Code Review

Reviewed the diff against origin/issue-37066-content-drive-status-filter. This is the feature-only diff (spec PR #37170 excluded). I focused on bugs, security, and dotCMS conventions in the changed code, and rechecked every prior review finding.

• Gather diff vs base branch
• Review backend (Java) changes
• Review frontend (Angular) changes
• Recheck prior review findings
• Post review

New Issues

No issues found.

The backend query construction is sound:

• appendContentStatusQuery / appendContentStatusESQuery (BrowserAPIImpl.java) both no-op on an empty set, keeping the default path byte-identical, and both put the disjuncts in one OR group (and (…) / +(… OR …)) rather than AND-ing required terms.
• The archived-baseline suppression correctly folds ARCHIVED into the admitsArchived condition alongside showArchived and the archive-step guard.
• parseStatuses values come from a closed enum via ContentStatus.valueOf(...) — never interpolated into SQL — and the 400 message passes user input as a String... format argument, so a %-bearing value can't re-trigger String.format in HttpStatusCodeException. BadRequestException(String, String...) matches the call. Locale.ROOT on toUpperCase is correct.

Frontend:

• decodeByFilterKey.status sanitizes to real ContentStatus values and decodeFilters drops keys that decode to [], so a hand-edited ?filters=status:BOGUS degrades to no-filter rather than reaching the 400-swallowing catchError — consistent with the store comment.
• The linkedSignal value-equality + signalMethod sync in dot-content-drive-status-filter.component.ts breaks the write-back cycle at the source rather than relying on incidental reference identity.
• The folder-dialog footer now sits outside the @formReady gate with Cancel always rendered and only the submit button gated — the loading-with-no-exit bug is fixed.

Resolved

• ✅ dot-content-drive-status-filter.component.ts — reactive signalMethod sync with value-comparing linkedSignal (nicobytes, feat(content-drive): Status filter (Archived, Unpublished, Locked), three regression fixes, and a Refresh role gate (#37066) #37216).
• ✅ AbstractDriveRequestForm.java:363 — javadoc no longer claims status "drops folders"; now states "no side effects on other fields", matching the code (oidacra).
• ✅ ContentDriveResource.java — @Operation description updated to "showFolders is honoured as sent" (oidacra).
• ✅ BrowserAPIImpl.java build-log comment — status removed from the list of flags that override showFolders (oidacra).
• ✅ BrowserQuery.java:163 — mixed live:true + ARCHIVED/UNPUBLISHED version-join interaction now documented on showWorking and cross-referenced from live() (oidacra).
• ✅ ContentStatus.java — baseline phrasing corrected to "unless something explicitly lifts it"; UNPUBLISHED now spells out identifier-scoped SQL vs per-version live:false (oidacra).
• ✅ ContentDriveStatusFilterTest — keyword test now asserts against liveItemMatchingKeyword ("archived-decoy"), making the intersection observable (oidacra).
• ✅ ContentDriveWorkflowArchiveStepTest — admitsArchived interaction now pinned by testArchivedStatusWithArchiveStepStillReturnsArchivedContent and its complement (oidacra).
• ✅ dot-content-drive.store.ts:163 — ?filters=status:BOGUS closed at the decoder; the broader catchError swallowing (401/403/5xx into a stale grid) is correctly deferred to a follow-up ticket, not a regression this PR introduces.

Notes (non-blocking, already acknowledged in the PR)

• The PURE_ES UNPUBLISHED divergence is a spec decision (FR-009a, ADR-0018), not a code defect — ARCHIVED/LOCKED retain parity.
• After grouping, the if (!actionsMenu.length) guard in dot-folder-list-context-menu.component.ts is now unreachable on the contentlet branch (Edit Content + group label are unconditional). Harmless dead check, not worth a change.

The implementation is careful and well-tested. My one caveat is the same one the PR is transparent about: the integration suite's real verification is CI (MainSuite3a), since local :dotcms-integration resolution is blocked by the unflattened tika-api pom — worth confirming the latest CI run is green before merge.
• issue-37066-content-drive-status-filter-impl

[github-actions]

🐳 PR Docker test image

Latest build for commit aad8e69 pushed to dotcms/dotcms-test:

docker pull dotcms/dotcms-test:pr-37216-issue-37066-content-drive-status-filter-impl
docker pull dotcms/dotcms-test:pr-37216-issue-37066-content-drive-status-filter-impl_aad8e69

[zJaaal]

Both 🟡 findings are real. Fixed in eabb797 — and the second one surfaced something worse than the review could have known.

🟡 Locale — fixed. Exactly right. "unpublished".toUpperCase() under tr-TR yields UNPUBLİSHED and 400s a valid value on Turkish-locale servers only. Now toUpperCase(Locale.ROOT), with a regression test that flips the default locale rather than trusting the reasoning.

🟡 SQL vs PURE_ES UNPUBLISHED — confirmed, your model of the index is correct. ESMappingAPIImpl:523 sets live from contentlet.isLive(), i.e. per VERSION. So a published item with pending working edits has a working document carrying live:false, which +working:true +(live:false) matches, while SQL's cvi.live_inode is null excludes it. Two different questions: "the identifier has no live version" versus "this version is not the live one".

The verification you suggested could not have run. There is no PURE_ES parity case in ContentDriveStatusFilterTest. It was listed as covered in tasks.md T031, in this PR's body and in quickstart.md — and was never written. That is my error, and the more serious of the two: I have been reporting coverage that does not exist. All three claims are corrected in this commit, with T031 un-ticked and marked ⚠️ NOT DONE.

The fixture was also blind to it — no published-then-edited item, so a parity test would have compared two identical answers and passed for the wrong reason. That fixture now exists, plus a test pinning the intended semantics on the default path: a published-then-edited item HAS a live version, so UNPUBLISHED must exclude it.

The divergence itself is left open deliberately. "The identifier has no live version" is not expressible in a per-document index query — -live:true does not help, because the working doc genuinely carries live:false. So the options are a spec decision, not an implementation one:

1. Accept it and narrow FR-009/SC-005, leaning on ADR-0018 already stating PURE_ES forfeits consistency guarantees for every criterion
2. Redefine UNPUBLISHED as "this version is not live" — matches legacy ContentletAjax and ES, but changes what users get on the default path
3. Reject or degrade UNPUBLISHED under PURE_ES

Recorded in tasks.md rather than guessed at. @zJaaal's call.

On your non-blocking note about LOCKED + live:true: agreed, and it is documented in data-model.md as deliberate — LOCKED does not force working-version scoping because a locked item may well be live, so live:true + LOCKED is the coherent "live content that is locked" query.

_{Analysis and reply by Claude (Claude Code), posted from @zJaaal's account.}

Already solved them