Merge lp:~abompard/mailman/fix-import-from-mm2 into lp:mailman

[Aurélien Bompard]

This branch contains a few fixes for the import-from-mailman2 codepath.

[lp:~abompard/mailman/fix-import-from-mm2]

7300. By Barry Warsaw on 2015-03-03

Move some old bin scripts to a holding tank.

7301. By Barry Warsaw on 2015-03-03

Doc fixes given by Abhilash Raj.

7302. By Barry Warsaw on 2015-03-09

copybump should ignore .git directories. This isn't currently relevant for
Mailman, but I use this script for other repos.

7303. By Barry Warsaw on 2015-03-09

Fix a read used during the PostgreSQL tests.

7304. By Barry Warsaw on 2015-03-13

 * When trying to subscribe an address to a mailing list through the REST API
   where a case-differing version of the address is already subscribed, return
   a 409 error instead of a 500 error. Found by Ankush Sharma. (LP: #1425359)

7305. By Barry Warsaw on 2015-03-14

Documentation fixes, given by Abhilash Raj.

7306. By Barry Warsaw on 2015-03-14

 * ``mailman lists --domain`` was not properly handling its arguments. Given
   by Manish Gill. (LP: #1166911)

7307. By Aurélien Bompard on 2015-03-16

Fixes in the import process

[Aurélien Bompard (abompard)]

I merged the changes in HEAD, please check out those fixes.

[Barry Warsaw (barry)]

Accepted and merged, with some style changes:

* Single quotes for double quotes in several places.
* Full sentences in comments.
* No hanging comments.
* Refactored getUtility()
* Comments after test function name/signature.
* Long lines.
* Abbreviations.

I'm moderately concerned that the Bouncer class isn't tested explicitly; does this show up in real data or in the test sample data? Any chance you can write a follow up test for that? (It's fine that bounce info is thrown away during an import.)

I also added a context manager to ensure sys.modules gets cleaned up, and then I used an ExitStack in the with statement. These things are awesome. :)

review: Approve

[Aurélien Bompard (abompard)]

Thanks a lot for this merge. I'll try to keep your changes in mind for future MP (single quotes are preferred, describe tests with comments, etc.)
I don't quite understand your concern about the Bouncer class. It's there to allow loading the pickles when they include instances of a Bouncer._BounceInfo class (where Bouncer is the name of a module in MM2.1). I don't do anything with it, but the unpickling will fail without it (with real data from the Fedora lists).

[Barry Warsaw (barry)]

On Mar 26, 2015, at 06:55 AM, Aurélien Bompard wrote:

>I don't quite understand your concern about the Bouncer class. It's there to
>allow loading the pickles when they include instances of a
>Bouncer._BounceInfo class (where Bouncer is the name of a module in MM2.1). I
>don't do anything with it, but the unpickling will fail without it (with real
>data from the Fedora lists).

Yep, that was my question. I don't think we have any tests that require the
new Bouncer mock, so I was wondering why you added it. "To handle real data
from Fedora lists" is a good reason. :) I was just hoping we could get a
test that shows how and verifies that this works.

[Aurélien Bompard (abompard)]

Ah, gotcha. I need to find a pickle file that has these _BouncerInfo instances and write a test that uses it.

=== modified file 'src/mailman/commands/cli_import.py'
--- src/mailman/commands/cli_import.py	2015-01-05 01:40:47 +0000
+++ src/mailman/commands/cli_import.py	2015-03-16 10:22:44 +0000
@@ -35,6 +35,14 @@
 
 
 

+# Mock the Bouncer class from Mailman 2.1, we don't use it but there are
+# instances in the pickled config files
+class Bouncer:
+    class _BounceInfo:
+        pass
+
+
+

 @implementer(ICLISubCommand)
 class Import21:
     """Import Mailman 2.1 list data."""
@@ -74,10 +82,11 @@
         assert len(args.pickle_file) == 1, (
             'Unexpected positional arguments: %s' % args.pickle_file)
         filename = args.pickle_file[0]
+        sys.modules["Mailman.Bouncer"] = Bouncer
         with open(filename, 'rb') as fp:
             while True:
                 try:
-                    config_dict = pickle.load(fp)
+                    config_dict = pickle.load(fp, encoding="utf-8", errors="ignore")
                 except EOFError:
                     break
                 except pickle.UnpicklingError:
@@ -94,3 +103,4 @@
                     except Import21Error as error:
                         print(error, file=sys.stderr)
                         sys.exit(1)
+        del sys.modules["Mailman.Bouncer"]

=== modified file 'src/mailman/utilities/importer.py'
--- src/mailman/utilities/importer.py	2015-01-05 01:40:47 +0000
+++ src/mailman/utilities/importer.py	2015-03-16 10:22:44 +0000
@@ -32,6 +32,7 @@
 from mailman.core.errors import MailmanError
 from mailman.handlers.decorate import decorate, decorate_template
 from mailman.interfaces.action import Action, FilterAction
+from mailman.interfaces.address import IEmailValidator
 from mailman.interfaces.archiver import ArchivePolicy
 from mailman.interfaces.autorespond import ResponseAction
 from mailman.interfaces.bans import IBanManager
@@ -387,11 +388,17 @@
     regulars_set = set(config_dict.get('members', {}))
     digesters_set = set(config_dict.get('digest_members', {}))
     members = regulars_set.union(digesters_set)
-    import_roster(mlist, config_dict, members, MemberRole.member)
-    import_roster(mlist, config_dict, config_dict.get('owner', []),
-                  MemberRole.owner)
-    import_roster(mlist, config_dict, config_dict.get('moderator', []),
-                  MemberRole.moderator)
+    # don't send welcome messages...
+    send_welcome_message = mlist.send_welcome_message
+    mlist.send_welcome_message = False
+    try:
+        import_roster(mlist, config_dict, members, MemberRole.member)
+        import_roster(mlist, config_dict, config_dict.get('owner', []),
+                      MemberRole.owner)
+        import_roster(mlist, config_dict, config_dict.get('moderator', []),
+                      MemberRole.moderator)
+    finally:
+        mlist.send_welcome_message = send_welcome_message
 
 
 

@@ -427,8 +434,12 @@
                 merged_members.update(config_dict.get('digest_members', {}))
                 if merged_members.get(email, 0) != 0:
                     original_email = bytes_to_str(merged_members[email])
+                    if not getUtility(IEmailValidator).is_valid(original_email):
+                        original_email = email
                 else:
                     original_email = email
+                if not getUtility(IEmailValidator).is_valid(original_email):
+                    continue # skip this one entirely
                 address = usermanager.create_address(original_email)
                 address.verified_on = datetime.datetime.now()
             user.link(address)

=== modified file 'src/mailman/utilities/tests/test_import.py'
--- src/mailman/utilities/tests/test_import.py	2015-01-05 01:40:47 +0000
+++ src/mailman/utilities/tests/test_import.py	2015-03-16 10:22:44 +0000
@@ -38,6 +38,7 @@
 from mailman.config import config
 from mailman.handlers.decorate import decorate
 from mailman.interfaces.action import Action, FilterAction
+from mailman.interfaces.address import InvalidEmailAddressError
 from mailman.interfaces.archiver import ArchivePolicy
 from mailman.interfaces.autorespond import ResponseAction
 from mailman.interfaces.bans import IBanManager
@@ -747,6 +748,40 @@
         anne = self._usermanager.get_user('[e-mail]')
         self.assertTrue(anne.controls('[e-mail]'))
 
+    def test_invalid_original_email(self):
+        self._pckdict["members"]["[e-mail]"] = b'invalid email address'
+        try:
+            import_config_pck(self._mlist, self._pckdict)
+        except InvalidEmailAddressError as e:
+            self.fail(e)
+        self.assertIn('[e-mail]',
+                      [a.email for a in self._mlist.members.addresses])
+        anne = self._usermanager.get_address('[e-mail]')
+        self.assertEqual(anne.original_email, '[e-mail]')
+
+    def test_invalid_email(self):
+        self._pckdict["members"] = {
+            '[e-mail]': 0,
+            'invalid email address': b'invalid email address'
+        }
+        self._pckdict["digest_members"] = {}
+        try:
+            import_config_pck(self._mlist, self._pckdict)
+        except InvalidEmailAddressError as e:
+            self.fail(e)
+        self.assertEqual(['[e-mail]'],
+                         [a.email for a in self._mlist.members.addresses])
+
+    def test_no_email_sent(self):
+        self._pckdict
+        import_config_pck(self._mlist, self._pckdict)
+        self.assertIn("[e-mail]",
+                      [a.email for a in self._mlist.members.addresses])
+        # no email in any queue
+        for qname, sb in config.switchboards.items():
+            self.assertEqual(len(sb.files), 0,
+                "Queue '{}' has {} emails".format(qname, len(sb.files)))
+        self.assertTrue(self._mlist.send_welcome_message)