launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === 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

I know it's just copying existing code, but perhaps this is an opportunity to make this (and the Bazaar case) not complete garbage.

> +
>  
>  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."""
>  
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-mp-move-outside-ref/+merge/260105
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.