[PATCH 6/6] REST: Resolve issues with filters
Philippe Pepiot
philippe.pepiot at logilab.fr
Tue May 16 19:04:30 AEST 2017
On 05/16/2017 01:14 AM, Stephen Finucane wrote:
> Turns out filtering patches using a series string wasn't as easy as we
> thought. We need to slugify State.name, but unfortunately that isn't
> stored in the database. The tests were hiding this fact as State objects
> created by 'tests.utils.create_state' don't have spaces in them.
>
> Override custom versions of both django-filter's 'Filter' class and
> the Django 'Form' required by this, and update the tests to prevent a
> regression.
>
> Signed-off-by: Stephen Finucane <stephen at that.guru>
> Fixes: 6222574 ("REST: filter patches by state name")
> Cc: Philippe Pepiot <philippe.pepiot at logilab.fr>
> ---
> patchwork/api/filters.py | 36 +++++++++++++++++++++++++++++++-----
> patchwork/api/patch.py | 2 +-
> patchwork/models.py | 4 ++++
> patchwork/tests/test_rest_api.py | 10 +++++-----
> 4 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py
> index 12a54a8..00efc99 100644
> --- a/patchwork/api/filters.py
> +++ b/patchwork/api/filters.py
> @@ -17,10 +17,11 @@
> # along with Patchwork; if not, write to the Free Software
> # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>
> +from django.core.exceptions import ValidationError
> from django_filters import FilterSet
> from django_filters import IsoDateTimeFilter
> -from django_filters import CharFilter
> from django_filters import ModelChoiceFilter
> +from django.forms import ModelChoiceField
>
> from patchwork.compat import LOOKUP_FIELD
> from patchwork.models import Bundle
> @@ -30,6 +31,7 @@ from patchwork.models import Event
> from patchwork.models import Patch
> from patchwork.models import Project
> from patchwork.models import Series
> +from patchwork.models import State
>
>
> class TimestampMixin(FilterSet):
> @@ -59,15 +61,39 @@ class CoverLetterFilter(ProjectMixin, TimestampMixin, FilterSet):
> fields = ('project', 'series', 'submitter')
>
>
> +class StateChoiceField(ModelChoiceField):
> +
> + def prepare_value(self, value):
> + if hasattr(value, '_meta'):
> + return value.slug
> + else:
> + return super(StateChoiceField, self).prepare_value(value)
> +
> + def to_python(self, value):
> + if value in self.empty_values:
> + return None
> + try:
> + value = ' '.join(value.split('-'))
> + value = self.queryset.get(name__iexact=value)
> + except (ValueError, TypeError, self.queryset.model.DoesNotExist):
> + raise ValidationError(self.error_messages['invalid_choice'],
> + code='invalid_choice')
> + return value
Thanks for fixing this !
It seems that the ValidationError is silently hidden:
$ http 'http://localhost:8000/api/patches/?state=doesnotexists'
HTTP/1.0 200 OK
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Date: Tue, 16 May 2017 08:47:51 GMT
Server: WSGIServer/0.2 CPython/3.5.2
Vary: Accept, Cookie
[]
Maybe modifying 'strict' behavior here ?
http://django-filter.readthedocs.io/en/develop/ref/filterset.html#customise-filter-generation-with-filter-overrides
> +
> +
> +class StateFilter(ModelChoiceFilter):
> +
> + field_class = StateChoiceField
> +
> +
> class PatchFilter(ProjectMixin, FilterSet):
>
> - # TODO(stephenfin): We should probably be using a ChoiceFilter here?
> - state = CharFilter(name='state__name')
> + state = StateFilter(queryset=State.objects.all())
>
> class Meta:
> model = Patch
> - fields = ('project', 'series', 'submitter', 'delegate', 'state',
> - 'archived')
> + fields = ('project', 'series', 'submitter', 'delegate',
> + 'state', 'archived')
>
>
> class CheckFilter(TimestampMixin, FilterSet):
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index f0c7225..1f0ead1 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -70,7 +70,7 @@ class StateField(RelatedField):
> self.fail('incorrect_type', data_type=type(data).__name__)
>
> def to_representation(self, obj):
> - return '-'.join(obj.name.lower().split())
> + return obj.slug
>
> def get_queryset(self):
> return State.objects.all()
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 8913fac..0ed37ab 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -192,6 +192,10 @@ class State(models.Model):
> ordering = models.IntegerField(unique=True)
> action_required = models.BooleanField(default=True)
>
> + @property
> + def slug(self):
> + return '-'.join(self.name.lower().split())
> +
> def __str__(self):
> return self.name
>
> diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
> index 70410d0..0956010 100644
> --- a/patchwork/tests/test_rest_api.py
> +++ b/patchwork/tests/test_rest_api.py
> @@ -309,7 +309,7 @@ class TestPatchAPI(APITestCase):
> self.assertEqual(patch_obj.id, patch_json['id'])
> self.assertEqual(patch_obj.name, patch_json['name'])
> self.assertEqual(patch_obj.msgid, patch_json['msgid'])
> - self.assertEqual(patch_obj.state.name, patch_json['state'])
> + self.assertEqual(patch_obj.state.slug, patch_json['state'])
> self.assertIn(patch_obj.get_mbox_url(), patch_json['mbox'])
>
> # nested fields
> @@ -325,7 +325,8 @@ class TestPatchAPI(APITestCase):
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(0, len(resp.data))
>
> - patch_obj = create_patch()
> + state_obj = create_state(name='Under Review')
> + patch_obj = create_patch(state=state_obj)
>
> # anonymous user
> resp = self.client.get(self.api_url())
> @@ -338,10 +339,9 @@ class TestPatchAPI(APITestCase):
> self.assertNotIn('diff', patch_rsp)
>
> # test filtering by state
> - other_state = create_state()
> - resp = self.client.get(self.api_url(), {'state': patch_obj.state.name})
> + resp = self.client.get(self.api_url(), {'state': 'under-review'})
> self.assertEqual([patch_obj.id], [x['id'] for x in resp.data])
> - resp = self.client.get(self.api_url(), {'state': other_state.name})
> + resp = self.client.get(self.api_url(), {'state': 'missing-state'})
> self.assertEqual(0, len(resp.data))
>
> # authenticated user
>
More information about the Patchwork
mailing list