Launchpad itself

Merge lp:~sinzui/launchpad/packagebugs-search-0 into lp:launchpad

  1. packagebugs-search-0
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Merged at revision: 12384
Proposed branch: lp:~sinzui/launchpad/packagebugs-search-0
Merge into: lp:launchpad
Diff against target: 91 lines (+72/-2)
2 files modified
lib/lp/bugs/browser/tests/test_person_bugs.py (+70/-0)
lib/lp/registry/browser/person.py (+2/-2)
To merge this branch: bzr merge lp:~sinzui/launchpad/packagebugs-search-0
Reviewer Review Type Date Requested Status
Henning Eggers (community) code Approve
Review via email: mp+49479@code.launchpad.net

Description of the change

packagebugs+search must handle non-existant package names.

    Launchpad bug: https://bugs.launchpad.net/bugs/68702
    Pre-implementation: no one
    Test command: ./bin/test -vv \
      -t TestBugSubscriberPackageBugsSearchListingView

field.sourcepackagename must be a existent sourcepackagename. This bug happens
when you change the URL manually; It should raise an UnexpectedFormData.

This rule seemed odd. I assumed the view could recover and tell the user to
search with valid information. It is not possible though to select
a distribution or source package name from the form; +packagebugs-search
is dependent on the information setup in the links on other Launchpad pages.
This error happens when users hack the URLs found in those pages.

--------------------------------------------------------------------

RULES

    * Update BugSubscriberPackageBugsSearchListingView.current_package to
      handle the case where the input is invalid. The code checks both the
      distro and package widgets for hasInput() and getInputValue(). It
      should check hasValidInput() /before/ getInputValue() or maybe just
      that.

QA

    * Visit https://qastaging.launchpad.net/~xubuntu-team/+packagebugs-search?field.distribution=ubuntu&field.sourcepackagename=fooblah
    * Verify the page reports a UnexpectedFormData error page.

LINT

    lib/lp/bugs/browser/tests/test_person_bugs.py
    lib/lp/registry/browser/person.py

IMPLEMENTATION

Added unittests to verify the documented behaviour. Added two new tests
for unknown distribution or source package name. hasValidInput() fixed the
issue. I discovered getInputValue() was needed to support the missing case,
but hasInput() was not not needed to support the use cases, so I removed it.
    lib/lp/bugs/browser/tests/test_person_bugs.py
    lib/lp/registry/browser/person.py

To post a comment you must log in.
Revision history for this message
Henning Eggers (henninge) wrote :

I think make_form should be called _makeForm to comply with PEP8.
Other than that I find this a nice little fix. Thank you!

review: Approve (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

Hi Henning.

lowercase_underscores methods names is PEP8. I think you mean you want Zope mixedCase method names.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added file 'lib/lp/bugs/browser/tests/test_person_bugs.py'
--- lib/lp/bugs/browser/tests/test_person_bugs.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/browser/tests/test_person_bugs.py 2011-02-14 15:57:52 +0000
@@ -0,0 +1,70 @@
1# Copyright 2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version (see the file LICENSE).
3
4"""Unit tests for person bug views."""
5
6__metaclass__ = type
7
8from canonical.testing.layers import DatabaseFunctionalLayer
9from lp.app.errors import UnexpectedFormData
10from lp.testing import TestCaseWithFactory
11from lp.testing.views import create_initialized_view
12
13
14class TestBugSubscriberPackageBugsSearchListingView(TestCaseWithFactory):
15
16 layer = DatabaseFunctionalLayer
17
18 def setUp(self):
19 super(TestBugSubscriberPackageBugsSearchListingView, self).setUp()
20 self.person = self.factory.makePerson()
21 self.distribution = self.factory.makeDistribution()
22 self.spn = self.factory.makeSourcePackageName()
23 self.dsp = self.distribution.getSourcePackage(self.spn)
24
25 def makeForm(self, package_name, distribution_name):
26 return {
27 'field.sourcepackagename': package_name,
28 'field.distribution': distribution_name,
29 'search': 'Search',
30 }
31
32 def test_current_package_known(self):
33 # current_package contains the distribution source package that
34 # matches the source package name.
35 form = self.makeForm(self.spn.name, self.distribution.name)
36 view = create_initialized_view(
37 self.person, name='+packagebugs-search', form=form)
38 self.assertEqual(self.dsp, view.current_package)
39
40 def test_current_package_missing_distribution(self):
41 # UnexpectedFormData is raised if the distribution is not provided.
42 form = self.makeForm(self.spn.name, '')
43 view = create_initialized_view(
44 self.person, name='+packagebugs-search', form=form)
45 self.assertRaises(
46 UnexpectedFormData, getattr, view, 'current_package')
47
48 def test_current_package_unknown_distribution(self):
49 # UnexpectedFormData is raised if the distribution is not known.
50 form = self.makeForm(self.spn.name, 'unknown-distribution')
51 view = create_initialized_view(
52 self.person, name='+packagebugs-search', form=form)
53 self.assertRaises(
54 UnexpectedFormData, getattr, view, 'current_package')
55
56 def test_current_package_missing_sourcepackagename(self):
57 # UnexpectedFormData is raised if the package name is not provided.
58 form = self.makeForm('', self.distribution.name)
59 view = create_initialized_view(
60 self.person, name='+packagebugs-search', form=form)
61 self.assertRaises(
62 UnexpectedFormData, getattr, view, 'current_package')
63
64 def test_current_package_unknown_sourcepackagename(self):
65 # UnexpectedFormData is raised if the package name is not known.
66 form = self.makeForm('unknown-package', self.distribution.name)
67 view = create_initialized_view(
68 self.person, name='+packagebugs-search', form=form)
69 self.assertRaises(
70 UnexpectedFormData, getattr, view, 'current_package')
071
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2011-02-07 21:29:06 +0000
+++ lib/lp/registry/browser/person.py 2011-02-14 15:57:52 +0000
@@ -1926,11 +1926,11 @@
1926 def current_package(self):1926 def current_package(self):
1927 """Get the package whose bugs are currently being searched."""1927 """Get the package whose bugs are currently being searched."""
1928 if not (1928 if not (
1929 self.widgets['distribution'].hasInput() and1929 self.widgets['distribution'].hasValidInput() and
1930 self.widgets['distribution'].getInputValue()):1930 self.widgets['distribution'].getInputValue()):
1931 raise UnexpectedFormData("A distribution is required")1931 raise UnexpectedFormData("A distribution is required")
1932 if not (1932 if not (
1933 self.widgets['sourcepackagename'].hasInput() and1933 self.widgets['sourcepackagename'].hasValidInput() and
1934 self.widgets['sourcepackagename'].getInputValue()):1934 self.widgets['sourcepackagename'].getInputValue()):
1935 raise UnexpectedFormData("A sourcepackagename is required")1935 raise UnexpectedFormData("A sourcepackagename is required")
19361936