Merge pull request #13464 from edx/robrap/TNL-5115

TNL-5115: Remove sort order from Discussions in edx-platform

lms/djangoapps/discussion/views.py

@@ -63,16 +63,27 @@ def make_course_settings(course, user):
 
 
 @newrelic.agent.function_trace()
-def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
+def get_threads(request, course, user_info, discussion_id=None, per_page=THREADS_PER_PAGE):
     """
     This may raise an appropriate subclass of cc.utils.CommentClientError
     if something goes wrong, or ValueError if the group_id is invalid.
+
+    Arguments:
+        request (WSGIRequest): The user request.
+        course (CourseDescriptorWithMixins): The course object.
+        user_info (dict): The comment client User object as a dict.
+        discussion_id (unicode): Optional discussion id/commentable id for context.
+        per_page (int): Optional number of threads per page.
+
+    Returns:
+        (tuple of list, dict): A tuple of the list of threads and a dict of the
+            query parameters used for the search.
+
     """
     default_query_params = {
         'page': 1,
         'per_page': per_page,
         'sort_key': 'activity',
-        'sort_order': 'desc',
         'text': '',
         'course_id': unicode(course.id),
         'user_id': request.user.id,
@@ -90,12 +101,9 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
 
     if not request.GET.get('sort_key'):
         # If the user did not select a sort key, use their last used sort key
-        cc_user = cc.User.from_django_user(request.user)
-        cc_user.retrieve()
-        # TODO: After the comment service is updated this can just be
-        # user.default_sort_key because the service returns the default value
-        default_query_params['sort_key'] = cc_user.get('default_sort_key') or default_query_params['sort_key']
-    else:
+        default_query_params['sort_key'] = user_info.get('default_sort_key') or default_query_params['sort_key']
+
+    elif request.GET.get('sort_key') != user_info.get('default_sort_key'):
         # If the user clicked a sort key, update their default sort key
         cc_user = cc.User.from_django_user(request.user)
         cc_user.default_sort_key = request.GET.get('sort_key')
@@ -113,7 +121,6 @@ def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
                 [
                     'page',
                     'sort_key',
-                    'sort_order',
                     'text',
                     'commentable_ids',
                     'flagged',
@@ -175,7 +182,7 @@ def inline_discussion(request, course_key, discussion_id):
     user_info = cc_user.to_dict()
 
     try:
-        threads, query_params = get_threads(request, course, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
+        threads, query_params = get_threads(request, course, user_info, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
     except ValueError:
         return HttpResponseBadRequest("Invalid group_id")
 
@@ -212,7 +219,7 @@ def forum_form_discussion(request, course_key):
     user_info = user.to_dict()
 
     try:
-        unsafethreads, query_params = get_threads(request, course)   # This might process a search query
+        unsafethreads, query_params = get_threads(request, course, user_info)   # This might process a search query
         is_staff = has_permission(request.user, 'openclose_thread', course.id)
         threads = [utils.prepare_content(thread, course_key, is_staff) for thread in unsafethreads]
     except cc.utils.CommentClientMaintenanceError:
@@ -335,7 +342,7 @@ def single_thread(request, course_key, discussion_id, thread_id):
 
     else:
         try:
-            threads, query_params = get_threads(request, course)
+            threads, query_params = get_threads(request, course, user_info)
         except ValueError:
             return HttpResponseBadRequest("Invalid group_id")
         threads.append(thread.to_dict())
@@ -474,7 +481,6 @@ def followed_threads(request, course_key, user_id):
             'page': 1,
             'per_page': THREADS_PER_PAGE,   # more than threads_per_page to show more activities
             'sort_key': 'date',
-            'sort_order': 'desc',
         }
 
         query_params = merge_dict(
@@ -485,7 +491,6 @@ def followed_threads(request, course_key, user_id):
                     [
                         'page',
                         'sort_key',
-                        'sort_order',
                         'flagged',
                         'unread',
                         'unanswered',

lms/djangoapps/discussion_api/api.py

@@ -498,8 +498,9 @@ def get_thread_list(
     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".
+    order_direction: The direction in which to sort the threads by. The default
+        and only value is "desc". This will be removed in a future major
+        version.
     requested_fields: Indicates which additional fields to return
         for each thread. (i.e. ['profile_image'])
 
@@ -528,9 +529,9 @@ def get_thread_list(
             "order_by":
                 ["Invalid value. '{}' must be 'last_activity_at', 'comment_count', or 'vote_count'".format(order_by)]
         })
-    if order_direction not in ["asc", "desc"]:
+    if order_direction != "desc":
         raise ValidationError({
-            "order_direction": ["Invalid value. '{}' must be 'asc' or 'desc'".format(order_direction)]
+            "order_direction": ["Invalid value. '{}' must be 'desc'".format(order_direction)]
         })
 
     course = _get_course(course_key, request.user)
@@ -546,7 +547,6 @@ def get_thread_list(
         "per_page": page_size,
         "text": text_search,
         "sort_key": cc_map.get(order_by),
-        "sort_order": order_direction,
     }
 
     if view:

lms/djangoapps/discussion_api/forms.py

@@ -48,7 +48,7 @@ class ThreadListGetForm(_PaginationForm):
         required=False
     )
     order_direction = ChoiceField(
-        choices=[(choice, choice) for choice in ["asc", "desc"]],
+        choices=[(choice, choice) for choice in ["desc"]],
         required=False
     )
     requested_fields = MultiValueField(required=False)

lms/djangoapps/discussion_api/tests/test_api.py

@@ -627,7 +627,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["1"],
             "commentable_ids": ["topic_x,topic_meow"]
@@ -639,7 +638,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["6"],
             "per_page": ["14"],
         })
@@ -851,7 +849,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
             "text": ["test search string"],
@@ -883,7 +880,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["11"],
         })
