Skip to content

feat: edit own task comments (3-dots menu, inline editor, updated timestamp) - EXO-88900 - #633

Merged
Julien-Dubois-eXo merged 5 commits into
feature/ai-contributionfrom
feat/task-comment-edition-EXO-88900
Jul 30, 2026
Merged

feat: edit own task comments (3-dots menu, inline editor, updated timestamp) - EXO-88900#633
Julien-Dubois-eXo merged 5 commits into
feature/ai-contributionfrom
feat/task-comment-edition-EXO-88900

Conversation

@Julien-Dubois-eXo

@Julien-Dubois-eXo Julien-Dubois-eXo commented Jul 28, 2026

Copy link
Copy Markdown

Covered scenario

Step Behaviour
Hovering over a comment I authored A 3-dots button instead of the trash one
Clicking the 3-dots button An action menu with Delete and Edit
Edit The editor opens inline, prefilled, with Update and Cancel buttons
Update Saves the modification and the timestamp becomes "Updated "
Cancel Exits the edition without saving

It works on the replies (sub-comments) as well. A single editor can be opened at a time: starting an edition closes any other opened edition or reply editor, and the other way round.

Backend

  • Liquibase task.db.changelog-7.3.0.xml: new TASK_COMMENTS.UPDATED_TIME column (nullable, left NULL for the comments which were never edited), declared in configuration.xml
  • updatedTime carried along Comment -> CommentDto -> StorageUtil -> CommentEntity
  • updateComment(commentId, commentText, identity) added to CommentStorage then CommentService (Storage/Service/REST layering respected): it rewrites the content, stamps the update time and recomputes the mentions
  • PUT /rest/tasks/comments/{commentId}, with the same URL decoding as the comment creation. The endpoint only maps the service exceptions to the HTTP statuses: EntityNotFoundException -> 404, NotAllowedOperationOnEntityException -> 403
  • The authorization is centralized in CommentServiceImpl through TaskUtil.canEditComment: only the author can edit a comment, whereas canDeleteComment also allows the project managers
  • New exo.task.taskCommentUpdate event, handled by TaskCommentContentLinkListener to reprocess the content links of the edited text. The notification, gamification and analytics listeners are deliberately left out: an edition must neither notify again nor reward again

Frontend

  • TaskCommentItem.vue: v-menu action menu (following the TaskDrawer.vue pattern: attach, offset-y, right aligned through :left="!$vuetify.rtl"), icons in icon-default-color, local edition state, mutual exclusion through the root event bus
  • TaskCommentEditor.vue: edit-mode — CKEditor prefilled with the raw comment text, object-id set to the comment so that its attachments are loaded, Update/Cancel buttons aligned on the composer left edge, no duplicated avatar. In edition mode the editor ignores the reply global events so that it does not close itself
  • Updated <relative time> timestamp in the comments list and in the last comment preview
  • 4 i18n keys added to the _en bundle only (Crowdin handles the others)

Tests

  • TestTaskRestService.testUpdateComment: empty content (400), unknown comment (404), comment of another author (403), successful update (200 with the update time), URL encoded content and service failure (500)
  • CommentServiceTest: content and update time, EntityNotFoundException on an unknown comment, NotAllowedOperationOnEntityException when the user is not the author, CommentDto.clone() carrying the update time
  • CommentStorageTest.testUpdateComment: update time stamping, mentions recomputed from the new content, EntityNotFoundException on an unknown id
  • 32 tests pass, npm run build + eslint report no error
  • Manually validated on exoplatform/exo-community:7.3.x-devx-SNAPSHOT: the 7.3.0 changelog applies without error on an already populated database

Review notes

  • Closing a concurrent edition discards the text being typed without any confirmation (requested behaviour) — an exo-confirm-dialog guard could be added if wanted
  • No update_task_comment MCP tool was exposed, the need being UI only

🤖 Generated with Claude Code

