Merge lp:~benji/launchpad/bug-413174 into lp:launchpad

[Benji York]

Often times functions have preconditions that must be met for the
functions to operate correctly. When users interact with those
functions via the web interface the precondition checks are often done
by view code. When called via the web service those functions instead
generate exceptions when called without the preconditions being met.

Previously those exceptions would be recorded as an OOPS and propagated
to the client as HTTP 500 errors with unhelpful response bodies.

This branch fixes bug 413174 by taking advantage of the
lazr.restful.error.expose function that signals lazr.restful to generate
an HTTP 400 error signaling a client error and causes the web service to
generate a response with the error message from the raised exception.
This also means that these client-induced errors will no longer generate
an OOPS.

In the future similar OOPSes that arrise can be given the same
treatment.

The tests added to verify the nice-error-message, non-OOPS producing
effects of this branch are in the TestLaunchpadlibAPI test cases in
lib/lp/registry/tests/test_project.py and TestDuplicateProductReleases
in lib/lp/registry/tests/test_project_milestone.py and can be run with

    bin/test -t TestLaunchpadlibAPI -t TestDuplicateProductReleases

A small bit of whitespace lint was cleaned up in this branch.

[Leonard Richardson (leonardr)]

Refactor get_last_oops_id and (as recommended by james_w) use assertNoNewOops. Looks good otherwise.

review: Approve

[Benji York (benji)]

> Refactor get_last_oops_id and (as recommended by james_w) use
> assertNoNewOops. Looks good otherwise.

Done.

Thanks for the review.

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/milestone.py	2010-09-07 21:32:48 +0000
@@ -36,6 +36,7 @@
     sqlvalues,
     )
 from canonical.launchpad.webapp.sorting import expand_numbers
+from lazr.restful.error import expose
 from lp.app.errors import NotFoundError
 from lp.blueprints.model.specification import Specification
 from lp.bugs.interfaces.bugtarget import IHasBugs
@@ -109,6 +110,13 @@
         return sorted(result, key=milestone_sort_key, reverse=True)
 
 
+class MultipleProductReleases(Exception):
+    """Raised when a second ProductRelease is created for a milestone."""
+
+    def __init__(self, msg='A milestone can only have one ProductRelease.'):
+        super(MultipleProductReleases, self).__init__(msg)
+
+
 class Milestone(SQLBase, StructuralSubscriptionTargetMixin, HasBugsBase):
     implements(IHasBugs, IMilestone)
 
@@ -194,8 +202,7 @@
                              changelog=None, release_notes=None):
         """See `IMilestone`."""
         if self.product_release is not None:
-            raise AssertionError(
-                'A milestone can only have one ProductRelease.')
+            raise expose(MultipleProductReleases())
         release = ProductRelease(
             owner=owner,
             changelog=changelog,
 
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2010-09-03 11:05:21 +0000
+++ lib/lp/registry/model/product.py	2010-09-07 21:32:48 +0000
@@ -17,6 +17,7 @@
 import operator
 
 from lazr.delegates import delegates
+from lazr.restful.error import expose
 import pytz
 from sqlobject import (
     BoolCol,
@@ -271,6 +272,13 @@
                 tables=[ProductLicense]))
 
 
