launchpad-reviewers team mailing list archive

["j.c.sackett" <jonathan.sackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/one-hundred-and-ten-percent into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #777357 in Launchpad itself: "Searches containing a percent sign generates an oops"
  https://bugs.launchpad.net/launchpad/+bug/777357

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/one-hundred-and-ten-percent/+merge/61286

Summary
=======
A problem with escaping %'s in search text in blueprints leads to an oops. This branch escapes the % by replacing it with %%.

Preimplementation
=================
Spoke with Curtis Hovey

Implementation
=============
Where searchtext is used as a filter, it's now escaped via replace('%', '%%'). This is the same method we've used to escape in other code.

Tests
=====
bin/test -t search_with_percent

QA
==
https://blueprints.qastaging.launchpad.net/?searchtext=% will load without oopsing.

Lint
====
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/browser/specificationtarget.py
  lib/lp/blueprints/browser/tests/test_specification.py

./lib/lp/blueprints/browser/specificationtarget.py
     279: E301 expected 1 blank line, found 0
     282: E301 expected 1 blank line, found 0

Both of the 301s are from functions defined within a method.
-- 
https://code.launchpad.net/~jcsackett/launchpad/one-hundred-and-ten-percent/+merge/61286
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/one-hundred-and-ten-percent into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/specificationtarget.py'
--- lib/lp/blueprints/browser/specificationtarget.py	2010-11-01 03:36:04 +0000
+++ lib/lp/blueprints/browser/specificationtarget.py	2011-05-17 17:31:15 +0000
@@ -355,7 +355,7 @@
 
         # include text for filtering if it was given
         if self.searchtext is not None and len(self.searchtext) > 0:
-            filter.append(self.searchtext)
+            filter.append(self.searchtext.replace('%', '%%'))
 
         # filter on completeness
         if show == 'all':

=== modified file 'lib/lp/blueprints/browser/tests/test_specification.py'
--- lib/lp/blueprints/browser/tests/test_specification.py	2011-04-14 22:33:16 +0000
+++ lib/lp/blueprints/browser/tests/test_specification.py	2011-05-17 17:31:15 +0000
@@ -9,6 +9,7 @@
 from lazr.restful.testing.webservice import FakeRequest
 import pytz
 from testtools.matchers import Equals
+from zope.component import getUtility
 from zope.publisher.interfaces import NotFound
 from zope.security.proxy import removeSecurityProxy
 
@@ -19,7 +20,10 @@
 from lp.app.browser.tales import format_link
 from lp.blueprints.browser import specification
 from lp.blueprints.enums import SpecificationImplementationStatus
-from lp.blueprints.interfaces.specification import ISpecification
+from lp.blueprints.interfaces.specification import (
+    ISpecification,
+    ISpecificationSet,
+    )
 from lp.testing import (
     login_person,
     person_logged_in,
@@ -28,6 +32,18 @@
 from lp.testing.views import create_initialized_view
 
 
+class TestSpecificationSearch(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_search_with_percent(self):
+        # Using '%' in a search should not error.
+        specs = getUtility(ISpecificationSet)
+        form = {'field.search_text': r'%'}
+        view = create_initialized_view(specs, '+index', form=form)
+        self.assertEqual([], view.errors)
+
+
 class LocalFakeRequest(FakeRequest):
 
     @property
@@ -146,8 +162,9 @@
     layer = DatabaseFunctionalLayer
 
     def test_records_started(self):
+        not_started = SpecificationImplementationStatus.NOTSTARTED
         spec = self.factory.makeSpecification(
-            implementation_status=SpecificationImplementationStatus.NOTSTARTED)
+            implementation_status=not_started)
         login_person(spec.owner)
         form = {
             'field.implementation_status': 'STARTED',
@@ -155,7 +172,8 @@
             }
         view = create_initialized_view(spec, name='+status', form=form)
         self.assertEqual(
-            SpecificationImplementationStatus.STARTED, spec.implementation_status)
+            SpecificationImplementationStatus.STARTED,
+            spec.implementation_status)
         self.assertEqual(spec.owner, spec.starter)
         [notification] = view.request.notifications
         self.assertEqual(BrowserNotificationLevel.INFO, notification.level)
@@ -172,12 +190,13 @@
             }
         view = create_initialized_view(spec, name='+status', form=form)
         self.assertEqual(
-            SpecificationImplementationStatus.SLOW, spec.implementation_status)
+            SpecificationImplementationStatus.SLOW,
+            spec.implementation_status)
         self.assertEqual(0, len(view.request.notifications))
 
     def test_records_unstarting(self):
-        # If a spec was started, and is changed to not started, a notice is shown.
-        # Also the spec.starter is cleared out.
+        # If a spec was started, and is changed to not started,
+        # a notice is shown. Also the spec.starter is cleared out.
         spec = self.factory.makeSpecification(
             implementation_status=SpecificationImplementationStatus.STARTED)
         login_person(spec.owner)
@@ -193,7 +212,8 @@
         [notification] = view.request.notifications
         self.assertEqual(BrowserNotificationLevel.INFO, notification.level)
         self.assertEqual(
-            'Blueprint is now considered "Not started".', notification.message)
+            'Blueprint is now considered "Not started".',
+            notification.message)
 
     def test_records_completion(self):
         # If a spec is marked as implemented the user is notifiec it is now
@@ -278,8 +298,6 @@
         self.assertThat(repr_method(), Equals(expected))
 
 
-
-
 def test_suite():
     suite = unittest.TestSuite()
     suite.addTest(unittest.TestLoader().loadTestsFromName(__name__))