| Message ID | 1480097915-12158-10-git-send-email-stephen@that.guru |
|---|---|
| State | Accepted |
| Headers | show |
On 11/25/2016 12:18 PM, Stephen Finucane wrote: > The 'Patch.state' field is exposed by the API, but is not editable. > Tests exist that suggest the field is editable, but they lie as they > don't actually validate the field is changed (it hasn't). Make this > field editable, using a custom field type to allow said user to submit a > string representation of the state rather than an integer id. > > This has the side effect of changing the output representation of the > 'state' field for the '/patches' endpoint to a slugified representation, > i.e.: > > "state": "under-review", > > Instead of: > > "state": "Under review", > > The same slugified representation will be used in PATCH requests. This > seems more consistent with how APIs generally do this stuff making it a > "good thing" (TM). > > Signed-off-by: Stephen Finucane <stephen@that.guru> Reviewed-by: Andy Doan <andy.doan@linaro.org> > --- > v3: > - Rework test to demonstrate the prior non-editability of the field > (Daniel Axtens) > - Use less confusing variable names in a list comprehensions (Andy > Doan) > --- > patchwork/api/base.py | 5 +++++ > patchwork/api/patch.py | 30 +++++++++++++++++++++++++----- > patchwork/tests/test_rest_api.py | 8 ++++++-- > 3 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/patchwork/api/base.py b/patchwork/api/base.py > index dbd8148..13a8432 100644 > --- a/patchwork/api/base.py > +++ b/patchwork/api/base.py > @@ -23,6 +23,11 @@ from rest_framework import permissions > from rest_framework.pagination import PageNumberPagination > from rest_framework.response import Response > > +from patchwork.models import State > + > +STATE_CHOICES = ['-'.join(x.name.lower().split(' ')) > + for x in State.objects.all()] > + > > class LinkHeaderPagination(PageNumberPagination): > """Provide pagination based on rfc5988. > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > index 8427450..f818f20 100644 > --- a/patchwork/api/patch.py > +++ b/patchwork/api/patch.py > @@ -20,25 +20,45 @@ > import email.parser > > from django.core.urlresolvers import reverse > -from rest_framework.serializers import HyperlinkedModelSerializer > +from rest_framework.exceptions import ValidationError > from rest_framework.generics import ListAPIView > from rest_framework.generics import RetrieveUpdateAPIView > +from rest_framework.serializers import ChoiceField > +from rest_framework.serializers import HyperlinkedModelSerializer > from rest_framework.serializers import SerializerMethodField > > from patchwork.api.base import PatchworkPermission > +from patchwork.api.base import STATE_CHOICES > from patchwork.models import Patch > +from patchwork.models import State > + > + > +class StateField(ChoiceField): > + """Avoid the need for a state endpoint.""" > + > + def __init__(self, *args, **kwargs): > + kwargs['choices'] = STATE_CHOICES > + super(StateField, self).__init__(*args, **kwargs) > + > + def to_internal_value(self, data): > + data = ' '.join(data.split('-')) > + try: > + return State.objects.get(name__iexact=data) > + except State.DoesNotExist: > + raise ValidationError('Invalid state. Expected one of: %s ' % > + ', '.join(STATE_CHOICES)) > + > + def to_representation(self, obj): > + return '-'.join(obj.name.lower().split()) > > > class PatchListSerializer(HyperlinkedModelSerializer): > mbox = SerializerMethodField() > - state = SerializerMethodField() > + state = StateField() > tags = SerializerMethodField() > check = SerializerMethodField() > checks = SerializerMethodField() > > - def get_state(self, instance): > - return instance.state.name > - > def get_mbox(self, instance): > request = self.context.get('request') > return request.build_absolute_uri(instance.get_mbox_url()) > diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py > index 469fd26..d764945 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -31,6 +31,7 @@ from patchwork.tests.utils import create_maintainer > from patchwork.tests.utils import create_patch > from patchwork.tests.utils import create_person > from patchwork.tests.utils import create_project > +from patchwork.tests.utils import create_state > from patchwork.tests.utils import create_user > > if settings.ENABLE_REST_API: > @@ -368,19 +369,22 @@ class TestPatchAPI(APITestCase): > # A maintainer can update > project = create_project() > patch = create_patch(project=project) > + state = create_state() > user = create_maintainer(project) > self.client.force_authenticate(user=user) > > + self.assertNotEqual(Patch.objects.get(id=patch.id).state, state) > resp = self.client.patch(self.api_url(patch.id), > - {'state': 2}) > + {'state': state.name}) > self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertEqual(Patch.objects.get(id=patch.id).state, state) > > # A normal user can't > user = create_user() > self.client.force_authenticate(user=user) > > resp = self.client.patch(self.api_url(patch.id), > - {'state': 2}) > + {'state': state.name}) > self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > def test_delete(self): >
This is much better, thank you. Reviewed-by: Daniel Axtens <dja@axtens.net> Regards, Daniel Stephen Finucane <stephen@that.guru> writes: > The 'Patch.state' field is exposed by the API, but is not editable. > Tests exist that suggest the field is editable, but they lie as they > don't actually validate the field is changed (it hasn't). Make this > field editable, using a custom field type to allow said user to submit a > string representation of the state rather than an integer id. > > This has the side effect of changing the output representation of the > 'state' field for the '/patches' endpoint to a slugified representation, > i.e.: > > "state": "under-review", > > Instead of: > > "state": "Under review", > > The same slugified representation will be used in PATCH requests. This > seems more consistent with how APIs generally do this stuff making it a > "good thing" (TM). > > Signed-off-by: Stephen Finucane <stephen@that.guru> > --- > v3: > - Rework test to demonstrate the prior non-editability of the field > (Daniel Axtens) > - Use less confusing variable names in a list comprehensions (Andy > Doan) > --- > patchwork/api/base.py | 5 +++++ > patchwork/api/patch.py | 30 +++++++++++++++++++++++++----- > patchwork/tests/test_rest_api.py | 8 ++++++-- > 3 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/patchwork/api/base.py b/patchwork/api/base.py > index dbd8148..13a8432 100644 > --- a/patchwork/api/base.py > +++ b/patchwork/api/base.py > @@ -23,6 +23,11 @@ from rest_framework import permissions > from rest_framework.pagination import PageNumberPagination > from rest_framework.response import Response > > +from patchwork.models import State > + > +STATE_CHOICES = ['-'.join(x.name.lower().split(' ')) > + for x in State.objects.all()] > + > > class LinkHeaderPagination(PageNumberPagination): > """Provide pagination based on rfc5988. > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > index 8427450..f818f20 100644 > --- a/patchwork/api/patch.py > +++ b/patchwork/api/patch.py > @@ -20,25 +20,45 @@ > import email.parser > > from django.core.urlresolvers import reverse > -from rest_framework.serializers import HyperlinkedModelSerializer > +from rest_framework.exceptions import ValidationError > from rest_framework.generics import ListAPIView > from rest_framework.generics import RetrieveUpdateAPIView > +from rest_framework.serializers import ChoiceField > +from rest_framework.serializers import HyperlinkedModelSerializer > from rest_framework.serializers import SerializerMethodField > > from patchwork.api.base import PatchworkPermission > +from patchwork.api.base import STATE_CHOICES > from patchwork.models import Patch > +from patchwork.models import State > + > + > +class StateField(ChoiceField): > + """Avoid the need for a state endpoint.""" > + > + def __init__(self, *args, **kwargs): > + kwargs['choices'] = STATE_CHOICES > + super(StateField, self).__init__(*args, **kwargs) > + > + def to_internal_value(self, data): > + data = ' '.join(data.split('-')) > + try: > + return State.objects.get(name__iexact=data) > + except State.DoesNotExist: > + raise ValidationError('Invalid state. Expected one of: %s ' % > + ', '.join(STATE_CHOICES)) > + > + def to_representation(self, obj): > + return '-'.join(obj.name.lower().split()) > > > class PatchListSerializer(HyperlinkedModelSerializer): > mbox = SerializerMethodField() > - state = SerializerMethodField() > + state = StateField() > tags = SerializerMethodField() > check = SerializerMethodField() > checks = SerializerMethodField() > > - def get_state(self, instance): > - return instance.state.name > - > def get_mbox(self, instance): > request = self.context.get('request') > return request.build_absolute_uri(instance.get_mbox_url()) > diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py > index 469fd26..d764945 100644 > --- a/patchwork/tests/test_rest_api.py > +++ b/patchwork/tests/test_rest_api.py > @@ -31,6 +31,7 @@ from patchwork.tests.utils import create_maintainer > from patchwork.tests.utils import create_patch > from patchwork.tests.utils import create_person > from patchwork.tests.utils import create_project > +from patchwork.tests.utils import create_state > from patchwork.tests.utils import create_user > > if settings.ENABLE_REST_API: > @@ -368,19 +369,22 @@ class TestPatchAPI(APITestCase): > # A maintainer can update > project = create_project() > patch = create_patch(project=project) > + state = create_state() > user = create_maintainer(project) > self.client.force_authenticate(user=user) > > + self.assertNotEqual(Patch.objects.get(id=patch.id).state, state) > resp = self.client.patch(self.api_url(patch.id), > - {'state': 2}) > + {'state': state.name}) > self.assertEqual(status.HTTP_200_OK, resp.status_code) > + self.assertEqual(Patch.objects.get(id=patch.id).state, state) > > # A normal user can't > user = create_user() > self.client.force_authenticate(user=user) > > resp = self.client.patch(self.api_url(patch.id), > - {'state': 2}) > + {'state': state.name}) > self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) > > def test_delete(self): > -- > 2.7.4 > > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
diff --git a/patchwork/api/base.py b/patchwork/api/base.py index dbd8148..13a8432 100644 --- a/patchwork/api/base.py +++ b/patchwork/api/base.py @@ -23,6 +23,11 @@ from rest_framework import permissions from rest_framework.pagination import PageNumberPagination from rest_framework.response import Response +from patchwork.models import State + +STATE_CHOICES = ['-'.join(x.name.lower().split(' ')) + for x in State.objects.all()] + class LinkHeaderPagination(PageNumberPagination): """Provide pagination based on rfc5988. diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py index 8427450..f818f20 100644 --- a/patchwork/api/patch.py +++ b/patchwork/api/patch.py @@ -20,25 +20,45 @@ import email.parser from django.core.urlresolvers import reverse -from rest_framework.serializers import HyperlinkedModelSerializer +from rest_framework.exceptions import ValidationError from rest_framework.generics import ListAPIView from rest_framework.generics import RetrieveUpdateAPIView +from rest_framework.serializers import ChoiceField +from rest_framework.serializers import HyperlinkedModelSerializer from rest_framework.serializers import SerializerMethodField from patchwork.api.base import PatchworkPermission +from patchwork.api.base import STATE_CHOICES from patchwork.models import Patch +from patchwork.models import State + + +class StateField(ChoiceField): + """Avoid the need for a state endpoint.""" + + def __init__(self, *args, **kwargs): + kwargs['choices'] = STATE_CHOICES + super(StateField, self).__init__(*args, **kwargs) + + def to_internal_value(self, data): + data = ' '.join(data.split('-')) + try: + return State.objects.get(name__iexact=data) + except State.DoesNotExist: + raise ValidationError('Invalid state. Expected one of: %s ' % + ', '.join(STATE_CHOICES)) + + def to_representation(self, obj): + return '-'.join(obj.name.lower().split()) class PatchListSerializer(HyperlinkedModelSerializer): mbox = SerializerMethodField() - state = SerializerMethodField() + state = StateField() tags = SerializerMethodField() check = SerializerMethodField() checks = SerializerMethodField() - def get_state(self, instance): - return instance.state.name - def get_mbox(self, instance): request = self.context.get('request') return request.build_absolute_uri(instance.get_mbox_url()) diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py index 469fd26..d764945 100644 --- a/patchwork/tests/test_rest_api.py +++ b/patchwork/tests/test_rest_api.py @@ -31,6 +31,7 @@ from patchwork.tests.utils import create_maintainer from patchwork.tests.utils import create_patch from patchwork.tests.utils import create_person from patchwork.tests.utils import create_project +from patchwork.tests.utils import create_state from patchwork.tests.utils import create_user if settings.ENABLE_REST_API: @@ -368,19 +369,22 @@ class TestPatchAPI(APITestCase): # A maintainer can update project = create_project() patch = create_patch(project=project) + state = create_state() user = create_maintainer(project) self.client.force_authenticate(user=user) + self.assertNotEqual(Patch.objects.get(id=patch.id).state, state) resp = self.client.patch(self.api_url(patch.id), - {'state': 2}) + {'state': state.name}) self.assertEqual(status.HTTP_200_OK, resp.status_code) + self.assertEqual(Patch.objects.get(id=patch.id).state, state) # A normal user can't user = create_user() self.client.force_authenticate(user=user) resp = self.client.patch(self.api_url(patch.id), - {'state': 2}) + {'state': state.name}) self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code) def test_delete(self):
The 'Patch.state' field is exposed by the API, but is not editable. Tests exist that suggest the field is editable, but they lie as they don't actually validate the field is changed (it hasn't). Make this field editable, using a custom field type to allow said user to submit a string representation of the state rather than an integer id. This has the side effect of changing the output representation of the 'state' field for the '/patches' endpoint to a slugified representation, i.e.: "state": "under-review", Instead of: "state": "Under review", The same slugified representation will be used in PATCH requests. This seems more consistent with how APIs generally do this stuff making it a "good thing" (TM). Signed-off-by: Stephen Finucane <stephen@that.guru> --- v3: - Rework test to demonstrate the prior non-editability of the field (Daniel Axtens) - Use less confusing variable names in a list comprehensions (Andy Doan) --- patchwork/api/base.py | 5 +++++ patchwork/api/patch.py | 30 +++++++++++++++++++++++++----- patchwork/tests/test_rest_api.py | 8 ++++++-- 3 files changed, 36 insertions(+), 7 deletions(-)