launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Robert Collins has proposed merging lp:~lifeless/launchpad/registry into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #615237 /participants API timing out
  https://bugs.launchpad.net/bugs/615237
  #627412 Person:+mailinglist-moderate timeout : error while trying to moderate messages
  https://bugs.launchpad.net/bugs/627412


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).
-- 
https://code.launchpad.net/~lifeless/launchpad/registry/+merge/35354
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/registry into lp:launchpad/devel.

=== 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">