Communication: Allow tutors to monitor channels as moderator

[cremertim]

Checklist

General

• This is a small issue that I tested locally and was confirmed by another developer on a test server.
• I chose a title conforming to the naming conventions for pull requests.

Server

• Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
• I strictly followed the server coding and design guidelines.

Client

• I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
• I added multiple screenshots/screencasts of my UI changes.

Motivation and Context

Currently, tutors are not able to monitor chats without the cumbersome work of granting each tutor for each channel moderation rights. This affects teaching, since instructors cannot monitor every channel on their own with potentially over 2000 students asking questions

Description

I allowed tutors to pin, delete post and answer posts, if they are not an announcement.

Steps for Testing

Prerequisites:

• 1 Tutor
• 1 Instructor

1. Log in to Artemis
2. Navigate to Course Overview
3. Try to delete an post and an answer to this post from a different user as tutor and instructor (should be possible)
4. Try to delete an announcement as tutor (should not be possible) and as instructor (should be possible)

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.

Review Progress

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Test Coverage

Screenshots

Summary by CodeRabbit

Release Notes

• New Features

  • Enhanced user role management for moderators in course-wide channels based on channel type (announcement vs. regular).
  • Updated permissions for deleting posts, allowing tutors to delete answers in course-wide channels.

• Bug Fixes

  • Corrected visibility of edit and delete options for tutors and instructors based on user roles and post types.

• Tests

  • Updated test cases to reflect new logic for user permissions regarding post editing and deletion.
  • Added assertions to ensure correct behavior of UI elements based on user roles.

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (1)

Line range hint 162-171: Consider adding test cases for announcement channels.

While the test coverage for course-wide channels is good, consider adding specific test cases for announcement channels where only instructors should have deletion rights.

it('should not display delete options to tutors in announcement channels', () => {
    metisServiceUserIsAtLeastInstructorMock.mockReturnValue(false);
    metisServiceUserIsAtLeastTutorMock.mockReturnValue(true);
    metisServiceUserPostingAuthorMock.mockReturnValue(false);
    component.posting = { 
        ...metisResolvingAnswerPostUser1, 
        post: { 
            ...metisPostInChannel, 
            conversation: { 
                ...metisPostInChannel.conversation,
                isAnnouncementChannel: true 
            } 
        } 
    };
    component.ngOnInit();
    fixture.detectChanges();
    expect(getDeleteButton()).toBeNull();
});

src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1)

146-148: LGTM! Role-based moderator assignment logic is well-structured.

The implementation correctly differentiates between announcement and regular channels:

• Announcement channels require instructor privileges
• Regular channels allow teaching assistants (tutors) to moderate

This maintains security while achieving the PR objective of enabling tutors to monitor channels.

Consider adding logging for moderator role assignments to aid in auditing and troubleshooting.

src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (1)

206-211: Consider simplifying the boolean logic for better readability

The current implementation uses multiple boolean conditions that could be simplified for better maintainability.

Consider this refactoring:

-        const isAnswerOfAnnouncement = getAsChannelDTO(this.posting.conversation)?.isAnnouncementChannel ?? false;
-        const isAtLeastTutorInCourse = this.metisService.metisUserIsAtLeastTutorInCourse();
-        const canDeleteAnnouncement = isAnswerOfAnnouncement ? this.metisService.metisUserIsAtLeastInstructorInCourse() : true;
-        const mayDeleteOtherUsersAnswer =
-            (isCourseWideChannel && isAtLeastTutorInCourse) || (getAsChannelDTO(this.metisService.getCurrentConversation())?.hasChannelModerationRights ?? false);
-        this.mayDelete = !this.readOnlyMode && !this.previewMode && (this.isAuthorOfPosting || mayDeleteOtherUsersAnswer) && canDeleteAnnouncement;
+        const channel = getAsChannelDTO(this.posting.conversation);
+        const currentChannel = getAsChannelDTO(this.metisService.getCurrentConversation());
+        
+        const isAnnouncement = channel?.isAnnouncementChannel ?? false;
+        const hasModeratorRights = 
+            (isCourseWideChannel && this.metisService.metisUserIsAtLeastTutorInCourse()) || 
+            (currentChannel?.hasChannelModerationRights ?? false);
+            
+        const canDelete = 
+            (!isAnnouncement || this.metisService.metisUserIsAtLeastInstructorInCourse()) &&
+            (this.isAuthorOfPosting || hasModeratorRights);
+            
+        this.mayDelete = !this.readOnlyMode && !this.previewMode && canDelete;

