launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-838957 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #838957 in Launchpad itself: "AssertionError: Search by component requires a context with a distribution or distroseries."
  https://bugs.launchpad.net/launchpad/+bug/838957

For more details, see:
https://code.launchpad.net/~bac/launchpad/bug-838957/+merge/74460

= Summary =

Only distributions should have 'components' as part of an advanced bug
search.  Hacked URLs, however, could allow that condition to happen.
In that event we should display a helpful message to the user rather
than throwing an OOPS.

== Proposed fix ==

Change the assert to raising a ValueError which is caught, subdued,
and turned into said notice.

== Pre-implementation notes ==

Nah.

== Implementation details ==

As above.

== Tests ==

bin/test -vvm -t test_buglisting

== Demo and Q/A ==

Go to https://launchpad.dev/ubuntu/+bugs
Select a component
Search
Now, hack the URL to change 'ubuntu' to 'firefox'.
See the message and a redirect to the main bugs page for firefox.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugtask.py
  lib/lp/bugs/model/bugtask.py
  lib/lp/bugs/browser/tests/test_buglisting.py
-- 
https://code.launchpad.net/~bac/launchpad/bug-838957/+merge/74460
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-838957 into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-09-02 16:31:43 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-09-07 15:44:39 +0000
@@ -2726,7 +2726,13 @@
         search_params = self.buildSearchParams(
             searchtext=searchtext, extra_params=extra_params)
         search_params.user = self.user
-        tasks = context.searchTasks(search_params, prejoins=prejoins)
+        try:
+            tasks = context.searchTasks(search_params, prejoins=prejoins)
+        except ValueError, e:
+            self.request.response.addErrorNotification(str(e))
+            self.request.response.redirect(canonical_url(
+                self.context, rootsite='bugs', view_name='+bugs'))
+            tasks = None
         return tasks
 
     def getWidgetValues(
@@ -3734,7 +3740,7 @@
             self._redirectToSearchContext()
 
     def _redirectToSearchContext(self):
-        """Check wether a target was given and redirect to it.
+        """Check whether a target was given and redirect to it.
 
         All the URL parameters will be passed on to the target's +bugs
         page.

=== modified file 'lib/lp/bugs/browser/tests/test_buglisting.py'
--- lib/lp/bugs/browser/tests/test_buglisting.py	2011-08-03 11:00:11 +0000
+++ lib/lp/bugs/browser/tests/test_buglisting.py	2011-09-07 15:44:39 +0000
@@ -156,6 +156,29 @@
             [bugtask.owner for bugtask in bugtasks]
         self.assertThat(recorder, HasQueryCount(Equals(2)))
 
+    def test_search_components_error(self):
+        # Searching for using components for bug targets that are not a distro
+        # or distroseries will report an error, but not OOPS.  See bug
+        # 838957.
+        product = self.factory.makeProduct()
+        form = {
+            'search': 'Search',
+            'advanced': 1,
+            'field.component': 1,
+            'field.component-empty-marker': 1}
+        with person_logged_in(product.owner):
+            view = create_initialized_view(product, '+bugs', form=form)
+            view.searchUnbatched()
+        response = view.request.response
+        self.assertEqual(1, len(response.notifications))
+        expected = (
+            "Search by component requires a context with "
+            "a distribution or distroseries.")
+        self.assertEqual(expected, response.notifications[0].message)
+        self.assertEqual(
+            canonical_url(product, rootsite='bugs', view_name='+bugs'),
+            response.getHeader('Location'))
+
 
 class BugTargetTestCase(TestCaseWithFactory):
     """Test helpers for setting up `IBugTarget` tests."""

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-08-22 13:23:35 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-09-07 15:44:39 +0000
@@ -2004,9 +2004,10 @@
                 distroseries = params.distribution.currentseries
             elif params.distroseries:
                 distroseries = params.distroseries
-            assert distroseries, (
-                "Search by component requires a context with a distribution "
-                "or distroseries.")
+            if distroseries is None:
+                raise ValueError(
+                    "Search by component requires a context with a "
+                    "distribution or distroseries.")
 
             if zope_isinstance(params.component, any):
                 component_ids = sqlvalues(*params.component.query_values)