This is much better, thank you. Reviewed-by: Daniel Axtens <[email protected]>
Regards, Daniel Stephen Finucane <[email protected]> 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 <[email protected]> > --- > 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 > [email protected] > https://lists.ozlabs.org/listinfo/patchwork _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