This refactoring:

1. Reduces variable dereferencing
2. Groups related conditions
3. Improves readability with better variable names
4. Maintains the same logic while being more maintainable

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a82ca47 and a732eb7.

📒 Files selected for processing (5)

• src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1 hunks)
• src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1 hunks)
• src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (2 hunks)
• src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (3 hunks)
• src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (3 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

src/main/java/de/tum/cit/aet/artemis/communication/service/conversation/ConversationService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (1)

Pattern src/main/webapp/**/*.ts: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalCase;funcs:camelCase;props:camelCase;no_priv_prefix:true;strings:single_quotes;localize:true;btns:functionality;links:navigation;icons_text:newline;labels:associate;code_style:arrow_funcs,curly_braces,open_braces_same_line,indent_4;memory_leak_prevention:true;routes:naming_schema;chart_framework:ngx-charts;responsive_layout:true

src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (1)

Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}

🔇 Additional comments (6)

src/main/webapp/app/shared/metis/posting-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.ts (1)

77-80: Verify the role-based permission logic.

The permission model has been updated to allow tutors to delete posts in regular channels while restricting deletion in announcement channels to instructors only. Let's verify this implementation:

✅ Verification successful

Based on the search results, I can now generate the final response as I have sufficient information to verify the role-based permission logic.

Role-based permission logic is correctly implemented

The implementation consistently enforces that:

• In regular channels, tutors and above can delete posts
• In announcement channels, only instructors and above can delete posts
• Users can always delete their own posts regardless of channel type
• Channel moderators have deletion rights in their channels

This is verified through consistent usage across:

• answer-post-reactions-bar.component.ts
• post-reactions-bar.component.ts
• The underlying metisService methods

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any inconsistencies in role-based permission handling across the codebase.

# Test 1: Search for other instances of role checks to ensure consistent permission model
rg -A 3 "metisUserIsAtLeastTutorInCourse|metisUserIsAtLeastInstructorInCourse"

# Test 2: Search for potential security-sensitive operations that might be affected
ast-grep --pattern 'delete$_($$$)'

Length of output: 15929

src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts (1)

134-134: LGTM! Test coverage updated for tutor permissions.

The test case correctly sets up the tutor permission mock to verify the new permission model.

src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts (2)

Line range hint 226-235: LGTM! Test case correctly verifies tutor moderation capabilities.

The test case properly verifies that tutors can delete but not edit posts in course-wide channels, which aligns with the PR objective of enabling tutors to monitor channels as moderators.

---

260-267: LGTM! Test case validates instructor permissions.

The test case correctly verifies that instructors retain their ability to delete student posts in course-wide channels, ensuring that the new tutor permissions don't affect existing instructor capabilities.

src/main/webapp/app/shared/metis/posting-reactions-bar/post-reactions-bar/post-reactions-bar.component.ts (2)

206-211: LGTM: Enhanced deletion permissions with proper role checks

The implementation correctly handles the special cases for announcements and course-wide channels while granting appropriate permissions to tutors.

Let's verify the test coverage for these conditions:

✅ Verification successful

Test coverage exists for the new deletion permissions logic

The test files show comprehensive coverage for the new deletion permissions logic, including:

• Tests for announcement channel permissions
• Tests for course-wide channel permissions
• Tests for different user roles (instructor, tutor, regular user)
• Tests for author vs non-author scenarios
• Tests for the combined conditions in mayDelete

Specific test cases that verify the new conditions:

• "should not display edit and delete options to tutor if posting is announcement"
• "should display edit and delete options to instructor if his posting is announcement"
• "should display the delete option to instructor if posting is in course-wide channel from a student"
• "should display the delete option to user with channel moderation rights when not the author"

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check test coverage for announcement and course-wide channel deletion permissions

# Search for relevant test cases
rg -A 5 "isAnnouncementChannel|isCourseWide.*delete" "src/test/javascript/spec/**/*spec.ts"

