launchpad-reviewers team mailing list archive

[Nigel Jones <dev@xxxxxxxxxx>]

Nigel Jones has proposed merging lp:~dev-nigelj/launchpad/bug61428 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #61428 in Launchpad itself: "Want a "subscribed to teams" portlet..."
  https://bugs.launchpad.net/launchpad/+bug/61428

For more details, see:
https://code.launchpad.net/~dev-nigelj/launchpad/bug61428/+merge/73632

Summary

Bug 61428 describes that the team portlet is missing from the +subscribedbugs page (it exists on the +assignedbugs page) and asked for it to be included.

Proposed fix

The proposed fix is to unify the portlet, and then include it in the rendering for +subscribedbugs like it is for +assignedbugs

Pre-implementation notes

>From discussing this issue with danilos on IRC, we agreed the best route for this fix is to start off by making the portlet generic (instead of having multiple similar portlets with essentially a string substitution) and applying to both the assignedbugs and subscribedbugs page.

This was with the view that it could be extended on in the future to also include an option to turn on/off showing of the indirectly assigned/subscribed bugs in the same view without individually clicking on teams.

Implementation details

While making the portlet generic, I realised that the best way seemed to be was to make the portlet a macro so that the classes that handle assignedbugs & subscribedbugs could share (via the variable 'link_back') to the template code.

This involved the following changes:

lib/lp/registry/browser/person.py : add the afore-mentioned 'link_back' variables to the PersonAssignedBugTaskSearchListingView & PersonSubscribedBugTaskSearchListingView classes.  As well as rename shouldShowAssignedToTeamPortlet() to shouldShowTeamPortlet() in PersonAssigned... and add to PersonSubscribed... classes

lib/lp/bugs/templates/person-team-bugs-macro.pt : essentially the same as person-portlet-team-assignedbugs.pt except to use view/link_back from the classes

lib/lp/bugs/browser/configure.zcml : Modification to reflect template name change, and macro-ization.

lib/lp/bugs/templates/buglisting-embedded-advanced-search.pt : changed to include the portlet as a macro instead.

Tests

As it the template has become a macro, it is no longer accessible so the now failing pagetest was removed and not replaced.

Demo & QA

* Browse to a user's +assignedbugs & +subscribedbugs pages
* Note that on the right, there should be a list of the user's teams, each linked to: https://(launchpaddomain)/~(teamname)/+assignedbugs (or +subscribedbugs depending on the page)

lint

The only lint errors are:
./lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt
       7: source has bad indentation.
      13: source exceeds 78 characters.
      33: source has bad indentation.
      40: source has bad indentation.
      (etc...)

Due to (according to wgrant) not been 4 spaces prior to the code blocks.
-- 
https://code.launchpad.net/~dev-nigelj/launchpad/bug61428/+merge/73632
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~dev-nigelj/launchpad/bug61428 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt'
--- lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2011-07-08 03:06:16 +0000
+++ lib/canonical/launchpad/pagetests/basics/notfound-traversals.txt	2011-09-01 06:36:44 +0000
@@ -392,7 +392,6 @@
 >>> check("/~name16/+codesofconduct", auth=True)
 >>> check("/~name16/+edithomepage", auth=True)
 >>> check("/~name16/+review", auth=True)
->>> check("/~name16/+portlet-team-assignedbugs")
 >>> check("/~name16/+specworkload")
 >>> check("/~name16/+imports", host='translations.launchpad.dev')
 

=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2011-08-31 17:40:09 +0000
+++ lib/lp/bugs/browser/configure.zcml	2011-09-01 06:36:44 +0000
@@ -271,10 +271,10 @@
         permission="launchpad.AnyPerson"/>
     <browser:page
         for="lp.registry.interfaces.person.IPerson"
+        name="+team-bugs-macro"
+        template="../templates/person-team-bugs-macro.pt"
         permission="zope.Public"
-        class="lp.registry.browser.person.PersonView"
-        name="+portlet-team-assignedbugs"
-        template="../templates/person-portlet-team-assignedbugs.pt"/>
+        class="lp.app.browser.launchpad.Macro"/>
     <browser:page
         for="lp.registry.interfaces.person.IPerson"
         name="+portlet-otherpackages"

