Merge lp:~jcsackett/launchpad/projectgroup-branches-652156 into lp:launchpad

[j.c.sackett]

Updates the projectgroup branches page to show a message consistent with the other codehosting_usage messages when there are no branches, instead of an empty table.

Summary
=======

Updates the projectgroup branch listing page to display a message similar to other codehosting usage messages for other pillars (e.g. "<Product> does not use Launchpad for codehosting.")

Right now if no products are configured to have branches on Launchpad (the condition to set codehosting_usage to LAUNCHPAD), the project group simply shows an empty table with a message about using bazaar; in line with the bridging the gap tasks, it should simply state the the project group isn't using launchpad for codehosting, and be done with it.

Proposed fix
============

Detect when there are no branches and display the appropriate message instead of using the branch-listing portlet.

Preimplementation talk
======================

Spoke with Edwin Grubbs. We brought up the possiblity of linking out to all the products for the group so someone could configure the products, but to do so sanely (i.e. only show the links for products the viewer can configure) requires doing database checks for every product; this could lead to death by SQL timeouts.

Implementation details
======================

Largely as in proposed. A new property, has_branches, was added to projectgroups to check if there are any branches for the projectgroup's products. If this returns false, the message is shown.

Tests
=====

bin/test -vvct TestProjectBranches
bin/test -vvct TestProjectBranchListing

Screenshots
===========

The old appearance is available at:
http://people.canonical.com/~jc/images/withoutbranches-old.png

The new appearance is available at:
http://people.canonical.com/~jc/images/withoutbranches-new.png

The appearance with any branches (in either case) is available at:
http://people.canonical.com/~jc/images/withbranches.png

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/browser/tests/test_branchlisting.py
  lib/lp/code/templates/project-branches.pt
  lib/lp/code/tests/test_project.py
  lib/lp/registry/interfaces/projectgroup.py
  lib/lp/registry/model/projectgroup.py

[Edwin Grubbs (edwin-grubbs)]

Hi JC,

This branch looks good. I just have one comment below.

-Edwin

>=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
>--- lib/lp/code/browser/tests/test_branchlisting.py 2010-08-24 02:21:50 +0000
>+++ lib/lp/code/browser/tests/test_branchlisting.py 2010-10-14 14:37:22 +0000
>@@ -421,5 +421,33 @@
> self.assertIs(None, branches)
>
>
>+class TestProjectBranchListing(TestCaseWithFactory):
>+
>+ layer = DatabaseFunctionalLayer
>+
>+ def setUp(self):
>+ super(TestProjectBranchListing, self).setUp()
>+ self.project = self.factory.makeProject()
>+ self.product = self.factory.makeProduct(project=self.project)
>+
>+ def test_no_branches_gets_message_not_listing(self):
>+ # If there are no product branches on the project's products, then
>+ # the view shows the no code hosting message instead of a listing.
>+ browser = self.getUserBrowser(
>+ canonical_url(self.project, rootsite='code'))
>+ expected_text = ("None of %s's projects are using Launchpad to host "
>+ "code." % self.project.displayname)
>+ no_branch_div = find_tag_by_id(browser.contents, "no-branchtable")
>+ text = extract_text(no_branch_div)
>
>
>You're missing an assertEqual or assertIn here.
>
>
>+
>+ def test_branches_get_listing(self):
>+ # If a product has a branch, then the project view has a branch
>+ # listing.
>+ branch = self.factory.makeProductBranch(product=self.product)
>+ browser = self.getUserBrowser(
>+ canonical_url(self.project, rootsite='code'))
>+ table = find_tag_by_id(browser.contents, "branchtable")
>+ self.assertIsNot(None, table)
>+
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)
>

review: Approve (code)

[j.c.sackett (jcsackett)]

On Oct 14, 2010, at 11:07 AM, Edwin Grubbs wrote:

