Merge lp:~sinzui/launchpad/moderated-messages-0 into lp:launchpad

[Curtis Hovey]

Fix message moderation timeouts by avoiding the librarian.

The mailing list moderation view can timeout showing a list of messages
even though it shows fewer messages than most listings.

--------------------------------------------------------------------

RULES

    Pre-implementation: jcsackett
    * Lp is calling the librarian for each message to learn which
      email address was used to send the message.
      * This is not relevant because message approval is based on user,
        not email addresses.
      * The view is making a link to the lp user, but does extra work
        to craft the link to look like an email address.
    * HeldMessageDetails.email_message implicitly calls the librarian.
      * Only one callsite knows about this cachedproperty,
        HeldMessageDetails.sender
      * Only one callsite knows about the HeldMessageDetails.sender
        property.
    * Change the view to just use standard links for the the message
      author.
    * Remove .sender and .email_message from the HeldMessageDetails
      class

    ADDENDUM
    * The query that gets the MessageApproval also precaches the Message,
      but not the Person that sent the message. The Person is always
      accessed when working with MessageApproval.

QA

    * Ask a webops to make you an admin of
      https://launchpad.net/~ubuntu-cyclists
    * Visit https://launchpad.net/~ubuntu-cyclists/+moderation
    * Verify the page loads
    * Discard the messages prior to 2012 (the spam)
    * verify the page loads.

LINT

    lib/lp/registry/browser/mailinglists.py
    lib/lp/registry/browser/tests/mailinglist-message-views.txt
    lib/lp/registry/interfaces/mailinglist.py
    lib/lp/registry/model/mailinglist.py
    lib/lp/registry/tests/test_mailinglist.py

LoC

    This branch removes code that is unneeded by this refactoring.

TEST

    ./bin/test -vvc -t HeldMessage -t getReviewableMessages \
        lp.registry.tests.test_mailinglist
    ./bin/test -vvc -t HeldMessage lp.registry.browser.tests.test_mailinglist

IMPLEMENTATION

I updated the view to make a standard Lp link to the sender, which is faster
to format and provides consistency.
    lib/lp/registry/browser/mailinglists.py
    lib/lp/registry/browser/tests/mailinglist-message-views.txt

I updated MailingList.getReviewableMessages() to also cache the user in
MessageApproval.posted_by, then change HeldMessageDetails.posted_by to
use that same user. So the moderated view never asks the librarian
for the sender's email address, and the sender is cached along with the
Lp copy of the message. I removed .sender and .email_message because the
attributes are no longer unused.
    lib/lp/registry/interfaces/mailinglist.py
    lib/lp/registry/model/mailinglist.py
    lib/lp/registry/tests/test_mailinglist.py

[Richard Harding (rharding)]

review: Approve

=== modified file 'lib/lp/registry/browser/mailinglists.py'
--- lib/lp/registry/browser/mailinglists.py	2012-02-21 22:46:28 +0000
+++ lib/lp/registry/browser/mailinglists.py	2012-10-11 01:57:24 +0000
@@ -16,13 +16,13 @@
 
 from zope.component import getUtility
 
+from lp.app.browser.tales import PersonFormatterAPI
 from lp.registry.interfaces.mailinglist import (
     IHeldMessageDetails,
     IMailingListSet,
     )
 from lp.registry.interfaces.person import ITeam
 from lp.services.webapp import (
-    canonical_url,
     LaunchpadView,
     )
 
@@ -44,11 +44,7 @@
         self.subject = self.details.subject
         self.date = self.details.date
         self.widget_name = 'field.' + quote(self.message_id)
-        # The author field is very close to what the details has, except that
-        # the view wants to include a link to the person's overview page.
-        self.author = '<a href="%s">%s</a>' % (
-            canonical_url(self.details.author),
-            escape(self.details.sender))
+        self.author = PersonFormatterAPI(self.details.author).link(None)
 
     def initialize(self):
         """See `LaunchpadView`."""

