diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index ee3d6dd67e..87ed4394f8 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -69,22 +69,21 @@ def get_threads(request, course, user_info, discussion_id=None, per_page=THREADS if something goes wrong, or ValueError if the group_id is invalid. Arguments: - request: The user request. - course: The course object. - user_info: The comment client User object as a dict. - discussion_id: Optional discussion id/commentable id for context. - per_page: Optional number of threads per page. + 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: - A tuple of the threads and the query parameters used for the search. - :return: + (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, @@ -122,7 +121,6 @@ def get_threads(request, course, user_info, discussion_id=None, per_page=THREADS [ 'page', 'sort_key', - 'sort_order', 'text', 'commentable_ids', 'flagged', @@ -483,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( @@ -494,7 +491,6 @@ def followed_threads(request, course_key, user_id): [ 'page', 'sort_key', - 'sort_order', 'flagged', 'unread', 'unanswered', diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index c1bb20a096..7c03c5d0c6 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/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: diff --git a/lms/djangoapps/discussion_api/forms.py b/lms/djangoapps/discussion_api/forms.py index ca3cdd8440..2d5ac81c9f 100644 --- a/lms/djangoapps/discussion_api/forms.py +++ b/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) diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 63ad61840c..f7e60a0df2 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/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 diff --git a/lms/djangoapps/discussion_api/tests/test_forms.py b/lms/djangoapps/discussion_api/tests/test_forms.py index e116824895..48b32d4fc9 100644 --- a/lms/djangoapps/discussion_api/tests/test_forms.py +++ b/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 diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index a04274d072..7c26618368 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/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): diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index ff93e97366..4f3fd6340d 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/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')