launchpad-reviewers team mailing list archive

[Ines Almeida <mp+445657@xxxxxxxxxxxxxxxxxx>]

Ines Almeida has proposed merging ~ines-almeida/launchpad:invert-bug-webhook-feature-flag into launchpad:master.

Commit message:
Invert bug webhooks feature flag to be enabled by default
    
For feature flags that we intend to keep enabled (and particularly for the unit tests to run with the newly added feature) this change is inverting this feature flag so that the feature is enabled by default. To disable the feature, one just needs to set the "bugs.webhooks.disabled" feature flag

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/445657

I haven't run the full test suite in this (I ran enough to feel confident to MP it), so we might discover a couple extra places that are missing permissions when all the full test suite runs after merge.

IMO, there might still be value in setting feature flags globally for testing, and I opened a ticket to potentially address that in the future (https://warthogs.atlassian.net/browse/LP-1272). For this particular case, I think this is reasonable.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:invert-bug-webhook-feature-flag into launchpad:master.

diff --git a/lib/lp/bugs/interfaces/bugtarget.py b/lib/lp/bugs/interfaces/bugtarget.py
index 540079f..3a090e6 100644
--- a/lib/lp/bugs/interfaces/bugtarget.py
+++ b/lib/lp/bugs/interfaces/bugtarget.py
@@ -15,7 +15,7 @@ __all__ = [
     "ISeriesBugTarget",
     "BUG_POLICY_ALLOWED_TYPES",
     "BUG_POLICY_DEFAULT_TYPES",
-    "BUG_WEBHOOKS_FEATURE_FLAG",
+    "DISABLE_BUG_WEBHOOKS_FEATURE_FLAG",
 ]
 
 
@@ -157,7 +157,7 @@ BUG_POLICY_DEFAULT_TYPES = {
 }
 
 
-BUG_WEBHOOKS_FEATURE_FLAG = "bugs.webhooks.enabled"
+DISABLE_BUG_WEBHOOKS_FEATURE_FLAG = "bugs.webhooks.disabled"
 
 
 @exported_as_webservice_entry(as_of="beta")
diff --git a/lib/lp/bugs/subscribers/webhooks.py b/lib/lp/bugs/subscribers/webhooks.py
index 4ea257f..8d1b56f 100644
--- a/lib/lp/bugs/subscribers/webhooks.py
+++ b/lib/lp/bugs/subscribers/webhooks.py
@@ -10,7 +10,7 @@ from zope.component import getUtility
 
 from lp.bugs.interfaces.bug import IBug
 from lp.bugs.interfaces.bugmessage import IBugMessage
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
+from lp.bugs.interfaces.bugtarget import DISABLE_BUG_WEBHOOKS_FEATURE_FLAG
 from lp.bugs.interfaces.bugtask import IBugTask
 from lp.bugs.subscribers.bugactivity import what_changed
 from lp.services.database.sqlbase import block_implicit_flushes
@@ -27,7 +27,7 @@ def _trigger_bugtask_webhook(
     previous_state: Union[IBug, IBugTask] = None,
 ):
     """Triggers 'bug' event for a specific bugtask"""
-    if not getFeatureFlag(BUG_WEBHOOKS_FEATURE_FLAG):
+    if not getFeatureFlag(DISABLE_BUG_WEBHOOKS_FEATURE_FLAG):
         return
 
     if IWebhookTarget.providedBy(bugtask.target):
@@ -38,7 +38,7 @@ def _trigger_bugtask_webhook(
 @block_implicit_flushes
 def _trigger_bug_comment_webhook(action: str, bug_comment: IBugMessage):
     """Triggers 'bug:comment' events for each bug target for that comment"""
-    if not getFeatureFlag(BUG_WEBHOOKS_FEATURE_FLAG):
+    if not getFeatureFlag(DISABLE_BUG_WEBHOOKS_FEATURE_FLAG):
         return
 
     bugtasks = bug_comment.bug.bugtasks
diff --git a/lib/lp/bugs/tests/test_subscribers.py b/lib/lp/bugs/tests/test_subscribers.py
index 7bddc92..938002d 100644
--- a/lib/lp/bugs/tests/test_subscribers.py
+++ b/lib/lp/bugs/tests/test_subscribers.py
@@ -9,7 +9,7 @@ from testtools.matchers import Equals, MatchesDict, MatchesStructure
 from zope.event import notify
 
 from lp.bugs.interfaces.bug import IBug
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
+from lp.bugs.interfaces.bugtarget import DISABLE_BUG_WEBHOOKS_FEATURE_FLAG
 from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.bugs.subscribers.bugactivity import what_changed
 from lp.services.features.testing import FeatureFixture
@@ -63,7 +63,7 @@ class TestBugWebhooksTriggered(TestCaseWithFactory):
         self.useFixture(
             FeatureFixture(
                 {
-                    BUG_WEBHOOKS_FEATURE_FLAG: "on",
+                    DISABLE_BUG_WEBHOOKS_FEATURE_FLAG: "on",
                 }
             )
         )
diff --git a/lib/lp/registry/browser/distribution.py b/lib/lp/registry/browser/distribution.py
index b990f6f..716da3d 100644
--- a/lib/lp/registry/browser/distribution.py
+++ b/lib/lp/registry/browser/distribution.py
@@ -83,7 +83,7 @@ from lp.bugs.browser.structuralsubscription import (
     StructuralSubscriptionTargetTraversalMixin,
     expose_structural_subscription_data_to_js,
 )
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
+from lp.bugs.interfaces.bugtarget import DISABLE_BUG_WEBHOOKS_FEATURE_FLAG
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.code.browser.vcslisting import TargetDefaultVCSNavigationMixin
 from lp.registry.browser import RegistryEditFormView, add_subscribe_link
@@ -440,7 +440,7 @@ class DistributionNavigationMenu(NavigationMenu, DistributionLinksMixin):
             "+webhooks",
             "Manage webhooks",
             icon="edit",
-            enabled=bool(getFeatureFlag(BUG_WEBHOOKS_FEATURE_FLAG)),
+            enabled=not (getFeatureFlag(DISABLE_BUG_WEBHOOKS_FEATURE_FLAG)),
         )
 
 
diff --git a/lib/lp/registry/browser/distributionsourcepackage.py b/lib/lp/registry/browser/distributionsourcepackage.py
index e0d960a..f7df149 100644
--- a/lib/lp/registry/browser/distributionsourcepackage.py
+++ b/lib/lp/registry/browser/distributionsourcepackage.py
@@ -42,7 +42,7 @@ from lp.bugs.browser.structuralsubscription import (
     StructuralSubscriptionTargetTraversalMixin,
     expose_structural_subscription_data_to_js,
 )
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
+from lp.bugs.interfaces.bugtarget import DISABLE_BUG_WEBHOOKS_FEATURE_FLAG
 from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
 from lp.code.browser.vcslisting import TargetDefaultVCSNavigationMixin
@@ -179,7 +179,7 @@ class DistributionSourcePackageLinksMixin:
             "+webhooks",
             "Manage webhooks",
             icon="edit",
-            enabled=bool(getFeatureFlag(BUG_WEBHOOKS_FEATURE_FLAG)),
+            enabled=not (getFeatureFlag(DISABLE_BUG_WEBHOOKS_FEATURE_FLAG)),
         )
 
 
