From 5a355eac0d6ae43a202b122a67479942887445aa Mon Sep 17 00:00:00 2001 From: rjvelazco Date: Thu, 6 Aug 2026 12:59:07 -0400 Subject: [PATCH 1/5] Add failing tests for bulk Lock and Unlock quick actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../dot-workflow-actions-fire.service.spec.ts | 31 +- ...tent-drive-action-center.component.spec.ts | 190 ++++++++- .../src/lib/utils/action-center.spec.ts | 86 ++++ ...flowResourceLockUnlockIntegrationTest.java | 384 ++++++++++++++++++ 4 files changed, 689 insertions(+), 2 deletions(-) create mode 100644 dotcms-integration/src/test/java/com/dotcms/rest/api/v1/workflow/WorkflowResourceLockUnlockIntegrationTest.java diff --git a/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.spec.ts b/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.spec.ts index c93d72379418..43dfb45418d7 100644 --- a/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.spec.ts +++ b/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.spec.ts @@ -2,7 +2,11 @@ import { createHttpFactory, HttpMethod, SpectatorHttp } from '@openng/spectator/ import { HttpHeaders } from '@angular/common/http'; -import { DotActionBulkRequestOptions, DotActionBulkResult } from '@dotcms/dotcms-models'; +import { + DotActionBulkRequestOptions, + DotActionBulkResult, + DotFireDefaultActionResult +} from '@dotcms/dotcms-models'; import { dotcmsContentletMock } from '@dotcms/utils-testing'; import { DotWorkflowActionsFireService } from './dot-workflow-actions-fire.service'; @@ -346,6 +350,31 @@ describe('DotWorkflowActionsFireService', () => { }); }); + it('should fire a default system action over multiple inodes and return its summary', (done) => { + // The endpoint streams one entry per contentlet plus a summary. The summary is the only + // honest source of success/fail counts: individual items can fail while the request is 200. + const mockResult: DotFireDefaultActionResult = { + results: [], + summary: { affected: 2, successCount: 1, failCount: 1, time: 12 } + }; + + spectator.service + .fireDefaultAction({ action: 'UNLOCK', inodes: ['1', '2'] }) + .subscribe((res) => { + expect(res).toEqual(mockResult); + done(); + }); + + const req = spectator.expectOne( + '/api/v1/workflow/actions/default/fire/UNLOCK?indexPolicy=WAIT_FOR', + HttpMethod.POST + ); + + expect(req.request.body).toEqual({ contentlet: [{ inode: '1' }, { inode: '2' }] }); + + req.flush({ entity: mockResult }); + }); + afterEach(() => { spectator.controller.verify(); }); diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.spec.ts index 353ecfaaa5c2..b960f78a9b7e 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.spec.ts @@ -133,7 +133,12 @@ describe('DotContentDriveActionCenterComponent', () => { jest.spyOn(workflowsActionsService, 'getBulkActions').mockReturnValue( of(BULK_ACTIONS_RESPONSE) ); - jest.spyOn(fireService, 'fireDefaultAction').mockReturnValue(of([])); + jest.spyOn(fireService, 'fireDefaultAction').mockReturnValue( + of({ + results: [], + summary: { affected: 1, successCount: 1, failCount: 0, time: 1 } + }) + ); jest.spyOn(fireService, 'bulkFire').mockReturnValue( of({ successCount: 2, skippedCount: 0, fails: [] }) ); @@ -409,6 +414,189 @@ describe('DotContentDriveActionCenterComponent', () => { expect(httpErrorManager.handle).toHaveBeenCalledWith(error); expect(store.closeDialog).not.toHaveBeenCalled(); }); + + it('should report the counts the endpoint returned, not the number of inodes sent', () => { + // Per-item failures are an expected outcome (a lock held by another user, a permission + // the row state can't see), so the toast has to reflect what the server actually did. + jest.spyOn(fireService, 'fireDefaultAction').mockReturnValue( + of({ + results: [], + summary: { affected: 2, successCount: 1, failCount: 1, time: 1 } + }) + ); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-PUBLISH"]'); + + expect(messageService.add).toHaveBeenCalledWith( + expect.objectContaining({ + severity: 'warn', + detail: expect.stringContaining('executed-with-fails') + }) + ); + }); + + it('should report a plain success when nothing failed', () => { + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-PUBLISH"]'); + + expect(messageService.add).toHaveBeenCalledWith( + expect.objectContaining({ + severity: 'success', + detail: expect.stringContaining('executed-detail') + }) + ); + }); + }); + + describe('lock and unlock quick actions', () => { + it('should fire LOCK with only the unlocked inodes', () => { + mockSelectedItems.set([ + contentlet({ inode: 'unlocked-1', locked: false }), + contentlet({ inode: 'locked-1', locked: true }) + ]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-LOCK"]'); + + expect(fireService.fireDefaultAction).toHaveBeenCalledWith({ + action: 'LOCK', + inodes: ['unlocked-1'] + }); + }); + + it('should fire UNLOCK with only the locked inodes', () => { + mockSelectedItems.set([ + contentlet({ inode: 'unlocked-1', locked: false }), + contentlet({ inode: 'locked-1', locked: true }) + ]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-UNLOCK"]'); + + expect(fireService.fireDefaultAction).toHaveBeenCalledWith({ + action: 'UNLOCK', + inodes: ['locked-1'] + }); + }); + + it('should keep Unlock non-selectable when nothing in the selection is locked', () => { + mockSelectedItems.set([contentlet({ inode: 'unlocked-1', locked: false })]); + + spectator.detectChanges(); + + const unlock = spectator.query( + '[data-testid="quick-action-UNLOCK"]' + ) as HTMLButtonElement; + + expect(unlock.disabled).toBe(true); + }); + + it('should keep Lock non-selectable when everything is already locked', () => { + mockSelectedItems.set([contentlet({ inode: 'locked-1', locked: true })]); + + spectator.detectChanges(); + + const lock = spectator.query('[data-testid="quick-action-LOCK"]') as HTMLButtonElement; + + expect(lock.disabled).toBe(true); + }); + + it('should flag on the Unlock row how many locks are held by other users', () => { + mockSelectedItems.set([ + contentlet({ inode: 'mine', locked: true, contentEditable: true }), + contentlet({ inode: 'theirs', locked: true, contentEditable: false }) + ]); + + spectator.detectChanges(); + + expect(spectator.query('[data-testid="quick-action-warning-UNLOCK"]')).toBeTruthy(); + }); + + it('should not flag the Unlock row when every lock is the current user’s own', () => { + mockSelectedItems.set([ + contentlet({ inode: 'mine', locked: true, contentEditable: true }) + ]); + + spectator.detectChanges(); + + expect(spectator.query('[data-testid="quick-action-warning-UNLOCK"]')).toBeNull(); + }); + + it('should still fire every locked item when some are held by other users', () => { + // Attempt-all is deliberate: the client cannot know whether the user holds the CMS + // Administrator role that lets them release someone else's lock. + mockSelectedItems.set([ + contentlet({ inode: 'mine', locked: true, contentEditable: true }), + contentlet({ inode: 'theirs', locked: true, contentEditable: false }) + ]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-UNLOCK"]'); + + expect(fireService.fireDefaultAction).toHaveBeenCalledWith({ + action: 'UNLOCK', + inodes: ['mine', 'theirs'] + }); + }); + + it('should report partially failed unlocks back to the user', () => { + jest.spyOn(fireService, 'fireDefaultAction').mockReturnValue( + of({ + results: [], + summary: { affected: 2, successCount: 1, failCount: 1, time: 1 } + }) + ); + mockSelectedItems.set([ + contentlet({ inode: 'mine', locked: true, contentEditable: true }), + contentlet({ inode: 'theirs', locked: true, contentEditable: false }) + ]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-UNLOCK"]'); + + expect(messageService.add).toHaveBeenCalledWith( + expect.objectContaining({ + severity: 'warn', + detail: expect.stringContaining('executed-with-fails') + }) + ); + }); + + it('should exclude folders from Lock', () => { + mockSelectedItems.set([ + contentlet({ inode: 'unlocked-1', locked: false }), + folder('folder-1') + ]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-LOCK"]'); + + expect(fireService.fireDefaultAction).toHaveBeenCalledWith({ + action: 'LOCK', + inodes: ['unlocked-1'] + }); + }); + + it('should refresh the grid and clear the selection after unlocking', () => { + mockSelectedItems.set([contentlet({ inode: 'locked-1', locked: true })]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-UNLOCK"]'); + + expect(store.setSelectedItems).toHaveBeenCalledWith([]); + expect(store.loadItems).toHaveBeenCalled(); + expect(store.closeDialog).toHaveBeenCalled(); + }); + + it('should not confirm before locking or unlocking', () => { + mockSelectedItems.set([contentlet({ inode: 'locked-1', locked: true })]); + + spectator.detectChanges(); + spectator.click('[data-testid="quick-action-UNLOCK"]'); + + expect(confirmationService.confirm).not.toHaveBeenCalled(); + }); }); describe('workflow actions', () => { diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/action-center.spec.ts b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/action-center.spec.ts index 7484d4b3a5af..9538d9219c2a 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/action-center.spec.ts +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/utils/action-center.spec.ts @@ -144,6 +144,87 @@ describe('action-center utils', () => { expect(byId.get(WORKFLOW_ACTION_ID.UNARCHIVE)).toBe(1); }); + it('should count Lock for items that are not locked', () => { + const items = [ + contentlet({ inode: 'a', locked: false }), + contentlet({ inode: 'b', locked: true }), + contentlet({ inode: 'c', locked: false }) + ]; + + const lock = getQuickActions(items).find( + (action) => action.id === WORKFLOW_ACTION_ID.LOCK + ); + + expect(lock?.count).toBe(2); + expect(lock?.eligibleInodes).toEqual(['a', 'c']); + }); + + it('should not count archived items as lockable', () => { + // An archived contentlet is always unlocked (archive refuses locked content), so it + // would otherwise be counted — but locking it would only block Unarchive and Delete. + const items = [contentlet({ inode: 'a', archived: true, locked: false })]; + + const lock = getQuickActions(items).find( + (action) => action.id === WORKFLOW_ACTION_ID.LOCK + ); + + expect(lock?.count).toBe(0); + }); + + it('should count Unlock only for locked items', () => { + const items = [ + contentlet({ inode: 'a', locked: true }), + contentlet({ inode: 'b', locked: false }), + contentlet({ inode: 'c', locked: true }) + ]; + + const unlock = getQuickActions(items).find( + (action) => action.id === WORKFLOW_ACTION_ID.UNLOCK + ); + + expect(unlock?.count).toBe(2); + expect(unlock?.eligibleInodes).toEqual(['a', 'c']); + }); + + it('should warn on Unlock about locks held by other users', () => { + // `contentEditable` is the server's answer to "is this locked by *me*?" — false on a + // locked row means someone else holds it, and only an administrator can release it. + const items = [ + contentlet({ inode: 'mine', locked: true, contentEditable: true }), + contentlet({ inode: 'theirs', locked: true, contentEditable: false }), + contentlet({ inode: 'also-theirs', locked: true, contentEditable: false }) + ]; + + const unlock = getQuickActions(items).find( + (action) => action.id === WORKFLOW_ACTION_ID.UNLOCK + ); + + // Every locked item is still fired — the server is the authority on who may unlock. + expect(unlock?.count).toBe(3); + expect(unlock?.warningCount).toBe(2); + expect(unlock?.warningHint).toBeTruthy(); + }); + + it('should not warn on Unlock when every lock is the current user’s own', () => { + const items = [contentlet({ inode: 'mine', locked: true, contentEditable: true })]; + + const unlock = getQuickActions(items).find( + (action) => action.id === WORKFLOW_ACTION_ID.UNLOCK + ); + + expect(unlock?.warningCount).toBe(0); + }); + + it('should not warn on actions other than Unlock', () => { + const items = [contentlet({ inode: 'a', locked: true, contentEditable: false })]; + + for (const action of getQuickActions(items)) { + if (action.id !== WORKFLOW_ACTION_ID.UNLOCK) { + expect(action.warningCount).toBe(0); + } + } + }); + it('should still list actions that apply to nothing, with a zero count', () => { // Nothing archived, so Delete applies to no item — but stays in the list so the dialog // can render it as non-selectable rather than dropping the row. @@ -161,6 +242,8 @@ describe('action-center utils', () => { WORKFLOW_ACTION_ID.ARCHIVE, WORKFLOW_ACTION_ID.DELETE, WORKFLOW_ACTION_ID.UNARCHIVE, + WORKFLOW_ACTION_ID.LOCK, + WORKFLOW_ACTION_ID.UNLOCK, ADD_TO_BUNDLE_ACTION_ID ]; @@ -172,6 +255,9 @@ describe('action-center utils', () => { (a) => a.id ) ).toEqual(expected); + expect( + getQuickActions([contentlet({ inode: 'c', locked: true })]).map((a) => a.id) + ).toEqual(expected); }); it('should mark Add to Bundle as pending so it can never be fired', () => { diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/workflow/WorkflowResourceLockUnlockIntegrationTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/workflow/WorkflowResourceLockUnlockIntegrationTest.java new file mode 100644 index 000000000000..2e9c0285a129 --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/workflow/WorkflowResourceLockUnlockIntegrationTest.java @@ -0,0 +1,384 @@ +package com.dotcms.rest.api.v1.workflow; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import com.dotcms.datagen.ContentTypeDataGen; +import com.dotcms.datagen.ContentletDataGen; +import com.dotcms.datagen.TestUserUtils; +import com.dotcms.datagen.UserDataGen; +import com.dotcms.mock.request.MockAttributeRequest; +import com.dotcms.mock.request.MockHeaderRequest; +import com.dotcms.mock.request.MockHttpRequestIntegrationTest; +import com.dotcms.mock.request.MockSessionRequest; +import com.dotcms.contenttype.model.type.ContentType; +import com.dotcms.rest.EmptyHttpResponse; +import com.dotcms.util.IntegrationTestInitService; +import com.dotcms.workflow.form.FireActionForm; +import com.dotcms.workflow.form.FireMultipleActionForm; +import com.dotmarketing.beans.Permission; +import com.dotmarketing.business.APILocator; +import com.dotmarketing.business.PermissionAPI; +import com.dotmarketing.business.Role; +import com.dotmarketing.portlets.contentlet.business.ContentletAPI; +import com.dotmarketing.portlets.contentlet.model.Contentlet; +import com.dotmarketing.portlets.languagesmanager.model.Language; +import com.dotmarketing.portlets.workflows.business.WorkflowAPI.SystemAction; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.liferay.portal.model.User; +import java.io.ByteArrayOutputStream; +import java.util.ArrayList; +import java.util.Base64; +import java.util.List; +import java.util.Map; +import javax.servlet.http.HttpServletRequest; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.Response.Status; +import javax.ws.rs.core.StreamingOutput; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Covers firing {@link SystemAction#LOCK} and {@link SystemAction#UNLOCK} through the default + * system-action endpoints, both for a single contentlet and for a collection. + * + *

Locking is per user, which makes these two system actions behave unlike the rest: + * {@code canLock} refuses to release a lock held by somebody else unless the caller holds the CMS + * Administrator role. The bulk endpoint therefore reports those items as failures instead of + * refusing the whole batch, and that contract is what most of these tests pin down.

+ * + * @see SystemActionApiFireCommandFactory + */ +public class WorkflowResourceLockUnlockIntegrationTest { + + private static WorkflowResource workflowResource; + private static ContentletAPI contentletAPI; + private static PermissionAPI permissionAPI; + private static User systemUser; + private static Language defaultLanguage; + private static ContentType contentType; + + private static final String ADMIN_EMAIL = "admin@dotcms.com"; + private static final String ADMIN_PASSWORD = "admin"; + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + + // The real WebResource on purpose: these tests hinge on *which* user is calling, so the + // caller has to be resolved from the request's Basic auth rather than stubbed. + workflowResource = new WorkflowResource(); + contentletAPI = APILocator.getContentletAPI(); + permissionAPI = APILocator.getPermissionAPI(); + systemUser = APILocator.systemUser(); + defaultLanguage = APILocator.getLanguageAPI().getDefaultLanguage(); + contentType = new ContentTypeDataGen().nextPersisted(); + } + + /** + * Method to test: {@link WorkflowResource#fireActionDefaultSinglePart} + *

+ * Given scenario: An unlocked contentlet and an administrator firing {@code LOCK} over it. + *

+ * Expected result: 200, and the contentlet is locked by the calling user. + */ + @Test + public void test_fireLock_singleContentlet_locksItForTheCaller() throws Exception { + final Contentlet contentlet = newContentlet(); + + final Response response = fireDefaultAction(adminRequest(), SystemAction.LOCK, contentlet); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + assertEquals(adminUser().getUserId(), lockedBy(contentlet)); + } + + /** + * Method to test: {@link WorkflowResource#fireActionDefaultSinglePart} + *

+ * Given scenario: A contentlet the caller has locked, then firing {@code UNLOCK} over it. + *

+ * Expected result: 200, and the lock is released. + */ + @Test + public void test_fireUnlock_singleContentlet_releasesTheLock() throws Exception { + final Contentlet contentlet = newContentlet(); + contentletAPI.lock(contentlet, adminUser(), false); + + final Response response = fireDefaultAction(adminRequest(), SystemAction.UNLOCK, contentlet); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + assertNull(lockedBy(contentlet)); + } + + /** + * Method to test: {@link WorkflowResource#fireActionDefaultSinglePart} + *

+ * Given scenario: A brand new (never persisted) contentlet firing {@code LOCK}. + *

+ * Expected result: A non-OK response — there is nothing to lock yet, and letting this through + * would lock a contentlet the caller never created. + */ + @Test + public void test_fireLock_newContentlet_isRejected() throws Exception { + final FireActionForm form = new FireActionForm( + new FireActionForm.Builder().contentlet(Map.of("stInode", contentType.inode()))); + + final Response response = workflowResource.fireActionDefaultSinglePart(adminRequest(), + new EmptyHttpResponse(), null, null, "WAIT_FOR", + String.valueOf(defaultLanguage.getId()), "DEFAULT", SystemAction.LOCK, form); + + assertNotEquals(Status.OK.getStatusCode(), response.getStatus()); + } + + /** + * Method to test: {@link WorkflowResource#fireMultipleActionDefault} + *

+ * Given scenario: Three unlocked contentlets, one {@code LOCK} call over all of them. + *

+ * Expected result: Every contentlet is locked and the streamed summary reports three + * successes and no failures — one request, not three. + */ + @Test + public void test_fireLock_multipleContentlets_locksAllOfThem() throws Exception { + final List contentlets = List.of(newContentlet(), newContentlet(), + newContentlet()); + + final JsonNode summary = fireMultiple(adminRequest(), SystemAction.LOCK, contentlets); + + assertEquals(3, summary.get("successCount").asInt()); + assertEquals(0, summary.get("failCount").asInt()); + for (final Contentlet contentlet : contentlets) { + assertEquals(adminUser().getUserId(), lockedBy(contentlet)); + } + } + + /** + * Method to test: {@link WorkflowResource#fireMultipleActionDefault} + *

+ * Given scenario: Two locked contentlets, one {@code UNLOCK} call over both. + *

+ * Expected result: Both locks are released and the summary reports two successes. + */ + @Test + public void test_fireUnlock_multipleContentlets_releasesAllOfThem() throws Exception { + final List contentlets = List.of(newContentlet(), newContentlet()); + for (final Contentlet contentlet : contentlets) { + contentletAPI.lock(contentlet, adminUser(), false); + } + + final JsonNode summary = fireMultiple(adminRequest(), SystemAction.UNLOCK, contentlets); + + assertEquals(2, summary.get("successCount").asInt()); + assertEquals(0, summary.get("failCount").asInt()); + for (final Contentlet contentlet : contentlets) { + assertNull(lockedBy(contentlet)); + } + } + + /** + * Method to test: {@link WorkflowResource#fireMultipleActionDefault} + *

+ * Given scenario: Two contentlets locked by an administrator. A limited user who does + * hold EDIT permission on them — so the permission gate passes — fires {@code UNLOCK}. + *

+ * Expected result: The batch is not refused; both items come back as failures and the locks + * survive. This is the documented rule: a lock held by another user is reported per item, not + * silently skipped and not fatal to the whole request. + */ + @Test + public void test_fireUnlock_lockHeldByAnotherUser_reportsPerItemFailure() throws Exception { + final String password = "TestPass" + System.currentTimeMillis() + "!"; + final User limitedUser = newLimitedUser(password); + final List contentlets = List.of(newContentlet(), newContentlet()); + + for (final Contentlet contentlet : contentlets) { + grantEdit(contentlet, limitedUser); + contentletAPI.lock(contentlet, adminUser(), false); + } + + final JsonNode summary = fireMultiple( + requestForUser(limitedUser.getEmailAddress(), password), SystemAction.UNLOCK, + contentlets); + + assertEquals(0, summary.get("successCount").asInt()); + assertEquals(2, summary.get("failCount").asInt()); + for (final Contentlet contentlet : contentlets) { + assertEquals("The lock must survive a denied unlock", adminUser().getUserId(), + lockedBy(contentlet)); + } + } + + /** + * Method to test: {@link WorkflowResource#fireMultipleActionDefault} + *

+ * Given scenario: A mixed batch — one contentlet locked by the calling user, one locked by + * somebody else — fired as {@code UNLOCK} by that limited user. + *

+ * Expected result: The caller's own lock is released while the other one is reported as a + * failure. A partial result, not an all-or-nothing one. + */ + @Test + public void test_fireUnlock_mixedOwnership_unlocksOwnLockAndFailsTheOther() throws Exception { + final String password = "TestPass" + System.currentTimeMillis() + "!"; + final User limitedUser = newLimitedUser(password); + final Contentlet ownLock = newContentlet(); + final Contentlet foreignLock = newContentlet(); + + grantEdit(ownLock, limitedUser); + grantEdit(foreignLock, limitedUser); + contentletAPI.lock(ownLock, limitedUser, false); + contentletAPI.lock(foreignLock, adminUser(), false); + + final JsonNode summary = fireMultiple( + requestForUser(limitedUser.getEmailAddress(), password), SystemAction.UNLOCK, + List.of(ownLock, foreignLock)); + + assertEquals(1, summary.get("successCount").asInt()); + assertEquals(1, summary.get("failCount").asInt()); + assertNull(lockedBy(ownLock)); + assertEquals(adminUser().getUserId(), lockedBy(foreignLock)); + } + + /** + * Method to test: {@link WorkflowResource#fireMultipleActionDefault} + *

+ * Given scenario: A limited user with no EDIT permission on the contentlet fires {@code LOCK}. + *

+ * Expected result: The item is reported as a failure and stays unlocked — permission is + * enforced server-side, regardless of what the UI offered. + */ + @Test + public void test_fireLock_withoutEditPermission_reportsFailure() throws Exception { + final String password = "TestPass" + System.currentTimeMillis() + "!"; + final User limitedUser = newLimitedUser(password); + final Contentlet contentlet = newContentlet(); + + // Individual permissions granting READ to the system role only, which overrides whatever + // the contentlet inherited — the limited user is left without EDIT. + final Permission readForSystemRoleOnly = new Permission(contentlet.getPermissionId(), + APILocator.getRoleAPI().loadRoleByKey(systemUser.getUserId()).getId(), + PermissionAPI.PERMISSION_READ, true); + permissionAPI.save(readForSystemRoleOnly, contentlet, systemUser, false); + + final JsonNode summary = fireMultiple( + requestForUser(limitedUser.getEmailAddress(), password), SystemAction.LOCK, + List.of(contentlet)); + + assertEquals(0, summary.get("successCount").asInt()); + assertEquals(1, summary.get("failCount").asInt()); + assertNull(lockedBy(contentlet)); + } + + /** + * Method to test: {@link WorkflowResource#fireActionDefaultSinglePart} + *

+ * Given scenario: A locked contentlet, unlocked and then locked again by the same caller. + *

+ * Expected result: Both calls succeed — LOCK on content the caller already holds is a no-op + * rather than an error, so re-firing over a stale selection cannot fail the batch. + */ + @Test + public void test_fireLock_alreadyLockedByCaller_succeeds() throws Exception { + final Contentlet contentlet = newContentlet(); + contentletAPI.lock(contentlet, adminUser(), false); + + final Response response = fireDefaultAction(adminRequest(), SystemAction.LOCK, contentlet); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + assertEquals(adminUser().getUserId(), lockedBy(contentlet)); + } + + // ------------------------------------------------------------------------ + // helpers + // ------------------------------------------------------------------------ + + private static Contentlet newContentlet() { + return new ContentletDataGen(contentType.id()).languageId(defaultLanguage.getId()) + .nextPersisted(); + } + + private static User adminUser() throws Exception { + return APILocator.getUserAPI().loadByUserByEmail(ADMIN_EMAIL, systemUser, false); + } + + private static User newLimitedUser(final String password) throws Exception { + final Role role = TestUserUtils.getBackendRole(); + + return new UserDataGen().password(password) + .roles(role, TestUserUtils.getFrontendRole()).nextPersisted(); + } + + private static void grantEdit(final Contentlet contentlet, final User user) throws Exception { + final List permissions = new ArrayList<>(); + permissions.add(new Permission(contentlet.getPermissionId(), + APILocator.getRoleAPI().getUserRole(user).getId(), + PermissionAPI.PERMISSION_READ | PermissionAPI.PERMISSION_EDIT, true)); + permissionAPI.save(permissions, contentlet, systemUser, false); + } + + /** + * The user id currently holding the lock, or {@code null} when the contentlet is unlocked. + */ + private static String lockedBy(final Contentlet contentlet) throws Exception { + return APILocator.getVersionableAPI() + .getContentletVersionInfo(contentlet.getIdentifier(), contentlet.getLanguageId()) + .map(versionInfo -> versionInfo.getLockedBy()) + .orElse(null); + } + + private static Response fireDefaultAction(final HttpServletRequest request, + final SystemAction systemAction, final Contentlet contentlet) { + + return workflowResource.fireActionDefaultSinglePart(request, new EmptyHttpResponse(), + contentlet.getInode(), null, "WAIT_FOR", + String.valueOf(contentlet.getLanguageId()), "DEFAULT", systemAction, null); + } + + /** + * Fires the multi-contentlet variant and returns the streamed {@code summary} node, which is + * where the per-item success/failure counts live. + */ + private static JsonNode fireMultiple(final HttpServletRequest request, + final SystemAction systemAction, final List contentlets) throws Exception { + + final List> contentletForms = new ArrayList<>(); + for (final Contentlet contentlet : contentlets) { + contentletForms.add(Map.of("inode", contentlet.getInode())); + } + + final FireMultipleActionForm form = new FireMultipleActionForm.Builder() + .contentlets(contentletForms).build(); + + final Response response = workflowResource.fireMultipleActionDefault(request, + new EmptyHttpResponse(), systemAction, form); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + + final ByteArrayOutputStream output = new ByteArrayOutputStream(); + StreamingOutput.class.cast(response.getEntity()).write(output); + + final JsonNode entity = new ObjectMapper().readTree(output.toByteArray()).get("entity"); + assertTrue("The response must carry a summary", entity.has("summary")); + + return entity.get("summary"); + } + + private static HttpServletRequest adminRequest() { + return requestForUser(ADMIN_EMAIL, ADMIN_PASSWORD); + } + + private static HttpServletRequest requestForUser(final String email, final String password) { + final MockHeaderRequest request = new MockHeaderRequest(new MockSessionRequest( + new MockAttributeRequest( + new MockHttpRequestIntegrationTest("localhost", "/").request()).request()) + .request()); + + request.setHeader("Authorization", "Basic " + Base64.getEncoder() + .encodeToString((email + ":" + password).getBytes())); + + return request; + } +} From 5aad4e9e27876bd797fef96e37718b3f6ab44c35 Mon Sep 17 00:00:00 2001 From: rjvelazco Date: Thu, 6 Aug 2026 13:36:35 -0400 Subject: [PATCH 2/5] Implement bulk Lock and Unlock quick actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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) --- .../dot-workflow-actions-fire.service.ts | 18 +++-- .../src/lib/dot-action-bulk-result.model.ts | 22 ++++++ .../src/lib/dot-contentlet.model.ts | 9 +++ ...content-drive-action-center.component.html | 17 +++++ ...t-content-drive-action-center.component.ts | 45 +++++++---- .../src/lib/utils/action-center.spec.ts | 19 ++--- .../portlet/src/lib/utils/action-center.ts | 66 ++++++++++++++--- .../portlet/src/lib/utils/workflow-actions.ts | 4 +- .../SystemActionApiFireCommandFactory.java | 74 +++++++++++++++++++ .../api/v1/workflow/WorkflowResource.java | 20 +++-- .../workflows/business/WorkflowAPI.java | 12 ++- .../WEB-INF/messages/Language.properties | 2 + .../main/webapp/WEB-INF/openapi/openapi.yaml | 12 +++ ...flowResourceLockUnlockIntegrationTest.java | 72 ++++++++++++++---- 14 files changed, 334 insertions(+), 58 deletions(-) diff --git a/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.ts b/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.ts index deae5161f80e..f97839c27edf 100644 --- a/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.ts +++ b/core-web/libs/data-access/src/lib/dot-workflow-actions-fire/dot-workflow-actions-fire.service.ts @@ -8,7 +8,8 @@ import { map, take } from 'rxjs/operators'; import { DotActionBulkRequestOptions, DotCMSContentlet, - DotActionBulkResult + DotActionBulkResult, + DotFireDefaultActionResult } from '@dotcms/dotcms-models'; export interface DotActionRequestOptions { @@ -81,13 +82,20 @@ export class DotWorkflowActionsFireService { } /** - * Fire a default workflow action over one or multiple contentlets + * Fire a default workflow action over one or multiple contentlets. + * + * Resolves to the endpoint's `{ results, summary }` entity — **not** a contentlet list, which is + * what this used to claim. Individual contentlets can fail while the request itself succeeds + * with a 200, so callers reporting an outcome must read `summary.successCount` / + * `summary.failCount` rather than assume every inode they sent was acted on. * * @param {DotFireDefaultActionOptions} options - * @return {*} {Observable} + * @return {*} {Observable} * @memberof DotWorkflowActionsFireService */ - fireDefaultAction(options: DotFireDefaultActionOptions): Observable { + fireDefaultAction( + options: DotFireDefaultActionOptions + ): Observable { const { action, inodes } = options; const url = `${this.BASE_URL}/actions/default/fire/${action}`; const urlParams = new HttpParams().set('indexPolicy', 'WAIT_FOR'); @@ -96,7 +104,7 @@ export class DotWorkflowActionsFireService { }; return this.httpClient - .post<{ entity: DotCMSContentlet[] }>(url, body, { + .post<{ entity: DotFireDefaultActionResult }>(url, body, { headers: this.defaultHeaders, params: urlParams }) diff --git a/core-web/libs/dotcms-models/src/lib/dot-action-bulk-result.model.ts b/core-web/libs/dotcms-models/src/lib/dot-action-bulk-result.model.ts index 2881d4cc6d05..27cf21606dda 100644 --- a/core-web/libs/dotcms-models/src/lib/dot-action-bulk-result.model.ts +++ b/core-web/libs/dotcms-models/src/lib/dot-action-bulk-result.model.ts @@ -11,6 +11,28 @@ export interface DotActionBulkResult { action?: string; } +/** + * Response of `POST /api/v1/workflow/actions/default/fire/{systemAction}`, the multi-contentlet + * system-action endpoint. + * + * Streamed rather than assembled, which is why the shape differs from {@link DotActionBulkResult}: + * one entry per contentlet keyed by its id, then a summary. A per-item failure does **not** fail the + * request — the status stays 200 and the item is counted in {@link summary.failCount} — so the + * summary is the only honest source of what actually happened. + */ +export interface DotFireDefaultActionResult { + /** One entry per contentlet, keyed by identifier. A failed item holds an error payload. */ + results: Record[]; + summary: { + /** Number of contentlets the request was asked to act on. */ + affected: number; + successCount: number; + failCount: number; + /** Server-side duration in ms. */ + time: number; + }; +} + // optional attrs because api is not consistent export interface DotBulkFailItem { errorMessage: string; diff --git a/core-web/libs/dotcms-models/src/lib/dot-contentlet.model.ts b/core-web/libs/dotcms-models/src/lib/dot-contentlet.model.ts index 69b8a993aefc..2f3b3ce1041f 100644 --- a/core-web/libs/dotcms-models/src/lib/dot-contentlet.model.ts +++ b/core-web/libs/dotcms-models/src/lib/dot-contentlet.model.ts @@ -11,6 +11,15 @@ export interface DotCMSContentlet { binaryContentAsset?: string; binaryVersion?: string; contentType: string; + /** + * Whether the current user may edit the contentlet *right now* — stamped by the browser/drive + * search, not by the contentlet itself. + * + * True only when the caller has WRITE permission **and** holds the lock, so on a `locked` row a + * `false` means the lock belongs to somebody else. That makes it the cheapest available answer + * to "can I unlock this?", short of a per-item `_canlock` call. + */ + contentEditable?: boolean; file?: string; folder: string; hasLiveVersion?: boolean; diff --git a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.html b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.html index fab326ba6b1c..8e4ca0797582 100644 --- a/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.html +++ b/core-web/libs/portlets/dot-content-drive/portlet/src/lib/components/dialogs/dot-content-drive-action-center/dot-content-drive-action-center.component.html @@ -85,6 +85,23 @@