Add comments REST API

Message ID 20180323123338.17509-1-vkabatov@redhat.com
State New
Headers show
Series
  • Add comments REST API
Related show

Commit Message

Veronika Kabatova March 23, 2018, 12:33 p.m.
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

Comments

Veronika Kabatova March 26, 2018, 12:38 p.m. | #1
----- 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
Stephen Finucane April 9, 2018, 1:02 p.m. | #2
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
Veronika Kabatova April 9, 2018, 1:29 p.m. | #3
----- 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
>
Stephen Finucane April 9, 2018, 2:06 p.m. | #4
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'.
Daniel Axtens April 10, 2018, 2:56 p.m. | #5
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
Veronika Kabatova April 10, 2018, 3:59 p.m. | #6
----- 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
>

Patch

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.