Merge lp:~jcsackett/launchpad/no-canonical-urls-628247 into lp:launchpad

[j.c.sackett]

Fixes a bad condition in canonical validation of team email addresses; now instead of assuming that if an email is taken in LP there must be a team, it finds all associations with that email before returning errors.

Summary
=======

Fixes a condition which raised an OOPS on team creation when emails were taken.

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 function to get the highest order association of an email (Person/Team, then Account, then the Email model instance itself, then None). This is used to craft an appropriate error message. Previously the checker assumed that if an email was already in the LP db, then a person must exist--it was in crafting the error message with this assumed person that NoCanonicalURL would be raised.

Tests
=====

bin/test -t TestTeamCreation

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

If you create an Account without a Person in the database for launchpad.dev, then try to add a team via the web interface with that email address, it should inform you in an error on contactemail that an account uses that email address. Previously it would OOPS.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/database/emailaddress.py
  lib/canonical/launchpad/interfaces/emailaddress.py
  lib/canonical/launchpad/interfaces/validation.py
  lib/lp/registry/browser/tests/test_person_view.py

./lib/canonical/launchpad/interfaces/validation.py
      65: E302 expected 2 blank lines, found 1

Error in validation.py is from the linter getting confused about whitespace around comments.

[j.c.sackett (jcsackett)]

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 '
+ ...

[Aaron Bentley (abentley)]

review: Approve

=== modified file 'lib/canonical/launchpad/interfaces/emailaddress.py'
--- lib/canonical/launchpad/interfaces/emailaddress.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/emailaddress.py	2010-09-15 13:37:49 +0000
@@ -115,7 +115,7 @@
 
     def destroySelf():
         """Destroy this email address and any associated subscriptions.
-        
+
         :raises UndeletableEmailAddress: When the email address is a person's
             preferred one or a hosted mailing list's address.
         """
@@ -153,4 +153,3 @@
 
         Return None if there is no such email address.
         """
-

=== modified file 'lib/canonical/launchpad/interfaces/validation.py'
--- lib/canonical/launchpad/interfaces/validation.py	2010-08-22 18:31:30 +0000
+++ lib/canonical/launchpad/interfaces/validation.py	2010-09-15 13:37:49 +0000
@@ -19,7 +19,7 @@
     'validate_new_distrotask',
     'valid_upstreamtask',
     'valid_password',
-    'validate_date_interval'
+    'validate_date_interval',
     ]
 
 from cgi import escape
@@ -31,10 +31,16 @@
 
 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,
+    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 import canonical_url
 from canonical.launchpad.webapp.menu import structured
 from lp.app.errors import NotFoundError
 
@@ -124,6 +130,7 @@
             scheme (for instance, http:// for a web URL), and ensure the
             URL uses either http, https or ftp.""")))
 
+
 def valid_branch_url(branch_url):
     """Returns True if web_ref is a valid download URL, or raises a
     LaunchpadValidationError.
@@ -188,6 +195,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)})
+        elif 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
@@ -195,16 +224,9 @@
     """
     from canonical.launchpad.webapp import canonical_url
     from canonical.launchpad.interfaces import IEmailAddressSet
+
     _validate_email(email)
-    email_address = getUtility(IEmailAddressSet).getByEmail(email)
-    if email_address is not None:
-        person = email_address.person
-        message = _('${email} is already registered in Launchpad and is '
-                    'associated with <a href="${url}">${team}</a>.',
-                    mapping={'email': escape(email),
-                             'url': canonical_url(person),
-                             'team': escape(person.displayname)})
-        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-02 19:28:35 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-09-15 13:37:49 +0000
@@ -5,6 +5,7 @@
 
 import transaction
 from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
 
 from canonical.config import config
 from canonical.launchpad.ftests import (
@@ -29,7 +30,10 @@
     )
 
 from lp.registry.interfaces.karma import IKarmaCacheManager
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    IPersonSet,
+    )
 from lp.registry.interfaces.teammembership import (
     ITeamMembershipSet,
     TeamMembershipStatus,
@@ -257,6 +261,58 @@
         self.assertFalse(self.view.form_fields['name'].for_display)
 
 
+class TestTeamCreationView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestTeamCreationView, self).setUp()
+        person = self.factory.makePerson()
+        login_person(person)
+
+    def test_team_creation_good_data(self):
+        form = {
+            'field.actions.create': 'Create Team',
+            'field.contactemail': '[e-mail]',
+            'field.displayname': 'liberty-land',
+            'field.name': 'libertyland',
+            'field.renewal_policy': 'NONE',
+            'field.renewal_policy-empty-marker': 1,
+            'field.subscriptionpolicy': 'RESTRICTED',
+            'field.subscriptionpolicy-empty-marker': 1,
+            }
+        person_set = getUtility(IPersonSet)
+        view = create_initialized_view(
+            person_set, '+newteam', form=form)
+        team = person_set.getByName('libertyland')
+        self.assertTrue(team is not None)
+        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)
+        form = {
+            'field.actions.create': 'Create Team',
+            'field.contactemail': email_address,
+            'field.displayname': 'liberty-land',
+            'field.name': 'libertyland',
+            'field.renewal_policy': 'NONE',
+            'field.renewal_policy-empty-marker': 1,
+            'field.subscriptionpolicy': 'RESTRICTED',
+            'field.subscriptionpolicy-empty-marker': 1,
+            }
+        person_set = getUtility(IPersonSet)
+        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)
+        error_msg = view.errors[0].errors[0]
+        self.assertEqual(expected_msg, error_msg)
+
+
 class TestPersonParticipationView(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
@@ -602,6 +658,7 @@
         self.assertEqual(
             'This account is already deactivated.', view.errors[0])
 
+
 class TestTeamInvitationView(TestCaseWithFactory):
     """Tests for TeamInvitationView."""