Conversation
…e` responses Updated the `ContentResource` class to return hydrated contentlets in API responses instead of raw contentlets to ensure data consistency. Ref: #34541
Updated the `lockedBy` type to `string` and added `lockedByName` field. Simplified lock user display to use `lockedByName` directly. Ref: #34541
Adjusted tests to use updated `lockedBy` structure (`string` and `lockedByName`) for consistency with recent model changes. Ref: #34541
|
@claude generate a PR description |
|
Claude finished @erickgonzalez's task —— View job PR Description GeneratedTodo List:
SummaryThis PR fixes issue #34541 by resolving inconsistencies in the content lock system where Changes MadeFrontend (TypeScript/Angular)
Backend (Java)
Technical DetailsThe issue was caused by inconsistent data structures for the
Root Cause: Backend lock/unlock endpoints were returning non-hydrated contentlets, causing incomplete user data in the Solution:
Testing
|
There was a problem hiding this comment.
Pull request overview
Fixes inconsistencies in content locking data returned/consumed by the New Edit Content flow (Issue #34541), aiming to prevent “undefined” lock owner display and suppress the lock banner when the current user owns the lock.
Changes:
- Backend: lock/unlock endpoints now return a hydrated contentlet map.
- Frontend:
DotCMSContentlet.lockedBymodel changed to astringuserId and introducedlockedByName. - Frontend: lock banner logic and related unit tests updated to the new lock field shape.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| dotCMS/src/main/java/com/dotcms/rest/api/v1/content/ContentResource.java | Returns hydrated contentlets for lock/unlock responses. |
| core-web/libs/dotcms-models/src/lib/dot-contentlet.model.ts | Changes the lockedBy type and adds lockedByName. |
| core-web/libs/edit-content/src/lib/store/features/lock/lock.feature.ts | Updates banner logic to use lockedBy (string) and lockedByName. |
| core-web/libs/edit-content/src/lib/store/features/lock/lock.feature.spec.ts | Updates lock feature tests to the new lock shape. |
| core-web/libs/edit-content/src/lib/utils/functions.util.spec.ts | Updates copy-prep test fixture to include the new lock fields. |
| const { lockedBy, lockedByName } = contentlet; | ||
|
|
||
| const isLockedByCurrentUser = currentUser?.userId === lockedBy?.userId; | ||
| const isLockedByCurrentUser = currentUser?.userId === lockedBy; | ||
|
|
||
| // content is not locked or locked by the current user | ||
| if (!lockedBy || isLockedByCurrentUser) { | ||
| return null; | ||
| } | ||
|
|
||
| const userDisplay = lockedBy.firstName + ' ' + lockedBy.lastName; | ||
| const userDisplay = lockedByName; |
There was a problem hiding this comment.
lockedBy is now treated as a string userId, but the backend contentlet transformation used by ContentResource/WorkflowHelper currently serializes lockedBy as an object (it sets a map with { userId, firstName, lastName }). At runtime this makes isLockedByCurrentUser always false and lockedByName is likely undefined, so the banner can regress back to showing for the current user / showing "undefined".
To fix: either (a) keep supporting the existing lockedBy object shape here (and derive the display name from it), or (b) change the backend response for contentlets to return lockedBy as a string plus a lockedByName string consistently (including lock/unlock + get content endpoints).
| live: boolean; | ||
| locked: boolean; | ||
| lockedBy?: DotContentletLockUser; | ||
| lockedBy?: string; |
There was a problem hiding this comment.
Changing DotCMSContentlet.lockedBy to string doesn’t match the current backend JSON shape for contentlets, which is serialized as an object with { userId, firstName, lastName } under lockedBy (via the default contentlet transform strategy). This type change can mask runtime contract issues and will break code comparing lockedBy to currentUser.userId.
Suggested fix: make lockedBy a union type that matches the API (string | DotContentletLockUser) until the backend is updated everywhere, or keep it as DotContentletLockUser and only add lockedByName once the backend reliably returns it for contentlets.
| lockedBy?: string; | |
| lockedBy?: string | DotContentletLockUser; |
| final Contentlet contentletHydrated = new DotTransformerBuilder().contentResourceOptions(false) | ||
| .content(contentlet).build().hydrate().get(0); | ||
| return new ResponseEntityMapView(WorkflowHelper.getInstance().contentletToMap(contentlet)); | ||
| return new ResponseEntityMapView(WorkflowHelper.getInstance().contentletToMap(contentletHydrated)); |
There was a problem hiding this comment.
This lock/unlock endpoint returns WorkflowHelper.contentletToMap(...), which (via the default contentlet transform strategy) serializes lockedBy as a user object map, and does not populate lockedByName for non-page contentlets. Given the frontend changes in this PR, the response contract likely still won’t match what the UI expects.
Suggested fix: either adapt the backend mapping used here to return lockedBy as a string userId plus lockedByName as a full name string, or revert the frontend to consume the existing lockedBy object shape.
| return new ResponseEntityMapView(WorkflowHelper.getInstance().contentletToMap(contentletHydrated)); | |
| @SuppressWarnings("unchecked") | |
| final Map<String, Object> contentMap = | |
| (Map<String, Object>) WorkflowHelper.getInstance().contentletToMap(contentletHydrated); | |
| // Normalize lockedBy to a userId string and populate lockedByName (full name) | |
| Object lockedByObj = contentMap.get("lockedBy"); | |
| String lockedById = null; | |
| String lockedByName = null; | |
| if (lockedByObj instanceof Map) { | |
| Map<?, ?> lockedByMap = (Map<?, ?>) lockedByObj; | |
| Object idObj = lockedByMap.get("userId"); | |
| if (idObj instanceof String) { | |
| lockedById = (String) idObj; | |
| } | |
| Object nameObj = lockedByMap.get("fullName"); | |
| if (nameObj instanceof String) { | |
| lockedByName = (String) nameObj; | |
| } | |
| } else if (lockedByObj instanceof String) { | |
| lockedById = (String) lockedByObj; | |
| } | |
| if (lockedById != null && (lockedByName == null || lockedByName.isEmpty())) { | |
| try { | |
| User lockedByUser = APILocator.getUserAPI().loadUserById(lockedById, false); | |
| if (lockedByUser != null) { | |
| lockedByName = lockedByUser.getFullName(); | |
| } | |
| } catch (Exception e) { | |
| Logger.warn(this, "Unable to resolve lockedBy user name for userId: " + lockedById, e); | |
| } | |
| } | |
| if (lockedById != null) { | |
| contentMap.put("lockedBy", lockedById); | |
| } | |
| if (lockedByName != null && !lockedByName.isEmpty()) { | |
| contentMap.put("lockedByName", lockedByName); | |
| } | |
| return new ResponseEntityMapView(contentMap); |
| final Contentlet contentletHydrated = new DotTransformerBuilder().contentResourceOptions(false) | ||
| .content(contentlet).build().hydrate().get(0); | ||
| return new ResponseEntityMapView(WorkflowHelper.getInstance().contentletToMap(contentlet)); | ||
| return new ResponseEntityMapView(WorkflowHelper.getInstance().contentletToMap(contentletHydrated)); |
There was a problem hiding this comment.
Same as unlock: this lock endpoint still uses WorkflowHelper.contentletToMap(...) which produces lockedBy as an object map and typically won’t include a lockedByName string for contentlets. With the updated frontend model/logic, the API response here likely remains inconsistent.
Suggested fix: align the serialized response shape (lockedBy userId string + lockedByName string) or keep the frontend consuming the existing object shape.
| return new ResponseEntityMapView(WorkflowHelper.getInstance().contentletToMap(contentletHydrated)); | |
| final Map<String, Object> contentletMap = | |
| WorkflowHelper.getInstance().contentletToMap(contentletHydrated); | |
| // Normalize lock information to match updated frontend expectations: | |
| // - lockedBy: userId string | |
| // - lockedByName: user display name string | |
| contentletMap.put("lockedBy", user.getUserId()); | |
| contentletMap.put("lockedByName", user.getFullName()); | |
| return new ResponseEntityMapView(contentletMap); |
| const contentlet = createFakeContentlet({ | ||
| locked: true, | ||
| lockedBy: { | ||
| firstName: 'John', | ||
| lastName: 'Doe', | ||
| userId: 'user123' | ||
| } | ||
| lockedBy: 'user123', | ||
| lockedByName: 'John Doe' | ||
| }); |
There was a problem hiding this comment.
This test now models the lock owner as lockedByName, but prepareContentletForCopy only removes lockedBy. With the new split fields, the copy-prep utility should probably also clear lockedByName (otherwise the copied contentlet can retain stale lock-owner display data).
Suggested fix: update prepareContentletForCopy to unset both lockedBy and lockedByName, and adjust this expectation accordingly.
| store.updateContent({ | ||
| locked: true, | ||
| lockedBy: { userId: '123', firstName: 'John', lastName: 'Doe' } | ||
| lockedBy: '123', | ||
| lockedByName: 'John Doe' | ||
| } as DotCMSContentlet); |
There was a problem hiding this comment.
The updated test data assumes the production DotCMSContentlet contract is lockedBy: string + lockedByName, but the backend currently serializes lockedBy as an object for contentlets. This can let the unit tests pass while the feature still fails at runtime.
Suggested fix: update the fixtures to match the actual response shape (or update the backend response shape in the same PR and add backend coverage validating it).
Summary
This PR fixes issue #34541 by resolving inconsistencies in the content lock system where
lockedBydata was undefined or incorrectly structured.Changes Made
Frontend (TypeScript/Angular)
DotCMSContentletmodel (dot-contentlet.model.ts:27-28): ChangedlockedByfrom complexDotContentletLockUserobject to simplestringand added separatelockedByNamefieldlock.feature.ts:58-67): Simplified user comparison logic and user display using the newlockedByNamefield directlylockedBystructure with separatelockedBy(string) andlockedByName(string) fieldsBackend (Java)
ContentResource.java:647,705): Ensured lock/unlock endpoints return hydrated contentlets instead of raw ones, preventing undefinedlockedBydata in API responsesTechnical Details
The issue was caused by inconsistent data structures for the
lockedByfield:lockedByas a user object with{userId, firstName, lastName}undefinedvalues when accessing lock user informationRoot Cause: Backend lock/unlock endpoints were returning non-hydrated contentlets, causing incomplete user data in the
lockedByfield.Solution:
lockedByfor user ID,lockedByNamefor display name)Testing
lockedBystructureThis PR fixes: #34541