REST: Allow setting of values using embedded serializers

Unfortunately, the use of embedded serializers for some fields breaks
the ability to update these fields, either via the HTML interface (where
the widget is totally busted) or via a client like 'git-pw'. What we
actually want is to be able to update these fields like normal primary
key but show them using the embedded serializer. We do just this by
using a modified variant of the PrimaryKeyRelatedField and using the
serializers simply for displaying.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Closes: #216
(cherry picked from commit 1a0021a21a8bfe822b469da7fb3e5f0ab6dcaed1)

diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index 1d5aba84a64c3d499de7a66452706bcecb3e78a1..314a27fedcb7b16c0bcfb7bacf983ec6733904b7 100644 (file)
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -23,14 +23,54 @@ A collection of serializers. None of the serializers here should reference
 nested fields.
 """
 
+from collections import OrderedDict
+
 from rest_framework.serializers import CharField
 from rest_framework.serializers import SerializerMethodField
+from rest_framework.serializers import PrimaryKeyRelatedField
 
 from patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import CheckHyperlinkedIdentityField
 from patchwork import models
 
 
+class SerializedRelatedField(PrimaryKeyRelatedField):
+    """
+    A read-write field that expects a primary key for writes and returns a
+    serialized version of the underlying field on reads.
+    """
+
+    def use_pk_only_optimization(self):
+        # We're using embedded serializers so we want the whole object
+        return False
+
+    def get_queryset(self):
+        return self._Serializer.Meta.model.objects.all()
+
+    def get_choices(self, cutoff=None):
+        # Override this so we don't call 'to_representation', which no longer
+        # returns a flat value
+        queryset = self.get_queryset()
+        if queryset is None:
+            # Ensure that field.choices returns something sensible
+            # even when accessed with a read-only field.
+            return {}
+
+        if cutoff is not None:
+            queryset = queryset[:cutoff]
+
+        return OrderedDict([
+            (
+                item.pk,
+                self.display_value(item)
+            )
+            for item in queryset
+        ])
+
+    def to_representation(self, data):
+        return self._Serializer(context=self.context).to_representation(data)
+
+
 class MboxMixin(BaseHyperlinkedModelSerializer):
     """Embed a link to the mbox URL.
 
@@ -55,132 +95,150 @@ class WebURLMixin(BaseHyperlinkedModelSerializer):
         return request.build_absolute_uri(instance.get_absolute_url())
 
 
-class BundleSerializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
-
-    class Meta:
-        model = models.Bundle
-        fields = ('id', 'url', 'web_url', 'name', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-bundle-detail'},
-        }
+class BundleSerializer(SerializedRelatedField):
+
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        class Meta:
+            model = models.Bundle
+            fields = ('id', 'url', 'web_url', 'name', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-bundle-detail'},
+            }
+
+
+class CheckSerializer(SerializedRelatedField):
+
+    class _Serializer(BaseHyperlinkedModelSerializer):
+
+        url = CheckHyperlinkedIdentityField('api-check-detail')
+
+        def to_representation(self, instance):
+            data = super(CheckSerializer._Serializer, self).to_representation(
+                instance)
+            data['state'] = instance.get_state_display()
+            return data
+
+        class Meta:
+            model = models.Check
+            fields = ('id', 'url', 'date', 'state', 'target_url', 'context')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-check-detail'},
+            }
+
+
+class CoverLetterSerializer(SerializedRelatedField):
+
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        class Meta:
+            model = models.CoverLetter
+            fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', 'mbox', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-cover-detail'},
+            }
+
+
+class PatchSerializer(SerializedRelatedField):
+
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        class Meta:
+            model = models.Patch
+            fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-patch-detail'},
+            }
 
 
-class CheckSerializer(BaseHyperlinkedModelSerializer):
+class PersonSerializer(SerializedRelatedField):
 
-    url = CheckHyperlinkedIdentityField('api-check-detail')
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-    def to_representation(self, instance):
-        data = super(CheckSerializer, self).to_representation(instance)
-        data['state'] = instance.get_state_display()
-        return data
+        class Meta:
+            model = models.Person
+            fields = ('id', 'url', 'name', 'email')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-person-detail'},
+            }
 
-    class Meta:
-        model = models.Check
-        fields = ('id', 'url', 'date', 'state', 'target_url', 'context')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-check-detail'},
 
-        }
+class ProjectSerializer(SerializedRelatedField):
 
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-class CoverLetterSerializer(MboxMixin, WebURLMixin,
-                            BaseHyperlinkedModelSerializer):
+        link_name = CharField(max_length=255, source='linkname')
+        list_id = CharField(max_length=255, source='listid')
+        list_email = CharField(max_length=200, source='listemail')
 
-    class Meta:
-        model = models.CoverLetter
-        fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', 'mbox', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-cover-detail'},
-        }
+        class Meta:
+            model = models.Project
+            fields = ('id', 'url', 'name', 'link_name', 'list_id',
+                      'list_email', 'web_url', 'scm_url', 'webscm_url')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-project-detail'},
+            }
 
 
-class PatchSerializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+class SeriesSerializer(SerializedRelatedField):
 
