Merge branch 'reasons' into 'master'

Implement reasons for why a message is being held for moderator approval.

Given by Aurélien Bompard, tweaked by Barry Warsaw.

Remove the inaccurate confirmation url and admindb urls from the substitution
dictionaries for postauth.txt and posthold.txt, and the templates.

See merge request !10

8 files changed, 133 insertions, 81 deletions

diff --git a/src/mailman/chains/hold.py b/src/mailman/chains/hold.py
index 3773aa3e5..0509655a2 100644
--- a/src/mailman/chains/hold.py
+++ b/src/mailman/chains/hold.py
@@ -44,9 +44,12 @@ from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implementer
 
+SEMISPACE = '; '
+SPACE = ' '
+NL = '\n'
+
 
 log = logging.getLogger('mailman.vette')
-SEMISPACE = '; '
 
 
 
@@ -56,6 +59,15 @@ class HeldMessagePendable(dict):
 
 
 
+def _compose_reasons(msgdata, column=66):
+    # Rules can add reasons to the metadata.
+    reasons = msgdata.get('moderation_reasons', [_('N/A')])
+    return NL.join(
+        [(SPACE * 4) + wrap(_(reason), column=column)
+         for reason in reasons])
+
+
+
 def autorespond_to_sender(mlist, sender, language=None):
     """Should Mailman automatically respond to this sender?
 
@@ -134,7 +146,6 @@ class HoldChain(TerminalChainBase):
         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,9 +169,7 @@ class HoldChain(TerminalChainBase):
             listname    = mlist.fqdn_listname,
             subject     = original_subject,
             sender      = msg.sender,
-            reason      = 'N/A', #reason,
-            confirmurl  = '{0}/{1}'.format(mlist.script_url('confirm'), token),
-            admindb_url = mlist.script_url('admindb'),
+            reasons     = _compose_reasons(msgdata),
             )
         # At this point the message is held, but now we have to craft at least
         # two responses.  The first will go to the original author of the
@@ -203,10 +212,10 @@ class HoldChain(TerminalChainBase):
             with _.using(mlist.preferred_language.code):
                 language = mlist.preferred_language
                 charset = language.charset
+                substitutions['subject'] = original_subject
                 # We need to regenerate or re-translate a few values in the
                 # substitution dictionary.
-                #d['reason'] = _(reason) # XXX reason
-                substitutions['subject'] = original_subject
+                substitutions['reasons'] = _compose_reasons(msgdata, 55)
                 # craft the admin notification message and deliver it
                 subject = _(
                     '$mlist.fqdn_listname post from $msg.sender requires '
@@ -235,10 +244,11 @@ also appear in the first line of the body of the reply.""")),
                 nmsg.attach(MIMEMessage(msg))
                 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)
+        # Log the held message.  Log messages are not translated, so recast
+        # the reasons in the English.
+        with _.using('en'):
+            reasons = _compose_reasons(msgdata)
+            log.info('HOLD: %s post from %s held, message-id=%s: %s',
+                     mlist.fqdn_listname, msg.sender,
+                     msg.get('message-id', 'n/a'), SEMISPACE.join(reasons))
         notify(HoldEvent(mlist, msg, msgdata, self))
diff --git a/src/mailman/chains/tests/test_hold.py b/src/mailman/chains/tests/test_hold.py
index b896157c4..1643b6ce3 100644
--- a/src/mailman/chains/tests/test_hold.py
+++ b/src/mailman/chains/tests/test_hold.py
@@ -19,6 +19,7 @@
 
 __all__ = [
     'TestAutorespond',
+    'TestHoldChain',
     ]
 
 
@@ -26,9 +27,12 @@ import unittest
 
 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.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
 from zope.component import getUtility
 
