Launchpad itself

Merge lp:~jcsackett/launchpad/private-team-to-private-team-519306 into lp:launchpad

  1. private-team-to-private-team-519306
  2. Merge into devel
Proposed by j.c.sackett
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: 11639
Proposed branch: lp:~jcsackett/launchpad/private-team-to-private-team-519306
Merge into: lp:launchpad
Diff against target: 152 lines (+64/-9)
4 files modified
lib/lp/registry/browser/tests/test_person_webservice.py (+1/-0)
lib/lp/registry/interfaces/person.py (+9/-4)
lib/lp/registry/tests/test_project.py (+3/-5)
lib/lp/registry/tests/test_team_webservice.py (+51/-0)
To merge this branch: bzr merge lp:~jcsackett/launchpad/private-team-to-private-team-519306
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+36589@code.launchpad.net

Commit message

Uses expose() on an exception in team validator so that the exception is properly passed through the API, rather than OOPSing.

Description of the change

Summary
=======

Exposes an exception across the API when a bad attempt is made to link private teams. This prevents the OOPS from the related bug.

Proposed Fix
============

Wrap the exception normally returned by the validator for the website in the expose method and tie it to a 403 error.

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

Largely as in proposed fix. Instead of 403, it returns a 400, as webservice_error reverts to that if the exception is called from deeper in the call stack than the API named method. As this occurs in the validator, it will always be a 400 so the code reflects that.

Tests
=====

bin/test -t test_team_webservice

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

Use launchpadlib to connect launchpad.dev; try to add one private team as a member to another (as the owner of both teams). You will get an HTTP 400 response.

Lint
====
make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/tests/test_project.py
  lib/lp/registry/tests/test_team_webservice.py

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

Looks good Jon with these comments:

1) use httplib.BAD_REQUEST instead of a raw 400 throughout your changes. Please leave the comment that shows the raw number.

2) run utilities/format-imports on your changed files. Easier than me pointing out the issues.

