Re: [PATCH v2 1/2] api: Add comments to patch and cover endpoints
- Original Message -
> From: "Stephen Finucane"
> To: vkaba...@redhat.com, patchwork@lists.ozlabs.org
> Sent: Monday, April 16, 2018 1:15:24 AM
> Subject: Re: [PATCH v2 1/2] api: Add comments to patch and cover endpoints
>
> On Wed, 2018-04-11 at 19:45 +0200, vkaba...@redhat.com wrote:
> > From: Veronika Kabatova
> >
> > Signed-off-by: Veronika Kabatova
>
> I thought I'd replied to this already but I clearly didn't. This all
> looks pretty good but I think I might have confused you. It seems with
> this version our patch/cover letter responses gain an entire nested
> version of the comments. This seems overly detailed, given that it's
> not something most people need. What I meant instead is to add a
> comments URL like so:
>
Yes, that's why I double questioned your idea :)
> /patch/{patchID}/comments
>
> ...and include this in the patch response:
>
> {
> "id": 123,
> ...
> "comments": "https://www.example.com/api/patches/123/comments";
> }
>
> Any reason not to go for this option?
>
I guess not, will move the comments there.
> Also, if you do end up respinning this just squash tests/migrations in
> with this patch. I'd be applying them together anyway.
>
Will do.
Thanks,
Veronika
> Stephen
>
> > ---
> > patchwork/api/cover.py | 14 ++--
> > patchwork/api/embedded.py | 38
> > ++
> > patchwork/api/patch.py | 15 ++---
> > patchwork/models.py| 3 ++
> > .../notes/comments-api-b7dff6ee4ce04c9b.yaml | 8 +
> > 5 files changed, 70 insertions(+), 8 deletions(-)
> > create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
> >
> > diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> > index fc7ae97..5a6e377 100644
> > --- a/patchwork/api/cover.py
> > +++ b/patchwork/api/cover.py
> > @@ -25,6 +25,7 @@ from rest_framework.serializers import
> > SerializerMethodField
> >
> > from patchwork.api.base import BaseHyperlinkedModelSerializer
> > from patchwork.api.filters import CoverLetterFilter
> > +from patchwork.api.embedded import CommentSerializer
> > from patchwork.api.embedded import PersonSerializer
> > from patchwork.api.embedded import ProjectSerializer
> > from patchwork.api.embedded import SeriesSerializer
> > @@ -58,5 +59,6 @@ class
> > CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> > class CoverLetterDetailSerializer(CoverLetterListSerializer):
> >
> > headers = SerializerMethodField()
> > +comments = CommentSerializer(many=True, read_only=True)
> >
> > def get_headers(self, instance):
> > @@ -65,9 +67,14 @@ class
> > CoverLetterDetailSerializer(CoverLetterListSerializer):
> >
> > class Meta:
> > model = CoverLetter
> > -fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > 'content')
> > +fields = CoverLetterListSerializer.Meta.fields + ('headers',
> > + 'content',
> > + 'comments')
> > read_only_fields = fields
> > extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs
> > +versioned_fields = {
> > +'1.1': ('mbox', 'comments'),
> > +}
> >
> >
> > class CoverLetterList(ListAPIView):
> > @@ -91,5 +98,6 @@ class CoverLetterDetail(RetrieveAPIView):
> > serializer_class = CoverLetterDetailSerializer
> >
> > def get_queryset(self):
> > -return CoverLetter.objects.all().prefetch_related('series')\
> > -.select_related('project', 'submitter')
> > +return CoverLetter.objects.all().prefetch_related(
> > +'series', 'comments'
> > +).select_related('project', 'submitter')
> > diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
> > index d79724c..afd511a 100644
> > --- a/patchwork/api/embedded.py
> > +++ b/patchwork/api/embedded.py
> > @@ -23,6 +23,8 @@ A collection of serializers. None of the serializers here
> > should reference
> > nested fields.
> > """
> >
> > +import email.parser
> > +
> > from rest_framewor
Re: [PATCH v2 1/2] api: Add comments to patch and cover endpoints
On Wed, 2018-04-11 at 19:45 +0200, vkaba...@redhat.com wrote:
> From: Veronika Kabatova
>
> Signed-off-by: Veronika Kabatova
I thought I'd replied to this already but I clearly didn't. This all
looks pretty good but I think I might have confused you. It seems with
this version our patch/cover letter responses gain an entire nested
version of the comments. This seems overly detailed, given that it's
not something most people need. What I meant instead is to add a
comments URL like so:
/patch/{patchID}/comments
...and include this in the patch response:
{
"id": 123,
...
"comments": "https://www.example.com/api/patches/123/comments";
}
Any reason not to go for this option?
Also, if you do end up respinning this just squash tests/migrations in
with this patch. I'd be applying them together anyway.
Stephen
> ---
> patchwork/api/cover.py | 14 ++--
> patchwork/api/embedded.py | 38
> ++
> patchwork/api/patch.py | 15 ++---
> patchwork/models.py| 3 ++
> .../notes/comments-api-b7dff6ee4ce04c9b.yaml | 8 +
> 5 files changed, 70 insertions(+), 8 deletions(-)
> create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
>
> diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
> index fc7ae97..5a6e377 100644
> --- a/patchwork/api/cover.py
> +++ b/patchwork/api/cover.py
> @@ -25,6 +25,7 @@ from rest_framework.serializers import SerializerMethodField
>
> from patchwork.api.base import BaseHyperlinkedModelSerializer
> from patchwork.api.filters import CoverLetterFilter
> +from patchwork.api.embedded import CommentSerializer
> from patchwork.api.embedded import PersonSerializer
> from patchwork.api.embedded import ProjectSerializer
> from patchwork.api.embedded import SeriesSerializer
> @@ -58,5 +59,6 @@ class
> CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
> class CoverLetterDetailSerializer(CoverLetterListSerializer):
>
> headers = SerializerMethodField()
> +comments = CommentSerializer(many=True, read_only=True)
>
> def get_headers(self, instance):
> @@ -65,9 +67,14 @@ class
> CoverLetterDetailSerializer(CoverLetterListSerializer):
>
> class Meta:
> model = CoverLetter
> -fields = CoverLetterListSerializer.Meta.fields + ('headers',
> 'content')
> +fields = CoverLetterListSerializer.Meta.fields + ('headers',
> + 'content',
> + 'comments')
> read_only_fields = fields
> extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs
> +versioned_fields = {
> +'1.1': ('mbox', 'comments'),
> +}
>
>
> class CoverLetterList(ListAPIView):
> @@ -91,5 +98,6 @@ class CoverLetterDetail(RetrieveAPIView):
> serializer_class = CoverLetterDetailSerializer
>
> def get_queryset(self):
> -return CoverLetter.objects.all().prefetch_related('series')\
> -.select_related('project', 'submitter')
> +return CoverLetter.objects.all().prefetch_related(
> +'series', 'comments'
> +).select_related('project', 'submitter')
> diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
> index d79724c..afd511a 100644
> --- a/patchwork/api/embedded.py
> +++ b/patchwork/api/embedded.py
> @@ -23,6 +23,8 @@ A collection of serializers. None of the serializers here
> should reference
> nested fields.
> """
>
> +import email.parser
> +
> from rest_framework.serializers import CharField
> from rest_framework.serializers import SerializerMethodField
>
> @@ -163,3 +165,39 @@ class
> UserProfileSerializer(BaseHyperlinkedModelSerializer):
> extra_kwargs = {
> 'url': {'view_name': 'api-user-detail'},
> }
> +
> +
> +class CommentSerializer(BaseHyperlinkedModelSerializer):
> +
> +tags = SerializerMethodField()
> +subject = SerializerMethodField()
> +headers = SerializerMethodField()
> +submitter = PersonSerializer(read_only=True)
> +
> +def get_tags(self, comment):
> +# TODO implement after we get support for tags on comments
> +return {}
> +
> +def get_subject(self, comment):
> +return email.parser.Parser().parsestr(comment.headers,
> + True).get('Subject', '')
> +
> +def get_headers(self, comment):
> +headers = {}
> +
> +if comment.headers:
> +parsed = email.parser.Parser().parsestr(comment.headers, True)
> +for key in parsed.keys():
> +headers[key] = parsed.get_all(key)
> +# Let's return a single string instead of a list if only one
> +# header with this key is present
> +if len(headers[key]) == 1:
> +heade
[PATCH v2 1/2] api: Add comments to patch and cover endpoints
From: Veronika Kabatova
Signed-off-by: Veronika Kabatova
---
patchwork/api/cover.py | 14 ++--
patchwork/api/embedded.py | 38 ++
patchwork/api/patch.py | 15 ++---
patchwork/models.py| 3 ++
.../notes/comments-api-b7dff6ee4ce04c9b.yaml | 8 +
5 files changed, 70 insertions(+), 8 deletions(-)
create mode 100644 releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml
diff --git a/patchwork/api/cover.py b/patchwork/api/cover.py
index fc7ae97..5a6e377 100644
--- a/patchwork/api/cover.py
+++ b/patchwork/api/cover.py
@@ -25,6 +25,7 @@ from rest_framework.serializers import SerializerMethodField
from patchwork.api.base import BaseHyperlinkedModelSerializer
from patchwork.api.filters import CoverLetterFilter
+from patchwork.api.embedded import CommentSerializer
from patchwork.api.embedded import PersonSerializer
from patchwork.api.embedded import ProjectSerializer
from patchwork.api.embedded import SeriesSerializer
@@ -58,5 +59,6 @@ class
CoverLetterListSerializer(BaseHyperlinkedModelSerializer):
class CoverLetterDetailSerializer(CoverLetterListSerializer):
headers = SerializerMethodField()
+comments = CommentSerializer(many=True, read_only=True)
def get_headers(self, instance):
@@ -65,9 +67,14 @@ class CoverLetterDetailSerializer(CoverLetterListSerializer):
class Meta:
model = CoverLetter
-fields = CoverLetterListSerializer.Meta.fields + ('headers', 'content')
+fields = CoverLetterListSerializer.Meta.fields + ('headers',
+ 'content',
+ 'comments')
read_only_fields = fields
extra_kwargs = CoverLetterListSerializer.Meta.extra_kwargs
+versioned_fields = {
+'1.1': ('mbox', 'comments'),
+}
class CoverLetterList(ListAPIView):
@@ -91,5 +98,6 @@ class CoverLetterDetail(RetrieveAPIView):
serializer_class = CoverLetterDetailSerializer
def get_queryset(self):
-return CoverLetter.objects.all().prefetch_related('series')\
-.select_related('project', 'submitter')
+return CoverLetter.objects.all().prefetch_related(
+'series', 'comments'
+).select_related('project', 'submitter')
diff --git a/patchwork/api/embedded.py b/patchwork/api/embedded.py
index d79724c..afd511a 100644
--- a/patchwork/api/embedded.py
+++ b/patchwork/api/embedded.py
@@ -23,6 +23,8 @@ A collection of serializers. None of the serializers here
should reference
nested fields.
"""
+import email.parser
+
from rest_framework.serializers import CharField
from rest_framework.serializers import SerializerMethodField
@@ -163,3 +165,39 @@ class
UserProfileSerializer(BaseHyperlinkedModelSerializer):
extra_kwargs = {
'url': {'view_name': 'api-user-detail'},
}
+
+
+class CommentSerializer(BaseHyperlinkedModelSerializer):
+
+tags = SerializerMethodField()
+subject = SerializerMethodField()
+headers = SerializerMethodField()
+submitter = PersonSerializer(read_only=True)
+
+def get_tags(self, comment):
+# TODO implement after we get support for tags on comments
+return {}
+
+def get_subject(self, comment):
+return email.parser.Parser().parsestr(comment.headers,
+ True).get('Subject', '')
+
+def get_headers(self, comment):
+headers = {}
+
+if comment.headers:
+parsed = email.parser.Parser().parsestr(comment.headers, True)
+for key in parsed.keys():
+headers[key] = parsed.get_all(key)
+# Let's return a single string instead of a list if only one
+# header with this key is present
+if len(headers[key]) == 1:
+headers[key] = headers[key][0]
+
+return headers
+
+class Meta:
+model = models.Comment
+fields = ('id', 'msgid', 'date', 'subject', 'submitter', 'tags',
+ 'content', 'headers')
+read_only_fields = fields
diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
index 115feff..5affa87 100644
--- a/patchwork/api/patch.py
+++ b/patchwork/api/patch.py
@@ -24,11 +24,12 @@ from rest_framework.generics import ListAPIView
from rest_framework.generics import RetrieveUpdateAPIView
from rest_framework.relations import RelatedField
from rest_framework.reverse import reverse
-from rest_framework.serializers import HyperlinkedModelSerializer
from rest_framework.serializers import SerializerMethodField
+from patchwork.api.base import BaseHyperlinkedModelSerializer
from patchwork.api.base import PatchworkPermission
from patchwork.api.filters import PatchFilter
+from patchwork.api.embedded import CommentSerializer
from patchwork.api.em
