launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/packagebugs-search-0 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #68702 +packagebugs-search crashes when field.sourcepackagename is a non-existent package
  https://bugs.launchpad.net/bugs/68702

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/packagebugs-search-0/+merge/49479

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
-- 
https://code.launchpad.net/~sinzui/launchpad/packagebugs-search-0/+merge/49479
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/packagebugs-search-0 into lp:launchpad.

=== 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-11 23:39:56 +0000
@@ -0,0 +1,70 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version (see the file LICENSE).
+
+"""Unit tests for person bug views."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.errors import UnexpectedFormData
+from lp.testing import TestCaseWithFactory
+from lp.testing.views import create_initialized_view
+
+
+class TestBugSubscriberPackageBugsSearchListingView(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestBugSubscriberPackageBugsSearchListingView, self).setUp()
+        self.person = self.factory.makePerson()
+        self.distribution = self.factory.makeDistribution()
+        self.spn = self.factory.makeSourcePackageName()
+        self.dsp = self.distribution.getSourcePackage(self.spn)
+
+    def make_form(self, package_name, distribution_name):
+        return {
+            'field.sourcepackagename': package_name,
+            'field.distribution': distribution_name,
+            'search': 'Search',
+            }
+
+    def test_current_package_known(self):
+        # current_package contains the distribution source package that
+        # matches the source package name.
+        form = self.make_form(self.spn.name, self.distribution.name)
+        view = create_initialized_view(
+            self.person, name='+packagebugs-search', form=form)
+        self.assertEqual(self.dsp, view.current_package)
+
+    def test_current_package_missing_distribution(self):
+        # UnexpectedFormData is raised if the distribution is not provided.
+        form = self.make_form(self.spn.name, '')
+        view = create_initialized_view(
+            self.person, name='+packagebugs-search', form=form)
+        self.assertRaises(
+            UnexpectedFormData, getattr, view, 'current_package')
+
+    def test_current_package_unknown_distribution(self):
+        # UnexpectedFormData is raised if the distribution is not known.
+        form = self.make_form(self.spn.name, 'unknown-distribution')
+        view = create_initialized_view(
+            self.person, name='+packagebugs-search', form=form)
+        self.assertRaises(
+            UnexpectedFormData, getattr, view, 'current_package')
+
+    def test_current_package_missing_sourcepackagename(self):
+        # UnexpectedFormData is raised if the package name is not provided.
+        form = self.make_form('', self.distribution.name)
+        view = create_initialized_view(
+            self.person, name='+packagebugs-search', form=form)
+        self.assertRaises(
+            UnexpectedFormData, getattr, view, 'current_package')
+
+    def test_current_package_unknown_sourcepackagename(self):
+        # UnexpectedFormData is raised if the package name is not known.
+        form = self.make_form('unknown-package', self.distribution.name)
+        view = create_initialized_view(
+            self.person, name='+packagebugs-search', form=form)
+        self.assertRaises(
+            UnexpectedFormData, getattr, view, 'current_package')

=== 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-11 23:39:56 +0000
@@ -1926,11 +1926,11 @@
     def current_package(self):
         """Get the package whose bugs are currently being searched."""
         if not (
-            self.widgets['distribution'].hasInput() and
+            self.widgets['distribution'].hasValidInput() and
             self.widgets['distribution'].getInputValue()):
             raise UnexpectedFormData("A distribution is required")
         if not (
-            self.widgets['sourcepackagename'].hasInput() and
+            self.widgets['sourcepackagename'].hasValidInput() and
             self.widgets['sourcepackagename'].getInputValue()):
             raise UnexpectedFormData("A sourcepackagename is required")