Merge lp:~abompard/mailman/bug-1130696 into lp:mailman

[Aurélien Bompard]

Handle the MIMEMultipart created by the DigestRunner properly, fixes bug #1130696.

[Barry Warsaw (barry)]

This doesn't quite feel like it's getting to the root cause. Why don't we get a mailman.email.message.Message object here?

[Aurélien Bompard (abompard)]

Because the digest runner is creating a MIMEMultipart instance on line 175 of src/mailman/runners/digest.py. Should we change that instead?

[Barry Warsaw (barry)]

> Because the digest runner is creating a MIMEMultipart instance on line 175 of
> src/mailman/runners/digest.py. Should we change that instead?

Good catch. Yes, I think we should change that. What I think I'd like to see is a test that creates a mime digest, and uses that to trigger the attribute errors, rather than creating a MIMEMultipart inside the tests themselves. Does that makes sense?

Then the fix would be to change MIMEDigester._make_message() to create the proper subclass. The question then is what that method should return. My first thought is a class that inherits both from MIMEMultipart and mailman.email.message.Message.

What do you think?

[lp:~abompard/mailman/bug-1130696]

7271. By Aurélien Bompard on 2014-12-09

Make the MIMEDigester generate a subclass of MIMEMultipart with our additions

Fixes bug #1130696 (differently)

[Aurélien Bompard (abompard)]

Alright, I just changed that, and I tried generating a very simple subclass of MIMEMultipart and our Message instance. All tests seem to pass, but I'm not very familiar with the tricks of multiple inheritance / mixins.
I'm pushing the branch right now.

[Barry Warsaw (barry)]

The general approach works great. I just did some cleanup and added a test (mostly because during my merge, I made a typo that broke digester.rst and required more investigation ;).

As always, thanks!

review: Approve

=== modified file 'src/mailman/core/tests/test_runner.py'
--- src/mailman/core/tests/test_runner.py	2014-01-01 14:59:42 +0000
+++ src/mailman/core/tests/test_runner.py	2014-12-09 11:27:43 +0000
@@ -25,6 +25,7 @@
     ]
 
 
+import os
 import unittest
 
 from mailman.app.lifecycle import create_list
@@ -32,8 +33,9 @@
 from mailman.core.runner import Runner
 from mailman.interfaces.runner import RunnerCrashEvent
 from mailman.testing.helpers import (
-    configuration, event_subscribers, get_queue_messages,
-    make_testable_runner, specialized_message_from_string as mfs)
+    configuration, event_subscribers, get_queue_messages, LogFileMark,
+    make_testable_runner, specialized_message_from_string as mfs,
+    make_digest_messages)
 from mailman.testing.layers import ConfigLayer
 
 
@@ -42,6 +44,11 @@
     def _dispose(self, mlist, msg, msgdata):
         raise RuntimeError('borked')
 
+
 
+class StoringRunner(Runner):
+    _disposed = []
+    def _dispose(self, mlist, msg, msgdata):
+        self._disposed.append((mlist, msg, msgdata))
 
 
 
 class TestRunner(unittest.TestCase):
@@ -87,3 +94,17 @@
         shunted = get_queue_messages('shunt')
         self.assertEqual(len(shunted), 1)
         self.assertEqual(shunted[0].msg['message-id'], '<ant>')
+
+    def test_digest_messages(self):
+        # Digest messages can be MIMEMultipart (LP#1130696)
+        runner = make_testable_runner(StoringRunner, 'in')
+        make_digest_messages(self._mlist)
+        for bag in get_queue_messages("virgin"):
+            config.switchboards['in'].enqueue(bag.msg, listname='[e-mail]')
+        error_log = LogFileMark('mailman.error')
+        with event_subscribers(self._got_event):
+            runner.run()
+        errors = error_log.read()
+        self.assertEqual(len(self._events), 0, errors)
+        self.assertEqual(len(get_queue_messages("shunt")), 0, errors)
+        self.assertEqual(len(runner._disposed), 2)
 
