| Message ID | 20180323123338.17509-1-vkabatov@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add comments REST API | expand |
----- Original Message ----- > From: vkabatov@redhat.com > To: patchwork@lists.ozlabs.org > Cc: "Veronika Kabatova" <vkabatov@redhat.com> > Sent: Friday, March 23, 2018 1:33:38 PM > Subject: [PATCH] Add comments REST API > > From: Veronika Kabatova <vkabatov@redhat.com> > > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com> > --- > 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 Before someone mentions it, I'm aware the tests are missing. I wanted to get feedback about the functionality first so I don't need to rework them later (and with the recent Stephen's API tests rework, it was definitely a good idea to wait). Veronika
On Fri, 2018-03-23 at 13:33 +0100, vkabatov@redhat.com wrote: > From: Veronika Kabatova <vkabatov@redhat.com> > > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com> 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. 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'? > + 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? > 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. > ] > > 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. Stephen
----- Original Message ----- > From: "Stephen Finucane" <stephen@that.guru> > To: vkabatov@redhat.com, patchwork@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@redhat.com wrote: > > From: Veronika Kabatova <vkabatov@redhat.com> > > > > Signed-off-by: Veronika Kabatova <vkabatov@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 >
On Mon, 2018-04-09 at 09:29 -0400, Veronika Kabatova wrote: > ----- Original Message ----- > > From: "Stephen Finucane" <stephen@that.guru> > > To: vkabatov@redhat.com, patchwork@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@redhat.com wrote: > > > From: Veronika Kabatova <vkabatov@redhat.com> > > > > > > Signed-off-by: Veronika Kabatova <vkabatov@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? Indeed. I'd expect all replies to a given patch to be under that patch and so on. This is what we present in the web UI so it probably makes sense to do the same for the REST API. Unless there's a strong reason _not_ to, this is the way I'd go, yeah. > 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. Yup, I didn't do it because I wasn't sure how helpful it was. I'm a-ok adding it though so go for it. Stephen PS: You should probably put a 'comments' url in the 'patches' and 'covers' resources, similar to what we do for 'checks' of 'patches'.
Stephen Finucane <stephen@that.guru> writes: > On Mon, 2018-04-09 at 09:29 -0400, Veronika Kabatova wrote: >> ----- Original Message ----- >> > From: "Stephen Finucane" <stephen@that.guru> >> > To: vkabatov@redhat.com, patchwork@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@redhat.com wrote: >> > > From: Veronika Kabatova <vkabatov@redhat.com> >> > > >> > > Signed-off-by: Veronika Kabatova <vkabatov@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? > > Indeed. I'd expect all replies to a given patch to be under that patch > and so on. This is what we present in the web UI so it probably makes > sense to do the same for the REST API. Unless there's a strong reason > _not_ to, this is the way I'd go, yeah. > >> 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. > We do have 1 pending request re: the comments API: https://github.com/getpatchwork/patchwork/issues/143 I don't know how that would interact with either design, but I wanted to flag it as a user story to consider. Regards, Daniel > Yup, I didn't do it because I wasn't sure how helpful it was. I'm a-ok > adding it though so go for it. > > Stephen > > PS: You should probably put a 'comments' url in the 'patches' and > 'covers' resources, similar to what we do for 'checks' of 'patches'. > _______________________________________________ > Patchwork mailing list > Patchwork@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/patchwork
----- Original Message ----- > From: "Daniel Axtens" <dja@axtens.net> > To: "Stephen Finucane" <stephen@that.guru>, "Veronika Kabatova" <vkabatov@redhat.com> > Cc: patchwork@lists.ozlabs.org > Sent: Tuesday, April 10, 2018 4:56:59 PM > Subject: Re: [PATCH] Add comments REST API > > Stephen Finucane <stephen@that.guru> writes: > > > On Mon, 2018-04-09 at 09:29 -0400, Veronika Kabatova wrote: > >> ----- Original Message ----- > >> > From: "Stephen Finucane" <stephen@that.guru> > >> > To: vkabatov@redhat.com, patchwork@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@redhat.com wrote: > >> > > From: Veronika Kabatova <vkabatov@redhat.com> > >> > > > >> > > Signed-off-by: Veronika Kabatova <vkabatov@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? > > > > Indeed. I'd expect all replies to a given patch to be under that patch > > and so on. This is what we present in the web UI so it probably makes > > sense to do the same for the REST API. Unless there's a strong reason > > _not_ to, this is the way I'd go, yeah. > > > >> 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. > > > > We do have 1 pending request re: the comments API: > https://github.com/getpatchwork/patchwork/issues/143 > > I don't know how that would interact with either design, but I wanted to > flag it as a user story to consider. > Thanks for posting that! I checked the issue list briefly but the subject didn't seem relevant so I missed it. Taking this feature into consideration: (previous solution) keeping the API as /comments, the tools can filter through it and then send requests for patch updates (currently working on) putting the comments into patches / covers endpoints as a serialized list, tools can query the patch and check the comments there directly, then send the request to the same /patch/ID endpoint used previously It would be the best if those changes were triggered automatically with comment parsing but until we have a safe way of doing that and a proof of concept, having a tool run periodically and marking patches should work fine with both implementations. I don't think the hidden endpoint Stephen mentioned below will play well with this request or add anything useful - we have a hard time getting checks associated with given patch since the link from patch goes to the list of all checks which can't be filtered by patch ID (instead of listing checks associated with the patch directly). Whichever way we go with the comments API implementation, the hidden endpoint will be more confusing than useful, IMHO. I'll go ahead and implement the changes discussed previously if you guys don't have additional ideas. Thanks, Veronika > Regards, > Daniel > > > Yup, I didn't do it because I wasn't sure how helpful it was. I'm a-ok > > adding it though so go for it. > > > > Stephen > > > > PS: You should probably put a 'comments' url in the 'patches' and > > 'covers' resources, similar to what we do for 'checks' of 'patches'. > > _______________________________________________ > > Patchwork mailing list > > Patchwork@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/patchwork >
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()) + 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'), 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'), ] 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 + '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.