Merge lp:~cjwatson/launchpad/git-mp-move-outside-ref into lp:launchpad

[Colin Watson]

Place Git-based merge proposals under their source repository, not the (removable) source reference.

Place Git-based merge proposals under their source repository, not the (removable) source reference.

I decided to leave a redirection around for a while even though it probably isn't very necessary, but we can remove that after a month or two.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2015-04-22 16:41:41 +0000
+++ lib/lp/code/browser/branch.py	2015-05-26 12:18:43 +0000
@@ -215,9 +215,7 @@
         except ValueError:
             # Not a number.
             return None
-        for proposal in self.context.landing_targets:
-            if proposal.id == id:
-                return proposal
+        return self.context.getMergeProposalByID(id)
 
     @stepto("+code-import")
     def traverse_code_import(self):

=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml	2015-05-19 11:31:59 +0000
+++ lib/lp/code/browser/configure.zcml	2015-05-26 12:18:43 +0000
@@ -253,7 +253,7 @@
     <browser:url
         for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal"
         path_expression="string:+merge/${id}"
-        attribute_to_parent="merge_source"
+        attribute_to_parent="parent"
         rootsite="code"/>
     <browser:page
         for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalListingBatchNavigator"

=== modified file 'lib/lp/code/browser/gitref.py'
--- lib/lp/code/browser/gitref.py	2015-05-05 10:15:19 +0000
+++ lib/lp/code/browser/gitref.py	2015-05-26 12:18:43 +0000
@@ -56,6 +56,8 @@
 from lp.services.webapp.authorization import check_permission
 
 
+# XXX cjwatson 2015-05-26: We can get rid of this after a short while, since
+# it's just a compatibility redirection.
 class GitRefNavigation(Navigation):
 
     usedfor = IGitRef
@@ -68,9 +70,9 @@
         except ValueError:
             # Not a number.
             return None
-        for proposal in self.context.landing_targets:
-            if proposal.id == id:
-                return proposal
+        proposal = self.context.getMergeProposalByID(id)
+        if proposal is not None:
+            return self.redirectSubTree(canonical_url(proposal))
 
 
 class GitRefContextMenu(ContextMenu):

=== modified file 'lib/lp/code/browser/gitrepository.py'
--- lib/lp/code/browser/gitrepository.py	2015-05-19 11:31:59 +0000
+++ lib/lp/code/browser/gitrepository.py	2015-05-26 12:18:43 +0000
@@ -37,6 +37,7 @@
     Link,
     Navigation,
     NavigationMenu,
+    stepthrough,
     stepto,
     )
 from lp.services.webapp.authorization import (
@@ -88,6 +89,16 @@
                 return ref
         raise NotFoundError
 
+    @stepthrough("+merge")
+    def traverse_merge_proposal(self, id):
+        """Traverse to an `IBranchMergeProposal`."""
+        try:
+            id = int(id)
+        except ValueError:
+            # Not a number.
+            return None
+        return self.context.getMergeProposalByID(id)
+
 
 class GitRepositoryEditMenu(NavigationMenu):
     """Edit menu for `IGitRepository`."""

=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py	2015-05-19 11:27:02 +0000
+++ lib/lp/code/interfaces/branch.py	2015-05-26 12:18:43 +0000
@@ -649,6 +649,9 @@
                           merged_revnos=None, eager_load=False):
         """Return matching BranchMergeProposals."""
 
+    def getMergeProposalByID(id):
+        """Return this branch's merge proposal with this id, or None."""
+
     def scheduleDiffUpdates():
         """Create UpdatePreviewDiffJobs for this branch's targets."""
 

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2015-04-22 14:52:10 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2015-05-26 12:18:43 +0000
@@ -215,6 +215,10 @@
     merge_prerequisite = Attribute(
         "The branch that the source branch branched from (VCS-agnostic).")
 
+    parent = Attribute(
+        "The parent object for use in navigation: source branch for Bazaar, "
+        "or source repository for Git.")
+
 
 class IBranchMergeProposalView(Interface):
 

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2015-05-21 16:36:14 +0000
+++ lib/lp/code/interfaces/gitref.py	2015-05-26 12:18:43 +0000
@@ -285,6 +285,9 @@
                           merged_revision_ids=None, eager_load=False):
         """Return matching BranchMergeProposals."""
 
+    def getMergeProposalByID(id):
+        """Return this reference's merge proposal with this id, or None."""
+
     pending_writes = Attribute(
         "Whether there are recent changes in this repository that have not "
         "yet been scanned.")

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2015-05-19 11:29:24 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2015-05-26 12:18:43 +0000
@@ -461,6 +461,9 @@
         "A collection of the merge proposals that are dependent on this "
         "repository.")
 
