[PATCH v2] REST: Don't iterate through models at class level

[Stephen Finucane]

This causes two issues. Firstly, on fresh installs you see the following
error message:

  "Table 'patchwork.patchwork_state' doesn't exist"

Secondly, any new states created when the process is running will not be
reflected in the API until the server process is restarted.

Resolve this issue by moving the step into a method, thus ensuring it's
continuously refreshed. It doesn't seem possible to add tests to prevent
this regressing but some similarly useful tests are included to at least
validate the behavior of that field.

Signed-off-by: Stephen Finucane <stephen at that.guru>
Fixes: a2993505 ("REST: Make 'Patch.state' editable")
Closes-bug: #67
Closes-bug: #80
Cc: denis at laxalde.org
Cc: steve at asksteved.com
Cc: Daniel Axtens <dja at axtens.net>
---
v2:
- Use a subclass of RelatedField instead to ensure lazy-loading of the
  queryset
- Improve tests by checking error messages and removing unnecessary
  checks
---
 patchwork/api/base.py            |  5 -----
 patchwork/api/patch.py           | 39 ++++++++++++++++++++++++++++-----------
 patchwork/tests/test_rest_api.py | 14 ++++++++++++++
 3 files changed, 42 insertions(+), 16 deletions(-)

diff --git a/patchwork/api/base.py b/patchwork/api/base.py
index 13a8432..dbd8148 100644
--- a/patchwork/api/base.py
+++ b/patchwork/api/base.py
@@ -23,11 +23,6 @@ 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 e8fb0ef..2c9ae2c 100644
--- a/patchwork/api/patch.py
+++ b/patchwork/api/patch.py
@@ -20,38 +20,55 @@
 import email.parser
 
 from django.core.urlresolvers import reverse
+from django.utils.translation import ugettext_lazy as _
 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.relations import RelatedField
 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.api.filters import PatchFilter
 from patchwork.models import Patch
 from patchwork.models import State
 
 
-class StateField(ChoiceField):
-    """Avoid the need for a state endpoint."""
+def format_state_name(state):
+    return ' '.join(state.split('-'))
 
-    def __init__(self, *args, **kwargs):
-        kwargs['choices'] = STATE_CHOICES
-        super(StateField, self).__init__(*args, **kwargs)
+
+class StateField(RelatedField):
+    """Avoid the need for a state endpoint.
+
+    NOTE(stephenfin): This field will only function for State names consisting
+    of alphanumeric characters, underscores and single spaces. In Patchwork
+    2.0+, we should consider adding a slug field to the State object and make
+    use of the SlugRelatedField in DRF.
+    """
+    default_error_messages = {
+        'required': _('This field is required.'),
+        'invalid_choice': _('Invalid state {name}. Expected one of: {choices}.'),
+        'incorrect_type': _('Incorrect type. Expected string value, received '
+                            '{data_type}.'),
+    }
 
     def to_internal_value(self, data):
-        data = ' '.join(data.split('-'))
         try:
-            return State.objects.get(name__iexact=data)
+            data = format_state_name(data)
+            return self.get_queryset().get(name__iexact=data)
         except State.DoesNotExist:
-            raise ValidationError('Invalid state. Expected one of: %s ' %
-                                  ', '.join(STATE_CHOICES))
+            self.fail('invalid_choice', name=data, choices=', '.join([
+                format_state_name(x.name) for x in self.get_queryset()]))
+        except (TypeError, ValueError):
+            self.fail('incorrect_type', data_type=type(data).__name__)
 
     def to_representation(self, obj):
         return '-'.join(obj.name.lower().split())
 
+    def get_queryset(self):
+        return State.objects.all()
+
 
 class PatchListSerializer(HyperlinkedModelSerializer):
     mbox = SerializerMethodField()
diff --git a/patchwork/tests/test_rest_api.py b/patchwork/tests/test_rest_api.py
index cc1fcef..b6e6144 100644
--- a/patchwork/tests/test_rest_api.py
+++ b/patchwork/tests/test_rest_api.py
@@ -398,6 +398,20 @@ class TestPatchAPI(APITestCase):
         self.assertEqual(status.HTTP_200_OK, resp.status_code)
         self.assertEqual(Patch.objects.get(id=patch.id).state, state)
 
+    def test_update_invalid(self):
+        """Ensure we handle invalid Patch states."""
+        project = create_project()
+        state = create_state()
+        patch = create_patch(project=project, state=state)
+        user = create_maintainer(project)
+
+        # invalid state
+        self.client.force_authenticate(user=user)
+        resp = self.client.patch(self.api_url(patch.id), {'state': 'foobar'})
+        self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
+        self.assertContains(resp, 'Expected one of: %s.' % state.name,
+                            status_code=status.HTTP_400_BAD_REQUEST)
+
     def test_delete(self):
         """Ensure deletions are always rejected."""
         project = create_project()
-- 
2.9.3