From 54756317f1ab388c000408983ec9d0826869f343 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 17 Jun 2015 00:41:18 -0400 Subject: [PATCH] Add followed thread retrieval to discussion API This implements the "following" value for the "view" query parameter on the thread list endpoint. --- lms/djangoapps/discussion_api/api.py | 43 ++++++++++++++----- lms/djangoapps/discussion_api/forms.py | 12 +++++- lms/djangoapps/discussion_api/serializers.py | 4 +- .../discussion_api/tests/test_api.py | 28 ++++++++++++ .../discussion_api/tests/test_forms.py | 15 +++++-- .../discussion_api/tests/test_views.py | 25 +++++++++++ lms/djangoapps/discussion_api/tests/utils.py | 13 ++++++ lms/djangoapps/discussion_api/views.py | 7 +++ 8 files changed, 131 insertions(+), 16 deletions(-) diff --git a/lms/djangoapps/discussion_api/api.py b/lms/djangoapps/discussion_api/api.py index c307140552..0e842d2931 100644 --- a/lms/djangoapps/discussion_api/api.py +++ b/lms/djangoapps/discussion_api/api.py @@ -92,14 +92,28 @@ def _get_comment_and_context(request, comment_id): raise Http404 -def get_thread_list_url(request, course_key, topic_id_list=None): +def _is_user_author_or_privileged(cc_content, context): + """ + Check if the user is the author of a content object or a privileged user. + + Returns: + Boolean + """ + return ( + context["is_requester_privileged"] or + context["cc_requester"]["id"] == cc_content["user_id"] + ) + + +def get_thread_list_url(request, course_key, topic_id_list=None, following=False): """ Returns the URL for the thread_list_url field, given a list of topic_ids """ path = reverse("thread-list") query_list = ( [("course_id", unicode(course_key))] + - [("topic_id", topic_id) for topic_id in topic_id_list or []] + [("topic_id", topic_id) for topic_id in topic_id_list or []] + + ([("following", following)] if following else []) ) return request.build_absolute_uri(urlunparse(("", "", path, "", urlencode(query_list), ""))) @@ -132,7 +146,8 @@ def get_course(request, course_key): {"start": blackout["start"].isoformat(), "end": blackout["end"].isoformat()} for blackout in course.get_discussion_blackout_datetimes() ], - "thread_list_url": get_thread_list_url(request, course_key, topic_id_list=[]), + "thread_list_url": get_thread_list_url(request, course_key), + "following_thread_list_url": get_thread_list_url(request, course_key, following=True), "topics_url": request.build_absolute_uri( reverse("course_topics", kwargs={"course_id": course_key}) ) @@ -211,7 +226,7 @@ def get_course_topics(request, course_key): } -def get_thread_list(request, course_key, page, page_size, topic_id_list=None, text_search=None): +def get_thread_list(request, course_key, page, page_size, topic_id_list=None, text_search=None, following=False): """ Return the list of all discussion threads pertaining to the given course @@ -223,8 +238,9 @@ def get_thread_list(request, course_key, page, page_size, topic_id_list=None, te page_size: The number of threads to retrieve per page topic_id_list: The list of topic_ids to get the discussion threads for text_search A text search query string to match + following: If true, retrieve only threads the requester is following - Note that topic_id_list and text_search are mutually exclusive. + Note that topic_id_list, text_search, and following are mutually exclusive. Returns: @@ -238,15 +254,13 @@ def get_thread_list(request, course_key, page, page_size, topic_id_list=None, te Http404: if the requesting user does not have access to the requested course or a page beyond the last is requested """ - exclusive_param_count = sum(1 for param in [topic_id_list, text_search] if param) + exclusive_param_count = sum(1 for param in [topic_id_list, text_search, following] if param) if exclusive_param_count > 1: # pragma: no cover raise ValueError("More than one mutually exclusive param passed to get_thread_list") course = _get_course_or_404(course_key, request.user) context = get_context(course, request) - topic_ids_csv = ",".join(topic_id_list) if topic_id_list else None - threads, result_page, num_pages, text_search_rewrite = Thread.search({ - "course_id": unicode(course.id), + query_params = { "group_id": ( None if context["is_requester_privileged"] else get_cohort_id(request.user, course.id) @@ -255,9 +269,16 @@ def get_thread_list(request, course_key, page, page_size, topic_id_list=None, te "sort_order": "desc", "page": page, "per_page": page_size, - "commentable_ids": topic_ids_csv, "text": text_search, - }) + } + text_search_rewrite = None + if following: + threads, result_page, num_pages = context["cc_requester"].subscribed_threads(query_params) + else: + query_params["course_id"] = unicode(course.id) + query_params["commentable_ids"] = ",".join(topic_id_list) if topic_id_list else None + query_params["text"] = text_search + threads, result_page, num_pages, text_search_rewrite = Thread.search(query_params) # 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 # behavior and return a 404 in that case diff --git a/lms/djangoapps/discussion_api/forms.py b/lms/djangoapps/discussion_api/forms.py index 67a32b0f42..6ebc6bcd9d 100644 --- a/lms/djangoapps/discussion_api/forms.py +++ b/lms/djangoapps/discussion_api/forms.py @@ -5,6 +5,7 @@ from django.core.exceptions import ValidationError from django.forms import ( BooleanField, CharField, + ChoiceField, Field, Form, IntegerField, @@ -45,11 +46,12 @@ class ThreadListGetForm(_PaginationForm): """ A form to validate query parameters in the thread list retrieval endpoint """ - EXCLUSIVE_PARAMS = ["topic_id", "text_search"] + EXCLUSIVE_PARAMS = ["topic_id", "text_search", "following"] course_id = CharField() topic_id = TopicIdField(required=False) text_search = CharField(required=False) + following = NullBooleanField(required=False) def clean_course_id(self): """Validate course_id""" @@ -59,6 +61,14 @@ class ThreadListGetForm(_PaginationForm): except InvalidKeyError: raise ValidationError("'{}' is not a valid course id".format(value)) + def clean_following(self): + """Validate following""" + value = self.cleaned_data["following"] + if value is False: + raise ValidationError("The value of the 'following' parameter must be true.") + else: + return value + def clean(self): cleaned_data = super(ThreadListGetForm, self).clean() exclusive_params_count = sum( diff --git a/lms/djangoapps/discussion_api/serializers.py b/lms/djangoapps/discussion_api/serializers.py index 147e612b44..4ee207e16f 100644 --- a/lms/djangoapps/discussion_api/serializers.py +++ b/lms/djangoapps/discussion_api/serializers.py @@ -47,6 +47,8 @@ def get_context(course, request, thread=None): for user in role.users.all() } requester = request.user + cc_requester = CommentClientUser.from_django_user(requester).retrieve() + cc_requester["course_id"] = course.id return { "course": course, "request": request, @@ -56,7 +58,7 @@ def get_context(course, request, thread=None): "is_requester_privileged": requester.id in staff_user_ids or requester.id in ta_user_ids, "staff_user_ids": staff_user_ids, "ta_user_ids": ta_user_ids, - "cc_requester": CommentClientUser.from_django_user(requester).retrieve(), + "cc_requester": cc_requester, } diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index e46a95e527..2008b2a907 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -99,6 +99,9 @@ class GetCourseTest(UrlResetMixin, ModuleStoreTestCase): "id": unicode(self.course.id), "blackouts": [], "thread_list_url": "http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz", + "following_thread_list_url": ( + "http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&following=True" + ), "topics_url": "http://testserver/api/discussion/v1/course_topics/x/y/z", } ) @@ -741,6 +744,31 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, ModuleStoreTest "text": ["test search string"], }) + def test_following(self): + self.register_subscribed_threads_response(self.user, [], page=1, num_pages=1) + result = get_thread_list( + self.request, + self.course.id, + page=1, + page_size=11, + following=True, + ) + self.assertEqual( + result, + {"results": [], "next": None, "previous": None, "text_search_rewrite": None} + ) + self.assertEqual( + urlparse(httpretty.last_request().path).path, + "/api/v1/users/{}/subscribed_threads".format(self.user.id) + ) + self.assert_last_query_params({ + "course_id": [unicode(self.course.id)], + "sort_key": ["date"], + "sort_order": ["desc"], + "page": ["1"], + "per_page": ["11"], + }) + @ddt.ddt class GetCommentListTest(CommentsServiceMockMixin, ModuleStoreTestCase): diff --git a/lms/djangoapps/discussion_api/tests/test_forms.py b/lms/djangoapps/discussion_api/tests/test_forms.py index 60223c58fd..1d3c8e87e5 100644 --- a/lms/djangoapps/discussion_api/tests/test_forms.py +++ b/lms/djangoapps/discussion_api/tests/test_forms.py @@ -94,6 +94,7 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase): "page_size": 13, "topic_id": [], "text_search": "", + "following": None, } ) @@ -125,12 +126,20 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase): self.form_data.setlist("topic_id", ["", "not empty"]) self.assert_error("topic_id", "This field cannot be empty.") - @ddt.data(*itertools.combinations(["topic_id", "text_search"], 2)) + def test_following_true(self): + self.form_data["following"] = "True" + self.assert_field_value("following", True) + + def test_following_false(self): + self.form_data["following"] = "False" + self.assert_error("following", "The value of the 'following' parameter must be true.") + + @ddt.data(*itertools.combinations(["topic_id", "text_search", "following"], 2)) def test_mutually_exclusive(self, params): - self.form_data.update({param: "dummy" for param in params}) + self.form_data.update({param: "True" for param in params}) self.assert_error( "__all__", - "The following query parameters are mutually exclusive: topic_id, text_search" + "The following query parameters are mutually exclusive: topic_id, text_search, following" ) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index 302badbc14..fff1722d82 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -92,6 +92,9 @@ class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "id": unicode(self.course.id), "blackouts": [], "thread_list_url": "http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz", + "following_thread_list_url": ( + "http://testserver/api/discussion/v1/threads/?course_id=x%2Fy%2Fz&following=True" + ), "topics_url": "http://testserver/api/discussion/v1/course_topics/x/y/z", } ) @@ -270,6 +273,28 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "text": ["test search string"], }) + def test_following(self): + self.register_get_user_response(self.user) + self.register_subscribed_threads_response(self.user, [], page=1, num_pages=1) + response = self.client.get( + self.url, + { + "course_id": unicode(self.course.id), + "page": "1", + "page_size": "4", + "following": "True", + } + ) + self.assert_response_correct( + response, + 200, + {"results": [], "next": None, "previous": None, "text_search_rewrite": None} + ) + self.assertEqual( + urlparse(httpretty.last_request().path).path, + "/api/v1/users/{}/subscribed_threads".format(self.user.id) + ) + @httpretty.activate class ThreadViewSetCreateTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): diff --git a/lms/djangoapps/discussion_api/tests/utils.py b/lms/djangoapps/discussion_api/tests/utils.py index 5dc0e3f5f5..aacab5b42d 100644 --- a/lms/djangoapps/discussion_api/tests/utils.py +++ b/lms/djangoapps/discussion_api/tests/utils.py @@ -191,6 +191,19 @@ class CommentsServiceMockMixin(object): status=200 ) + def register_subscribed_threads_response(self, user, threads, page, num_pages): + """Register a mock response for GET on the CS user instance endpoint""" + httpretty.register_uri( + httpretty.GET, + "http://localhost:4567/api/v1/users/{}/subscribed_threads".format(user.id), + body=json.dumps({ + "collection": threads, + "page": page, + "num_pages": num_pages, + }), + status=200 + ) + def register_subscription_response(self, user): """ Register a mock response for POST and DELETE on the CS user subscription diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index 0658bcf382..9002187fe6 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -141,6 +141,12 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): (including the bodies of comments in the thread) matches the search string will be returned. + * following: If true, retrieve only threads the requesting user is + following + + The topic_id, text_search, and following parameters are mutually + exclusive (i.e. only one may be specified in a request) + **POST Parameters**: * course_id (required): The course to create the thread in @@ -229,6 +235,7 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): form.cleaned_data["page_size"], form.cleaned_data["topic_id"], form.cleaned_data["text_search"], + form.cleaned_data["following"], ) )