From f3b89770e03ff60ebba1256c8850c92b19570258 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 18 Mar 2022 11:43:30 +0530 Subject: [PATCH] feat: add support for specifying and retrieving moderation reason codes from the LMS settings (#30015) Reason codes will be used by the frontend to list and validate the reasons for specifying moderation actions. Co-authored-by: Kshitij Sobti Co-authored-by: Felipe Trzaskowski --- lms/djangoapps/discussion/rest_api/api.py | 38 ++++++---- .../discussion/rest_api/serializers.py | 69 ++++++++++++++++--- .../discussion/rest_api/tests/test_api.py | 26 +++++-- .../discussion/rest_api/tests/test_views.py | 28 ++++---- .../discussion/rest_api/tests/utils.py | 1 + lms/djangoapps/discussion/rest_api/views.py | 2 +- lms/djangoapps/discussion/toggles.py | 13 ++++ lms/envs/common.py | 23 ++++++- 8 files changed, 157 insertions(+), 43 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index ac1edff574b5..cbe509dac14e 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -9,6 +9,7 @@ from typing import Dict, Iterable, List, Literal, Optional, Set, Tuple from urllib.parse import urlencode, urlunparse +from django.conf import settings from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.http import Http404 @@ -54,6 +55,19 @@ from openedx.core.djangoapps.user_api.accounts.api import get_account_settings from openedx.core.lib.exceptions import CourseNotFoundError, DiscussionNotFoundError, PageNotFoundError +from ..django_comment_client.base.views import ( + track_comment_created_event, + track_thread_created_event, + track_thread_viewed_event, + track_voted_event, +) +from ..django_comment_client.utils import ( + get_group_id_for_user, + get_user_role_names, + has_discussion_privileges, + is_commentable_divided, +) +from ..toggles import ENABLE_DISCUSSION_MODERATION_REASON_CODES from .exceptions import CommentNotFoundError, DiscussionBlackOutException, DiscussionDisabledError, ThreadNotFoundError from .forms import CommentActionsForm, ThreadActionsForm, UserOrdering from .pagination import DiscussionAPIPagination @@ -73,18 +87,6 @@ get_context, ) from .utils import discussion_open_for_user -from ..django_comment_client.base.views import ( - track_comment_created_event, - track_thread_created_event, - track_thread_viewed_event, - track_voted_event, -) -from ..django_comment_client.utils import ( - get_group_id_for_user, - get_user_role_names, - has_discussion_privileges, - is_commentable_divided, -) User = get_user_model() @@ -281,6 +283,8 @@ def _format_datetime(dt): course = _get_course(course_key, request.user) user_roles = get_user_role_names(request.user, course_key) course_config = DiscussionsConfiguration.get(course_key) + EDIT_REASON_CODES = getattr(settings, "DISCUSSION_MODERATION_EDIT_REASON_CODES", {}) + CLOSE_REASON_CODES = getattr(settings, "DISCUSSION_MODERATION_CLOSE_REASON_CODES", {}) return { "id": str(course_key), @@ -308,6 +312,16 @@ def _format_datetime(dt): "enable_in_context": course_config.enable_in_context, "group_at_subsection": course_config.plugin_configuration.get("group_at_subsection", False), 'learners_tab_enabled': ENABLE_LEARNERS_TAB_IN_DISCUSSIONS_MFE.is_enabled(course_key), + "reason_codes_enabled": ENABLE_DISCUSSION_MODERATION_REASON_CODES.is_enabled(course_key), + "edit_reasons": [ + {"code": reason_code, "label": label} + for (reason_code, label) in EDIT_REASON_CODES.items() + ], + "post_close_reasons": [ + {"code": reason_code, "label": label} + for (reason_code, label) in CLOSE_REASON_CODES.items() + ], + } diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index 365ce9fdf54d..03675d5fb97e 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -4,6 +4,7 @@ from typing import Dict from urllib.parse import urlencode, urlunparse +from django.conf import settings from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError from django.db.models import TextChoices @@ -43,6 +44,9 @@ User = get_user_model() +CLOSE_REASON_CODES = getattr(settings, "DISCUSSION_MODERATION_CLOSE_REASON_CODES", {}) +EDIT_REASON_CODES = getattr(settings, "DISCUSSION_MODERATION_EDIT_REASON_CODES", {}) + class TopicOrdering(TextChoices): """ @@ -99,6 +103,26 @@ def validate_not_blank(value): raise ValidationError("This field may not be blank.") +def validate_edit_reason_code(value): + """ + Validate that the value is a valid edit reason code. + + Raises: ValidationError + """ + if value not in EDIT_REASON_CODES: + raise ValidationError("Invalid edit reason code") + + +def validate_close_reason_code(value): + """ + Validate that the value is a valid close reason code. + + Raises: ValidationError + """ + if value not in CLOSE_REASON_CODES: + raise ValidationError("Invalid close reason code") + + def _validate_privileged_access(context: Dict) -> bool: """ Return the field specified by ``field_name`` if requesting user is privileged. @@ -137,7 +161,7 @@ class _ContentSerializer(serializers.Serializer): anonymous = serializers.BooleanField(default=False) anonymous_to_peers = serializers.BooleanField(default=False) last_edit = serializers.SerializerMethodField(required=False) - edit_reason_code = serializers.CharField(required=False) + edit_reason_code = serializers.CharField(required=False, validators=[validate_edit_reason_code]) non_updatable_fields = set() @@ -245,10 +269,16 @@ def get_last_edit(self, obj): Returns information about the last edit for this content for privileged users. """ - if _validate_privileged_access(self.context): - edit_history = obj.get("edit_history") - if edit_history: - return edit_history[-1] + if not _validate_privileged_access(self.context): + return None + edit_history = obj.get("edit_history") + if not edit_history: + return None + last_edit = edit_history[-1] + reason_code = last_edit.get("reason_code") + if reason_code: + last_edit["reason"] = EDIT_REASON_CODES.get(reason_code) + return last_edit class ThreadSerializer(_ContentSerializer): @@ -281,8 +311,9 @@ class ThreadSerializer(_ContentSerializer): read = serializers.BooleanField(required=False) has_endorsed = serializers.BooleanField(source="endorsed", read_only=True) response_count = serializers.IntegerField(source="resp_total", read_only=True, required=False) - close_reason_code = serializers.SerializerMethodField(required=False) - closed_by = serializers.SerializerMethodField(required=False) + close_reason_code = serializers.CharField(required=False, validators=[validate_close_reason_code]) + close_reason = serializers.SerializerMethodField() + closed_by = serializers.SerializerMethodField() non_updatable_fields = NON_UPDATABLE_THREAD_FIELDS @@ -374,12 +405,14 @@ def get_preview_body(self, obj): """ return Truncator(strip_tags(self.get_rendered_body(obj))).chars(35, ).replace('\n', ' ') - def get_close_reason_code(self, obj): + def get_close_reason(self, obj): """ Returns the reason for which the thread was closed. """ - if _validate_privileged_access(self.context): - return obj.get("close_reason_code") + if not _validate_privileged_access(self.context): + return None + reason_code = obj.get("close_reason_code") + return CLOSE_REASON_CODES.get(reason_code) def get_closed_by(self, obj): """ @@ -750,6 +783,14 @@ class BlackoutDateSerializer(serializers.Serializer): end = serializers.DateTimeField(help_text="The ISO 8601 timestamp for the end of the blackout period") +class ReasonCodeSeralizer(serializers.Serializer): + """ + Serializer for reason codes. + """ + code = serializers.CharField(help_text="A code for the an edit or close reason") + label = serializers.CharField(help_text="A user-friendly name text for the close or edit reason") + + class CourseMetadataSerailizer(serializers.Serializer): """ Serializer for course metadata. @@ -789,3 +830,11 @@ class CourseMetadataSerailizer(serializers.Serializer): group_at_subsection = serializers.BooleanField( help_text="A boolean indicating whether discussions should be grouped at subsection", ) + post_close_reasons = serializers.ListField( + child=ReasonCodeSeralizer(), + help_text="A list of reasons that can be specified by moderators for closing a post", + ) + edit_reasons = serializers.ListField( + child=ReasonCodeSeralizer(), + help_text="A list of reasons that can be specified by moderators for editing a post, response, or comment", + ) diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index 171a930541c1..bdfcb6b3185d 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -12,6 +12,7 @@ import ddt import httpretty import pytest +from django.test import override_settings from edx_toggles.toggles.testutils import override_waffle_flag from django.contrib.auth import get_user_model from django.core.exceptions import ValidationError @@ -153,6 +154,8 @@ def _set_course_discussion_blackout(course, user_id): @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) +@override_settings(DISCUSSION_MODERATION_EDIT_REASON_CODES={"test-edit-reason": "Test Edit Reason"}) +@override_settings(DISCUSSION_MODERATION_CLOSE_REASON_CODES={"test-close-reason": "Test Close Reason"}) @ddt.ddt class GetCourseTest(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase): """Test for get_course""" @@ -199,6 +202,9 @@ def test_basic(self): 'user_is_privileged': False, 'user_roles': {'Student'}, 'learners_tab_enabled': False, + 'reason_codes_enabled': False, + 'edit_reasons': [{'code': 'test-edit-reason', 'label': 'Test Edit Reason'}], + 'post_close_reasons': [{'code': 'test-close-reason', 'label': 'Test Close Reason'}], } @ddt.data( @@ -2825,6 +2831,9 @@ def test_invalid_field(self): FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_STUDENT, ) + @mock.patch("lms.djangoapps.discussion.rest_api.serializers.EDIT_REASON_CODES", { + "test-edit-reason": "Test Edit Reason", + }) def test_update_thread_with_edit_reason_code(self, role_name): """ Test editing comments, specifying and retrieving edit reason codes. @@ -2834,16 +2843,17 @@ def test_update_thread_with_edit_reason_code(self, role_name): try: result = update_thread(self.request, "test_thread", { "raw_body": "Edited body", - "edit_reason_code": "someReason", + "edit_reason_code": "test-edit-reason", }) assert role_name != FORUM_ROLE_STUDENT assert result["last_edit"] == { "original_body": "Original body", - "reason_code": "someReason", + "reason": "Test Edit Reason", + "reason_code": "test-edit-reason", "author": self.user.username, } request_body = httpretty.last_request().parsed_body # pylint: disable=no-member - assert request_body["edit_reason_code"] == ["someReason"] + 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."]} @@ -3237,6 +3247,9 @@ def test_abuse_flagged(self, old_flagged, new_flagged): FORUM_ROLE_COMMUNITY_TA, FORUM_ROLE_STUDENT, ) + @mock.patch("lms.djangoapps.discussion.rest_api.serializers.EDIT_REASON_CODES", { + "test-edit-reason": "Test Edit Reason", + }) def test_update_comment_with_edit_reason_code(self, role_name): """ Test editing comments, specifying and retrieving edit reason codes. @@ -3246,16 +3259,17 @@ def test_update_comment_with_edit_reason_code(self, role_name): try: result = update_comment(self.request, "test_comment", { "raw_body": "Edited body", - "edit_reason_code": "someReason", + "edit_reason_code": "test-edit-reason", }) assert role_name != FORUM_ROLE_STUDENT assert result["last_edit"] == { "original_body": "Original body", - "reason_code": "someReason", + "reason": "Test Edit Reason", + "reason_code": "test-edit-reason", "author": self.user.username, } request_body = httpretty.last_request().parsed_body # pylint: disable=no-member - assert request_body["edit_reason_code"] == ["someReason"] + assert request_body["edit_reason_code"] == ["test-edit-reason"] except ValidationError: assert role_name == FORUM_ROLE_STUDENT diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index d46c5f3fe092..e7ec8a28ef02 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -7,17 +7,18 @@ import random from datetime import datetime from unittest import mock -from urllib.parse import parse_qs, urlparse, urlencode +from urllib.parse import parse_qs, urlencode, urlparse import ddt import httpretty -from django.urls import reverse from django.core.files.uploadedfile import SimpleUploadedFile +from django.test import override_settings +from django.urls import reverse from opaque_keys.edx.keys import CourseKey from pytz import UTC +from rest_framework import status from rest_framework.parsers import JSONParser from rest_framework.test import APIClient, APITestCase -from rest_framework import status from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -27,11 +28,7 @@ from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.models import get_retired_username_by_username from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole, GlobalStaff -from common.djangoapps.student.tests.factories import ( - CourseEnrollmentFactory, - SuperuserFactory, - UserFactory, -) +from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, SuperuserFactory, UserFactory from common.djangoapps.util.testing import PatchMediaTypeMixin, UrlResetMixin from common.test.utils import disable_signal from lms.djangoapps.discussion.django_comment_client.tests.utils import ( @@ -483,6 +480,8 @@ def test_request_with_empty_results_page(self): @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) +@override_settings(DISCUSSION_MODERATION_EDIT_REASON_CODES={"test-edit-reason": "Test Edit Reason"}) +@override_settings(DISCUSSION_MODERATION_CLOSE_REASON_CODES={"test-close-reason": "Test Close Reason"}) class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for CourseView""" def setUp(self): @@ -512,14 +511,17 @@ def test_basic(self): "http://testserver/api/discussion/v1/threads/?course_id=course-v1%3Ax%2By%2Bz&following=True" ), "topics_url": "http://testserver/api/discussion/v1/course_topics/course-v1:x+y+z", - 'enable_in_context': True, - 'group_at_subsection': False, - 'provider': 'legacy', + "enable_in_context": True, + "group_at_subsection": False, + "provider": "legacy", "allow_anonymous": True, "allow_anonymous_to_peers": False, - 'user_is_privileged': False, - 'user_roles': ['Student'], + "user_is_privileged": False, + "user_roles": ["Student"], 'learners_tab_enabled': False, + "reason_codes_enabled": False, + "edit_reasons": [{"code": "test-edit-reason", "label": "Test Edit Reason"}], + "post_close_reasons": [{"code": "test-close-reason", "label": "Test Close Reason"}], } ) diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index 94a902953ca9..d22d049b1d56 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -487,6 +487,7 @@ def expected_thread_data(self, overrides=None): "response_count": 0, "last_edit": None, "closed_by": None, + "close_reason": None, "close_reason_code": None, } response_data.update(overrides or {}) diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index fe0bf2825eae..28fad72d5236 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -50,7 +50,7 @@ get_thread_list, get_user_comments, update_comment, - update_thread + update_thread, ) from ..rest_api.forms import ( CommentGetForm, diff --git a/lms/djangoapps/discussion/toggles.py b/lms/djangoapps/discussion/toggles.py index 3a3ee02126e6..6d2f8e39095e 100644 --- a/lms/djangoapps/discussion/toggles.py +++ b/lms/djangoapps/discussion/toggles.py @@ -32,3 +32,16 @@ # .. toggle_target_removal_date: 2022-05-21 # lint-amnesty, pylint: disable=line-too-long ENABLE_LEARNERS_TAB_IN_DISCUSSIONS_MFE = CourseWaffleFlag(WAFFLE_FLAG_NAMESPACE, 'enable_learners_tab_in_discussions_mfe', __name__) + +# .. toggle_name: discussions.enable_moderation_reason_codes +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Waffle flag to toggle support for the new edit and post close reason codes +# .. toggle_use_cases: temporary, open_edx +# .. toggle_creation_date: 2022-02-22 +# .. toggle_target_removal_date: 2022-09-22 +ENABLE_DISCUSSION_MODERATION_REASON_CODES = CourseWaffleFlag( + WAFFLE_FLAG_NAMESPACE, + 'enable_moderation_reason_codes', + __name__, +) diff --git a/lms/envs/common.py b/lms/envs/common.py index 3602943ca7ce..fbb44d1e3746 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -4788,8 +4788,8 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # waffle flag. ORA_GRADING_MICROFRONTEND_URL = None # .. setting_name: DISCUSSIONS_MICROFRONTEND_URL -# .. setting_default: None # .. setting_description: Base URL of the micro-frontend-based discussions page. +# .. setting_default: None # .. setting_warning: Also set site's courseware.discussions_mfe waffle flag. DISCUSSIONS_MICROFRONTEND_URL = None # .. setting_name: DISCUSSIONS_MFE_FEEDBACK_URL = None @@ -4969,3 +4969,24 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring # The expected value is an Integer representing the cutoff point (in months) for inclusion to the message. Example: # a value of `3` would include learners who have logged in within the past 3 months. BULK_COURSE_EMAIL_LAST_LOGIN_ELIGIBILITY_PERIOD = None + +################ Settings for the Discussion Service ######### +# Provide a list of reason codes for moderators editing posts and +# comments, as a mapping from the internal reason code representation, +# to an internationalizable label to be shown to moderators in the form UI. +DISCUSSION_MODERATION_EDIT_REASON_CODES = { + "grammar-spelling": _("Has grammar / spelling issues"), + "needs-clarity": _("Content needs clarity"), + "academic-integrity": _("Has academic integrity concern"), + "inappropriate-language": _("Has inappropriate language"), + "contains-pii": _("Contains personally identifiable information"), +} +# Provide a list of reason codes for moderators to close posts, as a mapping +# from the internal reason code representation, to an internationalizable label +# to be shown to moderators in the form UI. +DISCUSSION_MODERATION_CLOSE_REASON_CODES = { + "academic-integrity": _("Post violates honour code or academic integrity"), + "read-only": _("Post should be read-only"), + "duplicate": _("Post is a duplicate"), + "off-topic": _("Post is off-topic"), +}