Merge lp:~jtv/launchpad/bug-411514 into lp:launchpad

[Jeroen T. Vermeulen]

Fix "email address already in use" poimport oopses.

[Jeroen T. Vermeulen (jtv)]

= Bug 411514 =

Now that we're getting proper oops reports from the translations import script, it's become painfully obvious that imports are failing every day because of a bug in handling the person/account split.

Danilo did some digging and came up with this explanation: sometimes there's an Account with the email address we're looking for, but no Person is associated with it. In that case, creating a Person (and implicitly an Account) will fail because the Account already exists.

The Soyuz team has already solved this by calling IPersonSet.ensurePerson, which handles the various cases of existence, nonexistence, and partial existence.

A test exercises this scenario; I used it to reproduce the bug first. The actual code is a bit simpler now, as it now always prepares to create a person without knowing or caring whether there already is one. Another test verifies that behavior for invalid email addresses is unchanged.

For Q/A, ensure that imports still happen & watch the oopses go away. One way to trigger the former oops deliberately is to upload a translation file listing martijn at foodfight (org, not com) as the author in the header. Hoi Martijn!

Test:
{{{
./bin/test -vv -t test_file_importer
}}}

No lint.

Jeroen

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/translations/utilities/tests/test_file_importer.py'
--- lib/lp/translations/utilities/tests/test_file_importer.py	2009-07-17 00:26:05 +0000
+++ lib/lp/translations/utilities/tests/test_file_importer.py	2010-02-12 11:33:20 +0000
@@ -307,6 +307,30 @@
                 po_importer.potemplate.displayname),
             'Did not create the correct comment for %s' % test_email)
 
+    def test_getPersonByEmail_personless_account(self):
+        # An Account without a Person attached is a difficult case for
+        # _getPersonByEmail: it has to create the Person but re-use an
+        # existing Account and email address.
+        (pot_importer, po_importer) = self._createImporterForExportedEntries()
+        test_email = '[e-mail]'
+        account = self.factory.makeAccount('Send me Ubuntu', test_email)
+
+        person = po_importer._getPersonByEmail(test_email)
+
+        self.assertEqual(account, person.account)
+
+        # The same person will come up for the same address next time.
+        self.assertEqual(person, po_importer._getPersonByEmail(test_email))
+
+    def test_getPersonByEmail_bad_address(self):
+        # _getPersonByEmail returns None for malformed addresses.
+        (pot_importer, po_importer) = self._createImporterForExportedEntries()
+        test_email = 'john over at swansea'
+
+        person = po_importer._getPersonByEmail(test_email)
+
+        self.assertEqual(None, person)
+
     def test_FileImporter_importFile_ok(self):
         # Test correct import operation for both
         # exported and published files.

=== modified file 'lib/lp/translations/utilities/translation_import.py'
--- lib/lp/translations/utilities/translation_import.py	2009-09-14 09:12:58 +0000
+++ lib/lp/translations/utilities/translation_import.py	2010-02-12 11:33:20 +0000
@@ -758,21 +758,18 @@
             return None
 
         personset = getUtility(IPersonSet)
-        person = personset.getByEmail(email)
-
-        if person is None:
-            # We create a new person, without a password.
-            comment = 'when importing the %s translation of %s' % (
-                self.pofile.language.displayname, self.potemplate.displayname)
-
-            try:
-                person, dummy = personset.createPersonAndEmail(
-                    email, PersonCreationRationale.POFILEIMPORT,
-                    displayname=name, comment=comment)
-            except InvalidEmailAddress:
-                return None
-
-        return person
+
+        # We may have to create a new person.  If we do, this is the
+        # rationale.
+        comment = 'when importing the %s translation of %s' % (
+            self.pofile.language.displayname, self.potemplate.displayname)
+        rationale = PersonCreationRationale.POFILEIMPORT
+
+        try:
+            return personset.ensurePerson(
+                email, displayname=name, rationale=rationale, comment=comment)
+        except InvalidEmailAddress:
+            return None
 
     def importMessage(self, message):
         """See FileImporter."""