launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01402
[Merge] lp:~sinzui/launchpad/hold-message into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/hold-message into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
This is my branch to forward small messages to the moderation queue.
lp:~sinzui/launchpad/hold-message
Diff size: 311
Launchpad bug:
https://bugs.launchpad.net/bugs/645702
Test command: ./bin/test -vv \
-t test_lpsize
Pre-implementation: EdwinGrubbs, jcsacket
Target release: 10.10
Forward small messages to the moderation queue
-----------------------------------------------
OOPS-1726XMLP285 shows that mailman forwarded a message for moderation that
exceeds the db storage limit.
There are two views on this issue. The first is that this is a copy of a
message mailman wants an admin to decide if the real message should be sent to
the list or discarded--the person only needs to see enough to make a decision.
the second issue is that mailman has rules for handling large messages and
they do not appear to agree with Lp. The soft limit enters moderation (which
is this case), and the hard limit is discarded immediately. I recall the soft
limit was raised at the request of statik.
Rules
-----
* Update the lpsize tests to be unittests and delete the tests from
postings.txt (often fails because of timeouts)
* Add a rule to truncate the message so that it is small enough to
be moderated.
* Delete non text/plain parts because Lp only stores text/plain.
* Truncate large text/parts. 10k will be enough for the moderator
to make a decision to discard or forward. This message is shown
in a list of messages; we want to limit the text to ensure the page
is usable.
QA
--
* Send an email with a large attachment to a list on staging.
the attachment must be larger than 11m and not over 20m
* Verify the message appears in the moderation queue
Lint
----
Linting changed files:
lib/lp/services/mailman/doc/postings.txt
lib/lp/services/mailman/monkeypatches/lpsize.py
lib/lp/services/mailman/testing/__init__.py
lib/lp/services/mailman/tests/test_lpsize.py
Test
----
* lib/lp/services/mailman/doc/postings.txt
* Removed redundant test that were prone to fail
* lib/lp/services/mailman/testing/__init__.py
* Fixed the test harness to make a proper email with an attachment.
These tests were the first to really exercise it.
* lib/lp/services/mailman/tests/test_lpsize.py
* Converted the doctests to unittests
* Added tests for the truncated_message function.
Implementation
--------------
* lib/lp/services/mailman/monkeypatches/lpsize.py
* Added truncated_message to create a small message and used it when
calling hold().
--
https://code.launchpad.net/~sinzui/launchpad/hold-message/+merge/37750
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/hold-message into lp:launchpad/devel.
=== modified file 'lib/lp/services/mailman/doc/postings.txt'
--- lib/lp/services/mailman/doc/postings.txt 2010-10-03 15:30:06 +0000
+++ lib/lp/services/mailman/doc/postings.txt 2010-10-06 15:06:08 +0000
@@ -832,79 +832,3 @@
>>> vette_watcher.wait_for_discard('narwhale')
>>> len(list(smtpd))
0
-
-
-Large messages
-==============
-
-Only messages which are less than about 40k in size are allowed straight
-through on the mailing list. A message bigger than that will be held for
-moderator approval.
-
- >>> from canonical.config import config
- >>> config.mailman.soft_max_size
- 40000
-
- >>> from email.mime.multipart import MIMEMultipart
- >>> from email.mime.application import MIMEApplication
- >>> from email.mime.text import MIMEText
-
- >>> big_message = MIMEMultipart()
- >>> big_message['From'] = 'anne.person@xxxxxxxxxxx'
- >>> big_message['To'] = 'itest-one@xxxxxxxxxxxxxxxxxxx'
- >>> big_message['Subject'] = 'A huge message'
- >>> big_message['Message-ID'] = '<puma>'
- >>> big_message.attach(
- ... MIMEText('look at this pdf.', 'plain'))
- >>> big_message.attach(
- ... MIMEApplication('\n'.join(['x' * 50] * 1000), 'octet-stream'))
- >>> inject('itest-one', big_message.as_string())
-
- >>> vette_watcher.wait_for_hold('itest-one', 'puma')
- >>> print_message_summaries()
- Number of messages: 1
- bounces@xxxxxxxxxxxxx
- ...
- Itest One <noreply@xxxxxxxxxxxxx>
- New mailing list message requiring approval for Itest One
-
-Once this message is approved, it is posted through to the mailing list.
-
- >>> browser.open(
- ... 'http://launchpad.dev:8085/~itest-one/+mailinglist-moderate')
- >>> browser.getControl(name='field.%3Cpuma%3E').value = ['approve']
- >>> browser.getControl('Moderate').click()
- >>> smtpd_watcher.wait_for_mbox_delivery('puma')
- >>> smtpd_watcher.wait_for_mbox_delivery('puma')
-
- >>> print_message_summaries()
- Number of messages: 2
- itest-one-bounces+anne.person=example.com@xxxxxxxxxxxxxxxxxxx
- <puma>
- anne.person@xxxxxxxxxxx
- [Itest-one] A huge message
- itest-one-bounces+archive=mail-archive.dev@xxxxxxxxxxxxxxxxxxx
- <puma>
- anne.person@xxxxxxxxxxx
- [Itest-one] A huge message
-
-There is a hard limit of 1MB, over which the message is summarily logged and
-discarded.
-
- >>> config.mailman.hard_max_size
- 1000000
-
- >>> huge_message = MIMEMultipart()
- >>> huge_message['From'] = 'anne.person@xxxxxxxxxxx'
- >>> huge_message['To'] = 'itest-one@xxxxxxxxxxxxxxxxxxx'
- >>> huge_message['Subject'] = 'A huge message'
- >>> huge_message['Message-ID'] = '<quahog>'
- >>> huge_message.attach(
- ... MIMEText('look at this huge pdf.', 'plain'))
- >>> huge_message.attach(
- ... MIMEApplication('\n'.join(['x' * 50] * 20000), 'octet-stream'))
- >>> inject('itest-one', huge_message.as_string())
-
- >>> vette_watcher.wait_for_discard('quahog')
- >>> print_message_summaries()
- Number of messages: 0
=== 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:06:08 +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':
+ # 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]',
+ 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.
@@ -42,7 +70,10 @@
if message_size < mm_cfg.LAUNCHPAD_HARD_MAX_SIZE:
# Hold the message in Mailman. See lpmoderate.py for similar
# algorithm.
- hold(mlist, msg, msgdata, 'Too big')
+ # There is a limit to the size that can be stored in Lp. Send
+ # a trucated copy of the message that has enough information for
+ # the moderator to make a decision.
+ hold(mlist, truncated_message(msg), msgdata, 'Too big')
# The message is larger than the hard limit, so log and discard.
syslog('vette', 'Discarding message w/size > hard limit: %s',
msg.get('message-id', 'n/a'))
=== modified file 'lib/lp/services/mailman/testing/__init__.py'
--- lib/lp/services/mailman/testing/__init__.py 2010-10-04 19:50:45 +0000
+++ lib/lp/services/mailman/testing/__init__.py 2010-10-06 15:06:08 +0000
@@ -7,6 +7,7 @@
from contextlib import contextmanager
import email
+from email.mime.multipart import MIMEMultipart
from email.mime.text import MIMEText
import os
import shutil
@@ -87,13 +88,16 @@
# Make a Mailman Message.Message.
if isinstance(sender, (list, tuple)):
sender = ', '.join(sender)
- message = MIMEText(content, mime_type)
+ message = MIMEMultipart()
message['from'] = sender
message['to'] = mm_list.getListAddress()
message['subject'] = subject
message['message-id'] = self.getUniqueString()
+ message.attach(MIMEText(content, mime_type))
+ if attachment is not None:
+ # Rewrap the text message in a multipart message and add the
+ # attachment.
+ message.attach(attachment)
mm_message = email.message_from_string(
message.as_string(), Message.Message)
- if attachment is not None:
- mm_message.attach(attachment, 'octet-stream')
return mm_message
=== 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:06:08 +0000
@@ -0,0 +1,129 @@
+# Copyright 20010 Canonical Ltd. This software is licensed under the
+# 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
+ 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
+ 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.
+ 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())
+
+
+class TestTruncatedMessage(MailmanTestCase):
+ """Test truncated_message helper."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestTruncatedMessage, 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 test_attchments_are_removed(self):
+ # Plain-text and multipart are preserved, everything else is removed.
+ attachment = MIMEApplication('binary gibberish', 'octet-stream')
+ message = self.makeMailmanMessage(
+ self.mm_list, self.subscriber_email, 'subject', 'content',
+ attachment=attachment)
+ moderated_message = LPSize.truncated_message(message)
+ parts = [part for part in moderated_message.walk()]
+ types = [part.get_content_type() for part in parts]
+ self.assertEqual(['multipart/mixed', 'text/plain'], types)
+
+ def test_small_text_is_preserved(self):
+ # Text parts below the limit are unchanged.
+ message = self.makeMailmanMessage(
+ self.mm_list, self.subscriber_email, 'subject', 'content')
+ moderated_message = LPSize.truncated_message(message, limit=1000)
+ parts = [part for part in moderated_message.walk()]
+ types = [part.get_content_type() for part in parts]
+ self.assertEqual(['multipart/mixed', 'text/plain'], types)
+ self.assertEqual('content', parts[1].get_payload())
+
+ def test_large_text_is_truncated(self):
+ # Text parts above the limit are truncated.
+ message = self.makeMailmanMessage(
+ self.mm_list, self.subscriber_email, 'subject', 'content excess')
+ moderated_message = LPSize.truncated_message(message, limit=7)
+ parts = [part for part in moderated_message.walk()]
+ types = [part.get_content_type() for part in parts]
+ self.assertEqual(['multipart/mixed', 'text/plain'], types)
+ self.assertEqual(
+ 'content [truncated for moderation]', parts[1].get_payload())
