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
1=== modified file 'lib/lp/registry/browser/tests/test_person_webservice.py'
2--- lib/lp/registry/browser/tests/test_person_webservice.py 2010-08-20 20:31:18 +0000
3+++ lib/lp/registry/browser/tests/test_person_webservice.py 2010-09-27 14:03:46 +0000
4@@ -14,6 +14,7 @@
5
6
7 class TestPersonRepresentation(TestCaseWithFactory):
8+
9 layer = DatabaseFunctionalLayer
10
11 def setUp(self):
12
13=== modified file 'lib/lp/registry/interfaces/person.py'
14--- lib/lp/registry/interfaces/person.py 2010-09-23 02:33:17 +0000
15+++ lib/lp/registry/interfaces/person.py 2010-09-27 14:03:46 +0000
16@@ -41,6 +41,7 @@
17 'validate_public_person',
18 ]
19
20+import httplib
21
22 from lazr.enum import (
23 DBEnumeratedType,
24@@ -66,6 +67,7 @@
25 REQUEST_USER,
26 webservice_error,
27 )
28+from lazr.restful.error import expose
29 from lazr.restful.fields import (
30 CollectionField,
31 Reference,
32@@ -158,6 +160,10 @@
33
34 class PrivatePersonLinkageError(ValueError):
35 """An attempt was made to link a private person/team to something."""
36+ # HTTP 400 -- BAD REQUEST
37+ # HTTP 403 would be better, but as this excpetion is raised inside a
38+ # validator, it will default to 400 anyway.
39+ webservice_error(httplib.BAD_REQUEST)
40
41
42 @block_implicit_flushes
43@@ -168,15 +174,14 @@
44 assert isinstance(value, (int, long)), (
45 "Expected int for Person foreign key reference, got %r" % type(value))
46
47- # XXX sinzui 2009-04-03 bug=354881: We do not want to import from the
48- # DB. This needs cleaning up.
49+ # Importing here to avoid a cyclic import.
50 from lp.registry.model.person import Person
51 person = Person.get(value)
52 if not validate_func(person):
53- raise PrivatePersonLinkageError(
54+ raise expose(PrivatePersonLinkageError(
55 "Cannot link person (name=%s, visibility=%s) to %s (name=%s)"
56 % (person.name, person.visibility.name,
57- obj, getattr(obj, 'name', None)))
58+ obj, getattr(obj, 'name', None))))
59 return value
60
61
62
63=== modified file 'lib/lp/registry/tests/test_project.py'
64--- lib/lp/registry/tests/test_project.py 2010-09-07 21:29:33 +0000
65+++ lib/lp/registry/tests/test_project.py 2010-09-27 14:03:46 +0000
66@@ -5,20 +5,17 @@
67
68 import unittest
69
70+from lazr.restfulclient.errors import ClientError
71 from zope.component import getUtility
72-from lazr.restfulclient.errors import ClientError
73
74-from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
75 from canonical.launchpad.ftests import login
76 from canonical.launchpad.webapp.errorlog import globalErrorUtility
77 from canonical.testing import (
78+ DatabaseFunctionalLayer,
79 LaunchpadFunctionalLayer,
80- DatabaseFunctionalLayer,
81 )
82 from lp.registry.interfaces.projectgroup import IProjectGroupSet
83-from lp.soyuz.enums import ArchivePurpose
84 from lp.testing import (
85- celebrity_logged_in,
86 launchpadlib_for,
87 TestCaseWithFactory,
88 )
89@@ -111,6 +108,7 @@
90
91
92 class TestLaunchpadlibAPI(TestCaseWithFactory):
93+
94 layer = DatabaseFunctionalLayer
95
96 def test_inappropriate_deactivation_does_not_cause_an_OOPS(self):
97
98=== added file 'lib/lp/registry/tests/test_team_webservice.py'
99--- lib/lp/registry/tests/test_team_webservice.py 1970-01-01 00:00:00 +0000
100+++ lib/lp/registry/tests/test_team_webservice.py 2010-09-27 14:03:46 +0000
101@@ -0,0 +1,51 @@
102+# Copyright 2010 Canonical Ltd. This software is licensed under the
103+# GNU Affero General Public License version 3 (see the file LICENSE).
104+
105+__metaclass__ = type
106+
107+import unittest
108+
109+from lazr.restfulclient.errors import HTTPError
110+
111+from canonical.testing import DatabaseFunctionalLayer
112+from lp.registry.interfaces.person import PersonVisibility
113+from lp.testing import (
114+ launchpadlib_for,
115+ TestCaseWithFactory,
116+ )
117+
118+
119+class TestTeamLinking(TestCaseWithFactory):
120+
121+ layer = DatabaseFunctionalLayer
122+
123+ def setUp(self):
124+ super(TestTeamLinking, self).setUp()
125+ self.team_owner = self.factory.makePerson(name='team-owner')
126+ self.private_team_one = self.factory.makeTeam(
127+ owner=self.team_owner,
128+ name='private-team',
129+ displayname='Private Team',
130+ visibility=PersonVisibility.PRIVATE)
131+ self.private_team_two = self.factory.makeTeam(
132+ owner=self.team_owner,
133+ name='private-team-two',
134+ displayname='Private Team Two',
135+ visibility=PersonVisibility.PRIVATE)
136+
137+ def test_private_links(self):
138+ # A private team cannot be linked to another team, private or
139+ # or otherwise.
140+ launchpad = launchpadlib_for("test", self.team_owner.name)
141+ team_one = launchpad.people['private-team']
142+ team_two = launchpad.people['private-team-two']
143+ api_error = self.assertRaises(
144+ HTTPError,
145+ team_one.addMember,
146+ person=team_two)
147+ self.assertIn('Cannot link person', api_error.content)
148+ self.assertEqual(400, api_error.response.status)
149+
150+
151+def test_suite():
152+ return unittest.TestLoader().loadTestsFromName(__name__)