=== modified file 'src/mailman/email/message.py'
--- src/mailman/email/message.py	2014-12-09 01:38:26 +0000
+++ src/mailman/email/message.py	2014-12-09 11:27:43 +0000
@@ -36,6 +36,7 @@
 import email
 import email.message
 import email.utils
+import email.mime.multipart
 
 from email.header import Header
 
@@ -133,6 +134,11 @@
 
 
 
 
+class MultipartMessage(email.mime.multipart.MIMEMultipart, Message):
+    """Class for MIME digest messages"""
+
+
+
 
 class UserNotification(Message):
     """Class for internally crafted messages."""
 
 
=== added file 'src/mailman/handlers/tests/test_cook_headers.py'
--- src/mailman/handlers/tests/test_cook_headers.py	1970-01-01 00:00:00 +0000
+++ src/mailman/handlers/tests/test_cook_headers.py	2014-12-09 11:27:43 +0000
@@ -0,0 +1,54 @@
+# Copyright (C) 2012-2014 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test the cook_headers handler."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'TestCookHeaders',
+    ]
+
+
+import unittest
+
+from mailman.app.lifecycle import create_list
+from mailman.handlers import cook_headers
+from mailman.testing.helpers import get_queue_messages, make_digest_messages
+from mailman.testing.layers import ConfigLayer
+
+
+
 
