Merge lp:~abompard/mailman/moderation_reasons into lp:mailman

[Aurélien Bompard]

Currently moderation reasons are always 'XXX'. This branch fixes that with a message that comes from the moderation rules that matched.

[Barry Warsaw (barry)]

Thanks for the contribution. I've adapted this to the current git trunk and will apply over there.

review: Approve

=== modified file 'src/mailman/app/docs/chains.rst'
--- src/mailman/app/docs/chains.rst	2012-03-07 23:45:09 +0000
+++ src/mailman/app/docs/chains.rst	2014-04-17 20:19:21 +0000
@@ -132,7 +132,7 @@
         List:    [e-mail]
         From:    [e-mail]
         Subject: My first post
-        Reason:  XXX
+        Reasons:  (n/a)
     <BLANKLINE>
     At your convenience, visit:
     <BLANKLINE>
@@ -189,7 +189,7 @@
     <BLANKLINE>
     The reason it is being held:
     <BLANKLINE>
-        XXX
+        (n/a)
     <BLANKLINE>
     Either the message will get posted to the list, or you will receive
     notification of the moderator's decision.  If you would like to cancel
 
=== modified file 'src/mailman/chains/hold.py'
--- src/mailman/chains/hold.py	2014-01-01 14:59:42 +0000
+++ src/mailman/chains/hold.py	2014-04-17 20:19:21 +0000
@@ -26,6 +26,7 @@
 
 
 import logging
+import re
 
 from email.mime.message import MIMEMessage
 from email.mime.text import MIMEText
@@ -138,7 +139,6 @@
         if rule_misses:
             msg['X-Mailman-Rule-Misses'] = SEMISPACE.join(rule_misses)
         # Hold the message by adding it to the list's request database.
-        # XXX How to calculate the reason?
         request_id = hold_message(mlist, msg, msgdata, None)
         # Calculate a confirmation token to send to the author of the
         # message.
@@ -158,11 +158,13 @@
             original_subject = _('(no subject)')
         else:
             original_subject = oneline(original_subject, charset)
+        reasons = msgdata.get("moderation_reasons", [_("(n/a)")])
         substitutions = dict(
             listname    = mlist.fqdn_listname,
             subject     = original_subject,
             sender      = msg.sender,
-            reason      = 'XXX', #reason,
+            reasons     = "\r\n".join(
+                ['    ' + wrap(_(r), column=66) for r in reasons]),
             confirmurl  = '{0}/{1}'.format(mlist.script_url('confirm'), token),
             admindb_url = mlist.script_url('admindb'),
             )
@@ -209,7 +211,8 @@
                 charset = language.charset
                 # We need to regenerate or re-translate a few values in the
                 # substitution dictionary.
-                #d['reason'] = _(reason) # XXX reason
+                substitutions["reasons"] = wrap(
+                        ", ".join([_(r) for r in reasons]), column=55)
                 substitutions['subject'] = original_subject
                 # craft the admin notification message and deliver it
                 subject = _(
@@ -240,9 +243,7 @@
                 nmsg.attach(MIMEMessage(dmsg))
                 nmsg.send(mlist, **dict(tomoderators=True))
         # Log the held message
-        # XXX reason
-        reason = 'n/a'
         log.info('HOLD: %s post from %s held, message-id=%s: %s',
                  mlist.fqdn_listname, msg.sender,
-                 msg.get('message-id', 'n/a'), reason)
+                 msg.get('message-id', 'n/a'), " ; ".join(reasons))
         notify(HoldEvent(mlist, msg, msgdata, self))
 
=== modified file 'src/mailman/chains/tests/test_hold.py'
--- src/mailman/chains/tests/test_hold.py	2014-01-01 14:59:42 +0000
+++ src/mailman/chains/tests/test_hold.py	2014-04-17 20:19:21 +0000
@@ -29,10 +29,14 @@
 from zope.component import getUtility
 
 from mailman.app.lifecycle import create_list
