Merge lp:~allenap/launchpad/sub-search-ui-bug-656823 into lp:launchpad/db-devel

[Gavin Panella]

Various bits and pieces around PersonSubscriptionsView.

This isn't directly relevant to the bug, but is some stuff I wanted to
do before I begin making bigger changes.

- PersonSubscriptionsView only needs to inherit from LaunchpadView.

- I also simplified it a bit, and added a couple of XXXs to mark out
  some potentially inefficient bits.

- I prettified the +subscriptions template, added lower navigation
  links, fixed it to only display navigation links if there are more
  items than fit on the page, removed an illegal id (a static id in a
  repeated block), and generally tidied up.

This is targeted to db-devel but I may land these changes in devel.

[Curtis Hovey (sinzui)]

Thanks for this refactoring, the code and template are easier to read.

I see metal:use-macro="view/macro:page/main_side", but this template does not use the side. Is the intent to add navigation to see other listings such as indirect bug subscriptions or blueprints?

(we decided to switch to main_only while discussing it on IRC. It can be changed back to mail_side when the page has side portlets.)

review: Approve (code)

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-10-07 23:49:13 +0000
+++ lib/lp/registry/browser/person.py	2010-10-08 15:45:58 +0000
@@ -206,10 +206,6 @@
 from canonical.launchpad.webapp.login import logoutPerson
 from canonical.launchpad.webapp.menu import get_current_view
 from canonical.launchpad.webapp.publisher import LaunchpadView
