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 <step...@that.guru>
Closes: #216
---
I'm planning to backport this to stable/2.1 and stable/2.0. Technically
this is a change in behavior but as this was a bug, I think it's a
reasonable fix.
---
patchwork/api/embedded.py | 272 +++++++++++-------
patchwork/tests/api/test_patch.py | 10 +-
...fix-patch-delegation-74d7d90f0fb55989.yaml | 5 +
3 files changed, 174 insertions(+), 113 deletions(-)
create mode 100644
releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.yaml
diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index be61cd5f..83df0e50 100644
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -9,14 +9,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.
@@ -41,132 +81,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 53099256..497cb2de 100644
--- a/patchwork/tests/api/test_patch.py
+++ b/patchwork/tests/api/test_patch.py
@@ -208,8 +208,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."""
@@ -229,10 +228,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/fix-patch-delegation-74d7d90f0fb55989.yaml
b/releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.yaml
new file mode 100644
index 00000000..c3756aa0
--- /dev/null
+++ b/releasenotes/notes/fix-patch-delegation-74d7d90f0fb55989.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>`__)
--
2.17.1
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
