diff mbox series

[RFC/PATCH,1/3] api: add addressed field and detail endpoint for patch comments

Message ID 20210722213543.2592044-2-raxel@google.com
State Superseded
Headers show
Series patch-detail: add unaddressed/addressed status to patch comments | expand
Related show

Commit Message

Raxel Gutierrez July 22, 2021, 9:35 p.m. UTC 
Add addressed boolean field to PatchComment to be able to distinguish
between them in the patch-detail page. Change PatchComment to have same
is_editable from the related patch.

Add 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 patch comments with JavaScript on the client side.
In the process of these changes, clean up use of CurrentPatchDefault so
that it exists in base.py and can be used throughout the API.

Signed-off-by: Raxel Gutierrez <raxel@google.com>
---
 patchwork/api/base.py                         | 24 +++++-
 patchwork/api/check.py                        | 21 +----
 patchwork/api/comment.py                      | 76 +++++++++++++++----
 .../migrations/0045_patchcomment_addressed.py | 18 +++++
 patchwork/models.py                           |  3 +-
 patchwork/urls.py                             |  7 +-
 6 files changed, 111 insertions(+), 38 deletions(-)
 create mode 100644 patchwork/migrations/0045_patchcomment_addressed.py

Comments

Raxel Gutierrez July 23, 2021, 2:27 p.m. UTC | #1 
Hi,

I'm sure this might be asked but I used the
RetrieveUpdateDestroyAPIView [1] which handles delete requests. I
changed this to RetrieveUpdateAPIView because patch comments should
not be removable. Currently working on tests for the endpoint :)

[1] https://www.django-rest-framework.org/api-guide/generic-views/#retrieveupdatedestroyapiview