3) 'e' is a pretty recognized variable for an exception but we still try to avoid one letter variables, save for i,j,k loop variables.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
--- lib/lp/registry/browser/tests/test_person_webservice.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/tests/test_person_webservice.py 2010-09-27 14:03:46 +0000
@@ -14,6 +14,7 @@
1414
1515
16class TestPersonRepresentation(TestCaseWithFactory):16class TestPersonRepresentation(TestCaseWithFactory):
17
17 layer = DatabaseFunctionalLayer18 layer = DatabaseFunctionalLayer
1819
19 def setUp(self):20 def setUp(self):
2021
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-09-23 02:33:17 +0000
+++ lib/lp/registry/interfaces/person.py 2010-09-27 14:03:46 +0000
@@ -41,6 +41,7 @@
41 'validate_public_person',41 'validate_public_person',
42 ]42 ]
4343
44import httplib
4445
45from lazr.enum import (46from lazr.enum import (
46 DBEnumeratedType,47 DBEnumeratedType,
@@ -66,6 +67,7 @@
66 REQUEST_USER,67 REQUEST_USER,
67 webservice_error,68 webservice_error,
68 )69 )
70from lazr.restful.error import expose
69from lazr.restful.fields import (71from lazr.restful.fields import (
70 CollectionField,72 CollectionField,
71 Reference,73 Reference,
@@ -158,6 +160,10 @@
158160
159class PrivatePersonLinkageError(ValueError):161class PrivatePersonLinkageError(ValueError):
160 """An attempt was made to link a private person/team to something."""162 """An attempt was made to link a private person/team to something."""
163 # HTTP 400 -- BAD REQUEST
164 # HTTP 403 would be better, but as this excpetion is raised inside a
165 # validator, it will default to 400 anyway.
166 webservice_error(httplib.BAD_REQUEST)
161167
162168
163@block_implicit_flushes169@block_implicit_flushes
@@ -168,15 +174,14 @@
168 assert isinstance(value, (int, long)), (174 assert isinstance(value, (int, long)), (
169 "Expected int for Person foreign key reference, got %r" % type(value))175 "Expected int for Person foreign key reference, got %r" % type(value))
170176
171 # XXX sinzui 2009-04-03 bug=354881: We do not want to import from the177 # Importing here to avoid a cyclic import.
172 # DB. This needs cleaning up.
173 from lp.registry.model.person import Person178 from lp.registry.model.person import Person
174 person = Person.get(value)179 person = Person.get(value)
175 if not validate_func(person):180 if not validate_func(person):
176 raise PrivatePersonLinkageError(181 raise expose(PrivatePersonLinkageError(
177 "Cannot link person (name=%s, visibility=%s) to %s (name=%s)"182 "Cannot link person (name=%s, visibility=%s) to %s (name=%s)"
178 % (person.name, person.visibility.name,183 % (person.name, person.visibility.name,
179 obj, getattr(obj, 'name', None)))184 obj, getattr(obj, 'name', None))))
180 return value185 return value
181186
182187
183188
=== modified file 'lib/lp/registry/tests/test_project.py'
--- lib/lp/registry/tests/test_project.py 2010-09-07 21:29:33 +0000
+++ lib/lp/registry/tests/test_project.py 2010-09-27 14:03:46 +0000
@@ -5,20 +5,17 @@
55
6import unittest6import unittest
77
8from lazr.restfulclient.errors import ClientError
8from zope.component import getUtility9from zope.component import getUtility
9from lazr.restfulclient.errors import ClientError
1010
11from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
12from canonical.launchpad.ftests import login11from canonical.launchpad.ftests import login
13from canonical.launchpad.webapp.errorlog import globalErrorUtility12from canonical.launchpad.webapp.errorlog import globalErrorUtility
14from canonical.testing import (13from canonical.testing import (
14 DatabaseFunctionalLayer,
15 LaunchpadFunctionalLayer,15 LaunchpadFunctionalLayer,
16 DatabaseFunctionalLayer,
17 )16 )
18from lp.registry.interfaces.projectgroup import IProjectGroupSet17from lp.registry.interfaces.projectgroup import IProjectGroupSet
19from lp.soyuz.enums import ArchivePurpose
20from lp.testing import (18from lp.testing import (
21 celebrity_logged_in,
22 launchpadlib_for,19 launchpadlib_for,
23 TestCaseWithFactory,20 TestCaseWithFactory,
24 )21 )
@@ -111,6 +108,7 @@
111108
112109
113class TestLaunchpadlibAPI(TestCaseWithFactory):110class TestLaunchpadlibAPI(TestCaseWithFactory):
111
114 layer = DatabaseFunctionalLayer112 layer = DatabaseFunctionalLayer
115113
116 def test_inappropriate_deactivation_does_not_cause_an_OOPS(self):114 def test_inappropriate_deactivation_does_not_cause_an_OOPS(self):
117115
=== added file 'lib/lp/registry/tests/test_team_webservice.py'
--- lib/lp/registry/tests/test_team_webservice.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_team_webservice.py 2010-09-27 14:03:46 +0000
@@ -0,0 +1,51 @@
1# Copyright 2010 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4__metaclass__ = type
5
6import unittest
7
8from lazr.restfulclient.errors import HTTPError
9
10from canonical.testing import DatabaseFunctionalLayer
11from lp.registry.interfaces.person import PersonVisibility
12from lp.testing import (
13 launchpadlib_for,
14 TestCaseWithFactory,
15 )
16
17
18class TestTeamLinking(TestCaseWithFactory):
19
20 layer = DatabaseFunctionalLayer
21
22 def setUp(self):
23 super(TestTeamLinking, self).setUp()
24 self.team_owner = self.factory.makePerson(name='team-owner')
25 self.private_team_one = self.factory.makeTeam(
26 owner=self.team_owner,
27 name='private-team',
28 displayname='Private Team',
29 visibility=PersonVisibility.PRIVATE)
30 self.private_team_two = self.factory.makeTeam(
31 owner=self.team_owner,
32 name='private-team-two',
33 displayname='Private Team Two',
34 visibility=PersonVisibility.PRIVATE)
35
36 def test_private_links(self):
37 # A private team cannot be linked to another team, private or
38 # or otherwise.
39 launchpad = launchpadlib_for("test", self.team_owner.name)
40 team_one = launchpad.people['private-team']
41 team_two = launchpad.people['private-team-two']
42 api_error = self.assertRaises(
43 HTTPError,
44 team_one.addMember,
45 person=team_two)
46 self.assertIn('Cannot link person', api_error.content)
47 self.assertEqual(400, api_error.response.status)
48
49
50def test_suite():
51 return unittest.TestLoader().loadTestsFromName(__name__)