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
1=== modified file 'lib/lp/code/templates/project-branches.pt'
2--- lib/lp/code/templates/project-branches.pt 2010-10-13 17:57:41 +0000
3+++ lib/lp/code/templates/project-branches.pt 2010-10-15 20:13:03 +0000
4@@ -20,8 +20,22 @@
5 <tal:no-branches
6 condition="not:context/has_branches">
7 <div id="no-branchtable">
8- <strong>None of <tal:project replace="context/displayname"/>'s
9- projects are using Launchpad to host their code.</strong>
10+ <p>
11+ <strong>None of <tal:project replace="context/displayname"/>'s
12+ projects are using Launchpad to host their code.</strong>
13+ </p>
14+ <div tal:define="products context/getConfigurableProducts">
15+ <p id="projectgroup-products"
16+ tal:condition="products">
17+ You can set up code hosting for the following projects that
18+ are part of <tal:project replace="context/displayname"/>.
19+ </p>
20+ <ul id="product-list" tal:repeat="product products">
21+ <li>
22+ <a tal:replace="structure product/fmt:link"/>
23+ </li>
24+ </ul>
25+ </div>
26 </div>
27 </tal:no-branches>
28 <tal:has-branches
29
30=== modified file 'lib/lp/registry/interfaces/projectgroup.py'
31--- lib/lp/registry/interfaces/projectgroup.py 2010-10-15 16:23:38 +0000
32+++ lib/lp/registry/interfaces/projectgroup.py 2010-10-15 20:19:13 +0000
33@@ -315,6 +315,9 @@
34 def getProduct(name):
35 """Get a product with name `name`."""
36
37+ def getConfigurableProducts(self):
38+ """Get all products that can be edited by user."""
39+
40 def translatables():
41 """Return an iterator over products that have resources translatables.
42
43
44=== modified file 'lib/lp/registry/model/projectgroup.py'
45--- lib/lp/registry/model/projectgroup.py 2010-10-15 16:23:38 +0000
46+++ lib/lp/registry/model/projectgroup.py 2010-10-15 20:19:26 +0000
47@@ -42,6 +42,7 @@
48 IHasLogo,
49 IHasMugshot,
50 )
51+from canonical.launchpad.webapp.authorization import check_permission
52 from lp.answers.interfaces.faqcollection import IFAQCollection
53 from lp.answers.interfaces.questioncollection import (
54 ISearchableByQuestionOwner,
55@@ -174,6 +175,10 @@
56 def getProduct(self, name):
57 return Product.selectOneBy(project=self, name=name)
58
59+ def getConfigurableProducts(self):
60+ return [product for product in self.products
61+ if check_permission('launchpad.Edit', product)]
62+
63 @property
64 def drivers(self):
65 """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
1=== modified file 'lib/lp/code/browser/tests/test_branchlisting.py'
2--- lib/lp/code/browser/tests/test_branchlisting.py 2010-08-24 02:21:50 +0000
3+++ lib/lp/code/browser/tests/test_branchlisting.py 2010-10-20 18:33:49 +0000
4@@ -44,6 +44,7 @@
5 from lp.testing import (
6 BrowserTestCase,
7 login_person,
8+ normalize_whitespace,
9 person_logged_in,
10 TestCase,
11 TestCaseWithFactory,
12@@ -421,5 +422,37 @@
13 self.assertIs(None, branches)
14
15
16+class TestProjectBranchListing(TestCaseWithFactory):
17+
18+ layer = DatabaseFunctionalLayer
19+
20+ def setUp(self):
21+ super(TestProjectBranchListing, self).setUp()
22+ self.project = self.factory.makeProject()
23+ self.product = self.factory.makeProduct(project=self.project)
24+
25+ def test_no_branches_gets_message_not_listing(self):
26+ # If there are no product branches on the project's products, then
27+ # the view shows the no code hosting message instead of a listing.
28+ browser = self.getUserBrowser(
29+ canonical_url(self.project, rootsite='code'))
30+ displayname = self.project.displayname
31+ expected_text = normalize_whitespace(
32+ ("Launchpad does not know where any of %s's "
33+ "projects host their code." % displayname))
34+ no_branch_div = find_tag_by_id(browser.contents, "no-branchtable")
35+ text = normalize_whitespace(extract_text(no_branch_div))
36+ self.assertEqual(expected_text, text)
37+
38+ def test_branches_get_listing(self):
39+ # If a product has a branch, then the project view has a branch
40+ # listing.
41+ branch = self.factory.makeProductBranch(product=self.product)
42+ browser = self.getUserBrowser(
43+ canonical_url(self.project, rootsite='code'))
44+ table = find_tag_by_id(browser.contents, "branchtable")
45+ self.assertIsNot(None, table)
46+
47+
48 def test_suite():
49 return unittest.TestLoader().loadTestsFromName(__name__)
50
51=== modified file 'lib/lp/code/stories/branches/xx-project-branches.txt'
52--- lib/lp/code/stories/branches/xx-project-branches.txt 2010-05-13 16:22:19 +0000
53+++ lib/lp/code/stories/branches/xx-project-branches.txt 2010-10-20 18:33:49 +0000
54@@ -38,6 +38,8 @@
55 >>> browser.open('http://code.launchpad.dev/aaa')
56 >>> print browser.title
57 Code : the Test Project
58- >>> message = find_tag_by_id(browser.contents, 'no-branch-message')
59+ >>> message = find_tag_by_id(browser.contents, 'no-branchtable')
60 >>> print extract_text(message)
61- There are no branches registered for the Test Project in Launchpad...
62+ Launchpad does not know where any of
63+ the Test Project's
64+ projects host their code.
65
66=== modified file 'lib/lp/code/templates/project-branches.pt'
67--- lib/lp/code/templates/project-branches.pt 2009-09-17 00:27:40 +0000
68+++ lib/lp/code/templates/project-branches.pt 2010-10-20 18:33:49 +0000
69@@ -17,8 +17,33 @@
70 tal:condition="link/enabled"
71 tal:content="structure link/render" />
72 </div>
73-
74- <tal:branchlisting content="structure branches/@@+branch-listing" />
75+ <tal:no-branches
76+ condition="not:context/has_branches">
77+ <div id="no-branchtable">
78+ <p>
79+ <strong>Launchpad does not know where any of
80+ <tal:project replace="context/displayname"/>'s
81+ projects host their code.</strong>
82+ </p>
83+ <div tal:define="products context/getConfigurableProducts">
84+ <p id="projectgroup-products"
85+ tal:condition="products">
86+ You can set up code hosting for the following projects that
87+ are part of <tal:project replace="context/displayname"/>.
88+ </p>
89+ <ul id="product-list" tal:repeat="product products">
90+ <li>
91+ <a tal:attributes="href product/@@+code-index/configure_codehosting/fmt:url"
92+ tal:content="product/title" />
93+ </li>
94+ </ul>
95+ </div>
96+ </div>
97+ </tal:no-branches>
98+ <tal:has-branches
99+ condition="context/has_branches">
100+ <tal:branchlisting content="structure branches/@@+branch-listing" />
101+ </tal:has-branches>
102 </div>
103
104 </body>
105
106=== added file 'lib/lp/code/tests/test_project.py'
107--- lib/lp/code/tests/test_project.py 1970-01-01 00:00:00 +0000
108+++ lib/lp/code/tests/test_project.py 2010-10-20 18:33:49 +0000
109@@ -0,0 +1,36 @@
110+# Copyright 2010 Canonical Ltd. This software is licensed under the
111+# GNU Affero General Public License version 3 (see the file LICENSE).
112+
113+"""Tests for product views."""
114+
115+__metaclass__ = type
116+
117+import unittest
118+
119+from canonical.testing.layers import DatabaseFunctionalLayer
120+from lp.testing import TestCaseWithFactory
121+
122+
123+class TestProjectBranches(TestCaseWithFactory):
124+
125+ layer = DatabaseFunctionalLayer
126+
127+ def setUp(self):
128+ super(TestProjectBranches, self).setUp()
129+ self.project = self.factory.makeProject()
130+ self.product = self.factory.makeProduct(project=self.project)
131+
132+ def test_has_branches_with_no_branches(self):
133+ # If there are no product branches on the project's products, then
134+ # has branches returns False.
135+ self.assertFalse(self.project.has_branches())
136+
137+ def test_has_branches_with_branches(self):
138+ # If a product has a branch, then the product's project returns
139+ # true for has_branches.
140+ self.factory.makeProductBranch(product=self.product)
141+ self.assertTrue(self.project.has_branches())
142+
143+
144+def test_suite():
145+ return unittest.TestLoader().loadTestsFromName(__name__)
146
147=== modified file 'lib/lp/registry/interfaces/projectgroup.py'
148--- lib/lp/registry/interfaces/projectgroup.py 2010-10-07 13:26:53 +0000
149+++ lib/lp/registry/interfaces/projectgroup.py 2010-10-20 18:33:49 +0000
150@@ -315,6 +315,9 @@
151 def getProduct(name):
152 """Get a product with name `name`."""
153
154+ def getConfigurableProducts():
155+ """Get all products that can be edited by user."""
156+
157 def translatables():
158 """Return an iterator over products that have resources translatables.
159
160@@ -325,6 +328,10 @@
161 """Return a boolean showing the existance of translatables products.
162 """
163
164+ def has_branches():
165+ """Return a boolean showing the existance of products with branches.
166+ """
167+
168 def hasProducts():
169 """Returns True if a project has products associated with it, False
170 otherwise.
171
172=== modified file 'lib/lp/registry/model/projectgroup.py'
173--- lib/lp/registry/model/projectgroup.py 2010-10-13 03:24:29 +0000
174+++ lib/lp/registry/model/projectgroup.py 2010-10-20 18:33:49 +0000
175@@ -42,6 +42,7 @@
176 IHasLogo,
177 IHasMugshot,
178 )
179+from canonical.launchpad.webapp.authorization import check_permission
180 from lp.answers.interfaces.faqcollection import IFAQCollection
181 from lp.answers.interfaces.questioncollection import (
182 ISearchableByQuestionOwner,
183@@ -174,6 +175,10 @@
184 def getProduct(self, name):
185 return Product.selectOneBy(project=self, name=name)
186
187+ def getConfigurableProducts(self):
188+ return [product for product in self.products
189+ if check_permission('launchpad.Edit', product)]
190+
191 @property
192 def drivers(self):
193 """See `IHasDrivers`."""
194@@ -228,6 +233,10 @@
195 # return not self.translatables().is_empty()
196 return self.translatables().count() != 0
197
198+ def has_branches(self):
199+ """ See `IProjectGroup`."""
200+ return not self.getBranches().is_empty()
201+
202 def _getBaseQueryAndClauseTablesForQueryingSprints(self):
203 query = """
204 Product.project = %s