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

[j.c.sackett]

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

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.

[Aaron Bentley (abentley)]

As discussed, expose should not be necessary.

[j.c.sackett (jcsackett)]

> 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.

[Graham Binns (gmb)]

> === 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)

=== 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-29 21:54:47 +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-29 21:54:47 +0000
@@ -0,0 +1,20 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+__all__ = [
+    'TeamMembershipTransitionError',
+    ]
+
+import httplib
+
+from lazr.restful.declarations import webservice_error
+
+
+class TeamMembershipTransitionError(ValueError):
+    """Indicates something has gone wrong with the transtiion.
+
+    Generally, this indicates a bad transition (e.g. approved to proposed)
+    or an invalid transition (e.g. unicorn).
+    """
+    webservice_error(httplib.BAD_REQUEST)

=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py	2010-09-27 20:47:58 +0000
+++ lib/lp/registry/model/teammembership.py	2010-09-29 21:54:47 +0000
@@ -46,6 +46,7 @@
 from canonical.launchpad.mailnotification import MailWrapper
 from canonical.launchpad.webapp import canonical_url
 from lp.app.browser.tales import DurationFormatterAPI
+from lp.registry.errors import TeamMembershipTransitionError
 from lp.registry.interfaces.person import (
     IPersonSet,
     TeamMembershipRenewalPolicy,
@@ -331,11 +332,14 @@
             declined: [proposed, approved, admin],
             invited: [approved, admin, invitation_declined],
             invitation_declined: [invited, approved, admin]}
-        assert self.status in state_transition, (
-            "Unknown status: %s" % self.status.name)
-        assert status in state_transition[self.status], (
-            "Bad state transition from %s to %s"
-            % (self.status.name, status.name))
+
+        if self.status not in state_transition:
+            raise TeamMembershipTransitionError(
+                "Unknown status: %s" % self.status.name)
+        if status not in state_transition[self.status]:
+            raise TeamMembershipTransitionError(
+                "Bad state transition from %s to %s"
+                % (self.status.name, status.name))
 
         if status in ACTIVE_STATES and self.team in self.person.allmembers:
             raise CyclicalTeamMembershipError(
@@ -589,7 +593,8 @@
             SELECT 1 FROM TeamParticipation
             WHERE person = %(person_id)s AND team IN (
                     SELECT person
-                    FROM TeamParticipation JOIN Person ON (person = Person.id)
+                    FROM TeamParticipation JOIN Person ON
+                        (person = Person.id)
                     WHERE team = %(team_id)s
                         AND person NOT IN (%(team_id)s, %(person_id)s)
                         AND teamowner IS NOT NULL

=== added file 'lib/lp/registry/tests/test_teammembership_webservice.py'
--- lib/lp/registry/tests/test_teammembership_webservice.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_teammembership_webservice.py	2010-09-29 21:54:47 +0000
@@ -0,0 +1,68 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+
+from zope.component import getUtility
+
+from canonical.testing import DatabaseFunctionalLayer
+from lazr.restfulclient.errors import HTTPError
+from lp.registry.interfaces.teammembership import (
+    ITeamMembershipSet,
+    TeamMembershipStatus,
+    )
+from lp.testing import (
+    TestCaseWithFactory,
+    launchpadlib_for,
+    )
+
+
+class TestTeamMembershipTransitions(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestTeamMembershipTransitions, self).setUp()
+        self.person = self.factory.makePerson(name='some-person')
+        owner = self.factory.makePerson()
+        self.team = self.factory.makeTeam(
+            name='some-team',
+            owner=owner)
+        membership_set = getUtility(ITeamMembershipSet)
+        membership = membership_set.new(
+            self.person,
+            self.team,
+            TeamMembershipStatus.APPROVED,
+            self.person)
+        self.launchpad = launchpadlib_for("test", owner.name)
+
+    def test_no_such_status(self):
+        # An error should be thrown when transitioning to a status that
+        # doesn't exist.
+        team = self.launchpad.people['some-team']
+        team_membership = team.members_details[1]
+        # The error in this instance should be a valueerror, b/c the
+        # WADL used by launchpadlib will enforce the method args.
+        api_exception = self.assertRaises(
+            ValueError,
+            team_membership.setStatus,
+            status='NOTVALIDSTATUS')
+
+    def test_invalid_transition(self):
+        # An error should be thrown when transitioning to a status that
+        # isn't a valid move.
+        team = self.launchpad.people['some-team']
+        team_membership = team.members_details[1]
+        # The error used here should be an HTTPError, since it is being
+        # passed back by the server across the API.
+        api_exception = self.assertRaises(
+            HTTPError,
+            team_membership.setStatus,
+            status='Proposed')
+        self.assertEqual(400, api_exception.response.status)
+
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)