launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/code/browser/branchlisting.py'
> --- lib/lp/code/browser/branchlisting.py	2015-06-04 09:15:30 +0000
> +++ lib/lp/code/browser/branchlisting.py	2015-06-04 09:15:31 +0000
> @@ -84,6 +84,7 @@
>  from lp.code.interfaces.branchcollection import IAllBranches
>  from lp.code.interfaces.branchnamespace import IBranchNamespacePolicy
>  from lp.code.interfaces.branchtarget import IBranchTarget
> +from lp.code.interfaces.gitcollection import IGitCollection
>  from lp.code.interfaces.revision import IRevisionSet
>  from lp.code.interfaces.revisioncache import IRevisionCache
>  from lp.code.interfaces.seriessourcepackagebranch import (
> @@ -535,6 +536,10 @@
>      # enumeration to not offer in the sort_by widget.
>      no_sort_by = ()
>  
> +    # Many Bazaar branch listings have a closely related Git listing
> +    # that they should link to.
> +    show_git_link = False
> +
>      # Set the feed types to be only the various branches feed links.  The
>      # `feed_links` property will screen this list and produce only the feeds
>      # appropriate to the context.
> @@ -985,6 +990,11 @@
>          coll = super(PersonProductBranchesView, self)._getCollection()
>          return coll.inProduct(self.context.product)
>  
> +    @property
> +    def show_git_link(self):
> +        c = IGitCollection(self.context.product).ownedBy(self.context.person)

As with gitlisting, this can just be "c = IGitCollection(self.context)".

> +        return not c.visibleByUser(self.user).is_empty()
> +
>  
>  class PersonTeamBranchesView(LaunchpadView):
>      """View for team branches portlet."""
> @@ -1295,6 +1305,11 @@
>          """Is there a branch assigned as development focus?"""
>          return self.development_focus_branch is not None
>  
> +    @property
> +    def show_git_link(self):
> +        c = IGitCollection(self.context)
> +        return not c.visibleByUser(self.user).is_empty()
> +
>  
>  class ProjectBranchesView(BranchListingView):
>      """View for branch listing for a project."""
> 
> === modified file 'lib/lp/code/browser/gitlisting.py'
> --- lib/lp/code/browser/gitlisting.py	2015-06-04 09:15:30 +0000
> +++ lib/lp/code/browser/gitlisting.py	2015-06-04 09:15:31 +0000
> @@ -19,6 +19,7 @@
>  
>  from lp.app.enums import PRIVATE_INFORMATION_TYPES
>  from lp.code.browser.gitrepository import GitRefBatchNavigator
> +from lp.code.interfaces.branchcollection import IBranchCollection
>  from lp.code.interfaces.gitcollection import IGitCollection
>  from lp.code.interfaces.gitnamespace import (
>      get_git_namespace,
> @@ -67,6 +68,10 @@
>      def repo_collection(self):
>          raise NotImplementedError()
>  
> +    @property
> +    def show_bzr_link(self):
> +        raise NotImplementedError()
> +
>      def default_git_repository_branches(self):
>          """All branches in the default Git repository, sorted for display."""
>          return GitRefBatchNavigator(self, self.default_git_repository)
> @@ -121,6 +126,11 @@
>      def repo_collection(self):
>          return IGitCollection(self.target).visibleByUser(self.user)
>  
> +    @property
> +    def show_bzr_link(self):
> +        collection = IBranchCollection(self.target)
> +        return not collection.visibleByUser(self.user).is_empty()
> +
>  
>  class PersonTargetGitListingView(BaseGitListingView):
>  
> @@ -137,6 +147,13 @@
>          else:
>              raise Exception("Unknown context: %r" % self.context)
>  
> +    @property
> +    def owner(self):
> +        if IPersonProduct.providedBy(self.context):
> +            return self.context.person
> +        else:
> +            raise Exception("Unknown context: %r" % self.context)

As with gitlisting, this might as well preemptively handle IPersonDistributionSourcePackage (indeed it's the same code for both).

> +
>      @cachedproperty
>      def default_git_repository(self):
>          repo = getUtility(IGitRepositorySet).getDefaultRepositoryForOwner(
> @@ -152,3 +169,8 @@
>      def repo_collection(self):
>          return IGitCollection(self.target).ownedBy(
>              self.context.person).visibleByUser(self.user)
> +
> +    @property
> +    def show_bzr_link(self):
> +        collection = IBranchCollection(self.target).ownedBy(self.owner)

IBranchCollection actually has an adapter for PersonProduct, so if you added an adapter for PersonDistributionSourcePackage then you could abbreviate this for both cases.

> +        return not collection.visibleByUser(self.user).is_empty()
> 
> === modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
> --- lib/lp/code/browser/tests/test_branchlisting.py	2015-06-02 06:35:38 +0000
> +++ lib/lp/code/browser/tests/test_branchlisting.py	2015-06-04 09:15:31 +0000
> @@ -416,6 +416,14 @@
>                       'in Launchpad today.'))
>          self.assertThat(page, empty_message_matcher)
>  
> +    def test_git_link(self):
> +        page = self.get_branch_list_page()
> +        self.assertNotIn('View Git repositories', page)
> +
> +        self.factory.makeGitRepository(owner=self.person, target=self.product)
> +        page = self.get_branch_list_page()
> +        self.assertIn('View Git repositories', page)
> +
>  
>  class TestSourcePackageBranchesView(TestCaseWithFactory):
>  
> 
> === modified file 'lib/lp/code/browser/tests/test_gitlisting.py'
> --- lib/lp/code/browser/tests/test_gitlisting.py	2015-06-04 09:15:30 +0000
> +++ lib/lp/code/browser/tests/test_gitlisting.py	2015-06-04 09:15:31 +0000
> @@ -146,6 +146,18 @@
>                  [invisible_repo, other_repo],
>                  owner_view.repo_collection.getRepositories())
>  
> +    def test_bzr_link(self):
> +        product = self.factory.makeProduct()
> +
> +        # With a fresh product there's no Bazaar link.
> +        view = create_initialized_view(product, '+git')
> +        self.assertNotIn('View Bazaar branches', view())
> +
> +        # But it appears once we create a branch.
> +        self.factory.makeBranch(product=product)
> +        view = create_initialized_view(product, '+git')
> +        self.assertIn('View Bazaar branches', view())
> +
>  
>  class TestPersonTargetGitListingView(TestCaseWithFactory):
>  
> @@ -269,3 +281,17 @@
>              self.assertContentEqual(
>                  [invisible_repo, other_repo],
>                  owner_view.repo_collection.getRepositories())
> +
> +    def test_bzr_link(self):
> +        owner = self.factory.makePerson()
> +        product = self.factory.makeProduct()
> +        pp = PersonProduct(owner, product)
> +
> +        # With a fresh product there's no Bazaar link.
> +        view = create_initialized_view(pp, '+git')
> +        self.assertNotIn('View Bazaar branches', view())
> +
> +        # But it appears once we create a branch.
> +        self.factory.makeBranch(owner=owner, product=product)
> +        view = create_initialized_view(pp, '+git')
> +        self.assertIn('View Bazaar branches', view())
> 
> === modified file 'lib/lp/code/browser/tests/test_product.py'
> --- lib/lp/code/browser/tests/test_product.py	2015-06-04 09:15:30 +0000
> +++ lib/lp/code/browser/tests/test_product.py	2015-06-04 09:15:31 +0000
> @@ -145,6 +145,15 @@
>          expected = 'There are no branches for %s' % product.displayname
>          self.assertIn(expected, html)
>  
> +    def test_git_link(self):
> +        product = self.factory.makeProduct()
> +        view = create_initialized_view(product, '+branches')
> +        self.assertNotIn('View Git repositories', view())
> +
> +        self.factory.makeGitRepository(target=product)
> +        view = create_initialized_view(product, '+branches')
> +        self.assertIn('View Git repositories', view())
> +
>  
>  class TestProductBranchesServiceUsages(ProductTestBase, BrowserTestCase):
>      """Tests for the product code page, especially the usage messasges."""
> 
> === modified file 'lib/lp/code/templates/gitlisting.pt'
> --- lib/lp/code/templates/gitlisting.pt	2015-06-04 09:15:30 +0000
> +++ lib/lp/code/templates/gitlisting.pt	2015-06-04 09:15:31 +0000
> @@ -40,6 +40,9 @@
>      </div>
>    </metal:side>
>    <metal:main fill-slot="main">
> +    <span class="see-all" tal:condition="view/show_bzr_link">
> +      <a tal:attributes="href context/fmt:url:code/+branches">View Bazaar branches</a>
> +    </span>
>      <tal:default-repository
>          condition="view/default_git_repository"
>          define="repository view/default_git_repository">
> 
> === modified file 'lib/lp/code/templates/person-branches.pt'
> --- lib/lp/code/templates/person-branches.pt	2014-12-06 02:10:46 +0000
> +++ lib/lp/code/templates/person-branches.pt	2015-06-04 09:15:31 +0000
> @@ -22,6 +22,10 @@
>  <div metal:fill-slot="main"
>       tal:define="branches view/branches">
>  
> +  <span class="see-all" tal:condition="view/show_git_link">
> +    <a tal:attributes="href context/fmt:url:code/+git">View Git repositories</a>
> +  </span>
> +
>    <p id="junk-branch-directions" tal:condition="view/show_junk_directions">
>      You can push (upload) personal branches
>      (those not related to a project) with the following command:
> 
> === modified file 'lib/lp/code/templates/product-branches.pt'
> --- lib/lp/code/templates/product-branches.pt	2015-06-04 09:15:30 +0000
> +++ lib/lp/code/templates/product-branches.pt	2015-06-04 09:15:31 +0000
> @@ -54,6 +54,10 @@
>  
>    <tal:main metal:fill-slot="main">
>  
> +    <span class="see-all" tal:condition="view/show_git_link">
> +      <a tal:attributes="href context/fmt:url:code/+git">View Git repositories</a>
> +    </span>
> +
>      <tal:branch-summary content="structure context/@@+branch-summary" />
>  
>      <tal:code-statistics
> 


-- 
https://code.launchpad.net/~wgrant/launchpad/gitlisting-crosslinks/+merge/261067
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.