launchpad-reviewers team mailing list archive

[Vaishnavi Asawale <mp+494351@xxxxxxxxxxxxxxxxxx>]

Vaishnavi Asawale has proposed merging ~vaishnavi-asawale/launchpad:configure-archive-webhooks into launchpad:master.

Commit message:
Allow the creation of webhooks for an archive

This MP is part of the functionality to allow webhooks to be
configured on an archive to provide information related to source
uploads and builds.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~vaishnavi-asawale/launchpad/+git/launchpad/+merge/494351
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~vaishnavi-asawale/launchpad:configure-archive-webhooks into launchpad:master.

diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index 1515d2f..d9c6f67 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -67,6 +67,7 @@ WEBHOOK_EVENT_TYPES = {
     "oci-recipe:build:0.1": "OCI recipe build",
     "snap:build:0.1": "Snap build",
     "craft-recipe:build:0.1": "Craft recipe build",
+    "archive:source-package-upload:0.1": "Source package upload",
 }
 
 
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index 397c818..89b7843 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -31,7 +31,6 @@ from zope.security.proxy import removeSecurityProxy
 
 from lp.app import versioninfo
 from lp.registry.interfaces.role import IPersonRoles
-from lp.registry.model.person import Person
 from lp.services.config import config
 from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
@@ -117,6 +116,9 @@ class Webhook(StormBase):
         source_package_name_id, "SourcePackageName.id"
     )
 
+    archive_id = Int(name="archive")
+    archive = Reference(archive_id, "Archive.id")
+
     registrant_id = Int(name="registrant", allow_none=False)
     registrant = Reference(registrant_id, "Person.id")
     date_created = DateTime(tzinfo=timezone.utc, allow_none=False)
@@ -155,6 +157,8 @@ class Webhook(StormBase):
                 )
             else:
                 return self.distribution
+        elif self.archive is not None:
+            return self.archive
         else:
             raise AssertionError("No target.")
 
@@ -240,6 +244,7 @@ class WebhookSet:
         )
         from lp.registry.interfaces.product import IProduct
         from lp.snappy.interfaces.snap import ISnap
+        from lp.soyuz.interfaces.archive import IArchive
         from lp.soyuz.interfaces.livefs import ILiveFS
 
         hook = Webhook()
@@ -264,6 +269,8 @@ class WebhookSet:
         elif IDistributionSourcePackage.providedBy(target):
             hook.distribution = target.distribution
             hook.source_package_name = target.sourcepackagename
+        elif IArchive.providedBy(target):
+            hook.archive = target
         else:
             raise AssertionError("Unsupported target: %r" % (target,))
         hook.registrant = registrant
@@ -298,6 +305,7 @@ class WebhookSet:
         )
         from lp.registry.interfaces.product import IProduct
         from lp.snappy.interfaces.snap import ISnap
+        from lp.soyuz.interfaces.archive import IArchive
         from lp.soyuz.interfaces.livefs import ILiveFS
 
         if IGitRepository.providedBy(target):
@@ -326,6 +334,8 @@ class WebhookSet:
                 Webhook.distribution == target.distribution,
                 Webhook.source_package_name == target.sourcepackagename,
             )
+        elif IArchive.providedBy(target):
+            target_filter = Webhook.archive == target
         else:
             raise AssertionError("Unsupported target: %r" % (target,))
         return (
@@ -401,6 +411,8 @@ class WebhookSet:
 class WebhookTargetMixin:
     @property
     def webhooks(self):
+        from lp.registry.model.person import Person
+
         def preload_registrants(rows):
             load_related(Person, rows, ["registrant_id"])
 
diff --git a/lib/lp/services/webhooks/tests/test_job.py b/lib/lp/services/webhooks/tests/test_job.py
index 2fee4d2..e3c1e74 100644
--- a/lib/lp/services/webhooks/tests/test_job.py
+++ b/lib/lp/services/webhooks/tests/test_job.py
@@ -438,6 +438,17 @@ class TestWebhookDeliveryJob(TestCaseWithFactory):
             repr(job),
         )
 
+    def test_archive__repr__(self):
+        # `WebhookDeliveryJob` objects for archives have an informative
+        # __repr__.
+        archive = self.factory.makeArchive(name="test-archive")
+        hook = self.factory.makeWebhook(target=archive)
+        job = WebhookDeliveryJob.create(hook, "test", payload={"foo": "bar"})
+        self.assertEqual(
+            "<WebhookDeliveryJob for webhook %d on %r>" % (hook.id, archive),
+            repr(job),
+        )
+
     def test_short_lease_and_timeout(self):
         # Webhook jobs have a request timeout of 30 seconds, a celery
         # timeout of 45 seconds, and a lease of 60 seconds, to give
diff --git a/lib/lp/services/webhooks/tests/test_model.py b/lib/lp/services/webhooks/tests/test_model.py
index 8e9ec5a..1fad0ba 100644
--- a/lib/lp/services/webhooks/tests/test_model.py
+++ b/lib/lp/services/webhooks/tests/test_model.py
@@ -562,6 +562,16 @@ class TestWebhookSetCharmRecipe(TestWebhookSetBase, TestCaseWithFactory):
             )
 
 
+class TestWebhookSetArchive(TestWebhookSetBase, TestCaseWithFactory):
+    event_type = "archive:source-package-upload:0.1"
+
+    def makeTarget(self, owner=None, **kwargs):
+        if owner is None:
+            owner = self.factory.makePerson()
+
+        return self.factory.makeArchive(name="test-archive", **kwargs)
+
+
 class TestWebhookSetCraftRecipe(TestWebhookSetBase, TestCaseWithFactory):
     event_type = "craft-recipe:build:0.1"
 
