Communication: Mark messages as unread
#12079
Conversation
github-actions
bot
added
tests
server
|
@ayca-cevdet Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
End-to-End (E2E) Test Results SummaryTest Strategy: Running all tests Status: No tests needed to run
|
||||||||||||||||||
|
@ayca-cevdet Test coverage could not be fully measured because some tests failed. Please check the workflow logs for details. |
End-to-End (E2E) Test Results SummaryTest Strategy: Running all tests Status: No tests needed to run
|
||||||||||||||||||
|
@ayca-cevdet Test coverage has been automatically updated in the PR description. |
helios-aet
bot
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
mariyakoeva
left a comment
mariyakoeva
left a comment
There was a problem hiding this comment.
Deployed to TS1. Works as described, but I find "Mark unread" without "Mark read" confusing(either to the channel, or the message)
@mariyakoeva Marking message as read is not manageable. Marking the channel as read makes sense, however when user opens the channel it automatically means the channel and messages are now "read". Introducing marking the channel as read would require two clicks from the user, assuming "Mark as Read" option is added into the three dot menu on the channel. So instead of that, user can just open the channel with one click and that is it. Additionally, user also has the option to mark all channels as read. |
ayca-cevdet
dismissed stale reviews from mariyakoeva and jerrycai0006
via
895d52e
|
@ayca-cevdet Test coverage has been automatically updated in the PR description. |
WalkthroughAdds an end-to-end "mark as unread" feature: backend repository update and service/endpoint, frontend API, Metis services and UI wiring, shared unread-post utility, i18n keys, and unit/integration tests. Changes
Sequence DiagramsequenceDiagram
participant User
participant PostingReactionsBar
participant MetisService
participant MetisConversationService
participant ConversationAPI as ConversationResource (Backend)
participant ConversationServiceBE as ConversationService (Backend)
participant Repo as ConversationParticipantRepository
User->>PostingReactionsBar: click "Mark as unread"
PostingReactionsBar->>MetisService: markMessageAsUnread(post)
MetisService->>ConversationAPI: PATCH /.../mark-as-unread
ConversationAPI->>ConversationServiceBE: markAsUnread(conversationId,userId,messageId)
ConversationServiceBE->>Repo: markFromMessageAsUnread(conversationId,userId,messageDate,lastRead)
Repo-->>ConversationServiceBE: update result
ConversationServiceBE-->>ConversationAPI: 200 OK
ConversationAPI-->>MetisService: response
MetisService->>MetisConversationService: updateLastReadDateAndNumberOfUnreadMessages(...)
MetisConversationService-->>MetisService: emit updated conversations$
MetisService-->>PostingReactionsBar: observable triggers UI update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In
`@src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java`:
- Around line 244-249: In markAsUnread, validate server-side that the loaded
Post actually belongs to the conversation and that the current user is a
participant and not the post author before calling
conversationParticipantRepository.markFromMessageAsUnread: after Post post =
postRepository.findByIdElseThrow(messageId) assert
post.getConversationId().equals(conversationId) (throw
IllegalArgumentException/NotFound if not), assert
!post.getAuthorId().equals(userId) (throw AccessDeniedException if user is
author), and verify conversation membership (e.g.
conversationParticipantRepository.existsByConversationIdAndUserId or via
conversationRepository) before computing messageDate/lastRead and invoking
markFromMessageAsUnread.
In
`@src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ConversationResource.java`:
- Around line 189-208: Add a validation in ConversationService.markAsUnread to
ensure the Post fetched by messageId actually belongs to the provided
conversationId: after retrieving the Post (entity with getConversation()), check
post.getConversation().getId().equals(conversationId) and if not throw a
BadRequestException (with a clear message). This check should run before any
extraction of the message date or modifying unread state so cross-conversation
message references are rejected; update ConversationService.markAsUnread (and
any helper methods it calls) to perform this validation.
In
`@src/main/webapp/app/communication/service/metis-conversation.service.spec.ts`:
- Around line 507-516: The test name says isMarkedAsUnread should be true but
the test sets it false and the conversation isn't in the cache; update the test
so (metisConversationService as any).isMarkedAsUnread = true and ensure the
activeConversation (groupChat) is present in the mock cache backing
(metisConversationService as any)._conversationsOfUser$ (e.g., include groupChat
in its current value or mock the observable to contain it) before calling
(metisConversationService as any).updateConversationAsRead(), then assert that
the _conversationsOfUser$.next spy is not called; reference activeConversation,
isMarkedAsUnread, _conversationsOfUser$, updateConversationAsRead, and groupChat
when making the change.
In `@src/main/webapp/app/communication/service/metis-conversation.service.ts`:
- Around line 169-183: The method updateLastReadDateAndNumberOfUnreadMessages
currently forces hasUnreadMessage true regardless of unreadMessagesCount and
never updates the global hasUnreadMessages; change it to set
activeConversation.hasUnreadMessage and the conversationsOfUser[...]
.hasUnreadMessage to true only when unreadMessagesCount > 0 (set false when
count is 0 or undefined), and after updating the conversation entries call or
implement a global recompute (e.g., a helper or existing method) to refresh the
component-level flag hasUnreadMessages by scanning conversationsOfUser (or
include activeConversation) so the global unread indicator stays in sync with
per-conversation counts.
In
`@src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java`:
- Around line 722-747: The test testMarkMessageAsUnread currently may pass
without exercising the PATCH because unread count is already 2; fix it by
establishing a baseline: after creating createdPost1 and createdPost2, simulate
reading them (e.g., call the same API to mark the message(s) as read or invoke
the service method used to mark read) so
getUnreadMessagesCount(createdPost1.getConversation(), student2) becomes 0,
assert that baseline, then call request.patch(..."/mark-as-unread"...) for the
target messageId and finally assert that getUnreadMessagesCount(...) increments
to the expected value (e.g., 1 or 2) to prove the state change. Use the test
method testMarkMessageAsUnread, variables createdPost1/createdPost2,
request.patch endpoint and getUnreadMessagesCount to locate where to add the
baseline mark-as-read step and assertions.
🧹 Nitpick comments (2)
src/main/webapp/app/communication/service/metis.service.ts (1)
677-688: Scope unread-count computation to the conversation.If
cachedPostscontains multiple conversations (e.g., course-wide feed), the unread count can be inflated. Filtering byconversationIdis low-cost and safer.🔧 Suggested adjustment
next: () => { const lastReadDate = post.creationDate!.subtract(1, 'millisecond'); - const unreadMessagesCount = getUnreadPostsByLastReadDate(this.user, this.cachedPosts, lastReadDate!).length; - this.metisConversationService.updateLastReadDateAndNumberOfUnreadMessages(post.conversation!.id, lastReadDate, unreadMessagesCount); + const conversationId = post.conversation!.id!; + const conversationPosts = this.cachedPosts.filter((cachedPost) => cachedPost.conversation?.id === conversationId); + const unreadMessagesCount = getUnreadPostsByLastReadDate(this.user, conversationPosts, lastReadDate).length; + this.metisConversationService.updateLastReadDateAndNumberOfUnreadMessages(conversationId, lastReadDate, unreadMessagesCount); this.posts$.next(this.cachedPosts); }, });src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.ts (1)
185-193: Consider callingsetCanMarkAsUnread()inngOnChanges.
setMayDelete()andsetMayEdit()are called in bothngOnInitandngOnChanges, butsetCanMarkAsUnread()is only called inngOnInit. If the posting input changes, thecanMarkAsUnreadflag won't be recalculated, which could lead to stale state.Proposed fix
ngOnChanges() { this.updatePostingWithReactions(); this.isAtLeastTutorInCourse = this.metisService.metisUserIsAtLeastTutorInCourse(); if (this.getPostingType() === 'post') { this.resetTooltipsAndPriority(); } this.setMayDelete(); this.setMayEdit(); + this.setCanMarkAsUnread(); }
...main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ConversationResource.java
Show resolved
Hide resolved
src/main/webapp/app/communication/service/metis-conversation.service.spec.ts
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/communication/MessageIntegrationTest.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
End-to-End (E2E) Test Results SummaryTest Strategy: Running all tests Status: No tests needed to run
|
||||||||||||||||||
helios-aet
bot
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
NadiaCurumi
left a comment
NadiaCurumi
left a comment
There was a problem hiding this comment.
Tested on TS6. Works as expected.
- The notification indicator is updated correctly.
- The 'New' message indicator appears correctly above the message that is marked as unread.
unreadmsg.mp4
Jess-hub09
left a comment
Jess-hub09
left a comment
There was a problem hiding this comment.
Tested according to the steps above on TS6. Unread counts, notification indicators, and the “New” message marker behave correctly when marking a message as unread and revisiting the channel.
...main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/communication/web/conversation/ConversationResource.java
Show resolved
Hide resolved
src/main/webapp/app/communication/service/metis-conversation.service.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/communication/service/metis-conversation.service.spec.ts
Outdated
Show resolved
Hide resolved
krusche
left a comment
krusche
left a comment
There was a problem hiding this comment.
Code Review Summary
HIGH Risk (2 issues — must fix before merge)
-
Missing message-conversation validation (
ConversationService.java:244): ThemarkAsUnreadmethod does not verify that the providedmessageIdactually belongs to the specifiedconversationId. This allows a user to pass a message from a different conversation to manipulate unread state of an unrelated conversation. -
Missing conversation membership check (
ConversationResource.java:200): The endpoint only checks that the user is a student in the course, but does not verify they are a participant of the target conversation. Any enrolled student can call this on any conversation, including private channels and DMs they are not a member of.
MEDIUM Risk (2 issues)
-
Regression on conversation deselection (
metis-conversation.service.ts:127): WhensetActiveConversation(undefined)is called (deselecting a conversation),updateConversationAsRead()is never invoked because theconversationIdentifieris falsy and theifblock is skipped entirely. The old code marked the active conversation as read unconditionally before this block. -
Test bug — wrong flag value (
metis-conversation.service.spec.ts:509): Test says "if isMarkedAsUnread flag is true" but setsisMarkedAsUnread = false. It passes coincidentally due to an emptyconversationsOfUserarray, not because it tests the intended guard condition.
Nits (2)
-
No error handler in subscribe (
metis.service.ts:681): ThemarkMessageAsUnreadsubscribe has no error callback — network failures or 403s are silently swallowed. -
Behavioral change in unread post filtering (
metis.util.ts:150):getUnreadPostsByLastReadDatenow filters out the user's own posts, changing existing "New" indicator behavior. The change is correct (aligns with server-side logic) but worth noting.
The feature design is solid overall. The authorization gaps in the server-side code (issues 1 and 2) need to be addressed before merge, as they represent security vulnerabilities that could be exploited by any authenticated student in a course.
ayca-cevdet
dismissed stale reviews from Jess-hub09 and NadiaCurumi
via
3421432
|
@ayca-cevdet Test coverage has been automatically updated in the PR description. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/webapp/app/communication/service/metis-conversation.service.ts (1)
186-200:⚠️ Potential issue | 🟠 Major
updateConversationAsReaddoesn't refresh the global unread badge.After zeroing
unreadMessagesCounton the outgoing conversation,hasUnreadMessagesCheck()is not called. If this was the last conversation with unread messages, the globalhasUnreadMessages$indicator remainstrueuntil something else triggers a recheck.Proposed fix
private updateConversationAsRead() { if (this.activeConversation && !this.isMarkedAsUnread) { this.activeConversation.lastReadDate = dayjs(); this.activeConversation.unreadMessagesCount = 0; this.activeConversation.hasUnreadMessage = false; const indexOfConversationToUpdate = this.conversationsOfUser.findIndex((conversation) => conversation.id === this.activeConversation!.id); if (indexOfConversationToUpdate !== -1) { this.conversationsOfUser[indexOfConversationToUpdate].lastReadDate = dayjs(); this.conversationsOfUser[indexOfConversationToUpdate].unreadMessagesCount = 0; this.conversationsOfUser[indexOfConversationToUpdate].hasUnreadMessage = false; this._conversationsOfUser$.next(this.conversationsOfUser); + this.hasUnreadMessagesCheck(); } } }
🤖 Fix all issues with AI agents
In
`@src/main/webapp/app/communication/service/metis-conversation.service.spec.ts`:
- Around line 454-474: The tests are calling the no-arg private helper
updateConversationAsRead (which ignores args) instead of the parameterized API;
change both tests to call
updateLastReadDateAndNumberOfUnreadMessages(conversationId, lastReadDate,
unreadMessagesCount) so the args are actually used. In the first test keep
(metisConversationService as any).conversationsOfUser = [groupChat], spy on
(metisConversationService as any)._conversationsOfUser$.next, call
updateLastReadDateAndNumberOfUnreadMessages(nonExistentConversation.id, dayjs(),
5) and assert next was not called. In the second test set
(metisConversationService as any).activeConversation = groupChat and
conversationsOfUser = [groupChat], call
updateLastReadDateAndNumberOfUnreadMessages(nonExistentConversation.id, dayjs(),
5) and assert activeConversation.unreadMessagesCount and
activeConversation.hasUnreadMessage remain unchanged.
src/main/webapp/app/communication/service/metis-conversation.service.spec.ts
Show resolved
Hide resolved
End-to-End (E2E) Test Results SummaryTest Strategy: Running all tests Status: No tests needed to run Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
@krusche Thanks for pointing out these issues. They are now fixed and ready for review/merge. |
|
@ayca-cevdet Test coverage has been automatically updated in the PR description. |
End-to-End (E2E) Test Results SummaryTest Strategy: Running all tests Status: No tests needed to run Detailed Results: Check the individual phase reports in the workflow run for test counts, timing, and results. |
krusche
left a comment
krusche
left a comment
There was a problem hiding this comment.
PR Review: Communication - Mark Messages as Unread
Summary
The feature implementation is generally solid — the architecture follows existing patterns, the JPQL query is efficient, and the client-side logic is well-structured. The refactoring to extract getUnreadPostsByLastReadDate into a shared utility and the behavior change to exclude user's own posts from unread counts are both correct improvements. However, there are several issues that should be addressed before merging.
Issues by Priority
Medium
- Missing server-side validation: users can mark their own messages as unread — The client correctly hides the button for the author, but the server has no such guard. A direct API call can bypass the client check.
- Missing negative integration tests — No tests for error cases: non-member access, invalid message ID, message not belonging to conversation.
- Unused variable
student1in test —student1is fetched but never used. hasUnreadMessagealways set totrue—updateLastReadDateAndNumberOfUnreadMessagesalways setshasUnreadMessage = trueeven ifunreadMessagesCountis 0.
Low
5. Unrelated bookmark icon fix bundled in this PR — The faBookmark -> farBookmark fix is a separate bug fix.
6. createdPost2 is unused in the test — The variable is assigned but never referenced in assertions.
7. updateLastReadDateAndNumberOfUnreadMessages method name is misleading — Doesn't convey the "mark as unread" intent.
Nitpick
8. Redundant ! on lastReadDate in metis.service.ts
| ZonedDateTime messageDate = post.getCreationDate(); | ||
| ZonedDateTime lastRead = messageDate.minusNanos(1_000_000); // set last read to 1ms before the message date | ||
| // Recalculate unread count from this message onwards | ||
| conversationParticipantRepository.markFromMessageAsUnread(conversationId, userId, messageDate, lastRead); |
There was a problem hiding this comment.
[Medium] Missing server-side validation: users can mark their own messages as unread
The client correctly hides the "Mark as Unread" button when isAuthorOfPosting is true (in posting-reactions-bar.component.ts:476), but the server has no such guard. A user could call the API directly to mark their own message as unread, which contradicts the stated requirement: "Users are not allowed to mark their own messages as unread."
Suggested fix: Add a check in markAsUnread() before performing the update:
if (post.getAuthor().getId().equals(userId)) {
throw new BadRequestAlertException("You cannot mark your own message as unread", "post", "cannotMarkOwnAsUnread");
}| SecurityUtils.setAuthorizationObject(); | ||
| assertThat(getUnreadMessagesCount(createdPost1.getConversation(), student2)).isEqualTo(2); | ||
| }); | ||
| } |
There was a problem hiding this comment.
[Medium] Missing negative integration tests
This test only covers the happy path. The following error scenarios should also be tested:
- Non-member trying to mark a message as unread → should return 403
- Invalid message ID (non-existent) → should return 404
- Message not belonging to the conversation (mismatched conversationId) → should return 404
- Author marking own message as unread → should return 400 (once server-side validation is added)
These tests are important to verify the security checks in the endpoint work correctly.
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void testMarkMessageAsUnread() throws Exception { | ||
| var student1 = userTestRepository.findOneByLogin(TEST_PREFIX + "student1").orElseThrow(); |
There was a problem hiding this comment.
[Medium] Unused variable student1
student1 is fetched on this line but never used in the test. Either remove it or add assertions involving student1 (e.g., verifying that student1's unread count is unaffected, which would be a valuable assertion).
| if (this.activeConversation && this.activeConversation?.id === conversationId) { | ||
| this.activeConversation.lastReadDate = lastReadDate; | ||
| this.activeConversation.unreadMessagesCount = unreadMessagesCount; | ||
| this.activeConversation.hasUnreadMessage = true; |
There was a problem hiding this comment.
[Medium] hasUnreadMessage is unconditionally set to true
hasUnreadMessage is set to true regardless of the actual unreadMessagesCount. If unreadMessagesCount is 0 or undefined, the conversation would still be flagged as having unread messages, which is incorrect.
Suggested fix:
this.activeConversation.hasUnreadMessage = (unreadMessagesCount ?? 0) > 0;Same for line 181 in conversationsOfUser.
| [ngbTooltip]="(getSaved() ? 'artemisApp.metis.post.removeBookmarkPost' : 'artemisApp.metis.post.bookmarkPost') | artemisTranslate" | ||
| > | ||
| <fa-icon [icon]="getSaved() ? faBookmark : faBookmark" /> | ||
| <fa-icon [icon]="getSaved() ? faBookmark : farBookmark" /> |
There was a problem hiding this comment.
[Low] Unrelated bookmark icon fix bundled in this PR
The change from faBookmark : faBookmark (duplicate) to faBookmark : farBookmark (solid vs regular) is a legitimate bug fix, but it's unrelated to the "mark as unread" feature. Consider separating this into its own commit with a clear message so the fix is traceable independently.
|
|
||
| Post postToSave2 = createPostWithOneToOneChat(TEST_PREFIX); | ||
| CreatePostDTO postDTOToSave2 = new CreatePostDTO(postToSave2.getContent(), "", false, new CreatePostConversationDTO(postToSave1.getConversation().getId())); | ||
| Post createdPost2 = request.postWithResponseBody("/api/communication/courses/" + courseId + "/messages", postDTOToSave2, Post.class, HttpStatus.CREATED); |
There was a problem hiding this comment.
[Low] Unused variable createdPost2
createdPost2 is created but never referenced in any assertion or subsequent logic. If it was created to produce a second message in the conversation (to validate the unread count of 2), that intent should be documented with a comment. At minimum, suppress or remove the assignment to avoid confusion.
| private updateLastReadDateAndNumberOfUnreadMessages() { | ||
| // update last read date and number of unread messages of the conversation that is currently active before switching to another conversation | ||
| if (this.activeConversation) { | ||
| public updateLastReadDateAndNumberOfUnreadMessages(conversationId: number | undefined, lastReadDate: dayjs.Dayjs | undefined, unreadMessagesCount: number | undefined) { |
There was a problem hiding this comment.
[Low] Method name updateLastReadDateAndNumberOfUnreadMessages is misleading
This public method is now used to update the conversation state when marking as unread, but its name suggests a generic read-state update. Consider renaming to something like markConversationAsUnread or updateConversationUnreadState to better convey its intent. The old private method was renamed to updateConversationAsRead, which is good — this public counterpart should have similar clarity.
| .subscribe({ | ||
| next: () => { | ||
| const lastReadDate = post.creationDate!.subtract(1, 'millisecond'); | ||
| const unreadMessagesCount = getUnreadPostsByLastReadDate(this.user, this.cachedPosts, lastReadDate!).length; |
There was a problem hiding this comment.
[Nitpick] Redundant non-null assertion on lastReadDate
lastReadDate is already of type dayjs.Dayjs (non-nullable) since it's the result of post.creationDate!.subtract(1, 'millisecond'). The trailing ! is unnecessary:
const unreadMessagesCount = getUnreadPostsByLastReadDate(this.user, this.cachedPosts, lastReadDate).length;6 participants
Summary
Introduces marking messages as unread feature.
Checklist
General
Server
Client
Motivation and Context
Solves issue #11566
Description
A new 'Mark as Unread' button/option added on post's reactions bar and post's right-click dropdown. When user marks a message as unread, the conversation's lastReadDate and unreadMessagesCount is updated, notification counts and new message indicators are updated accordingly by counting onwards from the message that is marked as unread.
Note: Users are not allowed to mark their own messages as unread, and while calculating unread messages count user's own messages are not included in the count.
Steps for Testing
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Server
Last updated: 2026-02-08 19:45:23 UTC
Screenshots
Recording.2026-02-03.185341.mp4
Summary by CodeRabbit
New Features
Tests