Skip to content

Add failing tests for bulk Lock and Unlock quick actions - #36921

Open
rjvelazco wants to merge 5 commits into
mainfrom
issue-36844-content-drive-action-center-add-bulk-lock-and-unlock-quick-actions
Open

Add failing tests for bulk Lock and Unlock quick actions#36921
rjvelazco wants to merge 5 commits into
mainfrom
issue-36844-content-drive-action-center-add-bulk-lock-and-unlock-quick-actions

Conversation

@rjvelazco

@rjvelazco rjvelazco commented Aug 6, 2026

Copy link
Copy Markdown
Member

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 SystemAction values, fired through the existing multi-contentlet endpoint POST /api/v1/workflow/actions/default/fire/{systemAction}. No new _bulklock / _bulkunlock endpoint, and no shipped workflow action.

That works because SystemActionApiFireCommandFactory already provides a direct-API fallback for a system action with no workflow mapping — the same mechanism that lets PUBLISH work on a content type with no scheme. Locking is per user rather than a workflow transition, so LOCK/UNLOCK deliberately 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

  • SystemAction gains LOCK and UNLOCK, documented as the two members with no actionlet, served by the API-call fallback.
  • SystemActionApiFireCommandFactory gains fire commands for both, calling ContentletAPI#lock / #unlock directly.
  • Permission is enforced server-side, independent of what the UI offered.
  • openapi.yaml regenerated from the annotations.

Quick Actions

  • Lock counts selected contentlets that are not locked; Unlock counts those that are. Neither is listed at a count of zero, and folders are excluded — consistent with the rest of the dialog.
  • New warnWhen / warningCount / warningHint on a quick action. Unlock uses it to flag locks held by other users, detected from the row's contentEditable flag (the server's answer to "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, so failures are reported rather than pre-filtered.
  • Result counts now come from summary.successCount / summary.failCount on the response instead of the number of inodes sent. The endpoint answers 200 with 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

  • New withActionExecution store 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.
  • Firing hands off immediately: the dialog closes, the toolbar reports Applying <action> to N item(s)…, and a toast reports the outcome. Partial failure downgrades the toast to a warning.
  • The toast moved to the shell, which owns <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.
  • The folders-ignored notice became 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, 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

  • Tests
  • Translations — new keys in Language.properties: content-drive.action-center.unlock.locked-by-others, content-drive.action-center.applying
  • Security Implications Contemplated

Permission 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

  • Integration (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.
  • Frontend: 1,003 tests passing in portlets-content-drive, covering eligibility counts, the warning count, the execute path, delta accumulation and the toast's three outcomes.
  • pnpm nx build dotcms-ui succeeds.

One thing worth knowing: tsconfig.spec.json sets isolatedModules: 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 at nx build. Green tests are not a typecheck for anything touching signalStore composition.

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 DotConcurrentFactory rather than the job queue and have no jobId to monitor — see #36894.

Screenshots

Original Updated
** original screenshot ** ** updated screenshot **

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>
rjvelazco and others added 2 commits August 6, 2026 13:36
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

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rjvelazco's task in 8m 27s —— View job


Code Review: Bulk Lock/Unlock quick actions

Reviewed the full diff against origin/main (base ca0a0d23): backend fire commands + enum, the withActionExecution store feature, the Action Center refactor to route quick actions through preview, the shell toast/reload effect, the toolbar indicator, and the integration test.

New Issues

  • 🟡 Medium: withActionExecution.ts:109successCount: result?.summary?.successCount ?? inodes.length. The fallback contradicts the feature's own stated contract ("counts come from the response, never from inodes.length"). If the endpoint answers 200 with a missing/malformed summary, every submitted inode is reported as a success and the toast lands green — precisely the misreport the PR set out to fix. The bulkFire path defaults the same counts to 0 (result?.successCount ?? 0, line 161), so a defensive 0 here would be both safer and consistent with the sibling branch. Assumption: the endpoint always returns a well-formed summary; What to verify: confirm no code path (empty body, streamed error) can yield entity without summary. Fix this →

  • 🟡 Medium: dot-content-drive-shell.component.ts:405 — the result effect calls loadItems() (which clears the selection) for every settled run, including a run where successCount === 0 and everything failed. On an Unlock where all locks are held by others, nothing changed on the server yet the user's selection is wiped and the grid reloads, so they cannot retry or adjust the set without re-selecting. The PR body acknowledges this as a deliberate tradeoff — flagging as non-blocking so it's a conscious call rather than an oversight. Consider skipping the reload/clear when successCount === 0 && skippedCount === 0.

Notes (non-blocking, no action required)

  • Backend LockSystemActionApiFireCommandImpl / UnlockSystemActionApiFireCommandImpl (SystemActionApiFireCommandFactory.java:663,695) correctly follow the existing command pattern: @WrapInTransaction, contentletAPI.lock/unlock(contentlet, user, dependencies.isRespectAnonymousPermissions()), permission enforced inside canLock. Verified lock/unlock signatures match (third arg is respectFrontendRoles).
  • Adding LOCK/UNLOCK to the SystemAction enum is safe: the only switch on it (WorkflowHelper.findActions:527) has a default branch, and WorkflowCacheImpl iterates .values() generically. No exhaustive switch breaks.
  • The fireDefaultAction return-type change (DotCMSContentlet[]DotFireDefaultActionResult) has exactly one production consumer (withActionExecution), confirmed no other caller relies on the old shape.
  • contentEditable is genuinely populated on Content Drive rows — ContentDriveHelper delegates to BrowserAPI, and BrowserAPIImpl:2636 sets contentEditable = lockedUserId.present && user == lockedUser, so warnWhen: !item.contentEditable is a valid "locked by someone else" heuristic.
  • $executing moving from a local signal to computed(() => !!store.actionExecution()) correctly closes the double-fire window; the store's in-flight guard (withActionExecution.ts:86,128) is the real defense and is tested (dot-content-drive.store.spec.ts:1099).
  • Integration coverage is strong: the grantEdit-on-content-type detail (test lines 213-218, 319-338) is a genuine trap the tests correctly avoid — a contentlet-only grant would fail at populateContentlet and never exercise canLock.

Backend permission enforcement, transaction boundaries, TDD ordering, and i18n keys (content-drive.context-menu.lock/unlock, .applying, .locked-by-others, .executed-with-fails) all check out. No critical or high-severity issues.

@github-actions github-actions Bot added Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code labels Aug 6, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Content Drive Action Center: add bulk Lock and Unlock quick actions

1 participant