From 603eae65d8015f1ca7598e2cb74eeaea5fb79eb9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 5 May 2015 17:49:57 -0400 Subject: [PATCH] Add params to CS query in thread list endpoint The thread list endpoint needs to restrict the set of threads it retrieves if the course is cohorted and the user is not privileged. This also adds an explicit default sort (by most recent activity). --- lms/djangoapps/discussion_api/api.py | 19 +++++- .../discussion_api/tests/test_api.py | 59 +++++++++++++++---- .../discussion_api/tests/test_views.py | 4 ++ 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index 46919297fa..fd35f9a91c 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -5,9 +5,16 @@ from django.http import Http404 from collections import defaultdict -from lms.lib.comment_client.thread import Thread from discussion_api.pagination import get_paginated_data 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 openedx.core.djangoapps.course_groups.cohorts import get_cohort_id def get_course_topics(course, user): @@ -113,10 +120,18 @@ def get_thread_list(request, course_key, page, page_size): A paginated result containing a list of threads; see discussion_api.views.ThreadViewSet for more detail. """ + user_is_privileged = Role.objects.filter( + course_id=course_key, + name__in=[FORUM_ROLE_ADMINISTRATOR, FORUM_ROLE_MODERATOR, FORUM_ROLE_COMMUNITY_TA], + users=request.user + ).exists() 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), + "sort_key": "date", + "sort_order": "desc", "page": page, - "per_page": page_size + "per_page": page_size, }) # The comments service returns the last page of results if the requested # page is beyond the last page, but we want be consistent with DRF's general diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 79fead35d3..643f7eb51c 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -2,7 +2,9 @@ Tests for Discussion API internal interface """ from datetime import datetime, timedelta +import itertools +import ddt import httpretty import mock from pytz import UTC @@ -15,6 +17,13 @@ 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 +from django_comment_common.models import ( + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_STUDENT, + Role, +) from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from student.tests.factories import UserFactory @@ -313,22 +322,26 @@ class GetCourseTopicsTest(ModuleStoreTestCase): self.assertEqual(staff_actual, staff_expected) +@ddt.ddt @httpretty.activate class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): """Test for get_thread_list""" def setUp(self): super(GetThreadListTest, self).setUp() self.maxDiff = None # pylint: disable=invalid-name + self.user = UserFactory.create() self.request = RequestFactory().get("/test_path") - self.course_key = CourseLocator.from_string("a/b/c") + self.request.user = self.user + self.course = CourseFactory.create() - def get_thread_list(self, threads, page=1, page_size=1, num_pages=1): + def get_thread_list(self, threads, page=1, page_size=1, num_pages=1, course=None): """ Register the appropriate comments service response, then call get_thread_list and return the result. """ + course = course or self.course self.register_get_threads_response(threads, page, num_pages) - ret = get_thread_list(self.request, self.course_key, page, page_size) + ret = get_thread_list(self.request, course.id, page, page_size) return ret def test_empty(self): @@ -344,7 +357,9 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): def test_basic_query_params(self): self.get_thread_list([], page=6, page_size=14) self.assert_last_query_params({ - "course_id": [unicode(self.course_key)], + "course_id": [unicode(self.course.id)], + "sort_key": ["date"], + "sort_order": ["desc"], "page": ["6"], "per_page": ["14"], "recursive": ["False"], @@ -354,7 +369,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): source_threads = [ { "id": "test_thread_id_0", - "course_id": unicode(self.course_key), + "course_id": unicode(self.course.id), "commentable_id": "topic_x", "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", @@ -368,7 +383,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): }, { "id": "test_thread_id_1", - "course_id": unicode(self.course_key), + "course_id": unicode(self.course.id), "commentable_id": "topic_y", "created_at": "2015-04-28T22:22:22Z", "updated_at": "2015-04-28T00:33:33Z", @@ -382,7 +397,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): }, { "id": "test_thread_id_2", - "course_id": unicode(self.course_key), + "course_id": unicode(self.course.id), "commentable_id": "topic_x", "created_at": "2015-04-28T00:44:44Z", "updated_at": "2015-04-28T00:55:55Z", @@ -398,7 +413,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): expected_threads = [ { "id": "test_thread_id_0", - "course_id": unicode(self.course_key), + "course_id": unicode(self.course.id), "topic_id": "topic_x", "created_at": "2015-04-28T00:00:00Z", "updated_at": "2015-04-28T11:11:11Z", @@ -412,7 +427,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): }, { "id": "test_thread_id_1", - "course_id": unicode(self.course_key), + "course_id": unicode(self.course.id), "topic_id": "topic_y", "created_at": "2015-04-28T22:22:22Z", "updated_at": "2015-04-28T00:33:33Z", @@ -426,7 +441,7 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): }, { "id": "test_thread_id_2", - "course_id": unicode(self.course_key), + "course_id": unicode(self.course.id), "topic_id": "topic_x", "created_at": "2015-04-28T00:44:44Z", "updated_at": "2015-04-28T00:55:55Z", @@ -448,6 +463,28 @@ class GetThreadListTest(CommentsServiceMockMixin, ModuleStoreTestCase): } ) + @ddt.data( + *itertools.product( + [ + FORUM_ROLE_ADMINISTRATOR, + FORUM_ROLE_MODERATOR, + FORUM_ROLE_COMMUNITY_TA, + FORUM_ROLE_STUDENT, + ], + [True, False] + ) + ) + @ddt.unpack + def test_request_group(self, role_name, course_is_cohorted): + cohort_course = CourseFactory.create(cohort_config={"cohorted": course_is_cohorted}) + CohortFactory.create(course_id=cohort_course.id, users=[self.user]) + role = Role.objects.create(name=role_name, course_id=cohort_course.id) + role.users = [self.user] + self.get_thread_list([], course=cohort_course) + actual_has_group = "group_id" in httpretty.last_request().querystring + expected_has_group = (course_is_cohorted and role_name == FORUM_ROLE_STUDENT) + self.assertEqual(actual_has_group, expected_has_group) + def test_pagination(self): # N.B. Empty thread list is not realistic but convenient for this test self.assertEqual( @@ -478,4 +515,4 @@ 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_key, page=4, page_size=10) + get_thread_list(self.request, self.course.id, page=4, page_size=10) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 1bf852eceb..df6d63cfcc 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -182,6 +182,8 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) self.assert_last_query_params({ "course_id": [unicode(self.course.id)], + "sort_key": ["date"], + "sort_order": ["desc"], "page": ["1"], "per_page": ["10"], "recursive": ["False"], @@ -200,6 +202,8 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): ) self.assert_last_query_params({ "course_id": [unicode(self.course.id)], + "sort_key": ["date"], + "sort_order": ["desc"], "page": ["18"], "per_page": ["4"], "recursive": ["False"],