launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/no-more-terrible-generate-nick into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/no-more-terrible-generate-nick/+merge/143064

Seed the nickname generation from the full e-mail address, rather than just the localpart. This should drastically reduce the number of collisions we have to step through. Place test_nickname on a fairly brutal diet.
-- 
https://code.launchpad.net/~stevenk/launchpad/no-more-terrible-generate-nick/+merge/143064
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/no-more-terrible-generate-nick into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2013-01-10 06:04:00 +0000
+++ lib/lp/registry/model/person.py	2013-01-14 06:19:21 +0000
@@ -4908,8 +4908,8 @@
     email_addr = email_addr.strip().lower()
 
     if not valid_email(email_addr):
-        raise NicknameGenerationError("%s is not a valid email address"
-                                      % email_addr)
+        raise NicknameGenerationError(
+            "%s is not a valid email address" % email_addr)
 
     user = re.match("^(\S+)@(?:\S+)$", email_addr).groups()[0]
     user = user.replace(".", "-").replace("_", "-")
@@ -4933,7 +4933,7 @@
     # We seed the random number generator so we get consistent results,
     # making the algorithm repeatable and thus testable.
     random_state = random.getstate()
-    random.seed(sum(ord(letter) for letter in generated_nick))
+    random.seed(sum(ord(letter) for letter in email_addr))
     try:
         attempts = 0
         prefix = ''

=== modified file 'lib/lp/registry/tests/test_nickname.py'
--- lib/lp/registry/tests/test_nickname.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/tests/test_nickname.py	2013-01-14 06:19:21 +0000
@@ -1,4 +1,4 @@
-# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2011-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for nickname generation"""
@@ -21,11 +21,9 @@
     layer = DatabaseFunctionalLayer
 
     def test_rejects_invalid_emails(self):
-        # generate_nick rejects invalid email addresses
+        # generate_nick rejects invalid email addresses.
         self.assertRaises(
-            NicknameGenerationError,
-            generate_nick,
-            'foo@example@com')
+            NicknameGenerationError, generate_nick, 'foo@example@com')
 
     def test_uses_email_address(self):
         # generate_nick uses the first part of the email address to create
@@ -36,39 +34,26 @@
     def test_handles_symbols(self):
         # If an email starts with symbols, generate_nick still creates a
         # valid nick that doesn't start with symbols.
-        nicks = [generate_nick(email) for email in [
-                                            '---bar@xxxxxxxxxxx',
-                                            'foo.bar@xxxxxxxxxxx',
-                                            'foo-bar@xxxxxxxxxxx',
-                                            'foo+bar@xxxxxxxxxxx',
-                                            ]]
-        self.assertEqual(
-            ['bar', 'foo-bar', 'foo-bar', 'foo+bar'],
-            nicks)
+        parts = ['---bar', 'foo.bar', 'foo-bar', 'foo+bar']
+        nicks = [generate_nick("%s@xxxxxxxxxxxx" % part) for part in parts]
+        self.assertEqual(['bar', 'foo-bar', 'foo-bar', 'foo+bar'], nicks)
 
     def test_enforces_minimum_length(self):
         # Nicks must be a minimum length. generate_nick enforces this by 
         # adding random suffixes to the required length.
-        # The nick 'i' isn't used, so we know any additional prefi
-        person = getUtility(IPersonSet).getByName('i')
-        self.assertIs(None, person)
+        self.assertIs(None, getUtility(IPersonSet).getByName('i'))
         nick = generate_nick('i@xxxxxxxxxxx')
-        self.assertEqual('i-5', nick)
+        self.assertEqual('i-b', nick)
 
     def test_can_create_noncolliding_nicknames(self):
         # Given the same email address, generate_nick doesn't recreate the
         # same nick once that nick is used.
-        self._useNicknames(['bar'])
+        self.factory.makePerson(name='bar')
         nick = generate_nick('bar@xxxxxxxxxxx')
-        self.assertEqual('bar-c', nick)
+        self.assertEqual('bar-3', nick)
 
         # If we used the previously created nick and get another bar@ email
         # address, another new nick is generated.
-        self._useNicknames(['bar-c'])
+        self.factory.makePerson(name=nick)
         nick = generate_nick('bar@xxxxxxxxxxx')
-        self.assertEqual('a-bar', nick)
-
-    def _useNicknames(self, nicknames):
-        # Helper method to consume a nickname
-        for nick in nicknames:
-            self.factory.makePerson(name=nick)
+        self.assertEqual('5-bar', nick)