[v4] api: Add comments to patch and cover endpoints

Message ID	20180425173327.21728-1-vkabatov@redhat.com
State	Accepted
Headers	show Return-Path: <patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org> From: vkabatov@redhat.com To: patchwork@lists.ozlabs.org Subject: [PATCH v4] api: Add comments to patch and cover endpoints Date: Wed, 25 Apr 2018 19:33:27 +0200 Message-Id: <20180425173327.21728-1-vkabatov@redhat.com> Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" <patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>
Series	[v4] api: Add comments to patch and cover endpoints \| expand [v4] api: Add comments to patch and cover endpoints

Veronika Kabatova April 25, 2018, 5:33 p.m. UTC

From: Veronika Kabatova <vkabatov@redhat.com>

Signed-off-by: Veronika Kabatova <vkabatov@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

Stephen Finucane April 27, 2018, 3:37 p.m. UTC | #1

On Wed, 2018-04-25 at 19:33 +0200, vkabatov@redhat.com wrote:
> From: Veronika Kabatova <vkabatov@redhat.com>
> 
> Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>

Two comments below. I've actually addressed these already with my own
patch. Assuming you're happy with said patch, I can squash it with this
one and merge them.

Cheers,
Stephen

> ---
>  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):

Any reason we don't include these in the list resources? Seems like a
sensible thing to include.

>  
>      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):

Ditto.

>      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
> +
> +
> +@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'))
> +
> +
> +@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

Veronika Kabatova April 27, 2018, 3:57 p.m. UTC | #2

----- Original Message -----
> From: "Stephen Finucane" <stephen@that.guru>
> To: vkabatov@redhat.com, patchwork@lists.ozlabs.org
> Sent: Friday, April 27, 2018 5:37:57 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
> 
> On Wed, 2018-04-25 at 19:33 +0200, vkabatov@redhat.com wrote:
> > From: Veronika Kabatova <vkabatov@redhat.com>
> > 
> > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
> 
> Two comments below. I've actually addressed these already with my own
> patch. Assuming you're happy with said patch, I can squash it with this
> one and merge them.
> 

Either way works for me, and your follow-up patch looks good to me. I'll
send my ack right away.

Thanks,
Veronika 

> Cheers,
> Stephen
> 
> > ---
> >  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):
> 
> Any reason we don't include these in the list resources? Seems like a
> sensible thing to include.
> 
> >  
> >      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):
> 
> Ditto.
> 
> >      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
> > +
> > +
> > +@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'))
> > +
> > +
> > +@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
> 
>

Daniel Axtens April 30, 2018, 4:55 p.m. UTC | #3

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?

Regards,
Daniel

vkabatov@redhat.com writes:

> From: Veronika Kabatova <vkabatov@redhat.com>
>
> Signed-off-by: Veronika Kabatova <vkabatov@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
> +
> +
> +@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'))
> +
> +
> +@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork

Veronika Kabatova April 30, 2018, 5:56 p.m. UTC | #4

----- Original Message -----
> From: "Daniel Axtens" <dja@axtens.net>
> To: vkabatov@redhat.com, patchwork@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@redhat.com writes:
> 
> > From: Veronika Kabatova <vkabatov@redhat.com>
> >
> > Signed-off-by: Veronika Kabatova <vkabatov@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
> > +
> > +
> > +@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'))
> > +
> > +
> > +@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@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
>

Daniel Axtens May 2, 2018, 4:22 p.m. UTC | #5

>> 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.

I'm not really sure what you mean by this.

I can get the result I had in mind by changing the url pattern name in
api_1_1_patterns in urls.py (api-comment-list -> api-patch-comment-list
and api-cover-comment-list) - see below.

But I can't seem to do it by changing pk. However, I will readily admit
that I have always struggled with both urlpatterns and the rest API - so
feel free to propose whatever you had in mind and I will test it!

Regards,
Daniel

diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
index 7c80064c8df0..99cf9e68e093 100644
--- a/patchwork/api/cover.py
+++ b/patchwork/api/cover.py
@@ -46,7 +46,7 @@ class CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
 
     def get_comments(self, cover):
         return self.context.get('request').build_absolute_uri(
-            reverse('api-comment-list', kwargs={'pk': cover.id}))
+            reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
 
     class Meta:
         model = CoverLetter
diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
index d1931c013545..028d68544ea6 100644
--- a/patchwork/api/patch.py
+++ b/patchwork/api/patch.py
@@ -94,7 +94,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
 
     def get_comments(self, patch):
         return self.context.get('request').build_absolute_uri(
-            reverse('api-comment-list', kwargs={'pk': patch.id}))
+            reverse('api-patch-comment-list', kwargs={'pk': patch.id}))
 
     def get_check(self, instance):
         return instance.combined_check_state
diff --git a/patchwork/urls.py b/patchwork/urls.py
index 1dc4ffc2b7d3..e90de6b2c6ef 100644
--- a/patchwork/urls.py
+++ b/patchwork/urls.py
@@ -283,10 +283,10 @@ if settings.ENABLE_REST_API:
     api_1_1_patterns = [
         url(r'^patches/(?P<pk>[^/]+)/comments/$',
             api_comment_views.CommentList.as_view(),
-            name='api-comment-list'),
+            name='api-patch-comment-list'),
         url(r'^covers/(?P<pk>[^/]+)/comments/$',
             api_comment_views.CommentList.as_view(),
-            name='api-comment-list'),
+            name='api-cover-comment-list'),
     ]
 
     urlpatterns += [


>
> Thanks,
> Veronika
>
>> Regards,
>> Daniel
>> 
>> vkabatov@redhat.com writes:
>> 
>> > From: Veronika Kabatova <vkabatov@redhat.com>
>> >
>> > Signed-off-by: Veronika Kabatova <vkabatov@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
>> > +
>> > +
>> > +@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'))
>> > +
>> > +
>> > +@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@lists.ozlabs.org
>> > https://lists.ozlabs.org/listinfo/patchwork
>>

Veronika Kabatova May 2, 2018, 5:11 p.m. UTC | #6

----- Original Message -----
> From: "Daniel Axtens" <dja@axtens.net>
> To: "Veronika Kabatova" <vkabatov@redhat.com>
> Cc: patchwork@lists.ozlabs.org
> Sent: Wednesday, May 2, 2018 6:22:34 PM
> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
> 
> >> 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.
> 
> I'm not really sure what you mean by this.
> 
> I can get the result I had in mind by changing the url pattern name in
> api_1_1_patterns in urls.py (api-comment-list -> api-patch-comment-list
> and api-cover-comment-list) - see below.
> 
> But I can't seem to do it by changing pk. However, I will readily admit
> that I have always struggled with both urlpatterns and the rest API - so
> feel free to propose whatever you had in mind and I will test it!
> 

You have to change kwargs in the api as well and fix the submission in
queryset retrieval there too. Your solution is much easier and less
error-prone so I'd go with that :)

It will need the same change in tests too. Do you want to add it or should
I send the patch for both changes?


Veronika


> Regards,
> Daniel
> 
> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> index 7c80064c8df0..99cf9e68e093 100644
> --- a/patchwork/api/cover.py
> +++ b/patchwork/api/cover.py
> @@ -46,7 +46,7 @@ class
> CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
>  
>      def get_comments(self, cover):
>          return self.context.get('request').build_absolute_uri(
> -            reverse('api-comment-list', kwargs={'pk': cover.id}))
> +            reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
>  
>      class Meta:
>          model = CoverLetter
> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> index d1931c013545..028d68544ea6 100644
> --- a/patchwork/api/patch.py
> +++ b/patchwork/api/patch.py
> @@ -94,7 +94,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
>  
>      def get_comments(self, patch):
>          return self.context.get('request').build_absolute_uri(
> -            reverse('api-comment-list', kwargs={'pk': patch.id}))
> +            reverse('api-patch-comment-list', kwargs={'pk': patch.id}))
>  
>      def get_check(self, instance):
>          return instance.combined_check_state
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 1dc4ffc2b7d3..e90de6b2c6ef 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -283,10 +283,10 @@ if settings.ENABLE_REST_API:
>      api_1_1_patterns = [
>          url(r'^patches/(?P<pk>[^/]+)/comments/$',
>              api_comment_views.CommentList.as_view(),
> -            name='api-comment-list'),
> +            name='api-patch-comment-list'),
>          url(r'^covers/(?P<pk>[^/]+)/comments/$',
>              api_comment_views.CommentList.as_view(),
> -            name='api-comment-list'),
> +            name='api-cover-comment-list'),
>      ]
>  
>      urlpatterns += [
> 
> 
> >
> > Thanks,
> > Veronika
> >
> >> Regards,
> >> Daniel
> >> 
> >> vkabatov@redhat.com writes:
> >> 
> >> > From: Veronika Kabatova <vkabatov@redhat.com>
> >> >
> >> > Signed-off-by: Veronika Kabatova <vkabatov@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
> >> > +
> >> > +
> >> > +@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'))
> >> > +
> >> > +
> >> > +@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@lists.ozlabs.org
> >> > https://lists.ozlabs.org/listinfo/patchwork
> >> 
>

Daniel Axtens May 3, 2018, 12:15 a.m. UTC | #7

Veronika Kabatova <vkabatov@redhat.com> writes:

> ----- Original Message -----
>> From: "Daniel Axtens" <dja@axtens.net>
>> To: "Veronika Kabatova" <vkabatov@redhat.com>
>> Cc: patchwork@lists.ozlabs.org
>> Sent: Wednesday, May 2, 2018 6:22:34 PM
>> Subject: Re: [PATCH v4] api: Add comments to patch and cover endpoints
>> 
>> >> 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.
>> 
>> I'm not really sure what you mean by this.
>> 
>> I can get the result I had in mind by changing the url pattern name in
>> api_1_1_patterns in urls.py (api-comment-list -> api-patch-comment-list
>> and api-cover-comment-list) - see below.
>> 
>> But I can't seem to do it by changing pk. However, I will readily admit
>> that I have always struggled with both urlpatterns and the rest API - so
>> feel free to propose whatever you had in mind and I will test it!
>> 
>
> You have to change kwargs in the api as well and fix the submission in
> queryset retrieval there too. Your solution is much easier and less
> error-prone so I'd go with that :)

Sounds good.
>
> It will need the same change in tests too. Do you want to add it or should
> I send the patch for both changes?
>
Please go ahead and send a patch.

Thanks!

Regards,
Daniel

>
> Veronika
>
>
>> Regards,
>> Daniel
>> 
>> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
>> index 7c80064c8df0..99cf9e68e093 100644
>> --- a/patchwork/api/cover.py
>> +++ b/patchwork/api/cover.py
>> @@ -46,7 +46,7 @@ class
>> CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
>>  
>>      def get_comments(self, cover):
>>          return self.context.get('request').build_absolute_uri(
>> -            reverse('api-comment-list', kwargs={'pk': cover.id}))
>> +            reverse('api-cover-comment-list', kwargs={'pk': cover.id}))
>>  
>>      class Meta:
>>          model = CoverLetter
>> diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
>> index d1931c013545..028d68544ea6 100644
>> --- a/patchwork/api/patch.py
>> +++ b/patchwork/api/patch.py
>> @@ -94,7 +94,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer):
>>  
>>      def get_comments(self, patch):
>>          return self.context.get('request').build_absolute_uri(
>> -            reverse('api-comment-list', kwargs={'pk': patch.id}))
>> +            reverse('api-patch-comment-list', kwargs={'pk': patch.id}))
>>  
>>      def get_check(self, instance):
>>          return instance.combined_check_state
>> diff --git a/patchwork/urls.py b/patchwork/urls.py
>> index 1dc4ffc2b7d3..e90de6b2c6ef 100644
>> --- a/patchwork/urls.py
>> +++ b/patchwork/urls.py
>> @@ -283,10 +283,10 @@ if settings.ENABLE_REST_API:
>>      api_1_1_patterns = [
>>          url(r'^patches/(?P<pk>[^/]+)/comments/$',
>>              api_comment_views.CommentList.as_view(),
>> -            name='api-comment-list'),
>> +            name='api-patch-comment-list'),
>>          url(r'^covers/(?P<pk>[^/]+)/comments/$',
>>              api_comment_views.CommentList.as_view(),
>> -            name='api-comment-list'),
>> +            name='api-cover-comment-list'),
>>      ]
>>  
>>      urlpatterns += [
>> 
>> 
>> >
>> > Thanks,
>> > Veronika
>> >
>> >> Regards,
>> >> Daniel
>> >> 
>> >> vkabatov@redhat.com writes:
>> >> 
>> >> > From: Veronika Kabatova <vkabatov@redhat.com>
>> >> >
>> >> > Signed-off-by: Veronika Kabatova <vkabatov@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
>> >> > +
>> >> > +
>> >> > +@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'))
>> >> > +
>> >> > +
>> >> > +@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@lists.ozlabs.org
>> >> > https://lists.ozlabs.org/listinfo/patchwork
>> >> 
>>

[v4] api: Add comments to patch and cover endpoints

Commit Message

Comments

Patch