Raxel Gutierrez <ra...@google.com> writes: > Add addressed boolean field to PatchComment to be able to distinguish > between them in the patch-detail page. Change PatchComment to have same > is_editable from the related patch with the addition of being editable > by the user who submitted the comment. > > Add endpoint for patch comments at api/.../comments/<comment_id>. > The endpoint will make it possible to use the REST API to update the new > `addressed` field for patch comments with JavaScript on the client side. > In the process of these changes, clean up use of CurrentPatchDefault so > that it exists in base.py and can be used throughout the API. > > Add the OpenAPI definition of the new endpoint and upgrade API version > to v1.3 to reflect the minor change. In order for tests to pass, clean > up test_comment.py to reflect the change from the <pk> to <patch_id> > parameter for the `api-patch-comment-list` endpoint. > > Signed-off-by: Raxel Gutierrez <ra...@google.com> > --- > docs/api/schemas/generate-schemas.py | 4 +- > docs/api/schemas/latest/patchwork.yaml | 144 +- > docs/api/schemas/patchwork.j2 | 148 +- > docs/api/schemas/v1.0/patchwork.yaml | 35 +- > docs/api/schemas/v1.1/patchwork.yaml | 35 +- > docs/api/schemas/v1.2/patchwork.yaml | 35 +- > docs/api/schemas/v1.3/patchwork.yaml | 2749 +++++++++++++++++ > patchwork/api/base.py | 24 +- > patchwork/api/check.py | 21 +- > patchwork/api/comment.py | 76 +- > patchwork/api/patch.py | 2 +- > .../migrations/0045_patchcomment_addressed.py | 18 + > patchwork/models.py | 5 +- > patchwork/tests/api/test_comment.py | 4 +- > patchwork/urls.py | 17 +- > 15 files changed, 3256 insertions(+), 61 deletions(-) > create mode 100644 docs/api/schemas/v1.3/patchwork.yaml > create mode 100644 patchwork/migrations/0045_patchcomment_addressed.py > > diff --git a/docs/api/schemas/generate-schemas.py > b/docs/api/schemas/generate-schemas.py > index a0c1e45..3a436a1 100755 > --- a/docs/api/schemas/generate-schemas.py > +++ b/docs/api/schemas/generate-schemas.py > @@ -14,8 +14,8 @@ except ImportError: > yaml = None > > ROOT_DIR = os.path.dirname(os.path.realpath(__file__)) > -VERSIONS = [(1, 0), (1, 1), (1, 2), None] > -LATEST_VERSION = (1, 2) > +VERSIONS = [(1, 0), (1, 1), (1, 2), (1, 3), None] > +LATEST_VERSION = (1, 3) > > > def generate_schemas():
[generated schema snipped] > diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2 > index af20743..3600139 100644 > --- a/docs/api/schemas/patchwork.j2 > +++ b/docs/api/schemas/patchwork.j2 > @@ -619,14 +619,14 @@ paths: > {% endif %} > tags: > - patches > - /api/{{ version_url }}patches/{id}/comments/: > + /api/{{ version_url }}patches/{patch_id}/comments/: > parameters: > - in: path > - name: id > + name: patch_id As Emily suggested, this is probably not something we can do in old schema versions. I tried running the OpenAPI generator (https://openapi-generator.tech/) over the new and old v1.2 schema, generating a Go client for the v1.2 API, and observed different method signatures and struct field names. I don't know if people use named parameters in Go so maybe things would keep working by accident but hopefully you get what I'm going for: if you had written some program that interacts with the API using code generated from the API definition file, things might break. There shouldn't generally be any meaningful changes to older API version descriptions at this stage. > description: A unique integer value identifying the parent patch. > required: true > schema: > - title: ID > + title: Patch ID > type: integer > get: > description: List comments > @@ -656,6 +656,112 @@ paths: > $ref: '#/components/schemas/Error' > tags: > - comments > + /api/{{ version_url }}patches/{patch_id}/comments/{comment_id}/: I think the {% if ... needs to start above this line - otherwise all the API versions see this endpoint, just without any methods. I think you've correctly hidden it from older versions with the api_1_3_patterns part in urls.py, so we don't want them to see it in the definitions file either. > + parameters: > + - in: path > + name: patch_id > + description: A unique integer value identifying the parent patch. > + required: true > + schema: > + title: Patch ID > + type: integer > + - in: path > + name: comment_id > + description: A unique integer value identifying this comment. > + required: true > + schema: > + title: Comment ID > + type: integer > +{% if version >= (1, 3) %} > + get: > + description: Show a patch comment. > + operationId: patch_comments_read > + responses: > + '200': > + description: '' > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/CommentDetail' > + '404': > + description: Not found > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/Error' > + tags: > + - comments > + patch: > + description: Update a patch comment (partial). > + operationId: patch_comments_partial_update > +# security: > +# - basicAuth: [] > +# - apiKeyAuth: [] I can see you've copy-pasted this from the other definitions, which is exactly what I said to do. It feels silly to keep it but also seems silly to break consistency. Idk. Unless Stephen weighs in feel free to just do whatever you feel most comfortable with. [See commit 2047107468a1 ("docs: Resolve issues with 'patches'")] > + requestBody: > + $ref: '#/components/requestBodies/Comment' > + responses: > + '200': > + description: '' > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/CommentDetail' > + '400': > + description: Invalid Request > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/ErrorCommentUpdate' > + '403': > + description: Forbidden > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/Error' > + '404': > + description: Not found > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/Error' > + tags: > + - comments > + put: > + description: Update a patch comment. > + operationId: patch_comments_update > +# security: > +# - basicAuth: [] > +# - apiKeyAuth: [] > + requestBody: > + $ref: '#/components/requestBodies/Comment' > + responses: > + '200': > + description: '' > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/CommentDetail' > + '400': > + description: Invalid Request > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/ErrorCommentUpdate' > + '403': > + description: Forbidden > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/Error' > + '404': > + description: Not found > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/Error' > + tags: > + - comments Hmm. Do we want a PUT method? We have a pretty eclectic collection of PUTable objects atm: User, Bundle, Patch and Project. If I understand the HTTP methods correctly, I don't think that we're supposed to allow the PUT method unless you can create the object or replace its contents via the request. So I think it makes sense to allow us to PUT a Bundle, but I think we should only allow Users, Patches, Projects and Comments to be PATCHed: they should all be created in other ways, and usually we don't allow their contents to be replaced. I think you only need a PATCH method here, unless Stephen has any other ideas. > +{% endif %} > /api/{{ version_url }}patches/{patch_id}/checks/: > parameters: > - in: path > @@ -1277,6 +1383,12 @@ components: > application/x-www-form-urlencoded: > schema: > $ref: '#/components/schemas/CheckCreate' > + Comment: > + required: true > + content: > + application/json: > + schema: > + $ref: '#/components/schemas/CommentUpdate' > Patch: > required: true > content: > @@ -1586,6 +1698,25 @@ components: > additionalProperties: > type: string > readOnly: true > + CommentDetail: > + allOf: > + - $ref: '#/components/schemas/Comment' > +{% if version >= (1, 3) %} > + - properties: > + patch: > + $ref: '#/components/schemas/PatchEmbedded' > + addressed: > + title: Addressed > + type: boolean > +{% endif %} > + CommentUpdate: > + type: object > +{% if version >= (1, 3) %} > + properties: > + addressed: > + title: Addressed > + type: boolean > +{% endif %} > CoverList: > type: object > properties: > @@ -2659,6 +2790,17 @@ components: > items: > type: string > readOnly: true > + ErrorCommentUpdate: > + type: object > +{% if version >= (1, 3) %} > + properties: > + addressed: > + title: Addressed > + type: array > + items: > + type: string > + readOnly: true > +{% endif %} I think we should move the if condition for these objects too. I think if the types are only used in v1.3 they should only be defined in v1.3 - I can't think of a reason we would need to have objects with no properties in older API versions? > ErrorPatchUpdate: > type: object > properties: > diff --git a/docs/api/schemas/v1.0/patchwork.yaml > b/docs/api/schemas/v1.0/patchwork.yaml > index 23e8930..3f90b3e 100644 [snip generated versions] > diff --git a/patchwork/api/base.py b/patchwork/api/base.py > index 89a4311..856fbd3 100644 > --- a/patchwork/api/base.py > +++ b/patchwork/api/base.py > @@ -3,6 +3,7 @@ > # > # SPDX-License-Identifier: GPL-2.0-or-later > > +import rest_framework > > from django.conf import settings > from django.shortcuts import get_object_or_404 > @@ -15,6 +16,24 @@ from rest_framework.serializers import > HyperlinkedModelSerializer > from patchwork.api import utils > > > +DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.')) > + > + > +if DRF_VERSION > (3, 11): > + class CurrentPatchDefault(object): > + requires_context = True > + > + def __call__(self, serializer_field): > + return serializer_field.context['request'].patch > +else: > + class CurrentPatchDefault(object): > + def set_context(self, serializer_field): > + self.patch = serializer_field.context['request'].patch > + > + def __call__(self): > + return self.patch Hmm. I was going to comment on this but then I realised you just moved this code. I think we might be able to drop the DRF version condition entirely but you don't need to be the person to do that (and this series isn't necessarily the right place). No action required here. > + > + > class LinkHeaderPagination(PageNumberPagination): > """Provide pagination based on rfc5988. > > @@ -44,7 +63,10 @@ class LinkHeaderPagination(PageNumberPagination): > > > class PatchworkPermission(permissions.BasePermission): > - """This permission works for Project and Patch model objects""" > + """ > + This permission works for Project, Patch, and PatchComment > + model objects > + """ > def has_object_permission(self, request, view, obj): > # read only for everyone > if request.method in permissions.SAFE_METHODS: > diff --git a/patchwork/api/check.py b/patchwork/api/check.py > index a6bf5f8..fde6b61 100644 > --- a/patchwork/api/check.py > +++ b/patchwork/api/check.py > @@ -6,7 +6,7 @@ > from django.http import Http404 > from django.http.request import QueryDict > from django.shortcuts import get_object_or_404 > -import rest_framework > + > from rest_framework.exceptions import PermissionDenied > from rest_framework.generics import ListCreateAPIView > from rest_framework.generics import RetrieveAPIView > @@ -17,30 +17,13 @@ from rest_framework.serializers import ValidationError > > from patchwork.api.base import CheckHyperlinkedIdentityField > from patchwork.api.base import MultipleFieldLookupMixin > +from patchwork.api.base import CurrentPatchDefault > from patchwork.api.embedded import UserSerializer > from patchwork.api.filters import CheckFilterSet > from patchwork.models import Check > from patchwork.models import Patch > > > -DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.')) > - > - > -if DRF_VERSION > (3, 11): > - class CurrentPatchDefault(object): > - requires_context = True > - > - def __call__(self, serializer_field): > - return serializer_field.context['request'].patch > -else: > - class CurrentPatchDefault(object): > - def set_context(self, serializer_field): > - self.patch = serializer_field.context['request'].patch > - > - def __call__(self): > - return self.patch > - > - > class CheckSerializer(HyperlinkedModelSerializer): > > url = CheckHyperlinkedIdentityField('api-check-detail') > diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py > index 43b26c6..50d70d1 100644 > --- a/patchwork/api/comment.py > +++ b/patchwork/api/comment.py > @@ -5,12 +5,17 @@ > > import email.parser > > +from django.shortcuts import get_object_or_404 > from django.http import Http404 > from rest_framework.generics import ListAPIView > +from rest_framework.generics import RetrieveUpdateAPIView > +from rest_framework.serializers import HiddenField > from rest_framework.serializers import SerializerMethodField > > from patchwork.api.base import BaseHyperlinkedModelSerializer > +from patchwork.api.base import MultipleFieldLookupMixin > from patchwork.api.base import PatchworkPermission > +from patchwork.api.base import CurrentPatchDefault > from patchwork.api.embedded import PersonSerializer > from patchwork.models import Cover > from patchwork.models import CoverComment > @@ -66,15 +71,50 @@ class > CoverCommentListSerializer(BaseCommentListSerializer): > versioned_fields = BaseCommentListSerializer.Meta.versioned_fields > > > -class PatchCommentListSerializer(BaseCommentListSerializer): > +class PatchCommentSerializer(BaseCommentListSerializer): > + > + patch = HiddenField(default=CurrentPatchDefault()) > > class Meta: > model = PatchComment > - fields = BaseCommentListSerializer.Meta.fields > - read_only_fields = fields > + fields = BaseCommentListSerializer.Meta.fields + ( > + 'patch', 'addressed') > + read_only_fields = fields[:-1] # able to write to addressed field Please make this explicit, that is BaseCommentListSerializer.Meta.fields + ('patch', ) > + versioned_fields = { > + '1.3': ('patch', 'addressed'), > + } > + extra_kwargs = { > + 'url': {'view_name': 'api-patch-comment-detail'} > + } > versioned_fields = BaseCommentListSerializer.Meta.versioned_fields > > > +class PatchCommentMixin(object): > + > + permission_classes = (PatchworkPermission,) > + serializer_class = PatchCommentSerializer > + > + def get_object(self): > + queryset = self.filter_queryset(self.get_queryset()) > + comment_id = self.kwargs['comment_id'] > + try: > + obj = queryset.get(id=int(comment_id)) > + except (ValueError, PatchComment.DoesNotExist): > + obj = get_object_or_404(queryset, linkname=comment_id) > + self.kwargs['comment_id'] = obj.id > + self.check_object_permissions(self.request, obj) > + return obj > + > + def get_queryset(self): > + patch_id = self.kwargs['patch_id'] > + if not Patch.objects.filter(id=patch_id).exists(): > + raise Http404 > + > + return PatchComment.objects.filter( > + patch=patch_id > + ).select_related('submitter') > + > + > class CoverCommentList(ListAPIView): > """List cover comments""" > > @@ -94,20 +134,24 @@ class CoverCommentList(ListAPIView): > ).select_related('submitter') > > > -class PatchCommentList(ListAPIView): > - """List comments""" > +class PatchCommentList(PatchCommentMixin, ListAPIView): > + """List patch comments""" > > - permission_classes = (PatchworkPermission,) > - serializer_class = PatchCommentListSerializer > search_fields = ('subject',) > ordering_fields = ('id', 'subject', 'date', 'submitter') > ordering = 'id' > - lookup_url_kwarg = 'pk' > - > - def get_queryset(self): > - if not Patch.objects.filter(pk=self.kwargs['pk']).exists(): > - raise Http404 > - > - return PatchComment.objects.filter( > - patch=self.kwargs['pk'] > - ).select_related('submitter') > + lookup_url_kwarg = 'patch_id' > + > + > +class PatchCommentDetail(PatchCommentMixin, MultipleFieldLookupMixin, > + RetrieveUpdateAPIView): > + """ > + get: > + Show a patch comment. > + patch: > + Update a patch comment. > + put: > + Update a patch comment. > + """ > + lookup_url_kwargs = ('patch_id', 'comment_id') > + lookup_fields = ('patch_id', 'id') > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > index 9d22275..a97a882 100644 > --- a/patchwork/api/patch.py > +++ b/patchwork/api/patch.py > @@ -97,7 +97,7 @@ class PatchListSerializer(BaseHyperlinkedModelSerializer): > > def get_comments(self, patch): > return self.context.get('request').build_absolute_uri( > - reverse('api-patch-comment-list', kwargs={'pk': patch.id})) > + reverse('api-patch-comment-list', kwargs={'patch_id': patch.id})) > > def get_check(self, instance): > return instance.combined_check_state > diff --git a/patchwork/migrations/0045_patchcomment_addressed.py > b/patchwork/migrations/0045_patchcomment_addressed.py > new file mode 100644 > index 0000000..92e6c4e > --- /dev/null > +++ b/patchwork/migrations/0045_patchcomment_addressed.py > @@ -0,0 +1,18 @@ > +# Generated by Django 3.1.12 on 2021-07-16 04:12 > + > +from django.db import migrations, models > + > + > +class Migration(migrations.Migration): > + > + dependencies = [ > + ('patchwork', '0044_add_project_linkname_validation'), > + ] > + > + operations = [ > + migrations.AddField( > + model_name='patchcomment', > + name='addressed', > + field=models.BooleanField(default=False), > + ), > + ] > diff --git a/patchwork/models.py b/patchwork/models.py > index 00273da..ef52f2c 100644 > --- a/patchwork/models.py > +++ b/patchwork/models.py > @@ -693,6 +693,7 @@ class PatchComment(EmailMixin, models.Model): > related_query_name='comment', > on_delete=models.CASCADE, > ) > + addressed = models.BooleanField(default=False) > > @property > def list_archive_url(self): > @@ -718,7 +719,9 @@ class PatchComment(EmailMixin, models.Model): > self.patch.refresh_tag_counts() > > def is_editable(self, user): > - return False > + if user == self.submitter.user: > + return True > + return self.patch.is_editable(user) > > class Meta: > ordering = ['date'] > diff --git a/patchwork/tests/api/test_comment.py > b/patchwork/tests/api/test_comment.py > index 5bbebf2..59450d8 100644 > --- a/patchwork/tests/api/test_comment.py > +++ b/patchwork/tests/api/test_comment.py > @@ -90,7 +90,7 @@ class TestPatchComments(utils.APITestCase): > kwargs = {} > if version: > kwargs['version'] = version > - kwargs['pk'] = patch.id > + kwargs['patch_id'] = patch.id I think we might be able to get away with renaming the parameter internally even if we can't rename it in old versions of the API, but please split the 'pk'->'patch_id' change into a different patch. Kind regards, Daniel _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork