4 files changed, 76 insertions, 61 deletions

diff --git a/src/mailman/rules/moderation.py b/src/mailman/rules/moderation.py
index d2ca6ef6d..215a4c852 100644
--- a/src/mailman/rules/moderation.py
+++ b/src/mailman/rules/moderation.py
@@ -66,6 +66,12 @@ class MemberModeration:
 
 
 
+def _record_action(msgdata, action, sender, reason):
+    msgdata['moderation_action'] = action
+    msgdata['moderation_sender'] = sender
+    msgdata.setdefault('moderation_reasons', []).append(reason)
+
+
 @implementer(IRule)
 class NonmemberModeration:
     """The nonmember moderation rule."""
@@ -97,17 +103,19 @@ class NonmemberModeration:
             nonmember = mlist.nonmembers.get_member(sender)
             assert nonmember is not None, (
                 'Sender not added to the nonmembers: {0}'.format(sender))
-            # Check the '*_these_nonmembers' properties first
+            # Check the '*_these_nonmembers' properties first.  XXX These are
+            # legacy attributes from MM2.1; their database type is 'pickle' and
+            # they should eventually get replaced.
             for action in ('accept', 'hold', 'reject', 'discard'):
-                checklist = getattr(mlist, '{}_these_nonmembers'.format(action))
+                legacy_attribute_name = '{}_these_nonmembers'.format(action)
+                checklist = getattr(mlist, legacy_attribute_name)
                 for addr in checklist:
-                    if (addr.startswith('^') and re.match(addr, sender)) \
-                        or addr == sender:
-                        msgdata['moderation_action'] = action
-                        msgdata['moderation_sender'] = sender
-                        msgdata.setdefault('moderation_reasons', []).append(
-                            'The sender is in the nonmember {} list'.format(
-                            action))
+                    if ((addr.startswith('^') and re.match(addr, sender))
+                            or addr == sender):
+                        # The reason will get translated at the point of use.
+                        reason = 'The sender is in the nonmember {} list'
+                        _record_action(msgdata, action, sender,
+                                       reason.format(action))
                         return True
             action = nonmember.moderation_action
             if action is Action.defer:
@@ -116,11 +124,10 @@ class NonmemberModeration:
             elif action is not None:
                 # We must stringify the moderation action so that it can be
                 # 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')
+                #
+                # The reason will get translated at the point of use.
+                reason = 'The message is not from a list member'
+                _record_action(msgdata, action.name, sender, reason)
                 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 737b1f81d..79aade587 100644
--- a/src/mailman/rules/tests/test_moderation.py
+++ b/src/mailman/rules/tests/test_moderation.py
@@ -112,24 +112,26 @@ A message body.
             reasons, ['The message comes from a moderated member'])
 
     def test_these_nonmembers(self):
+        # Test the legacy *_these_nonmembers attributes.
         user_manager = getUtility(IUserManager)
         actions = {
-            'anne@example.com': "accept",
-            'bill@example.com': "hold",
-            'chris@example.com': "reject",
-            'dana@example.com': "discard",
-            '^anne-.*@example.com': "accept",
-            '^bill-.*@example.com': "hold",
-            '^chris-.*@example.com': "reject",
-            '^dana-.*@example.com': "discard",
-        }
+            'anne@example.com': 'accept',
+            'bill@example.com': 'hold',
+            'chris@example.com': 'reject',
+            'dana@example.com': 'discard',
+            '^anne-.*@example.com': 'accept',
+            '^bill-.*@example.com': 'hold',
+            '^chris-.*@example.com': 'reject',
+            '^dana-.*@example.com': 'discard',
+            }
         rule = moderation.NonmemberModeration()
         user_manager = getUtility(IUserManager)
         for address, action_name in actions.items():
