REST: Resolve issues with filters

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@that.guru>
Fixes: 6222574 ("REST: filter patches by state name")
Tested-by: Philippe Pepiot <philippe.pepiot@logilab.fr>

diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py
index 12a54a818932b55bc14f9b2b86ce2e086f7fc092..00efc9997711b6f1dd9e26df0094007dd6c6fe54 100644 (file)
--- 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
+
+
+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 f0c72250c25ad9a264f1b07ab0f491e781199c55..1f0ead1f799a762b43654348ec7e6dc067bc123c 100644 (file)
--- 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 974002d5ba34ee2c37a8d331fd0c57eff817472d..df9e6d3dfbd7ac3138ea608a0f81238b6d23d0a4 100644 (file)
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -195,6 +195,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 9b94c47f8c727babaf203de7cfd1ffbb4e51e8f8..8d6462594fab193a3880d23df4a62b72dfb09bf6 100644 (file)
--- 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