=== removed file 'lib/lp/registry/browser/tests/mailinglist-message-views.txt'
--- lib/lp/registry/browser/tests/mailinglist-message-views.txt	2012-04-10 14:01:17 +0000
+++ lib/lp/registry/browser/tests/mailinglist-message-views.txt	1970-01-01 00:00:00 +0000
@@ -1,74 +0,0 @@
-Held message view
------------------
-
-The HeldMessageView formats a held message so that it can be moderated by
-the mailing list administrator.
-
-    >>> from lp.services.messages.interfaces.message import IMessageSet
-
-    >>> team, mlist = factory.makeTeamAndMailingList('parrot', 'owner')
-
-    >>> login('[e-mail]')
-    >>> message_set = getUtility(IMessageSet)
-    >>> message = message_set.fromEmail("""\
-    ... From: Carlos <[e-mail]>
-    ... To: [e-mail]
-    ... Subject: monkey
-    ... Message-ID: <monkey>
-    ... Date: Fri, 01 Aug 2000 01:09:00 -0000
-    ...
-    ... First paragraph.
-    ...
-    ... Second paragraph.
-    ...
-    ... Third paragraph.
-    ... """)
-    >>> held_message = mlist.holdMessage(message)
-    >>> transaction.commit()
-
-    >>> ignored = login_person(team.teamowner)
-    >>> view = create_initialized_view(
-    ...     held_message, name='+moderation', principal=team.teamowner)
-    >>> print view.widget_name
-    field.%3Cmonkey%3E
-    >>> print view.message_id
-    <monkey>
-    >>> print view.subject
-    monkey
-    >>> print view.date
-    2000-08-01 01:09:00+00:00
-    >>> print view.author
-    <a href="http://launchpad.dev/~carlos">Carlos &lt;[e-mail]&gt;</a>
-    >>> print view.body_summary
-    First paragraph.
-    >>> print view.body_details
-    <p>
-    Second paragraph.
-    </p>
-    <p>
-    Third paragraph.
-    </p>
-
-
-View helpers
-------------
-
-The view uses a helper method to format the paragraphs.
-
-    >>> paragraphs = []
-    >>> current_paragraph = ['line 1', 'line 2']
-    >>> view._append_paragraph(paragraphs, current_paragraph)
-    >>> for line in paragraphs:
-    ...     print line
-    <p>
-    line 1
-    line 2
-    </p>
-
-The _append_paragraph method ignores empty paragraphs.
-
-    >>> paragraphs = []
-    >>> current_paragraph = []
-    >>> view._append_paragraph(paragraphs, current_paragraph)
-    >>> len(paragraphs)
-    0

=== modified file 'lib/lp/registry/browser/tests/test_mailinglists.py'
--- lib/lp/registry/browser/tests/test_mailinglists.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_mailinglists.py	2012-10-11 01:57:24 +0000
@@ -6,14 +6,22 @@
 
 __metaclass__ = type
 
+import transaction
+from zope.component import getUtility
 
+from lp.app.browser.tales import PersonFormatterAPI
 from lp.registry.interfaces.person import PersonVisibility
