Launchpad itself

Merge lp:~jcsackett/launchpad/bad-state-transition-641266 into lp:launchpad

  1. bad-state-transition-641266
  2. Merge into devel
Proposed by j.c.sackett
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 11660
Proposed branch: lp:~jcsackett/launchpad/bad-state-transition-641266
Merge into: lp:launchpad
Diff against target: 151 lines (+100/-6)
4 files modified
lib/canonical/launchpad/interfaces/__init__.py (+1/-0)
lib/lp/registry/errors.py (+20/-0)
lib/lp/registry/model/teammembership.py (+11/-6)
lib/lp/registry/tests/test_teammembership_webservice.py (+68/-0)
To merge this branch: bzr merge lp:~jcsackett/launchpad/bad-state-transition-641266
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+36726@code.launchpad.net

Commit message

Restructure stacked assertions into exceptions based on conditionals to provide appropriate errors to API users instead of OOPSing.

Description of the change

Summary
=======

Exposes an API error and refactors some assertions to more cleanly deal with an error condition across the API instead of OOPSing.

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

Rather than the stacked asserts in the setStatus method, implement a conditional tree to check each condition and raise a new Error class related to bad transitions that has been registered with the webservice.

Pre-Implementation Talk
=======================

Spoke with Curtis Hovey about the error conditions and API usage.

Spoke further with Aaron Bentley about appropriate exposure of Exceptions across the API.

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

As in proposed.

Tests
=====

bin/test -t test_teammembership_webservice

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

Use launchpadlib to connect launchpad.dev. Either create or find a team and retrieve one of its teammembership entries. Attempt to setStatus to one that isn't valid for the current (e.g. switch to Proposed from Approved).

Lint
====
make lint output:

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/launchpad/interfaces/__init__.py
  lib/lp/registry/errors.py
  lib/lp/registry/model/teammembership.py
  lib/lp/registry/tests/test_teammembership_webservice.py

./lib/lp/registry/model/teammembership.py
     569: Line exceeds 78 characters.

I can see no clear way to shorten the line on 569; as it predates this branch, I've elected to leave it alone, but welcome suggestions to clean it up.

There are a horde of errors in canonical.launchpad.interface.__init__; it was touched as part of getting errors registered with webservice. Another branch is in the works to remove registry items (or at least errors) from this file and into lp.registry.

To post a comment you must log in.
Revision history for this message
Aaron Bentley (abentley) wrote :

As discussed, expose should not be necessary.

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

> As discussed, expose should not be necessary.

Expose has been removed. It did require adding a line to the mess that is canonical.launchpad.interfaces.__init__; I have another branch in the works to move all registry error related matters out of that module, but it's out of scope for this bug fix.

Revision history for this message
Graham Binns (gmb) wrote :

> === modified file 'lib/canonical/launchpad/interfaces/__init__.py'
> --- lib/canonical/launchpad/interfaces/__init__.py 2010-09-11 19:25:13 +0000
> +++ lib/canonical/launchpad/interfaces/__init__.py 2010-09-28 15:23:43 +0000
> @@ -96,6 +96,7 @@
> from canonical.launchpad.interfaces.packagerelationship import *
> from canonical.launchpad.interfaces.pathlookup import *
> from lp.registry.interfaces.poll import *
> +from lp.registry.errors import TeamMembershipTransitionError
> from lp.soyuz.interfaces.processor import *
> from lp.registry.interfaces.product import *
> from lp.registry.interfaces.productlicense import *
> === added file 'lib/lp/registry/errors.py'
> --- lib/lp/registry/errors.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/registry/errors.py 2010-09-28 15:23:43 +0000
> @@ -0,0 +1,20 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the

It's 2010. If you want to update standard_template and
standard_test_template to be correct (for the next three months anyway)
then that's fine with me.

