[PATCH] Add comments REST API
Veronika Kabatova
vkabatov at redhat.com
Mon Apr 9 23:29:13 AEST 2018
----- Original Message -----
> From: "Stephen Finucane" <stephen at that.guru>
> To: vkabatov at redhat.com, patchwork at lists.ozlabs.org
> Sent: Monday, April 9, 2018 3:02:57 PM
> Subject: Re: [PATCH] Add comments REST API
>
> On Fri, 2018-03-23 at 13:33 +0100, vkabatov at redhat.com wrote:
> > From: Veronika Kabatova <vkabatov at redhat.com>
> >
> > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
>
Thanks for feedback!
> Looks pretty good. As you note, it does need some tests. I'm also
> curious about the idea of nesting this under the patches endpoint. Let
> me know what you think.
>
Hmm, that would require the same change for cover letters too. How do
you imagine it? Having all related comments under the detailed patch or
cover, or something else?
I opted for separate API because some people may not care about comments
(since AFAIK noone requested it yet), but we very much do. At the same
time I didn't want to push our needs on others. If you don't think it's
an issue I can rework this and put it under the patch/cover endpoints.
> Stephen
>
> > ---
> > docs/api/rest.rst | 6 +-
> > patchwork/api/comment.py | 117
> > +++++++++++++++++++++
> > patchwork/api/filters.py | 16 +++
> > patchwork/api/index.py | 1 +
> > patchwork/models.py | 3 +
> > patchwork/urls.py | 10 ++
> > .../notes/comments-api-b7dff6ee4ce04c9b.yaml | 10 ++
> > 7 files changed, 161 insertions(+), 2 deletions(-)
> > create mode 100644 patchwork/api/comment.py
> > create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> >
> > diff --git a/docs/api/rest.rst b/docs/api/rest.rst
> > index d526b27..e33aefc 100644
> > --- a/docs/api/rest.rst
> > +++ b/docs/api/rest.rst
> > @@ -48,7 +48,8 @@ Patchwork instance hosted at `patchwork.example.com`,
> > run:
> > "people": "https://patchwork.example.com/api/1.0/people/",
> > "projects": "https://patchwork.example.com/api/1.0/projects/",
> > "series": "https://patchwork.example.com/api/1.0/series/",
> > - "users": "https://patchwork.example.com/api/1.0/users/"
> > + "users": "https://patchwork.example.com/api/1.0/users/",
> > + "comments": "https://patchwork.example.com/api/1.0/comments/"
> > }
> >
> >
> > @@ -71,7 +72,8 @@ well-supported. To repeat the above example using
> > `requests`:, run
> > "people": "https://patchwork.example.com/api/1.0/people/",
> > "projects": "https://patchwork.example.com/api/1.0/projects/",
> > "series": "https://patchwork.example.com/api/1.0/series/",
> > - "users": "https://patchwork.example.com/api/1.0/users/"
> > + "users": "https://patchwork.example.com/api/1.0/users/",
> > + "comments": "https://patchwork.example.com/api/1.0/comments/"
> > }
> >
> > Tools like `curl` and libraries like `requests` can be used to build
> > anything
> > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> > new file mode 100644
> > index 0000000..4252ecd
> > --- /dev/null
> > +++ b/patchwork/api/comment.py
> > @@ -0,0 +1,117 @@
> > +# 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.generics import RetrieveUpdateAPIView
> > +from rest_framework.reverse import reverse
> > +from rest_framework.serializers import HyperlinkedModelSerializer
> > +from rest_framework.serializers import SerializerMethodField
> > +
> > +from patchwork.api.base import PatchworkPermission
> > +from patchwork.api.filters import CommentFilter
> > +from patchwork.api.embedded import PersonSerializer
> > +from patchwork.api.embedded import ProjectSerializer
> > +from patchwork.models import Comment
> > +
> > +
> > +class CommentListSerializer(HyperlinkedModelSerializer):
> > +
> > + submitter = PersonSerializer(read_only=True)
> > + tags = SerializerMethodField()
> > + subject = SerializerMethodField()
> > + parent = SerializerMethodField(source='submission')
> > +
> > + def get_parent(self, instance):
> > + attrs = {'subject': instance.submission.name,
> > + 'msgid': instance.submission.msgid,
> > + 'date': instance.submission.date}
> > +
> > + if hasattr(instance.submission, 'patch'):
> > + attrs['url'] = self.context.get('request').build_absolute_uri(
> > + reverse('api-patch-detail',
> > args=[instance.submission.id]))
> > + else:
> > + attrs['url'] = self.context.get('request').build_absolute_uri(
> > + reverse('api-cover-detail',
> > args=[instance.submission.id]))
> > +
> > + return attrs
> > +
> > + def get_subject(self, instance):
> > + return email.parser.Parser().parsestr(instance.headers,
> > + True).get('Subject', '')
> > +
> > + def get_tags(self, instance):
> > + # TODO implement after we get support for tags on comments
> > + return {}
> > +
> > + class Meta:
> > + model = Comment
> > + fields = ('id', 'url', 'msgid', 'date', 'subject', 'submitter',
> > + 'parent', 'tags')
> > + read_only_fields = fields
> > + extra_kwargs = {
> > + 'url': {'view_name': 'api-comment-detail'},
> > + }
> > +
> > +
> > +class CommentDetailSerializer(CommentListSerializer):
> > +
> > + headers = SerializerMethodField()
> > + project = ProjectSerializer(source='submission.project',
> > read_only=True)
> > +
> > + def get_headers(self, comment):
> > + if comment.headers:
> > + return email.parser.Parser().parsestr(comment.headers, True)
> > +
> > + class Meta:
> > + model = Comment
> > + fields = CommentListSerializer.Meta.fields + (
> > + 'content', 'headers', 'project'
> > + )
> > + read_only_fields = fields
> > + extra_kwargs = CommentListSerializer.Meta.extra_kwargs
> > +
> > +
> > +class CommentList(ListAPIView):
> > + """List comments"""
> > +
> > + permission_classes = (PatchworkPermission,)
> > + serializer_class = CommentListSerializer
> > + filter_class = CommentFilter
> > + search_fields = ('subject',)
> > + ordering_fields = ('id', 'subject', 'date', 'submitter')
> > + ordering = 'id'
> > +
> > + def get_queryset(self):
> > + return Comment.objects.all().select_related(
> > + 'submission'
> > + ).prefetch_related('related_tags').defer('content')
> > +
> > +
> > +class CommentDetail(RetrieveUpdateAPIView):
> > + """Show a comment"""
> > +
> > + permission_classes = (PatchworkPermission,)
> > + serializer_class = CommentDetailSerializer
> > +
> > + def get_queryset(self):
> > + return Comment.objects.all().select_related(
> > + 'submission'
> > + ).prefetch_related('related_tags')
> > diff --git a/patchwork/api/filters.py b/patchwork/api/filters.py
> > index d207b2b..8256d8c 100644
> > --- a/patchwork/api/filters.py
> > +++ b/patchwork/api/filters.py
> > @@ -26,6 +26,7 @@ from django.forms import ModelChoiceField
> >
> > from patchwork.models import Bundle
> > from patchwork.models import Check
> > +from patchwork.models import Comment
> > from patchwork.models import CoverLetter
> > from patchwork.models import Event
> > from patchwork.models import Patch
> > @@ -33,6 +34,7 @@ from patchwork.models import Person
> > from patchwork.models import Project
> > from patchwork.models import Series
> > from patchwork.models import State
> > +from patchwork.models import Submission
> >
> >
> > class TimestampMixin(FilterSet):
> > @@ -186,3 +188,17 @@ class BundleFilter(ProjectMixin, FilterSet):
> > class Meta:
> > model = Bundle
> > fields = ('project', 'owner', 'public')
> > +
> > +
> > +class CommentFilter(ProjectMixin, TimestampMixin, FilterSet):
> > +
> > + submitter = PersonFilter(queryset=Person.objects.all())
> > + parent = ModelChoiceFilter(name='submission',
> > + queryset=Submission.objects.all())
>
> Assuming we go this route, can you call this 'submission'?
>
Sure, I can change it back.
> > + project = ProjectFilter(to_field_name='linkname',
> > + name='submission__project',
> > + queryset=Project.objects.all())
> > +
> > + class Meta:
> > + model = Comment
> > + fields = ('project', 'parent', 'submitter')
> > diff --git a/patchwork/api/index.py b/patchwork/api/index.py
> > index 53494db..ebc8dbf 100644
> > --- a/patchwork/api/index.py
> > +++ b/patchwork/api/index.py
> > @@ -34,4 +34,5 @@ class IndexView(APIView):
> > 'series': reverse('api-series-list', request=request),
> > 'events': reverse('api-event-list', request=request),
> > 'bundles': reverse('api-bundle-list', request=request),
> > + 'comments': reverse('api-comment-list', request=request),
> > })
> > 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/urls.py b/patchwork/urls.py
> > index 7193472..7ae4425 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
> > @@ -238,6 +239,12 @@ if settings.ENABLE_REST_API:
> > url(r'^people/(?P<pk>[^/]+)/$',
> > api_person_views.PersonDetail.as_view(),
> > name='api-person-detail'),
> > + url(r'^comments/$',
> > + api_comment_views.CommentList.as_view(),
> > + name='api-comment-list'),
> > + url(r'^comments/(?P<pk>[^/]+)/$',
> > + api_comment_views.CommentDetail.as_view(),
> > + name='api-comment-detail'),
>
> Comments sound like something that's very much a child thing. What
> about nesting these underneath patch/cover?
>
> /patches/{patchID}/comments
>
> Would solve a couple of the filtering problems above too; however, it
> would prevent you from listing every comment for a given project. I'm
> not sure if the latter issue is something you'd really be bothered
> about?
>
Sounds good. Right now, I can't imagine a use case for listing all comments
for a project so I'm fine with the change.
> > url(r'^covers/$',
> > api_cover_views.CoverLetterList.as_view(),
> > name='api-cover-list'),
> > @@ -277,6 +284,9 @@ if settings.ENABLE_REST_API:
> > url(r'^events/$',
> > api_event_views.EventList.as_view(),
> > name='api-event-list'),
> > + url(r'^comments/$',
> > + api_comment_views.CommentList.as_view(),
> > + name='api-comment-list'),
>
> Looks like you've registered this twice.
>
Whoops.
> > ]
> >
> > urlpatterns += [
> > diff --git a/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > new file mode 100644
> > index 0000000..72b3339
> > --- /dev/null
> > +++ b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> > @@ -0,0 +1,10 @@
> > +---
> > +api:
> > + - |
> > + Added /comments endpoint. For comment list, fields
>
> ``/comments`` (or ``/patches/{patchID}/comments``)
>
> > + 'id', 'url', 'msgid', 'date', 'subject', 'submitter', 'parent',
> > 'tags'
> > + are available. For comment detail (/comments/29), comment's
> > + 'content', 'headers', 'project'
> > + are added as well. Filtering is possible based on
> > + 'project', 'parent', 'submitter'
> > + fields.
>
> You also need to version this, per recent changes. Basically you want
> this entire API to 404 if using version 1.0 and drop 'comments' from
> the root response for same. See the recent changes to the 'api'
> directory for examples of the latter.
>
Will do. These changes weren't present when I was sending the patch so
they are naturally not reflected here. I'll also implement the tests
based on your patch that split the API tests, and remove the tagging
dependency since the RFC is probably in for a longer run.
Thanks again,
Veronika
> Stephen
>
More information about the Patchwork
mailing list