Length of output: 170

---

Script:

#!/bin/bash
# Let's try different approaches to find test files and relevant code

# First, find test files related to post-reactions-bar
fd "post-reactions-bar.*spec" 

# Then search for test files containing "delete" in the entire codebase
fd "spec.*ts$" --exec rg -l "delete.*test|test.*delete"

# Search for the specific component's test coverage
rg -l "PostReactionsBarComponent" --type ts

# Look for test cases involving announcement channels
rg "isAnnouncementChannel" --type ts

Length of output: 12633

---

Script:

#!/bin/bash
# Let's examine the test coverage for deletion permissions in the post-reactions-bar component

# Check the specific test file content
cat "src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts"

# Also check the answer-post-reactions-bar tests as they might contain relevant test cases
cat "src/test/javascript/spec/component/shared/metis/postings-reactions-bar/answer-post-reactions-bar/answer-post-reactions-bar.component.spec.ts"

Length of output: 31227

---

103-103: LGTM: Permission check updated to include tutors

The change aligns with the PR objective of enabling tutors to monitor channels by granting them pin capabilities.

Let's verify the test coverage for this change:

✅ Verification successful

Based on the gathered evidence, I can now provide a final response:

Permission check for tutor pin capability is properly implemented and tested

The change to allow tutors to pin posts is correctly implemented:

• The metisUserIsAtLeastTutorInCourse() method in MetisService properly checks tutor permissions via course.isAtLeastTutor
• The component has comprehensive test coverage for various permission scenarios, including tutor-specific cases
• The implementation aligns with the PR's objective of enabling tutors to monitor channels as moderators

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if there are corresponding test cases for tutor pin permissions

# Search for test cases related to tutor pin permissions
rg -A 5 "tutor.*pin|pin.*tutor" "src/test/javascript/spec/**/*spec.ts"

Length of output: 149

---

Script:

#!/bin/bash
# Let's first locate the test files directory structure
fd "spec.ts$"

# Then search for any test files related to post-reactions-bar component
fd "post-reactions-bar.*spec.ts$"

# Also check for any test files containing metis in their path
fd "metis.*spec.ts$"

Length of output: 90041

---

Script:

#!/bin/bash
# Let's check the test file for post-reactions-bar component
cat "src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts"

# Also check the implementation of metisService.metisUserIsAtLeastTutorInCourse
ast-grep --pattern 'metisUserIsAtLeastTutorInCourse() {
  $$$
}'

Length of output: 19626

---

Script:

#!/bin/bash
# Let's check the implementation of MetisService
rg -A 10 "class MetisService" "src/main/webapp/app/shared/metis/"

# Also check for any usages of metisUserIsAtLeastTutorInCourse
rg "metisUserIsAtLeastTutorInCourse" "src/main/webapp/app/shared/metis/"

Length of output: 3240

---

Script:

#!/bin/bash
# Let's check the implementation of metisUserIsAtLeastTutorInCourse method
rg -A 5 "metisUserIsAtLeastTutorInCourse\(\): boolean" "src/main/webapp/app/shared/metis/metis.service.ts"

# Also check the test coverage for this change
rg -A 10 "should display edit option to tutor|should display pin option to tutor|should allow tutor to pin" "src/test/javascript/spec/component/shared/metis/postings-reactions-bar/post-reactions-bar/post-reactions-bar.component.spec.ts"

Length of output: 535

[ahbitaqu]

[Tested on TS4]
Doesn't work for me unfortunately. Even as an Instructor I am unable to edit messages now (see screenshot)

[HawKhiem]

Tested on TS2. I cannot edit messages both as instructor and tutor

[cremertim]

Tested on TS2. I cannot edit messages both as instructor and tutor

This is the intended behaviour. As introduced in #9830, you can now only edit your own messages. Sorry for the missleading description, i updated it. You can only delete them now

[asliayk]

tested on ts2, everything worked as described

[eylulnc]

Tested in TS2

[anian03]

Tested during Testing session on TS2, works as expected

[PaRangger]

Code LGTM, Tested on TS2.

[HawKhiem]

Tested on TS2. I can indeed delete messages of other users as instuctor but not as tutor