Raxel Gutierrez <ra...@google.com> writes:
I've merged this with some small changes as detailed below.
> Add new endpoint for patch and cover comments at
> api/.../comments/<comment_id>.
> This involves updating the API version to v1.3 to reflect the new
> endpoints as a minor change, following the usual semantic versioning
> convention.
>
> The endpoint will make it possible to use the REST API to update the new
> `addressed` field for individual patch and cover comments with JavaScript
> on the client side. In the process of these changes, clean up the use of
> the CurrentPatchDefault context so that it exists in base.py and can be
> used throughout the API (e.g. Check and Comment REST endpoints).
>
> The tests cover retrieval and update requests and also handle calls from
> the various API versions. Also, they cover permissions for update
> requests and handle invalid update values for the new `addressed` field.
>
> Signed-off-by: Raxel Gutierrez <ra...@google.com>
> ---
> docs/api/schemas/generate-schemas.py | 4 +-
> docs/api/schemas/patchwork.j2 | 165 ++++++++++++
> patchwork/api/base.py | 24 +-
> patchwork/api/check.py | 20 +-
> patchwork/api/comment.py | 146 ++++++++--
> patchwork/tests/api/test_comment.py | 388 ++++++++++++++++++++++++---
> patchwork/urls.py | 20 +-
> 7 files changed, 682 insertions(+), 85 deletions(-)
>
> diff --git a/docs/api/schemas/generate-schemas.py
> b/docs/api/schemas/generate-schemas.py
> index a0c1e45f..3a436a16 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():
> diff --git a/docs/api/schemas/patchwork.j2 b/docs/api/schemas/patchwork.j2
> index af20743d..3b4ad2f6 100644
> --- a/docs/api/schemas/patchwork.j2
> +++ b/docs/api/schemas/patchwork.j2
> @@ -324,6 +324,74 @@ paths:
> $ref: '#/components/schemas/Error'
> tags:
> - comments
> +{% if version >= (1, 3) %}
> + /api/{{ version_url }}covers/{cover_id}/comments/{comment_id}/:
> + parameters:
> + - in: path
> + name: cover_id
> + description: A unique integer value identifying the parent cover.
> + required: true
> + schema:
> + title: Cover 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 cover comment.
> + operationId: cover_comments_read
> + responses:
> + '200':
> + description: ''
> + content:
> + application/json:
> + schema:
> + $ref: '#/components/schemas/Comment'
> + '404':
> + description: Not found
> + content:
> + application/json:
> + schema:
> + $ref: '#/components/schemas/Error'
> + tags:
> + - comments
> + patch:
> + description: Update a cover comment (partial).
> + operationId: cover_comments_partial_update
> + requestBody:
> + $ref: '#/components/requestBodies/Comment'
> + responses:
> + '200':
> + description: ''
> + content:
> + application/json:
> + schema:
> + $ref: '#/components/schemas/Comment'
> + '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
> +{% endif %}
> /api/{{ version_url }}events/:
> get:
> description: List events.
> @@ -656,6 +724,74 @@ paths:
> $ref: '#/components/schemas/Error'
> tags:
> - comments
> +{% if version >= (1, 3) %}
> + /api/{{ version_url }}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/Comment'
> + '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
> + requestBody:
> + $ref: '#/components/requestBodies/Comment'
> + responses:
> + '200':
> + description: ''
> + content:
> + application/json:
> + schema:
> + $ref: '#/components/schemas/Comment'
> + '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
> +{% endif %}
> /api/{{ version_url }}patches/{patch_id}/checks/:
> parameters:
> - in: path
> @@ -1277,6 +1413,14 @@ components:
> application/x-www-form-urlencoded:
> schema:
> $ref: '#/components/schemas/CheckCreate'
> +{% if version >= (1, 3) %}
> + Comment:
> + required: true
> + content:
> + application/json:
> + schema:
> + $ref: '#/components/schemas/CommentUpdate'
> +{% endif %}
> Patch:
> required: true
> content:
> @@ -1586,6 +1730,17 @@ components:
> additionalProperties:
> type: string
> readOnly: true
> +{% if version >= (1, 3) %}
> + addressed:
> + title: Addressed
> + type: boolean
> + CommentUpdate:
> + type: object
> + properties:
> + addressed:
> + title: Addressed
> + type: boolean
> +{% endif %}
> CoverList:
> type: object
> properties:
> @@ -2659,6 +2814,16 @@ components:
> items:
> type: string
> readOnly: true
> +{% if version >= (1, 3) %}
> + ErrorCommentUpdate:
> + type: object
> + properties:
> + addressed:
> + title: Addressed
> + type: array
> + items:
> + type: string
> +{% endif %}
> ErrorPatchUpdate:
> type: object
> properties:
> diff --git a/patchwork/api/base.py b/patchwork/api/base.py
> index 89a43114..a98644ee 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
> +
> +
> 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, PatchComment
> + and CoverComment 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 a6bf5f8c..2049d2f9 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -6,7 +6,6 @@
> 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 +16,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 5d7a77a1..334016d7 100644
> --- a/patchwork/api/comment.py
> +++ b/patchwork/api/comment.py
> @@ -4,13 +4,19 @@
> # SPDX-License-Identifier: GPL-2.0-or-later
>
> import email.parser
> +import rest_framework
>
> +from django.shortcuts import get_object_or_404
I replaced this with the rest_framework.generics one.
> 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
> @@ -18,6 +24,24 @@ from patchwork.models import Patch
> from patchwork.models import PatchComment
>
>
> +DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
> +
> +
> +if DRF_VERSION > (3, 11):
> + class CurrentCoverDefault(object):
> + requires_context = True
> +
> + def __call__(self, serializer_field):
> + return serializer_field.context['request'].cover
> +else:
> + class CurrentCoverDefault(object):
> + def set_context(self, serializer_field):
> + self.patch = serializer_field.context['request'].cover
> +
> + def __call__(self):
> + return self.cover
> +
> +
I moved these into base.py.
> class BaseCommentListSerializer(BaseHyperlinkedModelSerializer):
>
> web_url = SerializerMethodField()
> @@ -49,65 +73,133 @@ class
> BaseCommentListSerializer(BaseHyperlinkedModelSerializer):
>
> class Meta:
> fields = ('id', 'web_url', 'msgid', 'list_archive_url', 'date',
> - 'subject', 'submitter', 'content', 'headers')
> - read_only_fields = fields
> + 'subject', 'submitter', 'content', 'headers', 'addressed')
> + read_only_fields = ('id', 'web_url', 'msgid', 'list_archive_url',
> + 'date', 'subject', 'submitter', 'content',
> + 'headers')
> versioned_fields = {
> '1.1': ('web_url', ),
> '1.2': ('list_archive_url',),
> + '1.3': ('addressed',),
> }
>
>
> -class CoverCommentListSerializer(BaseCommentListSerializer):
> +class CoverCommentSerializer(BaseCommentListSerializer):
> +
> + cover = HiddenField(default=CurrentCoverDefault())
>
> class Meta:
> model = CoverComment
> - fields = BaseCommentListSerializer.Meta.fields
> - read_only_fields = fields
> + fields = BaseCommentListSerializer.Meta.fields + (
> + 'cover', 'addressed')
> + read_only_fields = BaseCommentListSerializer.Meta.read_only_fields +
> (
> + 'cover', )
> versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
> + extra_kwargs = {
> + 'url': {'view_name': 'api-cover-comment-detail'}
> + }
> +
>
> +class CoverCommentMixin(object):
> +
> + permission_classes = (PatchworkPermission,)
> + serializer_class = CoverCommentSerializer
>
> -class PatchCommentListSerializer(BaseCommentListSerializer):
> + def get_object(self):
> + queryset = self.filter_queryset(self.get_queryset())
> + comment_id = self.kwargs['comment_id']
> + obj = get_object_or_404(queryset, id=int(comment_id))
I got rid of both int() calls; they should no longer be necessary and
also not-fully-guarded calls to int() caused us 500 errors recently.
> + self.check_object_permissions(self.request, obj)
> + return obj
> +
> + def get_queryset(self):
> + cover_id = self.kwargs['cover_id']
> + if not Cover.objects.filter(id=cover_id).exists():
> + raise Http404
> +
> + return CoverComment.objects.filter(
> + cover=cover_id
> + ).select_related('submitter')
> +
> +
> +class PatchCommentSerializer(BaseCommentListSerializer):
> +
> + patch = HiddenField(default=CurrentPatchDefault())
>
> class Meta:
> model = PatchComment
> - fields = BaseCommentListSerializer.Meta.fields
> - read_only_fields = fields
> + fields = BaseCommentListSerializer.Meta.fields + ('patch', )
> + read_only_fields = BaseCommentListSerializer.Meta.read_only_fields +
> (
> + 'patch', )
> versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
> + extra_kwargs = {
> + 'url': {'view_name': 'api-patch-comment-detail'}
> + }
>
>
> -class CoverCommentList(ListAPIView):
> - """List cover comments"""
> +class PatchCommentMixin(object):
>
> permission_classes = (PatchworkPermission,)
> - serializer_class = CoverCommentListSerializer
> + serializer_class = PatchCommentSerializer
> +
> + def get_object(self):
> + queryset = self.filter_queryset(self.get_queryset())
> + comment_id = self.kwargs['comment_id']
> + obj = get_object_or_404(queryset, id=int(comment_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
I replaced these (raise Http404) with get_object_or_404 calls.
> +
> + return PatchComment.objects.filter(
> + patch=patch_id
> + ).select_related('submitter')
> +
> +
> +class CoverCommentList(CoverCommentMixin, ListAPIView):
> + """List cover comments"""
> +
> search_fields = ('subject',)
> ordering_fields = ('id', 'subject', 'date', 'submitter')
> ordering = 'id'
> lookup_url_kwarg = 'cover_id'
>
> - def get_queryset(self):
> - if not Cover.objects.filter(id=self.kwargs['cover_id']).exists():
> - raise Http404
>
> - return CoverComment.objects.filter(
> - cover=self.kwargs['cover_id']
> - ).select_related('submitter')
> +class CoverCommentDetail(CoverCommentMixin, MultipleFieldLookupMixin,
> + RetrieveUpdateAPIView):
> + """
> + get:
> + Show a cover comment.
I added blank lines between methods for consistency with other views
(see e.g. PatchDetail)
> + patch:
> + Update a cover comment.
> + put:
> + Update a cover comment.
> + """
> + lookup_url_kwargs = ('cover_id', 'comment_id')
> + lookup_fields = ('cover_id', 'id')
>
>
> -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 = 'patch_id'
>
> - def get_queryset(self):
> - if not Patch.objects.filter(id=self.kwargs['patch_id']).exists():
> - raise Http404
>
> - return PatchComment.objects.filter(
> - patch=self.kwargs['patch_id']
> - ).select_related('submitter')
> +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/tests/api/test_comment.py
> b/patchwork/tests/api/test_comment.py
> index 53abf8f0..033a1416 100644
> --- a/patchwork/tests/api/test_comment.py
> +++ b/patchwork/tests/api/test_comment.py
> @@ -9,11 +9,17 @@ from django.conf import settings
> from django.urls import NoReverseMatch
> from django.urls import reverse
>
> +from patchwork.models import PatchComment
> +from patchwork.models import CoverComment
> from patchwork.tests.api import utils
> from patchwork.tests.utils import create_cover
> from patchwork.tests.utils import create_cover_comment
> from patchwork.tests.utils import create_patch
> from patchwork.tests.utils import create_patch_comment
> +from patchwork.tests.utils import create_maintainer
> +from patchwork.tests.utils import create_project
> +from patchwork.tests.utils import create_person
> +from patchwork.tests.utils import create_user
> from patchwork.tests.utils import SAMPLE_CONTENT
>
> if settings.ENABLE_REST_API:
> @@ -23,18 +29,26 @@ if settings.ENABLE_REST_API:
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> class TestCoverComments(utils.APITestCase):
> @staticmethod
> - def api_url(cover, version=None):
> - kwargs = {}
> + def api_url(cover, version=None, item=None):
> + kwargs = {'cover_id': cover.id}
> if version:
> kwargs['version'] = version
> - kwargs['cover_id'] = cover.id
> + if item is None:
> + return reverse('api-cover-comment-list', kwargs=kwargs)
> + kwargs['comment_id'] = item.id
> + return reverse('api-cover-comment-detail', kwargs=kwargs)
>
> - return reverse('api-cover-comment-list', kwargs=kwargs)
> + def setUp(self):
> + super(TestCoverComments, self).setUp()
> + self.project = create_project()
> + self.user = create_maintainer(self.project)
> + self.cover = create_cover(project=self.project)
>
> def assertSerialized(self, comment_obj, comment_json):
> self.assertEqual(comment_obj.id, comment_json['id'])
> self.assertEqual(comment_obj.submitter.id,
> comment_json['submitter']['id'])
> + self.assertEqual(comment_obj.addressed, comment_json['addressed'])
> self.assertIn(SAMPLE_CONTENT, comment_json['content'])
>
> def test_list_empty(self):
> @@ -47,10 +61,9 @@ class TestCoverComments(utils.APITestCase):
> @utils.store_samples('cover-comment-list')
> def test_list(self):
> """List cover letter comments."""
> - cover = create_cover()
> - comment = create_cover_comment(cover=cover)
> + comment = create_cover_comment(cover=self.cover)
>
> - resp = self.client.get(self.api_url(cover))
> + resp = self.client.get(self.api_url(self.cover))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(1, len(resp.data))
> self.assertSerialized(comment, resp.data[0])
> @@ -58,23 +71,21 @@ class TestCoverComments(utils.APITestCase):
>
> def test_list_version_1_1(self):
> """List cover letter comments using API v1.1."""
> - cover = create_cover()
> - comment = create_cover_comment(cover=cover)
> + create_cover_comment(cover=self.cover)
>
> - resp = self.client.get(self.api_url(cover, version='1.1'))
> + resp = self.client.get(self.api_url(self.cover, version='1.1'))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(1, len(resp.data))
> - self.assertSerialized(comment, resp.data[0])
> self.assertNotIn('list_archive_url', resp.data[0])
> + self.assertNotIn('addressed', resp.data[0])
>
> def test_list_version_1_0(self):
> - """List cover letter comments using API v1.0."""
> - cover = create_cover()
> - create_cover_comment(cover=cover)
> + """List cover letter comments using API v1.0.
>
> - # check we can't access comments using the old version of the API
> + Ensure we can't access cover comments using the old version of the
> API.
> + """
> with self.assertRaises(NoReverseMatch):
> - self.client.get(self.api_url(cover, version='1.0'))
> + self.client.get(self.api_url(self.cover, version='1.0'))
>
> def test_list_invalid_cover(self):
> """Ensure we get a 404 for a non-existent cover letter."""
> @@ -82,38 +93,193 @@ class TestCoverComments(utils.APITestCase):
> reverse('api-cover-comment-list', kwargs={'cover_id': '99999'}))
> self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
>
> + @utils.store_samples('cover-comment-detail')
> + def test_detail(self):
> + """Show a cover letter comment."""
> + comment = create_cover_comment(cover=self.cover)
> +
> + resp = self.client.get(self.api_url(self.cover, item=comment))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(comment, resp.data)
> +
> + def test_detail_version_1_3(self):
> + """Show a cover letter comment using API v1.3."""
> + comment = create_cover_comment(cover=self.cover)
> +
> + resp = self.client.get(
> + self.api_url(self.cover, version='1.3', item=comment))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(comment, resp.data)
> +
> + def test_detail_version_1_2(self):
> + """Show a cover letter comment using API v1.2."""
> + comment = create_cover_comment(cover=self.cover)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.cover, version='1.2', item=comment))
> +
> + def test_detail_version_1_1(self):
> + """Show a cover letter comment using API v1.1."""
> + comment = create_cover_comment(cover=self.cover)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.cover, version='1.1', item=comment))
> +
> + def test_detail_version_1_0(self):
> + """Show a cover letter comment using API v1.0."""
> + comment = create_cover_comment(cover=self.cover)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.cover, version='1.0', item=comment))
> +
> + @utils.store_samples('cover-comment-detail-error-not-found')
> + def test_detail_invalid_cover(self):
> + """Ensure we handle non-existent cover letters."""
> + comment = create_cover_comment()
> + resp = self.client.get(
> + reverse('api-cover-comment-detail', kwargs={
> + 'cover_id': '99999',
> + 'comment_id': comment.id}
> + ),
> + )
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def _test_update(self, person, **kwargs):
> + submitter = kwargs.get('submitter', person)
> + cover = kwargs.get('cover', self.cover)
> + comment = create_cover_comment(submitter=submitter, cover=cover)
> +
> + if kwargs.get('authenticate', True):
> + self.client.force_authenticate(user=person.user)
> + return self.client.patch(
> + self.api_url(cover, item=comment),
> + {'addressed': kwargs.get('addressed', True)},
> + validate_request=kwargs.get('validate_request', True)
> + )
> +
> + @utils.store_samples('cover-comment-detail-update-authorized')
> + def test_update_authorized(self):
> + """Update an existing cover letter comment as an authorized user.
> +
> + To be authorized users must meet at least one of the following:
> + - project maintainer, cover letter submitter, or cover letter
> + comment submitter
> +
> + Ensure updates can only be performed by authorized users.
> + """
> + # Update as maintainer
> + person = create_person(user=self.user)
> + resp = self._test_update(person=person)
> + self.assertEqual(1, CoverComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as cover letter submitter
> + person = create_person(name='cover-submitter', user=create_user())
> + cover = create_cover(submitter=person)
> + resp = self._test_update(person=person, cover=cover)
> + self.assertEqual(2, CoverComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as cover letter comment submitter
> + person = create_person(name='comment-submitter', user=create_user())
> + cover = create_cover()
> + resp = self._test_update(person=person, cover=cover,
> submitter=person)
> + self.assertEqual(3, CoverComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + @utils.store_samples('cover-comment-detail-update-not-authorized')
> + def test_update_not_authorized(self):
> + """Update an existing cover letter comment when not signed in and
> + not authorized.
> +
> + To be authorized users must meet at least one of the following:
> + - project maintainer, cover letter submitter, or cover letter
> + comment submitter
> +
> + Ensure updates can only be performed by authorized users.
> + """
> + person = create_person(user=self.user)
> + resp = self._test_update(person=person, authenticate=False)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + person = create_person() # normal user without edit permissions
> + resp = self._test_update(person=person) # signed-in
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + @utils.store_samples('cover-comment-detail-update-error-bad-request')
> + def test_update_invalid_addressed(self):
> + """Update an existing cover letter comment using invalid values.
> +
> + Ensure we handle invalid cover letter comment addressed values.
> + """
> + person = create_person(name='cover-submitter', user=create_user())
> + cover = create_cover(submitter=person)
> + resp = self._test_update(person=person,
> + cover=cover,
> + addressed='not-valid',
> + validate_request=False)
> + self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
> + self.assertFalse(
> + getattr(CoverComment.objects.all().first(), 'addressed')
> + )
> +
> + def test_create_delete(self):
> + """Ensure creates and deletes aren't allowed"""
> + comment = create_cover_comment(cover=self.cover)
> + self.user.is_superuser = True
> + self.user.save()
> + self.client.force_authenticate(user=self.user)
> +
> + resp = self.client.post(self.api_url(self.cover, item=comment))
> + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED,
> resp.status_code)
> +
> + resp = self.client.delete(self.api_url(self.cover, item=comment))
> + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED,
> resp.status_code)
> +
>
> @unittest.skipUnless(settings.ENABLE_REST_API, 'requires ENABLE_REST_API')
> class TestPatchComments(utils.APITestCase):
> @staticmethod
> - def api_url(patch, version=None):
> - kwargs = {}
> + def api_url(patch, version=None, item=None):
> + kwargs = {'patch_id': patch.id}
> if version:
> kwargs['version'] = version
> - kwargs['patch_id'] = patch.id
> + if item is None:
> + return reverse('api-patch-comment-list', kwargs=kwargs)
> + kwargs['comment_id'] = item.id
> + return reverse('api-patch-comment-detail', kwargs=kwargs)
>
> - return reverse('api-patch-comment-list', kwargs=kwargs)
> + def setUp(self):
> + super(TestPatchComments, self).setUp()
> + self.project = create_project()
> + self.user = create_maintainer(self.project)
> + self.patch = create_patch(project=self.project)
>
> def assertSerialized(self, comment_obj, comment_json):
> self.assertEqual(comment_obj.id, comment_json['id'])
> self.assertEqual(comment_obj.submitter.id,
> comment_json['submitter']['id'])
> + self.assertEqual(comment_obj.addressed, comment_json['addressed'])
> self.assertIn(SAMPLE_CONTENT, comment_json['content'])
>
> def test_list_empty(self):
> """List patch comments when none are present."""
> - patch = create_patch()
> - resp = self.client.get(self.api_url(patch))
> + resp = self.client.get(self.api_url(self.patch))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(0, len(resp.data))
>
> @utils.store_samples('patch-comment-list')
> def test_list(self):
> """List patch comments."""
> - patch = create_patch()
> - comment = create_patch_comment(patch=patch)
> + comment = create_patch_comment(patch=self.patch)
>
> - resp = self.client.get(self.api_url(patch))
> + resp = self.client.get(self.api_url(self.patch))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(1, len(resp.data))
> self.assertSerialized(comment, resp.data[0])
> @@ -121,26 +287,180 @@ class TestPatchComments(utils.APITestCase):
>
> def test_list_version_1_1(self):
> """List patch comments using API v1.1."""
> - patch = create_patch()
> - comment = create_patch_comment(patch=patch)
> + create_patch_comment(patch=self.patch)
>
> - resp = self.client.get(self.api_url(patch, version='1.1'))
> + resp = self.client.get(self.api_url(self.patch, version='1.1'))
> self.assertEqual(status.HTTP_200_OK, resp.status_code)
> self.assertEqual(1, len(resp.data))
> - self.assertSerialized(comment, resp.data[0])
> self.assertNotIn('list_archive_url', resp.data[0])
> + self.assertNotIn('addressed', resp.data[0])
>
> def test_list_version_1_0(self):
> - """List patch comments using API v1.0."""
> - patch = create_patch()
> - create_patch_comment(patch=patch)
> + """List patch comments using API v1.0.
>
> - # check we can't access comments using the old version of the API
> + Ensure we can't access comments using the old version of the API.
> + """
> with self.assertRaises(NoReverseMatch):
> - self.client.get(self.api_url(patch, version='1.0'))
> + self.client.get(self.api_url(self.patch, version='1.0'))
>
> 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={'patch_id': '99999'}))
> self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + @utils.store_samples('patch-comment-detail')
> + def test_detail(self):
> + """Show a patch comment."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + resp = self.client.get(self.api_url(self.patch, item=comment))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(comment, resp.data)
> +
> + def test_detail_version_1_3(self):
> + """Show a patch comment using API v1.3."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + resp = self.client.get(
> + self.api_url(self.patch, version='1.3', item=comment))
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertSerialized(comment, resp.data)
> +
> + def test_detail_version_1_2(self):
> + """Show a patch comment using API v1.2."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.patch, version='1.2', item=comment))
> +
> + def test_detail_version_1_1(self):
> + """Show a patch comment using API v1.1."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.patch, version='1.1', item=comment))
> +
> + def test_detail_version_1_0(self):
> + """Show a patch comment using API v1.0."""
> + comment = create_patch_comment(patch=self.patch)
> +
> + with self.assertRaises(NoReverseMatch):
> + self.client.get(
> + self.api_url(self.patch, version='1.0', item=comment))
> +
> + @utils.store_samples('patch-comment-detail-error-not-found')
> + def test_detail_invalid_patch(self):
> + """Ensure we handle non-existent patches."""
> + comment = create_patch_comment()
> + resp = self.client.get(
> + reverse('api-patch-comment-detail', kwargs={
> + 'patch_id': '99999',
> + 'comment_id': comment.id}
> + ),
> + )
> + self.assertEqual(status.HTTP_404_NOT_FOUND, resp.status_code)
> +
> + def _test_update(self, person, **kwargs):
> + submitter = kwargs.get('submitter', person)
> + patch = kwargs.get('patch', self.patch)
> + comment = create_patch_comment(submitter=submitter, patch=patch)
> +
> + if kwargs.get('authenticate', True):
> + self.client.force_authenticate(user=person.user)
> + return self.client.patch(
> + self.api_url(patch, item=comment),
> + {'addressed': kwargs.get('addressed', True)},
> + validate_request=kwargs.get('validate_request', True)
> + )
> +
> + @utils.store_samples('patch-comment-detail-update-authorized')
> + def test_update_authorized(self):
> + """Update an existing patch comment as an authorized user.
> +
> + To be authorized users must meet at least one of the following:
> + - project maintainer, patch submitter, patch delegate, or
> + patch comment submitter
> +
> + Ensure updates can only be performed by authorized users.
> + """
> + # Update as maintainer
> + person = create_person(user=self.user)
> + resp = self._test_update(person=person)
> + self.assertEqual(1, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as patch submitter
> + person = create_person(name='patch-submitter', user=create_user())
> + patch = create_patch(submitter=person)
> + resp = self._test_update(person=person, patch=patch)
> + self.assertEqual(2, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as patch delegate
> + person = create_person(name='patch-delegate', user=create_user())
> + patch = create_patch(delegate=person.user)
> + resp = self._test_update(person=person, patch=patch)
> + self.assertEqual(3, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + # Update as patch comment submitter
> + person = create_person(name='comment-submitter', user=create_user())
> + patch = create_patch()
> + resp = self._test_update(person=person, patch=patch,
> submitter=person)
> + self.assertEqual(4, PatchComment.objects.all().count())
> + self.assertEqual(status.HTTP_200_OK, resp.status_code)
> + self.assertTrue(resp.data['addressed'])
> +
> + @utils.store_samples('patch-comment-detail-update-not-authorized')
> + def test_update_not_authorized(self):
> + """Update an existing patch comment when not signed in and not
> authorized.
> +
> + To be authorized users must meet at least one of the following:
> + - project maintainer, patch submitter, patch delegate, or
> + patch comment submitter
> +
> + Ensure updates can only be performed by authorized users.
> + """
> + person = create_person(user=self.user)
> + resp = self._test_update(person=person, authenticate=False)
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + person = create_person() # normal user without edit permissions
> + resp = self._test_update(person=person) # signed-in
> + self.assertEqual(status.HTTP_403_FORBIDDEN, resp.status_code)
> +
> + @utils.store_samples('patch-comment-detail-update-error-bad-request')
> + def test_update_invalid_addressed(self):
> + """Update an existing patch comment using invalid values.
> +
> + Ensure we handle invalid patch comment addressed values.
> + """
> + person = create_person(name='patch-submitter', user=create_user())
> + patch = create_patch(submitter=person)
> + resp = self._test_update(person=person,
> + patch=patch,
> + addressed='not-valid',
> + validate_request=False)
> + self.assertEqual(status.HTTP_400_BAD_REQUEST, resp.status_code)
> + self.assertFalse(
> + getattr(PatchComment.objects.all().first(), 'addressed')
> + )
> +
> + def test_create_delete(self):
> + """Ensure creates and deletes aren't allowed"""
> + comment = create_patch_comment(patch=self.patch)
> + self.user.is_superuser = True
> + self.user.save()
> + self.client.force_authenticate(user=self.user)
> +
> + resp = self.client.post(self.api_url(self.patch, item=comment))
> + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED,
> resp.status_code)
> +
> + resp = self.client.delete(self.api_url(self.patch, item=comment))
> + self.assertEqual(status.HTTP_405_METHOD_NOT_ALLOWED,
> resp.status_code)
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 0180e76d..b3f40cbf 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -343,12 +343,28 @@ if settings.ENABLE_REST_API:
> ),
> ]
>
> + api_1_3_patterns = [
> + path(
> + 'patches/<patch_id>/comments/<comment_id>/',
> + api_comment_views.PatchCommentDetail.as_view(),
> + name='api-patch-comment-detail',
> + ),
> + path(
> + 'covers/<cover_id>/comments/<comment_id>/',
> + api_comment_views.CoverCommentDetail.as_view(),
> + name='api-cover-comment-detail',
> + ),
> + ]
I added int: filters to these per the weekend's bugs.
> +
> urlpatterns += [
> re_path(
> - r'^api/(?:(?P<version>(1.0|1.1|1.2))/)?', include(api_patterns)
> + r'^api/(?:(?P<version>(1.0|1.1|1.2|1.3))/)?',
> include(api_patterns)
> + ),
> + re_path(
> + r'^api/(?:(?P<version>(1.1|1.2|1.3))/)?',
> include(api_1_1_patterns)
> ),
> re_path(
> - r'^api/(?:(?P<version>(1.1|1.2))/)?', include(api_1_1_patterns)
> + r'^api/(?:(?P<version>(1.3))/)?', include(api_1_3_patterns)
> ),
> # token change
> path(
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
_______________________________________________
Patchwork mailing list
Patchwork@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/patchwork