+from lp.services.messages.interfaces.message import IMessageSet
+from lp.services.webapp.authorization import check_permission
 from lp.testing import (
     login_person,
     person_logged_in,
     TestCaseWithFactory,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.pages import (
     extract_text,
     find_tag_by_id,
@@ -36,6 +44,26 @@
         self.factory.makeMailingList(team=team, owner=owner)
         return team
 
+    def makeHeldMessage(self, team, sender=None):
+        # Requires LaunchpadFunctionalLayer.
+        if sender is None:
+            sender = self.factory.makePerson(
+                email='[e-mail]', name='him', displayname='Him')
+        raw = '\n'.join([
+            'From: Him <[e-mail]>',
+            'To: %s' % str(team.mailing_list.address),
+            'Subject: monkey',
+            'Message-ID: <monkey>',
+            'Date: Fri, 01 Aug 2000 01:09:00 -0000',
+            '',
+            'First paragraph.\n\nSecond paragraph.\n\nThird paragraph.'
+            ])
+        message_set = getUtility(IMessageSet)
+        message = message_set.fromEmail(raw)
+        transaction.commit()
+        held_message = team.mailing_list.holdMessage(message)
+        return sender, message, held_message
+
 
 class MailingListSubscriptionControlsTestCase(TestCaseWithFactory):
     """Verify the team index subscribe/unsubscribe to mailing list content."""
@@ -138,3 +166,168 @@
             team, name='+mailinglist', principal=owner)
         element = find_tag_by_id(view(), 'mailing-list-archive')
         self.assertEqual('private', extract_text(element))
+
+
+class HeldMessageViewTestCase(MailingListTestCase):
+    """Verify the +moderation view."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_view_properties(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        view = create_initialized_view(
+            held_message, name='+moderation')
+        self.assertEqual(message.subject, view.subject)
+        self.assertEqual(message.rfc822msgid, view.message_id)
+        self.assertEqual(message.datecreated, view.date)
+        self.assertEqual(PersonFormatterAPI(sender).link(None), view.author)
+        self.assertEqual("First paragraph.", view.body_summary)
+        self.assertEqual(
+            "\n<p>\nSecond paragraph.\n</p>\n\n<p>\nThird paragraph.\n</p>\n",
+            view.body_details)
+
+    def test_view_append_paragraph(self):
+        # Consecutive lines are wrapped in html <p> tags.
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        view = create_initialized_view(
+            held_message, name='+moderation')
+        paragraphs = []
+        view._append_paragraph(paragraphs, ['line 1', 'line 2'])
+        self.assertEqual(
+            ['\n<p>\n', 'line 1\nline 2', '\n</p>\n'], paragraphs)
+        paragraphs = []
+        view._append_paragraph(paragraphs, [])
+        self.assertEqual([], paragraphs)
+
+    def test_render(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        view = create_initialized_view(
+            held_message, name='+moderation', principal=team.teamowner)
+        markup = view.render()
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*Subject:.*monkey.*From:.*Him.*Date:.*2000-08-01.*Message-ID'
+            '.*&lt;monkey&gt;.*class="foldable-quoted".*',
+            markup)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*<input type="radio" value="approve"'
+            '.*name="field.%3Cmonkey%3E" />'
+            '.*<input type="radio" value="reject"'
+            '.*name="field.%3Cmonkey%3E" />'
+            '.*<input type="radio" value="discard"'
+            '.*name="field.%3Cmonkey%3E" />'
+            '.*<input type="radio" value="hold"'
+            '.* name="field.%3Cmonkey%3E" checked="checked" />.*',
+            markup)
+
+
+class TeamMailingListModerationViewTestCase(MailingListTestCase):
+    """Verify the +mailinglist-moderate view."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_permissions(self):
+        # Team admins and privileged users can see the view others cannot.
+        team = self.makeTeamWithMailingList()
+        member = self.factory.makePerson()
+        with person_logged_in(team.teamowner):
+            team.addMember(member, team.teamowner)
+            view = create_initialized_view(team, name='+mailinglist-moderate')
+            self.assertIs(True, check_permission('launchpad.Edit', view))
+        with person_logged_in(member):
+            self.assertIs(False, check_permission('launchpad.Edit', view))
+
+    def test_message_summary_text(self):
+        team = self.makeTeamWithMailingList()
+        # No messages.
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', principal=team.teamowner)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*There are no mailing list messages requiring your review.*',
+            view.render())
+        # One message.
+        self.makeHeldMessage(team)
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', principal=team.teamowner)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*1.*message has.*been posted to your mailing list.*',
+            view.render())
+
+    def test_batching(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        for i in range(5):
+            self.makeHeldMessage(team, sender)
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', principal=team.teamowner)
+        self.assertEqual(6, view.hold_count)
+        self.assertEqual('messages', view.held_messages.heading)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*upper-batch-nav-batchnav-next.*lower-batch-nav-batchnav-next.*',
+            view.render())
+
+    def test_widgets(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', principal=team.teamowner)
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            '.*name="field.%3Cmonkey%3E.*', view.render())
+
+    def test_approve(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        form = {
+            'field.%3Cmonkey%3E': 'approve',
+            'field.actions.moderate': 'Moderate',
+            }
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(
+            'Held message approved; Message-ID: &lt;monkey&gt;',
+             view.request.notifications[0].message)
+
+    def test_discard(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        form = {
+            'field.%3Cmonkey%3E': 'discard',
+            'field.actions.moderate': 'Moderate',
+            }
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(
+            'Held message discarded; Message-ID: &lt;monkey&gt;',
+             view.request.notifications[0].message)
+
+    def test_reject(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        form = {
+            'field.%3Cmonkey%3E': 'reject',
+            'field.actions.moderate': 'Moderate',
+            }
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(
+            'Held message rejected; Message-ID: &lt;monkey&gt;',
+             view.request.notifications[0].message)
+
+    def test_held(self):
+        team = self.makeTeamWithMailingList()
+        sender, message, held_message = self.makeHeldMessage(team)
+        form = {
+            'field.%3Cmonkey%3E': 'hold',
+            'field.actions.moderate': 'Moderate',
+            }
+        view = create_initialized_view(
+            team, name='+mailinglist-moderate', form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(
+            'Messages still held for review: 1 of 1',
+             view.request.notifications[0].message)

=== modified file 'lib/lp/registry/interfaces/mailinglist.py'
--- lib/lp/registry/interfaces/mailinglist.py	2012-09-14 22:39:54 +0000
+++ lib/lp/registry/interfaces/mailinglist.py	2012-10-11 01:57:24 +0000
@@ -834,13 +834,6 @@
         description=_('The RFC 2822 Subject header.'),
         required=True, readonly=True)
 
-    sender = Text(
-        title=_('Message author'),
-        description=_('The message originator (i.e. author), formatted as '
-                      'per RFC 2822 and derived from the RFC 2822 originator '
-                      'fields From and Reply-To.  This is a unicode string.'),
-        required=True, readonly=True)
-
     author = Object(
         schema=IPerson,
         title=_('Message author'),
@@ -857,12 +850,6 @@
         description=_('The message body as plain text.'),
         required=True, readonly=True)
 
-    email_message = Text(
-        title=_('The email message object'),
-        description=_('The email.message.Message object created from the '
-                      "original message's raw text."),
-        required=True, readonly=True)
-
 
 class BaseSubscriptionErrors(Exception):
     """Base class for subscription exceptions."""

=== modified file 'lib/lp/registry/model/mailinglist.py'
--- lib/lp/registry/model/mailinglist.py	2012-09-15 03:39:21 +0000
+++ lib/lp/registry/model/mailinglist.py	2012-10-11 01:57:24 +0000
@@ -16,12 +16,6 @@
 
 
 import collections
-from email import message_from_string
-from email.Header import (
-    decode_header,
-    make_header,
-    )
-from itertools import repeat
 import operator
 from socket import getfqdn
 from string import Template
@@ -456,11 +450,12 @@
             MessageApproval.mailing_listID == self.id,
             MessageApproval.status == PostedMessageStatus.NEW,
             MessageApproval.messageID == Message.id,
+            MessageApproval.posted_byID == Person.id
             ]
         if message_id_filter is not None:
             clauses.append(Message.rfc822msgid.is_in(message_id_filter))
-        results = store.find((MessageApproval, Message),
-            *clauses)
+        results = store.find(
+            (MessageApproval, Message, Person), *clauses)
         results.order_by(MessageApproval.posted_date, Message.rfc822msgid)
         return DecoratedResultSet(results, operator.itemgetter(0))
 
@@ -797,31 +792,7 @@
         self.message_id = message_approval.message_id
         self.subject = self.message.subject
         self.date = self.message.datecreated
-        self.author = self.message.owner
-
-    @cachedproperty
-    def email_message(self):
-        self.message.raw.open()
-        try:
-            return message_from_string(self.message.raw.read())
-        finally:
-            self.message.raw.close()
-
-    @cachedproperty
-    def sender(self):
-        """See `IHeldMessageDetails`."""
-        originators = self.email_message.get_all('from', [])
-        originators.extend(self.email_message.get_all('reply-to', []))
-        if len(originators) == 0:
-            return 'n/a'
-        unicode_parts = []
-        for bytes, charset in decode_header(originators[0]):
-            if charset is None:
-                charset = 'us-ascii'
-            unicode_parts.append(
-                bytes.decode(charset, 'replace').encode('utf-8'))
-        header = make_header(zip(unicode_parts, repeat('utf-8')))
-        return unicode(header)
+        self.author = self.message_approval.posted_by
 
     @cachedproperty
     def body(self):

=== removed file 'lib/lp/registry/stories/mailinglists/moderation.txt'
--- lib/lp/registry/stories/mailinglists/moderation.txt	2011-12-24 15:18:32 +0000
+++ lib/lp/registry/stories/mailinglists/moderation.txt	1970-01-01 00:00:00 +0000
@@ -1,237 +0,0 @@
-=======================
-Mailing list moderation
-=======================
-
-When a non-member posts a message to a team's mailing list, that message
-will be held for moderator review, if the sender is not a Launchpad
-member in good standing (as is the default).
-
-    >>> admin_browser.open('http://launchpad.dev/~guadamen')
-    >>> admin_browser.getLink(url='+mailinglist').click()
-    >>> admin_browser.getControl('Create new Mailing List').click()
-
-    >>> from lp.registry.tests import mailinglists_helper
-    >>> login('[e-mail]')
-    >>> mailinglists_helper.mailman.act()
-    >>> transaction.commit()
-
-    # Ignore any notifications up to this point.
-    >>> from lp.testing import mail_helpers
-    >>> ignore = mail_helpers.pop_notifications()
-
-    >>> from lp.testing import login, logout
-    >>> from zope.component import getUtility
-    >>> from lp.registry.interfaces.mailinglist import IMailingListSet
-
-    >>> guadamen = getUtility(IMailingListSet).get('guadamen')
-    >>> logout()
-
-Anonymous users cannot see the messages waiting for moderation.
-
-    >>> anon_browser.open(
-    ...     'http://launchpad.dev/~guadamen/+mailinglist-moderate')
-    Traceback (most recent call last):
-    ...
-    Unauthorized: ...
-
-Non-administrators can also not review the messages.
-
-    >>> user_browser.open(
-    ...     'http://launchpad.dev/~guadamen/+mailinglist-moderate')
-    Traceback (most recent call last):
-    ...
-    Unauthorized: ...
-
-Foo Bar though, is the moderator for the Guadamen team, so he can visit
-the moderation page.  But right now, there is nothing to moderate.
-
-    >>> admin_browser.open(
-    ...     'http://launchpad.dev/~guadamen/+mailinglist-moderate')
-    >>> admin_browser.title
-    'Mailing list moderation...
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    There are no mailing list messages requiring your review.
-
-Carlos is not a member of the Guadamen team, nor is he a Launchpad
-member in good standing.  He posts a message to the team's mailing list,
-and it is held for approval.
-
-    >>> login('[e-mail]')
-    >>> from lp.services.messages.interfaces.message import IMessageSet
-    >>> message_set = getUtility(IMessageSet)
-    >>> message = message_set.fromEmail("""\
-    ... From: [e-mail]
-    ... To: [e-mail]
-    ... Subject: Aardvark
-    ... Message-ID: <aardvark>
-    ... Date: Fri, 01 Aug 2000 01:09:00 -0000
-    ...
-    ... This is my first post!
-    ... """)
-    >>> held_message = guadamen.holdMessage(message)
-    >>> logout()
-
-Foo Bar sees that there is one message waiting for approval.
-
-    >>> admin_browser.reload()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    1 message has been posted to your mailing list...
-    ...
-
-Let's say Carlos posts another message to the list, which also gets held
-for moderation.  Now there are two messages waiting for review.
-
-    >>> login('[e-mail]')
-    >>> message = message_set.fromEmail("""\
-    ... From: [e-mail]
-    ... To: [e-mail]
-    ... Subject: Bobcat
-    ... Message-ID: <bobcat>
-    ... Date: Fri, 01 Aug 2000 01:09:01 -0000
-    ...
-    ... This is my second post!
-    ... """)
-    >>> held_message = guadamen.holdMessage(message)
-    >>> logout()
-
-    >>> admin_browser.reload()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    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(
-    ...     admin_browser.contents, 'field.%3Caardvark%3E'))
-    Subject:
-    Aardvark
-    From:
-    [e-mail]
-    Date:
-    2000-08-01 01:09:00+00:00
-    Message-ID:
-    &lt;aardvark&gt;
-    This is my first post!
-
-    >>> print extract_text(find_tag_by_id(
-    ...     admin_browser.contents, 'field.%3Cbobcat%3E'))
-    Subject:
-    Bobcat
-    From:
-    [e-mail]
-    Date:
-    2000-08-01 01:09:01+00:00
-    Message-ID:
-    &lt;bobcat&gt;
-    This is my second post!
-
-By default, the action to take on all messages is to continue to hold
-them.  So if the messages are moderated now, they'll just show up the
-next time Foo visits this page.
-
-    >>> admin_browser.getControl('Moderate').click()
-    >>> admin_browser.title
-    'GuadaMen in Launchpad'
-    >>> admin_browser.getLink('Moderate mailing list').click()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    2 messages have been posted to your mailing list...
-    ...
-
-Foo has three other options available to him for disposing of these
-messages.  Carlos sends one more to the list, and it is also held, which
-gives Foo a perfect opportunity for exercises each of his options.
-
-    >>> login('[e-mail]')
-    >>> message = message_set.fromEmail("""\
-    ... From: [e-mail]
-    ... To: [e-mail]
-    ... Subject: Caribou
-    ... Message-ID: <caribou>
-    ... Date: Fri, 01 Aug 2000 01:09:02 -0000
-    ...
-    ... This is my third post!
-    ... """)
-    >>> held_message = guadamen.holdMessage(message)
-    >>> logout()
-
-    >>> admin_browser.reload()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    3 messages have been posted to your mailing list...
-    ...
-
-Foo decides that Carlos's first message is spam and should just be
-discarded.
-
-    >>> admin_browser.getControl(name='field.%3Caardvark%3E').value = [
-    ...    'discard']
-    >>> admin_browser.getControl('Moderate').click()
-
-    >>> login('[e-mail]')
-    >>> from lp.registry.interfaces.mailinglist import IMessageApprovalSet
-    >>> message_approval_set = getUtility(IMessageApprovalSet)
-    >>> message_approval = message_approval_set.getMessageByMessageID(
-    ...     '<aardvark>')
-    >>> message_approval.status
-    <DBItem PostedMessageStatus.DISCARD_PENDING, (60) Discard pending>
-    >>> logout()
-
-    >>> admin_browser.getLink('Moderate mailing list').click()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    2 messages have been posted to your mailing list...
-    ...
-
-Foo decides that Carlos's second message isn't spam, but it isn't
-on-topic, so he decides to decline this message.
-
-    >>> admin_browser.getControl(name='field.%3Cbobcat%3E').value = [
-    ...    'reject']
-    >>> admin_browser.getControl('Moderate').click()
-
-    >>> login('[e-mail]')
-    >>> from lp.registry.interfaces.mailinglist import IMessageApprovalSet
-    >>> message_approval_set = getUtility(IMessageApprovalSet)
-    >>> message_approval = message_approval_set.getMessageByMessageID(
-    ...     '<bobcat>')
-    >>> message_approval.status
-    <DBItem PostedMessageStatus.REJECTION_PENDING, (30) Decline pending>
-    >>> logout()
-
-    >>> admin_browser.getLink('Moderate mailing list').click()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    1 message has been posted to your mailing list...
-    ...
-
-Foo decides that Carlos's third message is just right, and he decides to
-accept the message.
-
-    >>> admin_browser.getControl(name='field.%3Ccaribou%3E').value = [
-    ...    'approve']
-    >>> admin_browser.getControl('Moderate').click()
-
-    >>> login('[e-mail]')
-    >>> from lp.registry.interfaces.mailinglist import IMessageApprovalSet
-    >>> message_approval_set = getUtility(IMessageApprovalSet)
-    >>> message_approval = message_approval_set.getMessageByMessageID(
-    ...     '<caribou>')
-    >>> message_approval.status
-    <DBItem PostedMessageStatus.APPROVAL_PENDING, (20) Approval pending>
-    >>> logout()
-
-    >>> admin_browser.getLink('Moderate mailing list').click()
-    >>> print extract_text(find_tag_by_id(admin_browser.contents, 'legend'))
-    There are no mailing list messages requiring your review.

=== modified file 'lib/lp/registry/tests/test_mailinglist.py'
--- lib/lp/registry/tests/test_mailinglist.py	2012-09-28 06:15:58 +0000
+++ lib/lp/registry/tests/test_mailinglist.py	2012-10-11 01:57:24 +0000
@@ -8,6 +8,7 @@
 
 import transaction
 from zope.component import getUtility
+from testtools.matchers import Equals
 
 from lp.registry.interfaces.mailinglist import (
     CannotChangeSubscription,
@@ -34,6 +35,7 @@
 from lp.testing import (
     login_celebrity,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
@@ -41,6 +43,7 @@
     LaunchpadFunctionalLayer,
     )
 from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import HasQueryCount
 
 
 class PersonMailingListTestCase(TestCaseWithFactory):
@@ -745,6 +748,20 @@
         self.assertEqual(1, held_messages.count())
         self.assertEqual(held_message.message_id, held_messages[0].message_id)
 
+    def test_getReviewableMessages_queries(self):
+        # The Message and user that posted it are retrieved with the query
+        # that get the MessageApproval.
+        test_objects = self.makeMailingListAndHeldMessage()
+        team, member, sender, held_message = test_objects
+        held_messages = team.mailing_list.getReviewableMessages()
+        with StormStatementRecorder() as recorder:
+            held_message = held_messages[0]
+        self.assertThat(recorder, HasQueryCount(Equals(1)))
+        with StormStatementRecorder() as recorder:
+            held_message.message
+            held_message.posted_by
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
 
 class MessageApprovalTestCase(MailingListMessageTestCase):
     """Test the MessageApproval data behaviour."""
@@ -876,17 +893,6 @@
         self.assertEqual(held_message.message.datecreated, details.date)
         self.assertEqual(held_message.message.owner, details.author)
 
-    def test_email_message(self):
-        held_message = self.makeMailingListAndHeldMessage()[-1]
-        details = IHeldMessageDetails(held_message)
-        self.assertEqual('A question', details.email_message['subject'])
-
-    def test_sender(self):
-        test_objects = self.makeMailingListAndHeldMessage()
-        team, member, sender, held_message = test_objects
-        details = IHeldMessageDetails(held_message)
-        self.assertEqual(sender.preferredemail.email, details.sender)
-
     def test_body(self):
         held_message = self.makeMailingListAndHeldMessage()[-1]
         details = IHeldMessageDetails(held_message)