Content Drive: Add Lock and Unlock quick actions - #36921
Conversation
Tests only — no implementation yet. Every assertion here is expected to fail; they are the Red step for issue #36844. Route decided: expose Lock/Unlock as SystemAction values so the existing multi-contentlet endpoint POST /api/v1/workflow/actions/default/fire/ {systemAction} can fire them. No new endpoint and no workflow action are needed — SystemActionApiFireCommandFactory already supplies a direct-API fallback for system actions with no workflow mapping, which is how PUBLISH works on a content type with no scheme. Frontend (18 failing assertions): - Lock counts unlocked rows, Unlock counts locked rows, both excluded from folders and non-selectable at a zero count. - New warningCount/warningHint on a quick action, driven by a warnWhen predicate. Unlock uses it to flag locks held by other users, detected via the row's contentEditable flag (the server's "is this locked by me?"). - Every locked row is still fired: only a CMS Administrator may release someone else's lock, and the client cannot know whether the caller is one. Failures are reported, not pre-filtered. - The result toast reports summary.successCount/failCount from the response instead of the number of inodes sent. This corrects every quick action, not just these two. Backend (8 integration tests, currently failing to compile on the two missing enum constants — the only errors in the module): - Single and bulk lock/unlock happy paths. - Denied path: a lock held by another user is reported per item, the batch is not refused, and the lock survives. - Mixed ownership yields a partial result rather than all-or-nothing. - Permission is enforced server-side, independent of what the UI offered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turns the failing tests from the previous commit green. Lock and Unlock are
exposed as SystemAction values so the existing multi-contentlet endpoint
fires them in one request — no new endpoint, no workflow action, no DB
migration.
Backend:
- SystemAction gains LOCK and UNLOCK, documented as the two members that
deliberately have no actionlet and no mappable workflow action.
- SystemActionApiFireCommandFactory gains two commands calling
ContentletAPI.lock/unlock. Registered in commandMap only, not in
systemActionHasActionletHandlerMap, so they always win instead of
deferring to a workflow action. Both ignore needSave: a lock is per-user
state on the version info, not a step transition.
- checkContentletState rejects LOCK/UNLOCK on a new contentlet, so the
caller gets a clear bad request instead of a blank-inode failure deeper
in lock().
- Swagger allowableValues on the three fire endpoints, with openapi.yaml
regenerated. The PATCH merge and scheme-mapping lookup endpoints are
deliberately left out: a merge would silently discard body fields, and
mapping these two to a workflow action has no effect.
Frontend:
- Lock and Unlock lead the Quick Actions list. Lock counts unlocked rows,
Unlock counts locked rows; both exclude archived content, which is a dead
end until unarchived and where a stray lock would make the item
undeletable by anyone but the lock holder.
- Generic warnWhen predicate produces warningCount/warningHint. Unlock uses
it to flag locks held by other users via the row's contentEditable flag,
which the drive search already returns but the model never declared.
Those items are still fired: only a CMS Administrator can release
someone else's lock and the client cannot know whether the caller is one.
- fireDefaultAction's return type corrected to DotFireDefaultActionResult.
It claimed Observable<DotCMSContentlet[]> while the endpoint actually
sends { results, summary }.
- The result toast now reports summary.successCount/failCount and drops to
warn severity on any failure, instead of reporting the number of inodes
sent as successes. This corrects every quick action, not just these two.
Tests: 9 integration tests pass, covering single and bulk paths, the
denied path, mixed ownership, and server-side permission enforcement. The
denied-path test asserts the failure message mentions locking — without
that guard it passed on a content-type permission rejection and never
reached canLock. Frontend: 979 content-drive and 752 data-access specs pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quick actions fired straight from the row click, so the preview table was reachable only from a workflow action. Both sections now commit the same way: clicking an action opens the preview, and nothing is sent until Execute. The old reasoning for the split was that a quick action's count is derived from the rows themselves, so "which items is this about to touch?" already had an obvious answer. That confused knowing the answer with being able to change it. The set was knowable but not adjustable: clicking Publish (12) published twelve items with no chance to drop one. It matters most on Unlock. The row warns that N locks belong to other users and will fail, and until now there was nothing the user could do with that warning. In the preview those rows can be unchecked. - $pendingQuickAction discriminates the preview screen: which rows it lists, what Execute fires, and whether the partial-match warning applies. - The confirmation prompt moved from the row click to Execute. That is the commit point; opening a preview changes nothing, so prompting there asked the user to confirm a decision they had not made yet. - The partial-match warning stays workflow-only. A quick action's count and its rows are the same client-side filter, so it cannot apply to fewer items than it lists. - onContinueToPreview clears $pendingQuickAction before reading $previewItems, which is discriminated on it — reading first would resolve the rows against a stale quick action. Tests: 9 new preview specs, and the 16 existing quick-action specs rewired to the two-step path. 988 content-drive specs pass, lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @rjvelazco's task in 2m 30s —— View job Claude finished @rjvelazco's task in 3m 51s —— View job Code Review — Angular anti-patterns, defects, bugs, flaky tests
The latest commit ( New Issues
Existing (still open from prior review)
Resolved
Checked and clean
Reviewed by Claude (Claude Code) on this synchronize. The three |
github-actions
Bot
added
Area : Backend
…loses Execution moves out of the Action Center dialog and into the store, so a run outlives the dialog that started it. Firing now hands off immediately: the dialog closes, the toolbar reports progress, and a toast reports the outcome. - `withActionExecution` owns both fire paths (`fireDefaultAction`, `bulkFire`), the in-flight state and the result. Holding the subscription in the store makes surviving the dialog close a deliberate property rather than a leaked subscription, and gives a reopened dialog truthful state — which is what stops the same rows being fired twice. - The completion toast moves to the shell, which owns `<p-toast>` and is never destroyed while the portlet is open, so it can report a result whose originating dialog is already gone. It also refreshes the grid and closes the dialog. - The toolbar shows "Applying <action> to N item(s)…" while a run is in flight — the only progress signal once the dialog is closed. - The folders-ignored notice becomes a plain element instead of `p-message`. PrimeNG 21 animates a Message's height from zero over 300ms with no way to opt out (`motionOptions` is declared but never consumed, the duration is not a design token, and there is no global config), which read as the notice arriving late and shoving the action list down as it expanded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…for improved animation handling
…bulk-lock-and-unlock-quick-actions
rjvelazco
changed the title
|
@claude Review this code and look for: Angular anti-patterns, defects, possible bugs, flaky tests, etc. For each issue found, also propose the correct way to fix it. |
Code Review — Angular anti-patterns, defects, bugs, flaky tests
Working on this now. |
zJaaal
left a comment
zJaaal
left a comment
There was a problem hiding this comment.
Review of the full diff (26 files): the backend SystemAction additions, the withActionExecution store feature, the Action Center preview rework and the toolbar/shell reporting.
The route decision holds up. SystemActionApiFireCommandFactory's direct-API fallback really does cover a system action with no workflow mapping, so adding LOCK/UNLOCK there rather than shipping _bulklock endpoints is the right call, and the enum documenting why those two have no actionlet is the sort of thing the next reader needs.
Eight findings, seven of them inline. Two I would fix before merge:
- The integration tests never run.
WorkflowResourceLockUnlockIntegrationTestis not registered in anyMainSuite, so the whole server-side proof of the contract is inert. Inline on the class declaration. - The Unlock warning is wrong for exactly the users it is about.
contentEditabledoes not know about the CMS Administrator role. Inline onaction-center.ts.
Plus one design gap worth a decision rather than a fix: the preview was added so the user could uncheck locks held by others, but the preview cannot show which rows those are. Suggestion inline on the Action Center template.
Not inline (line falls outside the diff hunks)
WorkflowResource.java:3961-3966 — fireMergeActionDefault still advertises allowableValues without LOCK/UNLOCK.
The other three fire endpoints were updated (3420, 3653, 4552); this one was missed. PATCH /actions/default/fire/{systemAction} routes through the same fireTransactionalAction -> mergeContentlet -> factory fallback, so it does accept both at runtime. The result is that the generated openapi.yaml documents the same enum inconsistently across the four paths that share it, and a client generated from it cannot call this one with the new values.
Checked and clean
Ruled these out rather than assuming them, in case it saves anyone re-treading:
- DI after the move into the store:
DotWorkflowActionsFireServiceandDotHttpErrorManagerServiceare both inapps/dotcms-ui/src/app/providers.ts, so injecting them at store level resolves. Thedot-folder-list-context-menuspec was correctly updated for the real store. DotHttpErrorManagerService.handle()is eager (side effects beforeof(result)), so not subscribing to it insidecatchErroris fine.- The shell effect cannot loop:
loadItems/closeDialog/clearActionExecutionResultare all insideuntracked, andloadItemsdoes setLOADINGand clearselectedItemsas its comment claims. contentEditableis genuinely hydrated onto drive rows (BrowserAPIImpl.hydrate), so it is not an undefined field that would make every Unlock row warn.Contentlet.isLocked()reads throughVersionableAPIrather than the property map, sounlock's guard works on the detached copycreateContentletbuilds.fireDefaultAction's return-type change has exactly one caller, andentity = {results, summary}matches whatResponseUtil's streaming wrapper emits.- The
no-enter-motionCSS override does win: PrimeNG 21 putsstyleClasson thep-messagehost alongsideanimate.enter, and the theme rule isanimation: p-animate-message-enter, soanimation: noneat higher specificity beats it. content-drive.context-menu.lock/.unlockalready exist inLanguage.properties; the non-ASCII in that file is pre-existing.
Reviewed by Claude (Claude Code), posted from @zJaaal's account.
| * | ||
| * @see SystemActionApiFireCommandFactory | ||
| */ | ||
| public class WorkflowResourceLockUnlockIntegrationTest { |
There was a problem hiding this comment.
Medium — these 8 tests will never run.
This class is not registered in any suite:
$ git grep -n LockUnlockIntegrationTest -- dotcms-integration
dotcms-integration/.../WorkflowResourceLockUnlockIntegrationTest.java:55
One match, the file itself. Its sibling is wired up — MainSuite2a.java:71 lists com.dotcms.rest.api.v1.workflow.WorkflowResourceIntegrationTest.class.
Integration tests only execute through the MainSuite* classes, so as it stands the entire server-side proof of the contract this PR introduces — per-item failure reporting, mixed ownership yielding a partial result, canLock permission enforcement — does not run in CI. A regression in LockSystemActionApiFireCommandImpl or UnlockSystemActionApiFireCommandImpl would land green.
Adding the class next to its sibling in MainSuite2a should be all it takes.
| // grid has no idea whether the current user holds that role. So these items are counted, | ||
| // fired, and reported on rather than filtered out: `contentEditable` is false on a locked | ||
| // row the current user does not hold. | ||
| warnWhen: (item) => !item.contentEditable, |
There was a problem hiding this comment.
Medium — this warns the one user for whom it is false.
The hint says "Only an administrator can unlock those, and they will be reported as failed", but contentEditable cannot tell you whether the caller is that administrator.
contentEditable is derived in BrowserAPIImpl.WfData (BrowserAPIImpl.java:2636) as essentially lockedUserId.equals(user.getUserId()) — it never consults the CMS Administrator role. The server-side gate is ESContentletAPIImpl.canLock (:10358), which returns true immediately for a CMS Admin.
So: an admin selects 5 locked rows, 3 held by other users. The row reads "3 … will be reported as failed", they fire it, and all 5 unlock. The warning was pure noise for the only role that can act on it.
Second, narrower false positive: a user who holds the lock but has EDIT at the content-type level rather than on the contentlet. contentEditable only checks contentlet WRITE, while canLock accepts either.
The comment above is honest that "the grid has no idea whether the current user holds that role" — the gap is that the copy then asserts an outcome anyway. Two ways out: soften the hint to something conditional ("may require administrator permission"), or plumb the caller's admin-ness down so warnWhen can return false outright for admins. The second also fixes the preview marker (see the template comment).
| @if ($selectedAction(); as action) { | ||
| <!-- Workflow actions only. A quick action's count and its rows come from the same | ||
| client-side filter, so it can never apply to fewer items than it lists. --> | ||
| @if (!$pendingQuickAction() && $selectedAction(); as action) { |
There was a problem hiding this comment.
Medium — the preview cannot show the thing it was added for.
The PR body's justification for routing quick actions through the preview is that on Unlock "unchecking those rows is the only way to act on the warning". But the preview renders no per-row lock-owner information: this @if is gated on !$pendingQuickAction(), and dot-content-drive-action-preview.component.html has no lock marker at all — every row gets the same dot-contentlet-status-badge.
So the user sees "Unlock (10) ⚠3", drills in, and has no way to tell which 3 rows to uncheck. The capability the preview exists to provide is not reachable.
Suggestion: use Content Drive's own table instead of a second hand-rolled one.
DotFolderListViewComponent (@dotcms/portlets/content-drive/ui, exported from ui/src/index.ts) already renders the lock state per row:
<span data-testId="lock-hint" class="text-gray-700">
@if (!isFolder) {
@if (item.locked) {
<i class="pi pi-lock" data-testId="lock-icon"></i>
} @else {
<i class="pi pi-lock-open" data-testId="lock-open-icon"></i>
}
}
</span>…alongside the checkbox column, dot-content-thumbnail, dot-contentlet-status-badge, type and language. The preview component's own header comment says "Column markup follows the main grid (dot-folder-list-view) so a row reads the same in both places" — it is deliberately reproducing that component, and the one column it dropped is the column this finding needs. Reusing it gets the marker for free and removes the drift risk between the two tables.
Two honest caveats, both small and both in the ui lib:
- Selection is not an input yet. Today it is
[(selection)]="selectedItems"against an untyped internal field (selectedItems = [],:189), with an effect at:308that clears it whenever$items()changes. The preview needs the parent to own the included set, so this wants aselectioninput (and adisabledinput for the in-flight state) added toDotFolderListViewComponent. That effect would also need to not stomp a caller-provided selection. item.lockedalone is not the right signal. It says "locked", not "locked by someone else", so the marker has to key off whateverwarnWhensettles on in the sibling comment above — otherwise the icon flags all 10 rows instead of the 3 that matter.
If that is more than you want in this PR, the smaller version is to add the marker to the existing preview table and keep the two in sync; but given the preview is already a copy of dot-folder-list-view, reusing it looks like the cheaper end state.
| // Archived content is a dead end until unarchived, and `deleteContentlets` honours | ||
| // `canLock`, so locking an archived item would quietly make it undeletable by anyone but | ||
| // the lock holder. | ||
| eligibleWhen: (item) => !item.locked && !item.archived |
There was a problem hiding this comment.
Low/Medium — this guard is client-side only, and the state it prevents is now reachable over the API.
The comment is right about the hazard: deleteContentlets honours canLock, so a locked archived item becomes undeletable by anyone but the holder. The problem is that !item.archived is the only thing enforcing it.
ESContentletAPIImpl.lock (:3924) checks for a blank inode and canLock, and never rejects archived content. WorkflowResource.checkContentletState only guards isNew(). Now that LOCK is a fireable SystemAction, a direct POST /api/v1/workflow/actions/default/fire/LOCK on an archived inode produces exactly the dead end the comment says must be avoided — no UI needed.
Worth mirroring the check in LockSystemActionApiFireCommandImpl (or in checkContentletState) so the invariant lives where it is actually enforceable. Pre-existing for the context-menu path, but this PR is what makes it addressable without the UI.
| .subscribe((result) => | ||
| onSettled({ | ||
| actionName, | ||
| successCount: result?.summary?.successCount ?? inodes.length, |
There was a problem hiding this comment.
Low — ?? inodes.length undoes the fix the surrounding comment describes.
The PR body is explicit that counts must come from summary rather than the number of inodes sent, because the endpoint answers 200 with per-item failures inside. This fallback reinstates the old behaviour for any response that parses but arrives without a summary.
That is not hypothetical: printResponseEntityViewResult (WorkflowResource.java ~4200) swallows IOException and can abort mid-stream, and isFail can throw NoSuchElementException out of the loop on an empty result map. Either way the client can get a body with results and no summary, and then reports "all N succeeded, 0 failed" with a green toast — the most reassuring possible message for the case where nothing is known to have succeeded.
?? 0 would be consistent with the intent; treating a missing summary as an error and routing it through httpErrorManagerService would be better still. Note failCount below already defaults to 0, so the two halves currently disagree about what a missing summary means.
| const { actionName, successCount, skippedCount, failCount } = result; | ||
|
|
||
| const detail = | ||
| failCount > 0 |
There was a problem hiding this comment.
Low — a run that both skips and fails never reports the skips.
The ladder is failCount > 0 → skippedCount > 0 → plain, so the two are mutually exclusive in the output even though the backend can return both. A bulk workflow fire over a mixed-type selection where some contentlets' schemes do not own the action (skipped) and some are locked (failed) shows only the failure copy.
That is worse than dropping a number: executed-with-fails reads as "these N could not be actioned" and attributes the whole shortfall to permissions or locks, when part of it was items that were merely on the wrong step. The user's next move (go unlock things) is then wrong.
Either add a both-present message, or compose the detail from the non-zero parts instead of picking one branch.
| this.#store.loadItems(); | ||
| // A no-op when the user already closed the dialog, which is the common path now that | ||
| // firing hands off to the toolbar. | ||
| this.#store.closeDialog(); |
There was a problem hiding this comment.
Low — not a no-op in the other reachable case.
The comment covers the common path, but there is a second one: firing hands off and closes the dialog, and the dialog is [closable]="true", so the user can reopen the Action Center while a run is still in flight. When the run settles, this line closes it out from under them.
And the reopened dialog is not useful in the meantime — $executing disables every quick-action row and the Done button (dot-content-drive-action-center.component.ts:222). So the sequence is: reopen, find everything greyed out, then have the window vanish on its own.
The store already knows it is executing, which is the ingredient needed either way: block reopening while a run is in flight, or gate this closeDialog() on the dialog being the one that fired. The PR body frames truthful state in a reopened dialog as a feature of moving execution into the store, so this looks like a gap in that story rather than a disagreement with it.
…mary Three findings from @zJaaal's review of #36921. 1. The lock/unlock integration tests never ran in CI. WorkflowResourceLockUnlockIntegrationTest was in no MainSuite, so the entire server-side proof of this PR's contract — per-item failure reporting, mixed ownership, canLock enforcement — was inert and a regression in either fire command would have landed green. Registered next to its sibling in MainSuite2a. 2. The Unlock warning asserted an outcome it cannot know. The hint said locks held by others "will be reported as failed", but contentEditable is lockedBy == currentUser and never consults the CMS Administrator role, while canLock returns true for an admin before it looks at the owner. An admin was warned about rows they would go on to unlock successfully. The copy now says "may require administrator permission" rather than predicting the outcome. The two known false positives — administrators, and EDIT held at content-type rather than contentlet level — are documented at warnWhen, along with what removing them would take (plumbing DotCurrentUser.admin), since that is the same plumbing the preview needs to mark the offending rows. 3. A missing summary reported every item as a success. successCount fell back to inodes.length while failCount fell back to 0, so a 200 whose body carried no summary produced "all N succeeded" with a green toast — the most reassuring possible message for the case where nothing is known to have succeeded. That body is reachable: the endpoint streams results then summary, and the writer swallows an IOException mid-stream. Neither fallback is honest, since inodes.length and 0 both invent a number the server never sent, so an absent summary now routes through the same error path as a transport failure and publishes no result at all. A summary that genuinely reports zero successes is still a fact and still reaches the toast, which is pinned by its own test. Left for a separate decision: the preview still cannot show which rows hold foreign locks, which is what the preview was added to enable. Tests: 1032 content-drive specs pass, dotcms-integration compiles, lint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 participants
Closes #36844
Adds Lock and Unlock to the Content Drive Action Center's Quick Actions, and — because firing them exposed two rough edges in the surrounding dialog — routes every quick action through the preview screen and moves execution out of the dialog so a run survives the dialog closing.
Built TDD: the first commit is tests only (Red), the rest make them pass.
Route decision (AC #1)
Lock/Unlock become
SystemActionvalues, fired through the existing multi-contentlet endpointPOST /api/v1/workflow/actions/default/fire/{systemAction}. No new_bulklock/_bulkunlockendpoint, and no shipped workflow action.That works because
SystemActionApiFireCommandFactoryalready provides a direct-API fallback for a system action with no workflow mapping — the same mechanism that letsPUBLISHwork on a content type with no scheme. Locking is per user rather than a workflow transition, soLOCK/UNLOCKdeliberately have no actionlet and nothing to map to; mapping them to a workflow action has no effect, and the enum documents that.The alternative — a pair of bulk endpoints — would have duplicated a request/response shape the multi-contentlet fire endpoint already has (
{results, summary}), for no behaviour the fallback does not already give us.Proposed Changes
Backend
SystemActiongainsLOCKandUNLOCK, documented as the two members with no actionlet, served by the API-call fallback.SystemActionApiFireCommandFactorygains fire commands for both, callingContentletAPI#lock/#unlockdirectly.openapi.yamlregenerated from the annotations.Quick Actions
warnWhen/warningCount/warningHinton a quick action. Unlock uses it to flag locks held by other users, detected from the row'scontentEditableflag (the server's answer to "is this locked by me?").summary.successCount/summary.failCounton the response instead of the number of inodes sent. The endpoint answers200with per-item failures inside, so this corrects every quick action, not just these two.Quick actions now use the preview screen
Previously only workflow actions drilled into the preview; a quick action fired straight from its row. Clicking Publish (12) published twelve items with no chance to drop one. The set was knowable but not changeable — which matters most on Unlock, where the row warns that some locks belong to other users and unchecking those rows is the only way to act on the warning.
Execution moved into the store
withActionExecutionstore feature owns both fire paths, the in-flight state and the result. Holding the subscription in the store makes surviving a dialog close a deliberate property rather than a leaked subscription — and gives a reopened dialog truthful state, which is what stops the same rows being fired twice.Applying <action> to N item(s)…, and a toast reports the outcome. Partial failure downgrades the toast to a warning.<p-toast>and outlives every dialog, so it can report a result whose originating dialog is already gone. It also refreshes the grid and clears the selection.p-message: PrimeNG 21 animates a Message's height from zero over 300ms with no way to opt out (motionOptionsis declared but never consumed, the duration is not a design token, there is no global config), which read as the notice arriving late and shoving the action list down as it expanded.Scope note for reviewers
The last two sections go beyond #36844's acceptance criteria. They are here because Unlock is what surfaced them: it is the first quick action where the user needs to edit the set before firing, and the first where a partial failure is the expected outcome rather than an error. Happy to split them out if you would rather review them separately.
Checklist
Language.properties:content-drive.action-center.unlock.locked-by-others,content-drive.action-center.applyingPermission for lock/unlock is enforced server-side in the fire command, not only in the UI, and integration tests cover the allowed and denied paths. Releasing another user's lock stays restricted to CMS Administrators — the client offers the attempt and reports the failure, it does not bypass the check.
Additional Info
Testing
WorkflowResourceLockUnlockIntegrationTest, 8 tests): single and bulk happy paths; a lock held by another user reported per item with the batch not refused and the lock surviving; mixed ownership yielding a partial result rather than all-or-nothing; server-side permission enforcement.portlets-content-drive, covering eligibility counts, the warning count, the execute path, delta accumulation and the toast's three outcomes.pnpm nx build dotcms-uisucceeds.One thing worth knowing:
tsconfig.spec.jsonsetsisolatedModules: true, so Jest transpiles per-file with no cross-file type checking. A store-composition type error passed 1,000 green tests and only failed atnx build. Green tests are not a typecheck for anything touchingsignalStorecomposition.Follow-up, not in this PR: the same progress/result contract would let bulk publish/unpublish/archive report per-item outcomes, but those run on
DotConcurrentFactoryrather than the job queue and have nojobIdto monitor — see #36894.Screenshots