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.
Hi Curtis,
This is a nice improvement. I have some comments below.
-Edwin
>=== modified file 'lib/lp/ services/ mailman/ monkeypatches/ lpsize. py' services/ mailman/ monkeypatches/ lpsize. py 2010-08-20 20:31:18 +0000 services/ mailman/ monkeypatches/ lpsize. py 2010-10-06 15:36:24 +0000 Handlers. LPModerate import hold Logging. Syslog import syslog message( original_ message, limit=10000): from_string( message. as_string( ), Message.Message) .typed_ subpart_ iterator( message, 'multipart'): get_content_ maintype( ) == 'multipart': get_content_ type() == 'text/plain':
>--- lib/lp/
>+++ lib/lp/
>@@ -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.
> from Mailman.
>
>
>+def truncated_
>+ """Create a smaller version of the message for moderation."""
>+ message = email.message_
>+ original_
>+ for part in email.iterators
>+ subparts = part.get_payload()
>+ removeable = []
>+ for subpart in subparts:
>+ if subpart.
>+ # This part is handled in the outer loop.
>+ continue
>+ elif subpart.
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 get_payload( ).strip( ) set_payload(
>+ # information for the moderator to make a decision.
>+ content = subpart.
>+ if len(content) > limit:
>+ subpart.
>+ 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( )) append( subpart) remove( subpart) services/ mailman/ tests/test_ lpsize. py' services/ mailman/ tests/test_ lpsize. py 1970-01-01 00:00:00 +0000 services/ mailman/ tests/test_ lpsize. py 2010-10-06 15:36:24 +0000
>+ else:
>+ removeable.
>+ for subpart in removeable:
>+ subparts.
>+ return message
>+
>+
> def process(mlist, msg, msgdata):
> """Check the message size (approximately) against hard and soft limits.
>
>=== added file 'lib/lp/
>--- lib/lp/
>+++ lib/lp/
>@@ -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). application import MIMEApplication nalLayer, onalLayer, mailman. testing import MailmanTestCase ase(MailmanTest Case):
>+"""Test the lpsize monekypatches"""
>+
>+from __future__ import with_statement
>+
>+__metaclass__ = type
>+__all__ = []
>+
>+from email.mime.
>+
>+from Mailman import Errors
>+from Mailman.Handlers import LPSize
>+
>+from canonical.config import config
>+from canonical.testing import (
>+ DatabaseFunctio
>+ LaunchpadFuncti
>+ )
>+from lp.services.
>+
>+
>+class TestLPSizeTestC
>+ """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. onalLayer eTestCase, self).setUp() makeTeamAndMail ingList( nList(self. mailing_ list) _email = self.team. teamowner. preferredemail. email eTestCase, self).tearDown() anList( self.mm_ list) size_under_ soft_limit( self): nMessage( _email, 'subject', 'content', attachment) process( self.mm_ list, message, msg_data) l(None, silence) size_over_ soft_limit_ held(self) :
>+ """
>+
>+ layer = LaunchpadFuncti
>+
>+ def setUp(self):
>+ super(TestLPSiz
>+ self.team, self.mailing_list = self.factory.
>+ 'team-1', 'team-1-owner')
>+ self.mm_list = self.makeMailma
>+ self.subscriber
>+
>+ def tearDown(self):
>+ super(TestLPSiz
>+ self.cleanMailm
>+
>+ def test_process_
>+ # Any message under 40kb is sent to the list.
>+ attachment = MIMEApplication(
>+ '\n'.join(['x' * 20] * 1000), 'octet-stream')
>+ message = self.makeMailma
>+ self.mm_list, self.subscriber
>+ attachment=
>+ msg_data = {}
>+ silence = LPSize.
>+ self.assertEqua
>+
>+ def test_process_
>+ # Messages over 40km held for moderation.
Uh, based on how many millimeters per character?
>+ self.assertEqua l(40000, config. mailman. soft_max_ size) nMessage( _email, 'subject', 'content', attachment) list.getReviewa bleMessages( ).count( )) size_over_ hard_limit_ discarded( self): l(1000000, config. mailman. hard_max_ size) nMessage( _email, 'subject', 'content', attachment) DiscardMessage, LPSize.process, *args) list.getReviewa bleMessages( ).count( ))
>+ attachment = MIMEApplication(
>+ '\n'.join(['x' * 40] * 1000), 'octet-stream')
>+ message = self.makeMailma
>+ self.mm_list, self.subscriber
>+ attachment=
>+ msg_data = {}
>+ args = (self.mm_list, message, msg_data)
>+ self.assertRaises(
>+ Errors.HoldMessage, LPSize.process, *args)
>+ self.assertEqual(1, self.mailing_
>+
>+ def test_process_
>+ # Messages over 1MB are discarded.
>+ self.assertEqua
>+ attachment = MIMEApplication(
>+ '\n'.join(['x' * 1000] * 1000), 'octet-stream')
>+ message = self.makeMailma
>+ self.mm_list, self.subscriber
>+ attachment=
>+ msg_data = {}
>+ args = (self.mm_list, message, msg_data)
>+ self.assertRaises(
>+ Errors.
>+ self.assertEqual(0, self.mailing_
>+