From: Raxel Gutierrez Date: Fri, 20 Aug 2021 04:50:27 +0000 (+0000) Subject: api: add comments detail endpoint X-Git-Tag: v3.1.0~69 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=88f56051c41de5dc643a7ce1c0911974d8cb3c70;p=thirdparty%2Fpatchwork.git api: add comments detail endpoint Add new endpoint for patch and cover comments at api/.../comments/. 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 [dja: changes to not conflict with, and to adopt the changes in, fecf7c86c2c5 various other minor changes as described on list] Signed-off-by: Daniel Axtens --- 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 6cb703b1..d870a511 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,37 @@ 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 + + class CurrentCoverDefault(object): + requires_context = True + + def __call__(self, serializer_field): + return serializer_field.context['request'].cover +else: + class CurrentPatchDefault(object): + def set_context(self, serializer_field): + self.patch = serializer_field.context['request'].patch + + def __call__(self): + return self.patch + + class CurrentCoverDefault(object): + def set_context(self, serializer_field): + self.patch = serializer_field.context['request'].cover + + def __call__(self): + return self.cover + + class LinkHeaderPagination(PageNumberPagination): """Provide pagination based on rfc5988. @@ -44,7 +76,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 5137c1b9..aeb34889 100644 --- a/patchwork/api/check.py +++ b/patchwork/api/check.py @@ -4,7 +4,7 @@ # SPDX-License-Identifier: GPL-2.0-or-later from django.http.request import QueryDict -import rest_framework + from rest_framework.exceptions import PermissionDenied from rest_framework.generics import get_object_or_404 from rest_framework.generics import ListCreateAPIView @@ -16,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 479a85fe..b150365e 100644 --- a/patchwork/api/comment.py +++ b/patchwork/api/comment.py @@ -7,10 +7,15 @@ import email.parser from rest_framework.generics import get_object_or_404 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 CurrentCoverDefault +from patchwork.api.base import CurrentPatchDefault from patchwork.api.embedded import PersonSerializer from patchwork.models import Cover from patchwork.models import CoverComment @@ -49,63 +54,135 @@ 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 + + def get_object(self): + queryset = self.filter_queryset(self.get_queryset()) + comment_id = self.kwargs['comment_id'] + obj = get_object_or_404(queryset, id=comment_id) + self.check_object_permissions(self.request, obj) + return obj + + def get_queryset(self): + cover_id = self.kwargs['cover_id'] + get_object_or_404(Cover, pk=cover_id) + + return CoverComment.objects.filter( + cover=cover_id + ).select_related('submitter') -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', ) + 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=comment_id) + self.check_object_permissions(self.request, obj) + return obj + + def get_queryset(self): + patch_id = self.kwargs['patch_id'] + get_object_or_404(Patch, id=patch_id) + + 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): - get_object_or_404(Cover, pk=self.kwargs['cover_id']) - 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. -class PatchCommentList(ListAPIView): - """List comments""" + put: + Update a cover comment. + """ + lookup_url_kwargs = ('cover_id', 'comment_id') + lookup_fields = ('cover_id', 'id') + + +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): - get_object_or_404(Patch, id=self.kwargs['patch_id']) - 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 0bf74fa0..74acf447 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: @@ -24,18 +30,26 @@ if settings.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): @@ -48,10 +62,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]) @@ -59,23 +72,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_non_existent_cover(self): """Ensure we get a 404 for a non-existent cover letter.""" @@ -90,38 +101,193 @@ class TestCoverComments(utils.APITestCase): reverse('api-cover-comment-list', kwargs={'pk': 'foo'}), ) + @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]) @@ -129,23 +295,21 @@ 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_non_existent_patch(self): """Ensure we get a 404 for a non-existent patch.""" @@ -159,3 +323,159 @@ class TestPatchComments(utils.APITestCase): self.client.get( reverse('api-patch-comment-list', kwargs={'patch_id': 'foo'}), ) + + @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 075ab14b..5ddf2dbd 100644 --- a/patchwork/urls.py +++ b/patchwork/urls.py @@ -343,12 +343,28 @@ if settings.ENABLE_REST_API: ), ] + api_1_3_patterns = [ + path( + 'patches//comments//', + api_comment_views.PatchCommentDetail.as_view(), + name='api-patch-comment-detail', + ), + path( + 'covers//comments//', + api_comment_views.CoverCommentDetail.as_view(), + name='api-cover-comment-detail', + ), + ] + urlpatterns += [ re_path( - r'^api/(?:(?P(1.0|1.1|1.2))/)?', include(api_patterns) + r'^api/(?:(?P(1.0|1.1|1.2|1.3))/)?', include(api_patterns) + ), + re_path( + r'^api/(?:(?P(1.1|1.2|1.3))/)?', include(api_1_1_patterns) ), re_path( - r'^api/(?:(?P(1.1|1.2))/)?', include(api_1_1_patterns) + r'^api/(?:(?P(1.3))/)?', include(api_1_3_patterns) ), # token change path(