launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-823490 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #823490 in Launchpad itself: "AssertionError claiming a team."
  https://bugs.launchpad.net/launchpad/+bug/823490

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-823490/+merge/72577

= Summary =

If two outstanding attempts to claim an automatically created team are
both acted upon, the second one lost and caused an OOPS.  An error
should be shown to the user instead.

== Proposed fix ==

Don't use assert but instead raise and catch an exception and show the
error.

== Pre-implementation notes ==

None

== Implementation details ==

As above.

== Tests ==

bin/test -vvt test_logintoken -t doc/person.txt

== Demo and Q/A ==

Go to https://launchpad.dev/~doc and click "Is this a team you run?"
Enter doc@xxxxxxxxxxxxxxxx for the email address.
Do it again.
Now you should have two emails with the subject "Launchpad: Claim existing team".  Each will have a different token.
Follow the link in the first one to claim the team successfully.  You don't have to enter any info, just click 'Continue'.
Follow the link in the second one to attempt to claim the team again.  You should get an error message.


= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/browser/logintoken.py
  lib/canonical/launchpad/browser/tests/test_logintoken.py
  lib/lp/registry/doc/person.txt
  lib/lp/registry/model/person.py
-- 
https://code.launchpad.net/~bac/launchpad/bug-823490/+merge/72577
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-823490 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/browser/logintoken.py'
--- lib/canonical/launchpad/browser/logintoken.py	2011-04-09 02:52:13 +0000
+++ lib/canonical/launchpad/browser/logintoken.py	2011-08-23 14:00:29 +0000
@@ -18,7 +18,6 @@
 import cgi
 import urllib
 
-import pytz
 from zope.app.form.browser import TextAreaWidget
 from zope.component import getUtility
 from zope.interface import (
@@ -73,9 +72,6 @@
     )
 
 
-UTC = pytz.UTC
-
-
 class LoginTokenSetNavigation(GetitemNavigation):
 
     usedfor = ILoginTokenSet
@@ -245,7 +241,15 @@
 
     @action(_('Continue'), name='confirm')
     def confirm_action(self, action, data):
-        self.claimed_profile.convertToTeam(team_owner=self.context.requester)
+        # Avoid circular imports.
+        from lp.registry.model.person import AlreadyConvertedException
+        try:
+            self.claimed_profile.convertToTeam(
+                team_owner=self.context.requester)
+        except AlreadyConvertedException, e:
+            self.request.response.addErrorNotification(e)
+            self.context.consume()
+            return
         # Although we converted the person to a team it seems that the
         # security proxy still thinks it's an IPerson and not an ITeam,
         # which means to edit it we need to be logged in as the person we

=== modified file 'lib/canonical/launchpad/browser/tests/test_logintoken.py'
--- lib/canonical/launchpad/browser/tests/test_logintoken.py	2011-08-12 11:37:08 +0000
+++ lib/canonical/launchpad/browser/tests/test_logintoken.py	2011-08-23 14:00:29 +0000
@@ -11,6 +11,7 @@
     )
 from canonical.launchpad.ftests import LaunchpadFormHarness
 from canonical.launchpad.interfaces.authtoken import LoginTokenType
+from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
 from canonical.launchpad.interfaces.logintoken import ILoginTokenSet
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.testing import TestCaseWithFactory
@@ -51,7 +52,7 @@
 
     def _testCancelAction(self, view_class, token):
         """Test the 'Cancel' action of the given view, using the given token.
-        
+
         To test that the action works, we just submit the form with that
         action, check that there are no errors and make sure that the view's
         next_url is what we expect.
@@ -63,3 +64,35 @@
         self.assertEquals(actions['field.actions.cancel'].submitted(), True)
         self.assertEquals(harness.view.errors, [])
         self.assertEquals(harness.view.next_url, self.expected_next_url)
+
+
+class TestClaimTeamView(TestCaseWithFactory):
+    """Test the claiming of a team via login token."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        self.claimer = self.factory.makePerson(name='claimer')
+        self.claimee_email = 'claimee@xxxxxxxxxxx'
+        self.claimee = self.factory.makePerson(
+            name='claimee', email=self.claimee_email,
+            email_address_status=EmailAddressStatus.NEW)
+
+    def _claimToken(self, token):
+        harness = LaunchpadFormHarness(token, ClaimTeamView)
+        harness.submit('confirm', {})
+        return [n.message for n in harness.request.notifications]
+
+    def test_CannotClaimTwice(self):
+        token1 = getUtility(ILoginTokenSet).new(
+            requester=self.claimer, requesteremail=None,
+            email=self.claimee_email, tokentype=LoginTokenType.TEAMCLAIM)
+        token2 = getUtility(ILoginTokenSet).new(
+            requester=self.claimer, requesteremail=None,
+            email=self.claimee_email, tokentype=LoginTokenType.TEAMCLAIM)
+        msgs = self._claimToken(token1)
+        self.assertEquals([u'Team claimed successfully'], msgs)
+        msgs = self._claimToken(token2)
+        self.assertEquals(
+            [u'claimee has already been converted to a team.'], msgs)

=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt	2011-06-28 15:04:29 +0000
+++ lib/lp/registry/doc/person.txt	2011-08-23 14:00:29 +0000
@@ -529,7 +529,7 @@
     >>> not_a_person.convertToTeam(team_owner=landscape_devs)
     Traceback (most recent call last):
     ...
-    AssertionError: Can't convert a team to a team.
+    AlreadyConvertedException: foo-v has already been converted to a team.
 
 
 Team members

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-08-17 18:08:36 +0000
+++ lib/lp/registry/model/person.py	2011-08-23 14:00:29 +0000
@@ -7,6 +7,7 @@
 
 __metaclass__ = type
 __all__ = [
+    'AlreadyConvertedException',
     'get_recipients',
     'generate_nick',
     'IrcID',
@@ -14,6 +15,7 @@
     'JabberID',
     'JabberIDSet',
     'JoinTeamEvent',
+    'NicknameGenerationError',
     'Owner',
     'Person',
     'person_sort_key',
@@ -301,6 +303,10 @@
     )
 
 
+class AlreadyConvertedException(Exception):
+    """Raised when an attempt to claim a team that has been claimed."""
+
+
 class JoinTeamEvent:
     """See `IJoinTeamEvent`."""
 
@@ -674,7 +680,9 @@
 
     def convertToTeam(self, team_owner):
         """See `IPerson`."""
-        assert not self.is_team, "Can't convert a team to a team."
+        if self.is_team:
+            raise AlreadyConvertedException(
+                "%s has already been converted to a team." % self.name)
         assert self.account_status == AccountStatus.NOACCOUNT, (
             "Only Person entries whose account_status is NOACCOUNT can be "
             "converted into teams.")