Launchpad itself

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

checkUpload-bug-608891
Merge into devel

Proposed by Julian Edwards on 2010-07-27

Status:

Merged

Approved by:

Māris Fogels on 2010-07-27

Approved revision:

no longer in the source branch.

Merged at revision:

11238

Proposed branch:

lp://qastaging/~julian-edwards/launchpad/checkUpload-bug-608891

Merge into:

lp://qastaging/launchpad

Diff against target:

83 lines (+68/-0)

2 files modified

lib/lp/soyuz/browser/tests/test_archive_webservice.py (+67/-0)
lib/lp/soyuz/interfaces/archive.py (+1/-0)

To merge this branch:

bzr merge lp://qastaging/~julian-edwards/launchpad/checkUpload-bug-608891

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Māris Fogels (community)	code	2010-07-27	Approve on 2010-07-27
Review via email: mp+31041@code.qastaging.launchpad.net

Description of the change

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

Revision history for this message

Māris Fogels (mars) wrote on 2010-07-27:

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)

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2010-07-27:

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.

Revision history for this message

Jonathan Lange (jml) wrote on 2010-07-27:

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