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
1=== modified file 'lib/lp/code/browser/configure.zcml'
2--- lib/lp/code/browser/configure.zcml 2010-10-06 18:53:53 +0000
3+++ lib/lp/code/browser/configure.zcml 2010-10-19 14:12:52 +0000
4@@ -974,12 +974,6 @@
5 permission="zope.Public"
6 name="+portlet-product-codestatistics"
7 template="../templates/product-portlet-codestatistics.pt"/>
8- <browser:page
9- for="lp.registry.interfaces.product.IProduct"
10- class="lp.code.browser.branchlisting.ProductBranchStatisticsView"
11- permission="zope.Public"
12- name="+portlet-product-codestatistics-content"
13- template="../templates/product-portlet-codestatistics-content.pt"/>
14 <browser:page
15 for="lp.registry.interfaces.product.IProduct"
16 layer="lp.code.publisher.CodeLayer"
17
18=== modified file 'lib/lp/code/browser/tests/test_product.py'
19--- lib/lp/code/browser/tests/test_product.py 2010-10-15 22:37:23 +0000
20+++ lib/lp/code/browser/tests/test_product.py 2010-10-19 14:12:52 +0000
21@@ -255,7 +255,8 @@
22 login(ANONYMOUS)
23 text = extract_text(find_tag_by_id(
24 browser.contents, 'branch-count-summary'))
25- expected = "1 Active branch owned by 1 person.*"
26+ expected = ("%s has 1 active branch owned by 1"
27+ " person." % product.displayname)
28 self.assertTextMatchesExpressionIgnoreWhitespace(expected, text)
29
30 # The code page does not set robots to noindex, nofollow.
31
32=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt'
33--- lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt 2010-09-28 19:25:54 +0000
34+++ lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt 2010-10-19 14:12:52 +0000
35@@ -62,12 +62,12 @@
36 the number of active reviews and approved merges.
37
38 >>> browser.open('http://code.launchpad.dev/fooix')
39- >>> print_tag_with_id(browser.contents, 'merge-counts')
40- 3 Active reviews
41+ >>> print_tag_with_id(browser.contents, 'active-review-count')
42+ Fooix has 3 active reviews...
43
44 The 'active reviews' text links to the active reviews page.
45
46- >>> browser.getLink('Active reviews').click()
47+ >>> browser.getLink('active reviews').click()
48 >>> print browser.title
49 Active code reviews for Fooix...
50
51
52=== modified file 'lib/lp/code/stories/branches/xx-product-branches.txt'
53--- lib/lp/code/stories/branches/xx-product-branches.txt 2010-10-15 22:37:23 +0000
54+++ lib/lp/code/stories/branches/xx-product-branches.txt 2010-10-19 14:12:52 +0000
55@@ -252,6 +252,7 @@
56 The text that is shown giving a summary of the number of branches
57 shows correct singular and plural forms.
58
59+ >>> from lp.testing import normalize_whitespace
60 >>> def get_stats_portlet(browser):
61 ... return find_tag_by_id(
62 ... browser.contents,
63@@ -262,13 +263,11 @@
64 ... if portlet is None:
65 ... print 'None'
66 ... else:
67- ... print extract_text(portlet)
68+ ... print normalize_whitespace(extract_text(portlet))
69
70 >>> setup_code_hosting('gnome-terminal')
71 >>> print_portlet('gnome-terminal')
72- 0 Active reviews
73- 9 Active branches owned by 2 people and 2 teams
74- 0 Commits in the last month
75+ GNOME Terminal has 9 active branches owned by 2 people and 2 teams. There were 0 commits in the last month.
76
77 >>> from lp.testing import ANONYMOUS, login, logout
78 >>> login(ANONYMOUS)
79@@ -277,23 +276,17 @@
80 >>> logout()
81 >>> setup_code_hosting('fooix')
82 >>> print_portlet('fooix')
83- 0 Active reviews
84- 2 Active branches owned by 2 people
85- 0 Commits in the last month
86+ Fooix has 2 active branches owned by 2 people. There were 0 commits in the last month.
87
88 >>> print_portlet('evolution')
89- 0 Active reviews
90- 3 Active branches owned by 1 person and 1 team
91- 0 Commits in the last month
92+ Evolution has 3 active branches owned by 1 person and 1 team. There were 0 commits in the last month.
93
94 >>> login(ANONYMOUS)
95 >>> dinky = factory.makeProduct('dinky')
96 >>> logout()
97 >>> setup_code_hosting('dinky')
98 >>> print_portlet('dinky')
99- 0 Active reviews
100- 1 Active branch owned by 1 person
101- 0 Commits in the last month
102+ Dinky has 1 active branch owned by 1 person. There were 0 commits in the last month.
103
104
105 Product has Branches, but none initially visible
106
107=== modified file 'lib/lp/code/templates/product-branches.pt'
108--- lib/lp/code/templates/product-branches.pt 2010-10-08 15:57:37 +0000
109+++ lib/lp/code/templates/product-branches.pt 2010-10-19 14:12:52 +0000
110@@ -55,7 +55,6 @@
111 tal:content="structure link/render"></p>
112 </div>
113
114- <div tal:replace="structure context/@@+portlet-product-codestatistics" />
115 </div>
116 </metal:side>
117
118@@ -63,6 +62,10 @@
119
120 <tal:branch-summary content="structure context/@@+branch-summary" />
121
122+ <tal:code-statistics
123+ condition="not: view/context/codehosting_usage/enumvalue:UNKNOWN"
124+ replace="structure context/@@+portlet-product-codestatistics" />
125+
126 <tal:has-branches condition="view/branch_count"
127 define="branches view/branches">
128 <tal:branchlisting content="structure branches/@@+branch-listing" />
129
130=== removed file 'lib/lp/code/templates/product-portlet-codestatistics-content.pt'
131--- lib/lp/code/templates/product-portlet-codestatistics-content.pt 2010-09-28 19:25:54 +0000
132+++ lib/lp/code/templates/product-portlet-codestatistics-content.pt 1970-01-01 00:00:00 +0000
133@@ -1,55 +0,0 @@
134-<tal:portlet-product-codestatistics-content
135- xmlns:tal="http://xml.zope.org/namespaces/tal"
136- xmlns:metal="http://xml.zope.org/namespaces/metal">
137-
138- <tr tal:define="menu context/menu:branches" class="code-links"
139- id="merge-counts">
140- <td class="code-count"
141- tal:define="count menu/active_review_count"
142- tal:content="count" />
143- <td>
144- <tal:link
145- define="link menu/active_reviews"
146- replace="structure link/render"
147- />
148- </td>
149- </tr>
150-
151- <tr class="code-links" id="branch-count-summary">
152- <td class="code-count"
153- tal:define="count view/branch_count"
154- tal:content="count" />
155- <td>
156- <tal:branches replace="view/branch_text">branches</tal:branches
157- ><tal:has-branches condition="view/branch_count">
158- owned by
159- <tal:individuals condition="view/person_owner_count">
160- <tal:owners content="view/person_owner_count">42</tal:owners>
161- <tal:people replace="view/person_text">people</tal:people
162- ></tal:individuals
163- ><tal:teams condition="view/team_owner_count">
164- <tal:individuals condition="view/person_owner_count">
165- and
166- </tal:individuals>
167- <tal:toc content="view/team_owner_count">1</tal:toc>
168- <tal:people replace="view/team_text">team</tal:people
169- ></tal:teams></tal:has-branches>
170- </td>
171- </tr>
172-
173- <tr class="code-links">
174- <td class="code-count"
175- tal:define="count view/commit_count"
176- tal:content="count" />
177- <td>
178- <tal:commits replace="view/commit_text">commits</tal:commits>
179- <tal:has-committers condition="view/committer_count">
180- by
181- <tal:cc content="view/committer_count">4</tal:cc>
182- <tal:people replace="view/committer_text">people</tal:people>
183- </tal:has-committers>
184- in the last month
185- </td>
186- </tr>
187-
188-</tal:portlet-product-codestatistics-content>
189
190=== modified file 'lib/lp/code/templates/product-portlet-codestatistics.pt'
191--- lib/lp/code/templates/product-portlet-codestatistics.pt 2010-09-22 18:54:36 +0000
192+++ lib/lp/code/templates/product-portlet-codestatistics.pt 2010-10-19 14:12:52 +0000
193@@ -2,10 +2,59 @@
194 xmlns:tal="http://xml.zope.org/namespaces/tal"
195 xmlns:metal="http://xml.zope.org/namespaces/metal"
196 xmlns:i18n="http://xml.zope.org/namespaces/i18n"
197- class="portlet" id="portlet-product-codestatistics">
198-
199- <table class="code-links">
200- <tbody id="portlet-product-codestatistics"
201- tal:content="structure context/@@+portlet-product-codestatistics-content" />
202- </table>
203+ id="portlet-product-codestatistics">
204+
205+ <p id="active-review-count"
206+ tal:define="count context/menu:branches/active_review_count;
207+ link context/menu:branches/active_reviews"
208+ tal:condition="python: count &gt; 0">
209+ <tal:project replace="context/displayname"/> has
210+ <tal:active-count replace="count"/>
211+ <tal:link replace="structure python: link.render().lower()"/>.
212+ </p>
213+
214+ <!--branches-->
215+ <p>
216+ <tal:comment condition="nothing">
217+ The bad breaks in the following block are to force the period to be
218+ in the right place.
219+ </tal:comment>
220+ <span tal:define="count view/branch_count;"
221+ id="branch-count-summary">
222+ <tal:project replace="context/displayname"/> has
223+ <tal:branch-count replace="count"/>
224+ <tal:branches replace="python: view.branch_text.lower()">
225+ branches
226+ </tal:branches
227+ ><tal:has-branches condition="view/branch_count">
228+ owned by
229+ <tal:individuals condition="view/person_owner_count">
230+ <tal:owners content="view/person_owner_count">42</tal:owners>
231+ <tal:people replace="view/person_text">people</tal:people
232+ ></tal:individuals
233+ ><tal:teams condition="view/team_owner_count">
234+ <tal:individuals condition="view/person_owner_count">
235+ and
236+ </tal:individuals
237+ ><tal:toc content="view/team_owner_count">1</tal:toc>
238+ <tal:people replace="view/team_text">team</tal:people
239+ ></tal:teams></tal:has-branches>.
240+ </span>
241+
242+ <!--commits-->
243+ <span id="commits"
244+ tal:define="count view/commit_count">
245+ There were
246+ <tal:commit-count replace="count"/>
247+ <tal:commits replace="python: view.commit_text.lower()">
248+ commits
249+ </tal:commits>
250+ <tal:has-committers condition="view/committer_count">
251+ by
252+ <tal:cc content="view/committer_count">4</tal:cc>
253+ <tal:people replace="view/committer_text">people</tal:people>
254+ </tal:has-committers>
255+ in the last month.
256+ </span>
257+ </p>
258 </div>