From patchwork Fri Sep 30 16:19:17 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stephen Finucane X-Patchwork-Id: 1684925 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ozlabs.org (client-ip=112.213.38.117; helo=lists.ozlabs.org; envelope-from=patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=oIldzOgG; dkim-atps=neutral Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4MfFng6kyBz1ypH for ; Sat, 1 Oct 2022 02:20:23 +1000 (AEST) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4MfFng6Z44z3c6l for ; Sat, 1 Oct 2022 02:20:23 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=oIldzOgG; dkim-atps=neutral X-Original-To: patchwork@lists.ozlabs.org Delivered-To: patchwork@lists.ozlabs.org Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=that.guru (client-ip=136.175.108.156; helo=mail-108-mta156.mxroute.com; envelope-from=stephen@that.guru; receiver=) Authentication-Results: lists.ozlabs.org; dkim=fail reason="key not found in DNS" header.d=that.guru header.i=@that.guru header.a=rsa-sha256 header.s=x header.b=oIldzOgG; dkim-atps=neutral Received: from mail-108-mta156.mxroute.com (mail-108-mta156.mxroute.com [136.175.108.156]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4MfFmy3s3rz3c8h for ; Sat, 1 Oct 2022 02:19:45 +1000 (AEST) Received: from mail-111-mta2.mxroute.com ([136.175.111.2] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta156.mxroute.com (ZoneMTA) with ESMTPSA id 1838f3220550002b7a.001 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Fri, 30 Sep 2022 16:19:30 +0000 X-Zone-Loop: 4047a2d7ca562b85bd9d5d043ad84c6c3b3854bbd4d5 X-Originating-IP: [136.175.111.2] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=that.guru; s=x; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=zab8lzcGTRqlWNdXHUwwd2AKiCD4Vr8cYoS5HiCfsOQ=; b=oIldzOgGDRpxKjSboKizxdAzpy xKLk5TBgnlHx4Bf3y6JpTA0HprmJ8C0fq/qpHHYDBJArE1Fok8SzszYCMT1aWTIu8xlUk85Qg8HCL qY7bWoTUa3A6Sg/VRn7Fy844lKIQQcTxMpySGHRTlIR4+SZqh7B+FvwJNorl/v0ICgH++2FYIqNCR At8rAnktfuW4VaWecUuRHC960ke++dBSD0/gIYKe8bS2tgqmF2W/ZGi5tWhBLedPKNegoNtPH2/S4 c3XV+fPwvZENuVQnytyXSGHRNqgE+ASUtCXqJp1v4jovZVU7DOt/jFDCPgyupUl564hpYuVTcY1ub jlsU7JOw==; From: Stephen Finucane To: patchwork@lists.ozlabs.org Subject: [PATCH 06/10] REST: Fix issues with comment-related events Date: Fri, 30 Sep 2022 17:19:17 +0100 Message-Id: <20220930161921.266633-6-stephen@that.guru> In-Reply-To: <20220930161921.266633-1-stephen@that.guru> References: <20220930161921.266633-1-stephen@that.guru> MIME-Version: 1.0 X-Authenticated-Id: stephen@that.guru X-BeenThere: patchwork@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Patchwork development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Carlos O'Donell , Siddhesh Poyarekar , DJ Delorie Errors-To: patchwork-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Patchwork" 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 Fixes: e3d8f7548 ("REST: Add 'patch-comment-created', 'cover-comment-created' events") Cc: Siddhesh Poyarekar Cc: DJ Delorie Cc: Carlos O'Donell --- patchwork/api/base.py | 34 +++++++++++++++++++++++++++++++ patchwork/api/embedded.py | 10 +++++++-- patchwork/api/event.py | 25 ++++++++++++++++++++--- patchwork/tests/api/test_event.py | 30 +++++++++++++++++---------- 4 files changed, 83 insertions(+), 16 deletions(-) diff --git patchwork/api/base.py patchwork/api/base.py index 6268f67d..3ed4182c 100644 --- patchwork/api/base.py +++ patchwork/api/base.py @@ -139,6 +139,40 @@ class CheckHyperlinkedIdentityField(HyperlinkedIdentityField): ) +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( diff --git patchwork/api/embedded.py patchwork/api/embedded.py index c41511fe..485ed6f7 100644 --- patchwork/api/embedded.py +++ patchwork/api/embedded.py @@ -17,6 +17,8 @@ from rest_framework.serializers import SerializerMethodField 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 CoverSerializer(SerializedRelatedField): 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 CoverCommentSerializer(SerializedRelatedField): 'msgid', 'list_archive_url', 'date', - 'name', ) read_only_fields = fields versioned_fields = { @@ -177,6 +181,9 @@ class PatchSerializer(SerializedRelatedField): 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 PatchCommentSerializer(SerializedRelatedField): 'msgid', 'list_archive_url', 'date', - 'name', ) read_only_fields = fields versioned_fields = { diff --git patchwork/api/event.py patchwork/api/event.py index c1b09ab9..6d08b6ee 100644 --- patchwork/api/event.py +++ patchwork/api/event.py @@ -19,6 +19,7 @@ from patchwork.api.embedded import ProjectSerializer 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 @@ class EventList(ListAPIView): '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 diff --git patchwork/tests/api/test_event.py patchwork/tests/api/test_event.py index 9708f96b..7ca09c2e 100644 --- patchwork/tests/api/test_event.py +++ patchwork/tests/api/test_event.py @@ -12,8 +12,10 @@ from patchwork.models import Event 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 @@ class TestEventAPI(APITestCase): # 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 @@ class TestEventAPI(APITestCase): 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 @@ class TestEventAPI(APITestCase): 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 @@ class TestEventAPI(APITestCase): 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 @@ class TestEventAPI(APITestCase): 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 @@ class TestEventAPI(APITestCase): 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 @@ class TestEventAPI(APITestCase): # 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 @@ class TestEventAPI(APITestCase): 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):