From fa6e5338c238b6b895d7c0b8d4b390bcdc26d06e Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 4 May 2015 17:13:55 -0400 Subject: [PATCH 1/3] Add fields to discussion thread list endpoint This commit adds fields that are related to the requesting user's interaction with the thread (e.g. following). --- lms/djangoapps/discussion_api/api.py | 10 +++++-- .../discussion_api/tests/test_api.py | 28 ++++++++++++++++++- .../discussion_api/tests/test_views.py | 8 ++++++ lms/djangoapps/discussion_api/tests/utils.py | 13 +++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index fd35f9a91c..071fba0545 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -14,6 +14,7 @@ from django_comment_common.models import ( 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 @@ -76,7 +77,7 @@ def get_course_topics(course, user): } -def _cc_thread_to_api_thread(thread): +def _cc_thread_to_api_thread(thread, cc_user): """ 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 @@ -98,6 +99,10 @@ def _cc_thread_to_api_thread(thread): ret.update({ "topic_id": thread["commentable_id"], "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"], }) @@ -125,6 +130,7 @@ def get_thread_list(request, course_key, page, page_size): 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() threads, result_page, num_pages, _ = Thread.search({ "course_id": unicode(course_key), "group_id": None if user_is_privileged else get_cohort_id(request.user, course_key), @@ -139,5 +145,5 @@ def get_thread_list(request, course_key, page, page_size): if result_page != page: raise Http404 - results = [_cc_thread_to_api_thread(thread) for thread in threads] + results = [_cc_thread_to_api_thread(thread, cc_user) for thread in threads] return get_paginated_data(request, results, page, num_pages) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 643f7eb51c..7b5a729d9a 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -323,13 +323,16 @@ class GetCourseTopicsTest(ModuleStoreTestCase): @ddt.ddt -@httpretty.activate class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): """Test for get_thread_list""" def setUp(self): super(GetThreadListTest, 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.request = RequestFactory().get("/test_path") self.request.user = self.user self.course = CourseFactory.create() @@ -366,6 +369,11 @@ 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", @@ -378,6 +386,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "body": "Test body", "pinned": False, "closed": False, + "abuse_flaggers": [], + "votes": {"up_count": 4}, "comments_count": 5, "unread_comments_count": 3, }, @@ -392,6 +402,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "body": "More content", "pinned": False, "closed": True, + "abuse_flaggers": [], + "votes": {"up_count": 9}, "comments_count": 18, "unread_comments_count": 0, }, @@ -406,6 +418,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "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, }, @@ -422,6 +436,10 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "raw_body": "Test body", "pinned": False, "closed": False, + "following": True, + "abuse_flagged": False, + "voted": False, + "vote_count": 4, "comment_count": 5, "unread_comment_count": 3, }, @@ -436,6 +454,10 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "raw_body": "More content", "pinned": False, "closed": True, + "following": False, + "abuse_flagged": False, + "voted": True, + "vote_count": 9, "comment_count": 18, "unread_comment_count": 0, }, @@ -450,6 +472,10 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "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, }, diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index df6d63cfcc..69342ed039 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -141,6 +141,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) def test_basic(self): + self.register_get_user_response(self.user, upvoted_ids=["test_thread"]) source_threads = [{ "id": "test_thread", "course_id": unicode(self.course.id), @@ -152,6 +153,8 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "body": "Test body", "pinned": False, "closed": False, + "abuse_flaggers": [], + "votes": {"up_count": 4}, "comments_count": 5, "unread_comments_count": 3, }] @@ -166,6 +169,10 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "raw_body": "Test body", "pinned": False, "closed": False, + "following": False, + "abuse_flagged": False, + "voted": True, + "vote_count": 4, "comment_count": 5, "unread_comment_count": 3, }] @@ -190,6 +197,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): }) def test_pagination(self): + self.register_get_user_response(self.user) self.register_get_threads_response([], page=1, num_pages=1) response = self.client.get( self.url, diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 41f7f50e8c..b931d7dbc8 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -21,6 +21,19 @@ class CommentsServiceMockMixin(object): status=200 ) + def register_get_user_response(self, user, subscribed_thread_ids=None, upvoted_ids=None): + """Register a mock response for GET on the CS user instance endpoint""" + httpretty.register_uri( + httpretty.GET, + "http://localhost:4567/api/v1/users/{id}".format(id=user.id), + body=json.dumps({ + "id": str(user.id), + "subscribed_thread_ids": subscribed_thread_ids or [], + "upvoted_ids": upvoted_ids or [], + }), + status=200 + ) + def assert_last_query_params(self, expected_params): """ Assert that the last mock request had the expected query parameters From b1a9d3347213f247ea0389b7862b43258364ba7a Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 5 May 2015 11:43:36 -0400 Subject: [PATCH 2/3] Add authorship data to thread list endpoint --- lms/djangoapps/discussion_api/api.py | 35 +++++- .../discussion_api/tests/test_api.py | 119 ++++++++++++++++++ .../discussion_api/tests/test_views.py | 7 ++ 3 files changed, 159 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 071fba0545..4078976a8d 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -77,12 +77,19 @@ def get_course_topics(course, user): } -def _cc_thread_to_api_thread(thread, cc_user): +def _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids): """ 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 """ + 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 [ @@ -98,6 +105,13 @@ def _cc_thread_to_api_thread(thread, cc_user): } ret.update({ "topic_id": thread["commentable_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 + ), "raw_body": thread["body"], "following": thread["id"] in cc_user["subscribed_thread_ids"], "abuse_flagged": cc_user["id"] in thread["abuse_flaggers"], @@ -144,6 +158,23 @@ 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_key + ) + 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_key) + for user in role.users.all() + } - results = [_cc_thread_to_api_thread(thread, cc_user) for thread in threads] + results = [ + _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids) + for thread in threads + ] return get_paginated_data(request, results, page, num_pages) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 7b5a729d9a..8477de8a54 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -336,6 +336,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): self.request = RequestFactory().get("/test_path") self.request.user = self.user self.course = CourseFactory.create() + self.author = UserFactory.create() def get_thread_list(self, threads, page=1, page_size=1, num_pages=1, course=None): """ @@ -347,6 +348,40 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): ret = get_thread_list(self.request, course.id, page, page_size) return ret + def create_role(self, role_name, users): + """Create a Role in self.course with the given name and users""" + role = Role.objects.create(name=role_name, course_id=self.course.id) + 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", + "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", + "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_empty(self): self.assertEqual( self.get_thread_list([]), @@ -379,6 +414,10 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_0", "course_id": unicode(self.course.id), "commentable_id": "topic_x", + "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", "type": "discussion", @@ -395,6 +434,10 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_1", "course_id": unicode(self.course.id), "commentable_id": "topic_y", + "user_id": str(self.author.id), + "username": self.author.username, + "anonymous": False, + "anonymous_to_peers": False, "created_at": "2015-04-28T22:22:22Z", "updated_at": "2015-04-28T00:33:33Z", "type": "question", @@ -411,6 +454,10 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_2", "course_id": unicode(self.course.id), "commentable_id": "topic_x", + "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", "type": "discussion", @@ -429,6 +476,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_0", "course_id": unicode(self.course.id), "topic_id": "topic_x", + "author": self.author.username, + "author_label": None, "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", "type": "discussion", @@ -447,6 +496,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_1", "course_id": unicode(self.course.id), "topic_id": "topic_y", + "author": self.author.username, + "author_label": None, "created_at": "2015-04-28T22:22:22Z", "updated_at": "2015-04-28T00:33:33Z", "type": "question", @@ -465,6 +516,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_2", "course_id": unicode(self.course.id), "topic_id": "topic_x", + "author": self.author.username, + "author_label": None, "created_at": "2015-04-28T00:44:44Z", "updated_at": "2015-04-28T00:55:55Z", "type": "discussion", @@ -542,3 +595,69 @@ 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_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 69342ed039..de4110547d 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -121,6 +121,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): """Tests for ThreadViewSet list""" def setUp(self): super(ThreadViewSetListTest, self).setUp() + self.author = UserFactory.create() self.url = reverse("thread-list") def test_course_id_missing(self): @@ -146,6 +147,10 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "id": "test_thread", "course_id": unicode(self.course.id), "commentable_id": "test_topic", + "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", "type": "discussion", @@ -162,6 +167,8 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "id": "test_thread", "course_id": unicode(self.course.id), "topic_id": "test_topic", + "author": self.author.username, + "author_label": None, "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", "type": "discussion", From 7ce579c27417aad2c2911ad3418aa9d204d1adc5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 5 May 2015 16:21:52 -0400 Subject: [PATCH 3/3] Add group fields to thread list endpoint --- lms/djangoapps/discussion_api/api.py | 37 ++++++++++++++----- .../discussion_api/tests/test_api.py | 17 +++++++-- .../discussion_api/tests/test_views.py | 3 ++ lms/djangoapps/discussion_api/views.py | 5 +-- .../core/djangoapps/course_groups/cohorts.py | 6 +++ .../course_groups/tests/test_cohorts.py | 9 +++++ 6 files changed, 60 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 4078976a8d..fb6b793970 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -15,7 +15,7 @@ from django_comment_common.models import ( ) 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 +from openedx.core.djangoapps.course_groups.cohorts import get_cohort_id, get_cohort_names def get_course_topics(course, user): @@ -77,11 +77,24 @@ def get_course_topics(course, user): } -def _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids): +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 @@ -95,6 +108,7 @@ def _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids): for key in [ "id", "course_id", + "group_id", "created_at", "updated_at", "type", @@ -105,6 +119,7 @@ def _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids): } 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 @@ -123,14 +138,14 @@ def _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids): return ret -def get_thread_list(request, course_key, page, page_size): +def get_thread_list(request, course, page, page_size): """ Return the list of all discussion threads pertaining to the given course Parameters: request: The django request objects used for build_absolute_uri - course_key: The key of the course to get discussion threads for + course: The course to get discussion threads for page: The page number (1-indexed) to retrieve page_size: The number of threads to retrieve per page @@ -140,14 +155,14 @@ def get_thread_list(request, course_key, page, page_size): discussion_api.views.ThreadViewSet for more detail. """ user_is_privileged = Role.objects.filter( - course_id=course_key, + 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() threads, result_page, num_pages, _ = Thread.search({ - "course_id": unicode(course_key), - "group_id": None if user_is_privileged else get_cohort_id(request.user, course_key), + "course_id": unicode(course.id), + "group_id": None if user_is_privileged else get_cohort_id(request.user, course.id), "sort_key": "date", "sort_order": "desc", "page": page, @@ -163,18 +178,20 @@ def get_thread_list(request, course_key, page, page_size): user.id for role in Role.objects.filter( name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR], - course_id=course_key + 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_key) + 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) + _cc_thread_to_api_thread(thread, cc_user, staff_user_ids, ta_user_ids, group_ids_to_names) for thread in threads ] return get_paginated_data(request, results, page, num_pages) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 8477de8a54..3cf504fb04 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -12,8 +12,6 @@ from pytz import UTC from django.http import Http404 from django.test.client import RequestFactory -from opaque_keys.edx.locator import CourseLocator - from courseware.tests.factories import BetaTesterFactory, StaffFactory from discussion_api.api import get_course_topics, get_thread_list from discussion_api.tests.utils import CommentsServiceMockMixin @@ -337,6 +335,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): self.request.user = self.user self.course = CourseFactory.create() self.author = UserFactory.create() + self.cohort = CohortFactory.create(course_id=self.course.id) def get_thread_list(self, threads, page=1, page_size=1, num_pages=1, course=None): """ @@ -345,7 +344,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): """ course = course or self.course self.register_get_threads_response(threads, page, num_pages) - ret = get_thread_list(self.request, course.id, page, page_size) + ret = get_thread_list(self.request, course, page, page_size) return ret def create_role(self, role_name, users): @@ -363,6 +362,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "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, @@ -414,6 +414,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_0", "course_id": unicode(self.course.id), "commentable_id": "topic_x", + "group_id": None, "user_id": str(self.author.id), "username": self.author.username, "anonymous": False, @@ -434,6 +435,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_1", "course_id": unicode(self.course.id), "commentable_id": "topic_y", + "group_id": self.cohort.id, "user_id": str(self.author.id), "username": self.author.username, "anonymous": False, @@ -454,6 +456,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "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, @@ -476,6 +479,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_0", "course_id": unicode(self.course.id), "topic_id": "topic_x", + "group_id": None, + "group_name": None, "author": self.author.username, "author_label": None, "created_at": "2015-04-28T00:00:00Z", @@ -496,6 +501,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "id": "test_thread_id_1", "course_id": unicode(self.course.id), "topic_id": "topic_y", + "group_id": self.cohort.id, + "group_name": self.cohort.name, "author": self.author.username, "author_label": None, "created_at": "2015-04-28T22:22:22Z", @@ -516,6 +523,8 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): "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", @@ -594,7 +603,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): # Test page past the last one 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) + get_thread_list(self.request, self.course, page=4, page_size=10) @ddt.data( (FORUM_ROLE_ADMINISTRATOR, True, False, True), diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index de4110547d..1ed979b80b 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -147,6 +147,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "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, @@ -167,6 +168,8 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "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", diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 3362d5ba27..b17b7e0caa 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -133,12 +133,11 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): form = ThreadListGetForm(request.GET) if not form.is_valid(): raise ValidationError(form.errors) - course_key = form.cleaned_data["course_id"] - self.get_course_or_404(request.user, course_key) + course = self.get_course_or_404(request.user, form.cleaned_data["course_id"]) return Response( get_thread_list( request, - course_key, + course, form.cleaned_data["page"], form.cleaned_data["page_size"] ) diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index bac42c7f63..8fd0b23231 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -293,6 +293,12 @@ def get_course_cohorts(course, assignment_type=None): query_set = query_set.filter(cohort__assignment_type=assignment_type) if assignment_type else query_set return list(query_set) + +def get_cohort_names(course): + """Return a dict that maps cohort ids to names for the given course""" + return {cohort.id: cohort.name for cohort in get_course_cohorts(course)} + + ### Helpers for cohort management views diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index 5bc55f706c..ede2581223 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -462,6 +462,15 @@ class TestCohorts(ModuleStoreTestCase): cohort_set = {c.name for c in cohorts.get_course_cohorts(course)} self.assertEqual(cohort_set, {"AutoGroup1", "AutoGroup2", "ManualCohort", "ManualCohort2"}) + def test_get_cohort_names(self): + course = modulestore().get_course(self.toy_course_key) + cohort1 = CohortFactory(course_id=course.id, name="Cohort1") + cohort2 = CohortFactory(course_id=course.id, name="Cohort2") + self.assertEqual( + cohorts.get_cohort_names(course), + {cohort1.id: cohort1.name, cohort2.id: cohort2.name} + ) + def test_is_commentable_cohorted(self): course = modulestore().get_course(self.toy_course_key) self.assertFalse(cohorts.is_course_cohorted(course.id))