[PATCH 6/6] REST: Resolve issues with filters

[Stephen Finucane]

On Tue, 2017-05-16 at 11:04 +0200, Philippe Pepiot wrote:
> 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#cus
> tomise-filter-generation-with-filter-overrides
> 

Yes, good idea. I'll submit a follow-up for this.

If you've tested this/reviewed this, feel free to say as much by
repling with a 'Tested-by' or 'Acked-by' (on this or any other patch
:))

Stephen

> > +
> > +
> > +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
> >