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