From 092cc4b278205b9e0c7c2df1597249ba74e0d1b7 Mon Sep 17 00:00:00 2001 From: wajeeha-khalid Date: Wed, 25 Nov 2015 14:41:20 +0500 Subject: [PATCH] MA-1211; Discussion API - make boolean params case insensitive --- lms/djangoapps/discussion_api/forms.py | 10 +-- .../discussion_api/tests/test_views.py | 77 +++++++++++++++++-- openedx/core/djangoapps/util/forms.py | 39 +++++++++- 3 files changed, 114 insertions(+), 12 deletions(-) diff --git a/lms/djangoapps/discussion_api/forms.py b/lms/djangoapps/discussion_api/forms.py index 2bf779651c..596356472c 100644 --- a/lms/djangoapps/discussion_api/forms.py +++ b/lms/djangoapps/discussion_api/forms.py @@ -9,11 +9,11 @@ from django.forms import ( Form, IntegerField, NullBooleanField, -) + Select) from opaque_keys import InvalidKeyError from opaque_keys.edx.locator import CourseLocator -from openedx.core.djangoapps.util.forms import MultiValueField +from openedx.core.djangoapps.util.forms import MultiValueField, ExtendedNullBooleanField class _PaginationForm(Form): @@ -39,7 +39,7 @@ class ThreadListGetForm(_PaginationForm): course_id = CharField() topic_id = MultiValueField(required=False) text_search = CharField(required=False) - following = NullBooleanField(required=False) + following = ExtendedNullBooleanField(required=False) view = ChoiceField( choices=[(choice, choice) for choice in ["unread", "unanswered"]], required=False, @@ -106,9 +106,7 @@ class CommentListGetForm(_PaginationForm): A form to validate query parameters in the comment list retrieval endpoint """ thread_id = CharField() - # TODO: should we use something better here? This only accepts "True", - # "False", "1", and "0" - endorsed = NullBooleanField(required=False) + endorsed = ExtendedNullBooleanField(required=False) class CommentActionsForm(Form): diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index fdd3309601..343c6658ac 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -306,7 +306,7 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "has_endorsed": False, }] self.register_get_threads_response(source_threads, page=1, num_pages=2) - response = self.client.get(self.url, {"course_id": unicode(self.course.id)}) + response = self.client.get(self.url, {"course_id": unicode(self.course.id), "following": ""}) self.assert_response_correct( response, 200, @@ -395,16 +395,15 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "text": ["test search string"], }) - def test_following(self): + @ddt.data(True, "true", "1") + def test_following_true(self, following): 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", + "following": following, } ) self.assert_response_correct( @@ -417,6 +416,39 @@ class ThreadViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): "/api/v1/users/{}/subscribed_threads".format(self.user.id) ) + @ddt.data(False, "false", "0") + def test_following_false(self, following): + response = self.client.get( + self.url, + { + "course_id": unicode(self.course.id), + "following": following, + } + ) + self.assert_response_correct( + response, + 400, + {"field_errors": { + "following": {"developer_message": "The value of the 'following' parameter must be true."} + }} + ) + + def test_following_error(self): + response = self.client.get( + self.url, + { + "course_id": unicode(self.course.id), + "following": "invalid-boolean", + } + ) + self.assert_response_correct( + response, + 400, + {"field_errors": { + "following": {"developer_message": "Invalid Boolean Value."} + }} + ) + @ddt.data( ("last_activity_at", "activity"), ("comment_count", "comments"), @@ -740,6 +772,7 @@ class ThreadViewSetDeleteTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.assertEqual(response.status_code, 404) +@ddt.ddt @httpretty.activate @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @@ -750,6 +783,15 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): self.url = reverse("comment-list") self.thread_id = "test_thread" + def make_minimal_cs_thread(self, overrides=None): + """ + Create a thread with the given overrides, plus the course_id if not + already in overrides. + """ + overrides = overrides.copy() if overrides else {} + overrides.setdefault("course_id", unicode(self.course.id)) + return make_minimal_cs_thread(overrides) + def test_thread_id_missing(self): response = self.client.get(self.url) self.assert_response_correct( @@ -869,6 +911,31 @@ class CommentViewSetListTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): } ) + @ddt.data( + (True, "endorsed_comment"), + ("true", "endorsed_comment"), + ("1", "endorsed_comment"), + (False, "non_endorsed_comment"), + ("false", "non_endorsed_comment"), + ("0", "non_endorsed_comment"), + ) + @ddt.unpack + def test_question_content(self, endorsed, comment_id): + self.register_get_user_response(self.user) + thread = self.make_minimal_cs_thread({ + "thread_type": "question", + "endorsed_responses": [make_minimal_cs_comment({"id": "endorsed_comment"})], + "non_endorsed_responses": [make_minimal_cs_comment({"id": "non_endorsed_comment"})], + "non_endorsed_resp_total": 1, + }) + self.register_get_thread_response(thread) + response = self.client.get(self.url, { + "thread_id": thread["id"], + "endorsed": endorsed, + }) + parsed_content = json.loads(response.content) + self.assertEqual(parsed_content["results"][0]["id"], comment_id) + @httpretty.activate @disable_signal(api, 'comment_deleted') diff --git a/openedx/core/djangoapps/util/forms.py b/openedx/core/djangoapps/util/forms.py index aa7c5a6f4c..6ec4a92d5c 100644 --- a/openedx/core/djangoapps/util/forms.py +++ b/openedx/core/djangoapps/util/forms.py @@ -3,7 +3,7 @@ Custom forms-related types """ from django.core.exceptions import ValidationError -from django.forms import Field, MultipleHiddenInput +from django.forms import Field, MultipleHiddenInput, NullBooleanField, Select class MultiValueField(Field): @@ -42,3 +42,40 @@ class MultiValueField(Field): """ if values and "" in values: raise ValidationError("This field cannot be empty.") + + +class ExtendedNullBooleanField(NullBooleanField): + """ + A field whose valid values are None, True, 'True', 'true', '1', + False, 'False', 'false' and '0'. + """ + + NULL_BOOLEAN_CHOICES = ( + (None, ""), + (True, True), + (True, "True"), + (True, "true"), + (True, "1"), + (False, False), + (False, "False"), + (False, "false"), + (False, "0"), + ) + + widget = Select(choices=NULL_BOOLEAN_CHOICES) + + def to_python(self, value): + """ + Explicitly checks for the string 'True', 'False', 'true', + 'false', '1' and '0' and returns boolean True or False. + Returns None if value is not passed at all and raises an + exception for any other value. + """ + if value in (True, 'True', 'true', '1'): + return True + elif value in (False, 'False', 'false', '0'): + return False + elif not value: + return None + else: + raise ValidationError("Invalid Boolean Value.")