[PATCH v4 0/9] patch-detail: add unaddressed/addressed status to patch comments
Raxel Gutierrez
raxel at google.com
Fri Aug 20 14:50:21 AEST 2021
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 <raxel at 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
More information about the Patchwork
mailing list