diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 7a03dba870..5b28eba8cf 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -169,7 +169,7 @@ def get_comment_list(request, thread_id, endorsed, page, page_size): course_key = CourseLocator.from_string(cc_thread["course_id"]) course = _get_course_or_404(course_key, request.user) - context = get_context(course, request.user) + context = get_context(course, request.user, cc_thread) # Ensure user has access to the thread if not context["is_requester_privileged"] and cc_thread["group_id"]: diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py index db8f94c072..bd595f351a 100644 --- a/lms/djangoapps/discussion_api/serializers.py +++ b/lms/djangoapps/discussion_api/serializers.py @@ -1,6 +1,8 @@ """ Discussion API serializers """ +from django.contrib.auth.models import User as DjangoUser + from rest_framework import serializers from django_comment_common.models import ( @@ -9,14 +11,14 @@ from django_comment_common.models import ( FORUM_ROLE_MODERATOR, Role, ) -from lms.lib.comment_client.user import User +from lms.lib.comment_client.user import User as CommentClientUser from openedx.core.djangoapps.course_groups.cohorts import get_cohort_names -def get_context(course, requester): +def get_context(course, requester, thread=None): """ Returns a context appropriate for use with ThreadSerializer or - CommentSerializer. + (if thread is provided) CommentSerializer. """ # TODO: cache staff_user_ids and ta_user_ids if we need to improve perf staff_user_ids = { @@ -38,7 +40,8 @@ def get_context(course, requester): "is_requester_privileged": requester.id in staff_user_ids or requester.id in ta_user_ids, "staff_user_ids": staff_user_ids, "ta_user_ids": ta_user_ids, - "cc_requester": User.from_django_user(requester).retrieve(), + "cc_requester": CommentClientUser.from_django_user(requester).retrieve(), + "thread": thread, } @@ -60,6 +63,13 @@ class _ContentSerializer(serializers.Serializer): # name above and modify it here self.fields["id"] = self.fields.pop("id_") + def _is_user_privileged(self, user_id): + """ + 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"] + def _is_anonymous(self, obj): """ Returns a boolean indicating whether the content should be anonymous to @@ -156,12 +166,49 @@ class CommentSerializer(_ContentSerializer): """ thread_id = serializers.CharField() parent_id = serializers.SerializerMethodField("get_parent_id") + endorsed = serializers.BooleanField() + endorsed_by = serializers.SerializerMethodField("get_endorsed_by") + endorsed_by_label = serializers.SerializerMethodField("get_endorsed_by_label") + endorsed_at = serializers.SerializerMethodField("get_endorsed_at") children = serializers.SerializerMethodField("get_children") def get_parent_id(self, _obj): """Returns the comment's parent's id (taken from the context).""" return self.context.get("parent_id") + def get_endorsed_by(self, obj): + """ + Returns the username of the endorsing user, if the information is + available and would not identify the author of an anonymous thread. + """ + endorsement = obj.get("endorsement") + if endorsement: + endorser_id = int(endorsement["user_id"]) + # Avoid revealing the identity of an anonymous non-staff question + # author who has endorsed a comment in the thread + if not ( + self._is_anonymous(self.context["thread"]) and + not self._is_user_privileged(endorser_id) + ): + return DjangoUser.objects.get(id=endorser_id).username + return None + + def get_endorsed_by_label(self, obj): + """ + Returns the role label (i.e. "staff" or "community_ta") for the + endorsing user + """ + endorsement = obj.get("endorsement") + if endorsement: + return self._get_user_label(int(endorsement["user_id"])) + else: + return None + + def get_endorsed_at(self, obj): + """Returns the timestamp for the endorsement, if available.""" + endorsement = obj.get("endorsement") + return endorsement["time"] if endorsement else None + def get_children(self, obj): """Returns the list of the comment's children, serialized.""" child_context = dict(self.context) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index e55db83369..03a4268ad8 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -745,6 +745,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "created_at": "2015-05-11T00:00:00Z", "updated_at": "2015-05-11T11:11:11Z", "body": "Test body", + "endorsed": False, "abuse_flaggers": [], "votes": {"up_count": 4}, "children": [], @@ -759,6 +760,7 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "created_at": "2015-05-11T22:22:22Z", "updated_at": "2015-05-11T33:33:33Z", "body": "More content", + "endorsed": False, "abuse_flaggers": [str(self.user.id)], "votes": {"up_count": 7}, "children": [], @@ -774,6 +776,10 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "created_at": "2015-05-11T00:00:00Z", "updated_at": "2015-05-11T11:11:11Z", "raw_body": "Test body", + "endorsed": False, + "endorsed_by": None, + "endorsed_by_label": None, + "endorsed_at": None, "abuse_flagged": False, "voted": False, "vote_count": 4, @@ -788,6 +794,10 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "created_at": "2015-05-11T22:22:22Z", "updated_at": "2015-05-11T33:33:33Z", "raw_body": "More content", + "endorsed": False, + "endorsed_by": None, + "endorsed_by_label": None, + "endorsed_at": None, "abuse_flagged": True, "voted": False, "vote_count": 7, @@ -813,6 +823,22 @@ class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): non_endorsed_actual = self.get_comment_list(thread, endorsed=False) self.assertEqual(non_endorsed_actual["results"][0]["id"], "non_endorsed_comment") + def test_endorsed_by_anonymity(self): + """ + Ensure thread anonymity is properly considered in serializing + endorsed_by. + """ + thread = self.make_minimal_cs_thread({ + "anonymous": True, + "children": [ + make_minimal_cs_comment({ + "endorsement": {"user_id": str(self.author.id), "time": "2015-05-18T12:34:56Z"} + }) + ] + }) + actual_comments = self.get_comment_list(thread)["results"] + self.assertIsNone(actual_comments[0]["endorsed_by"]) + @ddt.data( ("discussion", None, "children", "resp_total"), ("question", False, "non_endorsed_responses", "non_endorsed_resp_total"), diff --git a/lms/djangoapps/discussion_api/tests/test_serializers.py b/lms/djangoapps/discussion_api/tests/test_serializers.py index 7426d39aba..b0d4d27de8 100644 --- a/lms/djangoapps/discussion_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion_api/tests/test_serializers.py @@ -1,6 +1,8 @@ """ Tests for Discussion API serializers """ +import itertools + import ddt import httpretty @@ -199,7 +201,12 @@ class ThreadSerializerTest(SerializerTestMixin, ModuleStoreTestCase): @ddt.ddt class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): """Tests for CommentSerializer.""" - def make_cs_content(self, overrides): + def setUp(self): + super(CommentSerializerTest, self).setUp() + self.endorser = UserFactory.create() + self.endorsed_at = "2015-05-18T12:34:56Z" + + def make_cs_content(self, overrides=None, with_endorsement=False): """ Create a comment with the given overrides, plus some useful test data. """ @@ -207,15 +214,21 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): "user_id": str(self.author.id), "username": self.author.username } - merged_overrides.update(overrides) + if with_endorsement: + merged_overrides["endorsement"] = { + "user_id": str(self.endorser.id), + "time": self.endorsed_at + } + merged_overrides.update(overrides or {}) return make_minimal_cs_comment(merged_overrides) - def serialize(self, comment): + def serialize(self, comment, thread_data=None): """ Create a serializer with an appropriate context and use it to serialize the given comment, returning the result. """ - return CommentSerializer(comment, context=get_context(self.course, self.user)).data + context = get_context(self.course, self.user, make_minimal_cs_thread(thread_data)) + return CommentSerializer(comment, context=context).data def test_basic(self): comment = { @@ -228,6 +241,7 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", "body": "Test body", + "endorsed": False, "abuse_flaggers": [], "votes": {"up_count": 4}, "children": [], @@ -241,6 +255,10 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", "raw_body": "Test body", + "endorsed": False, + "endorsed_by": None, + "endorsed_by_label": None, + "endorsed_at": None, "abuse_flagged": False, "voted": False, "vote_count": 4, @@ -248,6 +266,63 @@ class CommentSerializerTest(SerializerTestMixin, ModuleStoreTestCase): } self.assertEqual(self.serialize(comment), expected) + @ddt.data( + *itertools.product( + [ + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ], + [True, False] + ) + ) + @ddt.unpack + def test_endorsed_by(self, endorser_role_name, thread_anonymous): + """ + Test correctness of the endorsed_by field. + + The endorser should be anonymous iff the thread is anonymous to the + requester, and the endorser is not a privileged user. + + endorser_role_name is the name of the endorser's role. + thread_anonymous is the value of the anonymous field in the thread. + """ + self.create_role(endorser_role_name, [self.endorser]) + serialized = self.serialize( + self.make_cs_content(with_endorsement=True), + thread_data={"anonymous": thread_anonymous} + ) + actual_endorser_anonymous = serialized["endorsed_by"] is None + expected_endorser_anonymous = endorser_role_name == FORUM_ROLE_STUDENT and thread_anonymous + self.assertEqual(actual_endorser_anonymous, expected_endorser_anonymous) + + @ddt.data( + (FORUM_ROLE_ADMINISTRATOR, "staff"), + (FORUM_ROLE_MODERATOR, "staff"), + (FORUM_ROLE_COMMUNITY_TA, "community_ta"), + (FORUM_ROLE_STUDENT, None), + ) + @ddt.unpack + def test_endorsed_by_labels(self, role_name, expected_label): + """ + Test correctness of the endorsed_by_label field. + + The label should be "staff", "staff", or "community_ta" for the + Administrator, Moderator, and Community TA roles, respectively. + + role_name is the name of the author's role. + expected_label is the expected value of the author_label field in the + API output. + """ + self.create_role(role_name, [self.endorser]) + serialized = self.serialize(self.make_cs_content(with_endorsement=True)) + self.assertEqual(serialized["endorsed_by_label"], expected_label) + + def test_endorsed_at(self): + serialized = self.serialize(self.make_cs_content(with_endorsement=True)) + self.assertEqual(serialized["endorsed_at"], self.endorsed_at) + def test_children(self): comment = self.make_cs_content({ "id": "test_root", diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 1fea0ac3e3..ec5bed1b9a 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -240,6 +240,7 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "created_at": "2015-05-11T00:00:00Z", "updated_at": "2015-05-11T11:11:11Z", "body": "Test body", + "endorsed": False, "abuse_flaggers": [], "votes": {"up_count": 4}, "children": [], @@ -253,6 +254,10 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "created_at": "2015-05-11T00:00:00Z", "updated_at": "2015-05-11T11:11:11Z", "raw_body": "Test body", + "endorsed": False, + "endorsed_by": None, + "endorsed_by_label": None, + "endorsed_at": None, "abuse_flagged": False, "voted": True, "vote_count": 4, diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 8982abffcb..c3caace4a5 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -119,7 +119,6 @@ def make_minimal_cs_comment(overrides=None): "abuse_flaggers": [], "votes": {"up_count": 0}, "endorsed": False, - "endorsement": None, "children": [], } ret.update(overrides or {}) diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index f201a8f408..68fb136d12 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -174,6 +174,18 @@ class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): * raw_body: The comment's raw body text without any rendering applied + * endorsed: Boolean indicating whether the comment has been endorsed + (by a privileged user or, for a question thread, the thread + author) + + * endorsed_by: The username of the endorsing user, if available + + * endorsed_by_label: A label indicating whether the endorsing user + has a special role in the course (see author_label) + + * endorsed_at: The ISO 8601 timestamp for the endorsement, if + available + * abuse_flagged: Boolean indicating whether the requesting user has flagged the comment for abuse