+class UnDeactivateable(Exception):
+    """Raised when a project is requested to deactivate but can not."""
+
+    def __init__(self, msg):
+        super(UnDeactivateable, self).__init__(msg)
+
+
 class Product(SQLBase, BugTargetBase, MakesAnnouncements,
               HasSpecificationsMixin, HasSprintsMixin,
               KarmaContextMixin, BranchVisibilityPolicyMixin,
@@ -440,9 +448,9 @@
         # Validate deactivation.
         if self.active == True and value == False:
             if len(self.sourcepackages) > 0:
-                raise AssertionError(
+                raise expose(UnDeactivateable(
                     'This project cannot be deactivated since it is '
-                    'linked to source packages.')
+                    'linked to source packages.'))
         return value
 
     active = BoolCol(dbName='active', notNull=True, default=True,
 
=== modified file 'lib/lp/registry/stories/webservice/xx-project-registry.txt'
--- lib/lp/registry/stories/webservice/xx-project-registry.txt	2010-07-20 17:50:45 +0000
+++ lib/lp/registry/stories/webservice/xx-project-registry.txt	2010-09-07 21:32:48 +0000
@@ -1016,9 +1016,9 @@
     ...     date_released='2000-01-01T01:01:01+00:00Z',
     ...     changelog='Added 5,000 features.')
     >>> print response
-    HTTP/1.1 500 Internal Server Error
+    HTTP/1.1 400 Bad Request
     ...
-    AssertionError: A milestone can only have one ProductRelease.
+    MultipleProductReleases: A milestone can only have one ProductRelease.
 
 
 Project release entries
 
=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_milestone.py	2010-09-07 21:32:48 +0000
@@ -15,6 +15,7 @@
     logout,
     )
 from canonical.testing import LaunchpadFunctionalLayer
+
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.milestone import IMilestoneSet
@@ -79,6 +80,7 @@
             all_visible_milestones_ids,
             [1, 2, 3])
 
+
 def test_suite():
     """Return the test suite for the tests in this module."""
     return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2010-08-25 20:04:40 +0000
+++ lib/lp/registry/tests/test_product.py	2010-09-07 21:32:48 +0000
@@ -28,7 +28,7 @@
     License,
     )
 from lp.registry.model.commercialsubscription import CommercialSubscription
-from lp.registry.model.product import Product
+from lp.registry.model.product import Product, UnDeactivateable
 from lp.registry.model.productlicense import ProductLicense
 from lp.testing import TestCaseWithFactory
 
@@ -48,7 +48,7 @@
         source_package.setPackaging(
             product.development_focus, self.factory.makePerson())
         self.assertRaises(
-            AssertionError,
+            UnDeactivateable,
             setattr, product, 'active', False)
 
     def test_deactivation_success(self):
 
=== modified file 'lib/lp/registry/tests/test_project.py'
--- lib/lp/registry/tests/test_project.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_project.py	2010-09-07 21:32:48 +0000
@@ -6,11 +6,22 @@
 import unittest
 
 from zope.component import getUtility
+from lazr.restfulclient.errors import ClientError
 
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
 from canonical.launchpad.ftests import login
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.launchpad.webapp.errorlog import globalErrorUtility
+from canonical.testing import (
+    LaunchpadFunctionalLayer,
+    DatabaseFunctionalLayer,
+    )
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
-from lp.testing import TestCaseWithFactory
+from lp.soyuz.enums import ArchivePurpose
+from lp.testing import (
+    celebrity_logged_in,
+    launchpadlib_for,
+    TestCaseWithFactory,
+    )
 
 
 class ProjectGroupSearchTestCase(TestCaseWithFactory):
@@ -99,5 +110,27 @@
         self.assertEqual(self.project3, results[0])
 
 
+class TestLaunchpadlibAPI(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def test_inappropriate_deactivation_does_not_cause_an_OOPS(self):
+        # Make sure a 400 error and not an OOPS is returned when a ValueError
+        # is raised when trying to deactivate a project that has source
+        # releases.
+        last_oops = globalErrorUtility.getLastOopsReport()
+        launchpad = launchpadlib_for("test", "salgado", "WRITE_PUBLIC")
+
+        project = launchpad.projects['evolution']
+        project.active = False
+        e = self.assertRaises(ClientError, project.lp_save)
+
+        # no OOPS was generated as a result of the exception
+        self.assertNoNewOops(last_oops)
+        self.assertEqual(400, e.response.status)
+        self.assertIn(
+            'This project cannot be deactivated since it is linked to source '
+            'packages.', e.content)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'lib/lp/registry/tests/test_project_milestone.py'
--- lib/lp/registry/tests/test_project_milestone.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_project_milestone.py	2010-09-07 21:32:48 +0000
@@ -10,12 +10,19 @@
 
 from storm.store import Store
 from zope.component import getUtility
+import pytz
 
 from canonical.launchpad.ftests import (
     login,
     syncUpdate,
     )
-from canonical.testing import LaunchpadFunctionalLayer
+from canonical.launchpad.webapp.errorlog import globalErrorUtility
+from canonical.testing import (
+    LaunchpadFunctionalLayer,
+    DatabaseFunctionalLayer,
+    )
+from lazr.restfulclient.errors import ClientError
+
 from lp.blueprints.interfaces.specification import (
     ISpecificationSet,
     SpecificationDefinitionStatus,
@@ -30,6 +37,11 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.registry.interfaces.projectgroup import IProjectGroupSet
+from lp.registry.model.milestone import MultipleProductReleases
+from lp.testing import (
+    launchpadlib_for,
+    TestCaseWithFactory,
+    )
 
 
 class ProjectMilestoneTest(unittest.TestCase):
@@ -197,7 +209,7 @@
         spec = specset.new(
             name='%s-specification' % product_name,
             title='Title %s specification' % product_name,
-            specurl='http://www.example.com/spec/%s' %product_name ,
+            specurl='http://www.example.com/spec/%s' % product_name,
             summary='summary',
             definition_status=SpecificationDefinitionStatus.APPROVED,
             priority=SpecificationPriority.HIGH,
@@ -317,6 +329,48 @@
         self._createProductSeriesBugtask('evolution', 'trunk', '1.1')
 
 
+class TestDuplicateProductReleases(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def test_inappropriate_release_raises(self):
+        # A milestone that already has a ProductRelease can not be given
+        # another one.
+        login('[e-mail]')
+        product_set = getUtility(IProductSet)
+        product = product_set['evolution']
+        series = product.getSeries('trunk')
+        milestone = series.newMilestone(name='1.1', dateexpected=None)
+        now = datetime.now(pytz.UTC)
+        milestone.createProductRelease(1, now)
+        self.assertRaises(MultipleProductReleases,
+            milestone.createProductRelease, 1, now)
+        try:
+            milestone.createProductRelease(1, now)
+        except MultipleProductReleases, e:
+            self.assert_(
+                str(e), 'A milestone can only have one ProductRelease.')
+
+    def test_inappropriate_deactivation_does_not_cause_an_OOPS(self):
+        # Make sure a 400 error and not an OOPS is returned when an exception
+        # is raised when trying to create a product release when a milestone
+        # already has one.
+        last_oops = globalErrorUtility.getLastOopsReport()
+        launchpad = launchpadlib_for("test", "salgado", "WRITE_PUBLIC")
+
+        project = launchpad.projects['evolution']
+        milestone = project.getMilestone(name='2.1.6')
+        now = datetime.now(pytz.UTC)
+
+        e = self.assertRaises(
+            ClientError, milestone.createProductRelease, date_released=now)
+
+        # no OOPS was generated as a result of the exception
+        self.assertNoNewOops(last_oops)
+        self.assertEqual(400, e.response.status)
+        self.assertIn(
+            'A milestone can only have one ProductRelease.', e.content)
+
+
 def test_suite():
     """Return the test suite for the tests in this module."""
     return unittest.TestLoader().loadTestsFromName(__name__)
 
=== modified file 'versions.cfg'
--- versions.cfg	2010-09-06 18:23:01 +0000
+++ versions.cfg	2010-09-07 21:32:48 +0000
@@ -31,7 +31,7 @@
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.2
 lazr.lifecycle = 1.1
-lazr.restful = 0.11.3
+lazr.restful = 0.13.0
 lazr.restfulclient = 0.10.0
 lazr.smtptest = 1.1
 lazr.testing = 0.1.1