REST: Fix issues with comment-related events

When we introduced this functionality, we missed the fact that these
resources use nested-style URLs that need to be specially handled. Fix
this now.

Signed-off-by: Stephen Finucane <stephen@that.guru>
Fixes: e3d8f75 ("REST: Add 'patch-comment-created', 'cover-comment-created' events")
Cc: Siddhesh Poyarekar <sipoyare@redhat.com>
Cc: DJ Delorie <dj@redhat.com>
Cc: Carlos O'Donell <carlos@redhat.com>
(cherry picked from commit d05a2ff)

Author: stephenfin

patchwork/api/base.py

@@ -139,6 +139,40 @@ def get_url(self, obj, view_name, request, format):
         )
 
 
+class CoverCommentHyperlinkedIdentityField(HyperlinkedIdentityField):
+    def get_url(self, obj, view_name, request, format):
+        # Unsaved objects will not yet have a valid URL.
+        if obj.pk is None:
+            return None
+
+        return self.reverse(
+            view_name,
+            kwargs={
+                'cover_id': obj.cover.id,
+                'comment_id': obj.id,
+            },
+            request=request,
+            format=format,
+        )
+
+
+class PatchCommentHyperlinkedIdentityField(HyperlinkedIdentityField):
+    def get_url(self, obj, view_name, request, format):
+        # Unsaved objects will not yet have a valid URL.
+        if obj.pk is None:
+            return None
+
+        return self.reverse(
+            view_name,
+            kwargs={
+                'patch_id': obj.patch.id,
+                'comment_id': obj.id,
+            },
+            request=request,
+            format=format,
+        )
+
+
 class BaseHyperlinkedModelSerializer(HyperlinkedModelSerializer):
     def to_representation(self, instance):
         data = super(BaseHyperlinkedModelSerializer, self).to_representation(

patchwork/api/embedded.py

@@ -17,6 +17,8 @@
 
 from patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import CheckHyperlinkedIdentityField
+from patchwork.api.base import CoverCommentHyperlinkedIdentityField
+from patchwork.api.base import PatchCommentHyperlinkedIdentityField
 from patchwork import models
 
 
@@ -127,6 +129,9 @@ class Meta:
 
 class CoverCommentSerializer(SerializedRelatedField):
     class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        url = CoverCommentHyperlinkedIdentityField('api-cover-comment-detail')
+
         class Meta:
             model = models.CoverComment
             fields = (
@@ -136,7 +141,6 @@ class Meta:
                 'msgid',
                 'list_archive_url',
                 'date',
-                'name',
             )
             read_only_fields = fields
             versioned_fields = {
@@ -177,6 +181,9 @@ class Meta:
 
 class PatchCommentSerializer(SerializedRelatedField):
     class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
+
+        url = PatchCommentHyperlinkedIdentityField('api-patch-comment-detail')
+
         class Meta:
             model = models.PatchComment
             fields = (
@@ -186,7 +193,6 @@ class Meta:
                 'msgid',
                 'list_archive_url',
                 'date',
-                'name',
             )
             read_only_fields = fields
             versioned_fields = {

patchwork/api/event.py

@@ -19,6 +19,7 @@
 from patchwork.api.embedded import SeriesSerializer
 from patchwork.api.embedded import UserSerializer
 from patchwork.api.filters import EventFilterSet
+from patchwork.api import utils
 from patchwork.models import Event
 
 
@@ -140,7 +141,7 @@ class EventList(ListAPIView):
     ordering = '-date'
 
     def get_queryset(self):
-        return Event.objects.all().prefetch_related(
+        events = Event.objects.all().prefetch_related(
             'project',
             'patch__project',
             'series__project',
@@ -150,6 +151,24 @@ def get_queryset(self):
             'previous_delegate',
             'current_delegate',
             'created_check',
-            'cover_comment',
-            'patch_comment',
         )
+        # NOTE(stephenfin): We need to exclude comment-related events because
+        # until API v1.3, we didn't have an comment detail API to point to.
+        # This goes against our pledge to version events in the docs but must
+        # be done.
+        # TODO(stephenfin): Make this more generic.
+        if utils.has_version(self.request, '1.3'):
+            events = events.prefetch_related(
+                'cover_comment',
+                'cover_comment__cover__project',
+                'patch_comment',
+                'patch_comment__patch__project',
+            )
+        else:
+            events = events.exclude(
+                category__in=[
+                    Event.CATEGORY_COVER_COMMENT_CREATED,
+                    Event.CATEGORY_PATCH_COMMENT_CREATED,
+                ]
+            )
+        return events

patchwork/tests/api/test_event.py

@@ -12,8 +12,10 @@
 from patchwork.tests.api import utils
 from patchwork.tests.utils import create_check
 from patchwork.tests.utils import create_cover
+from patchwork.tests.utils import create_cover_comment
 from patchwork.tests.utils import create_maintainer
 from patchwork.tests.utils import create_patch
+from patchwork.tests.utils import create_patch_comment
 from patchwork.tests.utils import create_series
 from patchwork.tests.utils import create_state
 
@@ -70,7 +72,7 @@ def _create_events(self):
         # patch-created, patch-completed, series-completed
         patch = create_patch(series=series)
         # cover-created
-        create_cover(series=series)
+        cover = create_cover(series=series)
         # check-created
         create_check(patch=patch)
         # patch-delegated, patch-state-changed
@@ -81,6 +83,9 @@ def _create_events(self):
         patch.state = state
         self.assertTrue(patch.is_editable(actor))
         patch.save()
+        # patch-cover-created, cover-comment-created
+        create_patch_comment(patch=patch, submitter=patch.submitter)
+        create_cover_comment(cover=cover, submitter=cover.submitter)
 
         return Event.objects.all()
 
@@ -91,7 +96,9 @@ def test_list(self):
 
         resp = self.client.get(self.api_url())
         self.assertEqual(status.HTTP_200_OK, resp.status_code)
-        self.assertEqual(8, len(resp.data), [x['category'] for x in resp.data])
+        self.assertEqual(
+            10, len(resp.data), [x['category'] for x in resp.data]
+        )
         for event_rsp in resp.data:
             event_obj = events.get(category=event_rsp['category'])
             self.assertSerialized(event_obj, event_rsp)
@@ -104,7 +111,7 @@ def test_list_filter_project(self):
 
         resp = self.client.get(self.api_url(), {'project': project.pk})
         # All but one event belongs to the same project
-        self.assertEqual(8, len(resp.data))
+        self.assertEqual(10, len(resp.data))
 
         resp = self.client.get(self.api_url(), {'project': 'invalidproject'})
         self.assertEqual(0, len(resp.data))
@@ -132,9 +139,9 @@ def test_list_filter_patch(self):
 
         patch = events.get(category='patch-created').patch
         resp = self.client.get(self.api_url(), {'patch': patch.pk})
-        # There should be five - patch-created, patch-completed, check-created,
-        # patch-state-changed and patch-delegated
-        self.assertEqual(5, len(resp.data))
+        # There should be six - patch-created, patch-completed, check-created,
+        # patch-state-changed, patch-delegated and patch-comment-created
+        self.assertEqual(6, len(resp.data))
 
         resp = self.client.get(self.api_url(), {'patch': 999999})
         self.assertEqual(0, len(resp.data))
@@ -145,8 +152,8 @@ def test_list_filter_cover(self):
 
         cover = events.get(category='cover-created').cover
         resp = self.client.get(self.api_url(), {'cover': cover.pk})
-        # There should only be one - cover-created
-        self.assertEqual(1, len(resp.data))
+        # There should be two - cover-created and cover-comment-created
+        self.assertEqual(2, len(resp.data))
 
         resp = self.client.get(self.api_url(), {'cover': 999999})
         self.assertEqual(0, len(resp.data))
@@ -170,7 +177,7 @@ def test_list_filter_actor(self):
 
         # The final two events (patch-delegated, patch-state-changed)
         # have an actor set
-        actor = events[0].actor
+        actor = events.get(category='patch-delegated').actor
         resp = self.client.get(self.api_url(), {'actor': actor.pk})
         self.assertEqual(2, len(resp.data))
 
@@ -185,14 +192,15 @@ def test_list_filter_actor_version_1_1(self):
         resp = self.client.get(
             self.api_url(version='1.1'), {'actor': 'foo-bar'}
         )
-        self.assertEqual(len(events), len(resp.data))
+        # we don't see the two comment-related fields
+        self.assertEqual(len(events) - 2, len(resp.data))
 
     def test_list_bug_335(self):
         """Ensure we retrieve the embedded series project once."""
         for _ in range(3):
             self._create_events()
 
-        with self.assertNumQueries(27):
+        with self.assertNumQueries(33):
             self.client.get(self.api_url())
 
     def test_order_by_date_default(self):

releasenotes/notes/issue-499-1ee87a1e9d502dc6.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fix an issue whereby comment-based events would cause a HTTP 500 error
+    for the events API (``/api/events``).