diff --git a/lib/lp/registry/browser/product.py b/lib/lp/registry/browser/product.py
index 907d7f1..a75d7c0 100644
--- a/lib/lp/registry/browser/product.py
+++ b/lib/lp/registry/browser/product.py
@@ -111,7 +111,7 @@ from lp.bugs.browser.structuralsubscription import (
     StructuralSubscriptionTargetTraversalMixin,
     expose_structural_subscription_data_to_js,
 )
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
+from lp.bugs.interfaces.bugtarget import DISABLE_BUG_WEBHOOKS_FEATURE_FLAG
 from lp.bugs.interfaces.bugtask import RESOLVED_BUGTASK_STATUSES
 from lp.charms.browser.hascharmrecipes import HasCharmRecipesMenuMixin
 from lp.code.browser.branchref import BranchRef
@@ -521,7 +521,7 @@ class ProductEditLinksMixin(StructuralSubscriptionMenuMixin):
             "+webhooks",
             "Manage webhooks",
             icon="edit",
-            enabled=bool(getFeatureFlag(BUG_WEBHOOKS_FEATURE_FLAG)),
+            enabled=not (getFeatureFlag(DISABLE_BUG_WEBHOOKS_FEATURE_FLAG)),
         )
 
 
diff --git a/lib/lp/services/features/flags.py b/lib/lp/services/features/flags.py
index 5d42d9d..4ed46ff 100644
--- a/lib/lp/services/features/flags.py
+++ b/lib/lp/services/features/flags.py
@@ -296,9 +296,10 @@ flag_info = sorted(
             "",
         ),
         (
-            "bugs.webhooks.enabled",
+            "bugs.webhooks.disabled",
             "boolean",
-            "If true, allow adding webhooks to bug updates and comments",
+            "If true, disable webhooks from being triggered for bug updates "
+            "and comments.",
             "",
             "",
             "",
diff --git a/lib/lp/services/webhooks/tests/test_browser.py b/lib/lp/services/webhooks/tests/test_browser.py
index 4392228..7a73803 100644
--- a/lib/lp/services/webhooks/tests/test_browser.py
+++ b/lib/lp/services/webhooks/tests/test_browser.py
@@ -10,7 +10,6 @@ import transaction
 from testtools.matchers import MatchesAll, MatchesStructure, Not
 from zope.component import getUtility
 
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
 from lp.charms.interfaces.charmrecipe import (
     CHARM_RECIPE_ALLOW_CREATE,
     CHARM_RECIPE_WEBHOOKS_FEATURE_FLAG,
@@ -192,14 +191,12 @@ class BugUpdateTestHelpersBase:
 
 class ProductTestHelpers(BugUpdateTestHelpersBase):
     def makeTarget(self):
-        self.useFixture(FeatureFixture({BUG_WEBHOOKS_FEATURE_FLAG: "on"}))
         owner = self.factory.makePerson()
         return self.factory.makeProduct(owner=owner)
 
 
 class DistributionTestHelpers(BugUpdateTestHelpersBase):
     def makeTarget(self):
-        self.useFixture(FeatureFixture({BUG_WEBHOOKS_FEATURE_FLAG: "on"}))
         owner = self.factory.makePerson()
         return self.factory.makeDistribution(owner=owner)
 
@@ -209,7 +206,6 @@ class DistributionSourcePackageTestHelpers(BugUpdateTestHelpersBase):
         return self.target.distribution.owner
 
     def makeTarget(self):
-        self.useFixture(FeatureFixture({BUG_WEBHOOKS_FEATURE_FLAG: "on"}))
         return self.factory.makeDistributionSourcePackage()
 
 
diff --git a/lib/lp/soyuz/stories/distribution/xx-distribution-packages.rst b/lib/lp/soyuz/stories/distribution/xx-distribution-packages.rst
index 72471e9..ab342f1 100644
--- a/lib/lp/soyuz/stories/distribution/xx-distribution-packages.rst
+++ b/lib/lp/soyuz/stories/distribution/xx-distribution-packages.rst
@@ -333,6 +333,7 @@ menu, and a "Subscribers" portlet.
     Subscribe to bug mail
     Edit bug mail
     Configure bug tracker
+    Manage webhooks
 
     >>> print(
     ...     extract_text(find_tag_by_id(user_browser.contents, "involvement"))
diff --git a/lib/lp/soyuz/tests/test_packagecopyjob.py b/lib/lp/soyuz/tests/test_packagecopyjob.py
index f4d2e53..ddebebd 100644
--- a/lib/lp/soyuz/tests/test_packagecopyjob.py
+++ b/lib/lp/soyuz/tests/test_packagecopyjob.py
@@ -18,7 +18,6 @@ from zope.component import getUtility
 from zope.security.interfaces import Unauthorized
 from zope.security.proxy import removeSecurityProxy
 
-from lp.bugs.interfaces.bugtarget import BUG_WEBHOOKS_FEATURE_FLAG
 from lp.bugs.interfaces.bugtask import BugTaskStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -1664,16 +1663,6 @@ class PlainPackageCopyJobTests(TestCaseWithFactory, LocalTestHelper):
     def test_copying_closes_bugs_with_webhooks(self):
         # Copying a package into a primary archive closes any bugs mentioned
         # in its recent changelog, including triggering their webhooks.
-
-        # Set bug webhooks feature flag on for the purpose of this test
-        self.useFixture(
-            FeatureFixture(
-                {
-                    BUG_WEBHOOKS_FEATURE_FLAG: "on",
-                }
-            )
-        )
-
         target_archive = self.factory.makeArchive(
             self.distroseries.distribution, purpose=ArchivePurpose.PRIMARY
         )