-from lp.app.browser.tales import (
-    DateTimeFormatterAPI,
-    PersonFormatterAPI,
-    )
 from canonical.lazr.utils import smartquote
 from canonical.widgets import (
     LaunchpadDropdownWidget,
@@ -226,6 +222,10 @@
 from lp.answers.interfaces.questionenums import QuestionParticipation
 from lp.answers.interfaces.questionsperson import IQuestionsPerson
 from lp.app.browser.stringformatter import FormattersAPI
+from lp.app.browser.tales import (
+    DateTimeFormatterAPI,
+    PersonFormatterAPI,
+    )
 from lp.app.errors import (
     NotFoundError,
     UnexpectedFormData,
@@ -2344,14 +2344,16 @@
         return self.getSearchPageHeading()
 
 
-class PersonSubscriptionsView(BugTaskSearchListingView):
+class PersonSubscriptionsView(LaunchpadView):
     """All the subscriptions for a person."""
 
     page_title = 'Subscriptions'
 
     def subscribedBugTasks(self):
-        """Return a BatchNavigator for distinct bug tasks to which the
-        person is subscribed."""
+        """
+        Return a BatchNavigator for distinct bug tasks to which the person is
+        subscribed.
+        """
         bug_tasks = self.context.searchTasks(None, user=self.user,
             order_by='-date_last_updated',
             status=(BugTaskStatus.NEW,
@@ -2364,38 +2366,37 @@
             bug_subscriber=self.context)
 
         sub_bug_tasks = []
-        sub_bugs = []
+        sub_bugs = set()
 
+        # XXX: GavinPanella 2010-10-08 bug=656904: This materializes the
+        # entire result set. It would probably be more efficient implemented
+        # with a pre_iter_hook on a DecoratedResultSet.
         for task in bug_tasks:
+            # We order the bugtasks by date_last_updated but we always display
+            # the default task for the bug. This is to avoid ordering issues
+            # in tests and also prevents user confusion (because nothing is
+            # more confusing than your subscription targets changing seemingly
+            # at random).
             if task.bug not in sub_bugs:
-                # We order the bugtasks by date_last_updated but we
-                # always display the default task for the bug. This is
-                # to avoid ordering issues in tests and also prevents
-                # user confusion (because nothing is more confusing than
-                # your subscription targets changing seemingly at
-                # random).
+                # XXX: GavinPanella 2010-10-08 bug=656904: default_bugtask
+                # causes a query to be executed. It would be more efficient to
+                # get the default bugtask in bulk, in a pre_iter_hook on a
+                # DecoratedResultSet perhaps.
                 sub_bug_tasks.append(task.bug.default_bugtask)
-                sub_bugs.append(task.bug)
+                sub_bugs.add(task.bug)
+
         return BatchNavigator(sub_bug_tasks, self.request)
 
     def canUnsubscribeFromBugTasks(self):
-        viewed_person = self.context
-        if self.user is None:
-            return False
-        elif viewed_person == self.user:
-            return True
-        elif (viewed_person.is_team and
-              self.user.inTeam(viewed_person)):
-            return True
+        """Can the current user unsubscribe from the bug tasks shown?"""
+        return (self.user is not None and
+                self.user.inTeam(self.context))
 
-    def getSubscriptionsPageHeading(self):
+    @property
+    def label(self):
         """The header for the subscriptions page."""
         return "Subscriptions for %s" % self.context.displayname
 
-    @property
-    def label(self):
-        return self.getSubscriptionsPageHeading()
-
 
 class PersonVouchersView(LaunchpadFormView):
     """Form for displaying and redeeming commercial subscription vouchers."""

=== modified file 'lib/lp/registry/templates/person-subscriptions.pt'
--- lib/lp/registry/templates/person-subscriptions.pt	2010-09-28 20:55:37 +0000
+++ lib/lp/registry/templates/person-subscriptions.pt	2010-10-08 15:45:58 +0000
@@ -1,71 +1,75 @@
 <html
-  xmlns="http://www.w3.org/1999/xhtml"
-  xmlns:tal="http://xml.zope.org/namespaces/tal"
-  xmlns:metal="http://xml.zope.org/namespaces/metal"
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n"
-  xml:lang="en"
-  lang="en"
-  dir="ltr"
-  metal:use-macro="view/macro:page/main_side"
-  i18n:domain="launchpad"
->
+    xmlns="http://www.w3.org/1999/xhtml"
+    xmlns:tal="http://xml.zope.org/namespaces/tal"
+    xmlns:metal="http://xml.zope.org/namespaces/metal"
+    xmlns:i18n="http://xml.zope.org/namespaces/i18n"
+    xml:lang="en"
+    lang="en"
+    dir="ltr"
+    metal:use-macro="view/macro:page/main_only"
+    i18n:domain="launchpad">
   <body>
 
   <div metal:fill-slot="main"
-    tal:define="sub_bugs view/subscribedBugTasks;
-    sub_bug_batch sub_bugs/currentBatch">
+       tal:define="display_name context/fmt:displayname;
+                   sub_bugs view/subscribedBugTasks;
+                   sub_bug_batch sub_bugs/currentBatch">
+
     <tal:no_subscribed_bugs condition="not: sub_bug_batch|nothing">
-      <p><tal:person content="context/fmt:displayname">Sample
-      Person</tal:person> does not have any direct bug subscriptions.</p>
+      <p>
+        <tal:person content="display_name">Sample
+        Person</tal:person> does not have any direct bug subscriptions.
+      </p>
     </tal:no_subscribed_bugs>
 
     <tal:subscribed_bugs condition="sub_bug_batch|nothing">
       <p>
-      Direct bug subscriptions for
-      <tal:person content="context/fmt:displayname">Sample Person</tal:person>
+        Direct bug subscriptions for
+        <tal:person content="display_name">Sample Person</tal:person>
       </p>
-    <div tal:replace="structure sub_bugs/@@+navigation-links-lower"/>
-    <table id="bug_subscriptions" class="listing">
-    <thead>
-      <tr>
-        <th colspan="3">Summary</th>
-        <th>
-          In
-        </th>
-        <th tal:condition="view/canUnsubscribeFromBugTasks">
-        </th>
-      </tr>
-    </thead>
-    <tr tal:repeat="sub_bug_task sub_bug_batch">
-              <td class="icon left">
-                <span tal:replace="structure sub_bug_task/image:icon" />
-              </td>
-              <td id="bug_number" style="text-align: right">
-                <span tal:replace="sub_bug_task/bug/id" />
-              </td>
-              <td>
-                <a tal:attributes="href sub_bug_task/fmt:url"
-                   tal:content="sub_bug_task/bug/title" />
-              </td>
-        <td tal:content="sub_bug_task/bugtargetdisplayname"
-            tal:condition="sub_bug_task/bugtargetdisplayname|nothing"
-        >mozilla-firefox (Ubuntu)</td>
-        <td tal:condition="view/canUnsubscribeFromBugTasks"
-            align="center">
-            <a
-               tal:attributes="
-               title string:Unsubscribe ${context/fmt:displayname} from bug ${sub_bug_task/bug/id};
-                 id string:unsubscribe-subscriber-${context/id};
-                 href string:${sub_bug_task/fmt:url}/+subscribe;
-               "
-            >
+      <tal:multipage condition="sub_bugs/has_multiple_pages">
+        <tal:navigation
+            replace="structure sub_bugs/@@+navigation-links-upper"/>
+      </tal:multipage>
+      <table id="bug_subscriptions" class="listing">
+        <thead>
+          <tr>
+            <th colspan="3">Summary</th>
+            <th>
+              In
+            </th>
+            <th tal:condition="view/canUnsubscribeFromBugTasks">
+            </th>
+          </tr>
+        </thead>
+        <tr tal:repeat="sub_bug_task sub_bug_batch">
+          <td class="icon left">
+            <span tal:replace="structure sub_bug_task/image:icon" />
+          </td>
+          <td style="text-align: right">
+            <span tal:replace="sub_bug_task/bug/id" />
+          </td>
+          <td>
+            <a tal:attributes="href sub_bug_task/fmt:url"
+               tal:content="sub_bug_task/bug/title" />
+          </td>
+          <td tal:content="sub_bug_task/bugtargetdisplayname"
+              tal:condition="sub_bug_task/bugtargetdisplayname|nothing" />
+          <td tal:condition="view/canUnsubscribeFromBugTasks" align="center">
+            <a tal:attributes="id string:unsubscribe-subscriber-${context/id};
+                               href sub_bug_task/fmt:url/+subscribe;
+                               title string:Unsubscribe ${display_name} from bug ${sub_bug_task/bug/id};">
               <img src="/@@/edit"
-              tal:attributes="id string:unsubscribe-icon-${context/id};
-                              position string:relative"/>
+                   tal:attributes="id string:unsubscribe-icon-${context/id};
+                                   position string:relative"/>
             </a>
           </td>
-      </tr>
-    </table>
+        </tr>
+      </table>
+      <tal:multipage condition="sub_bugs/has_multiple_pages">
+        <tal:navigation
+            replace="structure sub_bugs/@@+navigation-links-lower"/>
+      </tal:multipage>
     </tal:subscribed_bugs>
 
   </div>