-            setattr(self._mlist, '{}_these_nonmembers'.format(action_name),
+            setattr(self._mlist,
+                    '{}_these_nonmembers'.format(action_name),
                     [address])
             if address.startswith('^'):
-                # It's a pattern, craft a proper address
+                # It's a pattern, craft a proper address.
                 address = address[1:].replace('.*', 'something')
             user_manager.create_address(address)
             msg = mfs("""\
@@ -146,4 +148,4 @@ A message body.
             self.assertTrue(result, 'NonmemberModeration rule should hit')
             self.assertIn('moderation_action', msgdata)
             self.assertEqual(msgdata['moderation_action'], action_name,
-                "Wrong action for {}: {}".format(address, action_name))
+                'Wrong action for {}: {}'.format(address, action_name))
diff --git a/src/mailman/utilities/importer.py b/src/mailman/utilities/importer.py
index 583c10e27..293e9c39c 100644
--- a/src/mailman/utilities/importer.py
+++ b/src/mailman/utilities/importer.py
@@ -261,17 +261,19 @@ def import_config_pck(mlist, config_dict):
             continue
         setattr(mlist, key, value)
     # Handle the moderation policy.
+    #
     # The mlist.default_member_action and mlist.default_nonmember_action enum
-    # values are different in Mailman 2.1, because they have been merged into
-    # a single enum in Mailman 3.
+    # values are different in Mailman 2.1, because they have been merged into a
+    # single enum in Mailman 3.
+    #
     # Unmoderated lists used to have default_member_moderation set to a false
-    # value, this translates to the Defer default action. Moderated lists with
+    # value; this translates to the Defer default action.  Moderated lists with
     # the default_member_moderation set to a true value used to store the
     # action in the member_moderation_action flag, the values were: 0==Hold,
     # 1=Reject, 2==Discard
-    if bool(config_dict.get("default_member_moderation", 0)):
+    if bool(config_dict.get('default_member_moderation', 0)):
         mlist.default_member_action = member_moderation_action_mapping(
-            config_dict.get("member_moderation_action"))
+            config_dict.get('member_moderation_action'))
     else:
         mlist.default_member_action = Action.defer
     # Handle the archiving policy.  In MM2.1 there were two boolean options
@@ -406,14 +408,15 @@ def import_config_pck(mlist, config_dict):
         import_roster(mlist, config_dict, config_dict.get('moderator', []),
                       MemberRole.moderator)
         # Now import the '*_these_nonmembers' properties, filtering out the
-        # regexps which will remain in the property
+        # regexps which will remain in the property.
         for action_name in ('accept', 'hold', 'reject', 'discard'):
-            prop_name = '{0}_these_nonmembers'.format(action_name)
-            emails = [ addr for addr in config_dict.get(prop_name, [])
-                       if not addr.startswith('^') ]
+            prop_name = '{}_these_nonmembers'.format(action_name)
+            emails = [addr
+                      for addr in config_dict.get(prop_name, [])
+                      if not addr.startswith('^')]
             import_roster(mlist, config_dict, emails, MemberRole.nonmember,
                           Action[action_name])
-            # Only keep the regexes in the legacy list property
+            # Only keep the regexes in the legacy list property.
             list_prop = getattr(mlist, prop_name)
             for email in emails:
                 list_prop.remove(email)
@@ -433,6 +436,8 @@ def import_roster(mlist, config_dict, members, role, action=None):
     :type members: list
     :param role: The MemberRole to import them as.
     :type role: MemberRole enum
+    :param action: The default nonmember action.
+    :type action: Action
     """
     usermanager = getUtility(IUserManager)
     validator = getUtility(IEmailValidator)
@@ -471,7 +476,7 @@ def import_roster(mlist, config_dict, members, role, action=None):
         if email in config_dict.get('members', {}):
             member.preferences.delivery_mode = DeliveryMode.regular
         elif email in config_dict.get('digest_members', {}):
-            if prefs is not None and prefs & 8: # DisableMime
+            if prefs is not None and prefs & 8:               # DisableMime
                 member.preferences.delivery_mode = \
                   DeliveryMode.plaintext_digests
             else:
@@ -506,19 +511,18 @@ def import_roster(mlist, config_dict, members, role, action=None):
             member.preferences.delivery_status = DeliveryStatus.by_bounces
         # Moderation.
         if prefs is not None:
-            # we're adding a member
+            # We're adding a member.
             if prefs & 128:
-                # Member is moderated, check the member_moderation_action option to
-                # know which action should be taken.
+                # The member is moderated.  Check the member_moderation_action
+                # option to know which action should be taken.
                 action = member_moderation_action_mapping(
                     config_dict.get("member_moderation_action"))
             else:
                 action = Action.accept
         if action is not None:
-            # either set right above or in the function's arguments for
-            # nonmembers
+            # Either this was set right above or in the function's arguments
+            # for nonmembers.
             member.moderation_action = action
-        #
         # Other preferences.
         if prefs is not None:
             # AcknowledgePosts
diff --git a/src/mailman/utilities/tests/test_import.py b/src/mailman/utilities/tests/test_import.py
index d0e954fda..a0b1767c1 100644
--- a/src/mailman/utilities/tests/test_import.py
+++ b/src/mailman/utilities/tests/test_import.py
@@ -445,8 +445,8 @@ class TestMemberActionImport(unittest.TestCase):
             self.assertEqual(getattr(self._mlist, key), value)
 
     def test_member_defer(self):
-        # if default_member_moderation is not set, the member_moderation_action
-        # value is meaningless
+        # If default_member_moderation is not set, the member_moderation_action
+        # value is meaningless.
         self._pckdict['default_member_moderation'] = 0
         for mmaval in range(3):
             self._pckdict['member_moderation_action'] = mmaval
@@ -849,11 +849,12 @@ class TestRosterImport(unittest.TestCase):
 
     def test_nonmembers(self):
         import_config_pck(self._mlist, self._pckdict)
-        expected = {"gene": Action.accept,
-                    "homer": Action.hold,
-                    "iris": Action.reject,
-                    "kenny": Action.discard,
-                    }
+        expected = {
+            'gene': Action.accept,
+            'homer': Action.hold,
+            'iris': Action.reject,
+            'kenny': Action.discard,
+            }
         for name, action in expected.items():
             self.assertIn('{}@example.com'.format(name),
                           [a.email for a in self._mlist.nonmembers.addresses],
@@ -861,8 +862,9 @@ class TestRosterImport(unittest.TestCase):
             member = self._mlist.nonmembers.get_member(
                 '{}@example.com'.format(name))
             self.assertEqual(member.moderation_action, action)
-            # Only regexps should remain in the list property
-            list_prop = getattr(self._mlist,
+            # Only regexps should remain in the list property.
+            list_prop = getattr(
+                self._mlist,
                 '{}_these_nonmembers'.format(action.name))
             self.assertEqual(len(list_prop), 1)
             self.assertTrue(all(addr.startswith('^') for addr in list_prop))
@@ -951,25 +953,25 @@ class TestPreferencesImport(unittest.TestCase):
 
     def test_moderate_hold(self):
         # Option flag Moderate is translated to the action set in
-        # member_moderation_action
-        self._pckdict["member_moderation_action"] = 0
+        # member_moderation_action.
+        self._pckdict['member_moderation_action'] = 0
         self._do_test(128, dict(moderation_action=Action.hold))
 
-    def test_moderate_hold(self):
+    def test_moderate_reject(self):
         # Option flag Moderate is translated to the action set in
-        # member_moderation_action
-        self._pckdict["member_moderation_action"] = 1
+        # member_moderation_action.
+        self._pckdict['member_moderation_action'] = 1
         self._do_test(128, dict(moderation_action=Action.reject))
 
-    def test_moderate_hold(self):
+    def test_moderate_hold_discard(self):
         # Option flag Moderate is translated to the action set in
-        # member_moderation_action
-        self._pckdict["member_moderation_action"] = 2
+        # member_moderation_action.
+        self._pckdict['member_moderation_action'] = 2
         self._do_test(128, dict(moderation_action=Action.discard))
 
     def test_no_moderate(self):
         # If option flag Moderate is not set, action is accept
-        self._pckdict["member_moderation_action"] = 1 # reject
+        self._pckdict['member_moderation_action'] = 1 # reject
         self._do_test(0, dict(moderation_action=Action.accept))
 
     def test_multiple_options(self):