Merge ~ines-almeida/launchpad:invert-bug-webhook-feature-flag into launchpad:master

[Ines Almeida]

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

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.

[Guruprasad (lgp171188)]

Inês, here is what our documentation on Feature Flags says about the type of change made in this MP

> The effect is typically 'enabled', 'visible', or 'timeout'. These should always be in the positive sense, not 'disabled'. If timeouts are given, they should be in seconds (decimals can be given in the value.)

I agree with this and also believe that the feature flags should be opt-in and not opt-out by default. Since your intent is to enable it globally to catch all permission issues, we should try to make the relevant test suite changes to that effect and not toggle the default behaviour of a feature flag, imho.

review: Needs Information

[Colin Watson (cjwatson)]

Guruprasad, you weren't here for the standup meeting where we discussed this. The problem is that test suite changes to do that would slow down the test suite globally by a non-trivial amount (adding extra SQL statements to every test that uses the database, which is most of them). `MemoryFeatureFixture` can avoid the extra SQL statements in many cases, but isn't effective for things like tests of scripts run in subprocesses, which are likely to be important here.

An opt-out feature rule does feel a little dirty, and it doesn't give us a reasonable way to enable it on a per-team basis, so the general injunction in the documentation does make sense. However, I think it can make some sense when the feature rule is intended to be short-term: that is, the intent is to make the behaviour in question unconditional, but we want a feature rule as an escape hatch to let us turn it off quickly in case of problems.

[Ines Almeida (ines-almeida)]

> when the feature rule is intended to be short-term

That being said, one thing to consider is to cleanup a lot of our feature flags that have been enabled for 1+ years, and cleaning this feature flag up in a few of months once we're confident.

I agree with both here. Happy to re-discuss this in a standup

[Guruprasad (lgp171188)]

Thanks for the details Colin and Inês!

[Guruprasad (lgp171188)]

LGTM 👍

review: Approve

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..43a51c2 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 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 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..dd1f43e 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..ebd3895 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..fa45f0d 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
         )