-from mailman.chains.hold import autorespond_to_sender
+from mailman.chains.hold import autorespond_to_sender, HoldChain
+from mailman.email.message import Message
 from mailman.interfaces.autorespond import IAutoResponseSet, Response
 from mailman.interfaces.usermanager import IUserManager
-from mailman.testing.helpers import configuration, get_queue_messages
+from mailman.testing.helpers import (
+    configuration,
+    get_queue_messages,
+    specialized_message_from_string as mfs)
 from mailman.testing.layers import ConfigLayer
 
 
@@ -89,3 +93,48 @@
 
 If you believe this message is in error, or if you have any questions,
 please contact the list owner at [e-mail].""")
+
+
+
 
+class TestHoldChain(unittest.TestCase):
+    """Test the hold chain code."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('[e-mail]')
+
+    def test_hold_chain(self):
+        msg = mfs("""\
+From: [e-mail]
+To: [e-mail]
+Subject: A message
+Message-ID: <ant>
+MIME-Version: 1.0
+
+A message body.
+""")
+        metadata = {
+            "moderation_reasons": [
+                "TEST-REASON-1",
+                "TEST-REASON-2",
+            ],
+        }
+        chain = HoldChain()
+        chain._process(self._mlist, msg, metadata)
+        messages = get_queue_messages('virgin')
+        self.assertEqual(len(messages), 2)
+        sent_msgs = {}
+        for msg_bag in messages:
+            if msg_bag.msg["To"] == "[e-mail]":
+                sent_msgs["owner"] = msg_bag.msg
+            elif msg_bag.msg["To"] == "[e-mail]":
+                sent_msgs["sender"] = msg_bag.msg
+            else:
+                self.fail("Unexpected message: %s" % msg_bag.msg)
+        self.assertTrue("TEST-REASON-1, TEST-REASON-2" in
+                        sent_msgs["owner"].as_string())
+        self.assertTrue("    TEST-REASON-1" in
+                        sent_msgs["sender"].as_string())
+        self.assertTrue("    TEST-REASON-2" in
+                        sent_msgs["sender"].as_string())
 
=== modified file 'src/mailman/rules/docs/moderation.rst'
--- src/mailman/rules/docs/moderation.rst	2013-06-19 02:43:40 +0000
+++ src/mailman/rules/docs/moderation.rst	2014-04-17 20:19:21 +0000
@@ -51,8 +51,9 @@
     >>> member_rule.check(mlist, member_msg, msgdata)
     True
     >>> dump_msgdata(msgdata)
-    moderation_action: hold
-    moderation_sender: [e-mail]
+    moderation_action : hold
+    moderation_reasons: [u'The message comes from a moderated member']
+    moderation_sender : [e-mail]
 
 
 Nonmembers
@@ -87,8 +88,9 @@
     >>> nonmember_rule.check(mlist, nonmember_msg, msgdata)
     True
     >>> dump_msgdata(msgdata)
-    moderation_action: hold
-    moderation_sender: [e-mail]
+    moderation_action : hold
+    moderation_reasons: [u'The message is not from a list member']
+    moderation_sender : [e-mail]
 
 Of course, the nonmember action can be set to defer the decision, in which
 case the rule does not match.
@@ -140,8 +142,9 @@
     >>> nonmember_rule.check(mlist, msg, msgdata)
     True
     >>> dump_msgdata(msgdata)
-    moderation_action: hold
-    moderation_sender: [e-mail]
+    moderation_action : hold
+    moderation_reasons: [u'The message is not from a list member']
+    moderation_sender : [e-mail]
 
     >>> dump_list(mlist.members.members, key=memberkey)
     <Member: Anne Person <[e-mail]>
 
=== modified file 'src/mailman/rules/moderation.py'
--- src/mailman/rules/moderation.py	2014-03-15 19:13:46 +0000
+++ src/mailman/rules/moderation.py	2014-04-17 20:19:21 +0000
@@ -59,6 +59,10 @@
                 # stored in the pending request table.
                 msgdata['moderation_action'] = action.name
                 msgdata['moderation_sender'] = sender
