Merge lp:~lifeless/launchpad/registry into lp:launchpad

[Robert Collins]

Help timeouts in mailing list pages by batching.

Address timeouts in mailing list pages by batching (the batch size may need tweaking, but we'll want to see if the default is sufficient).

[Robert Collins (lifeless)]

Oh, this also includes a small fix from curtis which was needed to test this change properly; he swears it won't break the world.

[Tim Penhey (thumper)]

Looks fine. Are the id changes in the generic template going to cause other issues?

review: Approve

=== modified file 'lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt'
--- lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt	2010-02-25 00:47:17 +0000
+++ lib/canonical/launchpad/templates/batchnavigator-navigation-links.pt	2010-09-14 00:24:54 +0000
@@ -27,9 +27,9 @@
           class="batch-navigation-links">
         <a
           tal:condition="first_page_url"
-          tal:attributes="href first_page_url"
+          tal:attributes="href first_page_url;
+                          id string:${view/css_class}-batchnav-first"
           class="first"
-          id="batchnav_first"
           rel="first"
         >First</a>
         <span tal:condition="not:first_page_url" class="first inactive"
@@ -37,9 +37,9 @@
         &#149;
         <a
           tal:condition="prev_page_url"
-          tal:attributes="href prev_page_url"
+          tal:attributes="href prev_page_url;
+                          id string:${view/css_class}-batchnav-previous"
           class="previous"
-          id="batchnav_previous"
           rel="previous"
         >Previous</a>
         <span tal:condition="not:prev_page_url" class="previous inactive"
@@ -47,8 +47,8 @@
         &#149;
         <a
           tal:condition="next_page_url"
-          tal:attributes="href next_page_url"
-          id="batchnav_next"
+          tal:attributes="href next_page_url;
+                          id string:${view/css_class}-batchnav-next"
           class="next"
           rel="next"
         ><strong>Next</strong></a>
@@ -58,9 +58,9 @@
           &#149;
           <a
             tal:condition="last_page_url"
-            tal:attributes="href last_page_url"
+            tal:attributes="href last_page_url;
+                            id string:${view/css_class}-batchnav-last"
             class="last"
-            id="batchnav_last"
             rel="last"
           >Last</a>
           <span tal:condition="not:last_page_url" class="last inactive"

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2010-09-08 13:26:50 +0000
+++ lib/lp/registry/browser/team.py	2010-09-14 00:24:54 +0000
@@ -807,21 +807,29 @@
         assert(self.mailing_list is not None), (
             'No mailing list: %s' % self.context.name)
 
-    @property
+    @cachedproperty
     def hold_count(self):
         """The number of message being held for moderator approval.
 
         :return: Number of message being held for moderator approval.
         """
-        return self.mailing_list.getReviewableMessages().count()
+        ## return self.mailing_list.getReviewableMessages().count()
+        # This looks like it would be more efficient, but it raises
+        # LocationError.
+        return self.held_messages.currentBatch().listlength
 
-    @property
+    @cachedproperty
     def held_messages(self):
         """All the messages being held for moderator approval.
 
         :return: Sequence of held messages.
         """
-        return self.mailing_list.getReviewableMessages()
+        results = self.mailing_list.getReviewableMessages()
+        navigator = BatchNavigator(results, self.request)
+        # Subclasses often set the singular and plural headings,
+        # but we can use the generic class too.
+        navigator.setHeadings('message', 'messages')
+        return navigator
 
     @action('Moderate', name='moderate')
     def moderate_action(self, action, data):
@@ -830,7 +838,7 @@
         # won't be in data.  Instead, get it out of the request.
         reviewable = self.hold_count
         disposed_count = 0
-        for message in self.held_messages:
+        for message in self.held_messages.currentBatch():
             action_name = self.request.form_ng.getOne(
                 'field.' + quote(message.message_id))
             # This essentially acts like a switch statement or if/elifs.  It

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2010-03-05 13:56:38 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2010-09-14 00:24:54 +0000
@@ -3,10 +3,12 @@
 
 __metaclass__ = type
 
+from canonical.launchpad.webapp.batching import BatchNavigator
 from canonical.testing import DatabaseFunctionalLayer
 from lp.registry.browser.person import TeamOverviewMenu
 from lp.testing import TestCaseWithFactory
 from lp.testing.menu import check_menu_links
+from lp.testing.views import create_initialized_view
 
 
 class TestTeamMenu(TestCaseWithFactory):
@@ -35,3 +37,22 @@
         self.assertEqual(True, check_menu_links(menu))
         link = menu.configure_mailing_list()
         self.assertEqual('Configure mailing list', link.text)
+
+
+class TestModeration(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_held_messages_is_batch_navigator(self):
+        team = self.factory.makeTeam()
+        self.factory.makeMailingList(team, team.teamowner)
+        view = create_initialized_view(team, name='+mailinglist-moderate')
+        self.assertIsInstance(view.held_messages, BatchNavigator)
+
+    def test_held_message_headings(self):
+        team = self.factory.makeTeam()
+        self.factory.makeMailingList(team, team.teamowner)
+        view = create_initialized_view(team, name='+mailinglist-moderate')
+        navigator = view.held_messages
+        self.assertEqual('message', navigator._singular_heading)
+        self.assertEqual('messages', navigator._plural_heading)

=== modified file 'lib/lp/registry/stories/mailinglists/moderation.txt'
--- lib/lp/registry/stories/mailinglists/moderation.txt	2009-12-03 20:28:54 +0000
+++ lib/lp/registry/stories/mailinglists/moderation.txt	2010-09-14 00:24:54 +0000
@@ -100,6 +100,21 @@
     2 messages have been posted to your mailing list...
     ...
 
+If there are more messages than the batch size, they get batched.
+
+    >>> admin_browser.open(
+    ...     'http://launchpad.dev/~guadamen/+mailinglist-moderate?batch=1')
+    >>> find_tag_by_id(admin_browser.contents, 'upper-batch-nav-batchnav-next')['class']
+    u'next'
+    >>> find_tag_by_id(admin_browser.contents, 'lower-batch-nav-batchnav-next')['class']
+    u'next'
+
+To test easily, we use the default batch size below.
+
+    >>> admin_browser.open(
+    ...     'http://launchpad.dev/~guadamen/+mailinglist-moderate')
+
+
 Each held message displays some details about what's being held.
 
     >>> print extract_text(find_tag_by_id(

=== modified file 'lib/lp/registry/templates/team-mailinglist-moderate.pt'
--- lib/lp/registry/templates/team-mailinglist-moderate.pt	2009-09-14 01:28:41 +0000
+++ lib/lp/registry/templates/team-mailinglist-moderate.pt	2010-09-14 00:24:54 +0000
@@ -35,16 +35,24 @@
           action for obvious spam.</li>
           <li><strong>Hold</strong> - Continue to hold the message, deferring
           your decision until later.</li>
+          <li>Toss</li>
         </ul>
       </div>
-      <table class="listing" metal:fill-slot="widgets">
+      <div metal:fill-slot="widgets">
+      <tal:navigation
+        replace="structure view/held_messages/@@+navigation-links-upper" />
+
+      <table class="listing">
         <thead><tr>
           <th>Message details</th>
           <th>Approve</th><th>Decline</th><th>Discard</th><th>Hold</th>
         </tr></thead>
-        <span tal:repeat="message view/held_messages"
+        <span tal:repeat="message view/held_messages/currentBatch"
              tal:content="structure message/@@+moderation" />
       </table>
+      <tal:navigation
+        replace="structure view/held_messages/@@+navigation-links-lower" />
+      </div>
     </metal:form>
   </div>
   <span tal:condition="not: view/hold_count" id="legend">