[PATCH v4 0/9] patch-detail: add unaddressed/addressed status to patch comments
Daniel Axtens
dja at axtens.net
Mon Aug 23 22:17:53 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.
Series applied, with some minor tweaks as (hopefully) documented in my replies.
We could probably have bike-shedded it a bit more but at this stage I'm
pretty happy with where we're at.
- The database schema changes are small and seem fast to apply.
- The UI is fairly unobstrustive IMO, and I'm happy to iterate on it as
we proceed.
- I'm fairly happy that it makes patchwork more useful for more people,
which I think is an outcome worth running some risks to attain.
- The code seems pretty solid and there aren't any objections that I'm
aware to to things like API design that would be difficult to change
later.
Congrats Raxel!
Kind regards,
Daniel
>
> 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
>
> _______________________________________________
> Patchwork mailing list
> Patchwork at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
More information about the Patchwork
mailing list