launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/launchpad/checkUpload-bug-608891 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #608891 archive.checkUpload() OOPSes when distoseries is set to a stable release
  https://bugs.launchpad.net/bugs/608891


= Summary =
Fix bug 608891, prevent an OOPS when there should not be one.

== Proposed fix ==
There's a webservice_error() missing from a the CannotUploadToPocket exception 
class, so when it gets raised it generates an OOPS instead of returning an 
error to the client.

== Tests ==
I added a new unit test file
bin/test -cvv test_archive_webservice


= Launchpad lint =

The lint checker blows on interface files with webservice exports.  Ignore 
this.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/soyuz/interfaces/archive.py
  lib/lp/soyuz/browser/tests/test_archive_webservice.py

./lib/lp/soyuz/interfaces/archive.py
     528: E301 expected 1 blank line, found 0
     545: E202 whitespace before ')'
     570: E501 line too long (81 characters)
     572: E501 line too long (80 characters)
     577: E501 line too long (81 characters)
     580: E501 line too long (80 characters)
     655: E301 expected 1 blank line, found 0
     678: E301 expected 1 blank line, found 0
     701: E301 expected 1 blank line, found 0
     801: E301 expected 1 blank line, found 0
    1138: E302 expected 2 blank lines, found 1
    1435: E301 expected 1 blank line, found 2
    1664: E303 too many blank lines (3)
     521: Line exceeds 78 characters.
     570: Line exceeds 78 characters.
     572: Line exceeds 78 characters.
     577: Line exceeds 78 characters.
     580: Line exceeds 78 characters.
    1449: Line exceeds 78 characters.
-- 
https://code.launchpad.net/~julian-edwards/launchpad/checkUpload-bug-608891/+merge/31041
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/checkUpload-bug-608891 into lp:launchpad/devel.

=== added file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
--- lib/lp/soyuz/browser/tests/test_archive_webservice.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py	2010-07-27 12:46:05 +0000
@@ -0,0 +1,68 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+__metaclass__ = type
+
+import unittest
+from lazr.restfulclient.errors import HTTPError
+
+from lp.testing import (
+    celebrity_logged_in, launchpadlib_for, TestCaseWithFactory)
+from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
+from canonical.testing import DatabaseFunctionalLayer
+from lp.soyuz.interfaces.archive import ArchivePurpose
+
+
+class TestArchiveWebservice(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        TestCaseWithFactory.setUp(self)
+        with celebrity_logged_in('admin'):
+            archive = self.factory.makeArchive(
+                purpose=ArchivePurpose.PRIMARY)
+            distroseries = self.factory.makeDistroSeries(
+                distribution=archive.distribution)
+            person = self.factory.makePerson()
+            distro_name = archive.distribution.name
+            distroseries_name = distroseries.name
+            person_name = person.name
+
+        self.webservice = LaunchpadWebServiceCaller(
+            'launchpad-library', 'salgado-change-anything')
+
+        self.launchpad = launchpadlib_for(
+            "archive test", "salgado", "WRITE_PUBLIC")
+        self.distribution = self.launchpad.distributions[distro_name]
+        self.distroseries = self.distribution.getSeries(
+            name_or_version=distroseries_name)
+        self.main_archive = self.distribution.main_archive
+        self.person = self.launchpad.people[person_name]
+
+    def test_checkUpload_bad_pocket(self):
+        # Make sure a 403 error and not an OOPS is returned when
+        # CannotUploadToPocket is raised when calling checkUpload.
+        # Remove the security proxy because IPerson.name is protected.
+
+        # When we're on Python 2.7, this code will be much nicer as
+        # assertRaises is a context manager so you can do something like
+        # with self.assertRaises(HTTPError) as cm; do_something
+        # .... then you have cm.exception available.
+        def _do_check():
+            self.main_archive.checkUpload(
+                distroseries=self.distroseries,
+                sourcepackagename="mozilla-firefox",
+                pocket="Updates",
+                component="restricted",
+                person=self.person)
+
+        e = self.assertRaises(HTTPError, _do_check)
+
+        self.assertEqual(403, e.response.status)
+        self.assertIn(
+            "Not permitted to upload to the UPDATES pocket in a series "
+            "in the 'DEVELOPMENT' state.", e.content)
+            
+
+def test_suite():
+    return unittest.TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2010-07-18 00:56:16 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2010-07-27 12:46:05 +0000
@@ -166,6 +166,7 @@
 
 class CannotUploadToPocket(Exception):
     """Returned when a pocket is closed for uploads."""
+    webservice_error(403) # Forbidden.
 
     def __init__(self, distroseries, pocket):
         Exception.__init__(self,