@@ -86,3 +90,44 @@ further responses today.  Please try again tomorrow.
 
 If you believe this message is in error, or if you have any questions,
 please contact the list owner at test-owner@example.com.""")
+
+
+
+class TestHoldChain(unittest.TestCase):
+    """Test the hold chain code."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('test@example.com')
+
+    def test_hold_chain(self):
+        msg = mfs("""\
+From: anne@example.com
+To: test@example.com
+Subject: A message
+Message-ID: <ant>
+MIME-Version: 1.0
+
+A message body.
+""")
+        msgdata = dict(moderation_reasons=[
+            'TEST-REASON-1',
+            'TEST-REASON-2',
+            ])
+        process_chain(self._mlist, msg, msgdata, start_chain='hold')
+        messages = get_queue_messages('virgin')
+        self.assertEqual(len(messages), 2)
+        payloads = {}
+        for item in messages:
+            if item.msg['to'] == 'test-owner@example.com':
+                part = item.msg.get_payload(0)
+                payloads['owner'] = part.get_payload().splitlines()
+            elif item.msg['To'] == 'anne@example.com':
+                payloads['sender'] = item.msg.get_payload().splitlines()
+            else:
+                self.fail('Unexpected message: %s' % item.msg)
+        self.assertIn('    TEST-REASON-1', payloads['owner'])
+        self.assertIn('    TEST-REASON-2', payloads['owner'])
+        self.assertIn('    TEST-REASON-1', payloads['sender'])
+        self.assertIn('    TEST-REASON-2', payloads['sender'])
diff --git a/src/mailman/core/docs/chains.rst b/src/mailman/core/docs/chains.rst
index 328d0b624..34f3e5c4e 100644
--- a/src/mailman/core/docs/chains.rst
+++ b/src/mailman/core/docs/chains.rst
@@ -132,8 +132,10 @@ This one is addressed to the list moderators.
         List:    test@example.com
         From:    aperson@example.com
         Subject: My first post
-        Reason:  N/A
     <BLANKLINE>
+    The message is being held because:
+    <BLANKLINE>
+        N/A
     At your convenience, visit your dashboard to approve or deny the
     request.
     <BLANKLINE>
@@ -184,63 +186,12 @@ This message is addressed to the sender of the message.
     <BLANKLINE>
     Is being held until the list moderator can review it for approval.
     <BLANKLINE>
-    The reason it is being held:
+    The message is being held because:
     <BLANKLINE>
         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
-    this posting, please visit the following URL:
-    <BLANKLINE>
-        http://lists.example.com/confirm/test@example.com/...
-    <BLANKLINE>
-    <BLANKLINE>
-
-In addition, the pending database is holding the original messages, waiting
-for them to be disposed of by the original author or the list moderators.  The
-database is essentially a dictionary, with the keys being the randomly
-selected tokens included in the urls and the values being a 2-tuple where the
-first item is a type code and the second item is a message id.
-::
-
-    >>> import re
-    >>> cookie = None
-    >>> for line in messages[1].get_payload().splitlines():
-    ...     mo = re.search('confirm/[^/]+/(?P<cookie>.*)$', line)
-    ...     if mo:
-    ...         cookie = mo.group('cookie')
-    ...         break
-    >>> assert cookie is not None, 'No confirmation token found'
-
-    >>> from mailman.interfaces.pending import IPendings
-    >>> from zope.component import getUtility
-
-    >>> data = getUtility(IPendings).confirm(cookie)
-    >>> dump_msgdata(data)
-    id  : 1
-    type: held message
-
-The message itself is held in the message store.
-::
-
-    >>> from mailman.interfaces.requests import IListRequests
-    >>> list_requests = IListRequests(mlist)
-    >>> rkey, rdata = list_requests.get_request(data['id'])
-
-    >>> from mailman.interfaces.messages import IMessageStore
-    >>> from zope.component import getUtility
-    >>> msg = getUtility(IMessageStore).get_message_by_id(
-    ...     rdata['_mod_message_id'])
-
-    >>> print(msg.as_string())
-    From: aperson@example.com
-    To: test@example.com
-    Subject: My first post
-    Message-ID: <first>
-    X-Message-ID-Hash: RXJU4JL6N2OUN3OYMXXPPSCR7P7JE2BW
-    <BLANKLINE>
-    An important message.
-    <BLANKLINE>
+    notification of the moderator's decision.
 
 
 The Accept chain
diff --git a/src/mailman/rules/docs/moderation.rst b/src/mailman/rules/docs/moderation.rst
index 401004f34..f5ceec29a 100644
--- a/src/mailman/rules/docs/moderation.rst
+++ b/src/mailman/rules/docs/moderation.rst
@@ -54,8 +54,9 @@ information for the eventual moderation chain.
     >>> member_rule.check(mlist, member_msg, msgdata)
     True
     >>> dump_msgdata(msgdata)
-    moderation_action: hold
-    moderation_sender: aperson@example.com
+    moderation_action : hold
+    moderation_reasons: ['The message comes from a moderated member']
+    moderation_sender : aperson@example.com
 
 
 Nonmembers
