Merge ~pappacena/launchpad:bugfix-private-mp-webhook-trigger into launchpad:master

[Thiago F. Pappacena]

Fixing webhook trigger for private git repositories

[Colin Watson (cjwatson)]

review: Needs Fixing

7b57c36... by Thiago F. Pappacena on 2020-07-22

Refactoring test code nad removing unused attribute from GitRef

[Thiago F. Pappacena (pappacena)]

cjwatson, pushed the requested changes.

I'll do the other test improvements in another MP, just to make it easier to review (I have made a good bunch of refactoring in the tests to remove some code duplication).

Feel free to approve this one only once the other MP is approved, if you think it will be necessary.

[Thiago F. Pappacena (pappacena)]

Here are the requested test improvements: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/387878

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/code/model/branchmergeproposal.py b/lib/lp/code/model/branchmergeproposal.py
index 5ebcf04..338cc29 100644
--- a/lib/lp/code/model/branchmergeproposal.py
+++ b/lib/lp/code/model/branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Database class for branch merge proposals."""
@@ -70,6 +70,7 @@ from lp.code.event.branchmergeproposal import (
     BranchMergeProposalNeedsReviewEvent,
     ReviewerNominatedEvent,
     )
+from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchcollection import IAllBranches
 from lp.code.interfaces.branchmergeproposal import (
     BRANCH_MERGE_PROPOSAL_FINAL_STATES as FINAL_STATES,
@@ -881,7 +882,7 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
             BranchSubscriptionDiffSize.NODIFF,
             CodeReviewNotificationLevel.FULL,
             user)
-        if branch.stacked_on is not None:
+        if IBranch.providedBy(branch) and branch.stacked_on is not None:
             checked_branches.append(branch)
             if branch.stacked_on not in checked_branches:
                 self._subscribeUserToStackedBranch(
@@ -903,14 +904,11 @@ class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
         the reviewer if the branch is private and the reviewer is an open
         team.
         """
-        if self.source_branch is None:
-            # This only applies to Bazaar, which has stacked branches.
-            return
-        source = self.source_branch
+        source = self.merge_source
         if (not source.visibleByUser(reviewer) and
             self._acceptable_to_give_visibility(source, reviewer)):
             self._subscribeUserToStackedBranch(source, reviewer)
-        target = self.target_branch
+        target = self.merge_target
         if (not target.visibleByUser(reviewer) and
             self._acceptable_to_give_visibility(source, reviewer)):
             self._subscribeUserToStackedBranch(target, reviewer)
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 8936574..7e380a3 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for BranchMergeProposals."""
@@ -91,6 +91,7 @@ from lp.services.webapp import canonical_url
 from lp.services.webhooks.testing import LogsScheduledWebhooks
 from lp.services.xref.interfaces import IXRefSet
 from lp.testing import (
+    admin_logged_in,
     ExpectedException,
     launchpadlib_for,
     login,
@@ -1035,15 +1036,24 @@ class TestMergeProposalWebhooks(WithScenarios, TestCaseWithFactory):
         self.useFixture(FeatureFixture(
             {BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG: "on"}))
 
-    def makeBranch(self, same_target_as=None):
-        kwargs = {}
+    def makeBranch(self, same_target_as=None, information_type=None):
+        # Create the product pillar and its access policy if information
+        # type is "PROPRIETARY".
+        if same_target_as is None:
+            product = self.factory.makeProduct()
+            if information_type == InformationType.PROPRIETARY:
+                self.factory.makeAccessPolicy(product)
+        else:
+            same_target_as = removeSecurityProxy(same_target_as)
+            product = (same_target_as.target if self.git else
+                       same_target_as.product)
+
+        kwargs = {"information_type": information_type}
         if self.git:
-            if same_target_as is not None:
-                kwargs["target"] = same_target_as.target
+            kwargs["target"] = product
             return self.factory.makeGitRefs(**kwargs)[0]
         else:
-            if same_target_as is not None:
-                kwargs["product"] = same_target_as.product
+            kwargs["product"] = product
             return self.factory.makeProductBranch(**kwargs)
 
     def getWebhookTarget(self, branch):
@@ -1102,6 +1112,39 @@ class TestMergeProposalWebhooks(WithScenarios, TestCaseWithFactory):
                     (hook, "merge-proposal:0.1",
                      MatchesDict(expected_redacted_payload))]))
 
+    def test_create_private_repo_triggers_webhooks(self):
+        # When a merge proposal is created, any relevant webhooks are
+        # triggered even if the repository is proprietary.
+        logger = self.useFixture(FakeLogger())
+        source = self.makeBranch(information_type=InformationType.PROPRIETARY)
+        target = self.makeBranch(
+            same_target_as=source,
+            information_type=InformationType.PROPRIETARY)
+
+        with admin_logged_in():
+            # Create the web hook and the proposal.
+            registrant = self.factory.makePerson()
+            hook = self.factory.makeWebhook(
+                target=self.getWebhookTarget(target),
+                event_types=["merge-proposal:0.1"])
+            proposal = source.addLandingTarget(
+                registrant, target, needs_review=True)
+            target_owner = target.owner
+
+        login_person(target_owner)
+        delivery = hook.deliveries.one()
+        expected_payload = {
+            "merge_proposal": Equals(self.getURL(proposal)),
+            "action": Equals("created"),
+            "new": MatchesDict(self.getExpectedPayload(proposal)),
+            }
+        expected_redacted_payload = dict(
+            expected_payload,
+            new=MatchesDict(
+                self.getExpectedPayload(proposal, redact=True)))
+        self.assertCorrectDelivery(expected_payload, hook, delivery)
+        self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+
     def test_create_triggers_webhooks(self):
         # When a merge proposal is created, any relevant webhooks are
         # triggered.