diff --git a/lib/lp/services/webhooks/tests/test_webservice.py b/lib/lp/services/webhooks/tests/test_webservice.py
index 18c548d..b4ad6cb 100644
--- a/lib/lp/services/webhooks/tests/test_webservice.py
+++ b/lib/lp/services/webhooks/tests/test_webservice.py
@@ -574,6 +574,14 @@ class TestWebhookTargetCharmRecipe(TestWebhookTargetBase, TestCaseWithFactory):
             return self.factory.makeCharmRecipe(registrant=owner, owner=owner)
 
 
+class TestWebhookTargetArchive(TestWebhookTargetBase, TestCaseWithFactory):
+    event_type = "archive:source-package-upload:0.1"
+
+    def makeTarget(self):
+        owner = self.factory.makePerson()
+        return self.factory.makeArchive(name="test-archive", owner=owner)
+
+
 class TestWebhookTargetProduct(TestWebhookTargetBase, TestCaseWithFactory):
     event_type = "bug:0.1"
 
diff --git a/lib/lp/soyuz/browser/archive.py b/lib/lp/soyuz/browser/archive.py
index 7c701f4..d592d15 100644
--- a/lib/lp/soyuz/browser/archive.py
+++ b/lib/lp/soyuz/browser/archive.py
@@ -99,6 +99,7 @@ from lp.services.webapp.escaping import structured
 from lp.services.webapp.interfaces import ICanonicalUrlData, IStructuredString
 from lp.services.webapp.menu import NavigationMenu
 from lp.services.webapp.publisher import RedirectionView
+from lp.services.webhooks.browser import WebhookTargetNavigationMixin
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.soyuz.adapters.archivedependencies import (
     default_component_dependency_name,
@@ -216,7 +217,9 @@ class PPAURL:
         )
 
 
-class ArchiveNavigation(Navigation, FileNavigationMixin):
+class ArchiveNavigation(
+    WebhookTargetNavigationMixin, Navigation, FileNavigationMixin
+):
     """Navigation methods for IArchive."""
 
     usedfor = IArchive
diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py
index 4d6b03e..656ea34 100644
--- a/lib/lp/soyuz/interfaces/archive.py
+++ b/lib/lp/soyuz/interfaces/archive.py
@@ -110,6 +110,7 @@ from lp.services.fields import (
     PublicPersonChoice,
     StrippedTextLine,
 )
+from lp.services.webhooks.interfaces import IWebhookTarget
 from lp.soyuz.enums import (
     ArchivePublishingMethod,
     ArchivePurpose,
@@ -2285,7 +2286,7 @@ class IArchiveAppend(Interface):
         """
 
 
-class IArchiveEdit(Interface):
+class IArchiveEdit(IWebhookTarget):
     """Archive interface for operations restricted by edit privilege."""
 
     @operation_parameters(
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 11c9b17..78c4d54 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -108,6 +108,8 @@ from lp.services.tokens import create_token
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.url import urlappend
+from lp.services.webhooks.interfaces import IWebhookSet
+from lp.services.webhooks.model import WebhookTargetMixin
 from lp.soyuz.adapters.archivedependencies import expand_dependencies
 from lp.soyuz.adapters.packagelocation import PackageLocation
 from lp.soyuz.enums import (
@@ -219,7 +221,7 @@ def storm_validate_external_dependencies(archive, attr, value):
 
 
 @implementer(IArchive, IHasOwner, IHasBuildRecords)
-class Archive(StormBase):
+class Archive(StormBase, WebhookTargetMixin):
     __storm_table__ = "Archive"
     __storm_order__ = "id"
 
@@ -228,6 +230,10 @@ class Archive(StormBase):
     owner_id = Int(name="owner", validator=validate_person, allow_none=False)
     owner = Reference(owner_id, "Person.id")
 
+    @property
+    def valid_webhook_event_types(self):
+        return ["archive:source-package-upload:0.1"]
+
     def _validate_archive_name(self, attr, value):
         """Only allow renaming of COPY archives.
 
@@ -2998,6 +3004,7 @@ class Archive(StormBase):
         self.status = ArchiveStatus.DELETING
         if self.enabled:
             self.disable()
+        getUtility(IWebhookSet).delete(self.webhooks)
 
     def getFilesAndSha1s(self, source_files):
         """See `IArchive`."""
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index 1fd3dd6..f167906 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -14,6 +14,7 @@ from urllib.parse import urlsplit
 import responses
 import transaction
 from aptsources.sourceslist import SourceEntry
+from storm.exceptions import LostObjectError
 from storm.store import Store
 from testtools.matchers import (
     AfterPreprocessing,
@@ -7340,3 +7341,17 @@ class TestArchiveMetadataOverrides(TestCaseWithFactory):
             Unauthorized,
             lambda: primary_archive.setMetadataOverrides(overrides),
         )
+
+
+class TestArchiveWebhooks(TestCaseWithFactory):
+    layer = LaunchpadZopelessLayer
+
+    def test_related_webhooks_deleted(self):
+        owner = self.factory.makePerson()
+        archive = self.factory.makeArchive(name="test-archive", owner=owner)
+        webhook = self.factory.makeWebhook(target=archive)
+        with person_logged_in(archive.owner):
+            webhook.ping()
+            archive.delete(archive.owner)
+            transaction.commit()
+            self.assertRaises(LostObjectError, getattr, webhook, "target")