=== modified file 'lib/lp/bugs/templates/buglisting-embedded-advanced-search.pt'
--- lib/lp/bugs/templates/buglisting-embedded-advanced-search.pt	2009-11-27 21:09:00 +0000
+++ lib/lp/bugs/templates/buglisting-embedded-advanced-search.pt	2011-09-01 06:36:44 +0000
@@ -9,8 +9,9 @@
 <body>
   <div metal:fill-slot="side">
     <tal:menu replace="structure context/@@+global-actions" />
-    <div tal:condition="view/shouldShowAssignedToTeamPortlet|nothing"
-      tal:content="structure context/@@+portlet-team-assignedbugs" />
+    <tal:portlet-team-bugs condition="view/shouldShowTeamPortlet|nothing">
+      <div metal:use-macro="context/@@+team-bugs-macro/show" />
+    </tal:portlet-team-bugs>
   </div>
 
   <div metal:fill-slot="main">

=== renamed file 'lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt' => 'lib/lp/bugs/templates/person-team-bugs-macro.pt'
--- lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt	2010-08-16 16:38:38 +0000
+++ lib/lp/bugs/templates/person-team-bugs-macro.pt	2011-09-01 06:36:44 +0000
@@ -4,26 +4,27 @@
   xmlns:i18n="http://xml.zope.org/namespaces/i18n";
   omit-tag="">
 
-<div class="portlet" id="portlet-team-assigned-bugs"
-     tal:condition="context/team_memberships">
-
-  <h2><span tal:replace="context/title" />'s teams</h2>
-
-  <div class="portletBody">
-
-    <table>
-      <tal:teams repeat="team context/teams_participated_in">
-        <tr tal:condition="team/@@+restricted-membership/userCanViewMembership">
-          <td tal:content="structure team/image:icon" />
-          <td>
-            <a tal:attributes="
-                href string:${team/fmt:url}/+assignedbugs"
-              tal:content="team/title">Team name</a>
-          </td>
-        </tr>
-      </tal:teams>
-    </table>
+<metal:block define-macro="show">
+  <div class="portlet" id="portlet-team-bugs"
+       tal:condition="context/team_memberships">
+
+    <h2><span tal:replace="context/title" />'s teams</h2>
+
+    <div class="portletBody">
+
+      <table>
+        <tal:teams repeat="team context/teams_participated_in">
+          <tr tal:condition="team/@@+restricted-membership/userCanViewMembership">
+            <td tal:content="structure team/image:icon" />
+            <td>
+              <a tal:attributes="
+                  href string:${team/fmt:url}/${view/link_back}"
+                tal:content="team/title">Team name</a>
+            </td>
+          </tr>
+        </tal:teams>
+      </table>
+    </div>
   </div>
-
-</div>
+</metal:block>
 </tal:root>

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2011-08-29 07:45:37 +0000
+++ lib/lp/registry/browser/person.py	2011-09-01 06:36:44 +0000
@@ -2199,6 +2199,7 @@
     columns_to_show = ["id", "summary", "bugtargetdisplayname",
                        "importance", "status"]
     page_title = 'Assigned bugs'
+    link_back = '+assignedbugs'
 
     def searchUnbatched(self, searchtext=None, context=None,
                         extra_params=None, prejoins=[]):
@@ -2220,7 +2221,7 @@
         """Should the assignee widget be shown on the advanced search page?"""
         return False
 
-    def shouldShowAssignedToTeamPortlet(self):
+    def shouldShowTeamPortlet(self):
         """Should the team assigned bugs portlet be shown?"""
         return True
 
@@ -2345,6 +2346,7 @@
     columns_to_show = ["id", "summary", "bugtargetdisplayname",
                        "importance", "status"]
     page_title = 'Subscribed bugs'
+    link_back = '+subscribedbugs'
 
     def searchUnbatched(self, searchtext=None, context=None,
                         extra_params=None, prejoins=[]):
@@ -2362,6 +2364,10 @@
         return sup.searchUnbatched(
             searchtext, context, extra_params, prejoins)
 
+    def shouldShowTeamPortlet(self):
+        """Should the team subscribed bugs portlet be shown?"""
+        return True
+
     def getSearchPageHeading(self):
         """The header for the search page."""
         return "Bugs %s is subscribed to" % self.context.displayname