Launchpad itself

Merge lp:~jcsackett/launchpad/branch-collection-links-652126 into lp:launchpad

  1. branch-collection-links-652126
  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: 11751
Proposed branch: lp:~jcsackett/launchpad/branch-collection-links-652126
Merge into: lp:launchpad
Diff against target: 258 lines (+70/-85)
7 files modified
lib/lp/code/browser/configure.zcml (+0/-6)
lib/lp/code/browser/tests/test_product.py (+2/-1)
lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt (+3/-3)
lib/lp/code/stories/branches/xx-product-branches.txt (+6/-13)
lib/lp/code/templates/product-branches.pt (+4/-1)
lib/lp/code/templates/product-portlet-codestatistics-content.pt (+0/-55)
lib/lp/code/templates/product-portlet-codestatistics.pt (+55/-6)
To merge this branch: bzr merge lp:~jcsackett/launchpad/branch-collection-links-652126
Reviewer Review Type Date Requested Status
Paul Hummer (community) code ui Approve
Guilherme Salgado ui Pending
Review via email: mp+38456@code.launchpad.net

Commit message

Moves statistic information on the product branches view into the main page and out of the sidebar, where it didn't really fit.

Description of the change

Summary
=======

This branch deals with bad expectations on product branches. There's a set of statistical information in a side portlet that has some links, but not all links. Given the side portlets tend to be for actions, it seems odd that the remaining statistics aren't actionable.

To deal with this, the branch moves the stats into text on the main page, keeping the useful information, but avoiding breaking the expectations.

There is still one link to active-reviews in the new inline text; I'm open to the suggestion that active reviews still belong in a side portlet.

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

Move the information in the sidebar into the main page, and eliminate the problematic part of the sidebar.

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

Spoke with Paul Hummer. Discussed intent of the statistics portlet and concluded it could be moved out of the sidebar portlet and into the main page. Additionally discussed not linking to active reviews if there are no active reviews.

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

As in proposed. In the process of moving the portlet out of the side bar and into the main page, the two portlets showing the statistics were merged into one, since the existance of two portlets used *only* to present that data seemed unnecessary at best.

Tests
=====

bin/test -vvcm lp.code.browser.tests.test_product
bin/test -t product-branches

Right now product-branches isn't passing b/c the want and received text are disagreeing on line breaks; I will fix this before submitting, but wanted to get this moving forward on review. If you run product-branches, you will see the text agrees, but the line breaks are failing. I'm investigating clean ways to improve the test.

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

The original appearance is here: http://people.canonical.com/~jc/images/oldstats.png
The new appearance is here: http://people.canonical.com/~jc/images/newstats.png

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/stories/branches/xx-product-branches.txt
  lib/lp/code/templates/product-branches.pt

./lib/lp/code/stories/branches/xx-product-branches.txt
      41: want exceeds 78 characters.

There doesn't seem to be a clean way to shorten the want in this test.

To post a comment you must log in.
Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> jcsackett, the code looks fine.
<rockstar> jcsackett, as far as UI though, I think you should put the commit count, branch count, and user count on the same line.
<rockstar> Active reviews should be its own little section.
<jcsackett> rockstar: i tried that, but you end up with either a really weird block of text, or it looks like the branches and users *also* occured in the commit time period.
<jcsackett> rockstar: you think move active reviews back into its own side thing?
<rockstar> jcsackett, X branches, owned by X people. There were X commits in the last X period.
<rockstar> jcsackett, the intention of the X commits in the last X period is to show how active the project is.
<jcsackett> rockstar: yeah, that just looked wierd to me, but i'll deal with the text block instead of dividing it up as it is.
<rockstar> It doesn't do a good job, but I think laying it out that way would be better.
<jcsackett> rockstar: regarding active reviews; side thingy, or just a separate line?
<rockstar> Active reviews need to be obvious. Putting text near it makes it hard to identify.
<rockstar> jcsackett, separate line should be fine.
<jcsackett> rockstar: dig.
<rockstar> jcsackett, for bonus points, it doesn't show if there are now active reviews.
<jcsackett> i'll make those changes.
<jcsackett> rockstar: i'll look into it. didn't want this growing beyond the scope of bridging the gap, but if it's easy to throw in (and it should be) i'll hit it.
<rockstar> jcsackett, yeah. UI reviews often cause feature creep. Truthfully though, I think it stays in line with the original purpose of the bug.
<jcsackett> rockstar: dig.
<jcsackett> i'll switch that up and then ping you when i've made the changes, rockstar.
<rockstar> jcsackett, great, thanks.

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

