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).
2015-05-05 17:49:57 -04:00
parent 2f398559ae
commit 603eae65d8
3 changed files with 69 additions and 13 deletions
--- 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
--- 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)
--- 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"],