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 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 + + 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)) + 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 + + 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. + 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', + ), + ] + 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