@@ -915,7 +911,6 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["11"],
             query: ["true"],
@@ -957,20 +952,22 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": [cc_query],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["11"],
         })
 
-    @ddt.data("asc", "desc")
-    def test_order_direction_query(self, http_query):
+    def test_order_direction(self):
+        """
+        Only "desc" is supported for order.  Also, since it is simply swallowed,
+        it isn't included in the params.
+        """
         self.register_get_threads_response([], page=1, num_pages=0)
         result = get_thread_list(
             self.request,
             self.course.id,
             page=1,
             page_size=11,
-            order_direction=http_query,
+            order_direction="desc",
         ).data
 
         expected_result = make_paginated_api_response(
@@ -986,11 +983,25 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": [http_query],
             "page": ["1"],
             "per_page": ["11"],
         })
 
+    def test_invalid_order_direction(self):
+        """
+        Test with invalid order_direction (e.g. "asc")
+        """
+        with self.assertRaises(ValidationError) as assertion:
+            self.register_get_threads_response([], page=1, num_pages=0)
+            get_thread_list(           # pylint: disable=expression-not-assigned
+                self.request,
+                self.course.id,
+                page=1,
+                page_size=11,
+                order_direction="asc",
+            ).data
+        self.assertIn("order_direction", assertion.exception.message_dict)
+
 
 @attr(shard=2)
 @ddt.ddt

lms/djangoapps/discussion_api/tests/test_forms.py

@@ -147,7 +147,6 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
         ("order_by", "last_activity_at"),
         ("order_by", "comment_count"),
         ("order_by", "vote_count"),
-        ("order_direction", "asc"),
         ("order_direction", "desc"),
     )
     @ddt.unpack

lms/djangoapps/discussion_api/tests/test_views.py

@@ -428,7 +428,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
         })
@@ -449,7 +448,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
             query: ["true"],
@@ -471,7 +469,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["18"],
             "per_page": ["4"],
         })
@@ -497,7 +494,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
             "sort_key": ["activity"],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
             "text": ["test search string"],
@@ -589,14 +585,16 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
         self.assert_last_query_params({
             "user_id": [unicode(self.user.id)],
             "course_id": [unicode(self.course.id)],
-            "sort_order": ["desc"],
             "page": ["1"],
             "per_page": ["10"],
             "sort_key": [cc_query],
         })
 
-    @ddt.data("asc", "desc")
-    def test_order_direction(self, query):
+    def test_order_direction(self):
+        """
+        Test order direction, of which "desc" is the only valid option.  The
+        option actually just gets swallowed, so it doesn't affect the params.
+        """
         threads = [make_minimal_cs_thread()]
         self.register_get_user_response(self.user)
         self.register_get_threads_response(threads, page=1, num_pages=1)
@@ -604,7 +602,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             self.url,
             {
                 "course_id": unicode(self.course.id),
-                "order_direction": query,
+                "order_direction": "desc",
             }
         )
         self.assert_last_query_params({
@@ -613,7 +611,6 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase, Pro
             "sort_key": ["activity"],
             "page": ["1"],
             "per_page": ["10"],
-            "sort_order": [query],
         })
 
     def test_mutually_exclusive(self):

lms/djangoapps/discussion_api/views.py

@@ -155,8 +155,9 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
             "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".
+        * order_direction: Must be "desc". The direction in which to sort the
+            threads by. The default and only value is "desc". This will be
+            removed in a future major version.
 
         * following: If true, retrieve only threads the requesting user is
             following
@@ -164,6 +165,7 @@ class ThreadViewSet(DeveloperErrorViewMixin, ViewSet):
         * view: "unread" for threads the requesting user has not read, or
             "unanswered" for question threads with no marked answer. Only one
             can be selected.
+
         * requested_fields: (list) Indicates which additional fields to return
           for each thread. (supports 'profile_image')