Barry Warsaw pushed to branch master at mailman / Mailman
Commits:
cb8ddd16 by Barry Warsaw at 2016-03-08T23:01:14-05:00
Fix cross-posting held on more than one list.
Closes #176
Also:
* IMessageStore no longer raises a ValueError if the Message-ID already
exists in the store; it just returns None.
* The internal handle_message() function no longer takes a `preserve`
argument, since messages are never removed from the IMessageStore.
- - - - -
8 changed files:
- src/mailman/app/docs/moderator.rst
- src/mailman/app/moderator.py
- src/mailman/app/tests/test_moderation.py
- src/mailman/chains/tests/test_hold.py
- src/mailman/docs/NEWS.rst
- src/mailman/interfaces/messages.py
- src/mailman/model/messagestore.py
- src/mailman/model/tests/test_messagestore.py
Changes:
=====================================
src/mailman/app/docs/moderator.rst
=====================================
--- a/src/mailman/app/docs/moderator.rst
+++ b/src/mailman/app/docs/moderator.rst
@@ -172,36 +172,11 @@ however the message metadata indicates that the message
has been approved.
version : 3
-Preserving and forwarding the message
--------------------------------------
-
-In addition to any of the above dispositions, the message can also be
-preserved for further study. Ordinarily the message is removed from the
-global message store after its disposition (though approved messages may be
-re-added to the message store later). When handling a message, we can ask for
-a copy to be preserve, which skips deleting the message from the storage.
-::
-
- >>> msg = message_from_string("""\
- ... From: d...@example.org
- ... To: a...@example.com
- ... Subject: Something important
- ... Message-ID: <dolphin>
- ...
- ... Here's something important about our mailing list.
- ... """)
- >>> id = hold_message(mlist, msg, {}, 'Needs approval')
- >>> handle_message(mlist, id, Action.discard, preserve=True)
-
- >>> from mailman.interfaces.messages import IMessageStore
- >>> from zope.component import getUtility
- >>> message_store = getUtility(IMessageStore)
- >>> print(message_store.get_message_by_id('<dolphin>')['message-id'])
- <dolphin>
+Forwarding the message
+----------------------
-Orthogonal to preservation, the message can also be forwarded to another
-address. This is helpful for getting the message into the inbox of one of the
-moderators.
+The message can be forwarded to another address. This is helpful for getting
+the message into the inbox of one of the moderators.
::
>>> msg = message_from_string("""\
@@ -241,8 +216,9 @@ the unsubscribing address is required.
Fred is a member of the mailing list...
- >>> mlist.send_welcome_message = False
>>> from mailman.interfaces.usermanager import IUserManager
+ >>> from zope.component import getUtility
+ >>> mlist.send_welcome_message = False
>>> fred = getUtility(IUserManager).create_address(
... 'f...@example.com', 'Fred Person')
>>> from mailman.interfaces.registrar import IRegistrar
=====================================
src/mailman/app/moderator.py
=====================================
--- a/src/mailman/app/moderator.py
+++ b/src/mailman/app/moderator.py
@@ -98,8 +98,7 @@ def hold_message(mlist, msg, msgdata=None, reason=None):
-def handle_message(mlist, id, action,
- comment=None, preserve=False, forward=None):
+def handle_message(mlist, id, action, comment=None, forward=None):
message_store = getUtility(IMessageStore)
requestdb = IListRequests(mlist)
key, msgdata = requestdb.get_request(id)
@@ -108,9 +107,10 @@ def handle_message(mlist, id, action,
message_id = msgdata['_mod_message_id']
sender = msgdata['_mod_sender']
subject = msgdata['_mod_subject']
+ keep = False
if action in (Action.defer, Action.hold):
# Nothing to do, but preserve the message for later.
- preserve = True
+ keep = True
elif action is Action.discard:
rejection = 'Discarded'
elif action is Action.reject:
@@ -174,9 +174,8 @@ def handle_message(mlist, id, action,
fmsg.set_type('message/rfc822')
fmsg.attach(msg)
fmsg.send(mlist)
- # Delete the message from the message store if it is not being preserved.
- if not preserve:
- message_store.delete_message(message_id)
+ # Delete the request if it's not being kept.
+ if not keep:
requestdb.delete_request(id)
# Log the rejection
if rejection:
=====================================
src/mailman/app/tests/test_moderation.py
=====================================
--- a/src/mailman/app/tests/test_moderation.py
+++ b/src/mailman/app/tests/test_moderation.py
@@ -121,31 +121,12 @@ Message-ID: <alpha>
key, data = self._request_db.get_request(request_id)
self.assertEqual(data['received_time'], received_time)
- def test_non_preserving_disposition(self):
- # By default, disposed messages are not preserved.
- request_id = hold_message(self._mlist, self._msg)
- handle_message(self._mlist, request_id, Action.discard)
- message_store = getUtility(IMessageStore)
- self.assertIsNone(message_store.get_message_by_id('<alpha>'))
-
- def test_preserving_disposition(self):
- # Preserving a message keeps it in the store.
- request_id = hold_message(self._mlist, self._msg)
- handle_message(self._mlist, request_id, Action.discard, preserve=True)
- message_store = getUtility(IMessageStore)
- preserved_message = message_store.get_message_by_id('<alpha>')
- self.assertEqual(preserved_message['message-id'], '<alpha>')
-
- def test_preserve_and_forward(self):
- # We can both preserve and forward the message.
+ def test_forward(self):
+ # We can forward the message to an email address.
request_id = hold_message(self._mlist, self._msg)
handle_message(self._mlist, request_id, Action.discard,
- preserve=True, forward=['z...@example.com'])
- # The message is preserved in the store.
- message_store = getUtility(IMessageStore)
- preserved_message = message_store.get_message_by_id('<alpha>')
- self.assertEqual(preserved_message['message-id'], '<alpha>')
- # And the forwarded message lives in the virgin queue.
+ forward=['z...@example.com'])
+ # The forwarded message lives in the virgin queue.
messages = get_queue_messages('virgin')
self.assertEqual(len(messages), 1)
self.assertEqual(str(messages[0].msg['subject']),
@@ -162,6 +143,15 @@ Message-ID: <alpha>
handle_message(self._mlist, request_id, Action.discard)
self.assertEqual(self._request_db.count, 0)
+ def test_handled_message_stays_in_store(self):
+ # The message is still available in the store, even when it's been
+ # disposed of.
+ request_id = hold_message(self._mlist, self._msg)
+ handle_message(self._mlist, request_id, Action.discard)
+ self.assertEqual(self._request_db.count, 0)
+ message = getUtility(IMessageStore).get_message_by_id('<alpha>')
+ self.assertEqual(message['subject'], 'hold me')
+
class TestUnsubscription(unittest.TestCase):
=====================================
src/mailman/chains/tests/test_hold.py
=====================================
--- a/src/mailman/chains/tests/test_hold.py
+++ b/src/mailman/chains/tests/test_hold.py
@@ -30,6 +30,7 @@ from mailman.app.lifecycle import create_list
from mailman.chains.hold import autorespond_to_sender
from mailman.core.chains import process as process_chain
from mailman.interfaces.autorespond import IAutoResponseSet, Response
+from mailman.interfaces.messages import IMessageStore
from mailman.interfaces.usermanager import IUserManager
from mailman.testing.helpers import (
LogFileMark, configuration, get_queue_messages,
@@ -165,3 +166,37 @@ A message body.
self.assertEqual(
msg['Subject'],
't...@example.com post from a...@example.com requires approval')
+
+ def test_hold_chain_crosspost(self):
+ mlist2 = create_list('te...@example.com')
+ msg = mfs("""\
+From: a...@example.com
+To: t...@example.com, te...@example.com
+Subject: A message
+Message-ID: <ant>
+MIME-Version: 1.0
+
+A message body.
+""")
+ process_chain(self._mlist, msg, {}, start_chain='hold')
+ process_chain(mlist2, msg, {}, start_chain='hold')
+ items = get_queue_messages('virgin')
+ # There are four items in the virgin queue. Two of them are for the
+ # list owners who need to moderate the held message, and the other is
+ # for anne telling her that her message was held for approval.
+ self.assertEqual(len(items), 4)
+ anne_froms = set()
+ owner_tos = set()
+ for item in items:
+ if item.msg['to'] == 'a...@example.com':
+ anne_froms.add(item.msg['from'])
+ else:
+ owner_tos.add(item.msg['to'])
+ self.assertEqual(anne_froms, set(['test-boun...@example.com',
+ 'test2-boun...@example.com']))
+ self.assertEqual(owner_tos, set(['test-ow...@example.com',
+ 'test2-ow...@example.com']))
+ # And the message appears in the store.
+ messages = list(getUtility(IMessageStore).messages)
+ self.assertEqual(len(messages), 1)
+ self.assertEqual(messages[0]['message-id'], '<ant>')
=====================================
src/mailman/docs/NEWS.rst
=====================================
--- a/src/mailman/docs/NEWS.rst
+++ b/src/mailman/docs/NEWS.rst
@@ -68,6 +68,7 @@ Bugs
* Trying to subscribe an address as a list owner (or moderator or nonmember)
which is already subscribed with that role produces a server error.
Originally given by Anirudh Dahiya. (Closes #198)
+ * Cross-posting messages held on both lists no longer fails. (Closes #176)
Configuration
-------------
=====================================
src/mailman/interfaces/messages.py
=====================================
--- a/src/mailman/interfaces/messages.py
+++ b/src/mailman/interfaces/messages.py
@@ -64,11 +64,12 @@ class IMessageStore(Interface):
:param message: An email.message.Message instance containing at least
a unique Message-ID header. The message will be given an
- Message-ID-Hash header, overriding any existing such header.
- :returns: The calculated Message-ID-Hash header.
+ Message-ID-Hash header, overriding any existing such header. If
+ the message already exists in the store, it is not added again.
+ :returns: The calculated Message-ID-Hash header or None if the message
+ already exists in the store.
+ :rtype: str or None
:raises ValueError: if the message is missing a Message-ID header.
- The storage service is also allowed to raise this exception if it
- find, but disallows collisions.
"""
def get_message_by_id(message_id):
=====================================
src/mailman/model/messagestore.py
=====================================
--- a/src/mailman/model/messagestore.py
+++ b/src/mailman/model/messagestore.py
@@ -56,13 +56,11 @@ class MessageStore:
message_id = message_ids[0]
if isinstance(message_id, bytes):
message_id = message_id.decode('ascii')
- # Complain if the Message-ID already exists in the storage.
+ # If the Message-ID already exists in the store, don't store it again.
existing = store.query(Message).filter(
Message.message_id == message_id).first()
if existing is not None:
- raise ValueError(
- 'Message ID already exists in message store: {0}'.format(
- message_id))
+ return None
hash32 = add_message_hash(message)
# Calculate the path on disk where we're going to store this message
# object, in pickled format.
=====================================
src/mailman/model/tests/test_messagestore.py
=====================================
--- a/src/mailman/model/tests/test_messagestore.py
+++ b/src/mailman/model/tests/test_messagestore.py
@@ -137,3 +137,22 @@ X-Message-ID-Hash: abc
self.assertEqual(len(x_message_id_hashes), 1)
self.assertEqual(x_message_id_hashes[0],
'MS6QLWERIJLGCRF44J7USBFDELMNT2BW')
+
+ def test_add_message_duplicate_okay(self):
+ msg = mfs("""\
+Subject: Once
+Message-ID: <ant>
+
+""")
+ hash32 = self._store.add(msg)
+ stored_msg = self._store.get_message_by_id('<ant>')
+ self.assertEqual(msg['subject'], stored_msg['subject'])
+ self.assertEqual(msg['message-id-hash'], hash32)
+ # A second insertion, even if headers change, does not store the
+ # message twice.
+ del msg['subject']
+ msg['Subject'] = 'Twice'
+ hash32 = self._store.add(msg)
+ stored_msg = self._store.get_message_by_id('<ant>')
+ self.assertNotEqual(msg['subject'], stored_msg['subject'])
+ self.assertIsNone(hash32)
View it on GitLab:
https://gitlab.com/mailman/mailman/commit/cb8ddd1671e424478f002e527871f0c5839425d9
_______________________________________________
Mailman-checkins mailing list
Mailman-checkins@python.org
Unsubscribe:
https://mail.python.org/mailman/options/mailman-checkins/archive%40jab.org
