Merge ~ines-almeida/launchpad:add-bug-webhooks/update-webhook-model into launchpad:master

[Ines Almeida]

Add new fields to webhook model

Update and add new unit tests to go with the change

This MP follows up this DB patch: https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/442541

[Colin Watson (cjwatson)]

Looks good! Just one small question.

review: Approve

diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index 114d953..bafd604 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -19,7 +19,7 @@ import psutil
 import transaction
 from lazr.delegates import delegate_to
 from lazr.enum import DBEnumeratedType, DBItem
-from storm.expr import Desc
+from storm.expr import And, Desc
 from storm.properties import JSON, Bool, DateTime, Int, Unicode
 from storm.references import Reference
 from storm.store import Store
@@ -92,6 +92,17 @@ class Webhook(StormBase):
     charm_recipe_id = Int(name="charm_recipe")
     charm_recipe = Reference(charm_recipe_id, "CharmRecipe.id")
 
+    project_id = Int(name="project")
+    project = Reference(project_id, "Product.id")
+
+    distribution_id = Int(name="distribution")
+    distribution = Reference(distribution_id, "Distribution.id")
+
+    source_package_name_id = Int(name="source_package_name")
+    source_package_name = Reference(
+        source_package_name_id, "SourcePackageName.id"
+    )
+
     registrant_id = Int(name="registrant", allow_none=False)
     registrant = Reference(registrant_id, "Person.id")
     date_created = DateTime(tzinfo=timezone.utc, allow_none=False)
@@ -117,6 +128,15 @@ class Webhook(StormBase):
             return self.oci_recipe
         elif self.charm_recipe is not None:
             return self.charm_recipe
+        elif self.project is not None:
+            return self.project
+        elif self.distribution is not None:
+            if self.source_package_name is not None:
+                return self.distribution.getSourcePackage(
+                    self.source_package_name
+                )
+            else:
+                return self.distribution
         else:
             raise AssertionError("No target.")
 
@@ -178,6 +198,11 @@ class WebhookSet:
         from lp.code.interfaces.branch import IBranch
         from lp.code.interfaces.gitrepository import IGitRepository
         from lp.oci.interfaces.ocirecipe import IOCIRecipe
+        from lp.registry.interfaces.distribution import IDistribution
+        from lp.registry.interfaces.distributionsourcepackage import (
+            IDistributionSourcePackage,
+        )
+        from lp.registry.interfaces.product import IProduct
         from lp.snappy.interfaces.snap import ISnap
         from lp.soyuz.interfaces.livefs import ILiveFS
 
@@ -194,6 +219,14 @@ class WebhookSet:
             hook.oci_recipe = target
         elif ICharmRecipe.providedBy(target):
             hook.charm_recipe = target
+        elif IProduct.providedBy(target):
+            hook.project = target
+        elif IDistribution.providedBy(target):
+            hook.distribution = target
+        elif IDistributionSourcePackage.providedBy(target):
+            hook.distribution = target.distribution
+            hook.source_package_name = target.sourcepackagename
+
         else:
             raise AssertionError("Unsupported target: %r" % (target,))
         hook.registrant = registrant
@@ -220,6 +253,11 @@ class WebhookSet:
         from lp.code.interfaces.branch import IBranch
         from lp.code.interfaces.gitrepository import IGitRepository
         from lp.oci.interfaces.ocirecipe import IOCIRecipe
+        from lp.registry.interfaces.distribution import IDistribution
+        from lp.registry.interfaces.distributionsourcepackage import (
+            IDistributionSourcePackage,
+        )
+        from lp.registry.interfaces.product import IProduct
         from lp.snappy.interfaces.snap import ISnap
         from lp.soyuz.interfaces.livefs import ILiveFS
 
@@ -235,6 +273,18 @@ class WebhookSet:
             target_filter = Webhook.oci_recipe == target
         elif ICharmRecipe.providedBy(target):
             target_filter = Webhook.charm_recipe == target