+    def getMergeProposalByID(id):
+        """Return this repository's merge proposal with this id, or None."""
+
     def isRepositoryMergeable(other):
         """Is the other repository mergeable into this one (or vice versa)?"""
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2015-05-19 11:27:02 +0000
+++ lib/lp/code/model/branch.py	2015-05-26 12:18:43 +0000
@@ -502,6 +502,13 @@
             status, target_branch=self, merged_revnos=merged_revnos,
             eager_load=eager_load)
 
+    def getMergeProposalByID(self, id):
+        """See `IBranch`."""
+        return Store.of(self).find(
+            BranchMergeProposal,
+            BranchMergeProposal.id == id,
+            BranchMergeProposal.source_branch == self).one()
+
     def isBranchMergeable(self, target_branch):
         """See `IBranch`."""
         # In some imaginary time we may actually check to see if this branch

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2015-05-13 09:36:02 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2015-05-26 12:18:43 +0000
@@ -283,6 +283,13 @@
         else:
             return self.prerequisite_git_ref
 
+    @property
+    def parent(self):
+        if self.source_branch is not None:
+            return self.source_branch
+        else:
+            return self.source_git_repository
+
     description = StringCol(default=None)
 
     whiteboard = StringCol(default=None)

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2015-05-21 16:36:14 +0000
+++ lib/lp/code/model/gitref.py	2015-05-26 12:18:43 +0000
@@ -203,6 +203,10 @@
             status, target_repository=self.repository, target_path=self.path,
             merged_revision_ids=merged_revision_ids, eager_load=eager_load)
 
+    def getMergeProposalByID(self, id):
+        """See `IGitRef`."""
+        return self.landing_targets.find(BranchMergeProposal.id == id).one()
+
     @property
     def pending_writes(self):
         """See `IGitRef`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-05-23 16:29:29 +0000
+++ lib/lp/code/model/gitrepository.py	2015-05-26 12:18:43 +0000
@@ -791,6 +791,10 @@
             Not(BranchMergeProposal.queue_status.is_in(
                 BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
 
+    def getMergeProposalByID(self, id):
+        """See `IGitRepository`."""
+        return self.landing_targets.find(BranchMergeProposal.id == id).one()
+
     def isRepositoryMergeable(self, other):
         """See `IGitRepository`."""
         return self.namespace.areRepositoriesMergeable(other.namespace)

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2015-04-21 16:53:45 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2015-05-26 12:18:43 +0000
@@ -96,28 +96,43 @@
         verifyObject(IBranchMergeProposal, bmp)
 
 
-class TestBranchMergeProposalCanonicalUrl(TestCaseWithFactory):
+class TestBranchMergeProposalCanonicalUrlMixin:
     """Tests canonical_url for merge proposals."""
 
     layer = DatabaseFunctionalLayer
 
     def test_BranchMergeProposal_canonical_url_base(self):
-        # The URL for a merge proposal starts with the source branch.
-        bmp = self.factory.makeBranchMergeProposal()
+        # The URL for a merge proposal starts with the parent (source branch
+        # or source Git repository).
+        bmp = self._makeBranchMergeProposal()
         url = canonical_url(bmp)
-        source_branch_url = canonical_url(bmp.source_branch)
-        self.assertTrue(url.startswith(source_branch_url))
+        parent_url = canonical_url(bmp.parent)
+        self.assertTrue(url.startswith(parent_url))
 
     def test_BranchMergeProposal_canonical_url_rest(self):
         # The rest of the URL for a merge proposal is +merge followed by the
         # db id.
-        bmp = self.factory.makeBranchMergeProposal()
+        bmp = self._makeBranchMergeProposal()
         url = canonical_url(bmp)
-        source_branch_url = canonical_url(bmp.source_branch)
-        rest = url[len(source_branch_url):]
+        parent_url = canonical_url(bmp.parent)
+        rest = url[len(parent_url):]
         self.assertEqual('/+merge/%s' % bmp.id, rest)
 
 
+class TestBranchMergeProposalCanonicalUrlBzr(
+    TestBranchMergeProposalCanonicalUrlMixin, TestCaseWithFactory):
+
+    def _makeBranchMergeProposal(self):
+        return self.factory.makeBranchMergeProposal()
+
+
+class TestBranchMergeProposalCanonicalUrlGit(
+    TestBranchMergeProposalCanonicalUrlMixin, TestCaseWithFactory):
+
+    def _makeBranchMergeProposal(self):
+        return self.factory.makeBranchMergeProposalForGit()
+
+
 class TestBranchMergeProposalPrivacy(TestCaseWithFactory):
     """Ensure that BranchMergeProposal implements privacy."""