From 7309352ef739dc79b1f3a1a99b1e68bca2cdb4e3 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 14 May 2015 17:50:10 -0400 Subject: [PATCH] Refactor discussion API to use DRF serializer This will make it easier to add the creation and update interfaces. --- lms/djangoapps/discussion_api/api.py | 104 +-------- lms/djangoapps/discussion_api/serializers.py | 134 +++++++++++ .../discussion_api/tests/test_api.py | 147 +----------- .../discussion_api/tests/test_serializers.py | 209 ++++++++++++++++++ 4 files changed, 353 insertions(+), 241 deletions(-) create mode 100644 lms/djangoapps/discussion_api/serializers.py create mode 100644 lms/djangoapps/discussion_api/tests/test_serializers.py diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 09fad100d9..b36ab441b2 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -7,16 +7,10 @@ from collections import defaultdict from courseware.courses import get_course_with_access from discussion_api.pagination import get_paginated_data +from discussion_api.serializers import ThreadSerializer, get_context from django_comment_client.utils import get_accessible_discussion_modules -from django_comment_common.models import ( - FORUM_ROLE_ADMINISTRATOR, - FORUM_ROLE_COMMUNITY_TA, - FORUM_ROLE_MODERATOR, - Role, -) from lms.lib.comment_client.thread import Thread -from lms.lib.comment_client.user import User -from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id, get_cohort_names +from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id from xmodule.tabs import DiscussionTab @@ -92,67 +86,6 @@ def get_course_topics(course_key, user): } -def _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids, group_ids_to_names): - """ - Convert a thread data dict from the comment_client format (which is a direct - representation of the format returned by the comments service) to the format - used in this API - - Arguments: - thread (comment_client.thread.Thread): The thread to convert - cc_user (comment_client.user.User): The comment_client representation of - the requesting user - staff_user_ids (set): The set of user ids for users with the Moderator or - Administrator role in the course - ta_user_ids (set): The set of user ids for users with the Community TA - role in the course - group_ids_to_names (dict): A mapping of group ids to names - - Returns: - dict: The discussion_api format representation of the thread. - """ - is_anonymous = ( - thread["anonymous"] or - ( - thread["anonymous_to_peers"] and - int(cc_user["id"]) not in (staff_user_ids | ta_user_ids) - ) - ) - ret = { - key: thread[key] - for key in [ - "id", - "course_id", - "group_id", - "created_at", - "updated_at", - "title", - "pinned", - "closed", - ] - } - ret.update({ - "topic_id": thread["commentable_id"], - "group_name": group_ids_to_names.get(thread["group_id"]), - "author": None if is_anonymous else thread["username"], - "author_label": ( - None if is_anonymous else - "staff" if int(thread["user_id"]) in staff_user_ids else - "community_ta" if int(thread["user_id"]) in ta_user_ids else - None - ), - "type": thread["thread_type"], - "raw_body": thread["body"], - "following": thread["id"] in cc_user["subscribed_thread_ids"], - "abuse_flagged": cc_user["id"] in thread["abuse_flaggers"], - "voted": thread["id"] in cc_user["upvoted_ids"], - "vote_count": thread["votes"]["up_count"], - "comment_count": thread["comments_count"], - "unread_comment_count": thread["unread_comments_count"], - }) - return ret - - def get_thread_list(request, course_key, page, page_size): """ Return the list of all discussion threads pertaining to the given course @@ -170,15 +103,13 @@ def get_thread_list(request, course_key, page, page_size): discussion_api.views.ThreadViewSet for more detail. """ course = _get_course_or_404(course_key, request.user) - user_is_privileged = Role.objects.filter( - course_id=course.id, - name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA], - users=request.user - ).exists() - cc_user = User.from_django_user(request.user).retrieve() + context = get_context(course, request.user) threads, result_page, num_pages, _ = Thread.search({ "course_id": unicode(course.id), - "group_id": None if user_is_privileged else get_cohort_id(request.user, course.id), + "group_id": ( + None if context["is_requester_privileged"] else + get_cohort_id(request.user, course.id) + ), "sort_key": "date", "sort_order": "desc", "page": page, @@ -189,25 +120,6 @@ def get_thread_list(request, course_key, page, page_size): # behavior and return a 404 in that case if result_page != page: raise Http404 - # TODO: cache staff_user_ids and ta_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() - } - ta_user_ids = { - user.id - for role in Role.objects.filter(name=FORUM_ROLE_COMMUNITY_TA, course_id=course.id) - for user in role.users.all() - } - # For now, the only groups are cohorts - group_ids_to_names = get_cohort_names(course) - results = [ - _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids, group_ids_to_names) - for thread in threads - ] + results = [ThreadSerializer(thread, context=context).data for thread in threads] return get_paginated_data(request, results, page, num_pages) diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py new file mode 100644 index 0000000000..727ea12b2e --- /dev/null +++ b/lms/djangoapps/discussion_api/serializers.py @@ -0,0 +1,134 @@ +""" +Discussion API serializers +""" +from rest_framework import serializers + +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_MODERATOR, + Role, +) +from lms.lib.comment_client.user import User +from openedx.core.djangoapps.course_groups.cohorts import get_cohort_names + + +def get_context(course, requester): + """Returns a context appropriate for use with ThreadSerializer.""" + # TODO: cache staff_user_ids and ta_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() + } + ta_user_ids = { + user.id + for role in Role.objects.filter(name=FORUM_ROLE_COMMUNITY_TA, course_id=course.id) + for user in role.users.all() + } + return { + # For now, the only groups are cohorts + "group_ids_to_names": get_cohort_names(course), + "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(), + } + + +class ThreadSerializer(serializers.Serializer): + """ + A serializer for thread data. + + N.B. This should not be used with a comment_client Thread object that has + not had retrieve() called, because of the interaction between DRF's attempts + at introspection and Thread's __getattr__. + """ + id_ = serializers.CharField(read_only=True) + course_id = serializers.CharField() + topic_id = serializers.CharField(source="commentable_id") + group_id = serializers.IntegerField() + group_name = serializers.SerializerMethodField("get_group_name") + author = serializers.SerializerMethodField("get_author") + author_label = serializers.SerializerMethodField("get_author_label") + created_at = serializers.CharField(read_only=True) + updated_at = serializers.CharField(read_only=True) + type_ = serializers.ChoiceField(source="thread_type", choices=("discussion", "question")) + title = serializers.CharField() + raw_body = serializers.CharField(source="body") + pinned = serializers.BooleanField() + closed = serializers.BooleanField() + following = serializers.SerializerMethodField("get_following") + abuse_flagged = serializers.SerializerMethodField("get_abuse_flagged") + voted = serializers.SerializerMethodField("get_voted") + vote_count = serializers.SerializerMethodField("get_vote_count") + comment_count = serializers.IntegerField(source="comments_count") + unread_comment_count = serializers.IntegerField(source="unread_comments_count") + + def __init__(self, *args, **kwargs): + super(ThreadSerializer, self).__init__(*args, **kwargs) + # type and id are invalid class attribute names, so we must declare + # different names above and modify them here + self.fields["id"] = self.fields.pop("id_") + self.fields["type"] = self.fields.pop("type_") + + def get_group_name(self, obj): + """Returns the name of the group identified by the thread's group_id.""" + return self.context["group_ids_to_names"].get(obj["group_id"]) + + def _is_anonymous(self, obj): + """ + Returns a boolean indicating whether the thread should be anonymous to + the requester. + """ + return ( + obj["anonymous"] or + obj["anonymous_to_peers"] and not self.context["is_requester_privileged"] + ) + + def get_author(self, obj): + """Returns the author's username, or None if the thread is anonymous.""" + return None if self._is_anonymous(obj) else obj["username"] + + def _get_user_label(self, user_id): + """ + Returns the role label (i.e. "staff" or "community_ta") for the user + with the given id. + """ + return ( + "staff" if user_id in self.context["staff_user_ids"] else + "community_ta" if user_id in self.context["ta_user_ids"] else + None + ) + + def get_author_label(self, obj): + """Returns the role label for the thread author.""" + return None if self._is_anonymous(obj) else self._get_user_label(int(obj["user_id"])) + + def get_following(self, obj): + """ + Returns a boolean indicating whether the requester is following the + thread. + """ + return obj["id"] in self.context["cc_requester"]["subscribed_thread_ids"] + + def get_abuse_flagged(self, obj): + """ + Returns a boolean indicating whether the requester has flagged the + thread as abusive. + """ + return self.context["cc_requester"]["id"] in obj["abuse_flaggers"] + + def get_voted(self, obj): + """ + Returns a boolean indicating whether the requester has voted for the + thread. + """ + return obj["id"] in self.context["cc_requester"]["upvoted_ids"] + + def get_vote_count(self, obj): + """Returns the number of votes for the thread.""" + return obj["votes"]["up_count"] diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index ccd164c266..604f7c2391 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -384,35 +384,6 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): role.users = users role.save() - def make_cs_thread(self, thread_data): - """ - Create a dictionary containing all needed thread fields as returned by - the comments service with dummy data overridden by thread_data - """ - ret = { - "id": "dummy", - "course_id": unicode(self.course.id), - "commentable_id": "dummy", - "group_id": None, - "user_id": str(self.author.id), - "username": self.author.username, - "anonymous": False, - "anonymous_to_peers": False, - "created_at": "1970-01-01T00:00:00Z", - "updated_at": "1970-01-01T00:00:00Z", - "thread_type": "discussion", - "title": "dummy", - "body": "dummy", - "pinned": False, - "closed": False, - "abuse_flaggers": [], - "votes": {"up_count": 0}, - "comments_count": 0, - "unread_comments_count": 0, - } - ret.update(thread_data) - return ret - def test_nonexistent_course(self): with self.assertRaises(Http404): get_thread_list(self.request, CourseLocator.from_string("non/existent/course"), 1, 1) @@ -449,11 +420,6 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): }) def test_thread_content(self): - self.register_get_user_response( - self.user, - subscribed_thread_ids=["test_thread_id_0"], - upvoted_ids=["test_thread_id_1"] - ) source_threads = [ { "id": "test_thread_id_0", @@ -497,27 +463,6 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "comments_count": 18, "unread_comments_count": 0, }, - { - "id": "test_thread_id_2", - "course_id": unicode(self.course.id), - "commentable_id": "topic_x", - "group_id": self.cohort.id + 1, # non-existent group - "user_id": str(self.author.id), - "username": self.author.username, - "anonymous": False, - "anonymous_to_peers": False, - "created_at": "2015-04-28T00:44:44Z", - "updated_at": "2015-04-28T00:55:55Z", - "thread_type": "discussion", - "title": "Yet Another Test Title", - "body": "Still more content", - "pinned": True, - "closed": False, - "abuse_flaggers": [str(self.user.id)], - "votes": {"up_count": 0}, - "comments_count": 0, - "unread_comments_count": 0, - }, ] expected_threads = [ { @@ -535,7 +480,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "raw_body": "Test body", "pinned": False, "closed": False, - "following": True, + "following": False, "abuse_flagged": False, "voted": False, "vote_count": 4, @@ -559,33 +504,11 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "closed": True, "following": False, "abuse_flagged": False, - "voted": True, + "voted": False, "vote_count": 9, "comment_count": 18, "unread_comment_count": 0, }, - { - "id": "test_thread_id_2", - "course_id": unicode(self.course.id), - "topic_id": "topic_x", - "group_id": self.cohort.id + 1, - "group_name": None, - "author": self.author.username, - "author_label": None, - "created_at": "2015-04-28T00:44:44Z", - "updated_at": "2015-04-28T00:55:55Z", - "type": "discussion", - "title": "Yet Another Test Title", - "raw_body": "Still more content", - "pinned": True, - "closed": False, - "following": False, - "abuse_flagged": True, - "voted": False, - "vote_count": 0, - "comment_count": 0, - "unread_comment_count": 0, - }, ] self.assertEqual( self.get_thread_list(source_threads), @@ -650,69 +573,3 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): self.register_get_threads_response([], page=3, num_pages=3) with self.assertRaises(Http404): get_thread_list(self.request, self.course.id, page=4, page_size=10) - - @ddt.data( - (FORUM_ROLE_ADMINISTRATOR, True, False, True), - (FORUM_ROLE_ADMINISTRATOR, False, True, False), - (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, False), - (FORUM_ROLE_STUDENT, True, False, True), - (FORUM_ROLE_STUDENT, False, True, True), - ) - @ddt.unpack - def test_anonymity(self, role_name, anonymous, anonymous_to_peers, expected_api_anonymous): - """ - Test that a thread is properly made anonymous. - - A thread should be anonymous iff the anonymous field is true or the - anonymous_to_peers field is true and the requester does not have a - privileged role. - - role_name is the name of the requester's role. - thread_anon is the value of the anonymous field in the thread data. - thread_anon_to_peers is the value of the anonymous_to_peers field in the - thread data. - expected_api_anonymous is whether the thread should actually be - anonymous in the API output when requested by a user with the given - role. - """ - self.create_role(role_name, [self.user]) - result = self.get_thread_list([ - self.make_cs_thread({ - "anonymous": anonymous, - "anonymous_to_peers": anonymous_to_peers, - }) - ]) - actual_api_anonymous = result["results"][0]["author"] is None - self.assertEqual(actual_api_anonymous, expected_api_anonymous) - - @ddt.data( - (FORUM_ROLE_ADMINISTRATOR, False, "staff"), - (FORUM_ROLE_ADMINISTRATOR, True, None), - (FORUM_ROLE_MODERATOR, False, "staff"), - (FORUM_ROLE_MODERATOR, True, None), - (FORUM_ROLE_COMMUNITY_TA, False, "community_ta"), - (FORUM_ROLE_COMMUNITY_TA, True, None), - (FORUM_ROLE_STUDENT, False, None), - (FORUM_ROLE_STUDENT, True, None), - ) - @ddt.unpack - def test_author_labels(self, role_name, anonymous, expected_label): - """ - Test correctness of the author_label field. - - The label should be "staff", "staff", or "community_ta" for the - Administrator, Moderator, and Community TA roles, respectively, but - the label should not be present if the thread is anonymous. - - role_name is the name of the author's role. - anonymous is the value of the anonymous field in the thread data. - expected_label is the expected value of the author_label field in the - API output. - """ - self.create_role(role_name, [self.author]) - result = self.get_thread_list([self.make_cs_thread({"anonymous": anonymous})]) - actual_label = result["results"][0]["author_label"] - self.assertEqual(actual_label, expected_label) diff --git a/lms/djangoapps/discussion_api/tests/test_serializers.py b/lms/djangoapps/discussion_api/tests/test_serializers.py new file mode 100644 index 0000000000..92ee95c6ad --- /dev/null +++ b/lms/djangoapps/discussion_api/tests/test_serializers.py @@ -0,0 +1,209 @@ +""" +Tests for Discussion API serializers +""" +import ddt +import httpretty + +from discussion_api.serializers import ThreadSerializer, get_context +from discussion_api.tests.utils import CommentsServiceMockMixin +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_STUDENT, + Role, +) +from student.tests.factories import UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory +from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory + + +@ddt.ddt +class ThreadSerializerTest(CommentsServiceMockMixin, ModuleStoreTestCase): + """Tests for ThreadSerializer.""" + def setUp(self): + super(ThreadSerializerTest, self).setUp() + httpretty.reset() + httpretty.enable() + self.addCleanup(httpretty.disable) + self.maxDiff = None # pylint: disable=invalid-name + self.user = UserFactory.create() + self.register_get_user_response(self.user) + self.course = CourseFactory.create() + self.author = UserFactory.create() + + def create_role(self, role_name, users, course=None): + """Create a Role in self.course with the given name and users""" + course = course or self.course + role = Role.objects.create(name=role_name, course_id=course.id) + role.users = users + + def make_cs_thread(self, thread_data=None): + """ + Create a dictionary containing all needed thread fields as returned by + the comments service with dummy data overridden by thread_data + """ + ret = { + "id": "dummy", + "course_id": unicode(self.course.id), + "commentable_id": "dummy", + "group_id": None, + "user_id": str(self.author.id), + "username": self.author.username, + "anonymous": False, + "anonymous_to_peers": False, + "created_at": "1970-01-01T00:00:00Z", + "updated_at": "1970-01-01T00:00:00Z", + "thread_type": "discussion", + "title": "dummy", + "body": "dummy", + "pinned": False, + "closed": False, + "abuse_flaggers": [], + "votes": {"up_count": 0}, + "comments_count": 0, + "unread_comments_count": 0, + "children": [], + "resp_total": 0, + } + if thread_data: + ret.update(thread_data) + return ret + + def serialize(self, thread): + """ + Create a serializer with an appropriate context and use it to serialize + the given thread, returning the result. + """ + return ThreadSerializer(thread, context=get_context(self.course, self.user)).data + + def test_basic(self): + thread = { + "id": "test_thread", + "course_id": unicode(self.course.id), + "commentable_id": "test_topic", + "group_id": None, + "user_id": str(self.author.id), + "username": self.author.username, + "anonymous": False, + "anonymous_to_peers": False, + "created_at": "2015-04-28T00:00:00Z", + "updated_at": "2015-04-28T11:11:11Z", + "thread_type": "discussion", + "title": "Test Title", + "body": "Test body", + "pinned": True, + "closed": False, + "abuse_flaggers": [], + "votes": {"up_count": 4}, + "comments_count": 5, + "unread_comments_count": 3, + } + expected = { + "id": "test_thread", + "course_id": unicode(self.course.id), + "topic_id": "test_topic", + "group_id": None, + "group_name": None, + "author": self.author.username, + "author_label": None, + "created_at": "2015-04-28T00:00:00Z", + "updated_at": "2015-04-28T11:11:11Z", + "type": "discussion", + "title": "Test Title", + "raw_body": "Test body", + "pinned": True, + "closed": False, + "following": False, + "abuse_flagged": False, + "voted": False, + "vote_count": 4, + "comment_count": 5, + "unread_comment_count": 3, + } + self.assertEqual(self.serialize(thread), expected) + + def test_group(self): + cohort = CohortFactory.create(course_id=self.course.id) + serialized = self.serialize(self.make_cs_thread({"group_id": cohort.id})) + self.assertEqual(serialized["group_id"], cohort.id) + self.assertEqual(serialized["group_name"], cohort.name) + + @ddt.data( + (FORUM_ROLE_ADMINISTRATOR, True, False, True), + (FORUM_ROLE_ADMINISTRATOR, False, True, False), + (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, False), + (FORUM_ROLE_STUDENT, True, False, True), + (FORUM_ROLE_STUDENT, False, True, True), + ) + @ddt.unpack + def test_anonymity(self, role_name, anonymous, anonymous_to_peers, expected_serialized_anonymous): + """ + Test that content is properly made anonymous. + + Content should be anonymous iff the anonymous field is true or the + anonymous_to_peers field is true and the requester does not have a + privileged role. + + role_name is the name of the requester's role. + anonymous is the value of the anonymous field in the content. + anonymous_to_peers is the value of the anonymous_to_peers field in the + content. + expected_serialized_anonymous is whether the content should actually be + anonymous in the API output when requested by a user with the given + role. + """ + self.create_role(role_name, [self.user]) + serialized = self.serialize( + self.make_cs_thread({"anonymous": anonymous, "anonymous_to_peers": anonymous_to_peers}) + ) + actual_serialized_anonymous = serialized["author"] is None + self.assertEqual(actual_serialized_anonymous, expected_serialized_anonymous) + + @ddt.data( + (FORUM_ROLE_ADMINISTRATOR, False, "staff"), + (FORUM_ROLE_ADMINISTRATOR, True, None), + (FORUM_ROLE_MODERATOR, False, "staff"), + (FORUM_ROLE_MODERATOR, True, None), + (FORUM_ROLE_COMMUNITY_TA, False, "community_ta"), + (FORUM_ROLE_COMMUNITY_TA, True, None), + (FORUM_ROLE_STUDENT, False, None), + (FORUM_ROLE_STUDENT, True, None), + ) + @ddt.unpack + def test_author_labels(self, role_name, anonymous, expected_label): + """ + Test correctness of the author_label field. + + The label should be "staff", "staff", or "community_ta" for the + Administrator, Moderator, and Community TA roles, respectively, but + the label should not be present if the thread is anonymous. + + role_name is the name of the author's role. + anonymous is the value of the anonymous field in the content. + expected_label is the expected value of the author_label field in the + API output. + """ + self.create_role(role_name, [self.author]) + serialized = self.serialize(self.make_cs_thread({"anonymous": anonymous})) + self.assertEqual(serialized["author_label"], expected_label) + + def test_following(self): + thread_id = "test_thread" + self.register_get_user_response(self.user, subscribed_thread_ids=[thread_id]) + serialized = self.serialize(self.make_cs_thread({"id": thread_id})) + self.assertEqual(serialized["following"], True) + + def test_abuse_flagged(self): + serialized = self.serialize(self.make_cs_thread({"abuse_flaggers": [str(self.user.id)]})) + self.assertEqual(serialized["abuse_flagged"], True) + + def test_voted(self): + thread_id = "test_thread" + self.register_get_user_response(self.user, upvoted_ids=[thread_id]) + serialized = self.serialize(self.make_cs_thread({"id": thread_id})) + self.assertEqual(serialized["voted"], True)