Launchpad itself

Merge lp:~jcsackett/launchpad/join-not-allowed-244527 into lp:launchpad

  1. join-not-allowed-244527
  2. Merge into devel
Proposed by j.c.sackett
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 11783
Proposed branch: lp:~jcsackett/launchpad/join-not-allowed-244527
Merge into: lp:launchpad
Diff against target: 160 lines (+65/-18)
4 files modified
lib/lp/registry/errors.py (+13/-7)
lib/lp/registry/interfaces/person.py (+0/-5)
lib/lp/registry/model/person.py (+4/-2)
lib/lp/registry/tests/test_team_webservice.py (+48/-4)
To merge this branch: bzr merge lp:~jcsackett/launchpad/join-not-allowed-244527
Reviewer Review Type Date Requested Status
Edwin Grubbs (community) code Approve
Review via email: mp+38949@code.launchpad.net

Commit message

Moves JoinNotAllowed to registry.errors. This registers it on the webservice so that it reports HTTP 400 BAD REQUEST across the API instead of a server error.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Edwin Grubbs (edwin-grubbs) wrote :

Hi JC,

This branch looks good. I just have one suggestion.

-Edwin

>=== 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:20:29 +0000
>@@ -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())

Instead of checking the count, can you make this assertion:
    self.assertEqual(
        ['test-team'],
        [membership.team.name
         for membership in self.person.team_memeberships])

review: Approve (code)
Revision history for this message
j.c.sackett (jcsackett) wrote :

> Instead of checking the count, can you make this assertion:
> self.assertEqual(
> ['test-team'],
> [membership.team.name
> for membership in self.person.team_memeberships])

