On Tue, 2018-04-17 at 12:50 +0200, vkaba...@redhat.com wrote:
> From: Veronika Kabatova <vkaba...@redhat.com>
>
> Signed-off-by: Veronika Kabatova <vkaba...@redhat.com>
> ---
> New changes: move things under /patches|covers/ID/comments
Almost there - just some queries to clean up.
> ---
> patchwork/api/comment.py | 80 ++++++++++++++
> 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, 267 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..a650168
> --- /dev/null
> +++ b/patchwork/api/comment.py
> @@ -0,0 +1,80 @@
> +# 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.api.embedded import ProjectSerializer
> +from patchwork.models import Comment
> +
> +
> +class CommentListSerializer(BaseHyperlinkedModelSerializer):
> +
> + project = ProjectSerializer(source='submission.project', read_only=True)
This results in N * 2 addition queries for N comments because of the
extra reads to 'submission' and 'project'. You can resolve this
somewhat, as noted below, but I wonder if this is even necessary?
Couldn't we infer it from the parent patch?
> + tags = SerializerMethodField()
> + subject = SerializerMethodField()
> + headers = SerializerMethodField()
> + submitter = PersonSerializer(read_only=True)
> +
> + def get_tags(self, comment):
> + # TODO implement after we get support for tags on comments
> + return {}
> +
> + 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', 'project', 'submitter',
> + 'tags', 'content', 'headers')
Personally I'd drop the 'tags' field until such a time as we _do_ have
support for this. The only reason they currently exist for '/patches'
is because API v1.0 was out in the wild before I realized there were
performance issues.
> + 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'])
To resolve the issue above, you'll need to modify this like so:
return Comment.objects\
.filter(submission=self.kwargs['pk'])\
.select_related('submitter', 'submission__project')
However, as noted above, I'm not sure if we even need/want to include
'project'. Assuming we don't, you just need the 'submitter' bit.
> 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'),
> + }
Good spot (the 'mbox' field).
>
>
> 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'),
> + ]
> +
I was wondering if you'd remember this. Nicely done :)
> 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..5bc4773
> --- /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
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
