launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bad-state-transition-641266 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #641266 AssertionError: Bad state transition from EXPIRED to DEACTIVATED
  https://bugs.launchpad.net/bugs/641266


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; use expose() to push these across the API.

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

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

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/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.
-- 
https://code.launchpad.net/~jcsackett/launchpad/bad-state-transition-641266/+merge/36726
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/bad-state-transition-641266 into lp:launchpad/devel.

=== 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-27 14:39:12 +0000
@@ -0,0 +1,16 @@
+# 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):
+    """comment"""
+    webservice_error(httplib.BAD_REQUEST)

=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/teammembership.py	2010-09-27 14:39:12 +0000
@@ -46,6 +46,8 @@
 from canonical.launchpad.mailnotification import MailWrapper
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.tales import DurationFormatterAPI
+from lazr.restful.error import expose
+from lp.registry.errors import TeamMembershipTransitionError
 from lp.registry.interfaces.person import (
     IPersonSet,
     TeamMembershipRenewalPolicy,
@@ -331,11 +333,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 expose(TeamMembershipTransitionError(
+                "Unknown status: %s" % self.status.name))
+        if status not in state_transition[self.status]:
+            raise expose(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(
@@ -588,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-27 14:39:12 +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__)