Done, as of this morning. Thanks, Edwin.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/errors.py'
2--- lib/lp/registry/errors.py 2010-09-30 15:13:57 +0000
3+++ lib/lp/registry/errors.py 2010-10-22 14:12:56 +0000
4@@ -3,19 +3,20 @@
5
6 __metaclass__ = type
7 __all__ = [
8- 'PrivatePersonLinkageError',
9- 'NameAlreadyTaken',
10- 'NoSuchDistroSeries',
11- 'UserCannotChangeMembershipSilently',
12- 'NoSuchSourcePackageName',
13 'CannotTransitionToCountryMirror',
14 'CountryMirrorAlreadySet',
15+ 'DeleteSubscriptionError',
16+ 'JoinNotAllowed',
17 'MirrorNotOfficial',
18 'MirrorHasNoHTTPURL',
19 'MirrorNotProbed',
20- 'DeleteSubscriptionError',
21+ 'NameAlreadyTaken',
22+ 'NoSuchDistroSeries',
23+ 'NoSuchSourcePackageName',
24+ 'PrivatePersonLinkageError',
25+ 'TeamMembershipTransitionError',
26+ 'UserCannotChangeMembershipSilently',
27 'UserCannotSubscribePerson',
28- 'TeamMembershipTransitionError',
29 ]
30
31 import httplib
32@@ -114,3 +115,8 @@
33 or an invalid transition (e.g. unicorn).
34 """
35 webservice_error(httplib.BAD_REQUEST)
36+
37+
38+class JoinNotAllowed(Exception):
39+ """User is not allowed to join a given team."""
40+ webservice_error(httplib.BAD_REQUEST)
41
42=== modified file 'lib/lp/registry/interfaces/person.py'
43--- lib/lp/registry/interfaces/person.py 2010-10-07 14:03:32 +0000
44+++ lib/lp/registry/interfaces/person.py 2010-10-22 14:12:56 +0000
45@@ -26,7 +26,6 @@
46 'ITeamReassignment',
47 'ImmutableVisibilityError',
48 'InvalidName',
49- 'JoinNotAllowed',
50 'NoSuchPerson',
51 'PersonCreationRationale',
52 'PersonVisibility',
53@@ -2146,10 +2145,6 @@
54 """XMLRPC application root for ISoftwareCenterAgentAPI."""
55
56
57-class JoinNotAllowed(Exception):
58- """User is not allowed to join a given team."""
59-
60-
61 class ImmutableVisibilityError(Exception):
62 """A change in team membership visibility is not allowed."""
63
64
65=== modified file 'lib/lp/registry/model/person.py'
66--- lib/lp/registry/model/person.py 2010-10-07 14:03:32 +0000
67+++ lib/lp/registry/model/person.py 2010-10-22 14:12:56 +0000
68@@ -191,7 +191,10 @@
69 HasMergeProposalsMixin,
70 HasRequestedReviewsMixin,
71 )
72-from lp.registry.errors import NameAlreadyTaken
73+from lp.registry.errors import (
74+ JoinNotAllowed,
75+ NameAlreadyTaken,
76+ )
77 from lp.registry.interfaces.codeofconduct import ISignedCodeOfConductSet
78 from lp.registry.interfaces.distribution import IDistribution
79 from lp.registry.interfaces.gpg import IGPGKeySet
80@@ -217,7 +220,6 @@
81 IPerson,
82 IPersonSet,
83 ITeam,
84- JoinNotAllowed,
85 PersonalStanding,
86 PersonCreationRationale,
87 PersonVisibility,
88
89=== modified file 'lib/lp/registry/tests/test_team_webservice.py'
90--- lib/lp/registry/tests/test_team_webservice.py 2010-10-04 20:46:55 +0000
91+++ lib/lp/registry/tests/test_team_webservice.py 2010-10-22 14:12:56 +0000
92@@ -4,14 +4,20 @@
93 __metaclass__ = type
94
95 import httplib
96-import unittest
97+
98+from zope.security.proxy import removeSecurityProxy
99
100 from lazr.restfulclient.errors import HTTPError
101
102 from canonical.testing.layers import DatabaseFunctionalLayer
103-from lp.registry.interfaces.person import PersonVisibility
104+from lp.registry.interfaces.person import (
105+ PersonVisibility,
106+ TeamSubscriptionPolicy,
107+ )
108 from lp.testing import (
109 launchpadlib_for,
110+ login_person,
111+ logout,
112 TestCaseWithFactory,
113 )
114
115@@ -48,5 +54,43 @@
116 self.assertEqual(httplib.FORBIDDEN, api_error.response.status)
117
118
119-def test_suite():
120- return unittest.TestLoader().loadTestsFromName(__name__)
121+class TestTeamJoining(TestCaseWithFactory):
122+
123+ layer = DatabaseFunctionalLayer
124+
125+ def test_restricted_rejects_membership(self):
126+ # Calling person.join with a team that has a restricted membership
127+ # subscription policy should raise an HTTP error with BAD_REQUEST
128+ self.person = self.factory.makePerson(name='test-person')
129+ self.team = self.factory.makeTeam(name='test-team')
130+ login_person(self.team.teamowner)
131+ self.team.subscriptionpolicy = TeamSubscriptionPolicy.RESTRICTED
132+ logout()
133+
134+ launchpad = launchpadlib_for("test", self.person)
135+ person = launchpad.people['test-person']
136+ api_error = self.assertRaises(
137+ HTTPError,
138+ person.join,
139+ team='test-team')
140+ self.assertEqual(httplib.BAD_REQUEST, api_error.response.status)
141+
142+ def test_open_accepts_membership(self):
143+ # Calling person.join with a team that has an open membership
144+ # subscription policy should add that that user to the team.
145+ self.person = self.factory.makePerson(name='test-person')
146+ self.team = self.factory.makeTeam(name='test-team')
147+ login_person(self.team.teamowner)
148+ self.team.subscriptionpolicy = TeamSubscriptionPolicy.OPEN
149+ logout()
150+
151+ launchpad = launchpadlib_for("test", self.person)
152+ test_person = launchpad.people['test-person']
153+ test_team = launchpad.people['test-team']
154+ test_person.join(team=test_team.self_link)
155+ login_person(self.team.teamowner)
156+ self.assertEqual(
157+ ['test-team'],
158+ [membership.team.name
159+ for membership in self.person.team_memberships])
160+ logout()