feat: implement discussion mute/unmute feature with user and staff-level controls#54
feat: implement discussion mute/unmute feature with user and staff-level controls#54naincy128 wants to merge 18 commits intorelease-ulmofrom
Conversation
naincy128
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| API endpoint to mute a user in discussions using forum service. | ||
|
|
||
| **POST /api/discussion/v1/moderation/forum-mute/** |
There was a problem hiding this comment.
The docstring shows the endpoint as /api/discussion/v1/moderation/forum-mute/ but it should include the course_id parameter to match the actual URL pattern: /api/discussion/v1/moderation/forum-mute/{course_id}/. This inconsistency could confuse developers trying to use this API.
| **POST /api/discussion/v1/moderation/forum-mute/** | |
| **POST /api/discussion/v1/moderation/forum-mute/{course_id}/** |
| if not course_id: | ||
| return False | ||
|
|
||
| # # Convert course_id to CourseKey if it's a string |
There was a problem hiding this comment.
There's a commented-out line with double comment markers that should be removed. This appears to be leftover debugging or refactoring code.
| # # Convert course_id to CourseKey if it's a string | |
| # Convert course_id to CourseKey if it's a string |
| fr"^v1/moderation/forum-mute-status/{settings.COURSE_ID_PATTERN}/(?P<user_id>[0-9]+)/$", | ||
| ForumMuteStatusView.as_view(), | ||
| name="forum_mute_status" | ||
| ), |
There was a problem hiding this comment.
The URL pattern allows an empty string for user_id because the regex pattern [0-9]+ requires at least one digit, but the pattern itself doesn't enforce that the entire path segment is consumed. However, Django's URL routing should handle this correctly. Consider using a more explicit pattern or adding validation in the view to ensure user_id is properly formatted.
| personal_muted = [ | ||
| u for u in muted_users | ||
| if u.get('scope') == 'personal' and str(u.get('muter_id')) == str(request.user.id) | ||
| ] | ||
| course_wide_muted = [u for u in muted_users if u.get('scope') == 'course'] | ||
|
|
||
| # Filter main muted_users list to exclude other users' personal mutes | ||
| filtered_muted_users = [ | ||
| u for u in muted_users | ||
| if u.get('scope') != 'personal' or str(u.get('muter_id')) == str(request.user.id) | ||
| ] | ||
|
|
There was a problem hiding this comment.
The code iterates through muted_users multiple times (lines 369-376, 380-384, 387-390) which is inefficient. Consider consolidating these operations into a single pass through the list to improve performance, especially when there are many muted users.
| personal_muted = [ | |
| u for u in muted_users | |
| if u.get('scope') == 'personal' and str(u.get('muter_id')) == str(request.user.id) | |
| ] | |
| course_wide_muted = [u for u in muted_users if u.get('scope') == 'course'] | |
| # Filter main muted_users list to exclude other users' personal mutes | |
| filtered_muted_users = [ | |
| u for u in muted_users | |
| if u.get('scope') != 'personal' or str(u.get('muter_id')) == str(request.user.id) | |
| ] | |
| personal_muted = [] | |
| course_wide_muted = [] | |
| filtered_muted_users = [] | |
| for user_data in muted_users: | |
| scope_value = user_data.get('scope') | |
| is_personal = scope_value == 'personal' | |
| is_course = scope_value == 'course' | |
| is_current_user_muter = str(user_data.get('muter_id')) == str(request.user.id) | |
| if is_personal and is_current_user_muter: | |
| personal_muted.append(user_data) | |
| if is_course: | |
| course_wide_muted.append(user_data) | |
| # Filter main muted_users list to exclude other users' personal mutes | |
| if not is_personal or is_current_user_muter: | |
| filtered_muted_users.append(user_data) |
| ["anonymous", "raw_body", "title", "topic_id", "type"] | ||
| ) | ||
| elif role == FORUM_ROLE_MODERATOR: | ||
|
|
There was a problem hiding this comment.
Empty line added between method definitions that doesn't follow the convention in the rest of the file. Python PEP 8 recommends two blank lines between top-level function and class definitions, but only one blank line between method definitions within a class. This inconsistency should be removed.
| """ | ||
| API endpoint to mute a user and report their content using forum service. | ||
|
|
||
| **POST /api/discussion/v1/moderation/forum-mute-and-report/** |
There was a problem hiding this comment.
The docstring shows the endpoint as /api/discussion/v1/moderation/forum-mute-and-report/ but it should include the course_id parameter to match the actual URL pattern: /api/discussion/v1/moderation/forum-mute-and-report/{course_id}/. This inconsistency could confuse developers trying to use this API.
| **POST /api/discussion/v1/moderation/forum-mute-and-report/** | |
| **POST /api/discussion/v1/moderation/forum-mute-and-report/{course_id}/** |
| """ | ||
| API endpoint to get mute status for a user using forum service. | ||
|
|
||
| **GET /api/discussion/v1/moderation/forum-mute-status/{user_id}/** |
There was a problem hiding this comment.
The docstring shows the endpoint as /api/discussion/v1/moderation/forum-mute-status/{user_id}/ but it should include the course_id parameter before user_id to match the actual URL pattern: /api/discussion/v1/moderation/forum-mute-status/{course_id}/{user_id}/. This inconsistency could confuse developers trying to use this API.
| **GET /api/discussion/v1/moderation/forum-mute-status/{user_id}/** | |
| **GET /api/discussion/v1/moderation/forum-mute-status/{course_id}/{user_id}/** |
| if paginated_results.page != page: | ||
| raise PageNotFoundError("Page not found (No results on this page).") | ||
|
|
||
| # Always filter muted content for All Posts tab (unless include_muted is explicitly True) | ||
| if include_muted: | ||
| # Only the muted section should set include_muted True | ||
| filtered_threads = paginated_results.collection | ||
| else: | ||
| # Always filter out muted content for All Posts, even after restoration | ||
| filtered_threads = filter_muted_content( | ||
| request.user, | ||
| course_key, | ||
| paginated_results.collection | ||
| ) | ||
|
|
||
| results = _serialize_discussion_entities( | ||
| request, | ||
| context, | ||
| paginated_results.collection, | ||
| filtered_threads, | ||
| requested_fields, | ||
| DiscussionEntity.thread, | ||
| ) |
There was a problem hiding this comment.
The pagination count uses paginated_results.thread_count which includes muted threads, but the actual results have been filtered to exclude muted content. This creates a mismatch where the pagination metadata (total count, page count) doesn't match the actual number of items returned, leading to incorrect pagination behavior and potentially confusing users. The paginator should use len(filtered_threads) instead to reflect the actual filtered count.
| ) | ||
| paginator = DiscussionAPIPagination( | ||
| request, page, num_pages, len(filtered_threads) | ||
| request, page, num_pages, len(filtered_threads_with_deletion_status) |
There was a problem hiding this comment.
The pagination count uses len(filtered_threads_with_deletion_status) which is correct, but this pagination is created after applying mute filtering. However, the original num_pages from the comment service doesn't account for the filtered count, which may cause pagination issues when many users are muted.
| request, page, num_pages, len(filtered_threads_with_deletion_status) | |
| request, | |
| page_num=page, | |
| num_pages=None, | |
| count=len(filtered_threads_with_deletion_status), |
jcapphelix
left a comment
jcapphelix
left a comment
There was a problem hiding this comment.
Approved.
Do inform before merging.
2 participants
Description
This update introduces a comprehensive Mute / Unmute feature for discussion forums, enabling learners and staff to manage unwanted interactions more effectively while maintaining a healthy learning environment. The feature supports both personal and course-wide mute scopes, with clear role-based restrictions and overrides.
The implementation ensures muted content is hidden retroactively as well as for future posts, without notifying muted users. Special handling is included to prevent learners from muting staff or themselves, while giving staff full moderation control across the course.
Features
Learner Capabilities
Staff Capabilities
Key Behaviors
Video Demonstration
compressed-video.mp4
Linked PRs