I've made the changes we discussed on IRC.

The appearance with reviews now is at http://people.canonical.com/~jc/images/withreviews.png

Without: http://people.canonical.com/~jc/images/noreviews.png

Revision history for this message
Paul Hummer (rockstar) wrote :

Thanks for making these changes!

review: Approve (code ui)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2010-10-06 18:53:53 +0000
+++ lib/lp/code/browser/configure.zcml 2010-10-19 14:12:52 +0000
@@ -974,12 +974,6 @@
974 permission="zope.Public"974 permission="zope.Public"
975 name="+portlet-product-codestatistics"975 name="+portlet-product-codestatistics"
976 template="../templates/product-portlet-codestatistics.pt"/>976 template="../templates/product-portlet-codestatistics.pt"/>
977 <browser:page
978 for="lp.registry.interfaces.product.IProduct"
979 class="lp.code.browser.branchlisting.ProductBranchStatisticsView"
980 permission="zope.Public"
981 name="+portlet-product-codestatistics-content"
982 template="../templates/product-portlet-codestatistics-content.pt"/>
983 <browser:page977 <browser:page
984 for="lp.registry.interfaces.product.IProduct"978 for="lp.registry.interfaces.product.IProduct"
985 layer="lp.code.publisher.CodeLayer"979 layer="lp.code.publisher.CodeLayer"
986980
=== modified file 'lib/lp/code/browser/tests/test_product.py'
--- lib/lp/code/browser/tests/test_product.py 2010-10-15 22:37:23 +0000
+++ lib/lp/code/browser/tests/test_product.py 2010-10-19 14:12:52 +0000
@@ -255,7 +255,8 @@
255 login(ANONYMOUS)255 login(ANONYMOUS)
256 text = extract_text(find_tag_by_id(256 text = extract_text(find_tag_by_id(
257 browser.contents, 'branch-count-summary'))257 browser.contents, 'branch-count-summary'))
258 expected = "1 Active branch owned by 1 person.*"258 expected = ("%s has 1 active branch owned by 1"
259 " person." % product.displayname)
259 self.assertTextMatchesExpressionIgnoreWhitespace(expected, text)260 self.assertTextMatchesExpressionIgnoreWhitespace(expected, text)
260261
261 # The code page does not set robots to noindex, nofollow.262 # The code page does not set robots to noindex, nofollow.
262263
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt 2010-09-28 19:25:54 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt 2010-10-19 14:12:52 +0000
@@ -62,12 +62,12 @@
62the number of active reviews and approved merges.62the number of active reviews and approved merges.
6363
64 >>> browser.open('http://code.launchpad.dev/fooix')64 >>> browser.open('http://code.launchpad.dev/fooix')
65 >>> print_tag_with_id(browser.contents, 'merge-counts')65 >>> print_tag_with_id(browser.contents, 'active-review-count')
66 3 Active reviews66 Fooix has 3 active reviews...
6767
68The 'active reviews' text links to the active reviews page.68The 'active reviews' text links to the active reviews page.
6969
70 >>> browser.getLink('Active reviews').click()70 >>> browser.getLink('active reviews').click()
71 >>> print browser.title71 >>> print browser.title
72 Active code reviews for Fooix...72 Active code reviews for Fooix...
7373
7474
=== modified file 'lib/lp/code/stories/branches/xx-product-branches.txt'
--- lib/lp/code/stories/branches/xx-product-branches.txt 2010-10-15 22:37:23 +0000
+++ lib/lp/code/stories/branches/xx-product-branches.txt 2010-10-19 14:12:52 +0000
@@ -252,6 +252,7 @@
252The text that is shown giving a summary of the number of branches252The text that is shown giving a summary of the number of branches
253shows correct singular and plural forms.253shows correct singular and plural forms.
254254
255 >>> from lp.testing import normalize_whitespace
255 >>> def get_stats_portlet(browser):256 >>> def get_stats_portlet(browser):
256 ... return find_tag_by_id(257 ... return find_tag_by_id(
257 ... browser.contents,258 ... browser.contents,
@@ -262,13 +263,11 @@
262 ... if portlet is None:263 ... if portlet is None:
263 ... print 'None'264 ... print 'None'
264 ... else:265 ... else:
265 ... print extract_text(portlet)266 ... print normalize_whitespace(extract_text(portlet))
266267
267 >>> setup_code_hosting('gnome-terminal')268 >>> setup_code_hosting('gnome-terminal')
268 >>> print_portlet('gnome-terminal')269 >>> print_portlet('gnome-terminal')
269 0 Active reviews270 GNOME Terminal has 9 active branches owned by 2 people and 2 teams. There were 0 commits in the last month.
270 9 Active branches owned by 2 people and 2 teams
271 0 Commits in the last month
272271
273 >>> from lp.testing import ANONYMOUS, login, logout272 >>> from lp.testing import ANONYMOUS, login, logout
274 >>> login(ANONYMOUS)273 >>> login(ANONYMOUS)
@@ -277,23 +276,17 @@
277 >>> logout()276 >>> logout()
278 >>> setup_code_hosting('fooix')277 >>> setup_code_hosting('fooix')
279 >>> print_portlet('fooix')278 >>> print_portlet('fooix')
280 0 Active reviews279 Fooix has 2 active branches owned by 2 people. There were 0 commits in the last month.
281 2 Active branches owned by 2 people
282 0 Commits in the last month
283280
284 >>> print_portlet('evolution')281 >>> print_portlet('evolution')
285 0 Active reviews282 Evolution has 3 active branches owned by 1 person and 1 team. There were 0 commits in the last month.
286 3 Active branches owned by 1 person and 1 team
287 0 Commits in the last month
288283
289 >>> login(ANONYMOUS)284 >>> login(ANONYMOUS)
290 >>> dinky = factory.makeProduct('dinky')285 >>> dinky = factory.makeProduct('dinky')
291 >>> logout()286 >>> logout()
292 >>> setup_code_hosting('dinky')287 >>> setup_code_hosting('dinky')
293 >>> print_portlet('dinky')288 >>> print_portlet('dinky')
294 0 Active reviews289 Dinky has 1 active branch owned by 1 person. There were 0 commits in the last month.
295 1 Active branch owned by 1 person
296 0 Commits in the last month
297290
298291
299Product has Branches, but none initially visible292Product has Branches, but none initially visible
300293
=== modified file 'lib/lp/code/templates/product-branches.pt'
--- lib/lp/code/templates/product-branches.pt 2010-10-08 15:57:37 +0000
+++ lib/lp/code/templates/product-branches.pt 2010-10-19 14:12:52 +0000
@@ -55,7 +55,6 @@
55 tal:content="structure link/render"></p>55 tal:content="structure link/render"></p>
56 </div>56 </div>
5757
58 <div tal:replace="structure context/@@+portlet-product-codestatistics" />
59 </div>58 </div>
60 </metal:side>59 </metal:side>
6160
@@ -63,6 +62,10 @@
6362
64 <tal:branch-summary content="structure context/@@+branch-summary" />63 <tal:branch-summary content="structure context/@@+branch-summary" />
6564
65 <tal:code-statistics
66 condition="not: view/context/codehosting_usage/enumvalue:UNKNOWN"
67 replace="structure context/@@+portlet-product-codestatistics" />
68
66 <tal:has-branches condition="view/branch_count"69 <tal:has-branches condition="view/branch_count"
67 define="branches view/branches">70 define="branches view/branches">
68 <tal:branchlisting content="structure branches/@@+branch-listing" />71 <tal:branchlisting content="structure branches/@@+branch-listing" />
6972
=== removed file 'lib/lp/code/templates/product-portlet-codestatistics-content.pt'
--- lib/lp/code/templates/product-portlet-codestatistics-content.pt 2010-09-28 19:25:54 +0000
+++ lib/lp/code/templates/product-portlet-codestatistics-content.pt 1970-01-01 00:00:00 +0000
@@ -1,55 +0,0 @@
1<tal:portlet-product-codestatistics-content
2 xmlns:tal="http://xml.zope.org/namespaces/tal"
3 xmlns:metal="http://xml.zope.org/namespaces/metal">
4
5 <tr tal:define="menu context/menu:branches" class="code-links"
6 id="merge-counts">
7 <td class="code-count"
8 tal:define="count menu/active_review_count"
9 tal:content="count" />
10 <td>
11 <tal:link
12 define="link menu/active_reviews"
13 replace="structure link/render"
14 />
15 </td>
16 </tr>
17
18 <tr class="code-links" id="branch-count-summary">
19 <td class="code-count"
20 tal:define="count view/branch_count"
21 tal:content="count" />
22 <td>
23 <tal:branches replace="view/branch_text">branches</tal:branches
24 ><tal:has-branches condition="view/branch_count">
25 owned by
26 <tal:individuals condition="view/person_owner_count">
27 <tal:owners content="view/person_owner_count">42</tal:owners>
28 <tal:people replace="view/person_text">people</tal:people
29 ></tal:individuals
30 ><tal:teams condition="view/team_owner_count">
31 <tal:individuals condition="view/person_owner_count">
32 and
33 </tal:individuals>
34 <tal:toc content="view/team_owner_count">1</tal:toc>
35 <tal:people replace="view/team_text">team</tal:people
36 ></tal:teams></tal:has-branches>
37 </td>
38 </tr>
39
40 <tr class="code-links">
41 <td class="code-count"
42 tal:define="count view/commit_count"
43 tal:content="count" />
44 <td>
45 <tal:commits replace="view/commit_text">commits</tal:commits>
46 <tal:has-committers condition="view/committer_count">
47 by
48 <tal:cc content="view/committer_count">4</tal:cc>
49 <tal:people replace="view/committer_text">people</tal:people>
50 </tal:has-committers>
51 in the last month
52 </td>
53 </tr>
54
55</tal:portlet-product-codestatistics-content>
560
=== modified file 'lib/lp/code/templates/product-portlet-codestatistics.pt'
--- lib/lp/code/templates/product-portlet-codestatistics.pt 2010-09-22 18:54:36 +0000
+++ lib/lp/code/templates/product-portlet-codestatistics.pt 2010-10-19 14:12:52 +0000
@@ -2,10 +2,59 @@
2 xmlns:tal="http://xml.zope.org/namespaces/tal"2 xmlns:tal="http://xml.zope.org/namespaces/tal"
3 xmlns:metal="http://xml.zope.org/namespaces/metal"3 xmlns:metal="http://xml.zope.org/namespaces/metal"
4 xmlns:i18n="http://xml.zope.org/namespaces/i18n"4 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
5 class="portlet" id="portlet-product-codestatistics">5 id="portlet-product-codestatistics">
66
7 <table class="code-links">7 <p id="active-review-count"
8 <tbody id="portlet-product-codestatistics"8 tal:define="count context/menu:branches/active_review_count;
9 tal:content="structure context/@@+portlet-product-codestatistics-content" />9 link context/menu:branches/active_reviews"
10 </table>10 tal:condition="python: count &gt; 0">
11 <tal:project replace="context/displayname"/> has
12 <tal:active-count replace="count"/>
13 <tal:link replace="structure python: link.render().lower()"/>.
14 </p>
15
16 <!--branches-->
17 <p>
18 <tal:comment condition="nothing">
19 The bad breaks in the following block are to force the period to be
20 in the right place.
21 </tal:comment>
22 <span tal:define="count view/branch_count;"
23 id="branch-count-summary">
24 <tal:project replace="context/displayname"/> has
25 <tal:branch-count replace="count"/>
26 <tal:branches replace="python: view.branch_text.lower()">
27 branches
28 </tal:branches
29 ><tal:has-branches condition="view/branch_count">
30 owned by
31 <tal:individuals condition="view/person_owner_count">
32 <tal:owners content="view/person_owner_count">42</tal:owners>
33 <tal:people replace="view/person_text">people</tal:people
34 ></tal:individuals
35 ><tal:teams condition="view/team_owner_count">
36 <tal:individuals condition="view/person_owner_count">
37 and
38 </tal:individuals
39 ><tal:toc content="view/team_owner_count">1</tal:toc>
40 <tal:people replace="view/team_text">team</tal:people
41 ></tal:teams></tal:has-branches>.
42 </span>
43
44 <!--commits-->
45 <span id="commits"
46 tal:define="count view/commit_count">
47 There were
48 <tal:commit-count replace="count"/>
49 <tal:commits replace="python: view.commit_text.lower()">
50 commits
51 </tal:commits>
52 <tal:has-committers condition="view/committer_count">
53 by
54 <tal:cc content="view/committer_count">4</tal:cc>
55 <tal:people replace="view/committer_text">people</tal:people>
56 </tal:has-committers>
57 in the last month.
58 </span>
59 </p>
11</div>60</div>