On the comments a user authored, the hover trash button is replaced by a
3-dots action menu offering Edit and Delete. Edit opens the comment editor
inline, prefilled with the comment content, with Update and Cancel actions.
Only one editor can be opened at a time: starting an edition closes any
other opened edition or reply editor, and vice versa.

An edition stamps a new TASK_COMMENTS.UPDATED_TIME column (Liquibase
changelog 7.3.0) so the comment timestamp is rendered as "Updated <relative
time>", in the comments list as well as in the last comment preview.

Backend: PUT /rest/tasks/comments/{commentId} + updateComment() down the
Storage and Service layers. Only the author can edit a comment, whereas the
deletion stays allowed to the project managers as well. The new
exo.task.taskCommentUpdate event is broadcast and handled by the content
link listener to reprocess the links of the edited content; the
notification, gamification and analytics listeners are deliberately left
out so that an edition does not notify nor reward again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Julien-Dubois-eXo
Julien-Dubois-eXo requested a review from ahamdi July 28, 2026 14:47
Julien-Dubois-eXo and others added 2 commits July 29, 2026 13:45
- use String.replace() for the literal '+' escaping instead of replaceAll()
  (java:S5361)
- optional chaining on the update comment response (javascript:S6582)
- declare the emits option on the 3 touched components (javascript:S8961),
  listing every emitted event and not only the reported ones

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…EXO-88900

New code coverage was at 35% for a 60% threshold, the REST endpoint being
the biggest uncovered part (22 lines).

- TestTaskRestService.testUpdateComment: unknown comment (404), comment of
  another author (403), empty content (400), successful update (200 with the
  update time), URL encoded content and service failure (500)
- CommentStorageTest.testUpdateComment: update time stamping, mentions
  recomputed from the new content, EntityNotFoundException on unknown id
- CommentServiceTest.testCloneCommentKeepsUpdateTime: CommentDto.clone()
  carries the update time

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@ahamdi ahamdi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the PR description and use English instead of french. Thanks

}