Other than that, r=me.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/interfaces/__init__.py'
2--- lib/canonical/launchpad/interfaces/__init__.py 2010-09-11 19:25:13 +0000
3+++ lib/canonical/launchpad/interfaces/__init__.py 2010-09-29 21:54:47 +0000
4@@ -96,6 +96,7 @@
5 from canonical.launchpad.interfaces.packagerelationship import *
6 from canonical.launchpad.interfaces.pathlookup import *
7 from lp.registry.interfaces.poll import *
8+from lp.registry.errors import TeamMembershipTransitionError
9 from lp.soyuz.interfaces.processor import *
10 from lp.registry.interfaces.product import *
11 from lp.registry.interfaces.productlicense import *
12
13=== added file 'lib/lp/registry/errors.py'
14--- lib/lp/registry/errors.py 1970-01-01 00:00:00 +0000
15+++ lib/lp/registry/errors.py 2010-09-29 21:54:47 +0000
16@@ -0,0 +1,20 @@
17+# Copyright 2009 Canonical Ltd. This software is licensed under the
18+# GNU Affero General Public License version 3 (see the file LICENSE).
19+
20+__metaclass__ = type
21+__all__ = [
22+ 'TeamMembershipTransitionError',
23+ ]
24+
25+import httplib
26+
27+from lazr.restful.declarations import webservice_error
28+
29+
30+class TeamMembershipTransitionError(ValueError):
31+ """Indicates something has gone wrong with the transtiion.
32+
33+ Generally, this indicates a bad transition (e.g. approved to proposed)
34+ or an invalid transition (e.g. unicorn).
35+ """
36+ webservice_error(httplib.BAD_REQUEST)
37
38=== modified file 'lib/lp/registry/model/teammembership.py'
39--- lib/lp/registry/model/teammembership.py 2010-09-27 20:47:58 +0000
40+++ lib/lp/registry/model/teammembership.py 2010-09-29 21:54:47 +0000
41@@ -46,6 +46,7 @@
42 from canonical.launchpad.mailnotification import MailWrapper
43 from canonical.launchpad.webapp import canonical_url
44 from lp.app.browser.tales import DurationFormatterAPI
45+from lp.registry.errors import TeamMembershipTransitionError
46 from lp.registry.interfaces.person import (
47 IPersonSet,
48 TeamMembershipRenewalPolicy,
49@@ -331,11 +332,14 @@
50 declined: [proposed, approved, admin],
51 invited: [approved, admin, invitation_declined],
52 invitation_declined: [invited, approved, admin]}
53- assert self.status in state_transition, (
54- "Unknown status: %s" % self.status.name)
55- assert status in state_transition[self.status], (
56- "Bad state transition from %s to %s"
57- % (self.status.name, status.name))
58+
59+ if self.status not in state_transition:
60+ raise TeamMembershipTransitionError(
61+ "Unknown status: %s" % self.status.name)
62+ if status not in state_transition[self.status]:
63+ raise TeamMembershipTransitionError(
64+ "Bad state transition from %s to %s"
65+ % (self.status.name, status.name))
66
67 if status in ACTIVE_STATES and self.team in self.person.allmembers:
68 raise CyclicalTeamMembershipError(
69@@ -589,7 +593,8 @@
70 SELECT 1 FROM TeamParticipation
71 WHERE person = %(person_id)s AND team IN (
72 SELECT person
73- FROM TeamParticipation JOIN Person ON (person = Person.id)
74+ FROM TeamParticipation JOIN Person ON
75+ (person = Person.id)
76 WHERE team = %(team_id)s
77 AND person NOT IN (%(team_id)s, %(person_id)s)
78 AND teamowner IS NOT NULL
79
80=== added file 'lib/lp/registry/tests/test_teammembership_webservice.py'
81--- lib/lp/registry/tests/test_teammembership_webservice.py 1970-01-01 00:00:00 +0000
82+++ lib/lp/registry/tests/test_teammembership_webservice.py 2010-09-29 21:54:47 +0000
83@@ -0,0 +1,68 @@
84+# Copyright 2010 Canonical Ltd. This software is licensed under the
85+# GNU Affero General Public License version 3 (see the file LICENSE).
86+
87+__metaclass__ = type
88+
89+import unittest
90+
91+from zope.component import getUtility
92+
93+from canonical.testing import DatabaseFunctionalLayer
94+from lazr.restfulclient.errors import HTTPError
95+from lp.registry.interfaces.teammembership import (
96+ ITeamMembershipSet,
97+ TeamMembershipStatus,
98+ )
99+from lp.testing import (
100+ TestCaseWithFactory,
101+ launchpadlib_for,
102+ )
103+
104+
105+class TestTeamMembershipTransitions(TestCaseWithFactory):
106+
107+ layer = DatabaseFunctionalLayer
108+
109+ def setUp(self):
110+ super(TestTeamMembershipTransitions, self).setUp()
111+ self.person = self.factory.makePerson(name='some-person')
112+ owner = self.factory.makePerson()
113+ self.team = self.factory.makeTeam(
114+ name='some-team',
115+ owner=owner)
116+ membership_set = getUtility(ITeamMembershipSet)
117+ membership = membership_set.new(
118+ self.person,
119+ self.team,
120+ TeamMembershipStatus.APPROVED,
121+ self.person)
122+ self.launchpad = launchpadlib_for("test", owner.name)
123+
124+ def test_no_such_status(self):
125+ # An error should be thrown when transitioning to a status that
126+ # doesn't exist.
127+ team = self.launchpad.people['some-team']
128+ team_membership = team.members_details[1]
129+ # The error in this instance should be a valueerror, b/c the
130+ # WADL used by launchpadlib will enforce the method args.
131+ api_exception = self.assertRaises(
132+ ValueError,
133+ team_membership.setStatus,
134+ status='NOTVALIDSTATUS')
135+
136+ def test_invalid_transition(self):
137+ # An error should be thrown when transitioning to a status that
138+ # isn't a valid move.
139+ team = self.launchpad.people['some-team']
140+ team_membership = team.members_details[1]
141+ # The error used here should be an HTTPError, since it is being
142+ # passed back by the server across the API.
143+ api_exception = self.assertRaises(
144+ HTTPError,
145+ team_membership.setStatus,
146+ status='Proposed')
147+ self.assertEqual(400, api_exception.response.status)
148+
149+
150+def test_suite():
151+ return unittest.TestLoader().loadTestsFromName(__name__)