[PATCH v3 09/16] REST: Make 'Patch.state' editable

[Andy Doan]

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 at that.guru>

Reviewed-by: Andy Doan <andy.doan at 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):
>