launchpad-reviewers team mailing list archive

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

j.c.sackett has proposed merging lp:~jcsackett/launchpad/join-not-allowed-244527 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #244527 JoinNotAllowed should cause a 400 response when raised on the API layer
  https://bugs.launchpad.net/bugs/244527



Summary
=======

As with many exceptions JoinNotAllowed wasn't properly exposed to the webservice, so API uses that trigger it get a 500 which mucks up our OOPS reports.

This is easily remedied by moving it to registry.errors, which is registered with the webservice and providing it a 400 webservice_error so it's reported back across the webservice rather than triggering a server error. 

Proposed fix
============

Move the JoinNotAllowed to registry.errors and the BAD_REQUEST webservice_error type to it.

Preimplementation talk
======================

Spoke with Curtis Hovey.

Implementation details
======================

As in proposed.

Tests
=====

bin/test -vvct TestTeamJoining

Lint
====

make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/errors.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_team_webservice.py

./lib/lp/registry/interfaces/person.py
     491: E302 expected 2 blank lines, found 1

The error lint found has to do with it being confused by comments.

-- 
https://code.launchpad.net/~jcsackett/launchpad/join-not-allowed-244527/+merge/38949
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/join-not-allowed-244527 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/errors.py'
--- lib/lp/registry/errors.py	2010-09-30 15:13:57 +0000
+++ lib/lp/registry/errors.py	2010-10-20 16:11:15 +0000
@@ -3,19 +3,20 @@
 
 __metaclass__ = type
 __all__ = [
-    'PrivatePersonLinkageError',
-    'NameAlreadyTaken',
-    'NoSuchDistroSeries',
-    'UserCannotChangeMembershipSilently',
-    'NoSuchSourcePackageName',
     'CannotTransitionToCountryMirror',
     'CountryMirrorAlreadySet',
+    'DeleteSubscriptionError',
+    'JoinNotAllowed',
     'MirrorNotOfficial',
     'MirrorHasNoHTTPURL',
     'MirrorNotProbed',
-    'DeleteSubscriptionError',
+    'NameAlreadyTaken',
+    'NoSuchDistroSeries',
+    'NoSuchSourcePackageName',
+    'PrivatePersonLinkageError',
+    'TeamMembershipTransitionError',
+    'UserCannotChangeMembershipSilently',
     'UserCannotSubscribePerson',
-    'TeamMembershipTransitionError',
     ]
 
 import httplib
@@ -114,3 +115,8 @@
     or an invalid transition (e.g. unicorn).
     """
     webservice_error(httplib.BAD_REQUEST)
+
+
+class JoinNotAllowed(Exception):
+    """User is not allowed to join a given team."""
+    webservice_error(httplib.BAD_REQUEST)

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-10-07 14:03:32 +0000
+++ lib/lp/registry/interfaces/person.py	2010-10-20 16:11:15 +0000
@@ -26,7 +26,6 @@
     'ITeamReassignment',
     'ImmutableVisibilityError',
     'InvalidName',
-    'JoinNotAllowed',
     'NoSuchPerson',
     'PersonCreationRationale',
     'PersonVisibility',
@@ -2146,10 +2145,6 @@
     """XMLRPC application root for ISoftwareCenterAgentAPI."""
 
 
-class JoinNotAllowed(Exception):
-    """User is not allowed to join a given team."""
-
-
 class ImmutableVisibilityError(Exception):
     """A change in team membership visibility is not allowed."""
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-10-07 14:03:32 +0000
+++ lib/lp/registry/model/person.py	2010-10-20 16:11:15 +0000
@@ -191,7 +191,10 @@
     HasMergeProposalsMixin,
     HasRequestedReviewsMixin,
     )
-from lp.registry.errors import NameAlreadyTaken
+from lp.registry.errors import (
+    JoinNotAllowed,
+    NameAlreadyTaken,
+    )
 from lp.registry.interfaces.codeofconduct import ISignedCodeOfConductSet
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.gpg import IGPGKeySet
@@ -217,7 +220,6 @@
     IPerson,
     IPersonSet,
     ITeam,
-    JoinNotAllowed,
     PersonalStanding,
     PersonCreationRationale,
     PersonVisibility,

=== modified file 'lib/lp/registry/tests/test_team_webservice.py'
--- lib/lp/registry/tests/test_team_webservice.py	2010-10-04 20:46:55 +0000
+++ lib/lp/registry/tests/test_team_webservice.py	2010-10-20 16:11:15 +0000
@@ -4,14 +4,18 @@
 __metaclass__ = type
 
 import httplib
-import unittest
 
 from lazr.restfulclient.errors import HTTPError
 
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.registry.interfaces.person import PersonVisibility
+from lp.registry.interfaces.person import (
+    PersonVisibility,
+    TeamSubscriptionPolicy,
+    )
 from lp.testing import (
     launchpadlib_for,
+    login_person,
+    logout,
     TestCaseWithFactory,
     )
 
@@ -48,5 +52,36 @@
         self.assertEqual(httplib.FORBIDDEN, api_error.response.status)
 
 
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
+class TestTeamJoining(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_restricted_rejects_membership(self):
+        self.person = self.factory.makePerson(name='test-person')
+        self.team = self.factory.makeTeam(name='test-team')
+        login_person(self.team.teamowner)
+        self.team.subscriptionpolicy = TeamSubscriptionPolicy.RESTRICTED
+        logout()
+
+        launchpad = launchpadlib_for("test", self.person)
+        person = launchpad.people['test-person']
+        api_error = self.assertRaises(
+            HTTPError,
+            person.join,
+            team='test-team')
+        self.assertEqual(httplib.BAD_REQUEST, api_error.response.status)
+
+    def test_open_accepts_membership(self):
+        # Calling person.join with a team that has an open membership
+        # subscription policy should add that that user to the team.
+        self.person = self.factory.makePerson(name='test-person')
+        self.team = self.factory.makeTeam(name='test-team')
+        login_person(self.team.teamowner)
+        self.team.subscriptionpolicy = TeamSubscriptionPolicy.OPEN
+        logout()
+
+        launchpad = launchpadlib_for("test", self.person)
+        test_person = launchpad.people['test-person']
+        test_team = launchpad.people['test-team']
+        test_person.join(team=test_team.self_link)
+        self.assertEqual(1, self.person.team_memberships.count())