launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/moderated-messages-0 into lp:launchpad.

Commit message:
Fix message moderation timeouts by avoiding the librarian.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #736012 in Launchpad itself: "Person:+mailinglist-moderate timeout"
  https://bugs.launchpad.net/launchpad/+bug/736012

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/moderated-messages-0/+merge/129013

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

-- 
https://code.launchpad.net/~sinzui/launchpad/moderated-messages-0/+merge/129013
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/moderated-messages-0 into lp:launchpad.

=== 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-10 18:00:38 +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`."""

=== modified 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	2012-10-10 18:00:38 +0000
@@ -38,7 +38,7 @@
     >>> print view.date
     2000-08-01 01:09:00+00:00
     >>> print view.author
-    <a href="http://launchpad.dev/~carlos";>Carlos &lt;carlos@xxxxxxxxxxxxx&gt;</a>
+    <a href="/~carlos" class="sprite person">Carlos Perell... Marín</a>
     >>> print view.body_summary
     First paragraph.
     >>> print view.body_details

=== 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-10 18:00:38 +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-10 18:00:38 +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):

=== 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-10 18:00:38 +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)