From 6bfb741c4a72153bac03a46cf1ae099b51175c24 Mon Sep 17 00:00:00 2001 From: Felipe Trzaskowski Date: Wed, 16 Mar 2022 09:02:47 -0300 Subject: [PATCH] feat: add edit_reason_code and close_reason_code (#29609) Add `edit_reason_code` field to both `Comments` and `Threads`, making it editable for anyone who can also edit `raw_body`. Add `close_reason_code` field to `Threads`, and make it editable by anyone who can also edit `closed`. --- lms/djangoapps/discussion/rest_api/api.py | 2 +- .../discussion/rest_api/permissions.py | 2 + .../discussion/rest_api/serializers.py | 29 ++++++ .../discussion/rest_api/tests/test_api.py | 90 ++++++++++++++++++- .../rest_api/tests/test_permissions.py | 8 +- .../rest_api/tests/test_serializers.py | 1 + .../discussion/rest_api/tests/test_views.py | 4 + .../discussion/rest_api/tests/utils.py | 34 ++++++- lms/djangoapps/discussion/rest_api/views.py | 11 +++ .../comment_client/comment.py | 4 +- .../comment_client/thread.py | 7 +- 11 files changed, 178 insertions(+), 14 deletions(-) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index 07c2c723cf..ac1edff574 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -975,7 +975,7 @@ def _do_extra_actions(api_content, cc_content, request_fields, actions_form, con update that require a separate comments service request. """ for field, form_value in actions_form.cleaned_data.items(): - if field in request_fields and form_value != api_content[field]: + if field in request_fields and field in api_content and form_value != api_content[field]: api_content[field] = form_value if field == "following": _handle_following_field(form_value, context["cc_requester"], cc_content) diff --git a/lms/djangoapps/discussion/rest_api/permissions.py b/lms/djangoapps/discussion/rest_api/permissions.py index 90519a356b..ad81793b77 100644 --- a/lms/djangoapps/discussion/rest_api/permissions.py +++ b/lms/djangoapps/discussion/rest_api/permissions.py @@ -102,6 +102,7 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se 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, "read": is_thread, } @@ -114,6 +115,7 @@ def get_editable_fields(cc_content: Union[Thread, Comment], context: Dict) -> Se editable_fields.update({ "voted": True, "raw_body": is_privileged or is_author, + "edit_reason_code": is_privileged, "following": is_thread, "topic_id": is_thread and (is_author or is_privileged), "type": is_thread and (is_author or is_privileged), diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index cbd88ddb3f..365ce9fdf5 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -136,6 +136,8 @@ class _ContentSerializer(serializers.Serializer): can_delete = serializers.SerializerMethodField() 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) non_updatable_fields = set() @@ -238,6 +240,16 @@ class _ContentSerializer(serializers.Serializer): """ return can_delete(obj, self.context) + 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] + class ThreadSerializer(_ContentSerializer): """ @@ -269,6 +281,8 @@ 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) non_updatable_fields = NON_UPDATABLE_THREAD_FIELDS @@ -360,6 +374,21 @@ class ThreadSerializer(_ContentSerializer): """ return Truncator(strip_tags(self.get_rendered_body(obj))).chars(35, ).replace('\n', ' ') + def get_close_reason_code(self, obj): + """ + Returns the reason for which the thread was closed. + """ + if _validate_privileged_access(self.context): + return obj.get("close_reason_code") + + def get_closed_by(self, obj): + """ + Returns the username of the moderator who closed this thread, + only to other privileged users. + """ + if _validate_privileged_access(self.context): + return obj.get("closed_by") + def create(self, validated_data): thread = Thread(user_id=self.context["cc_requester"]["id"], **validated_data) thread.save() diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api.py b/lms/djangoapps/discussion/rest_api/tests/test_api.py index cb4871ef56..171a930541 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api.py @@ -1425,6 +1425,7 @@ class GetCommentListTest(ForumsEnableMixin, CommentsServiceMockMixin, SharedModu "can_delete": False, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, }, { "id": "test_comment_2", @@ -1450,6 +1451,7 @@ class GetCommentListTest(ForumsEnableMixin, CommentsServiceMockMixin, SharedModu "can_delete": False, "anonymous": True, "anonymous_to_peers": False, + "last_edit": None, }, ] actual_comments = self.get_comment_list( @@ -1879,8 +1881,19 @@ class CreateThreadTest( "comment_list_url": "http://testserver/api/discussion/v1/comments/?thread_id=test_id", "read": True, "editable_fields": [ - "abuse_flagged", "anonymous", "closed", "following", "pinned", - "raw_body", "read", "title", "topic_id", "type", "voted" + "abuse_flagged", + "anonymous", + "close_reason_code", + "closed", + "edit_reason_code", + "following", + "pinned", + "raw_body", + "read", + "title", + "topic_id", + "type", + "voted", ], }) assert actual == expected @@ -2169,6 +2182,7 @@ class CreateCommentTest( "can_delete": True, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, } assert actual == expected expected_url = ( @@ -2250,11 +2264,19 @@ class CreateCommentTest( "voted": False, "vote_count": 0, "children": [], - "editable_fields": ["abuse_flagged", "anonymous", "endorsed", "raw_body", "voted"], + "editable_fields": [ + "abuse_flagged", + "anonymous", + "edit_reason_code", + "endorsed", + "raw_body", + "voted", + ], "child_count": 0, "can_delete": True, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, } assert actual == expected expected_url = ( @@ -2455,6 +2477,7 @@ class UpdateThreadTest( MockSignalHandlerMixin ): """Tests for update_thread""" + @classmethod def setUpClass(cls): super().setUpClass() @@ -2463,6 +2486,7 @@ class UpdateThreadTest( @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): super().setUp() + httpretty.reset() httpretty.enable() self.addCleanup(httpretty.reset) @@ -2514,7 +2538,7 @@ class UpdateThreadTest( 'preview_body': 'Edited body', 'topic_id': 'original_topic', 'read': True, - 'title': 'Original Title' + 'title': 'Original Title', }) assert parsed_body(httpretty.last_request()) == { 'course_id': [str(self.course.id)], @@ -2795,6 +2819,35 @@ class UpdateThreadTest( update_thread(self.request, "test_thread", {"raw_body": ""}) assert assertion.value.message_dict == {'raw_body': ['This field may not be blank.']} + @ddt.data( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ) + def test_update_thread_with_edit_reason_code(self, role_name): + """ + 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() + try: + result = update_thread(self.request, "test_thread", { + "raw_body": "Edited body", + "edit_reason_code": "someReason", + }) + assert role_name != FORUM_ROLE_STUDENT + assert result["last_edit"] == { + "original_body": "Original body", + "reason_code": "someReason", + "author": self.user.username, + } + request_body = httpretty.last_request().parsed_body # pylint: disable=no-member + assert request_body["edit_reason_code"] == ["someReason"] + except ValidationError as error: + assert role_name == FORUM_ROLE_STUDENT + assert error.message_dict == {"edit_reason_code": ["This field is not editable."]} + @ddt.ddt @disable_signal(api, 'comment_edited') @@ -2893,6 +2946,7 @@ class UpdateCommentTest( "editable_fields": ["abuse_flagged", "anonymous", "raw_body", "voted"], "child_count": 0, "can_delete": True, + "last_edit": None, } assert actual == expected assert parsed_body(httpretty.last_request()) == { @@ -3177,6 +3231,34 @@ class UpdateCommentTest( assert httpretty.last_request().method == 'PUT' assert parsed_body(httpretty.last_request()) == {'user_id': [str(self.user.id)]} + @ddt.data( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ) + def test_update_comment_with_edit_reason_code(self, role_name): + """ + 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() + try: + result = update_comment(self.request, "test_comment", { + "raw_body": "Edited body", + "edit_reason_code": "someReason", + }) + assert role_name != FORUM_ROLE_STUDENT + assert result["last_edit"] == { + "original_body": "Original body", + "reason_code": "someReason", + "author": self.user.username, + } + request_body = httpretty.last_request().parsed_body # pylint: disable=no-member + assert request_body["edit_reason_code"] == ["someReason"] + except ValidationError: + assert role_name == FORUM_ROLE_STUDENT + @ddt.ddt @disable_signal(api, 'thread_deleted') diff --git a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py index d449cc8932..e85a5be5e8 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_permissions.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_permissions.py @@ -66,7 +66,7 @@ class GetInitializableFieldsTest(ModuleStoreTestCase): "abuse_flagged", "course_id", "following", "raw_body", "read", "title", "topic_id", "type", "voted" } if is_privileged: - expected |= {"closed", "pinned"} + expected |= {"closed", "pinned", "close_reason_code", "edit_reason_code"} if is_privileged and is_cohorted: expected |= {"group_id"} if allow_anonymous: @@ -87,6 +87,8 @@ class GetInitializableFieldsTest(ModuleStoreTestCase): 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 @@ -116,7 +118,7 @@ class GetEditableFieldsTest(ModuleStoreTestCase): actual = get_editable_fields(thread, context) expected = {"abuse_flagged", "following", "read", "voted"} if is_privileged: - expected |= {"closed", "pinned"} + 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: @@ -148,6 +150,8 @@ class GetEditableFieldsTest(ModuleStoreTestCase): ) actual = get_editable_fields(comment, context) expected = {"abuse_flagged", "voted"} + if is_privileged: + expected |= {"edit_reason_code"} if is_author or is_privileged: expected |= {"raw_body"} if (is_thread_author and thread_type == "question") 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 d25049ce60..a2d11548d4 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_serializers.py @@ -317,6 +317,7 @@ class CommentSerializerTest(SerializerTestMixin, SharedModuleStoreTestCase): "editable_fields": ["abuse_flagged", "voted"], "child_count": 0, "can_delete": False, + "last_edit": None, } assert self.serialize(comment) == expected diff --git a/lms/djangoapps/discussion/rest_api/tests/test_views.py b/lms/djangoapps/discussion/rest_api/tests/test_views.py index 402d0ab334..d46c5f3fe0 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_views.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_views.py @@ -1510,6 +1510,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pr "can_delete": True, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, } response_data.update(overrides or {}) return response_data @@ -1902,6 +1903,7 @@ class CommentViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "can_delete": True, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, } response = self.client.post( self.url, @@ -1992,6 +1994,7 @@ class CommentViewSetPartialUpdateTest(DiscussionAPIViewTestMixin, ModuleStoreTes "can_delete": True, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, } response_data.update(overrides or {}) return response_data @@ -2179,6 +2182,7 @@ class CommentViewSetRetrieveTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase "can_delete": True, "anonymous": False, "anonymous_to_peers": False, + "last_edit": None, } response = self.client.get(self.url) diff --git a/lms/djangoapps/discussion/rest_api/tests/utils.py b/lms/djangoapps/discussion/rest_api/tests/utils.py index fc7c917a66..94a902953c 100644 --- a/lms/djangoapps/discussion/rest_api/tests/utils.py +++ b/lms/djangoapps/discussion/rest_api/tests/utils.py @@ -31,10 +31,19 @@ def _get_thread_callback(thread_data): additional required fields. """ response_data = make_minimal_cs_thread(thread_data) + original_data = response_data.copy() for key, val_list in parsed_body(request).items(): val = val_list[0] if key in ["anonymous", "anonymous_to_peers", "closed", "pinned"]: response_data[key] = val == "True" + elif key == "edit_reason_code": + response_data["edit_history"] = [ + { + "original_body": original_data["body"], + "author": thread_data.get('username'), + "reason_code": val, + }, + ] else: response_data[key] = val return (200, headers, json.dumps(response_data)) @@ -53,6 +62,7 @@ def _get_comment_callback(comment_data, thread_id, parent_id): Simulate the comment creation or update endpoint as described above. """ response_data = make_minimal_cs_comment(comment_data) + original_data = response_data.copy() # thread_id and parent_id are not included in request payload but # are returned by the comments service response_data["thread_id"] = thread_id @@ -61,6 +71,14 @@ def _get_comment_callback(comment_data, thread_id, parent_id): val = val_list[0] if key in ["anonymous", "anonymous_to_peers", "endorsed"]: response_data[key] = val == "True" + elif key == "edit_reason_code": + response_data["edit_history"] = [ + { + "original_body": original_data["body"], + "author": comment_data.get('username'), + "reason_code": val, + }, + ] else: response_data[key] = val return (200, headers, json.dumps(response_data)) @@ -438,8 +456,15 @@ class CommentsServiceMockMixin: "voted": False, "vote_count": 0, "editable_fields": [ - "abuse_flagged", "anonymous", "following", "raw_body", "read", - "title", "topic_id", "type", "voted" + "abuse_flagged", + "anonymous", + "following", + "raw_body", + "read", + "title", + "topic_id", + "type", + "voted", ], "course_id": str(self.course.id), "topic_id": "test_topic", @@ -460,6 +485,9 @@ class CommentsServiceMockMixin: "id": "test_thread", "type": "discussion", "response_count": 0, + "last_edit": None, + "closed_by": None, + "close_reason_code": None, } response_data.update(overrides or {}) return response_data @@ -497,6 +525,8 @@ def make_minimal_cs_thread(overrides=None): "read": False, "endorsed": False, "resp_total": 0, + "closed_by": None, + "close_reason_code": None, } ret.update(overrides or {}) return ret diff --git a/lms/djangoapps/discussion/rest_api/views.py b/lms/djangoapps/discussion/rest_api/views.py index 6501dad470..fe0bf2825e 100644 --- a/lms/djangoapps/discussion/rest_api/views.py +++ b/lms/djangoapps/discussion/rest_api/views.py @@ -401,6 +401,14 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet): * read (optional): A boolean to mark thread as read + * closed (optional, privileged): A boolean to mark thread as closed. + + * edit_reason_code (optional, privileged): A string containing a reason + code for editing the thread's body. + + * close_reason_code (optional, privileged): A string containing a reason + code for closing the thread. + * topic_id, type, title, raw_body, anonymous, and anonymous_to_peers are accepted with the same meaning as in a POST request @@ -630,6 +638,9 @@ class CommentViewSet(DeveloperErrorViewMixin, ViewSet): * raw_body, anonymous and anonymous_to_peers are accepted with the same meaning as in a POST request + * edit_reason_code (optional, privileged): A string containing a reason + code for a moderator to edit the comment. + If "application/merge-patch+json" is not the specified content type, a 415 error is returned. diff --git a/openedx/core/djangoapps/django_comment_common/comment_client/comment.py b/openedx/core/djangoapps/django_comment_common/comment_client/comment.py index ef3e72d04d..c03784ab04 100644 --- a/openedx/core/djangoapps/django_comment_common/comment_client/comment.py +++ b/openedx/core/djangoapps/django_comment_common/comment_client/comment.py @@ -14,12 +14,12 @@ class Comment(models.Model): 'endorsed', 'parent_id', 'thread_id', 'username', 'votes', 'user_id', 'closed', 'created_at', 'updated_at', 'depth', 'at_position_list', 'type', 'commentable_id', 'abuse_flaggers', 'endorsement', - 'child_count', + 'child_count', 'edit_history', ] updatable_fields = [ 'body', 'anonymous', 'anonymous_to_peers', 'course_id', 'closed', - 'user_id', 'endorsed', 'endorsement_user_id', + 'user_id', 'endorsed', 'endorsement_user_id', 'edit_reason_code', ] initializable_fields = updatable_fields diff --git a/openedx/core/djangoapps/django_comment_common/comment_client/thread.py b/openedx/core/djangoapps/django_comment_common/comment_client/thread.py index 0bb3378fcf..72642427d5 100644 --- a/openedx/core/djangoapps/django_comment_common/comment_client/thread.py +++ b/openedx/core/djangoapps/django_comment_common/comment_client/thread.py @@ -21,19 +21,20 @@ class Thread(models.Model): 'highlighted_body', 'endorsed', 'read', 'group_id', 'group_name', 'pinned', 'abuse_flaggers', 'resp_skip', 'resp_limit', 'resp_total', 'thread_type', 'endorsed_responses', 'non_endorsed_responses', 'non_endorsed_resp_total', - 'context', 'last_activity_at', + 'context', 'last_activity_at', 'closed_by', 'close_reason_code', 'edit_history', ] # updateable_fields are sent in PUT requests updatable_fields = [ 'title', 'body', 'anonymous', 'anonymous_to_peers', 'course_id', 'read', - 'closed', 'user_id', 'commentable_id', 'group_id', 'group_name', 'pinned', 'thread_type' + 'closed', 'user_id', 'commentable_id', 'group_id', 'group_name', 'pinned', 'thread_type', + 'close_reason_code', 'edit_reason_code', ] # metric_tag_fields are used by Datadog to record metrics about the model metric_tag_fields = [ 'course_id', 'group_id', 'pinned', 'closed', 'anonymous', 'anonymous_to_peers', - 'endorsed', 'read' + 'endorsed', 'read', ] # initializable_fields are sent in POST requests