Merge lp:~jcsackett/launchpad/user-email-existing-account-576757 into lp:launchpad

[j.c.sackett]

Fixes a bad conditional in email validation that assumes the email must have a person, causing an OOPS. Now the validation checks the email info to create the best possible error message from what's available.

Summary
=======

Fixes a condition which raised an OOPS on adding emails if the email already was associated with an account but not a person.

Proposed Fix
============

Create or improve a checker for email addresses so that the condition can't happen.

Pre-implementation notes
========================

Spoke with Curtis Hovey (sinzui) who proposed that the error wasn't actually where the OOPS was raised, but earlier in checking the availability of the email address entered.

Implementation details
======================

Adds a few conditions to the email validation. Where the previous setup simply assumed the email has a person to create the error message, the function now checks that the person actually exists, and if not checks for account to create the message. If no account exists, it defaults to a very basic error message.

Tests
=====

bin/test -t test_add_email

Demo and Q/A
============

If you create an Account without a Person in the database for launchpad.dev, then attempt to add that email to an existing account in the launchpad.dev, it should provide an error message rather than OOPSing.

Lint
====
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_person_view.py

[Robert Collins (lifeless)]

There's a fair amount of duplicate code in this, mainly in tests. It would be nice to look at reducing that in future.

review: Approve

[j.c.sackett (jcsackett)]

Agreed. Curtis and I had a long talk about how we might tackle this at some point.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-09-11 19:25:13 +0000
+++ lib/lp/registry/browser/person.py	2010-09-16 13:43:45 +0000
@@ -4835,6 +4835,11 @@
                 "'%s' doesn't seem to be a valid email address." % newemail)
             return self.errors
 
+        # XXX j.c.sackett 2010-09-15 bug=628247, 576757 There is a validation
+        # system set up for this that is almost identical in
+        # canonical.launchpad.interfaces.validation, called
+        # _check_email_available or similar. It would be really nice if we
+        # could merge that code somehow with this.
         email = getUtility(IEmailAddressSet).getByEmail(newemail)
         person = self.context
         if email is not None:
@@ -4846,20 +4851,32 @@
                     "detected it as being yours. If it was detected by our "
                     "system, it's probably shown on this page and is waiting "
                     "to be confirmed as yours." % newemail)
-            else:
+            elif email.person is not None:
                 owner = email.person
                 owner_name = urllib.quote(owner.name)
                 merge_url = (
                     '%s/+requestmerge?field.dupe_person=%s'
                     % (canonical_url(getUtility(IPersonSet)), owner_name))
-                self.addError(
-                    structured(
-                        "The email address '%s' is already registered to "
-                        '<a href="%s">%s</a>. If you think that is a '
-                        'duplicated account, you can <a href="%s">merge it'
-                        "</a> into your account.",
-                        newemail, canonical_url(owner), owner.displayname,
-                        merge_url))
+                self.addError(structured(
+                    "The email address '%s' is already registered to "
+                    '<a href="%s">%s</a>. If you think that is a '
+                    'duplicated account, you can <a href="%s">merge it'
+                    "</a> into your account.",
+                    newemail,
+                    canonical_url(owner),
+                    owner.displayname,
+                    merge_url))
+            elif email.account is not None:
+                account = email.account
+                self.addError(structured(
+                    "The email address '%s' is already registered to an "
+                    "account, %s.",
+                    newemail,
+                    account.displayname))
+            else:
+                self.addError(structured(
+                    "The email address '%s' is already registered.",
+                    newemail))
         return self.errors
 
     @action(_("Add"), name="add_email", validator=validate_action_add_email)
 
=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2010-09-02 19:28:35 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-09-16 13:43:45 +0000
@@ -11,7 +11,9 @@
     ANONYMOUS,
     login,
     )
+from canonical.launchpad.interfaces.authtoken import LoginTokenType
 from canonical.launchpad.interfaces.account import AccountStatus
+from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing import (
@@ -207,7 +209,8 @@
 
     def setUp(self):
         TestCaseWithFactory.setUp(self)
-        self.person = self.factory.makePerson()
+        self.valid_email_address = self.factory.getUniqueEmailAddress()
+        self.person = self.factory.makePerson(email=self.valid_email_address)
         login_person(self.person)
         self.ppa = self.factory.makeArchive(owner=self.person)
         self.view = PersonEditView(
@@ -256,6 +259,42 @@
         self.view.initialize()
         self.assertFalse(self.view.form_fields['name'].for_display)
 
+    def test_add_email_good_data(self):
+        email_address = self.factory.getUniqueEmailAddress()
+        form = {
+            'field.VALIDATED_SELECTED': self.valid_email_address,
+            'field.VALIDATED_SELECTED-empty-marker': 1,
+            'field.actions.add_email': 'Add',
+            'field.newemail': email_address,
+            }
+        view = create_initialized_view(self.person, "+editemails", form=form)
+
+        # If everything worked, there should now be a login token to validate
+        # this email address for this user.
+        token = getUtility(ILoginTokenSet).searchByEmailRequesterAndType(
+            email_address,
+            self.person,
+            LoginTokenType.VALIDATEEMAIL)
+        self.assertTrue(token is not None)
+
+    def test_add_email_address_taken(self):
+        email_address = self.factory.getUniqueEmailAddress()
+        account = self.factory.makeAccount(
+            displayname='deadaccount',
+            email=email_address,
+            status=AccountStatus.NOACCOUNT)
+        form = {
+            'field.VALIDATED_SELECTED': self.valid_email_address,
+            'field.VALIDATED_SELECTED-empty-marker': 1,
+            'field.actions.add_email': 'Add',
+            'field.newemail': email_address,
+            }
+        view = create_initialized_view(self.person, "+editemails", form=form)
+        error_msg = view.errors[0]
+        expected_msg = ("The email address '%s' is already registered to an "
+                        "account, deadaccount." % email_address)
+        self.assertEqual(expected_msg, error_msg)
+
 
 class TestPersonParticipationView(TestCaseWithFactory):
 
@@ -602,6 +641,7 @@
         self.assertEqual(
             'This account is already deactivated.', view.errors[0])
 
+
 class TestTeamInvitationView(TestCaseWithFactory):
     """Tests for TeamInvitationView."""