Code review comment for lp:~sinzui/launchpad/hold-message

Hi Curtis,

This is a nice improvement. I have some comments below.

-Edwin

>=== modified file 'lib/lp/services/mailman/monkeypatches/lpsize.py'
>--- lib/lp/services/mailman/monkeypatches/lpsize.py 2010-08-20 20:31:18 +0000
>+++ lib/lp/services/mailman/monkeypatches/lpsize.py 2010-10-06 15:36:24 +0000
>@@ -3,15 +3,43 @@
>
> """A pipeline handler for checking message sizes."""
>
>-# pylint: disable-msg=F0401
>+import email
>+
> from Mailman import (
> Errors,
>+ Message,
> mm_cfg,
> )
> from Mailman.Handlers.LPModerate import hold
> from Mailman.Logging.Syslog import syslog
>
>
>+def truncated_message(original_message, limit=10000):
>+ """Create a smaller version of the message for moderation."""
>+ message = email.message_from_string(
>+ original_message.as_string(), Message.Message)
>+ for part in email.iterators.typed_subpart_iterator(message, 'multipart'):
>+ subparts = part.get_payload()
>+ removeable = []
>+ for subpart in subparts:
>+ if subpart.get_content_maintype() == 'multipart':
>+ # This part is handled in the outer loop.
>+ continue
>+ elif subpart.get_content_type() == 'text/plain':

Shouldn't it truncate text/html or any other "text/" content-type,
since the user's mail client might create an empty text/plain section?

>+ # Truncate the message at 10kb so that there is enough
>+ # information for the moderator to make a decision.
>+ content = subpart.get_payload().strip()
>+ if len(content) > limit:
>+ subpart.set_payload(
>+ content[:limit] + ' [truncated for moderation]',

It seems like [truncated for moderation] will be easier to see if
it is on a new line.

>+ subpart.get_charset())
>+ else:
>+ removeable.append(subpart)
>+ for subpart in removeable:
>+ subparts.remove(subpart)
>+ return message
>+
>+
> def process(mlist, msg, msgdata):
> """Check the message size (approximately) against hard and soft limits.
>
>=== added file 'lib/lp/services/mailman/tests/test_lpsize.py'
>--- lib/lp/services/mailman/tests/test_lpsize.py 1970-01-01 00:00:00 +0000
>+++ lib/lp/services/mailman/tests/test_lpsize.py 2010-10-06 15:36:24 +0000
>@@ -0,0 +1,129 @@
>+# Copyright 20010 Canonical Ltd. This software is licensed under the

I think the copyright will expire by the time Bender and Leela can use
it.

>+# GNU Affero General Public License version 3 (see the file LICENSE).
>+"""Test the lpsize monekypatches"""
>+
>+from __future__ import with_statement
>+
>+__metaclass__ = type
>+__all__ = []
>+
>+from email.mime.application import MIMEApplication
>+
>+from Mailman import Errors
>+from Mailman.Handlers import LPSize
>+
>+from canonical.config import config
>+from canonical.testing import (
>+ DatabaseFunctionalLayer,
>+ LaunchpadFunctionalLayer,
>+ )
>+from lp.services.mailman.testing import MailmanTestCase
>+
>+
>+class TestLPSizeTestCase(MailmanTestCase):
>+ """Test LPSize.
>+
>+ Mailman process() methods quietly return. They may set msg_data key-values
>+ or raise an error to end processing. This group of tests tests often check

s/tests tests/tests/
Either, use "These tests" instead of "This group of tests" or change
"check" to "checks".

>+ for errors, but that does not mean there is an error condition, it only
>+ means message processing has reached a final decision. Messages that do
>+ not cause a final decision pass-through and the process() methods ends

"pass-through" with a hyphen looks like a noun instead of a verb.
Add a comma before the "and".

>+ without a return.
>+ """
>+
>+ layer = LaunchpadFunctionalLayer
>+
>+ def setUp(self):
>+ super(TestLPSizeTestCase, self).setUp()
>+ self.team, self.mailing_list = self.factory.makeTeamAndMailingList(
>+ 'team-1', 'team-1-owner')
>+ self.mm_list = self.makeMailmanList(self.mailing_list)
>+ self.subscriber_email = self.team.teamowner.preferredemail.email
>+
>+ def tearDown(self):
>+ super(TestLPSizeTestCase, self).tearDown()
>+ self.cleanMailmanList(self.mm_list)
>+
>+ def test_process_size_under_soft_limit(self):
>+ # Any message under 40kb is sent to the list.
>+ attachment = MIMEApplication(
>+ '\n'.join(['x' * 20] * 1000), 'octet-stream')
>+ message = self.makeMailmanMessage(
>+ self.mm_list, self.subscriber_email, 'subject', 'content',
>+ attachment=attachment)
>+ msg_data = {}
>+ silence = LPSize.process(self.mm_list, message, msg_data)
>+ self.assertEqual(None, silence)
>+
>+ def test_process_size_over_soft_limit_held(self):
>+ # Messages over 40km held for moderation.

Uh, based on how many millimeters per character?

>+ self.assertEqual(40000, config.mailman.soft_max_size)
>+ attachment = MIMEApplication(
>+ '\n'.join(['x' * 40] * 1000), 'octet-stream')
>+ message = self.makeMailmanMessage(
>+ self.mm_list, self.subscriber_email, 'subject', 'content',
>+ attachment=attachment)
>+ msg_data = {}
>+ args = (self.mm_list, message, msg_data)
>+ self.assertRaises(
>+ Errors.HoldMessage, LPSize.process, *args)
>+ self.assertEqual(1, self.mailing_list.getReviewableMessages().count())
>+
>+ def test_process_size_over_hard_limit_discarded(self):
>+ # Messages over 1MB are discarded.
>+ self.assertEqual(1000000, config.mailman.hard_max_size)
>+ attachment = MIMEApplication(
>+ '\n'.join(['x' * 1000] * 1000), 'octet-stream')
>+ message = self.makeMailmanMessage(
>+ self.mm_list, self.subscriber_email, 'subject', 'content',
>+ attachment=attachment)
>+ msg_data = {}
>+ args = (self.mm_list, message, msg_data)
>+ self.assertRaises(
>+ Errors.DiscardMessage, LPSize.process, *args)
>+ self.assertEqual(0, self.mailing_list.getReviewableMessages().count())
>+