From b8e466b55cc95ec63ae08da614086c4e2b2c9bac Mon Sep 17 00:00:00 2001 From: SaadYousaf Date: Wed, 31 Aug 2022 19:11:16 +0500 Subject: [PATCH] fix: update editable action item for different discussion and course roles --- lms/djangoapps/discussion/rest_api/api.py | 12 ++--- .../discussion/rest_api/permissions.py | 24 +++++----- .../discussion/rest_api/serializers.py | 33 ++++++++----- .../discussion/rest_api/tests/test_api.py | 11 ++--- .../rest_api/tests/test_permissions.py | 47 +++++++++--------- .../rest_api/tests/test_serializers.py | 2 +- .../discussion/rest_api/tests/test_utils.py | 11 +++-- lms/djangoapps/discussion/rest_api/utils.py | 48 +++++++++++-------- 8 files changed, 107 insertions(+), 81 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index a7155abcf8..71f8f021f7 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -198,12 +198,12 @@ def _get_thread_and_context(request, thread_id, retrieve_kwargs=None): course = _get_course(course_key, request.user) context = get_context(course, request, cc_thread) - if retrieve_kwargs.get("flagged_comments") and not context["is_requester_privileged"]: + if retrieve_kwargs.get("flagged_comments") and not context["has_moderation_privilege"]: raise ValidationError("Only privileged users can request flagged comments") course_discussion_settings = CourseDiscussionSettings.get(course_key) if ( - not context["is_requester_privileged"] and + not context["has_moderation_privilege"] and cc_thread["group_id"] and is_commentable_divided(course.id, cc_thread["commentable_id"], course_discussion_settings) ): @@ -241,7 +241,7 @@ def _is_user_author_or_privileged(cc_content, context): Boolean """ return ( - context["is_requester_privileged"] or + context["has_moderation_privilege"] or context["cc_requester"]["id"] == cc_content["user_id"] ) @@ -819,7 +819,7 @@ def get_thread_list( "text_search_rewrite": None, }) - if count_flagged and not context["is_requester_privileged"]: + if count_flagged and not context["has_moderation_privilege"]: raise PermissionDenied("`count_flagged` can only be set by users with moderator access or higher.") group_id = None @@ -836,7 +836,7 @@ def get_thread_list( except ValueError: pass - if (group_id is None) and (not context["is_requester_privileged"]): + if (group_id is None) and not context["has_moderation_privilege"]: group_id = get_group_id_for_user(request.user, CourseDiscussionSettings.get(course.id)) query_params = { @@ -1559,7 +1559,7 @@ def get_user_comments( course = _get_course(course_key, request.user) context = get_context(course, request) - if flagged and not context["is_requester_privileged"]: + if flagged and not context["has_moderation_privilege"]: raise ValidationError("Only privileged users can filter comments by flagged status") try: diff --git a/lms/djangoapps/discussion/rest_api/permissions.py b/lms/djangoapps/discussion/rest_api/permissions.py index 01fcfaff8e..dc888729bb 100644 --- a/lms/djangoapps/discussion/rest_api/permissions.py +++ b/lms/djangoapps/discussion/rest_api/permissions.py @@ -35,7 +35,7 @@ def _is_author_or_privileged(cc_content, context): Return True if the requester authored the given content or is a privileged user, False otherwise """ - return context["is_requester_privileged"] or _is_author(cc_content, context) + return context["has_moderation_privilege"] or _is_author(cc_content, context) NON_UPDATABLE_THREAD_FIELDS = {"course_id"} @@ -92,7 +92,7 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se # no edits, except 'abuse_flagged' is allowed for comment is_thread = cc_content["type"] == "thread" is_comment = cc_content["type"] == "comment" - is_privileged = context["is_requester_privileged"] + has_moderation_privilege = context["has_moderation_privilege"] if is_thread: is_thread_closed = cc_content["closed"] @@ -105,9 +105,9 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se # Map each field to the condition in which it's editable. editable_fields = { "abuse_flagged": True, - "closed": is_thread and is_privileged, - "close_reason_code": is_thread and is_privileged, - "pinned": is_thread and is_privileged, + "closed": is_thread and has_moderation_privilege, + "close_reason_code": is_thread and has_moderation_privilege, + "pinned": is_thread and has_moderation_privilege, "read": is_thread, } if is_thread: @@ -120,16 +120,16 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se is_author = _is_author(cc_content, context) editable_fields.update({ "voted": True, - "raw_body": is_privileged or is_author, - "edit_reason_code": is_privileged, + "raw_body": has_moderation_privilege or is_author, + "edit_reason_code": has_moderation_privilege and not is_author, "following": is_thread, - "topic_id": is_thread and (is_author or is_privileged), - "type": is_thread and (is_author or is_privileged), - "title": is_thread and (is_author or is_privileged), - "group_id": is_thread and is_privileged and context["discussion_division_enabled"], + "topic_id": is_thread and (is_author or has_moderation_privilege), + "type": is_thread and (is_author or has_moderation_privilege), + "title": is_thread and (is_author or has_moderation_privilege), + "group_id": is_thread and has_moderation_privilege and context["discussion_division_enabled"], "endorsed": ( (is_comment and cc_content.get("parent_id", None) is None) and - (is_privileged or + (has_moderation_privilege or (_is_author(context["thread"], context) and context["thread"]["thread_type"] == "question")) ), "anonymous": is_author and context["course"].allow_anonymous, diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 9d42437dfc..e5f68ef474 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -14,6 +14,7 @@ from django.utils.text import Truncator from rest_framework import serializers from common.djangoapps.student.models import get_user_by_username_or_email +from common.djangoapps.student.roles import GlobalStaff from lms.djangoapps.discussion.django_comment_client.base.views import track_thread_lock_unlock_event from lms.djangoapps.discussion.django_comment_client.utils import ( course_discussion_division_enabled, @@ -28,7 +29,11 @@ from lms.djangoapps.discussion.rest_api.permissions import ( get_editable_fields, ) from lms.djangoapps.discussion.rest_api.render import render_body -from lms.djangoapps.discussion.rest_api.utils import get_course_staff_users_list, get_course_ta_users_list +from lms.djangoapps.discussion.rest_api.utils import ( + get_course_staff_users_list, + get_moderator_users_list, + get_course_ta_users_list, +) from openedx.core.djangoapps.discussions.models import DiscussionTopicLink from openedx.core.djangoapps.discussions.utils import get_group_names_by_id from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment @@ -37,7 +42,6 @@ from openedx.core.djangoapps.django_comment_common.comment_client.user import Us from openedx.core.djangoapps.django_comment_common.comment_client.utils import CommentClientRequestError from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings from openedx.core.lib.api.serializers import CourseKeyField -from common.djangoapps.student.roles import GlobalStaff User = get_user_model() @@ -59,23 +63,26 @@ def get_context(course, request, thread=None): Returns a context appropriate for use with ThreadSerializer or (if thread is provided) CommentSerializer. """ - staff_user_ids = get_course_staff_users_list(course.id) + course_staff_user_ids = get_course_staff_users_list(course.id) + moderator_user_ids = get_moderator_users_list(course.id) ta_user_ids = get_course_ta_users_list(course.id) requester = request.user cc_requester = CommentClientUser.from_django_user(requester).retrieve() cc_requester["course_id"] = course.id course_discussion_settings = CourseDiscussionSettings.get(course.id) is_global_staff = GlobalStaff().has_user(requester) + has_moderation_privilege = requester.id in moderator_user_ids or requester.id in ta_user_ids or is_global_staff return { "course": course, "request": request, "thread": thread, "discussion_division_enabled": course_discussion_division_enabled(course_discussion_settings), "group_ids_to_names": get_group_names_by_id(course_discussion_settings), - "is_requester_privileged": requester.id in staff_user_ids or requester.id in ta_user_ids or is_global_staff, - "staff_user_ids": staff_user_ids, + "moderator_user_ids": moderator_user_ids, + "course_staff_user_ids": course_staff_user_ids, "ta_user_ids": ta_user_ids, "cc_requester": cc_requester, + "has_moderation_privilege": has_moderation_privilege, } @@ -123,7 +130,7 @@ def _validate_privileged_access(context: Dict) -> bool: bool: Course exists and the user has privileged access. """ course = context.get('course', None) - is_requester_privileged = context.get('is_requester_privileged') + is_requester_privileged = context.get('has_moderation_privilege') return course and is_requester_privileged @@ -169,7 +176,7 @@ class _ContentSerializer(serializers.Serializer): Returns a boolean indicating whether the given user_id identifies a privileged user. """ - return user_id in self.context["staff_user_ids"] or user_id in self.context["ta_user_ids"] + return user_id in self.context["moderator_user_ids"] or user_id in self.context["ta_user_ids"] def _is_anonymous(self, obj): """ @@ -177,7 +184,8 @@ class _ContentSerializer(serializers.Serializer): the requester. """ user_id = self.context["request"].user.id - is_user_staff = user_id in self.context["staff_user_ids"] + is_user_staff = user_id in self.context["moderator_user_ids"] or user_id in self.context["ta_user_ids"] + return ( obj["anonymous"] or obj["anonymous_to_peers"] and not is_user_staff @@ -194,9 +202,12 @@ class _ContentSerializer(serializers.Serializer): Returns the role label (i.e. "Staff" or "Community TA") for the user with the given id. """ + is_staff = user_id in self.context["course_staff_user_ids"] or user_id in self.context["moderator_user_ids"] + is_ta = user_id in self.context["ta_user_ids"] + return ( - "Staff" if user_id in self.context["staff_user_ids"] else - "Community TA" if user_id in self.context["ta_user_ids"] else + "Staff" if is_staff else + "Community TA" if is_ta else None ) @@ -225,7 +236,7 @@ class _ContentSerializer(serializers.Serializer): """ total_abuse_flaggers = len(obj.get("abuse_flaggers", [])) return ( - self.context["is_requester_privileged"] and total_abuse_flaggers > 0 or + self.context["has_moderation_privilege"] and total_abuse_flaggers > 0 or self.context["cc_requester"]["id"] in obj.get("abuse_flaggers", []) ) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index f431f8aa56..1fc3b621b8 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -1894,7 +1894,6 @@ class CreateThreadTest( "close_reason_code", "closed", "copy_link", - "edit_reason_code", "following", "pinned", "raw_body", @@ -2249,14 +2248,13 @@ class CreateCommentTest( editable_fields = [ "abuse_flagged", "anonymous", - "edit_reason_code", "raw_body", "voted", ] if parent_id: data["parent_id"] = parent_id else: - editable_fields.insert(3, "endorsed") + editable_fields.insert(2, "endorsed") _set_course_discussion_blackout(course=self.course, user_id=self.user.id) _assign_role_to_user(user=self.user, course_id=self.course.id, role=FORUM_ROLE_MODERATOR) @@ -2871,7 +2869,7 @@ class UpdateThreadTest( Test editing comments, specifying and retrieving edit reason codes. """ _assign_role_to_user(user=self.user, course_id=self.course.id, role=role_name) - self.register_thread() + self.register_thread({"user_id": str(self.user.id + 1)}) try: result = update_thread(self.request, "test_thread", { "raw_body": "Edited body", @@ -2888,7 +2886,8 @@ class UpdateThreadTest( assert request_body["edit_reason_code"] == ["test-edit-reason"] except ValidationError as error: assert role_name == FORUM_ROLE_STUDENT - assert error.message_dict == {"edit_reason_code": ["This field is not editable."]} + assert error.message_dict == {"edit_reason_code": ["This field is not editable."], + "raw_body": ["This field is not editable."]} @ddt.data( *itertools.product( @@ -3371,7 +3370,7 @@ class UpdateCommentTest( Test editing comments, specifying and retrieving edit reason codes. """ _assign_role_to_user(user=self.user, course_id=self.course.id, role=role_name) - self.register_comment() + self.register_comment({"user_id": str(self.user.id + 1)}) try: result = update_comment(self.request, "test_comment", { "raw_body": "Edited body", diff --git a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py index 4c2a4deed9..5ad67619ef 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py @@ -22,16 +22,15 @@ from openedx.core.djangoapps.django_comment_common.comment_client.user import Us def _get_context( requester_id, - is_requester_privileged, is_cohorted=False, thread=None, allow_anonymous=True, allow_anonymous_to_peers=False, + has_moderation_privilege=False, ): """Return a context suitable for testing the permissions module""" return { "cc_requester": User(id=requester_id), - "is_requester_privileged": is_requester_privileged, "course": CourseFactory( cohort_config={"cohorted": is_cohorted}, allow_anonymous=allow_anonymous, @@ -39,6 +38,7 @@ def _get_context( ), "discussion_division_enabled": is_cohorted, "thread": thread, + "has_moderation_privilege": has_moderation_privilege, } @@ -56,7 +56,7 @@ class GetInitializableFieldsTest(ModuleStoreTestCase): ): context = _get_context( requester_id="5", - is_requester_privileged=is_privileged, + has_moderation_privilege=is_privileged, is_cohorted=is_cohorted, allow_anonymous=allow_anonymous, allow_anonymous_to_peers=allow_anonymous_to_peers, @@ -67,7 +67,7 @@ class GetInitializableFieldsTest(ModuleStoreTestCase): "read", "title", "topic_id", "type", "voted" } if is_privileged: - expected |= {"closed", "pinned", "close_reason_code", "edit_reason_code"} + expected |= {"closed", "pinned", "close_reason_code"} if is_privileged and is_cohorted: expected |= {"group_id"} if allow_anonymous: @@ -81,15 +81,13 @@ class GetInitializableFieldsTest(ModuleStoreTestCase): def test_comment(self, is_thread_author, thread_type, is_privileged): context = _get_context( requester_id="5", - is_requester_privileged=is_privileged, + has_moderation_privilege=is_privileged, thread=Thread(user_id="5" if is_thread_author else "6", thread_type=thread_type) ) actual = get_initializable_comment_fields(context) expected = { "anonymous", "abuse_flagged", "parent_id", "raw_body", "thread_id", "voted" } - if is_privileged: - expected |= {"edit_reason_code"} if (is_thread_author and thread_type == "question") or is_privileged: expected |= {"endorsed"} assert actual == expected @@ -103,31 +101,34 @@ class GetEditableFieldsTest(ModuleStoreTestCase): def test_thread( self, is_author, - is_privileged, is_cohorted, allow_anonymous, - allow_anonymous_to_peers + allow_anonymous_to_peers, + has_moderation_privilege, ): thread = Thread(user_id="5" if is_author else "6", type="thread") context = _get_context( requester_id="5", - is_requester_privileged=is_privileged, is_cohorted=is_cohorted, allow_anonymous=allow_anonymous, allow_anonymous_to_peers=allow_anonymous_to_peers, + has_moderation_privilege=has_moderation_privilege, ) actual = get_editable_fields(thread, context) expected = {"abuse_flagged", "copy_link", "following", "read", "voted"} - if is_privileged: - expected |= {"closed", "pinned", "close_reason_code", "edit_reason_code"} - if is_author or is_privileged: - expected |= {"topic_id", "type", "title", "raw_body"} - if is_privileged and is_cohorted: + if has_moderation_privilege: + expected |= {"closed", "pinned", "close_reason_code"} + if has_moderation_privilege and not is_author: + expected |= {"edit_reason_code"} + if is_author or has_moderation_privilege: + expected |= {"raw_body", "topic_id", "type", "title"} + if has_moderation_privilege and is_cohorted: expected |= {"group_id"} if is_author and allow_anonymous: expected |= {"anonymous"} if is_author and allow_anonymous_to_peers: expected |= {"anonymous_to_peers"} + assert actual == expected @ddt.data(*itertools.product(*[[True, False] for _ in range(6)], ["question", "discussion"])) @@ -136,11 +137,11 @@ class GetEditableFieldsTest(ModuleStoreTestCase): self, is_author, is_thread_author, - is_privileged, allow_anonymous, allow_anonymous_to_peers, has_parent, - thread_type + has_moderation_privilege, + thread_type, ): comment = Comment( user_id="5" if is_author else "6", @@ -149,18 +150,18 @@ class GetEditableFieldsTest(ModuleStoreTestCase): ) context = _get_context( requester_id="5", - is_requester_privileged=is_privileged, thread=Thread(user_id="5" if is_thread_author else "6", thread_type=thread_type), allow_anonymous=allow_anonymous, allow_anonymous_to_peers=allow_anonymous_to_peers, + has_moderation_privilege=has_moderation_privilege, ) actual = get_editable_fields(comment, context) expected = {"abuse_flagged", "voted"} - if is_privileged: + if has_moderation_privilege and not is_author: expected |= {"edit_reason_code"} - if is_author or is_privileged: + if is_author or has_moderation_privilege: expected |= {"raw_body"} - if not has_parent and ((is_thread_author and thread_type == "question") or is_privileged): + if not has_parent and ((is_thread_author and thread_type == "question") or has_moderation_privilege): expected |= {"endorsed"} if is_author and allow_anonymous: expected |= {"anonymous"} @@ -176,7 +177,7 @@ class CanDeleteTest(ModuleStoreTestCase): @ddt.unpack def test_thread(self, is_author, is_privileged): thread = Thread(user_id="5" if is_author else "6") - context = _get_context(requester_id="5", is_requester_privileged=is_privileged) + context = _get_context(requester_id="5", has_moderation_privilege=is_privileged) assert can_delete(thread, context) == (is_author or is_privileged) @ddt.data(*itertools.product([True, False], [True, False], [True, False])) @@ -185,7 +186,7 @@ class CanDeleteTest(ModuleStoreTestCase): comment = Comment(user_id="5" if is_author else "6") context = _get_context( requester_id="5", - is_requester_privileged=is_privileged, + has_moderation_privilege=is_privileged, thread=Thread(user_id="5" if is_thread_author else "6") ) assert can_delete(comment, context) == (is_author or is_privileged) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py index a4cfd93406..0c417d6d13 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py @@ -73,7 +73,7 @@ class SerializerTestMixin(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetM (FORUM_ROLE_MODERATOR, True, False, True), (FORUM_ROLE_MODERATOR, False, True, False), (FORUM_ROLE_COMMUNITY_TA, True, False, True), - (FORUM_ROLE_COMMUNITY_TA, False, True, True), + (FORUM_ROLE_COMMUNITY_TA, False, True, False), (FORUM_ROLE_STUDENT, True, False, True), (FORUM_ROLE_STUDENT, False, True, True), ) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_utils.py b/lms/djangoapps/discussion/rest_api/tests/test_utils.py index a0924549c3..8f4b940f9e 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_utils.py @@ -14,8 +14,9 @@ from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, U from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory from lms.djangoapps.discussion.rest_api.utils import ( discussion_open_for_user, - get_course_staff_users_list, get_course_ta_users_list, + get_course_staff_users_list, + get_moderator_users_list, ) @@ -62,7 +63,11 @@ class DiscussionAPIUtilsTestCase(ModuleStoreTestCase): self.assertTrue(discussion_open_for_user(self.course, self.community_ta)) def test_course_staff_users_list(self): - assert len(get_course_staff_users_list(self.course.id)) == 3 + assert len(get_course_staff_users_list(self.course.id)) == 2 + + def test_course_moderator_users_list(self): + assert len(get_moderator_users_list(self.course.id)) == 1 def test_course_ta_users_list(self): - assert len(get_course_ta_users_list(self.course.id)) == 2 + ta_user_list = get_course_ta_users_list(self.course.id) + assert len(ta_user_list) == 2 diff --git a/lms/djangoapps/discussion/rest_api/utils.py b/lms/djangoapps/discussion/rest_api/utils.py index 8163f1e965..96bca970c0 100644 --- a/lms/djangoapps/discussion/rest_api/utils.py +++ b/lms/djangoapps/discussion/rest_api/utils.py @@ -118,22 +118,15 @@ def add_stats_for_users_with_no_discussion_content(course_stats, users_in_course def get_course_staff_users_list(course_id): """ Gets user ids for Staff roles for course discussions. - Roles include Discussion Administrator, Discussion Moderator, Course Instructor and Course Staff. + Roles Course Instructor and Course Staff. """ - # TODO: cache staff_user_ids if we need to improve perf - staff_user_ids = [ - user.id - for role in Role.objects.filter( - name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR], - course_id=course_id - ) - for user in role.users.all() - ] + # TODO: cache course_staff_user_ids if we need to improve perf + course_staff_user_ids = [] staff = list(CourseStaffRole(course_id).users_with_role().values_list('id', flat=True)) admins = list(CourseInstructorRole(course_id).users_with_role().values_list('id', flat=True)) - staff_user_ids.extend(staff) - staff_user_ids.extend(admins) - return set(staff_user_ids) + course_staff_user_ids.extend(staff) + course_staff_user_ids.extend(admins) + return list(set(course_staff_user_ids)) def get_course_ta_users_list(course_id): @@ -141,11 +134,28 @@ def get_course_ta_users_list(course_id): Gets user ids for TA roles for course discussions. Roles include Community TA and Group Community TA. """ - # TODO: cache ta_user_ids if we need to improve perf - ta_user_ids = { + # TODO: cache ta_users_ids if we need to improve perf + ta_users_ids = [ user.id - for role in Role.objects.filter(name__in=[FORUM_ROLE_COMMUNITY_TA, - FORUM_ROLE_GROUP_MODERATOR], course_id=course_id) + for role in Role.objects.filter(name__in=[FORUM_ROLE_GROUP_MODERATOR, + FORUM_ROLE_COMMUNITY_TA], course_id=course_id) for user in role.users.all() - } - return ta_user_ids + ] + return ta_users_ids + + +def get_moderator_users_list(course_id): + """ + Gets user ids for Moderator roles for course discussions. + Roles include Discussion Administrator and Discussion Moderator. + """ + # TODO: cache moderator_user_ids if we need to improve perf + moderator_user_ids = [ + user.id + for role in Role.objects.filter( + name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR], + course_id=course_id + ) + for user in role.users.all() + ] + return moderator_user_ids