+class TestCookHeaders(unittest.TestCase):
+    """Test the cook_headers handler."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('[e-mail]')
+
+    def test_process_digest(self):
+        # Digest messages can be MIMEMultipart (LP#1130696)
+        make_digest_messages(self._mlist)
+        messages = [ bag.msg for bag in get_queue_messages("virgin") ]
+        self.assertEqual(len(messages), 2)
+        for msg in messages:
+            try:
+                cook_headers.process(self._mlist, msg, {})
+            except AttributeError as e:
+                self.fail(e)
 
=== modified file 'src/mailman/runners/digest.py'
--- src/mailman/runners/digest.py	2014-11-29 20:10:59 +0000
+++ src/mailman/runners/digest.py	2014-12-09 11:27:43 +0000
@@ -32,7 +32,6 @@
 from StringIO import StringIO
 from copy import deepcopy
 from email.header import Header
-from email.message import Message
 from email.mime.message import MIMEMessage
 from email.mime.multipart import MIMEMultipart
 from email.mime.text import MIMEText
@@ -42,6 +41,7 @@
 from mailman.config import config
 from mailman.core.i18n import _
 from mailman.core.runner import Runner
+from mailman.email.message import Message, MultipartMessage
 from mailman.handlers.decorate import decorate
 from mailman.interfaces.member import DeliveryMode, DeliveryStatus
 from mailman.utilities.i18n import make
@@ -172,7 +172,7 @@
         self._keepers = set(config.digests.mime_digest_keep_headers.split())
 
     def _make_message(self):
-        return MIMEMultipart('mixed')
+        return MultipartMessage('mixed')
 
     def add_toc(self, count):
         """Add the table of contents."""
 
=== modified file 'src/mailman/runners/tests/test_digest.py'
--- src/mailman/runners/tests/test_digest.py	2014-11-29 20:10:59 +0000
+++ src/mailman/runners/tests/test_digest.py	2014-12-09 11:27:43 +0000
@@ -35,7 +35,7 @@
 from mailman.runners.digest import DigestRunner
 from mailman.testing.helpers import (
     LogFileMark, digest_mbox, get_queue_messages, make_testable_runner,
-    specialized_message_from_string as mfs)
+    specialized_message_from_string as mfs, make_digest_messages)
 from mailman.testing.layers import ConfigLayer
 
 
@@ -54,31 +54,21 @@
         self._runner = make_testable_runner(DigestRunner, 'digest')
         self._process = config.handlers['to-digest'].process
 
+    def _check_virgin_queue(self):
+        # There should be two messages in the virgin queue: the digest as
+        # plain-text and as multipart.
+        messages = get_queue_messages('virgin')
+        self.assertEqual(len(messages), 2)
+        self.assertEqual(
+            sorted(item.msg.get_content_type() for item in messages),
+            ['multipart/mixed', 'text/plain'])
+        for item in messages:
+            self.assertEqual(item.msg['subject'],
+                             'Test Digest, Vol 1, Issue 1')
+
     def test_simple_message(self):
-        msg = mfs("""\
-From: [e-mail]
-To: [e-mail]
-
-message triggering a digest
-""")
-        mbox_path = os.path.join(self._mlist.data_path, 'digest.mmdf')
-        self._process(self._mlist, msg, {})
-        self._digestq.enqueue(
-            msg,
-            listname=self._mlist.fqdn_listname,
-            digest_path=mbox_path,
-            volume=1, digest_number=1)
-        self._runner.run()
-        # There are two messages in the virgin queue: the digest as plain-text
-        # and as multipart.
-        messages = get_queue_messages('virgin')
-        self.assertEqual(len(messages), 2)
-        self.assertEqual(
-            sorted(item.msg.get_content_type() for item in messages),
-            ['multipart/mixed', 'text/plain'])
-        for item in messages:
-            self.assertEqual(item.msg['subject'],
-                             'Test Digest, Vol 1, Issue 1')
+        make_digest_messages(self._mlist)
+        self._check_virgin_queue()
 
     def test_non_ascii_message(self):
         msg = Message()
@@ -88,25 +78,10 @@
         msg.attach(MIMEText('message with non-ascii chars: \xc3\xa9',
                             'plain', 'utf-8'))
         mbox = digest_mbox(self._mlist)
-        mbox_path = os.path.join(self._mlist.data_path, 'digest.mmdf')
         mbox.add(msg.as_string())
-        self._digestq.enqueue(
-            msg,
-            listname=self._mlist.fqdn_listname,
-            digest_path=mbox_path,
-            volume=1, digest_number=1)
         # Use any error logs as the error message if the test fails.
         error_log = LogFileMark('mailman.error')
-        self._runner.run()
+        make_digest_messages(self._mlist, msg)
         # The runner will send the file to the shunt queue on exception.
         self.assertEqual(len(self._shuntq.files), 0, error_log.read())
-        # There are two messages in the virgin queue: the digest as plain-text
-        # and as multipart.
-        messages = get_queue_messages('virgin')
-        self.assertEqual(len(messages), 2)
-        self.assertEqual(
-            sorted(item.msg.get_content_type() for item in messages),
-            ['multipart/mixed', 'text/plain'])
-        for item in messages:
-            self.assertEqual(item.msg['subject'],
-                             'Test Digest, Vol 1, Issue 1')
+        self._check_virgin_queue()
 
=== modified file 'src/mailman/testing/helpers.py'
--- src/mailman/testing/helpers.py	2014-11-29 20:10:59 +0000
+++ src/mailman/testing/helpers.py	2014-12-09 11:27:43 +0000
@@ -72,6 +72,7 @@
 from mailman.interfaces.messages import IMessageStore
 from mailman.interfaces.styles import IStyleManager
 from mailman.interfaces.usermanager import IUserManager
+from mailman.runners.digest import DigestRunner
 from mailman.utilities.mailbox import Mailbox
 
 
@@ -529,3 +530,24 @@
         with open(self._filename) as fp:
             fp.seek(self._filepos)
             return fp.read()
+
+
+
 
+def make_digest_messages(mlist, msg=None):
+    if msg is None:
+        msg = specialized_message_from_string("""\
+From: [e-mail]
+To: {listname}
+Message-ID: <testing>
+
+message triggering a digest
+""".format(listname=mlist.fqdn_listname))
+    mbox_path = os.path.join(mlist.data_path, 'digest.mmdf')
+    config.handlers['to-digest'].process(mlist, msg, {})
+    config.switchboards['digest'].enqueue(
+        msg,
+        listname=mlist.fqdn_listname,
+        digest_path=mbox_path,
+        volume=1, digest_number=1)
+    runner = make_testable_runner(DigestRunner, 'digest')
+    runner.run()