launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/branch-collection-links-652126 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #652126 branch collection action portlet is missing links
  https://bugs.launchpad.net/bugs/652126


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.

-- 
https://code.launchpad.net/~jcsackett/launchpad/branch-collection-links-652126/+merge/38456
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/branch-collection-links-652126 into lp:launchpad/devel.

=== 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-14 20:29:43 +0000
@@ -974,12 +974,6 @@
       permission="zope.Public"
       name="+portlet-product-codestatistics"
       template="../templates/product-portlet-codestatistics.pt"/>
-   <browser:page
-      for="lp.registry.interfaces.product.IProduct"
-      class="lp.code.browser.branchlisting.ProductBranchStatisticsView"
-      permission="zope.Public"
-      name="+portlet-product-codestatistics-content"
-      template="../templates/product-portlet-codestatistics-content.pt"/>
     <browser:page
         for="lp.registry.interfaces.product.IProduct"
         layer="lp.code.publisher.CodeLayer"

=== modified file 'lib/lp/code/templates/product-branches.pt'
--- lib/lp/code/templates/product-branches.pt	2010-09-28 19:25:54 +0000
+++ lib/lp/code/templates/product-branches.pt	2010-10-14 20:29:43 +0000
@@ -49,7 +49,6 @@
            tal:content="structure link/render"></p>
       </div>
 
-      <div tal:replace="structure context/@@+portlet-product-codestatistics" />
     </div>
   </metal:side>
 
@@ -57,6 +56,10 @@
 
     <tal:branch-summary content="structure context/@@+branch-summary" />
 
+    <tal:code-statistics
+      condition="view/branch_count"
+      replace="structure context/@@+portlet-product-codestatistics" />
+
     <tal:has-branches condition="view/branch_count"
                       define="branches view/branches">
       <tal:branchlisting content="structure branches/@@+branch-listing" />

=== 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 @@
-<tal:portlet-product-codestatistics-content
-    xmlns:tal="http://xml.zope.org/namespaces/tal";
-    xmlns:metal="http://xml.zope.org/namespaces/metal";>
-
-  <tr tal:define="menu context/menu:branches" class="code-links"
-      id="merge-counts">
-    <td class="code-count"
-        tal:define="count menu/active_review_count"
-        tal:content="count" />
-    <td>
-      <tal:link
-         define="link menu/active_reviews"
-         replace="structure link/render"
-         />
-    </td>
-  </tr>
-
-  <tr class="code-links" id="branch-count-summary">
-    <td class="code-count"
-        tal:define="count view/branch_count"
-        tal:content="count" />
-    <td>
-  <tal:branches replace="view/branch_text">branches</tal:branches
-  ><tal:has-branches condition="view/branch_count">
-  owned by
-  <tal:individuals condition="view/person_owner_count">
-    <tal:owners content="view/person_owner_count">42</tal:owners>
-    <tal:people replace="view/person_text">people</tal:people
-    ></tal:individuals
-    ><tal:teams condition="view/team_owner_count">
-    <tal:individuals condition="view/person_owner_count">
-      and
-    </tal:individuals>
-    <tal:toc content="view/team_owner_count">1</tal:toc>
-    <tal:people replace="view/team_text">team</tal:people
-    ></tal:teams></tal:has-branches>
-    </td>
-  </tr>
-
-  <tr class="code-links">
-    <td class="code-count"
-        tal:define="count view/commit_count"
-        tal:content="count" />
-    <td>
-    <tal:commits replace="view/commit_text">commits</tal:commits>
-    <tal:has-committers condition="view/committer_count">
-      by
-      <tal:cc content="view/committer_count">4</tal:cc>
-      <tal:people replace="view/committer_text">people</tal:people>
-    </tal:has-committers>
-    in the last month
-    </td>
-  </tr>
-
-</tal:portlet-product-codestatistics-content>

=== 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-14 20:29:43 +0000
@@ -2,10 +2,55 @@
   xmlns:tal="http://xml.zope.org/namespaces/tal";
   xmlns:metal="http://xml.zope.org/namespaces/metal";
   xmlns:i18n="http://xml.zope.org/namespaces/i18n";
-  class="portlet" id="portlet-product-codestatistics">
-
-  <table class="code-links">
-    <tbody id="portlet-product-codestatistics"
-      tal:content="structure context/@@+portlet-product-codestatistics-content" />
-  </table>
+  id="portlet-product-codestatistics">
+
+    <p>
+    <tal:project replace="context/displayname"/> has
+    <!--reviews-->
+    <span tal:define="count context/menu:branches/active_review_count;
+                      link context/menu:branches/active_reviews"
+          id="merge-counts">
+      <tal:active-count replace="count"/>
+      <tal:link replace="structure python: link.render().lower()"/></span>.
+ 
+    <!--branches-->
+    <span tal:define="count view/branch_count;"
+          id="branch-count-summary">
+      It has
+      <tal:branch-count replace="count"/>
+      <tal:branches replace="python: view.branch_text.lower()">
+        branches
+      </tal:branches> 
+      <tal:has-branches condition="view/branch_count">
+        owned by
+        <tal:individuals condition="view/person_owner_count">
+          <tal:owners content="view/person_owner_count">42</tal:owners>
+          <tal:people replace="view/person_text">people</tal:people>
+        </tal:individuals>
+        <tal:teams condition="view/team_owner_count">
+          <tal:individuals condition="view/person_owner_count">
+            and
+          </tal:individuals>
+          <tal:toc content="view/team_owner_count">1</tal:toc>
+          <tal:people replace="view/team_text">team</tal:people>
+        </tal:teams>
+      </tal:has-branches></span>.
+    </p>
+
+    <p>
+    <!--commits-->
+    <span id="commits"
+          tal:define="count view/commit_count">
+      It has had
+      <tal:commit-count replace="count"/>
+        <tal:commits replace="python: view.commit_text.lower()">
+          commits
+        </tal:commits>
+        <tal:has-committers condition="view/committer_count">
+          by
+          <tal:cc content="view/committer_count">4</tal:cc>
+          <tal:people replace="view/committer_text">people</tal:people>
+        </tal:has-committers>
+        in the last month.
+    </span>
 </div>