----- Original Message ----- > From: "Daniel Axtens" <d...@axtens.net> > To: "Veronika Kabatova" <vkaba...@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 > >> > >> vkaba...@redhat.com writes: > >> > >> > From: Veronika Kabatova <vkaba...@redhat.com> > >> > > >> > Signed-off-by: Veronika Kabatova <vkaba...@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 > >> > _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork