On 05/16/2017 07:20 PM, Stephen Finucane wrote:
On Tue, 2017-05-16 at 17:38 +0100, Stephen Finucane wrote: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 <[email protected]> Fixes: 6222574 ("REST: filter patches by state name") Cc: Philippe Pepiot <[email protected]> --- 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 valueThanks 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#c us tomise-filter-generation-with-filter-overridesYes, good idea. I'll submit a follow-up for this.Actually, looking into this it seems django-filter doesn't handle this properly: enabling the option results in a '500 (Server Error)', rather than a '400 (Bad Request)'. We should never bubble up a 5xx-class error unless there's a serious problem. Unless you're aware of a way to handle this better, I'd be reluctant to do this and would keep things as they are. Stephen
Ok let's keep this, it's still better than before, thanks. Tested-by: Philippe Pepiot <[email protected]>
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_______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
_______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
