launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/bmp-buglinktarget into lp:launchpad.

Commit message:
Give BranchMergeProposal a basic implementation of IBugLinkTarget.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1492926 in Launchpad itself: "Git merge proposals can't be linked to bugs"
  https://bugs.launchpad.net/launchpad/+bug/1492926

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bmp-buglinktarget/+merge/298365

Give BranchMergeProposal a basic implementation of IBugLinkTarget.

This doesn't do much yet apart from exporting branch_merge_proposal.bugs on the webservice, but it's an essential building block for linking Git-based MPs to bugs.

Note that, for the time being, Bazaar-based MPs use the source branch as their storage for related bugs, which is perhaps a little weird but avoids a data migration and having to work out what to do about e.g. branches that have linked bugs but no associated merge proposals.  We can always move this around later if we work out a reasonable alternative approach.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bmp-buglinktarget into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2016-05-13 17:16:40 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2016-06-25 09:39:11 +0000
@@ -67,6 +67,7 @@
 
 from lp import _
 from lp.app.interfaces.launchpad import IPrivacy
+from lp.bugs.interfaces.buglink import IBugLinkTarget
 from lp.code.enums import (
     BranchMergeProposalStatus,
     CodeReviewVote,
@@ -627,7 +628,7 @@
         """
 
 
-class IBranchMergeProposalAnyAllowedPerson(Interface):
+class IBranchMergeProposalAnyAllowedPerson(IBugLinkTarget):
 
     @operation_parameters(
         subject=Text(), content=Text(),

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2016-06-03 14:08:52 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2016-06-25 09:39:11 +0000
@@ -11,6 +11,7 @@
     ]
 
 from email.utils import make_msgid
+from operator import attrgetter
 
 from lazr.lifecycle.event import (
     ObjectCreatedEvent,
@@ -42,6 +43,7 @@
 from zope.interface import implementer
 
 from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.bugs.model.buglinktarget import BugLinkTargetMixin
 from lp.code.enums import (
     BranchMergeProposalStatus,
     BranchSubscriptionDiffSize,
@@ -172,7 +174,7 @@
 
 
 @implementer(IBranchMergeProposal, IHasBranchTarget)
-class BranchMergeProposal(SQLBase):
+class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
     """A relationship between a person and a branch."""
 
     _table = 'BranchMergeProposal'
@@ -354,19 +356,51 @@
         storm_validator=validate_public_person, notNull=False,
         default=None)
 
+    @property
+    def bugs(self):
+        if self.source_branch is not None:
+            # For Bazaar, we currently only store bug/branch links.
+            bugs = self.source_branch.linked_bugs
+        else:
+            # XXX cjwatson 2016-06-24: Implement for Git.
+            bugs = []
+        return list(sorted(bugs, key=attrgetter('id')))
+
     def getRelatedBugTasks(self, user):
         """Bug tasks which are linked to the source but not the target.
 
         Implies that these would be fixed, in the target, by the merge.
         """
-        if self.source_branch is None:
-            # XXX cjwatson 2015-04-16: Implement once Git refs have linked
-            # bug tasks.
+        if self.source_branch is not None:
+            source_tasks = self.source_branch.getLinkedBugTasks(user)
+            target_tasks = self.target_branch.getLinkedBugTasks(user)
+            return [bugtask
+                for bugtask in source_tasks if bugtask not in target_tasks]
+        else:
+            # XXX cjwatson 2016-06-24: Implement for Git.
             return []
-        source_tasks = self.source_branch.getLinkedBugTasks(user)
-        target_tasks = self.target_branch.getLinkedBugTasks(user)
-        return [bugtask
-            for bugtask in source_tasks if bugtask not in target_tasks]
+
+    def linkBug(self, bug, user=None, check_permissions=True):
+        """See `BugLinkTargetMixin`."""
+        if self.source_branch is not None:
+            # For Bazaar, we currently only store bug/branch links.
+            return self.source_branch.linkBug(bug, user)
+        else:
+            # XXX cjwatson 2016-06-24: Implement for Git.
+            raise NotImplementedError
+
+    def unlinkBug(self, bug, user=None, check_permissions=True):
+        """See `BugLinkTargetMixin`."""
+        if self.source_branch is not None:
+            # For Bazaar, we currently only store bug/branch links.
+            # XXX cjwatson 2016-06-11: This may behave strangely in some
+            # cases: if a branch is the source for multiple merge proposals,
+            # then unlinking a bug from one will unlink them all.  Fixing
+            # this would require a complicated data migration.
+            return self.source_branch.unlinkBug(bug, user)
+        else:
+            # XXX cjwatson 2016-06-24: Implement for Git.
+            raise NotImplementedError
 
     @property
     def address(self):
@@ -385,9 +419,12 @@
     @property
     def target(self):
         """See `IHasBranchTarget`."""
-        # XXX cjwatson 2015-04-12: This is not an IBranchTarget for Git,
-        # although it has similar semantics.
-        return self.merge_source.target
+        if self.source_branch is not None:
+            return self.source_branch.target
+        else:
+            # XXX cjwatson 2015-04-12: This is not an IBranchTarget for Git,
+            # although it has similar semantics.
+            return self.source_git_repository.namespace
 
     root_message_id = StringCol(default=None)
 

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2016-03-02 21:21:26 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2016-06-25 09:39:11 +0000
@@ -1375,49 +1375,48 @@
             SQLObjectNotFound, BranchMergeProposalJob.get, job_id)
 
 
-class TestBranchMergeProposalBugs(TestCaseWithFactory):
+class TestBranchMergeProposalBugsMixin:
 
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        TestCaseWithFactory.setUp(self)
+        super(TestBranchMergeProposalBugsMixin, self).setUp()
         self.user = self.factory.makePerson()
         login_person(self.user)
 
+    def test_bugs(self):
+        # bugs includes all linked bugs.
+        bmp = self._makeBranchMergeProposal()
+        self.assertEqual([], bmp.bugs)
+        bugs = [self.factory.makeBug() for _ in range(2)]
+        bmp.linkBug(bugs[0], bmp.registrant)
+        self.assertEqual([bugs[0]], bmp.bugs)
+        bmp.linkBug(bugs[1], bmp.registrant)
+        self.assertContentEqual(bugs, bmp.bugs)
+        bmp.unlinkBug(bugs[0], bmp.registrant)
+        self.assertEqual([bugs[1]], bmp.bugs)
+        bmp.unlinkBug(bugs[1], bmp.registrant)
+        self.assertEqual([], bmp.bugs)
+
     def test_related_bugtasks_includes_source_bugtasks(self):
-        """related_bugtasks includes bugtasks linked to the source branch."""
-        bmp = self.factory.makeBranchMergeProposal()
-        source_branch = bmp.source_branch
+        # related_bugtasks includes bugtasks linked to the source branch in
+        # the Bazaar case.
+        bmp = self._makeBranchMergeProposal()
         bug = self.factory.makeBug()
-        source_branch.linkBug(bug, bmp.registrant)
+        bmp.linkBug(bug, bmp.registrant)
         self.assertEqual(
             bug.bugtasks, list(bmp.getRelatedBugTasks(self.user)))
 
-    def test_related_bugtasks_excludes_target_bugs(self):
-        """related_bugtasks ignores bugs linked to the source branch."""
-        bmp = self.factory.makeBranchMergeProposal()
-        bug = self.factory.makeBug()
-        bmp.target_branch.linkBug(bug, bmp.registrant)
-        self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
-
-    def test_related_bugtasks_excludes_mutual_bugs(self):
-        """related_bugtasks ignores bugs linked to both branches."""
-        bmp = self.factory.makeBranchMergeProposal()
-        bug = self.factory.makeBug()
-        bmp.source_branch.linkBug(bug, bmp.registrant)
-        bmp.target_branch.linkBug(bug, bmp.registrant)
-        self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
-
     def test_related_bugtasks_excludes_private_bugs(self):
-        """related_bugtasks ignores private bugs for non-authorised users."""
-        bmp = self.factory.makeBranchMergeProposal()
+        # related_bugtasks ignores private bugs for non-authorised users.
+        bmp = self._makeBranchMergeProposal()
         bug = self.factory.makeBug()
-        bmp.source_branch.linkBug(bug, bmp.registrant)
+        bmp.linkBug(bug, bmp.registrant)
         person = self.factory.makePerson()
         with person_logged_in(person):
             private_bug = self.factory.makeBug(
                 owner=person, information_type=InformationType.USERDATA)
-            bmp.source_branch.linkBug(private_bug, person)
+            bmp.linkBug(private_bug, person)
             private_tasks = private_bug.bugtasks
         self.assertEqual(
             bug.bugtasks, list(bmp.getRelatedBugTasks(self.user)))
@@ -1427,6 +1426,28 @@
             all_bugtasks, list(bmp.getRelatedBugTasks(person)))
 
 
+class TestBranchMergeProposalBugsBzr(
+    TestBranchMergeProposalBugsMixin, TestCaseWithFactory):
+
+    def _makeBranchMergeProposal(self):
+        return self.factory.makeBranchMergeProposal()
+
+    def test_related_bugtasks_excludes_target_bugs(self):
+        # related_bugtasks ignores bugs linked to the target branch.
+        bmp = self._makeBranchMergeProposal()
+        bug = self.factory.makeBug()
+        bmp.target_branch.linkBug(bug, bmp.registrant)
+        self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
+
+    def test_related_bugtasks_excludes_mutual_bugs(self):
+        # related_bugtasks ignores bugs linked to both branches.
+        bmp = self._makeBranchMergeProposal()
+        bug = self.factory.makeBug()
+        bmp.source_branch.linkBug(bug, bmp.registrant)
+        bmp.target_branch.linkBug(bug, bmp.registrant)
+        self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
+
+
 class TestNotifyModified(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer

=== modified file 'lib/lp/code/stories/webservice/xx-branchmergeproposal.txt'
--- lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2015-10-06 06:48:01 +0000
+++ lib/lp/code/stories/webservice/xx-branchmergeproposal.txt	2016-06-25 09:39:11 +0000
@@ -47,6 +47,8 @@
     address: u'mp+...@xxxxxxxxxxxxxxxxxx'
     all_comments_collection_link:
         u'http://api.launchpad.dev/devel/~.../+merge/.../all_comments'
+    bugs_collection_link:
+        u'http://api.launchpad.dev/devel/~.../+merge/.../bugs'
     commit_message: u'It was merged!\n'
     date_created: u'...'
     date_merged: None
@@ -148,7 +150,9 @@
     >>> merge_proposal = webservice.get(proposal_url).jsonBody()
     >>> pprint_entry(merge_proposal)
     address: u'mp+...@xxxxxxxxxxxxxxxxxx'
-    all_comments_collection_link: u'http://.../~source/fooix/fix-it/+merge/.../all_comments'
+    all_comments_collection_link:
+        u'http://.../~source/fooix/fix-it/+merge/.../all_comments'
+    bugs_collection_link: u'http://.../~source/fooix/fix-it/+merge/.../bugs'
     commit_message: None
     date_created: ...
     date_merged: None