[PATCH v2 1/5] api: add addressed field and detail endpoint for patch comments
Emily Shaffer
emilyshaffer at google.com
Tue Aug 3 11:13:43 AEST 2021
On Wed, Jul 28, 2021 at 06:17:14PM +0000, Raxel Gutierrez wrote:
>
> 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.
This is a pretty good example of what jrnieder and I mean when we're
talking about using the commit message to describe things about the
patch you cannot learn by examining the diff. In each paragraph of your
commit-msg, you've told us something about the diff - but haven't really
explained why you made that change. This commit message also lacks an
explanation of motivation. Why are you adding this field? What is the
ultimate goal which justifies incrementing the API version? This commit
message doesn't say anywhere that this is setup for tracking state on
comments.
Your cover letter *does* do a good job of that - but your cover letter
won't end up in 'git log'.
> 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)
Ok, we are incrementing the API version here. It's necessary because
we're adding a new REST endpoint. Sure.
> diff --git a/docs/api/schemas/latest/patchwork.yaml b/docs/api/schemas/latest/patchwork.yaml
> index a8910a7..95329c7 100644
> --- a/docs/api/schemas/latest/patchwork.yaml
> +++ b/docs/api/schemas/latest/patchwork.yaml
> @@ -13,7 +13,7 @@ info:
> license:
> name: GPL v2 License
> url: https://www.gnu.org/licenses/gpl-2.0.html
> - version: '1.2'
> + version: '1.3'
...and here as well. Ok.
> paths:
> /api/:
> get:
> @@ -598,14 +598,14 @@ paths:
> $ref: '#/components/schemas/Error'
> tags:
> - patches
> - /api/patches/{id}/comments/:
> + /api/patches/{patch_id}/comments/:
> parameters:
> - in: path
> - name: id
> + name: patch_id
> description: A unique integer value identifying the parent patch.
> required: true
> schema:
> - title: ID
> + title: Patch ID
This bit isn't mentioned in the commit message that I see. But I think
you are changing the field name to disambiguate it from the other
'comment_id'. I actually don't know - does this rename make any
functional difference for API consumers, or are we just doing it to make
our own codebase less confusing?
> type: integer
> get:
> description: List comments
> @@ -635,6 +635,110 @@ paths:
> $ref: '#/components/schemas/Error'
> tags:
> - comments
> + /api/patches/{patch_id}/comments/{comment_id}/:
> + 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
> + 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: []
> + 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
I've not got much to say on this bit but I'll acknowledge that I looked
at it and said "yep, that's YAML". Hopefully someone else has better
comments than me. ;)
> /api/patches/{patch_id}/checks/:
> parameters:
> - in: path
> @@ -1242,6 +1346,12 @@ components:
> application/x-www-form-urlencoded:
> schema:
> $ref: '#/components/schemas/CheckCreate'
> + Comment:
> + required: true
> + content:
> + application/json:
> + schema:
> + $ref: '#/components/schemas/CommentUpdate'
Is this requiring that there must be some comments present in order for
/api/patches/<patch id>/comments/ to work at all?
> Patch:
> required: true
> content:
> @@ -1528,6 +1638,21 @@ components:
> additionalProperties:
> type: string
> readOnly: true
> + CommentDetail:
> + allOf:
> + - $ref: '#/components/schemas/Comment'
> + - properties:
> + patch:
> + $ref: '#/components/schemas/PatchEmbedded'
> + addressed:
> + title: Addressed
> + type: boolean
> + CommentUpdate:
> + type: object
> + properties:
> + addressed:
> + title: Addressed
> + type: boolean
> CoverList:
> type: object
> properties:
> @@ -1712,9 +1837,11 @@ components:
> previous_relation:
> title: Previous relation
> type: string
> + nullable: true
> current_relation:
> title: Current relation
> type: string
> + nullable: true
I'm curious why this is being added (and what it's being added to)?
> diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
(I've not got anything useful to add here - this seems like basically
the same change as the file I commented on above.)
> diff --git a/docs/api/schemas/v1.0/patchwork.yaml b/docs/api/schemas/v1.0/patchwork.yaml
> diff --git a/docs/api/schemas/v1.1/patchwork.yaml b/docs/api/schemas/v1.1/patchwork.yaml
> diff --git a/docs/api/schemas/v1.2/patchwork.yaml b/docs/api/schemas/v1.2/patchwork.yaml
All these seem to be a) changing 'id' to 'patch_id' and b) adding the
extra comment stuff. But is it appropriate to do so for older API
versions?
> diff --git a/docs/api/schemas/v1.3/patchwork.yaml b/docs/api/schemas/v1.3/patchwork.yaml
> new file mode 100644
> index 0000000..2b100b4
> --- /dev/null
> +++ b/docs/api/schemas/v1.3/patchwork.yaml
> @@ -0,0 +1,2749 @@
> +# DO NOT EDIT THIS FILE. It is generated from a template. Changes should be
> +# proposed against the template and updated files generated using the
> +# 'generate-schemas.py' tool
...Or, I guess they were updated by the tool. Ok.
> diff --git a/patchwork/api/base.py b/patchwork/api/base.py
[snip]
I guess I see why you wanted to separate the testing from these schema
updates, then. Never mind my comment on the cover letter. :)
> --- 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
> +
> +
I'm a little confused why this seems to be moved from check.py to
here... but I think that's because I don't know the codebase well ;)
> 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
> + versioned_fields = {
> + '1.3': ('patch', 'addressed'),
> + }
> + extra_kwargs = {
> + 'url': {'view_name': 'api-patch-comment-detail'}
> + }
> versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
Cool, adding the new bit to the comment serializer. Seems fine.
> 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),
> + ),
> + ]
Oh cool, so this is what migrates existing data for a running instance?
> 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)
I guess this is what I was asking for in the cover - changeable if it's
my patch or if I have patch edit permissions. I wonder if it's possible
to make it changeable for the author of the comment too? Hrm.
> 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
>
> return reverse('api-patch-comment-list', kwargs=kwargs)
>
> @@ -142,5 +142,5 @@ class TestPatchComments(utils.APITestCase):
> def test_list_invalid_patch(self):
> """Ensure we get a 404 for a non-existent patch."""
> resp = self.client.get(
> - reverse('api-patch-comment-list', kwargs={'pk': '99999'}))
> + reverse('api-patch-comment-list', kwargs={'patch_id': '99999'}))
> self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
Ok, just teaching the test about the disambiguated name for the id
field, I guess.
- Emily
More information about the Patchwork
mailing list