> Review: Approve code
> Hi JC,
>
> This branch looks good. I just have one comment below.
>
> -Edwin
>
>> === modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
>> --- lib/lp/code/browser/tests/test_branchlisting.py 2010-08-24 02:21:50 +0000
>> +++ lib/lp/code/browser/tests/test_branchlisting.py 2010-10-14 14:37:22 +0000
>> @@ -421,5 +421,33 @@
>> self.assertIs(None, branches)
>>
>>
>> +class TestProjectBranchListing(TestCaseWithFactory):
>> +
>> + layer = DatabaseFunctionalLayer
>> +
>> + def setUp(self):
>> + super(TestProjectBranchListing, self).setUp()
>> + self.project = self.factory.makeProject()
>> + self.product = self.factory.makeProduct(project=self.project)
>> +
>> + def test_no_branches_gets_message_not_listing(self):
>> + # If there are no product branches on the project's products, then
>> + # the view shows the no code hosting message instead of a listing.
>> + browser = self.getUserBrowser(
>> + canonical_url(self.project, rootsite='code'))
>> + expected_text = ("None of %s's projects are using Launchpad to host "
>> + "code." % self.project.displayname)
>> + no_branch_div = find_tag_by_id(browser.contents, "no-branchtable")
>> + text = extract_text(no_branch_div)
>>
>>
>> You're missing an assertEqual or assertIn here.

Why yes. Yes I am. I'm not sure how that got deleted, but the fix is committed.

Thanks, Edwin.

[Guilherme Salgado (salgado)]

Hi Jon,

This looks good to me; I only have a couple comments below.

 review approve ui*

On Wed, 2010-10-13 at 18:49 +0000, j.c.sackett wrote:
>
> Preimplementation talk
> ======================
>
> Spoke with Edwin Grubbs. We brought up the possiblity of linking out
> to all the products for the group so someone could configure the
> products, but to do so sanely (i.e. only show the links for products
> the viewer can configure) requires doing database checks for every
> product; this could lead to death by SQL timeouts.

I think that a project group that doesn't have a single project using LP
for hosting code is unlikely to have more than a handful projects, so I
think we could afford these extra SQL queries in the name of a better
user experience.

That said, it's fine if you want to implement that in a separate branch,
as I might be wrong.

> === modified file 'lib/lp/code/templates/project-branches.pt'
> --- lib/lp/code/templates/project-branches.pt 2009-09-17 00:27:40 +0000
> +++ lib/lp/code/templates/project-branches.pt 2010-10-13 18:43:40 +0000
> @@ -17,8 +17,17 @@
> tal:condition="link/enabled"
> tal:content="structure link/render" />
> </div>
> -
> - <tal:branchlisting content="structure branches/@@+branch-listing" />
> + <tal:no-branches
> + condition="not:context/has_branches">
> + <div id="no-branchtable">
> + <strong>None of <tal:project replace="context/displayname"/>'s

