From 95563dabb7bdd6648bbe701d28a0848bcfc3297f Mon Sep 17 00:00:00 2001 From: Karl Hobley Date: Fri, 17 Jun 2016 13:22:45 +0100 Subject: [PATCH] Added parse_boolean function This commit adds the ability to use true/false on boolean fields and also validates integers properly --- wagtail/api/v2/filters.py | 25 ++++++++++-- wagtail/api/v2/tests/test_pages.py | 21 ++++++++++ wagtail/api/v2/tests/tests.py | 41 +++++++++++++++++++- wagtail/api/v2/utils.py | 17 ++++++++ wagtail/wagtailadmin/api/filters.py | 11 +++--- wagtail/wagtailadmin/tests/api/test_pages.py | 21 ++++++++-- 6 files changed, 122 insertions(+), 14 deletions(-) diff --git a/wagtail/api/v2/filters.py b/wagtail/api/v2/filters.py index dbb6061332..073eb060b8 100644 --- a/wagtail/api/v2/filters.py +++ b/wagtail/api/v2/filters.py @@ -1,13 +1,14 @@ from __future__ import absolute_import, unicode_literals from django.conf import settings +from django.db import models from rest_framework.filters import BaseFilterBackend -from taggit.managers import _TaggableManager +from taggit.managers import TaggableManager from wagtail.wagtailcore.models import Page from wagtail.wagtailsearch.backends import get_search_backend -from .utils import BadRequestError, pages_for_site +from .utils import BadRequestError, pages_for_site, parse_boolean class FieldsFilter(BaseFilterBackend): @@ -20,9 +21,25 @@ class FieldsFilter(BaseFilterBackend): for field_name, value in request.GET.items(): if field_name in fields: - field = getattr(queryset.model, field_name, None) + try: + field = queryset.model._meta.get_field(field_name) + except LookupError: + field = None - if isinstance(field, _TaggableManager): + # Convert value into python + try: + if isinstance(field, (models.BooleanField, models.NullBooleanField)): + value = parse_boolean(value) + elif isinstance(field, (models.IntegerField, models.AutoField)): + value = int(value) + except ValueError as e: + raise BadRequestError("field filter error. '%s' is not a valid value for %s (%s)" % ( + value, + field_name, + str(e) + )) + + if isinstance(field, TaggableManager): for tag in value.split(','): queryset = queryset.filter(**{field_name + '__name': tag}) diff --git a/wagtail/api/v2/tests/test_pages.py b/wagtail/api/v2/tests/test_pages.py index d5162af1ad..d8b70107e8 100644 --- a/wagtail/api/v2/tests/test_pages.py +++ b/wagtail/api/v2/tests/test_pages.py @@ -376,6 +376,13 @@ class TestPageListing(TestCase): page_id_list = self.get_page_id_list(content) self.assertEqual(page_id_list, [16]) + def test_filtering_on_boolean(self): + response = self.get_response(show_in_menus='false') + content = json.loads(response.content.decode('UTF-8')) + + page_id_list = self.get_page_id_list(content) + self.assertEqual(page_id_list, [8, 9, 16, 18, 19, 17]) + def test_filtering_doesnt_work_on_specific_fields_without_type(self): response = self.get_response(date='2013-12-02') content = json.loads(response.content.decode('UTF-8')) @@ -404,6 +411,20 @@ class TestPageListing(TestCase): self.assertEqual(response.status_code, 400) self.assertEqual(content, {'message': "query parameter is not an operation or a recognised field: not_a_field"}) + def test_filtering_int_validation(self): + response = self.get_response(id='abc') + content = json.loads(response.content.decode('UTF-8')) + + self.assertEqual(response.status_code, 400) + self.assertEqual(content, {'message': "field filter error. 'abc' is not a valid value for id (invalid literal for int() with base 10: 'abc')"}) + + def test_filtering_boolean_validation(self): + response = self.get_response(show_in_menus='abc') + content = json.loads(response.content.decode('UTF-8')) + + self.assertEqual(response.status_code, 400) + self.assertEqual(content, {'message': "field filter error. 'abc' is not a valid value for show_in_menus (expected 'true' or 'false', got 'abc')"}) + # CHILD OF FILTER diff --git a/wagtail/api/v2/tests/tests.py b/wagtail/api/v2/tests/tests.py index 377134ba24..7f61f0a2ed 100644 --- a/wagtail/api/v2/tests/tests.py +++ b/wagtail/api/v2/tests/tests.py @@ -2,7 +2,7 @@ from __future__ import absolute_import, unicode_literals from unittest import TestCase -from ..utils import FieldsParameterParseError, parse_fields_parameter +from ..utils import FieldsParameterParseError, parse_boolean, parse_fields_parameter class TestParseFieldsParameter(TestCase): @@ -235,3 +235,42 @@ class TestParseFieldsParameter(TestCase): parse_fields_parameter('*,_') self.assertEqual(str(e.exception), "'_' must be in the first position") + + +class TestParseBoolean(TestCase): + # GOOD STUFF + + def test_valid_true(self): + parsed = parse_boolean('true') + + self.assertEqual(parsed, True) + + def test_valid_false(self): + parsed = parse_boolean('false') + + self.assertEqual(parsed, False) + + def test_valid_1(self): + parsed = parse_boolean('1') + + self.assertEqual(parsed, True) + + def test_valid_0(self): + parsed = parse_boolean('0') + + self.assertEqual(parsed, False) + + + # BAD STUFF + + def test_invalid(self): + with self.assertRaises(ValueError) as e: + parse_boolean('foo') + + self.assertEqual(str(e.exception), "expected 'true' or 'false', got 'foo'") + + def test_invalid_integer(self): + with self.assertRaises(ValueError) as e: + parse_boolean('2') + + self.assertEqual(str(e.exception), "expected 'true' or 'false', got '2'") diff --git a/wagtail/api/v2/utils.py b/wagtail/api/v2/utils.py index a95b872b1b..273996a20f 100644 --- a/wagtail/api/v2/utils.py +++ b/wagtail/api/v2/utils.py @@ -216,3 +216,20 @@ def parse_fields_parameter(fields_str): fields, _ = parse_fields(fields_str) return fields + + +def parse_boolean(value): + """ + Parses strings into booleans using the following mapping (case-sensitive): + + 'true' => True + 'false' => False + '1' => True + '0' => False + """ + if value in ['true', '1']: + return True + elif value in ['false', '0']: + return False + else: + raise ValueError("expected 'true' or 'false', got '%s'" % value) diff --git a/wagtail/wagtailadmin/api/filters.py b/wagtail/wagtailadmin/api/filters.py index 3966bc819a..395739a761 100644 --- a/wagtail/wagtailadmin/api/filters.py +++ b/wagtail/wagtailadmin/api/filters.py @@ -2,7 +2,7 @@ from __future__ import absolute_import, unicode_literals from rest_framework.filters import BaseFilterBackend -from wagtail.api.v2.utils import BadRequestError +from wagtail.api.v2.utils import BadRequestError, parse_boolean class HasChildrenFilter(BaseFilterBackend): @@ -13,12 +13,11 @@ class HasChildrenFilter(BaseFilterBackend): def filter_queryset(self, request, queryset, view): if 'has_children' in request.GET: try: - has_children_filter = int(request.GET['has_children']) - assert has_children_filter is 1 or has_children_filter is 0 - except (ValueError, AssertionError): - raise BadRequestError("has_children must be 1 or 0") + has_children_filter = parse_boolean(request.GET['has_children']) + except ValueError: + raise BadRequestError("has_children must be 'true' or 'false'") - if has_children_filter == 1: + if has_children_filter is True: return queryset.filter(numchild__gt=0) else: return queryset.filter(numchild=0) diff --git a/wagtail/wagtailadmin/tests/api/test_pages.py b/wagtail/wagtailadmin/tests/api/test_pages.py index 2417dbaa55..9673d0365b 100644 --- a/wagtail/wagtailadmin/tests/api/test_pages.py +++ b/wagtail/wagtailadmin/tests/api/test_pages.py @@ -248,32 +248,47 @@ class TestAdminPageListing(AdminAPITestCase, TestPageListing): # HAS CHILDREN FILTER def test_has_children_filter(self): - response = self.get_response(has_children=1) + response = self.get_response(has_children='true') content = json.loads(response.content.decode('UTF-8')) page_id_list = self.get_page_id_list(content) self.assertEqual(page_id_list, [2, 4, 5, 6, 21, 20]) def test_has_children_filter_off(self): + response = self.get_response(has_children='false') + content = json.loads(response.content.decode('UTF-8')) + + page_id_list = self.get_page_id_list(content) + self.assertEqual(page_id_list, [8, 9, 16, 18, 19, 10, 15, 17, 22, 23, 13, 14, 12]) + + def test_has_children_filter_int(self): + response = self.get_response(has_children=1) + content = json.loads(response.content.decode('UTF-8')) + + page_id_list = self.get_page_id_list(content) + self.assertEqual(page_id_list, [2, 4, 5, 6, 21, 20]) + + def test_has_children_filter_int_off(self): response = self.get_response(has_children=0) content = json.loads(response.content.decode('UTF-8')) page_id_list = self.get_page_id_list(content) self.assertEqual(page_id_list, [8, 9, 16, 18, 19, 10, 15, 17, 22, 23, 13, 14, 12]) + def test_has_children_filter_invalid_integer(self): response = self.get_response(has_children=3) content = json.loads(response.content.decode('UTF-8')) self.assertEqual(response.status_code, 400) - self.assertEqual(content, {'message': "has_children must be 1 or 0"}) + self.assertEqual(content, {'message': "has_children must be 'true' or 'false'"}) def test_has_children_filter_invalid_value(self): response = self.get_response(has_children='yes') content = json.loads(response.content.decode('UTF-8')) self.assertEqual(response.status_code, 400) - self.assertEqual(content, {'message': "has_children must be 1 or 0"}) + self.assertEqual(content, {'message': "has_children must be 'true' or 'false'"}) class TestAdminPageDetail(AdminAPITestCase, TestPageDetail):