[PATCH v2 1/5] api: add addressed field and detail endpoint for patch comments
Daniel Axtens
dja at axtens.net
Wed Aug 11 09:20:51 AEST 2021
Raxel Gutierrez <raxel at 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 <raxel at 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
More information about the Patchwork
mailing list