This series is a revision to the previous version of the patch series. The series addresses the review comments from the v3 series [1] and also adds support for cover letter comments.
Since v3: - Patch 1: separates left align of comment headers - Patch 2: adds renaming of <pk> to <cover_id> - Patch 3: adds a utils file for template tags and filters - Patch 4: split from [v3 07/10] to only add addressed field to comments - Patch 5: split from [v3 07/10] to only change edit permissions for comments - Patch 6: add support for cover letter comments endpoint - Patch 7: split from [v3 08/10] to separate auto-generated files - Patch 8: add label and button for cover letter comments - Patch 9: reword release notes [1] https://lists.ozlabs.org/pipermail/patchwork/2021-August/007074.html Raxel Gutierrez (9): patch-detail: left align message headers api: change <pk> parameter to <cover_id> for cover comments endpoint templatetags: add utils template filters and tags models: add addressed field models: change edit permissions for comments api: add comments detail endpoint api: add auto-generated OpenAPI schema files patch-detail: add label and button for comment addressed status docs: add release note for addressed/unaddressed comments docs/api/schemas/generate-schemas.py | 4 +- docs/api/schemas/latest/patchwork.yaml | 159 +- docs/api/schemas/patchwork.j2 | 165 + docs/api/schemas/v1.3/patchwork.yaml | 2770 +++++++++++++++++ htdocs/css/style.css | 50 +- htdocs/js/submission.js | 20 + patchwork/api/base.py | 24 +- patchwork/api/check.py | 20 +- patchwork/api/comment.py | 148 +- patchwork/api/cover.py | 2 +- .../migrations/0045_auto_20210817_0136.py | 23 + patchwork/models.py | 20 +- patchwork/templates/patchwork/submission.html | 77 +- patchwork/templatetags/utils.py | 18 + patchwork/tests/api/test_comment.py | 390 ++- patchwork/urls.py | 22 +- patchwork/views/patch.py | 4 +- ...essed-patch-comments-bfe71689b6f35a22.yaml | 18 + 18 files changed, 3818 insertions(+), 116 deletions(-) create mode 100644 docs/api/schemas/v1.3/patchwork.yaml create mode 100644 patchwork/migrations/0045_auto_20210817_0136.py create mode 100644 patchwork/templatetags/utils.py create mode 100644 releasenotes/notes/comment-detail-endpoint-for-addressed-unaddressed-patch-comments-bfe71689b6f35a22.yaml Range-diff against v3: 1: 943864ee < -: -------- api: change <pk> parameter to <patch_id> for comments endpoint 2: 1c8ffa52 < -: -------- patch-detail: clean up patch detail page template 3: acfff461 < -: -------- patch-detail: refactor JS code into submission.js 4: f50f3450 < -: -------- patch-detail: change patch info toggles from links to buttons 5: 360b5454 < -: -------- static: add JS Cookie library to get csrftoken for client-side requests 6: abee581c < -: -------- static: add rest.js to handle PATCH requests & respective responses 7: a9cf3606 < -: -------- models: add addressed field and change edit permissions for comments -: -------- > 1: 4898823a patch-detail: left align message headers -: -------- > 2: f03fc850 api: change <pk> parameter to <cover_id> for cover comments endpoint -: -------- > 3: 96d9495b templatetags: add utils template filters and tags -: -------- > 4: 97ac397f models: add addressed field -: -------- > 5: 598e8432 models: change edit permissions for comments -: -------- > 6: 0aff2bd8 api: add comments detail endpoint 8: 86e01dc6 ! 7: abb11759 api: add patch comments detail endpoint and respective tests @@ Metadata Author: Raxel Gutierrez <ra...@google.com> ## Commit message ## - api: add patch comments detail endpoint and respective tests - - Add new endpoint for patch comments at api/.../comments/<comment_id>. - The endpoint will make it possible to use the REST API to update the new - `addressed` field for individual patch comments with JavaScript on the - client side. In the process of these changes, clean up 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). - - Add the OpenAPI definition of the new endpoint and upgrade API version - to v1.3 to reflect the new endpoint as minor change for semantic - versioning. - - Add tests for the new api/.../comments/<comment_id> endpoint that takes - GET, PATCH, and PUT requests. The tests cover retrieval and update - requests and handle calls from the various API versions. Also, they - handle permissions for update requests on the new `addressed` field and - invalid update values for the `addressed` field. - - Add `addressed` field to create_patch_comment helper in api tests - utils.py. - - ## docs/api/schemas/generate-schemas.py ## -@@ docs/api/schemas/generate-schemas.py: 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(): + api: add auto-generated OpenAPI schema files ## docs/api/schemas/latest/patchwork.yaml ## @@ docs/api/schemas/latest/patchwork.yaml: info: @@ docs/api/schemas/latest/patchwork.yaml: paths: $ref: '#/components/schemas/Error' tags: - comments -+ /api/patches/{patch_id}/comments/{comment_id}/: ++ /api/covers/{cover_id}/comments/{comment_id}/: + parameters: + - in: path -+ name: patch_id -+ description: A unique integer value identifying the parent patch. ++ name: cover_id ++ description: A unique integer value identifying the parent cover. + required: true + schema: -+ title: Patch ID ++ title: Cover ID + type: integer + - in: path + name: comment_id @@ docs/api/schemas/latest/patchwork.yaml: paths: + title: Comment ID + type: integer + get: -+ description: Show a patch comment. -+ operationId: patch_comments_read ++ description: Show a cover comment. ++ operationId: cover_comments_read + responses: + '200': + description: '' @@ docs/api/schemas/latest/patchwork.yaml: paths: + tags: + - comments + patch: -+ description: Update a patch comment (partial). -+ operationId: patch_comments_partial_update ++ description: Update a cover comment (partial). ++ operationId: cover_comments_partial_update + requestBody: + $ref: '#/components/requestBodies/Comment' + responses: @@ docs/api/schemas/latest/patchwork.yaml: paths: + $ref: '#/components/schemas/Error' + tags: + - comments - /api/patches/{patch_id}/checks/: - parameters: - - in: path -@@ docs/api/schemas/latest/patchwork.yaml: components: - application/x-www-form-urlencoded: - schema: - $ref: '#/components/schemas/CheckCreate' -+ Comment: -+ required: true -+ content: -+ application/json: -+ schema: -+ $ref: '#/components/schemas/CommentUpdate' - Patch: - required: true - content: -@@ docs/api/schemas/latest/patchwork.yaml: components: - additionalProperties: - type: string - readOnly: true -+ addressed: -+ title: Addressed -+ type: boolean -+ CommentUpdate: -+ type: object -+ properties: -+ addressed: -+ title: Addressed -+ type: boolean - CoverList: - type: object - properties: -@@ docs/api/schemas/latest/patchwork.yaml: components: - previous_relation: - title: Previous relation - type: string -+ nullable: true - current_relation: - title: Current relation - type: string -+ nullable: true - EventPatchDelegated: - allOf: - - $ref: '#/components/schemas/EventBase' -@@ docs/api/schemas/latest/patchwork.yaml: components: - items: - type: string - readOnly: true -+ ErrorCommentUpdate: -+ type: object -+ properties: -+ addressed: -+ title: Addressed -+ type: array -+ items: -+ type: string - ErrorPatchUpdate: - type: object - properties: - - ## docs/api/schemas/patchwork.j2 ## -@@ docs/api/schemas/patchwork.j2: paths: + /api/events/: + get: + description: List events. +@@ docs/api/schemas/latest/patchwork.yaml: paths: $ref: '#/components/schemas/Error' tags: - comments -+{% if version >= (1, 3) %} -+ /api/{{ version_url }}patches/{patch_id}/comments/{comment_id}/: ++ /api/patches/{patch_id}/comments/{comment_id}/: + parameters: + - in: path + name: patch_id @@ docs/api/schemas/patchwork.j2: paths: + $ref: '#/components/schemas/Error' + tags: + - comments -+{% endif %} - /api/{{ version_url }}patches/{patch_id}/checks/: + /api/patches/{patch_id}/checks/: parameters: - in: path -@@ docs/api/schemas/patchwork.j2: components: +@@ docs/api/schemas/latest/patchwork.yaml: 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: -@@ docs/api/schemas/patchwork.j2: components: +@@ docs/api/schemas/latest/patchwork.yaml: components: additionalProperties: type: string readOnly: true -+{% if version >= (1, 3) %} + addressed: + title: Addressed + type: boolean @@ docs/api/schemas/patchwork.j2: components: + addressed: + title: Addressed + type: boolean -+{% endif %} CoverList: type: object properties: -@@ docs/api/schemas/patchwork.j2: components: +@@ docs/api/schemas/latest/patchwork.yaml: components: + previous_relation: + title: Previous relation + type: string ++ nullable: true + current_relation: + title: Current relation + type: string ++ nullable: true + EventPatchDelegated: + allOf: + - $ref: '#/components/schemas/EventBase' +@@ docs/api/schemas/latest/patchwork.yaml: components: items: type: string readOnly: true -+{% if version >= (1, 3) %} + ErrorCommentUpdate: + type: object + properties: @@ docs/api/schemas/patchwork.j2: components: + type: array + items: + type: string -+{% endif %} ErrorPatchUpdate: type: object properties: @@ docs/api/schemas/v1.3/patchwork.yaml (new) + $ref: '#/components/schemas/Error' + tags: + - comments ++ /api/1.3/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 + /api/1.3/events/: + get: + description: List events. @@ docs/api/schemas/v1.3/patchwork.yaml (new) + title: First name + type: string + readOnly: true - - ## patchwork/api/base.py ## -@@ - # - # SPDX-License-Identifier: GPL-2.0-or-later - -+import rest_framework - - from django.conf import settings - from django.shortcuts import get_object_or_404 -@@ patchwork/api/base.py: 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. - -@@ patchwork/api/base.py: class LinkHeaderPagination(PageNumberPagination): - - - class PatchworkPermission(permissions.BasePermission): -- """This permission works for Project and Patch model objects""" -+ """ -+ This permission works for Project, Patch, and PatchComment -+ model objects -+ """ - def has_object_permission(self, request, view, obj): - # read only for everyone - if request.method in permissions.SAFE_METHODS: - - ## patchwork/api/check.py ## -@@ - 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 -@@ patchwork/api/check.py: 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') - - ## patchwork/api/comment.py ## -@@ - - import email.parser - -+from django.shortcuts import get_object_or_404 - from django.http import Http404 - from rest_framework.generics import ListAPIView -+from rest_framework.generics import RetrieveUpdateAPIView -+from rest_framework.serializers import HiddenField - from rest_framework.serializers import SerializerMethodField - - from patchwork.api.base import BaseHyperlinkedModelSerializer -+from patchwork.api.base import MultipleFieldLookupMixin - from patchwork.api.base import PatchworkPermission -+from patchwork.api.base import CurrentPatchDefault - from patchwork.api.embedded import PersonSerializer - from patchwork.models import Cover - from patchwork.models import CoverComment -@@ patchwork/api/comment.py: class CoverCommentListSerializer(BaseCommentListSerializer): - versioned_fields = BaseCommentListSerializer.Meta.versioned_fields - - --class PatchCommentListSerializer(BaseCommentListSerializer): -+class PatchCommentSerializer(BaseCommentListSerializer): -+ -+ patch = HiddenField(default=CurrentPatchDefault()) - - class Meta: - model = PatchComment -- fields = BaseCommentListSerializer.Meta.fields -- read_only_fields = fields -+ fields = BaseCommentListSerializer.Meta.fields + ( -+ 'patch', 'addressed') -+ read_only_fields = BaseCommentListSerializer.Meta.fields + ('patch', ) -+ versioned_fields = { -+ '1.3': ('patch', 'addressed'), -+ } -+ extra_kwargs = { -+ 'url': {'view_name': 'api-patch-comment-detail'} -+ } - versioned_fields = BaseCommentListSerializer.Meta.versioned_fields - - -+class PatchCommentMixin(object): -+ -+ permission_classes = (PatchworkPermission,) -+ serializer_class = PatchCommentSerializer -+ -+ def get_object(self): -+ queryset = self.filter_queryset(self.get_queryset()) -+ comment_id = self.kwargs['comment_id'] -+ try: -+ obj = queryset.get(id=int(comment_id)) -+ except (ValueError, PatchComment.DoesNotExist): -+ obj = get_object_or_404(queryset, linkname=comment_id) -+ self.kwargs['comment_id'] = obj.id -+ self.check_object_permissions(self.request, obj) -+ return obj -+ -+ def get_queryset(self): -+ patch_id = self.kwargs['patch_id'] -+ if not Patch.objects.filter(id=patch_id).exists(): -+ raise Http404 -+ -+ return PatchComment.objects.filter( -+ patch=patch_id -+ ).select_related('submitter') -+ -+ - class CoverCommentList(ListAPIView): - """List cover comments""" - -@@ patchwork/api/comment.py: class CoverCommentList(ListAPIView): - ).select_related('submitter') - - --class PatchCommentList(ListAPIView): -- """List comments""" -+class PatchCommentList(PatchCommentMixin, ListAPIView): -+ """List patch comments""" - -- permission_classes = (PatchworkPermission,) -- serializer_class = PatchCommentListSerializer - search_fields = ('subject',) - ordering_fields = ('id', 'subject', 'date', 'submitter') - ordering = 'id' - lookup_url_kwarg = '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') - - ## patchwork/tests/api/test_comment.py ## -@@ patchwork/tests/api/test_comment.py: from django.conf import settings - from django.urls import NoReverseMatch - from django.urls import reverse - -+from patchwork.models import PatchComment - 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: -@@ patchwork/tests/api/test_comment.py: class TestCoverComments(utils.APITestCase): - @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]) -@@ patchwork/tests/api/test_comment.py: 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) -+ comment = 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]) - - 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) - - ## patchwork/urls.py ## -@@ patchwork/urls.py: 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', -+ ), -+ ] -+ - 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( 9: a4091be6 ! 8: 8f95dd1e patch-detail: add label and button for comment addressed status @@ Metadata ## Commit message ## patch-detail: add label and button for comment addressed status - Add new label to patch comments to show the status of whether they are - addressed or not and add an adjacent button to allow users to change the - status of the comment. Only users that can edit the patch (i.e. patch - author, delegate, project maintainers) as well as comment authors can - change the status of a comment. Before [1] and after [2] images for - reference. - - Refactor both the message containers in the "Commit Message" and - "Comments" to have the same styling through the `patch-message` class so - that the headers are normalized to be left-aligned. Also, pass context - about whether the patch is `editable` by the user to the patch-detail - template. + Add new label to patch and cover comments to show the status of whether + they are addressed or not and add an adjacent button to allow users to + change the status of the comment. Only users that can edit the patch + (i.e. patch author, delegate, project maintainers) as well as comment + authors can change the status of a patch comment. For cover comments, + there are no delegates, so only maintainers and cover/cover comment + authors can edit the status of the cover comment. Before [1] and after + [2] images for reference. Use new comment detail REST API endpoint to update the addressed field - value when "Addressed" or "Unaddressed" buttons are clicked. After, the - request is made, the appearance of the comment status label and buttons - are toggled appropriately. + value when "Addressed" or "Unaddressed" buttons are clicked. After a + successful request is made, the appearance of the comment status label + and buttons are toggled appropriately. For unsuccessful requests (e.g. + network errors prevents reaching the server), the error message is + populated to the page. A future improvement on this behavior is to add + a spinner to the button to provide a feedback that the request is in a + pending state until it's handled. - [1] https://imgur.com/NDfcPJy - [2] https://imgur.com/kIhohED + [1] https://imgur.com/3ZKzgjN + [2] https://imgur.com/hWZrrnM ## htdocs/css/style.css ## @@ -+:root { -+ --light-color: #F7F7F7; -+ --warning-color: #f0ad4e; -+ --success-color: #5cb85c; -+} -+ - h2 { - font-size: 25px; - margin: 18px 0 18px 0; + :root { ++ --light-color:rgb(247, 247, 247); + --success-color:rgb(92, 184, 92); + --warning-color:rgb(240, 173, 78); + --danger-color:rgb(217, 83, 79); @@ htdocs/css/style.css: pre { top: 17em; } @@ htdocs/css/style.css: pre { /* Bootstrap overrides */ .navbar-inverse .navbar-brand > a { -@@ htdocs/css/style.css: table.patch-meta tr th, table.patch-meta tr td { - color: #f7977a; - } - --.comment .meta { -+.patch-message .meta { -+ display: flex; -+ align-items: center; - background: #f0f0f0; - padding: 0.3em 0.5em; - } - --.comment .content { -+.patch-message .message-date { -+ margin-left: 8px; -+} -+ -+.patch-message .content { - border: 0; - } - @@ htdocs/css/style.css: table.patch-meta tr th, table.patch-meta tr td { font-family: "DejaVu Sans Mono", fixed; } @@ htdocs/js/submission.js +import { updateProperty } from "./rest.js"; + $( document ).ready(function() { ++ const patchMeta = document.getElementById("patch-meta"); function toggleDiv(link_id, headers_id, label_show, label_hide) { const link = document.getElementById(link_id) + const headers = document.getElementById(headers_id) @@ htdocs/js/submission.js: $( document ).ready(function() { } } + $("button[class^='comment-action']").click((event) => { -+ const patchId = document.getElementById("patch").dataset.patchId; ++ const submissionType = patchMeta.dataset.submissionType; ++ const submissionId = patchMeta.dataset.submissionId; + const commentId = event.target.parentElement.dataset.commentId; -+ const url = "/api/patches/" + patchId + "/comments/" + commentId + "/"; ++ const url = `/api/${submissionType}/${submissionId}/comments/${commentId}/`; + const data = {'addressed': event.target.value} ; + const updateMessage = { -+ 'none': "No comments updated", -+ 'some': "1 comment(s) updated", ++ 'error': "No comments updated", ++ 'success': "1 comment(s) updated", + }; -+ updateProperty(url, data, updateMessage); -+ $("div[class^='comment-status-bar-'][data-comment-id='"+commentId+"']").toggleClass("hidden"); ++ updateProperty(url, data, updateMessage).then(isSuccess => { ++ if (isSuccess) { ++ $("div[class^='comment-status-bar-'][data-comment-id='"+commentId+"']").toggleClass("hidden"); ++ } ++ }) + }); + // Click listener to show/hide headers @@ htdocs/js/submission.js: $( document ).ready(function() { ## patchwork/templates/patchwork/submission.html ## @@ - {% else %} - <h2>Message</h2> - {% endif %} --<div class="comment"> --<div class="meta"> -- <span>{{ submission.submitter|personify:project }}</span> -- <span class="pull-right">{{ submission.date }} UTC</span> --</div> --<pre class="content"> --{{ submission|commentsyntax }} --</pre> -+<div class="patch-message"> -+ <div class="meta"> -+ <span>{{ submission.submitter|personify:project }}</span> -+ <span class="message-date">{{ submission.date }} UTC</span> -+ </div> -+ <pre class="content"> -+ {{ submission|commentsyntax }} -+ </pre> + {% load person %} + {% load patch %} + {% load static %} ++{% load utils %} + + {% block headers %} + <script type="module" src="{% static "js/submission.js" %}"></script> +@@ + <h1>{{ submission.name }}</h1> </div> - {% for item in comments %} +-<table class="patch-meta"> ++<table ++ id="patch-meta" ++ class="patch-meta" ++ data-submission-type={{submission|verbose_name_plural|lower}} ++ data-submission-id={{submission.id}} ++> + <tr> + <th>Message ID</th> + {% if submission.list_archive_url %} @@ + {% if forloop.first %} + <h2>Comments</h2> {% endif %} - +- ++{% is_editable item user as comment_is_editable %} <a name="{{ item.id }}"></a> --<div class="comment"> + <div class="submission-message"> -<div class="meta"> - <span>{{ item.submitter|personify:project }}</span> -- <span class="pull-right">{{ item.date }} UTC | <a href="{% url 'comment-redirect' comment_id=item.id %}" -- >#{{ forloop.counter }}</a></span> +- <span class="message-date">{{ item.date }} UTC | +- <a href="{% url 'comment-redirect' comment_id=item.id %}">#{{ forloop.counter }}</a> +- </span> -</div> -<pre class="content"> -{{ item|commentsyntax }} -</pre> -+<div class="patch-message"> + <div class="meta"> + {{ item.submitter|personify:project }} + <span class="message-date">{{ item.date }} UTC | @@ patchwork/templates/patchwork/submission.html + <span class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span> + Addressed + </div> -+ {% if editable %} ++ {% if editable or comment_is_editable %} + <button class="comment-action-unaddressed text-warning" value="false"> + <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span> -+ Unaddressed ++ Mark Unaddressed + </button> + {% endif %} + </div> @@ patchwork/templates/patchwork/submission.html + <span class="glyphicon glyphicon-warning-sign" aria-hidden="true"></span> + Unaddressed + </div> -+ {% if editable %} ++ {% if editable or comment_is_editable %} + <button class="comment-action-addressed text-success" value="true"> + <span class="glyphicon glyphicon-ok-circle" aria-hidden="true"></span> -+ Addressed ++ Mark Addressed + </button> + {% endif %} + </div> @@ patchwork/templates/patchwork/submission.html </div> {% endfor %} -@@ - {% include "patchwork/partials/download-buttons.html" %} - <h2>Patch</h2> - </div> --<div id="patch" class="patch"> -+<div id="patch" data-patch-id={{submission.id}} class="patch"> - <pre class="content"> - {{ submission|patchsyntax }} - </pre> ## patchwork/views/patch.py ## @@ patchwork/views/patch.py: def patch_detail(request, project_id, msgid): + + comments = patch.comments.all() + comments = comments.select_related('submitter') +- comments = comments.only('submitter', 'date', 'id', 'content', 'patch') ++ comments = comments.only('submitter', 'date', 'id', 'content', 'patch', ++ 'addressed') + + if patch.related: + related_same_project = patch.related.patches.only( +@@ patchwork/views/patch.py: def patch_detail(request, project_id, msgid): patch.check_set.all().select_related('user'), ) context['submission'] = patch 10: 457d8e2a < -: -------- docs: add release note for addressed/unaddressed comments -: -------- > 9: ec3edbee docs: add release note for addressed/unaddressed comments -- 2.33.0.rc2.250.ged5fa647cd-goog _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
