launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-744703 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #744703 in Launchpad itself: "PersonSet.ensurePerson doesn't associate existing EmailAddress with new Person"
  https://bugs.launchpad.net/launchpad/+bug/744703

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-744703/+merge/55268

PersonSet.ensurePerson knows how to deal with all sorts of email addresses: 1) those that we've never seen before, 2) those that have only a Person, 3) those that have only an Account, 4) those that have both an Account and a Person, 5) and those that have only an Account with a linked Person.

But handling of case 3 is broken: it will create a new Person, but leave the EmailAddress with only an Account, bringing the EmailAddress to case 5, not case 4 as was intended. A subsequent ensurePerson call with the same address will link the Person, completing the changes for case 4.

This branch fixes ensurePerson to correctly handle case 3, setting EmailAddress.person whether the Person is new or old.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-744703/+merge/55268
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-744703 into lp:launchpad.

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2011-03-25 09:10:21 +0000
+++ database/schema/security.cfg	2011-03-29 03:24:32 +0000
@@ -601,7 +601,7 @@
 public.distributionsourcepackage        = SELECT, INSERT, UPDATE
 public.distroarchseries                 = SELECT
 public.distroseries                     = SELECT
-public.emailaddress                     = SELECT, INSERT
+public.emailaddress                     = SELECT, INSERT, UPDATE
 public.job                              = SELECT, INSERT, UPDATE
 public.language                         = SELECT
 public.libraryfilealias                 = SELECT, INSERT
@@ -1384,7 +1384,7 @@
 public.distributionjob                  = SELECT, INSERT
 public.person                           = SELECT, INSERT
 public.personsettings                   = SELECT, INSERT
-public.emailaddress                     = SELECT, INSERT
+public.emailaddress                     = SELECT, INSERT, UPDATE
 public.teamparticipation                = SELECT, INSERT
 public.teammembership                   = SELECT
 public.distrocomponentuploader          = SELECT

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-03-26 16:28:39 +0000
+++ lib/lp/registry/model/person.py	2011-03-29 03:24:32 +0000
@@ -3072,28 +3072,27 @@
             account = IMasterStore(Account).get(
                 Account, email_address.accountID)
             account_person = self.getByAccount(account)
-            # There is a `Person` linked to the `Account`, link the
+            if account_person is None:
+                # There is no associated `Person` to the email `Account`.
+                # This is probably because the account was created externally
+                # to Launchpad. Create just the `Person`, associate it with
+                # the `EmailAddress` and return it.
+                name = generate_nick(email)
+                account_person = self._newPerson(
+                    name, displayname, hide_email_addresses=True,
+                    rationale=rationale, comment=comment,
+                    registrant=registrant, account=email_address.account)
+            # There is (now) a `Person` linked to the `Account`, link the
             # `EmailAddress` to this `Person` and return it.
-            if account_person is not None:
-                master_email = IMasterStore(EmailAddress).get(
-                    EmailAddress, email_address.id)
-                master_email.personID = account_person.id
-                # Populate the previously empty 'preferredemail' cached
-                # property, so the Person record is up-to-date.
-                if master_email.status == EmailAddressStatus.PREFERRED:
-                    cache = get_property_cache(account_person)
-                    cache.preferredemail = master_email
-                return account_person
-            # There is no associated `Person` to the email `Account`.
-            # This is probably because the account was created externally
-            # to Launchpad. Create just the `Person`, associate it with
-            # the `EmailAddress` and return it.
-            name = generate_nick(email)
-            person = self._newPerson(
-                name, displayname, hide_email_addresses=True,
-                rationale=rationale, comment=comment, registrant=registrant,
-                account=email_address.account)
-            return person
+            master_email = IMasterStore(EmailAddress).get(
+                EmailAddress, email_address.id)
+            master_email.personID = account_person.id
+            # Populate the previously empty 'preferredemail' cached
+            # property, so the Person record is up-to-date.
+            if master_email.status == EmailAddressStatus.PREFERRED:
+                cache = get_property_cache(account_person)
+                cache.preferredemail = master_email
+            return account_person
 
         # Easy, return the `Person` associated with the existing
         # `EmailAddress`.

=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py	2010-12-08 17:22:23 +0000
+++ lib/lp/registry/tests/test_personset.py	2011-03-29 03:24:32 +0000
@@ -102,6 +102,9 @@
         ensured_person = self.person_set.ensurePerson(
             self.email_address, self.displayname, self.rationale)
         self.assertEquals(test_account.id, ensured_person.account.id)
+        self.assertEquals(
+            test_account.preferredemail, ensured_person.preferredemail)
+        self.assertEquals(ensured_person, test_account.preferredemail.person)
         self.assertTrue(ensured_person.hide_email_addresses)
 
     def test_ensurePerson_for_existing_account_with_person(self):