@@ -94,8 +95,9 @@ carries some useful information.
     >>> nonmember_rule.check(mlist, nonmember_msg, msgdata)
     True
     >>> dump_msgdata(msgdata)
-    moderation_action: hold
-    moderation_sender: bperson@example.com
+    moderation_action : hold
+    moderation_reasons: ['The message is not from a list member']
+    moderation_sender : bperson@example.com
 
 Of course, the nonmember action can be set to defer the decision, in which
 case the rule does not match.
@@ -147,8 +149,9 @@ nonmember of the list.  The rule also matches.
     >>> nonmember_rule.check(mlist, msg, msgdata)
     True
     >>> dump_msgdata(msgdata)
-    moderation_action: hold
-    moderation_sender: cperson@example.com
+    moderation_action : hold
+    moderation_reasons: ['The message is not from a list member']
+    moderation_sender : cperson@example.com
 
     >>> dump_list(mlist.members.members, key=memberkey)
     <Member: Anne Person <aperson@example.com>
diff --git a/src/mailman/rules/moderation.py b/src/mailman/rules/moderation.py
index d591557c4..0cc0b81c3 100644
--- a/src/mailman/rules/moderation.py
+++ b/src/mailman/rules/moderation.py
@@ -55,6 +55,9 @@ class MemberModeration:
                 # stored in the pending request table.
                 msgdata['moderation_action'] = action.name
                 msgdata['moderation_sender'] = sender
+                msgdata.setdefault('moderation_reasons', []).append(
+                    # This will get translated at the point of use.
+                    'The message comes from a moderated member')
                 return True
         # The sender is not a member so this rule does not match.
         return False
@@ -100,6 +103,9 @@ class NonmemberModeration:
                 # stored in the pending request table.
                 msgdata['moderation_action'] = action.name
                 msgdata['moderation_sender'] = sender
+                msgdata.setdefault('moderation_reasons', []).append(
+                    # This will get translated at the point of use.
+                    'The message is not from a list member')
                 return True
         # The sender must be a member, so this rule does not match.
         return False
diff --git a/src/mailman/rules/tests/test_moderation.py b/src/mailman/rules/tests/test_moderation.py
index 1acf23258..a2e988874 100644
--- a/src/mailman/rules/tests/test_moderation.py
+++ b/src/mailman/rules/tests/test_moderation.py
@@ -25,6 +25,7 @@ __all__ = [
 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
@@ -73,3 +74,39 @@ A message body.
         # Bill is not a member.
         bill_member = self._mlist.members.get_member('bill@example.com')
         self.assertIsNone(bill_member)
+
+    def test_moderation_reason(self):
+        # When a message is moderated, a reason is added to the metadata.
+        user_manager = getUtility(IUserManager)
+        anne = user_manager.create_address('anne@example.com')
+        msg = mfs("""\
+From: anne@example.com
+To: test@example.com
+Subject: A test message
+Message-ID: <ant>
+MIME-Version: 1.0
+
+A message body.
+""")
+        # Anne is in the message's senders list.
+        self.assertIn('anne@example.com', 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.
+        reasons = msgdata['moderation_reasons']
+        self.assertEqual(reasons, ['The message is not from a list member'])
+        # 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.
+        reasons = msgdata['moderation_reasons']
+        self.assertEqual(
+            reasons, ['The message comes from a moderated member'])
diff --git a/src/mailman/templates/en/postauth.txt b/src/mailman/templates/en/postauth.txt
index 472ed32b4..9e9ae8f57 100644
--- a/src/mailman/templates/en/postauth.txt
+++ b/src/mailman/templates/en/postauth.txt
@@ -4,7 +4,10 @@ following mailing list posting:
     List:    $listname
     From:    $sender
     Subject: $subject
-    Reason:  $reason
+
+The message is being held because:
+
+$reasons
 
 At your convenience, visit your dashboard to approve or deny the
 request.
diff --git a/src/mailman/templates/en/postheld.txt b/src/mailman/templates/en/postheld.txt
index b2c938acf..ee769c8ae 100644
--- a/src/mailman/templates/en/postheld.txt
+++ b/src/mailman/templates/en/postheld.txt
@@ -4,12 +4,9 @@ Your mail to '$listname' with the subject
 
 Is being held until the list moderator can review it for approval.
 
-The reason it is being held:
+The message is being held because:
 
-    $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
-this posting, please visit the following URL:
-
-    $confirmurl
+notification of the moderator's decision.