Launchpad itself

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

  1. projectgroup-branches-652156
  2. Merge into devel
Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 11771
Proposed branch: lp:~jcsackett/launchpad/projectgroup-branches-652156
Merge into: lp:launchpad
Diff against target: 204 lines (+116/-4)
6 files modified
lib/lp/code/browser/tests/test_branchlisting.py (+33/-0)
lib/lp/code/stories/branches/xx-project-branches.txt (+4/-2)
lib/lp/code/templates/project-branches.pt (+27/-2)
lib/lp/code/tests/test_project.py (+36/-0)
lib/lp/registry/interfaces/projectgroup.py (+7/-0)
lib/lp/registry/model/projectgroup.py (+9/-0)
To merge this branch: bzr merge lp:~jcsackett/launchpad/projectgroup-branches-652156
Reviewer Review Type Date Requested Status
Curtis Hovey (community) ui Approve
Guilherme Salgado (community) ui* Approve
Edwin Grubbs (community) code Approve
Review via email: mp+38355@code.launchpad.net

Commit message

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.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

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)
Revision history for this message
j.c.sackett (jcsackett) wrote :

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.

Revision history for this message
Guilherme Salgado (salgado) wrote :

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*)
Revision history for this message
j.c.sackett (jcsackett) wrote :

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

tmp.diff
=== modified file 'lib/lp/code/templates/project-branches.pt'
--- lib/lp/code/templates/project-branches.pt 2010-10-13 17:57:41 +0000
+++ lib/lp/code/templates/project-branches.pt 2010-10-15 20:13:03 +0000
@@ -20,8 +20,22 @@
20 <tal:no-branches20 <tal:no-branches
21 condition="not:context/has_branches">21 condition="not:context/has_branches">
22 <div id="no-branchtable">22 <div id="no-branchtable">
23 <strong>None of <tal:project replace="context/displayname"/>'s23 <p>
24 projects are using Launchpad to host their code.</strong>24 <strong>None of <tal:project replace="context/displayname"/>'s
25 projects are using Launchpad to host their code.</strong>
26 </p>
27 <div tal:define="products context/getConfigurableProducts">
28 <p id="projectgroup-products"
29 tal:condition="products">
30 You can set up code hosting for the following projects that
31 are part of <tal:project replace="context/displayname"/>.
32 </p>
33 <ul id="product-list" tal:repeat="product products">
34 <li>
35 <a tal:replace="structure product/fmt:link"/>
36 </li>
37 </ul>
38 </div>
25 </div>39 </div>
26 </tal:no-branches>40 </tal:no-branches>
27 <tal:has-branches41 <tal:has-branches
2842
=== modified file 'lib/lp/registry/interfaces/projectgroup.py'
--- lib/lp/registry/interfaces/projectgroup.py 2010-10-15 16:23:38 +0000
+++ lib/lp/registry/interfaces/projectgroup.py 2010-10-15 20:19:13 +0000
@@ -315,6 +315,9 @@
315 def getProduct(name):315 def getProduct(name):
316 """Get a product with name `name`."""316 """Get a product with name `name`."""
317317
318 def getConfigurableProducts(self):
319 """Get all products that can be edited by user."""
320
318 def translatables():321 def translatables():
319 """Return an iterator over products that have resources translatables.322 """Return an iterator over products that have resources translatables.
320323
321324
=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py 2010-10-15 16:23:38 +0000
+++ lib/lp/registry/model/projectgroup.py 2010-10-15 20:19:26 +0000
@@ -42,6 +42,7 @@
42 IHasLogo,42 IHasLogo,
43 IHasMugshot,43 IHasMugshot,
44 )44 )
45from canonical.launchpad.webapp.authorization import check_permission
45from lp.answers.interfaces.faqcollection import IFAQCollection46from lp.answers.interfaces.faqcollection import IFAQCollection
46from lp.answers.interfaces.questioncollection import (47from lp.answers.interfaces.questioncollection import (
47 ISearchableByQuestionOwner,48 ISearchableByQuestionOwner,
@@ -174,6 +175,10 @@
174 def getProduct(self, name):175 def getProduct(self, name):
175 return Product.selectOneBy(project=self, name=name)176 return Product.selectOneBy(project=self, name=name)
176177
178 def getConfigurableProducts(self):
179 return [product for product in self.products
180 if check_permission('launchpad.Edit', product)]
181
177 @property182 @property
178 def drivers(self):183 def drivers(self):
179 """See `IHasDrivers`."""184 """See `IHasDrivers`."""
Revision history for this message
Guilherme Salgado (salgado) wrote :

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" />

Revision history for this message
Curtis Hovey (sinzui) wrote :

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)
Revision history for this message
j.c.sackett (jcsackett) wrote :

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

Revision history for this message
j.c.sackett (jcsackett) wrote :

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

I buy this, and have pushed up changes.

Revision history for this message
Curtis Hovey (sinzui) wrote :

Thanks to revising the text.

review: Approve (ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
44from lp.testing import (44from lp.testing import (
45 BrowserTestCase,45 BrowserTestCase,
46 login_person,46 login_person,
47 normalize_whitespace,
47 person_logged_in,48 person_logged_in,
48 TestCase,49 TestCase,
49 TestCaseWithFactory,50 TestCaseWithFactory,
@@ -421,5 +422,37 @@
421 self.assertIs(None, branches)422 self.assertIs(None, branches)
422423
423424
425class TestProjectBranchListing(TestCaseWithFactory):
426
427 layer = DatabaseFunctionalLayer
428
429 def setUp(self):
430 super(TestProjectBranchListing, self).setUp()
431 self.project = self.factory.makeProject()
432 self.product = self.factory.makeProduct(project=self.project)
433
434 def test_no_branches_gets_message_not_listing(self):
435 # If there are no product branches on the project's products, then
436 # the view shows the no code hosting message instead of a listing.
437 browser = self.getUserBrowser(
438 canonical_url(self.project, rootsite='code'))
439 displayname = self.project.displayname
440 expected_text = normalize_whitespace(
441 ("Launchpad does not know where any of %s's "
442 "projects host their code." % displayname))
443 no_branch_div = find_tag_by_id(browser.contents, "no-branchtable")
444 text = normalize_whitespace(extract_text(no_branch_div))
445 self.assertEqual(expected_text, text)
446
447 def test_branches_get_listing(self):
448 # If a product has a branch, then the project view has a branch
449 # listing.
450 branch = self.factory.makeProductBranch(product=self.product)
451 browser = self.getUserBrowser(
452 canonical_url(self.project, rootsite='code'))
453 table = find_tag_by_id(browser.contents, "branchtable")
454 self.assertIsNot(None, table)
455
456
424def test_suite():457def test_suite():
425 return unittest.TestLoader().loadTestsFromName(__name__)458 return unittest.TestLoader().loadTestsFromName(__name__)
426459
=== 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 @@
38 >>> browser.open('http://code.launchpad.dev/aaa')38 >>> browser.open('http://code.launchpad.dev/aaa')
39 >>> print browser.title39 >>> print browser.title
40 Code : the Test Project40 Code : the Test Project
41 >>> message = find_tag_by_id(browser.contents, 'no-branch-message')41 >>> message = find_tag_by_id(browser.contents, 'no-branchtable')
42 >>> print extract_text(message)42 >>> print extract_text(message)
43 There are no branches registered for the Test Project in Launchpad...43 Launchpad does not know where any of
44 the Test Project's
45 projects host their code.
4446
=== 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 @@
17 tal:condition="link/enabled"17 tal:condition="link/enabled"
18 tal:content="structure link/render" />18 tal:content="structure link/render" />
19 </div>19 </div>
2020 <tal:no-branches
21 <tal:branchlisting content="structure branches/@@+branch-listing" />21 condition="not:context/has_branches">
22 <div id="no-branchtable">
23 <p>
24 <strong>Launchpad does not know where any of
25 <tal:project replace="context/displayname"/>'s
26 projects host their code.</strong>
27 </p>
28 <div tal:define="products context/getConfigurableProducts">
29 <p id="projectgroup-products"
30 tal:condition="products">
31 You can set up code hosting for the following projects that
32 are part of <tal:project replace="context/displayname"/>.
33 </p>
34 <ul id="product-list" tal:repeat="product products">
35 <li>
36 <a tal:attributes="href product/@@+code-index/configure_codehosting/fmt:url"
37 tal:content="product/title" />
38 </li>
39 </ul>
40 </div>
41 </div>
42 </tal:no-branches>
43 <tal:has-branches
44 condition="context/has_branches">
45 <tal:branchlisting content="structure branches/@@+branch-listing" />
46 </tal:has-branches>
22 </div>47 </div>
2348
24 </body>49 </body>
2550
=== 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 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for product views."""
5
6__metaclass__ = type
7
8import unittest
9
10from canonical.testing.layers import DatabaseFunctionalLayer
11from lp.testing import TestCaseWithFactory
12
13
14class TestProjectBranches(TestCaseWithFactory):
15
16 layer = DatabaseFunctionalLayer
17
18 def setUp(self):
19 super(TestProjectBranches, self).setUp()
20 self.project = self.factory.makeProject()
21 self.product = self.factory.makeProduct(project=self.project)
22
23 def test_has_branches_with_no_branches(self):
24 # If there are no product branches on the project's products, then
25 # has branches returns False.
26 self.assertFalse(self.project.has_branches())
27
28 def test_has_branches_with_branches(self):
29 # If a product has a branch, then the product's project returns
30 # true for has_branches.
31 self.factory.makeProductBranch(product=self.product)
32 self.assertTrue(self.project.has_branches())
33
34
35def test_suite():
36 return unittest.TestLoader().loadTestsFromName(__name__)
037
=== 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 @@
315 def getProduct(name):315 def getProduct(name):
316 """Get a product with name `name`."""316 """Get a product with name `name`."""
317317
318 def getConfigurableProducts():
319 """Get all products that can be edited by user."""
320
318 def translatables():321 def translatables():
319 """Return an iterator over products that have resources translatables.322 """Return an iterator over products that have resources translatables.
320323
@@ -325,6 +328,10 @@
325 """Return a boolean showing the existance of translatables products.328 """Return a boolean showing the existance of translatables products.
326 """329 """
327330
331 def has_branches():
332 """Return a boolean showing the existance of products with branches.
333 """
334
328 def hasProducts():335 def hasProducts():
329 """Returns True if a project has products associated with it, False336 """Returns True if a project has products associated with it, False
330 otherwise.337 otherwise.
331338
=== 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 @@
42 IHasLogo,42 IHasLogo,
43 IHasMugshot,43 IHasMugshot,
44 )44 )
45from canonical.launchpad.webapp.authorization import check_permission
45from lp.answers.interfaces.faqcollection import IFAQCollection46from lp.answers.interfaces.faqcollection import IFAQCollection
46from lp.answers.interfaces.questioncollection import (47from lp.answers.interfaces.questioncollection import (
47 ISearchableByQuestionOwner,48 ISearchableByQuestionOwner,
@@ -174,6 +175,10 @@
174 def getProduct(self, name):175 def getProduct(self, name):
175 return Product.selectOneBy(project=self, name=name)176 return Product.selectOneBy(project=self, name=name)
176177
178 def getConfigurableProducts(self):
179 return [product for product in self.products
180 if check_permission('launchpad.Edit', product)]
181
177 @property182 @property
178 def drivers(self):183 def drivers(self):
179 """See `IHasDrivers`."""184 """See `IHasDrivers`."""
@@ -228,6 +233,10 @@
228 # return not self.translatables().is_empty()233 # return not self.translatables().is_empty()
229 return self.translatables().count() != 0234 return self.translatables().count() != 0
230235
236 def has_branches(self):
237 """ See `IProjectGroup`."""
238 return not self.getBranches().is_empty()
239
231 def _getBaseQueryAndClauseTablesForQueryingSprints(self):240 def _getBaseQueryAndClauseTablesForQueryingSprints(self):
232 query = """241 query = """
233 Product.project = %s242 Product.project = %s