From ef26e8e83fb42bf1d631ce683a16b7afe0ddd3dd Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 5 Jun 2015 15:05:26 -0400 Subject: [PATCH] Add comment endorsement to discussion API --- lms/djangoapps/discussion_api/api.py | 42 +++++++++--- lms/djangoapps/discussion_api/serializers.py | 7 +- .../discussion_api/tests/test_api.py | 65 ++++++++++++++++--- .../discussion_api/tests/test_serializers.py | 38 +++++++++-- 4 files changed, 128 insertions(+), 24 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 4f3565a056..9d2c0ade89 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -424,6 +424,20 @@ def _get_thread_editable_fields(cc_thread, context): return _THREAD_EDITABLE_BY_ANY +def _check_editable_fields(editable_fields, update_data): + """ + Raise ValidationError if the given update data contains a field that is not + in editable_fields. + """ + non_editable_errors = { + field: ["This field is not editable."] + for field in update_data.keys() + if field not in editable_fields + } + if non_editable_errors: + raise ValidationError(non_editable_errors) + + def update_thread(request, thread_id, update_data): """ Update a thread. @@ -444,13 +458,7 @@ def update_thread(request, thread_id, update_data): """ cc_thread, context = _get_thread_and_context(request, thread_id) editable_fields = _get_thread_editable_fields(cc_thread, context) - non_editable_errors = { - field: ["This field is not editable."] - for field in update_data.keys() - if field not in editable_fields - } - if non_editable_errors: - raise ValidationError(non_editable_errors) + _check_editable_fields(editable_fields, update_data) serializer = ThreadSerializer(cc_thread, data=update_data, partial=True, context=context) actions_form = ThreadActionsForm(update_data) if not (serializer.is_valid() and actions_form.is_valid()): @@ -463,6 +471,22 @@ def update_thread(request, thread_id, update_data): return api_thread +_COMMENT_EDITABLE_BY_AUTHOR = {"raw_body"} +_COMMENT_EDITABLE_BY_THREAD_AUTHOR = {"endorsed"} + + +def _get_comment_editable_fields(cc_comment, context): + """ + Get the list of editable fields for the given comment in the given context + """ + ret = set() + if _is_user_author_or_privileged(cc_comment, context): + ret |= _COMMENT_EDITABLE_BY_AUTHOR + if _is_user_author_or_privileged(context["thread"], context): + ret |= _COMMENT_EDITABLE_BY_THREAD_AUTHOR + return ret + + def update_comment(request, comment_id, update_data): """ Update a comment. @@ -493,8 +517,8 @@ def update_comment(request, comment_id, update_data): is empty or thread_id is included) """ cc_comment, context = _get_comment_and_context(request, comment_id) - if not _is_user_author_or_privileged(cc_comment, context): - raise PermissionDenied() + editable_fields = _get_comment_editable_fields(cc_comment, context) + _check_editable_fields(editable_fields, update_data) serializer = CommentSerializer(cc_comment, data=update_data, partial=True, context=context) if not serializer.is_valid(): raise ValidationError(serializer.errors) diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py index 5fdf775550..a875fc59f5 100644 --- a/lms/djangoapps/discussion_api/serializers.py +++ b/lms/djangoapps/discussion_api/serializers.py @@ -231,7 +231,7 @@ class CommentSerializer(_ContentSerializer): """ thread_id = serializers.CharField() parent_id = serializers.CharField(required=False) - endorsed = serializers.BooleanField(read_only=True) + endorsed = serializers.BooleanField(required=False) endorsed_by = serializers.SerializerMethodField("get_endorsed_by") endorsed_by_label = serializers.SerializerMethodField("get_endorsed_by_label") endorsed_at = serializers.SerializerMethodField("get_endorsed_at") @@ -300,6 +300,11 @@ class CommentSerializer(_ContentSerializer): if instance: for key, val in attrs.items(): instance[key] = val + # TODO: The comments service doesn't populate the endorsement + # field on comment creation, so we only provide + # endorsement_user_id on update + if key == "endorsed": + instance["endorsement_user_id"] = self.context["cc_requester"]["id"] return instance return Comment( course_id=self.context["thread"]["course_id"], diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 73354802b1..8d07d1557c 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -1787,22 +1787,67 @@ class UpdateCommentTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest except Http404: self.assertTrue(expected_error) - @ddt.data( - FORUM_ROLE_ADMINISTRATOR, - FORUM_ROLE_MODERATOR, - FORUM_ROLE_COMMUNITY_TA, - FORUM_ROLE_STUDENT, - ) - def test_role_access(self, role_name): + @ddt.data(*itertools.product( + [ + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ], + [True, False], + [True, False], + )) + @ddt.unpack + def test_raw_body_access(self, role_name, is_thread_author, is_comment_author): role = Role.objects.create(name=role_name, course_id=self.course.id) role.users = [self.user] - self.register_comment({"user_id": str(self.user.id + 1)}) - expected_error = role_name == FORUM_ROLE_STUDENT + self.register_comment( + {"user_id": str(self.user.id if is_comment_author else (self.user.id + 1))}, + thread_overrides={ + "user_id": str(self.user.id if is_thread_author else (self.user.id + 1)) + } + ) + expected_error = role_name == FORUM_ROLE_STUDENT and not is_comment_author try: update_comment(self.request, "test_comment", {"raw_body": "edited"}) self.assertFalse(expected_error) - except PermissionDenied: + except ValidationError as err: self.assertTrue(expected_error) + self.assertEqual( + err.message_dict, + {"raw_body": ["This field is not editable."]} + ) + + @ddt.data(*itertools.product( + [ + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ], + [True, False], + [True, False], + )) + @ddt.unpack + def test_endorsed_access(self, role_name, is_thread_author, is_comment_author): + role = Role.objects.create(name=role_name, course_id=self.course.id) + role.users = [self.user] + self.register_comment( + {"user_id": str(self.user.id if is_comment_author else (self.user.id + 1))}, + thread_overrides={ + "user_id": str(self.user.id if is_thread_author else (self.user.id + 1)) + } + ) + expected_error = role_name == FORUM_ROLE_STUDENT and not is_thread_author + try: + update_comment(self.request, "test_comment", {"endorsed": True}) + self.assertFalse(expected_error) + except ValidationError as err: + self.assertTrue(expected_error) + self.assertEqual( + err.message_dict, + {"endorsed": ["This field is not editable."]} + ) @ddt.ddt diff --git a/lms/djangoapps/discussion_api/tests/test_serializers.py b/lms/djangoapps/discussion_api/tests/test_serializers.py index 87863a3b5c..3d704b405e 100644 --- a/lms/djangoapps/discussion_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion_api/tests/test_serializers.py @@ -656,6 +656,27 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore {field: ["This field is required."]} ) + def test_create_endorsed(self): + # TODO: The comments service doesn't populate the endorsement field on + # comment creation, so this is sadly realistic + self.register_post_comment_response({}, thread_id="test_thread") + data = self.minimal_data.copy() + data["endorsed"] = True + saved = self.save_and_reserialize(data) + self.assertEqual( + httpretty.last_request().parsed_body, + { + "course_id": [unicode(self.course.id)], + "body": ["Test body"], + "user_id": [str(self.user.id)], + "endorsed": ["True"], + } + ) + self.assertTrue(saved["endorsed"]) + self.assertIsNone(saved["endorsed_by"]) + self.assertIsNone(saved["endorsed_by_label"]) + self.assertIsNone(saved["endorsed_at"]) + def test_update_empty(self): self.register_put_comment_response(self.existing_comment.attributes) self.save_and_reserialize({}, instance=self.existing_comment) @@ -672,8 +693,13 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore ) def test_update_all(self): - self.register_put_comment_response(self.existing_comment.attributes) - data = {"raw_body": "Edited body"} + cs_response_data = self.existing_comment.attributes.copy() + cs_response_data["endorsement"] = { + "user_id": str(self.user.id), + "time": "2015-06-05T00:00:00Z", + } + self.register_put_comment_response(cs_response_data) + data = {"raw_body": "Edited body", "endorsed": True} saved = self.save_and_reserialize(data, instance=self.existing_comment) self.assertEqual( httpretty.last_request().parsed_body, @@ -683,10 +709,14 @@ class CommentSerializerDeserializationTest(CommentsServiceMockMixin, ModuleStore "user_id": [str(self.user.id)], "anonymous": ["False"], "anonymous_to_peers": ["False"], - "endorsed": ["False"], + "endorsed": ["True"], + "endorsement_user_id": [str(self.user.id)], } ) - self.assertEqual(saved["raw_body"], data["raw_body"]) + for key in data: + self.assertEqual(saved[key], data[key]) + self.assertEqual(saved["endorsed_by"], self.user.username) + self.assertEqual(saved["endorsed_at"], "2015-06-05T00:00:00Z") def test_update_empty_raw_body(self): serializer = CommentSerializer(