Code review comment for lp:~jcsackett/launchpad/no-canonical-urls-628247

Diff from the last change:

=== modified file 'lib/canonical/launchpad/database/emailaddress.py'
--- lib/canonical/launchpad/database/emailaddress.py 2010-09-13 16:17:51 +0000
+++ lib/canonical/launchpad/database/emailaddress.py 2010-09-13 19:29:21 +0000
@@ -145,22 +145,6 @@
             personID=personID,
             account=account)

- def getEmailAssociation(self, email):
- """See IEmailAddressSet`."""
- # First check if the email address is in use.
- email_address = self.getByEmail(email)
- if email_address is None:
- # If the email address doesn't exist in the system, it is
- # available.
- return
-
- # The email already exists; determine what has it.
- if email_address.person is not None:
- return email_address.person
- if email_address.account is not None:
- return email_address.account
- return email_address
-

 class UndeletableEmailAddress(Exception):
     """User attempted to delete an email address which can't be deleted."""

=== modified file 'lib/canonical/launchpad/interfaces/emailaddress.py'
--- lib/canonical/launchpad/interfaces/emailaddress.py 2010-09-13 16:17:51 +0000
+++ lib/canonical/launchpad/interfaces/emailaddress.py 2010-09-13 19:29:21 +0000
@@ -153,12 +153,3 @@

         Return None if there is no such email address.
         """
-
- def getEmailAssociation(email):
- """Return the entity associated with the email.
-
- Returns the person or team with the email, if one exists. If not,
- returns the account with the email, if it exists. If it doesn't,
- return the email model instance, if it exists. If it doesn't, return
- None.
- """

=== modified file 'lib/canonical/launchpad/interfaces/validation.py'
--- lib/canonical/launchpad/interfaces/validation.py 2010-09-13 16:17:51 +0000
+++ lib/canonical/launchpad/interfaces/validation.py 2010-09-13 19:29:21 +0000
@@ -32,13 +32,15 @@
 from canonical.launchpad import _
 from canonical.launchpad.interfaces.launchpad import ILaunchBag
 from canonical.launchpad.interfaces.account import IAccount
-from canonical.launchpad.interfaces.emailaddress import IEmailAddress
+from canonical.launchpad.interfaces.emailaddress import (
+ IEmailAddress,
+ IEmailAddressSet,
+ )
 from canonical.launchpad.validators import LaunchpadValidationError
 from canonical.launchpad.validators.cve import valid_cve
 from canonical.launchpad.validators.email import valid_email
 from canonical.launchpad.validators.url import valid_absolute_url
 from canonical.launchpad.webapp.menu import structured
-
 from lp.app.errors import NotFoundError

@@ -192,6 +194,28 @@
             "${email} isn't a valid email address.",
             mapping={'email': email}))

+def _check_email_availability(email):
+ email_address = getUtility(IEmailAddressSet).getByEmail(email)
+ if email_address is not None:
+ # The email already exists; determine what has it.
+ if email_address.person is not None:
+ person = email_address.person
+ message = _('${email} is already registered in Launchpad and is '
+ 'associated with <a href="${url}">${person}</a>.',
+ mapping={'email': escape(email),
+ 'url': canonical_url(person),
+ 'person': escape(person.displayname)})
+ if email_address.account is not None:
+ account = email_address.account
+ message = _('${email} is already registered in Launchpad and is '
+ 'associated with the ${account} account.',
+ mapping={'email': escape(email),
+ 'account': escape(account.displayname)})
+ else:
+ message = _('${email} is already registered in Launchpad.',
+ mapping={'email': escape(email)})
+ raise LaunchpadValidationError(structured(message))
+

 def validate_new_team_email(email):
     """Check that the given email is valid and not registered to
@@ -199,34 +223,9 @@
     """
     from canonical.launchpad.webapp import canonical_url
     from canonical.launchpad.interfaces import IEmailAddressSet
+
     _validate_email(email)
- emailaddress_set = getUtility(IEmailAddressSet)
- email_association = emailaddress_set.getEmailAssociation(email)
-
- # Since an associaiton exists, find out what it is to get the best error
- # message. Ideally, this would check if it's a person, then an account,
- # and then assume email, but importing IPerson in this file causes a
- # circular import, so assume person if the association is neither Account
- # nor EmailAddress.
- if email_association is not None:
- if IEmailAddress.providedBy(email_association):
- message = _('${email} is already registered in Launchpad.',
- mapping={'email': escape(email)})
- elif IAccount.providedBy(email_association):
- account_name = email_association.displayname
- message = _('${email} is already registered in Launchpad and is '
- 'associated with the ${account} account.',
- mapping={'email': escape(email),
- 'account': escape(account_name)})
- else:
- team_name = email_association.displayname
- message = _('${email} is already registered in Launchpad and is '
- 'associated with <a href="${url}">${team}</a>.',
- mapping={'email': escape(email),
- 'url': canonical_url(email_association),
- 'team': escape(team_name)})
-
- raise LaunchpadValidationError(structured(message))
+ _check_email_availability(email)
     return True

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py 2010-09-13 16:17:51 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py 2010-09-13 19:29:21 +0000
@@ -286,15 +286,14 @@
             person_set, '+newteam', form=form)
         team = person_set.getByName('libertyland')
         self.assertTrue(team is not None)
- self.assertEqual(
- 'libertyland',
- team.name)
+ self.assertEqual('libertyland', team.name)

     def test_validate_email_catches_taken_emails(self):
+ email_address = self.factory.getUniqueEmailAddress()
         account = self.factory.makeAccount(
             displayname='libertylandaccount',
+ email=email_address,
             status=AccountStatus.NOACCOUNT)
- email_address = removeSecurityProxy(account.guessed_emails[0]).email
         form = {
             'field.actions.create': 'Create Team',
             'field.contactemail': email_address,
@@ -306,8 +305,7 @@
             'field.subscriptionpolicy-empty-marker': 1,
             }
         person_set = getUtility(IPersonSet)
- view = create_initialized_view(
- person_set, '+newteam', form=form)
+ view = create_initialized_view(person_set, '+newteam', form=form)
         expected_msg = ('%s is already registered in Launchpad and is '
                         'associated with the libertylandaccount '
                         'account.' % email_address)