+                if 'moderation_reasons' not in msgdata:
+                    msgdata['moderation_reasons'] = []
+                msgdata['moderation_reasons'].append(
+                        _('The message comes from a moderated member'))
                 return True
         # The sender is not a member so this rule does not match.
         return False
@@ -104,6 +108,10 @@
                 # stored in the pending request table.
                 msgdata['moderation_action'] = action.name
                 msgdata['moderation_sender'] = sender
+                if 'moderation_reasons' not in msgdata:
+                    msgdata['moderation_reasons'] = []
+                msgdata['moderation_reasons'].append(
+                        _('The message is not from a list member'))
                 return True
         # The sender must be a member, so this rule does not match.
         return False
 
=== modified file 'src/mailman/rules/tests/test_moderation.py'
--- src/mailman/rules/tests/test_moderation.py	2014-03-15 19:13:46 +0000
+++ src/mailman/rules/tests/test_moderation.py	2014-04-17 20:19:21 +0000
@@ -28,6 +28,7 @@
 import unittest
 
 from mailman.app.lifecycle import create_list
+from mailman.interfaces.action import Action
 from mailman.interfaces.member import MemberRole
 from mailman.interfaces.usermanager import IUserManager
 from mailman.rules import moderation
@@ -76,3 +77,37 @@
         # Bill is not a member.
         bill_member = self._mlist.members.get_member('[e-mail]')
         self.assertIsNone(bill_member)
+
+    def test_moderation_reason(self):
+        user_manager = getUtility(IUserManager)
+        anne = user_manager.create_address('[e-mail]')
+        msg = mfs("""\
+From: [e-mail]
+To: [e-mail]
+Subject: A test message
+Message-ID: <ant>
+MIME-Version: 1.0
+
+A message body.
+""")
+        # Anne is in the message's senders list.
+        self.assertIn('[e-mail]', msg.senders)
+        # Now run the rule
+        rule = moderation.NonmemberModeration()
+        msgdata = {}
+        result = rule.check(self._mlist, msg, msgdata)
+        self.assertTrue(result, 'NonmemberModeration rule should hit')
+        # The reason for moderation should be in the msgdata
+        self.assertTrue("moderation_reasons" in msgdata)
+        self.assertEqual(len(msgdata["moderation_reasons"]), 1)
+        # Now make Anne a moderated member
+        anne_member = self._mlist.subscribe(anne, MemberRole.member)
+        anne_member.moderation_action = Action.hold
+        # ...and run the rule again
+        rule = moderation.MemberModeration()
+        msgdata = {}
+        result = rule.check(self._mlist, msg, msgdata)
+        self.assertTrue(result, 'MemberModeration rule should hit')
+        # The reason for moderation should be in the msgdata
+        self.assertTrue("moderation_reasons" in msgdata)
+        self.assertEqual(len(msgdata["moderation_reasons"]), 1)
 
=== modified file 'src/mailman/templates/en/postauth.txt'
--- src/mailman/templates/en/postauth.txt	2008-09-29 06:44:19 +0000
+++ src/mailman/templates/en/postauth.txt	2014-04-17 20:19:21 +0000
@@ -4,10 +4,10 @@
     List:    $listname
     From:    $sender
     Subject: $subject
-    Reason:  $reason
+    Reasons: $reasons
 
 At your convenience, visit:
 
     $admindb_url
-        
+
 to approve or deny the request.
 
=== modified file 'src/mailman/templates/en/postheld.txt'
--- src/mailman/templates/en/postheld.txt	2008-09-29 06:44:19 +0000
+++ src/mailman/templates/en/postheld.txt	2014-04-17 20:19:21 +0000
@@ -6,7 +6,7 @@
 
 The reason it is being held:
 
-    $reason
+$reasons
 
 Either the message will get posted to the list, or you will receive
 notification of the moderator's decision.  If you would like to cancel