On Thu, Jul 22, 2021 at 5:36 PM Raxel Gutierrez <raxel@google.com> wrote:
>
> Add addressed boolean field to PatchComment to be able to distinguish
> between them in the patch-detail page. Change PatchComment to have same
> is_editable from the related patch.
>
> Add 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 patch comments with JavaScript on the client side.
> In the process of these changes, clean up use of CurrentPatchDefault so
> that it exists in base.py and can be used throughout the API.
>
> Signed-off-by: Raxel Gutierrez <raxel@google.com>
> ---
>  patchwork/api/base.py                         | 24 +++++-
>  patchwork/api/check.py                        | 21 +----
>  patchwork/api/comment.py                      | 76 +++++++++++++++----
>  .../migrations/0045_patchcomment_addressed.py | 18 +++++
>  patchwork/models.py                           |  3 +-
>  patchwork/urls.py                             |  7 +-
>  6 files changed, 111 insertions(+), 38 deletions(-)
>  create mode 100644 patchwork/migrations/0045_patchcomment_addressed.py
>
> diff --git a/patchwork/api/base.py b/patchwork/api/base.py
> index 89a4311..856fbd3 100644
> --- a/patchwork/api/base.py
> +++ b/patchwork/api/base.py
> @@ -3,6 +3,7 @@
>  #
>  # SPDX-License-Identifier: GPL-2.0-or-later
>
> +import rest_framework
>
>  from django.conf import settings
>  from django.shortcuts import get_object_or_404
> @@ -15,6 +16,24 @@ from rest_framework.serializers import HyperlinkedModelSerializer
>  from patchwork.api import utils
>
>
> +DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
> +
> +
> +if DRF_VERSION > (3, 11):
> +    class CurrentPatchDefault(object):
> +        requires_context = True
> +
> +        def __call__(self, serializer_field):
> +            return serializer_field.context['request'].patch
> +else:
> +    class CurrentPatchDefault(object):
> +        def set_context(self, serializer_field):
> +            self.patch = serializer_field.context['request'].patch
> +
> +        def __call__(self):
> +            return self.patch
> +
> +
>  class LinkHeaderPagination(PageNumberPagination):
>      """Provide pagination based on rfc5988.
>
> @@ -44,7 +63,10 @@ class LinkHeaderPagination(PageNumberPagination):
>
>
>  class PatchworkPermission(permissions.BasePermission):
> -    """This permission works for Project and Patch model objects"""
> +    """
> +    This permission works for Project, Patch, and PatchComment
> +    model objects
> +    """
>      def has_object_permission(self, request, view, obj):
>          # read only for everyone
>          if request.method in permissions.SAFE_METHODS:
> diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> index a6bf5f8..fde6b61 100644
> --- a/patchwork/api/check.py
> +++ b/patchwork/api/check.py
> @@ -6,7 +6,7 @@
>  from django.http import Http404
>  from django.http.request import QueryDict
>  from django.shortcuts import get_object_or_404
> -import rest_framework
> +
>  from rest_framework.exceptions import PermissionDenied
>  from rest_framework.generics import ListCreateAPIView
>  from rest_framework.generics import RetrieveAPIView
> @@ -17,30 +17,13 @@ from rest_framework.serializers import ValidationError
>
>  from patchwork.api.base import CheckHyperlinkedIdentityField
>  from patchwork.api.base import MultipleFieldLookupMixin
> +from patchwork.api.base import CurrentPatchDefault
>  from patchwork.api.embedded import UserSerializer
>  from patchwork.api.filters import CheckFilterSet
>  from patchwork.models import Check
>  from patchwork.models import Patch
>
>
> -DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
> -
> -
> -if DRF_VERSION > (3, 11):
> -    class CurrentPatchDefault(object):
> -        requires_context = True
> -
> -        def __call__(self, serializer_field):
> -            return serializer_field.context['request'].patch
> -else:
> -    class CurrentPatchDefault(object):
> -        def set_context(self, serializer_field):
> -            self.patch = serializer_field.context['request'].patch
> -
> -        def __call__(self):
> -            return self.patch
> -
> -
>  class CheckSerializer(HyperlinkedModelSerializer):
>
>      url = CheckHyperlinkedIdentityField('api-check-detail')
> diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
> index 43b26c6..7f1e401 100644
> --- a/patchwork/api/comment.py
> +++ b/patchwork/api/comment.py
> @@ -5,12 +5,17 @@
>
>  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 RetrieveUpdateDestroyAPIView
> +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
> @@ -55,6 +60,9 @@ class BaseCommentListSerializer(BaseHyperlinkedModelSerializer):
>              '1.1': ('web_url', ),
>              '1.2': ('list_archive_url',),
>          }
> +        extra_kwargs = {
> +            'url': {'view_name': 'api-patch-comment-detail'},
> +        }
>
>
>  class CoverCommentListSerializer(BaseCommentListSerializer):
> @@ -66,15 +74,47 @@ 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 = fields[:-1]  # able to write to addressed field
> +        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"""
>
> @@ -94,20 +134,24 @@ 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 = 'pk'
> -
> -    def get_queryset(self):
> -        if not Patch.objects.filter(pk=self.kwargs['pk']).exists():
> -            raise Http404
> -
> -        return PatchComment.objects.filter(
> -            patch=self.kwargs['pk']
> -        ).select_related('submitter')
> +    lookup_url_kwarg = 'patch_id'
> +
> +
> +class PatchCommentDetail(PatchCommentMixin, MultipleFieldLookupMixin,
> +                         RetrieveUpdateDestroyAPIView):
> +    """
> +    get:
> +    Show a patch comment.
> +    patch:
> +    Update a patch comment.
> +    put:
> +    Update a patch comment.
> +    """
> +    lookup_url_kwargs = ('patch_id', 'comment_id')
> +    lookup_fields = ('patch_id', 'id')
> diff --git a/patchwork/migrations/0045_patchcomment_addressed.py b/patchwork/migrations/0045_patchcomment_addressed.py
> new file mode 100644
> index 0000000..92e6c4e
> --- /dev/null
> +++ b/patchwork/migrations/0045_patchcomment_addressed.py
> @@ -0,0 +1,18 @@
> +# Generated by Django 3.1.12 on 2021-07-16 04:12
> +
> +from django.db import migrations, models
> +
> +
> +class Migration(migrations.Migration):
> +
> +    dependencies = [
> +        ('patchwork', '0044_add_project_linkname_validation'),
> +    ]
> +
> +    operations = [
> +        migrations.AddField(
> +            model_name='patchcomment',
> +            name='addressed',
> +            field=models.BooleanField(default=False),
> +        ),
> +    ]
> diff --git a/patchwork/models.py b/patchwork/models.py
> index 00273da..0a77b23 100644
> --- a/patchwork/models.py
> +++ b/patchwork/models.py
> @@ -693,6 +693,7 @@ class PatchComment(EmailMixin, models.Model):
>          related_query_name='comment',
>          on_delete=models.CASCADE,
>      )
> +    addressed = models.BooleanField(default=False)
>
>      @property
>      def list_archive_url(self):
> @@ -718,7 +719,7 @@ class PatchComment(EmailMixin, models.Model):
>          self.patch.refresh_tag_counts()
>
>      def is_editable(self, user):
> -        return False
> +        return self.patch.is_editable(user)
>
>      class Meta:
>          ordering = ['date']
> diff --git a/patchwork/urls.py b/patchwork/urls.py
> index 6ac9b81..81db982 100644
> --- a/patchwork/urls.py
> +++ b/patchwork/urls.py
> @@ -332,10 +332,15 @@ if settings.ENABLE_REST_API:
>
>      api_1_1_patterns = [
>          path(
> -            'patches/<pk>/comments/',
> +            'patches/<patch_id>/comments/',
>              api_comment_views.PatchCommentList.as_view(),
>              name='api-patch-comment-list',
>          ),
> +        path(
> +            'patches/<patch_id>/comments/<comment_id>/',
> +            api_comment_views.PatchCommentDetail.as_view(),
> +            name='api-patch-comment-detail',
> +        ),
>          path(
>              'covers/<pk>/comments/',
>              api_comment_views.CoverCommentList.as_view(),
> --
> 2.32.0.432.gabb21c7263-goog
>
diff mbox series

Patch

diff --git a/patchwork/api/base.py b/patchwork/api/base.py
index 89a4311..856fbd3 100644
--- a/patchwork/api/base.py
+++ b/patchwork/api/base.py
@@ -3,6 +3,7 @@ 
 #
 # SPDX-License-Identifier: GPL-2.0-or-later
 
+import rest_framework
 
 from django.conf import settings
 from django.shortcuts import get_object_or_404
@@ -15,6 +16,24 @@  from rest_framework.serializers import HyperlinkedModelSerializer
 from patchwork.api import utils
 
 
+DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
+
+
+if DRF_VERSION > (3, 11):
+    class CurrentPatchDefault(object):
+        requires_context = True
+
+        def __call__(self, serializer_field):
+            return serializer_field.context['request'].patch
+else:
+    class CurrentPatchDefault(object):
+        def set_context(self, serializer_field):
+            self.patch = serializer_field.context['request'].patch
+
+        def __call__(self):
+            return self.patch
+
+
 class LinkHeaderPagination(PageNumberPagination):
     """Provide pagination based on rfc5988.
 
@@ -44,7 +63,10 @@  class LinkHeaderPagination(PageNumberPagination):
 
 
 class PatchworkPermission(permissions.BasePermission):
-    """This permission works for Project and Patch model objects"""
+    """
+    This permission works for Project, Patch, and PatchComment
+    model objects
+    """
     def has_object_permission(self, request, view, obj):
         # read only for everyone
         if request.method in permissions.SAFE_METHODS:
diff --git a/patchwork/api/check.py b/patchwork/api/check.py
index a6bf5f8..fde6b61 100644
--- a/patchwork/api/check.py
+++ b/patchwork/api/check.py
@@ -6,7 +6,7 @@ 
 from django.http import Http404
 from django.http.request import QueryDict
 from django.shortcuts import get_object_or_404
-import rest_framework
+
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.generics import ListCreateAPIView
 from rest_framework.generics import RetrieveAPIView
@@ -17,30 +17,13 @@  from rest_framework.serializers import ValidationError
 
 from patchwork.api.base import CheckHyperlinkedIdentityField
 from patchwork.api.base import MultipleFieldLookupMixin
+from patchwork.api.base import CurrentPatchDefault
 from patchwork.api.embedded import UserSerializer
 from patchwork.api.filters import CheckFilterSet
 from patchwork.models import Check
 from patchwork.models import Patch
 
 
-DRF_VERSION = tuple(int(x) for x in rest_framework.__version__.split('.'))
-
-
-if DRF_VERSION > (3, 11):
-    class CurrentPatchDefault(object):
-        requires_context = True
-
-        def __call__(self, serializer_field):
-            return serializer_field.context['request'].patch
-else:
-    class CurrentPatchDefault(object):
-        def set_context(self, serializer_field):
-            self.patch = serializer_field.context['request'].patch
-
-        def __call__(self):
-            return self.patch
-
-
 class CheckSerializer(HyperlinkedModelSerializer):
 
     url = CheckHyperlinkedIdentityField('api-check-detail')
diff --git a/patchwork/api/comment.py b/patchwork/api/comment.py
index 43b26c6..7f1e401 100644
--- a/patchwork/api/comment.py
+++ b/patchwork/api/comment.py
@@ -5,12 +5,17 @@ 
 
 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 RetrieveUpdateDestroyAPIView
+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
@@ -55,6 +60,9 @@  class BaseCommentListSerializer(BaseHyperlinkedModelSerializer):
             '1.1': ('web_url', ),
             '1.2': ('list_archive_url',),
         }
+        extra_kwargs = {
+            'url': {'view_name': 'api-patch-comment-detail'},
+        }
 
 
 class CoverCommentListSerializer(BaseCommentListSerializer):
@@ -66,15 +74,47 @@  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 = fields[:-1]  # able to write to addressed field
+        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"""
 
@@ -94,20 +134,24 @@  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 = 'pk'
-
-    def get_queryset(self):
-        if not Patch.objects.filter(pk=self.kwargs['pk']).exists():
-            raise Http404
-
-        return PatchComment.objects.filter(
-            patch=self.kwargs['pk']
-        ).select_related('submitter')
+    lookup_url_kwarg = 'patch_id'
+
+
+class PatchCommentDetail(PatchCommentMixin, MultipleFieldLookupMixin,
+                         RetrieveUpdateDestroyAPIView):
+    """
+    get:
+    Show a patch comment.
+    patch:
+    Update a patch comment.
+    put:
+    Update a patch comment.
+    """
+    lookup_url_kwargs = ('patch_id', 'comment_id')
+    lookup_fields = ('patch_id', 'id')
diff --git a/patchwork/migrations/0045_patchcomment_addressed.py b/patchwork/migrations/0045_patchcomment_addressed.py
new file mode 100644
index 0000000..92e6c4e
--- /dev/null
+++ b/patchwork/migrations/0045_patchcomment_addressed.py
@@ -0,0 +1,18 @@ 
+# Generated by Django 3.1.12 on 2021-07-16 04:12
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0044_add_project_linkname_validation'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='patchcomment',
+            name='addressed',
+            field=models.BooleanField(default=False),
+        ),
+    ]
diff --git a/patchwork/models.py b/patchwork/models.py
index 00273da..0a77b23 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -693,6 +693,7 @@  class PatchComment(EmailMixin, models.Model):
         related_query_name='comment',
         on_delete=models.CASCADE,
     )
+    addressed = models.BooleanField(default=False)
 
     @property
     def list_archive_url(self):
@@ -718,7 +719,7 @@  class PatchComment(EmailMixin, models.Model):
         self.patch.refresh_tag_counts()
 
     def is_editable(self, user):
-        return False
+        return self.patch.is_editable(user)
 
     class Meta:
         ordering = ['date']
diff --git a/patchwork/urls.py b/patchwork/urls.py
index 6ac9b81..81db982 100644
--- a/patchwork/urls.py
+++ b/patchwork/urls.py
@@ -332,10 +332,15 @@  if settings.ENABLE_REST_API:
 
     api_1_1_patterns = [
         path(
-            'patches/<pk>/comments/',
+            'patches/<patch_id>/comments/',
             api_comment_views.PatchCommentList.as_view(),
             name='api-patch-comment-list',
         ),
+        path(
+            'patches/<patch_id>/comments/<comment_id>/',
+            api_comment_views.PatchCommentDetail.as_view(),
+            name='api-patch-comment-detail',
+        ),
         path(
             'covers/<pk>/comments/',
             api_comment_views.CoverCommentList.as_view(),