launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/no-canonical-urls-628247 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #628247 NoCanonicalUrl raised creating a new team
  https://bugs.launchpad.net/bugs/628247


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.
-- 
https://code.launchpad.net/~jcsackett/launchpad/no-canonical-urls-628247/+merge/35301
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/no-canonical-urls-628247 into lp:launchpad/devel.

=== modified file 'lib/canonical/launchpad/database/emailaddress.py'
--- lib/canonical/launchpad/database/emailaddress.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/database/emailaddress.py	2010-09-13 16:14:06 +0000
@@ -145,6 +145,21 @@
             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-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/emailaddress.py	2010-09-13 16:14:06 +0000
@@ -154,3 +154,11 @@
         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-08-22 18:31:30 +0000
+++ lib/canonical/launchpad/interfaces/validation.py	2010-09-13 16:14:06 +0000
@@ -31,14 +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
 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
 
-
 def can_be_nominated_for_series(series):
     """Can the bug be nominated for these series?"""
     current_bug = getUtility(ILaunchBag).bug
@@ -196,14 +198,32 @@
     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)})
+    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))
     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-13 16:14:06 +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 (
@@ -13,6 +14,8 @@
     )
 from canonical.launchpad.interfaces.account import AccountStatus
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.interfaces import NoCanonicalUrl
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing import (
     DatabaseFunctionalLayer,
@@ -29,7 +32,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,
@@ -256,6 +262,58 @@
         self.view.initialize()
         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': 'contactemail@xxxxxxxxxxx',
+            '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):
+        account = self.factory.makeAccount(
+            displayname='libertylandaccount',
+            status=AccountStatus.NOACCOUNT)
+        email_address = removeSecurityProxy(account.guessed_emails[0]).email
+        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):