+        elif IProduct.providedBy(target):
+            target_filter = Webhook.project == target
+        elif IDistribution.providedBy(target):
+            target_filter = And(
+                Webhook.distribution == target,
+                Webhook.source_package_name == None,
+            )
+        elif IDistributionSourcePackage.providedBy(target):
+            target_filter = And(
+                Webhook.distribution == target.distribution,
+                Webhook.source_package_name == target.sourcepackagename,
+            )
         else:
             raise AssertionError("Unsupported target: %r" % (target,))
         return (
diff --git a/lib/lp/services/webhooks/tests/test_model.py b/lib/lp/services/webhooks/tests/test_model.py
index 85d2c40..47031b4 100644
--- a/lib/lp/services/webhooks/tests/test_model.py
+++ b/lib/lp/services/webhooks/tests/test_model.py
@@ -77,10 +77,13 @@ class TestWebhookPermissions(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
+    def get_target_owner(self, target):
+        return target.owner
+
     def test_target_owner_can_view(self):
         target = self.factory.makeGitRepository()
         webhook = self.factory.makeWebhook(target=target)
-        with person_logged_in(target.owner):
+        with person_logged_in(self.get_target_owner(target)):
             self.assertTrue(check_permission("launchpad.View", webhook))
 
     def test_random_cannot_view(self):
@@ -140,9 +143,12 @@ class TestWebhookSetBase:
 
     layer = DatabaseFunctionalLayer
 
+    def get_target_owner(self, target):
+        return target.owner
+
     def test_new(self):
         target = self.makeTarget()
-        login_person(target.owner)
+        login_person(self.get_target_owner(target))
         person = self.factory.makePerson()
         hook = getUtility(IWebhookSet).new(
             target,
@@ -178,7 +184,7 @@ class TestWebhookSetBase:
                 self.factory.makeWebhook(
                     target, "http://path/%s/%d" % (name, i)
                 )
-        with person_logged_in(target1.owner):
+        with person_logged_in(self.get_target_owner(target1)):
             self.assertContentEqual(
                 [
                     "http://path/one/0",
@@ -190,7 +196,7 @@ class TestWebhookSetBase:
                     for hook in getUtility(IWebhookSet).findByTarget(target1)
                 ],
             )
-        with person_logged_in(target2.owner):
+        with person_logged_in(self.get_target_owner(target2)):
             self.assertContentEqual(
                 [
                     "http://path/two/0",
@@ -205,7 +211,7 @@ class TestWebhookSetBase:
 
     def test_delete(self):
         target = self.makeTarget()
-        login_person(target.owner)
+        login_person(self.get_target_owner(target))
         hooks = []
         for i in range(3):
             hook = self.factory.makeWebhook(target, "http://path/to/%d" % i)
@@ -237,7 +243,9 @@ class TestWebhookSetBase:
 
     def test__checkVisibility_public_artifact(self):
         target = self.makeTarget()
-        self.assertTrue(WebhookSet._checkVisibility(target, target.owner))
+        self.assertTrue(
+            WebhookSet._checkVisibility(target, self.get_target_owner(target))
+        )
 
     def test_trigger(self):
         owner = self.factory.makePerson()
@@ -520,3 +528,64 @@ class TestWebhookSetCharmRecipe(TestWebhookSetBase, TestCaseWithFactory):
             return self.factory.makeCharmRecipe(
                 registrant=owner, owner=owner, **kwargs
             )
+
+
+class TestWebhookSetBugBase(TestWebhookSetBase):
+
+    event_type = "bug:0.1"
+
+    def test__checkVisibility_private_artifact(self):
+        owner = self.factory.makePerson()
+        target = self.makeTarget(
+            owner=owner, information_type=InformationType.PROPRIETARY
+        )
+        self.assertTrue(WebhookSet._checkVisibility(target, owner))
+
+    def test_webhook_trigger_private_target(self):
+        owner = self.factory.makePerson()
+        target = self.makeTarget(
+            owner=owner,
+            information_type=InformationType.PROPRIETARY,
+        )
+        hook = self.factory.makeWebhook(
+            target=target, event_types=[self.event_type]
+        )
+
+        with admin_logged_in():
+            self.assertThat(list(hook.deliveries), HasLength(0))
+
+        getUtility(IWebhookSet).trigger(
+            target, self.event_type, {"some": "payload"}
+        )
+        with admin_logged_in():
+            self.assertThat(list(hook.deliveries), HasLength(1))
+            delivery = hook.deliveries.one()
+            self.assertEqual(delivery.payload, {"some": "payload"})
+
+
+class TestWebhookSetProductBugUpdate(
+    TestWebhookSetBugBase, TestCaseWithFactory
+):
+    def makeTarget(self, **kwargs):
+        return self.factory.makeProduct(**kwargs)
+
+
+class TestWebhookSetDistributionBugUpdate(
+    TestWebhookSetBugBase, TestCaseWithFactory
+):
+    def makeTarget(self, **kwargs):
+        return self.factory.makeDistribution(**kwargs)
+
+
+class TestWebhookSetDistributionSourcePackageBugUpdate(
+    TestWebhookSetBugBase, TestCaseWithFactory
+):
+    def get_target_owner(self, target):
+        return target.distribution.owner
+
+    def makeTarget(self, **kwargs):
+        with admin_logged_in():
+            distribution = self.factory.makeDistribution(**kwargs)
+            return self.factory.makeDistributionSourcePackage(
+                distribution=distribution
+            )