From: Stephen Finucane Date: Thu, 11 Oct 2018 10:47:56 +0000 (+0100) Subject: REST: Allow setting of values using embedded serializers X-Git-Tag: v2.1.1~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e1ae71a9f6a33c636a4d6babca36236d8a6c29ac;p=thirdparty%2Fpatchwork.git 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 Closes: #216 (cherry picked from commit 1a0021a21a8bfe822b469da7fb3e5f0ab6dcaed1) --- diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py index 1d5aba84..314a27fe 100644 --- 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 39915001..01770714 100644 --- 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 index 00000000..c3756aa0 --- /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 `__)