Merge pull request #8585 from edx/clee/discussion-api-order-and-sort-threads

Thread sort key and direction for discussion api

lms/djangoapps/discussion_api/api.py

@@ -239,7 +239,10 @@ def get_thread_list(
         topic_id_list=None,
         text_search=None,
         following=False,
-        view=None):
+        view=None,
+        order_by="last_activity_at",
+        order_direction="desc",
+):
     """
     Return the list of all discussion threads pertaining to the given course
 
@@ -253,6 +256,11 @@ def get_thread_list(
     text_search A text search query string to match
     following: If true, retrieve only threads the requester is following
     view: filters for either "unread" or "unanswered" threads
+    order_by: The key in which to sort the threads by. The only values are
+        "last_activity_at", "comment_count", and "vote_count". The default is
+        "last_activity_at".
+    order_direction: The direction in which to sort the threads by. The only
+        values are "asc" or "desc". The default is "desc".
 
     Note that topic_id_list, text_search, and following are mutually exclusive.
 
@@ -263,7 +271,7 @@ def get_thread_list(
 
     Raises:
 
-    ValidationError: if an invalid value is passed for a field
+    ValidationError: if an invalid value is passed for a field.
     ValueError: if more than one of the mutually exclusive parameters is
       provided
     Http404: if the requesting user does not have access to the requested course
@@ -273,19 +281,31 @@ def get_thread_list(
     if exclusive_param_count > 1:  # pragma: no cover
         raise ValueError("More than one mutually exclusive param passed to get_thread_list")
 
+    cc_map = {"last_activity_at": "date", "comment_count": "comments", "vote_count": "votes"}
+    if order_by not in cc_map:
+        raise ValidationError({
+            "order_by":
+                ["Invalid value. '{}' must be 'last_activity_at', 'comment_count', or 'vote_count'".format(order_by)]
+        })
+    if order_direction not in ["asc", "desc"]:
+        raise ValidationError({
+            "order_direction": ["Invalid value. '{}' must be 'asc' or 'desc'".format(order_direction)]
+        })
+
     course = _get_course_or_404(course_key, request.user)
     context = get_context(course, request)
+
     query_params = {
         "user_id": unicode(request.user.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,
         "per_page": page_size,
         "text": text_search,
+        "sort_key": cc_map.get(order_by),
+        "sort_order": order_direction,
     }
 
     text_search_rewrite = None

lms/djangoapps/discussion_api/forms.py

@@ -54,8 +54,24 @@ class ThreadListGetForm(_PaginationForm):
     following = NullBooleanField(required=False)
     view = ChoiceField(
         choices=[(choice, choice) for choice in ["unread", "unanswered"]],
+        required=False,
+    )
+    order_by = ChoiceField(
+        choices=[(choice, choice) for choice in ["last_activity_at", "comment_count", "vote_count"]],
         required=False
     )
+    order_direction = ChoiceField(
+        choices=[(choice, choice) for choice in ["asc", "desc"]],
+        required=False
+    )
+
+    def clean_order_by(self):
+        """Return a default choice"""
+        return self.cleaned_data.get("order_by") or "last_activity_at"
+
+    def clean_order_direction(self):
+        """Return a default choice"""
+        return self.cleaned_data.get("order_direction") or "desc"
 
     def clean_course_id(self):
         """Validate course_id"""

lms/djangoapps/discussion_api/tests/test_api.py

@@ -836,6 +836,74 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             query: ["true"],
         })
 
+    @ddt.data(
+        ("last_activity_at", "date"),
+        ("comment_count", "comments"),
+        ("vote_count", "votes")
+    )
+    @ddt.unpack
+    def test_order_by_query(self, http_query, cc_query):
+        """
+        Tests the order_by parameter
+
+        Arguments:
+            http_query (str): Query string sent in the http request
+            cc_query (str): Query string used for the comments client service
+        """
+        self.register_get_threads_response([], page=1, num_pages=1)
+        result = get_thread_list(
+            self.request,
+            self.course.id,
+            page=1,
+            page_size=11,
+            order_by=http_query,
+        )
+        self.assertEqual(
+            result,
+            {"results": [], "next": None, "previous": None, "text_search_rewrite": None}
+        )
+        self.assertEqual(
+            urlparse(httpretty.last_request().path).path,
+            "/api/v1/threads"
+        )
+        self.assert_last_query_params({
+            "user_id": [unicode(self.user.id)],
+            "course_id": [unicode(self.course.id)],
+            "sort_key": [cc_query],
+            "sort_order": ["desc"],
+            "page": ["1"],
+            "per_page": ["11"],
+            "recursive": ["False"],
+        })
+
+    @ddt.data("asc", "desc")
+    def test_order_direction_query(self, http_query):
+        self.register_get_threads_response([], page=1, num_pages=1)
+        result = get_thread_list(
+            self.request,
+            self.course.id,
+            page=1,
+            page_size=11,
+            order_direction=http_query,
+        )
+        self.assertEqual(
+            result,
+            {"results": [], "next": None, "previous": None, "text_search_rewrite": None}
+        )
+        self.assertEqual(
+            urlparse(httpretty.last_request().path).path,
+            "/api/v1/threads"
+        )
+        self.assert_last_query_params({
+            "user_id": [unicode(self.user.id)],
+            "course_id": [unicode(self.course.id)],
+            "sort_key": ["date"],
+            "sort_order": [http_query],
+            "page": ["1"],
+            "per_page": ["11"],
+            "recursive": ["False"],
+        })
+
 
 @ddt.ddt
 class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase):

lms/djangoapps/discussion_api/tests/test_forms.py

@@ -95,7 +95,9 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
                 "topic_id": [],
                 "text_search": "",
                 "following": None,
-                "view": ""
+                "view": "",
+                "order_by": "last_activity_at",
+                "order_direction": "desc",
             }
         )
 
@@ -147,6 +149,20 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
         self.form_data["view"] = "not_a_valid_choice"
         self.assert_error("view", "Select a valid choice. not_a_valid_choice is not one of the available choices.")
 
+    def test_invalid_sort_by_choice(self):
+        self.form_data["order_by"] = "not_a_valid_choice"
+        self.assert_error(
+            "order_by",
+            "Select a valid choice. not_a_valid_choice is not one of the available choices."
+        )
+
+    def test_invalid_sort_direction_choice(self):
+        self.form_data["order_direction"] = "not_a_valid_choice"
+        self.assert_error(
+            "order_direction",
+            "Select a valid choice. not_a_valid_choice is not one of the available choices."
+        )
+
 
 class CommentListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
     """Tests for CommentListGetForm"""

lms/djangoapps/discussion_api/tests/test_views.py

@@ -327,6 +327,62 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
             "/api/v1/users/{}/subscribed_threads".format(self.user.id)
         )
 
+    @ddt.data(
+        ("last_activity_at", "date"),
+        ("comment_count", "comments"),
+        ("vote_count", "votes")
+    )
+    @ddt.unpack
+    def test_order_by(self, http_query, cc_query):
+        """
+        Tests the order_by parameter
+
+        Arguments:
+            http_query (str): Query string sent in the http request
+            cc_query (str): Query string used for the comments client service
+        """
+        threads = [make_minimal_cs_thread()]
+        self.register_get_user_response(self.user)
+        self.register_get_threads_response(threads, page=1, num_pages=1)
+        self.client.get(
+            self.url,
+            {
+                "course_id": unicode(self.course.id),
+                "order_by": http_query,
+            }
+        )
+        self.assert_last_query_params({
+            "user_id": [unicode(self.user.id)],
+            "course_id": [unicode(self.course.id)],
+            "sort_order": ["desc"],
+            "recursive": ["False"],
+            "page": ["1"],
+            "per_page": ["10"],
+            "sort_key": [cc_query],
+        })
+
+    @ddt.data("asc", "desc")
+    def test_order_direction(self, query):
+        threads = [make_minimal_cs_thread()]
+        self.register_get_user_response(self.user)
+        self.register_get_threads_response(threads, page=1, num_pages=1)
+        self.client.get(
+            self.url,
+            {
+                "course_id": unicode(self.course.id),
+                "order_direction": query,
+            }
+        )
+        self.assert_last_query_params({
+            "user_id": [unicode(self.user.id)],
+            "course_id": [unicode(self.course.id)],
+            "sort_key": ["date"],
+            "recursive": ["False"],
+            "page": ["1"],
+            "per_page": ["10"],
+            "sort_order": [query],
+        })
+
 
 @httpretty.activate
 class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):

lms/djangoapps/discussion_api/views.py

@@ -141,6 +141,13 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet):
             (including the bodies of comments in the thread) matches the search
             string will be returned.
 
+        * order_by: Must be "last_activity_at", "comment_count", or
+            "vote_count". The key to sort the threads by. The default is
+            "last_activity_at".
+
+        * order_direction: Must be "asc" or "desc". The direction in which to
+            sort the threads by. The default is "desc".
+
         * following: If true, retrieve only threads the requesting user is
             following
 
@@ -245,6 +252,8 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet):
                 form.cleaned_data["text_search"],
                 form.cleaned_data["following"],
                 form.cleaned_data["view"],
+                form.cleaned_data["order_by"],
+                form.cleaned_data["order_direction"],
             )
         )