[PATCH v4] api: Add comments to patch and cover endpoints
Veronika Kabatova
vkabatov at redhat.com
Tue May 1 03:56:15 AEST 2018
----- Original Message -----
> From: "Daniel Axtens" <dja at axtens.net>
> To: vkabatov at redhat.com, patchwork at lists.ozlabs.org
> Sent: Monday, April 30, 2018 6:55:11 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
>
> Apologies if this is a silly question, but I was looking at this to
> check the perfomance, and I noticed that in the patch list endpoint
> (/api/patches/), it seems to be linking to the 'covers' comments
> endpoint rather than the 'patches' api endpoint:
>
> ...
> "delegate": null,
> "mbox": "http://localhost:8000/patch/1/mbox/",
> "series": [
> ...
> ],
> **** "comments": "http://localhost:8000/api/covers/1/comments/",
> "check": "pending",
> "checks": "http://localhost:8000/api/patches/1/checks/",
> "tags": {}
> ...
>
> Notice that it's /patch/1/ for mbox, checks, etc, but /covers/1/ for
> comments.
>
> Is this intentional - am I missing something? - or is this supposed to
> be /patch/1/ like the others?
>
Ookay, this is pretty embarrassing. reverse() looks to be confused when
you have multiple matches for the view and kwargs passed, ignoring the
origin of the request. I definitely didn't notice it - and Stephen probably
didn't either since he didn't bring it up.
Simply changing "pk" to unique identifiers fixes it for me. Can you verify
it does so on your instance as well? I'll send a patch if it does.
Thanks,
Veronika
> Regards,
> Daniel
>
> vkabatov at redhat.com writes:
>
> > From: Veronika Kabatova <vkabatov at redhat.com>
> >
> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
> > ---
> > patchwork/api/comment.py | 75 ++++++++++++++
> > patchwork/api/cover.py | 13 ++-
> > patchwork/api/patch.py | 16 ++-
> > patchwork/models.py | 3 +
> > patchwork/tests/api/test_comment.py | 115
> > +++++++++++++++++++++
> > patchwork/tests/api/test_cover.py | 11 +-
> > patchwork/tests/api/test_patch.py | 19 +++-
> > patchwork/urls.py | 11 ++
> > .../notes/comments-api-b7dff6ee4ce04c9b.yaml | 8 ++
> > 9 files changed, 262 insertions(+), 9 deletions(-)
> > create mode 100644 patchwork/api/comment.py
> > create mode 100644 patchwork/tests/api/test_comment.py
> > create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> >
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 0000000..fbb7cc8
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,75 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# This file is part of the Patchwork package.
> > +#
> > +# Patchwork is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# Patchwork is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with Patchwork; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> > USA
> > +
> > +import email.parser
> > +
> > +from rest_framework.generics import ListAPIView
> > +from rest_framework.serializers import SerializerMethodField
> > +
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > +from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.embedded import PersonSerializer
> > +from patchwork.models import Comment
> > +
> > +
> > +class CommentListSerializer(BaseHyperlinkedModelSerializer):
> > +
> > + subject = SerializerMethodField()
> > + headers = SerializerMethodField()
> > + submitter = PersonSerializer(read_only=True)
> > +
> > + def get_subject(self, comment):
> > + return email.parser.Parser().parsestr(comment.headers,
> > + True).get('Subject', '')
> > +
> > + def get_headers(self, comment):
> > + headers = {}
> > +
> > + if comment.headers:
> > + parsed = email.parser.Parser().parsestr(comment.headers, True)
> > + for key in parsed.keys():
> > + headers[key] = parsed.get_all(key)
> > + # Let's return a single string instead of a list if only
> > one
> > + # header with this key is present
> > + if len(headers[key]) == 1:
> > + headers[key] = headers[key][0]
> > +
> > + return headers
> > +
> > + class Meta:
> > + model = Comment
> > + fields = ('id', 'msgid', 'date', 'subject', 'submitter',
> > 'content',
> > + 'headers')
> > + read_only_fields = fields
> > +
> > +
> > +class CommentList(ListAPIView):
> > + """List comments"""
> > +
> > + permission_classes = (PatchworkPermission,)
> > + serializer_class = CommentListSerializer
> > + search_fields = ('subject',)
> > + ordering_fields = ('id', 'subject', 'date', 'submitter')
> > + ordering = 'id'
> > + lookup_url_kwarg = 'pk'
> > +
> > + def get_queryset(self):
> > + return Comment.objects.filter(
> > + submission=self.kwargs['pk']
> > + ).select_related('submitter')
> > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> > index fc7ae97..19f0f65 100644
> > --- a/patchwork/api/cover.py
> > +++ b/patchwork/api/cover.py
> > @@ -21,6 +21,7 @@ import email.parser
> >
> > from rest_framework.generics import ListAPIView
> > from rest_framework.generics import RetrieveAPIView
> > +from rest_framework.reverse import reverse
> > from rest_framework.serializers import SerializerMethodField
> >
> > from patchwork.api.base import BaseHyperlinkedModelSerializer
> > @@ -58,6 +59,11 @@ class
> > CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> > class CoverLetterDetailSerializer(CoverLetterListSerializer):
> >
> > headers = SerializerMethodField()
> > + comments = SerializerMethodField()
> > +
> > + def get_comments(self, cover):
> > + return self.context.get('request').build_absolute_uri(
> > + reverse('api-comment-list', kwargs={'pk': cover.id}))
> >
> > def get_headers(self, instance):
> > if instance.headers:
> > @@ -65,9 +71,14 @@ class
> > CoverLetterDetailSerializer(CoverLetterListSerializer):
> >
> > class Meta:
> > model = CoverLetter
> > - fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > 'content')
> > + fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > + 'content',
> > + 'comments')
> > read_only_fields = fields
> > extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs
> > + versioned_fields = {
> > + '1.1': ('mbox', 'comments'),
> > + }
> >
> >
> > class CoverLetterList(ListAPIView):
> > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> > index 115feff..feb7d80 100644
> > --- a/patchwork/api/patch.py
> > +++ b/patchwork/api/patch.py
> > @@ -24,9 +24,9 @@ from rest_framework.generics import ListAPIView
> > from rest_framework.generics import RetrieveUpdateAPIView
> > from rest_framework.relations import RelatedField
> > from rest_framework.reverse import reverse
> > -from rest_framework.serializers import HyperlinkedModelSerializer
> > from rest_framework.serializers import SerializerMethodField
> >
> > +from patchwork.api.base import BaseHyperlinkedModelSerializer
> > from patchwork.api.base import PatchworkPermission
> > from patchwork.api.filters import PatchFilter
> > from patchwork.api.embedded import PersonSerializer
> > @@ -75,7 +75,7 @@ class StateField(RelatedField):
> > return State.objects.all()
> >
> >
> > -class PatchListSerializer(HyperlinkedModelSerializer):
> > +class PatchListSerializer(BaseHyperlinkedModelSerializer):
> >
> > project = ProjectSerializer(read_only=True)
> > state = StateField()
> > @@ -121,6 +121,11 @@ class PatchDetailSerializer(PatchListSerializer):
> >
> > headers = SerializerMethodField()
> > prefixes = SerializerMethodField()
> > + comments = SerializerMethodField()
> > +
> > + def get_comments(self, patch):
> > + return self.context.get('request').build_absolute_uri(
> > + reverse('api-comment-list', kwargs={'pk': patch.id}))
> >
> > def get_headers(self, patch):
> > if patch.headers:
> > @@ -132,10 +137,13 @@ class PatchDetailSerializer(PatchListSerializer):
> > class Meta:
> > model = Patch
> > fields = PatchListSerializer.Meta.fields + (
> > - 'headers', 'content', 'diff', 'prefixes')
> > + 'headers', 'content', 'diff', 'prefixes', 'comments')
> > read_only_fields = PatchListSerializer.Meta.read_only_fields + (
> > - 'headers', 'content', 'diff', 'prefixes')
> > + 'headers', 'content', 'diff', 'prefixes', 'comments')
> > extra_kwargs = PatchListSerializer.Meta.extra_kwargs
> > + versioned_fields = {
> > + '1.1': ('comments', ),
> > + }
> >
> >
> > class PatchList(ListAPIView):
> > diff --git a/patchwork/models.py b/patchwork/models.py
> > index f91b994..67c2d3a 100644
> > --- a/patchwork/models.py
> > +++ b/patchwork/models.py
> > @@ -613,6 +613,9 @@ class Comment(EmailMixin, models.Model):
> > if hasattr(self.submission, 'patch'):
> > self.submission.patch.refresh_tag_counts()
> >
> > + def is_editable(self, user):
> > + return False
> > +
> > class Meta:
> > ordering = ['date']
> > unique_together = [('msgid', 'submission')]
> > diff --git a/patchwork/tests/api/test_comment.py
> > b/patchwork/tests/api/test_comment.py
> > new file mode 100644
> > index 0000000..b283bfe
> > --- /dev/null
> > +++ b/patchwork/tests/api/test_comment.py
> > @@ -0,0 +1,115 @@
> > +# Patchwork - automated patch tracking system
> > +# Copyright (C) 2018 Red Hat
> > +#
> > +# This file is part of the Patchwork package.
> > +#
> > +# Patchwork is free software; you can redistribute it and/or modify
> > +# it under the terms of the GNU General Public License as published by
> > +# the Free Software Foundation; either version 2 of the License, or
> > +# (at your option) any later version.
> > +#
> > +# Patchwork is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with Patchwork; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> > USA
> > +
> > +import unittest
> > +
> > +from django.conf import settings
> > +
> > +from patchwork.compat import NoReverseMatch
> > +from patchwork.compat import reverse
> > +from patchwork.tests.utils import create_comment
> > +from patchwork.tests.utils import create_cover
> > +from patchwork.tests.utils import create_patch
> > +from patchwork.tests.utils import SAMPLE_CONTENT
> > +
> > +if settings.ENABLE_REST_API:
> > + from rest_framework import status
> > + from rest_framework.test import APITestCase
> > +else:
> > + # stub out APITestCase
> > + from django.test import TestCase
> > + APITestCase = TestCase # noqa
> > +
> > +
> > + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> > +class TestCoverComments(APITestCase):
> > + @staticmethod
> > + def api_url(cover, version=None):
> > + kwargs = {}
> > + if version:
> > + kwargs['version'] = version
> > + kwargs['pk'] = cover.id
> > +
> > + return reverse('api-comment-list', kwargs=kwargs)
> > +
> > + def assertSerialized(self, comment_obj, comment_json):
> > + self.assertEqual(comment_obj.id, comment_json['id'])
> > + self.assertEqual(comment_obj.submitter.id,
> > + comment_json['submitter']['id'])
> > + self.assertIn(SAMPLE_CONTENT, comment_json['content'])
> > +
> > + def test_list(self):
> > + cover_obj = create_cover()
> > + resp = self.client.get(self.api_url(cover_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(0, len(resp.data))
> > +
> > + comment_obj = create_comment(submission=cover_obj)
> > + resp = self.client.get(self.api_url(cover_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(1, len(resp.data))
> > + self.assertSerialized(comment_obj, resp.data[0])
> > +
> > + another_comment = create_comment(submission=cover_obj)
> > + resp = self.client.get(self.api_url(cover_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(2, len(resp.data))
> > +
> > + # check we can't access comments using the old version of the API
> > + with self.assertRaises(NoReverseMatch):
> > + self.client.get(self.api_url(cover_obj, version='1.0'))
> > +
> > +
> > + at unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> > +class TestPatchComments(APITestCase):
> > + @staticmethod
> > + def api_url(patch, version=None):
> > + kwargs = {}
> > + if version:
> > + kwargs['version'] = version
> > + kwargs['pk'] = patch.id
> > +
> > + return reverse('api-comment-list', kwargs=kwargs)
> > +
> > + def assertSerialized(self, comment_obj, comment_json):
> > + self.assertEqual(comment_obj.id, comment_json['id'])
> > + self.assertEqual(comment_obj.submitter.id,
> > + comment_json['submitter']['id'])
> > + self.assertIn(SAMPLE_CONTENT, comment_json['content'])
> > +
> > + def test_list(self):
> > + patch_obj = create_patch()
> > + resp = self.client.get(self.api_url(patch_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(0, len(resp.data))
> > +
> > + comment_obj = create_comment(submission=patch_obj)
> > + resp = self.client.get(self.api_url(patch_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(1, len(resp.data))
> > + self.assertSerialized(comment_obj, resp.data[0])
> > +
> > + another_comment = create_comment(submission=patch_obj)
> > + resp = self.client.get(self.api_url(patch_obj))
> > + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > + self.assertEqual(2, len(resp.data))
> > +
> > + # check we can't access comments using the old version of the API
> > + with self.assertRaises(NoReverseMatch):
> > + self.client.get(self.api_url(patch_obj, version='1.0'))
> > diff --git a/patchwork/tests/api/test_cover.py
> > b/patchwork/tests/api/test_cover.py
> > index 3135b7e..9a0fbbb 100644
> > --- a/patchwork/tests/api/test_cover.py
> > +++ b/patchwork/tests/api/test_cover.py
> > @@ -49,7 +49,8 @@ class TestCoverLetterAPI(APITestCase):
> >
> > if item is None:
> > return reverse('api-cover-list', kwargs=kwargs)
> > - return reverse('api-cover-detail', args=[item], kwargs=kwargs)
> > + kwargs['pk'] = item
> > + return reverse('api-cover-detail', kwargs=kwargs)
> >
> > def assertSerialized(self, cover_obj, cover_json):
> > self.assertEqual(cover_obj.id, cover_json['id'])
> > @@ -115,6 +116,14 @@ class TestCoverLetterAPI(APITestCase):
> > self.assertEqual(status.HTTP_200_OK, resp.status_code)
> > self.assertSerialized(cover_obj, resp.data)
> >
> > + # test comments
> > + resp = self.client.get(self.api_url(cover_obj.id))
> > + self.assertIn('comments', resp.data)
> > +
> > + # test old version of API
> > + resp = self.client.get(self.api_url(cover_obj.id, version='1.0'))
> > + self.assertNotIn('comments', resp.data)
> > +
> > def test_create_update_delete(self):
> > user = create_maintainer()
> > user.is_superuser = True
> > diff --git a/patchwork/tests/api/test_patch.py
> > b/patchwork/tests/api/test_patch.py
> > index 909c1eb..d9d44d3 100644
> > --- a/patchwork/tests/api/test_patch.py
> > +++ b/patchwork/tests/api/test_patch.py
> > @@ -45,10 +45,15 @@ class TestPatchAPI(APITestCase):
> > fixtures = ['default_tags']
> >
> > @staticmethod
> > - def api_url(item=None):
> > + def api_url(item=None, version=None):
> > + kwargs = {}
> > + if version:
> > + kwargs['version'] = version
> > +
> > if item is None:
> > - return reverse('api-patch-list')
> > - return reverse('api-patch-detail', args=[item])
> > + return reverse('api-patch-list', kwargs=kwargs)
> > + kwargs['pk'] = item
> > + return reverse('api-patch-detail', kwargs=kwargs)
> >
> > def assertSerialized(self, patch_obj, patch_json):
> > self.assertEqual(patch_obj.id, patch_json['id'])
> > @@ -130,6 +135,14 @@ class TestPatchAPI(APITestCase):
> > self.assertEqual(patch.diff, resp.data['diff'])
> > self.assertEqual(0, len(resp.data['tags']))
> >
> > + # test comments
> > + resp = self.client.get(self.api_url(patch.id))
> > + self.assertIn('comments', resp.data)
> > +
> > + # test old version of API
> > + resp = self.client.get(self.api_url(item=patch.id, version='1.0'))
> > + self.assertNotIn('comments', resp.data)
> > +
> > def test_create(self):
> > """Ensure creations are rejected."""
> > project = create_project()
> > diff --git a/patchwork/urls.py b/patchwork/urls.py
> > index 0893fe2..1dc4ffc 100644
> > --- a/patchwork/urls.py
> > +++ b/patchwork/urls.py
> > @@ -213,6 +213,7 @@ if settings.ENABLE_REST_API:
> >
> > from patchwork.api import bundle as api_bundle_views # noqa
> > from patchwork.api import check as api_check_views # noqa
> > + from patchwork.api import comment as api_comment_views # noqa
> > from patchwork.api import cover as api_cover_views # noqa
> > from patchwork.api import event as api_event_views # noqa
> > from patchwork.api import index as api_index_views # noqa
> > @@ -279,8 +280,18 @@ if settings.ENABLE_REST_API:
> > name='api-event-list'),
> > ]
> >
> > + api_1_1_patterns = [
> > + url(r'^patches/(?P<pk>[^/]+)/comments/$',
> > + api_comment_views.CommentList.as_view(),
> > + name='api-comment-list'),
> > + url(r'^covers/(?P<pk>[^/]+)/comments/$',
> > + api_comment_views.CommentList.as_view(),
> > + name='api-comment-list'),
> > + ]
> > +
> > urlpatterns += [
> > url(r'^api/(?:(?P<version>(1.0|1.1))/)?', include(api_patterns)),
> > + url(r'^api/(?:(?P<version>1.1)/)?', include(api_1_1_patterns)),
> >
> > # token change
> > url(r'^user/generate-token/$', user_views.generate_token,
> > diff --git a/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > new file mode 100644
> > index 0000000..6cd6441
> > --- /dev/null
> > +++ b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > @@ -0,0 +1,8 @@
> > +---
> > +api:
> > + - |
> > + Links to related comments are now exposed when checking patch and
> > cover
> > + letter details. The comments themselves are then available via
> > + ``/patches/{patchID}/comments`` and ``/covers/{coverID}/comments``
> > + endpoints. Please note that comments are available only since API
> > + version 1.1
> > --
> > 2.13.6
> >
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
>
More information about the Patchwork
mailing list