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
1=== added file 'lib/lp/bugs/browser/tests/test_person_bugs.py'
2--- lib/lp/bugs/browser/tests/test_person_bugs.py 1970-01-01 00:00:00 +0000
3+++ lib/lp/bugs/browser/tests/test_person_bugs.py 2011-02-14 15:57:52 +0000
4@@ -0,0 +1,70 @@
5+# Copyright 2011 Canonical Ltd. This software is licensed under the
6+# GNU Affero General Public License version (see the file LICENSE).
7+
8+"""Unit tests for person bug views."""
9+
10+__metaclass__ = type
11+
12+from canonical.testing.layers import DatabaseFunctionalLayer
13+from lp.app.errors import UnexpectedFormData
14+from lp.testing import TestCaseWithFactory
15+from lp.testing.views import create_initialized_view
16+
17+
18+class TestBugSubscriberPackageBugsSearchListingView(TestCaseWithFactory):
19+
20+ layer = DatabaseFunctionalLayer
21+
22+ def setUp(self):
23+ super(TestBugSubscriberPackageBugsSearchListingView, self).setUp()
24+ self.person = self.factory.makePerson()
25+ self.distribution = self.factory.makeDistribution()
26+ self.spn = self.factory.makeSourcePackageName()
27+ self.dsp = self.distribution.getSourcePackage(self.spn)
28+
29+ def makeForm(self, package_name, distribution_name):
30+ return {
31+ 'field.sourcepackagename': package_name,
32+ 'field.distribution': distribution_name,
33+ 'search': 'Search',
34+ }
35+
36+ def test_current_package_known(self):
37+ # current_package contains the distribution source package that
38+ # matches the source package name.
39+ form = self.makeForm(self.spn.name, self.distribution.name)
40+ view = create_initialized_view(
41+ self.person, name='+packagebugs-search', form=form)
42+ self.assertEqual(self.dsp, view.current_package)
43+
44+ def test_current_package_missing_distribution(self):
45+ # UnexpectedFormData is raised if the distribution is not provided.
46+ form = self.makeForm(self.spn.name, '')
47+ view = create_initialized_view(
48+ self.person, name='+packagebugs-search', form=form)
49+ self.assertRaises(
50+ UnexpectedFormData, getattr, view, 'current_package')
51+
52+ def test_current_package_unknown_distribution(self):
53+ # UnexpectedFormData is raised if the distribution is not known.
54+ form = self.makeForm(self.spn.name, 'unknown-distribution')
55+ view = create_initialized_view(
56+ self.person, name='+packagebugs-search', form=form)
57+ self.assertRaises(
58+ UnexpectedFormData, getattr, view, 'current_package')
59+
60+ def test_current_package_missing_sourcepackagename(self):
61+ # UnexpectedFormData is raised if the package name is not provided.
62+ form = self.makeForm('', self.distribution.name)
63+ view = create_initialized_view(
64+ self.person, name='+packagebugs-search', form=form)
65+ self.assertRaises(
66+ UnexpectedFormData, getattr, view, 'current_package')
67+
68+ def test_current_package_unknown_sourcepackagename(self):
69+ # UnexpectedFormData is raised if the package name is not known.
70+ form = self.makeForm('unknown-package', self.distribution.name)
71+ view = create_initialized_view(
72+ self.person, name='+packagebugs-search', form=form)
73+ self.assertRaises(
74+ UnexpectedFormData, getattr, view, 'current_package')
75
76=== modified file 'lib/lp/registry/browser/person.py'
77--- lib/lp/registry/browser/person.py 2011-02-07 21:29:06 +0000
78+++ lib/lp/registry/browser/person.py 2011-02-14 15:57:52 +0000
79@@ -1926,11 +1926,11 @@
80 def current_package(self):
81 """Get the package whose bugs are currently being searched."""
82 if not (
83- self.widgets['distribution'].hasInput() and
84+ self.widgets['distribution'].hasValidInput() and
85 self.widgets['distribution'].getInputValue()):
86 raise UnexpectedFormData("A distribution is required")
87 if not (
88- self.widgets['sourcepackagename'].hasInput() and
89+ self.widgets['sourcepackagename'].hasValidInput() and
90 self.widgets['sourcepackagename'].getInputValue()):
91 raise UnexpectedFormData("A sourcepackagename is required")
92