-    class Meta:
-        model = models.Patch
-        fields = ('id', 'url', 'web_url', 'msgid', 'date', 'name', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-patch-detail'},
-        }
+    class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
 
+        class Meta:
+            model = models.Series
+            fields = ('id', 'url', 'date', 'name', 'version', 'mbox')
+            read_only_fields = fields
+            versioned_field = {
+                '1.1': ('web_url', ),
+            }
+            extra_kwargs = {
+                'url': {'view_name': 'api-series-detail'},
+            }
 
-class PersonSerializer(BaseHyperlinkedModelSerializer):
 
-    class Meta:
-        model = models.Person
-        fields = ('id', 'url', 'name', 'email')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-person-detail'},
-        }
+class UserSerializer(SerializedRelatedField):
 
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-class ProjectSerializer(BaseHyperlinkedModelSerializer):
+        class Meta:
+            model = models.User
+            fields = ('id', 'url', 'username', 'first_name', 'last_name',
+                      'email')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-user-detail'},
+            }
 
-    link_name = CharField(max_length=255, source='linkname')
-    list_id = CharField(max_length=255, source='listid')
-    list_email = CharField(max_length=200, source='listemail')
 
-    class Meta:
-        model = models.Project
-        fields = ('id', 'url', 'name', 'link_name', 'list_id', 'list_email',
-                  'web_url', 'scm_url', 'webscm_url')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-project-detail'},
-        }
+class UserProfileSerializer(SerializedRelatedField):
 
+    class _Serializer(BaseHyperlinkedModelSerializer):
 
-class SeriesSerializer(MboxMixin, WebURLMixin,
-                       BaseHyperlinkedModelSerializer):
+        username = CharField(source='user.username')
+        first_name = CharField(source='user.first_name')
+        last_name = CharField(source='user.last_name')
+        email = CharField(source='user.email')
 
-    class Meta:
-        model = models.Series
-        fields = ('id', 'url', 'date', 'name', 'version', 'mbox')
-        read_only_fields = fields
-        versioned_field = {
-            '1.1': ('web_url', ),
-        }
-        extra_kwargs = {
-            'url': {'view_name': 'api-series-detail'},
-        }
-
-
-class UserSerializer(BaseHyperlinkedModelSerializer):
-
-    class Meta:
-        model = models.User
-        fields = ('id', 'url', 'username', 'first_name', 'last_name', 'email')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-user-detail'},
-        }
-
-
-class UserProfileSerializer(BaseHyperlinkedModelSerializer):
-
-    username = CharField(source='user.username')
-    first_name = CharField(source='user.first_name')
-    last_name = CharField(source='user.last_name')
-    email = CharField(source='user.email')
-
-    class Meta:
-        model = models.UserProfile
-        fields = ('id', 'url', 'username', 'first_name', 'last_name', 'email')
-        read_only_fields = fields
-        extra_kwargs = {
-            'url': {'view_name': 'api-user-detail'},
-        }
+        class Meta:
+            model = models.UserProfile
+            fields = ('id', 'url', 'username', 'first_name', 'last_name',
+                      'email')
+            read_only_fields = fields
+            extra_kwargs = {
+                'url': {'view_name': 'api-user-detail'},
+            }

diff --git a/patchwork/tests/api/test_patch.py b/patchwork/tests/api/test_patch.py
index 39915001954721b2c5712313c1d851aa86c02ef3..017707149fe267de478a3fbf676f30996503672c 100644 (file)
--- a/patchwork/tests/api/test_patch.py
+++ b/patchwork/tests/api/test_patch.py
@@ -222,8 +222,7 @@ class TestPatchAPI(APITestCase):
             'state': state.name, 'delegate': user.id})
         self.assertEqual(status.HTTP_200_OK, resp.status_code, resp)
         self.assertEqual(Patch.objects.get(id=patch.id).state, state)
-        # TODO(stephenfin): This is currently broken due to #216
-        # self.assertEqual(Patch.objects.get(id=patch.id).delegate, user)
+        self.assertEqual(Patch.objects.get(id=patch.id).delegate, user)
 
     def test_update_invalid(self):
         """Ensure we handle invalid Patch updates."""
@@ -243,10 +242,9 @@ class TestPatchAPI(APITestCase):
         user_b = create_user()
         resp = self.client.patch(self.api_url(patch.id),
                                  {'delegate': user_b.id})
-        # TODO(stephenfin): This is currently broken due to #216
-        # self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
-        # self.assertContains(resp, "User '%s' is not a maintainer" % user_b,
-        #                     status_code=status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
+        self.assertContains(resp, "User '%s' is not a maintainer" % user_b,
+                            status_code=status.HTTP_400_BAD_REQUEST)
 
     def test_delete(self):
         """Ensure deletions are always rejected."""

diff --git a/releasenotes/notes/issue-216-d3bf9d1baa100f74.yaml b/releasenotes/notes/issue-216-d3bf9d1baa100f74.yaml
new file mode 100644 (file)
index 0000000..c3756aa
--- /dev/null
+++ b/releasenotes/notes/issue-216-d3bf9d1baa100f74.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    An issue that prevented updating of delegates using the REST API is
+    resolved. (`#216 <https://github.com/getpatchwork/patchwork/issues/216>`__)