launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/user-email-existing-account-576757 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #576757 user oopsing when attempting to add an email address belonging to a never activated account
  https://bugs.launchpad.net/bugs/576757


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

-- 
https://code.launchpad.net/~jcsackett/launchpad/user-email-existing-account-576757/+merge/35575
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/user-email-existing-account-576757 into lp:launchpad/devel.

=== 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-15 19:56:55 +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-15 19:56:55 +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."""