I've read somewhere
(<http://en.wikipedia.org/wiki/Apostrophe#cite_note-13>, I think) that
to form the possessive we should add an 's regardless of the final
consonant, and your change is in line with that, but I'm not sure that's
the preferred style in Launchpad (in case we have one), so I'd like to
know what Curtis thinks.

> + projects are using Launchpad to host their code.</strong>
> + </div>
> + </tal:no-branches>

> === modified file 'lib/lp/registry/model/projectgroup.py'
> --- lib/lp/registry/model/projectgroup.py 2010-10-07 13:26:53 +0000
> +++ lib/lp/registry/model/projectgroup.py 2010-10-13 18:43:40 +0000
> @@ -222,6 +222,10 @@
> """See `IProjectGroup`."""
> return self.translatables().count() != 0
>
> + def has_branches(self):
> + """ See `IProjectGroup`."""
> + return self.getBranches().count() != 0

Can you use .is_empty() instead of .count() here? It is faster and we
don't really need the count here.
(I know this was supposed to be a UI review, but I thought it was worth
mentioning)

> +
> def _getBaseQueryAndClauseTablesForQueryingSprints(self):
> query = """
> Product.project = %s
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

review: Approve (ui*)

[j.c.sackett (jcsackett)]

> I think that a project group that doesn't have a single project using LP
> for hosting code is unlikely to have more than a handful projects, so I
> think we could afford these extra SQL queries in the name of a better
> user experience.
>
> That said, it's fine if you want to implement that in a separate branch,
> as I might be wrong.

I can buy your justification; I've added in that feature and attached an incremental.

>> === modified file 'lib/lp/registry/model/projectgroup.py'
>> --- lib/lp/registry/model/projectgroup.py 2010-10-07 13:26:53 +0000
>> +++ lib/lp/registry/model/projectgroup.py 2010-10-13 18:43:40 +0000
>> @@ -222,6 +222,10 @@
>> """See `IProjectGroup`."""
>> return self.translatables().count() != 0
>>
>> + def has_branches(self):
>> + """ See `IProjectGroup`."""
>> + return self.getBranches().count() != 0
>
> Can you use .is_empty() instead of .count() here? It is faster and we
> don't really need the count here.
> (I know this was supposed to be a UI review, but I thought it was worth
> mentioning)

Taken care of.

[Guilherme Salgado (salgado)]

Thanks for implementing my suggestion, Jon. I think there's just one extra trivial change we can do to make for an even nicer user experience: link straight to the page where code hosting is configured rather than to the project's home page. You can easily do that with the following snippet, which has even been tested. ;)

<a tal:attributes="href product/@@+code-index/configure_codehosting/fmt:url" tal:content="product/title" />

[Curtis Hovey (sinzui)]

I think this message is inaccurate.
    "None of <Project Groups>"'s are using Launchpad to host their code

Lots of contributors set the branch of project to be an import. The project is not using Launchpad to host code in the common meaning of "host". The user reading the message needs to know that none of the sub project's have register branches. Maybe I mean "told Launchpad where code is hosted" The main issue here is if I am an Ubuntu Contributor looking for the upstream branches, I do not expect Launchpad to host them, and as the group owner I should not think the projects must use Launchpad to enable the view. Using the project unknown code hosting message as a guide:
    "Launchpad does not know where any of <Project Groups>"'s projects host their code.

review: Needs Information (ui)

[j.c.sackett (jcsackett)]

> Thanks for implementing my suggestion, Jon. I think there's just one extra
> trivial change we can do to make for an even nicer user experience: link
> straight to the page where code hosting is configured rather than to the
> project's home page. You can easily do that with the following snippet, which
> has even been tested. ;)
>
> <a tal:attributes="href product/@@+code-index/configure_codehosting/fmt:url"
> tal:content="product/title" />

Sounds good; I've made that change.

[j.c.sackett (jcsackett)]

> "Launchpad does not know where any of <Project Groups>"'s projects host
> their code.

I buy this, and have pushed up changes.

[Curtis Hovey (sinzui)]

Thanks to revising the text.

review: Approve (ui)

=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
--- lib/lp/code/browser/tests/test_branchlisting.py	2010-08-24 02:21:50 +0000
+++ lib/lp/code/browser/tests/test_branchlisting.py	2010-10-20 18:33:49 +0000
@@ -44,6 +44,7 @@
 from lp.testing import (
     BrowserTestCase,
     login_person,
+    normalize_whitespace,
     person_logged_in,
     TestCase,
     TestCaseWithFactory,
@@ -421,5 +422,37 @@
         self.assertIs(None, branches)
 
 
+class TestProjectBranchListing(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProjectBranchListing, self).setUp()
+        self.project = self.factory.makeProject()
+        self.product = self.factory.makeProduct(project=self.project)
+
+    def test_no_branches_gets_message_not_listing(self):
+        # If there are no product branches on the project's products, then
+        # the view shows the no code hosting message instead of a listing.
+        browser = self.getUserBrowser(
+            canonical_url(self.project, rootsite='code'))
+        displayname = self.project.displayname
+        expected_text = normalize_whitespace(
+                            ("Launchpad does not know where any of %s's "
+                             "projects host their code." % displayname))
+        no_branch_div = find_tag_by_id(browser.contents, "no-branchtable")
+        text = normalize_whitespace(extract_text(no_branch_div))
+        self.assertEqual(expected_text, text)
+
+    def test_branches_get_listing(self):
+        # If a product has a branch, then the project view has a branch
+        # listing.
+        branch = self.factory.makeProductBranch(product=self.product)
+        browser = self.getUserBrowser(
+            canonical_url(self.project, rootsite='code'))
+        table = find_tag_by_id(browser.contents, "branchtable")
+        self.assertIsNot(None, table)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/stories/branches/xx-project-branches.txt'
--- lib/lp/code/stories/branches/xx-project-branches.txt	2010-05-13 16:22:19 +0000
+++ lib/lp/code/stories/branches/xx-project-branches.txt	2010-10-20 18:33:49 +0000
@@ -38,6 +38,8 @@
     >>> browser.open('http://code.launchpad.dev/aaa')
     >>> print browser.title
     Code : the Test Project
-    >>> message = find_tag_by_id(browser.contents, 'no-branch-message')
+    >>> message = find_tag_by_id(browser.contents, 'no-branchtable')
     >>> print extract_text(message)
-    There are no branches registered for the Test Project in Launchpad...
+    Launchpad does not know where any of
+    the Test Project's
+    projects host their code.

=== modified file 'lib/lp/code/templates/project-branches.pt'
--- lib/lp/code/templates/project-branches.pt	2009-09-17 00:27:40 +0000
+++ lib/lp/code/templates/project-branches.pt	2010-10-20 18:33:49 +0000
@@ -17,8 +17,33 @@
              tal:condition="link/enabled"
              tal:content="structure link/render" />
       </div>
-
-      <tal:branchlisting content="structure branches/@@+branch-listing" />
+      <tal:no-branches
+        condition="not:context/has_branches">
+        <div id="no-branchtable">
+          <p>
+            <strong>Launchpad does not know where any of
+            <tal:project replace="context/displayname"/>'s
+            projects host their code.</strong>
+          </p>
+          <div tal:define="products context/getConfigurableProducts">
+          <p id="projectgroup-products"
+             tal:condition="products">
+            You can set up code hosting for the following projects that
+            are part of <tal:project replace="context/displayname"/>.
+          </p>
+            <ul id="product-list" tal:repeat="product products">
+              <li>
+                <a tal:attributes="href product/@@+code-index/configure_codehosting/fmt:url"
+                   tal:content="product/title" />
+              </li>  
+            </ul>
+        </div>
+        </div>
+      </tal:no-branches>
+      <tal:has-branches
+        condition="context/has_branches">
+        <tal:branchlisting content="structure branches/@@+branch-listing" />
+      </tal:has-branches>
     </div>
 
   </body>

=== added file 'lib/lp/code/tests/test_project.py'
--- lib/lp/code/tests/test_project.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/tests/test_project.py	2010-10-20 18:33:49 +0000
@@ -0,0 +1,36 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for product views."""
+
+__metaclass__ = type
+
+import unittest
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.testing import TestCaseWithFactory
+
+
+class TestProjectBranches(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestProjectBranches, self).setUp()
+        self.project = self.factory.makeProject()
+        self.product = self.factory.makeProduct(project=self.project)
+
+    def test_has_branches_with_no_branches(self):
+        # If there are no product branches on the project's products, then
+        # has branches returns False.
+        self.assertFalse(self.project.has_branches())
+
+    def test_has_branches_with_branches(self):
+        # If a product has a branch, then the product's project returns
+        # true for has_branches.
+        self.factory.makeProductBranch(product=self.product)
+        self.assertTrue(self.project.has_branches())
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/registry/interfaces/projectgroup.py'
--- lib/lp/registry/interfaces/projectgroup.py	2010-10-07 13:26:53 +0000
+++ lib/lp/registry/interfaces/projectgroup.py	2010-10-20 18:33:49 +0000
@@ -315,6 +315,9 @@
     def getProduct(name):
         """Get a product with name `name`."""
 
+    def getConfigurableProducts():
+        """Get all products that can be edited by user."""
+
     def translatables():
         """Return an iterator over products that have resources translatables.
 
@@ -325,6 +328,10 @@
         """Return a boolean showing the existance of translatables products.
         """
 
+    def has_branches():
+        """Return a boolean showing the existance of products with branches.
+        """
+
     def hasProducts():
         """Returns True if a project has products associated with it, False
         otherwise.

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2010-10-13 03:24:29 +0000
+++ lib/lp/registry/model/projectgroup.py	2010-10-20 18:33:49 +0000
@@ -42,6 +42,7 @@
     IHasLogo,
     IHasMugshot,
     )
+from canonical.launchpad.webapp.authorization import check_permission
 from lp.answers.interfaces.faqcollection import IFAQCollection
 from lp.answers.interfaces.questioncollection import (
     ISearchableByQuestionOwner,
@@ -174,6 +175,10 @@
     def getProduct(self, name):
         return Product.selectOneBy(project=self, name=name)
 
+    def getConfigurableProducts(self):
+        return [product for product in self.products
+                if check_permission('launchpad.Edit', product)]
+                    
     @property
     def drivers(self):
         """See `IHasDrivers`."""
@@ -228,6 +233,10 @@
         # return not self.translatables().is_empty()
         return self.translatables().count() != 0
 
+    def has_branches(self):
+        """ See `IProjectGroup`."""
+        return not self.getBranches().is_empty()
+
     def _getBaseQueryAndClauseTablesForQueryingSprints(self):
         query = """
             Product.project = %s