launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-mp-move-outside-ref into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1456589 in Launchpad itself: "Git-based merge proposal URLs are located under (removable) references"
  https://bugs.launchpad.net/launchpad/+bug/1456589

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mp-move-outside-ref/+merge/260105

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.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mp-move-outside-ref into lp:launchpad.

=== 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 07:58:07 +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 07:58:07 +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
@@ -70,7 +72,7 @@
             return None
         for proposal in self.context.landing_targets:
             if proposal.id == id:
-                return proposal
+                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 07:58:07 +0000
@@ -37,6 +37,7 @@
     Link,
     Navigation,
     NavigationMenu,
+    stepthrough,
     stepto,
     )
 from lp.services.webapp.authorization import (
@@ -88,6 +89,18 @@
                 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
+        for proposal in self.context.landing_targets:
+            if proposal.id == id:
+                return proposal
+
 
 class GitRepositoryEditMenu(NavigationMenu):
     """Edit menu for `IGitRepository`."""

=== 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 07:58:07 +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/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 07:58:07 +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/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 07:58:07 +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."""