Message ID | 20180411174513.25938-1-vkabatov@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series | [v2,1/2] api: Add comments to patch and cover endpoints | expand |
On Wed, 2018-04-11 at 19:45 +0200, vkabatov@redhat.com wrote: > From: Veronika Kabatova <vkabatov@redhat.com> > > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com> 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: > + 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.embedded import PersonSerializer > from patchwork.api.embedded import ProjectSerializer > from patchwork.api.embedded import SeriesSerializer > @@ -75,7 +76,7 @@ class StateField(RelatedField): > return State.objects.all() > > > -class PatchListSerializer(HyperlinkedModelSerializer): > +class PatchListSerializer(BaseHyperlinkedModelSerializer): > > project = ProjectSerializer(read_only=True) > state = StateField() > @@ -121,5 +122,6 @@ class PatchDetailSerializer(PatchListSerializer): > > headers = SerializerMethodField() > prefixes = SerializerMethodField() > + comments = CommentSerializer(many=True, read_only=True) > > def get_headers(self, patch): > @@ -132,10 +134,13 @@ class PatchDetailSerializer(PatchListSerializer): > class Meta: > model = Patch > fields = PatchListSerializer.Meta.fields + ( > - 'headers', 'content', 'diff', 'prefixes') > + 'headers', 'content', 'diff', 'prefixes', 'comments') > read_only_fields = PatchListSerializer.Meta.read_only_fields + ( > - 'headers', 'content', 'diff', 'prefixes') > + 'headers', 'content', 'diff', 'prefixes', 'comments') > extra_kwargs = PatchListSerializer.Meta.extra_kwargs > + versioned_fields = { > + '1.1': ('comments', ), > + } > > > class PatchList(ListAPIView): > @@ -164,5 +169,5 @@ class PatchDetail(RetrieveUpdateAPIView): > > def get_queryset(self): > return Patch.objects.all()\ > - .prefetch_related('series', 'check_set')\ > + .prefetch_related('series', 'check_set', 'comments')\ > .select_related('project', 'state', 'submitter', 'delegate') > 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/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml > new file mode 100644 > index 0000000..d696aab > --- /dev/null > +++ b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml > @@ -0,0 +1,8 @@ > +--- > +api: > + - | > + Comments are now exposed when checking patch and cover letter details > + (``/patches/{patchID}`` and ``/covers/{coverID}`` endpoints). Fields `id`, > + `msgid`, `date`, `subject`, `submitter`, `tags`, `content` and `headers` > + are available for each comment associated with the parent submission. > + Please note that comments are available only since API version 1.1
----- Original Message ----- > From: "Stephen Finucane" <stephen@that.guru> > To: vkabatov@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, vkabatov@redhat.com wrote: > > From: Veronika Kabatova <vkabatov@redhat.com> > > > > Signed-off-by: Veronika Kabatova <vkabatov@redhat.com> > > 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_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.embedded import PersonSerializer > > from patchwork.api.embedded import ProjectSerializer > > from patchwork.api.embedded import SeriesSerializer > > @@ -75,7 +76,7 @@ class StateField(RelatedField): > > return State.objects.all() > > > > > > -class PatchListSerializer(HyperlinkedModelSerializer): > > +class PatchListSerializer(BaseHyperlinkedModelSerializer): > > > > project = ProjectSerializer(read_only=True) > > state = StateField() > > @@ -121,5 +122,6 @@ class PatchDetailSerializer(PatchListSerializer): > > > > headers = SerializerMethodField() > > prefixes = SerializerMethodField() > > + comments = CommentSerializer(many=True, read_only=True) > > > > def get_headers(self, patch): > > @@ -132,10 +134,13 @@ class PatchDetailSerializer(PatchListSerializer): > > class Meta: > > model = Patch > > fields = PatchListSerializer.Meta.fields + ( > > - 'headers', 'content', 'diff', 'prefixes') > > + 'headers', 'content', 'diff', 'prefixes', 'comments') > > read_only_fields = PatchListSerializer.Meta.read_only_fields + ( > > - 'headers', 'content', 'diff', 'prefixes') > > + 'headers', 'content', 'diff', 'prefixes', 'comments') > > extra_kwargs = PatchListSerializer.Meta.extra_kwargs > > + versioned_fields = { > > + '1.1': ('comments', ), > > + } > > > > > > class PatchList(ListAPIView): > > @@ -164,5 +169,5 @@ class PatchDetail(RetrieveUpdateAPIView): > > > > def get_queryset(self): > > return Patch.objects.all()\ > > - .prefetch_related('series', 'check_set')\ > > + .prefetch_related('series', 'check_set', 'comments')\ > > .select_related('project', 'state', 'submitter', 'delegate') > > 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/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml > > b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml > > new file mode 100644 > > index 0000000..d696aab > > --- /dev/null > > +++ b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml > > @@ -0,0 +1,8 @@ > > +--- > > +api: > > + - | > > + Comments are now exposed when checking patch and cover letter details > > + (``/patches/{patchID}`` and ``/covers/{coverID}`` endpoints). Fields > > `id`, > > + `msgid`, `date`, `subject`, `submitter`, `tags`, `content` and > > `headers` > > + are available for each comment associated with the parent submission. > > + Please note that comments are available only since API version 1.1 > >
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.embedded import PersonSerializer from patchwork.api.embedded import ProjectSerializer from patchwork.api.embedded import SeriesSerializer @@ -75,7 +76,7 @@ class StateField(RelatedField): return State.objects.all() -class PatchListSerializer(HyperlinkedModelSerializer): +class PatchListSerializer(BaseHyperlinkedModelSerializer): project = ProjectSerializer(read_only=True) state = StateField() @@ -121,5 +122,6 @@ class PatchDetailSerializer(PatchListSerializer): headers = SerializerMethodField() prefixes = SerializerMethodField() + comments = CommentSerializer(many=True, read_only=True) def get_headers(self, patch): @@ -132,10 +134,13 @@ class PatchDetailSerializer(PatchListSerializer): class Meta: model = Patch fields = PatchListSerializer.Meta.fields + ( - 'headers', 'content', 'diff', 'prefixes') + 'headers', 'content', 'diff', 'prefixes', 'comments') read_only_fields = PatchListSerializer.Meta.read_only_fields + ( - 'headers', 'content', 'diff', 'prefixes') + 'headers', 'content', 'diff', 'prefixes', 'comments') extra_kwargs = PatchListSerializer.Meta.extra_kwargs + versioned_fields = { + '1.1': ('comments', ), + } class PatchList(ListAPIView): @@ -164,5 +169,5 @@ class PatchDetail(RetrieveUpdateAPIView): def get_queryset(self): return Patch.objects.all()\ - .prefetch_related('series', 'check_set')\ + .prefetch_related('series', 'check_set', 'comments')\ .select_related('project', 'state', 'submitter', 'delegate') 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/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml new file mode 100644 index 0000000..d696aab --- /dev/null +++ b/releasenotes/notes/comments-api-b7dff6ee4ce04c9b.yaml @@ -0,0 +1,8 @@ +--- +api: + - | + Comments are now exposed when checking patch and cover letter details + (``/patches/{patchID}`` and ``/covers/{coverID}`` endpoints). Fields `id`, + `msgid`, `date`, `subject`, `submitter`, `tags`, `content` and `headers` + are available for each comment associated with the parent submission. + Please note that comments are available only since API version 1.1