@Override
public CommentDto updateComment(long commentId, String commentText) throws EntityNotFoundException {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the check of the permission of who can edit a comment here instead of the Rest service to centralize the check at the service level

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 1f9b274: the permission check now lives in CommentServiceImpl.updateComment(), so every caller of the service goes through it. Details, including a follow-up NPE fix, in my comment below.

Julien-Dubois-eXo and others added 2 commits July 30, 2026 09:57
…XO-88900

Following the review: the check of who is allowed to edit a comment moves
from the REST layer to CommentServiceImpl, so that every caller of the
service goes through it.

updateComment() now takes the identity of the user attempting the edition
and raises NotAllowedOperationOnEntityException when he is not the author,
the same exception already used by the status and user services. The
endpoint no longer loads the comment beforehand nor checks the permission:
it only maps EntityNotFoundException to 404 and
NotAllowedOperationOnEntityException to 403.

The permission test moves accordingly from TestTaskRestService to
CommentServiceTest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SonarCloud reported a possible NullPointerException (javabugs:S2259) on the
update endpoint: CommentStorageImpl.updateComment() returns the result of
StorageUtil.commentToDto(), which is null when the DAO update returns null,
and the endpoint dereferenced it right away. Since the refactoring, the
endpoint no longer loads the comment beforehand, which made that path
reachable.

The endpoint now answers a server error instead of failing with a NPE, and
the case is covered by a test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

@Julien-Dubois-eXo

Copy link
Copy Markdown
Author

Thanks for the review @ahamdi. Both points are addressed, plus a follow-up fix for a quality gate failure they caused.

1. Permission check centralized at the service level (1f9b274)

TaskUtil.canEditComment() is now called in CommentServiceImpl.updateComment() instead of the REST layer, so every caller of the service goes through the check:

if (!TaskUtil.canEditComment(identity, comment)) {
  throw new NotAllowedOperationOnEntityException(commentId, CommentDto.class, "edit");
}

Three choices worth pointing out:

  • The identity is passed as a parameter (updateComment(commentId, commentText, Identity identity)) rather than read from ConversationState inside the service. This follows what the rest of the module does (LabelService.findLabelsByProject(..., Identity currentUser, ...), UserService.hideProject(Identity, ...)) and keeps the service testable without any ambient state.
  • NotAllowedOperationOnEntityException is used, as StatusServiceImpl and UserService already do. I did not use UnAuthorizedOperationException, which sits in the same package but is referenced nowhere.
  • The endpoint no longer loads the comment beforehand: its getComment() call plus null check duplicated what the service already does. It now only maps the service exceptions, EntityNotFoundException to 404 and NotAllowedOperationOnEntityException to 403.

The permission test moved along with the logic, from TestTaskRestService to CommentServiceTest.testUpdateCommentOfAnotherAuthor().

2. PR description translated to English

Done, same structure as before.

3. NullPointerException reported by SonarCloud after the refactoring (53e6068)

The refactoring above turned the quality gate red on new_reliability_rating with one bug, javabugs:S2259 on the endpoint. The analyzer was right: CommentStorageImpl.updateComment() returns StorageUtil.commentToDto(...), which is null when the DAO update() returns null, and the endpoint dereferenced the result immediately. That path became reachable precisely because the endpoint stopped loading the comment beforehand.

The endpoint now answers a server error rather than throwing, and the case is covered by a test:

if (updatedComment == null) {
  LOG.error("Comment {} could not be updated", commentId);
  return Response.serverError().build();
}

A server error rather than a 404 on purpose: the service already raises EntityNotFoundException when the comment does not exist, so a null here is an internal anomaly, not a missing resource.

Status

Quality gate is back to passing: 0 issue, reliability and maintainability ratings A, 94.9% coverage on new code. 32 backend tests pass, npm run build and eslint report no error.

@Julien-Dubois-eXo
Julien-Dubois-eXo requested a review from ahamdi July 30, 2026 08:13
@Julien-Dubois-eXo

Julien-Dubois-eXo commented Jul 30, 2026

Copy link
Copy Markdown
Author

@ahamdi one open point I deliberately left out of this PR. Could you tell me whether you want them handled here, in a follow-up, or not at all?

1. replaceAll() on a literal in the two other comment endpoints

The java:S5361 issue SonarCloud raised on my endpoint (replaceAll("\+", "%2b") where a plain replace() does the job) exists identically in addTaskComment() and addTaskSubComment():

commentText = commentText.replaceAll("\+", "%2b");

They are not reported because those lines are outside the diff. Fixing them is a two-line change with no behaviour difference — the regex \+ and the literal + match exactly the same thing — but it widens the diff beyond the scope of EXO-88900. Happy to include it here if you prefer having it consistent across the three endpoints, otherwise it can be a small separate cleanup.

@ahamdi ahamdi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@ahamdi

ahamdi commented Jul 30, 2026

Copy link
Copy Markdown
Member

@ahamdi one open point I deliberately left out of this PR. Could you tell me whether you want them handled here, in a follow-up, or not at all?

1. replaceAll() on a literal in the two other comment endpoints

The java:S5361 issue SonarCloud raised on my endpoint (replaceAll("\+", "%2b") where a plain replace() does the job) exists identically in addTaskComment() and addTaskSubComment():

commentText = commentText.replaceAll("\+", "%2b");

They are not reported because those lines are outside the diff. Fixing them is a two-line change with no behaviour difference — the regex \+ and the literal + match exactly the same thing — but it widens the diff beyond the scope of EXO-88900. Happy to include it here if you prefer having it consistent across the three endpoints, otherwise it can be a small separate cleanup.

Non blocker for me, we can proceed with the PR as is

@Julien-Dubois-eXo
Julien-Dubois-eXo merged commit 081f488 into feature/ai-contribution Jul 30, 2026
11 checks passed
@Julien-Dubois-eXo
Julien-Dubois-eXo deleted the feat/task-comment-edition-EXO-88900 branch July 30, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants