Merge lp:~julian-edwards/launchpad/checkUpload-bug-608891 into lp:launchpad

[Julian Edwards]

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

[Māris Fogels (mars)]

Hi Julian,

This code looks good. I have a few questions about the code, just trivial fixes:

• The copyright needs updating to 2010

• Does it matter if you explicitly call TestCaseWithFactory.setUp(self), or should you use super()?

• An alternative but still elegant way to write _do_check() is to use a partial function to save the parameters: http://docs.python.org/library/functools.html#functools.partial

With those considerations, r=mars

Maris

review: Approve (code)

[Julian Edwards (julian-edwards)]

On Tuesday 27 July 2010 17:11:01 you wrote:
> • The copyright needs updating to 2010

Woops, fixed, thanks.

> • Does it matter if you explicitly call TestCaseWithFactory.setUp(self), or
> should you use super()?

I was just following the lead of the existing test code. I think I've only
ever used super in __init__ but maybe we should use it everywhere? I'd raise
it at the reviewers' meeting.

> • An alternative but still elegant way to write _do_check() is to use a
> partial function to save the parameters:
> http://docs.python.org/library/functools.html#functools.partial

I'm not entirely convinced that's any more readable! But thanks for the
pointer.

> With those considerations, r=mars

Thanks for the review.

Cheers.

[Jonathan Lange (jml)]

On Tue, Jul 27, 2010 at 5:31 PM, Julian Edwards
<email address hidden> wrote:
...
>> • Does it matter if you explicitly call TestCaseWithFactory.setUp(self), or
>> should you use super()?
>
> I was just following the lead of the existing test code.  I think I've only
> ever used super in __init__ but maybe we should use it everywhere?  I'd raise
> it at the reviewers' meeting.
>

The guideline is to prefer it, except in cases where it's completely
broken due to old-style base classes.

jml

=== 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 16:31:09 +0000
@@ -0,0 +1,67 @@
+# 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 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.
+
+        # 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 16:31:09 +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,