| Message ID | 1479574288-24171-8-git-send-email-stephen@that.guru |
|---|---|
| State | Superseded |
| Headers | show |
Hi Stephen, > This is one of the most useful fields to allow editing of via the API. > Make it so. Please could you revise this commit message? We seem to already have tests that check if you can successfully PATCH the state of a patch. This patch can't be making that editable if it already is editable. You also change the way you interact with the state of a patch. I quite like the change you've made, but the commit message should explain it (and that it's a breaking change!). Thanks, Daniel > > Signed-off-by: Stephen Finucane <stephen@that.guru> > Cc: Andy Doan <andy.doan@linaro.org> > --- > patchwork/api/__init__.py | 5 +++++ > patchwork/api/patch.py | 30 +++++++++++++++++++++++++----- > patchwork/tests/test_rest_api.py | 6 ++++-- > 3 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/patchwork/api/__init__.py b/patchwork/api/__init__.py > index dbd8148..73a1dc7 100644 > --- a/patchwork/api/__init__.py > +++ b/patchwork/api/__init__.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(state.name.lower().split()) > + for state 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 737ada1..58fd843 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 import PatchworkPermission > +from patchwork.api 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..e8eb71f 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,11 +369,12 @@ 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) > > resp = self.client.patch(self.api_url(patch.id), > - {'state': 2}) > + {'state': state.name}) > self.assertEqual(status.HTTP_200_OK, resp.status_code) > > # A normal user can't > @@ -380,7 +382,7 @@ class TestPatchAPI(APITestCase): > 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
On 11/19/2016 10:51 AM, Stephen Finucane wrote: > This is one of the most useful fields to allow editing of via the API. > Make it so. So this is really about changing states to be managed by name rather than id, right? > Signed-off-by: Stephen Finucane <stephen@that.guru> > Cc: Andy Doan <andy.doan@linaro.org> > --- > patchwork/api/__init__.py | 5 +++++ > patchwork/api/patch.py | 30 +++++++++++++++++++++++++----- > patchwork/tests/test_rest_api.py | 6 ++++-- > 3 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/patchwork/api/__init__.py b/patchwork/api/__init__.py > index dbd8148..73a1dc7 100644 > --- a/patchwork/api/__init__.py > +++ b/patchwork/api/__init__.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(state.name.lower().split()) > + for state in State.objects.all()] This confused me for a moment. I think a more clear way to do this would be: [x.name.lower().replace(' ', '-') for x in State.objects.all()]
On Tue, 2016-11-22 at 12:10 -0600, Andy Doan wrote: > On 11/19/2016 10:51 AM, Stephen Finucane wrote: > > > > This is one of the most useful fields to allow editing of via the > > API. > > Make it so. > > So this is really about changing states to be managed by name rather > than id, right? Oh, that's a side effect too. More importantly though, it's about actually enabling this to change. The tests to check this were wrong, and we weren't actually able to change state using the code as is. That's fixed now. > > > > Signed-off-by: Stephen Finucane <stephen@that.guru> > > Cc: Andy Doan <andy.doan@linaro.org> > > --- > > patchwork/api/__init__.py | 5 +++++ > > patchwork/api/patch.py | 30 +++++++++++++++++++++++++--- > > -- > > patchwork/tests/test_rest_api.py | 6 ++++-- > > 3 files changed, 34 insertions(+), 7 deletions(-) > > > > diff --git a/patchwork/api/__init__.py b/patchwork/api/__init__.py > > index dbd8148..73a1dc7 100644 > > --- a/patchwork/api/__init__.py > > +++ b/patchwork/api/__init__.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(state.name.lower().split()) > > + for state in State.objects.all()] > > This confused me for a moment. I think a more clear way to do this > would be: > > [x.name.lower().replace(' ', '-') for x in State.objects.all()] I considered this, but 'replace' will replace multiple spaces with multiple '-'. It's unlikely to happen, but I'd rather avoid the change of getting 'hello--world' if possible. I'm happy to reassess that, however :) Stephen
On Mon, 2016-11-21 at 14:36 +1100, Daniel Axtens wrote: > Hi Stephen, > > > > > This is one of the most useful fields to allow editing of via the > > API. > > Make it so. > > Please could you revise this commit message? > > We seem to already have tests that check if you can successfully > PATCH > the state of a patch. This patch can't be making that editable if it > already is editable. Commit message revised. As said in the reply to Andy's mail, the test was broken and didn't validate that we could actually change the field - only that a HTTP 200 response code would be returned. That, and the test, are both updated now. > You also change the way you interact with the state of a patch. I > quite > like the change you've made, but the commit message should explain it > (and that it's a breaking change!). Good point. Done. Stephen
On 11/24/2016 02:01 PM, Stephen Fincane wrote: >>> +from patchwork.models import State >>> > > + >>> > > +STATE_CHOICES = ['-'.join(state.name.lower().split()) >>> > > + for state in State.objects.all()] >> > >> > This confused me for a moment. I think a more clear way to do this >> > would be: >> > >> > [x.name.lower().replace(' ', '-') for x in State.objects.all()] > I considered this, but 'replace' will replace multiple spaces with > multiple '-'. It's unlikely to happen, but I'd rather avoid the change > of getting 'hello--world' if possible. I'm happy to reassess that, > however :) ah - never mind then :)
diff --git a/patchwork/api/__init__.py b/patchwork/api/__init__.py index dbd8148..73a1dc7 100644 --- a/patchwork/api/__init__.py +++ b/patchwork/api/__init__.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(state.name.lower().split()) + for state 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 737ada1..58fd843 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 import PatchworkPermission +from patchwork.api 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..e8eb71f 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,11 +369,12 @@ 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) resp = self.client.patch(self.api_url(patch.id), - {'state': 2}) + {'state': state.name}) self.assertEqual(status.HTTP_200_OK, resp.status_code) # A normal user can't @@ -380,7 +382,7 @@ class TestPatchAPI(APITestCase): 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 one of the most useful fields to allow editing of via the API. Make it so. Signed-off-by: Stephen Finucane <stephen@that.guru> Cc: Andy Doan <andy.doan@linaro.org> --- patchwork/api/__init__.py | 5 +++++ patchwork/api/patch.py | 30 +++++++++++++++++++++++++----- patchwork/tests/test_rest_api.py | 6 ++++-- 3 files changed, 34 insertions(+), 7 deletions(-)