launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/devel-644872-unicode-error-in-search-text into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #644872 in Launchpad itself: "UnicodeEncodeError in search_text field"
  https://bugs.launchpad.net/launchpad/+bug/644872

For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-644872-unicode-error-in-search-text/+merge/59783

= Summary =

Searching for a non-ascii term caused a problem when there was lso an
FAQ for that term. The view method that created the link could not
handle non-ascii code.

== Proposed fix ==

Encode the string as utf-8. If the string is not unicode, this is a nop.

== Tests ==

I cleaned up the test module for questiontarget and added a test for
this bug. It really just needs to make sure that no exception gets
raised.

bin/test -vvcm lp.answers.browser.tests.test_questiontarget

== Demo and Q/A ==

Search for "português" on http://answers.qastaging.launchpad.net/ubuntu
It should not raise an exception anymore.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/answers/browser/tests/test_questiontarget.py
  lib/lp/answers/browser/questiontarget.py
-- 
https://code.launchpad.net/~henninge/launchpad/devel-644872-unicode-error-in-search-text/+merge/59783
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-644872-unicode-error-in-search-text into lp:launchpad.

=== modified file 'lib/lp/answers/browser/questiontarget.py'
--- lib/lp/answers/browser/questiontarget.py	2011-04-26 15:44:26 +0000
+++ lib/lp/answers/browser/questiontarget.py	2011-05-03 14:12:42 +0000
@@ -450,7 +450,7 @@
             "can't call matching_faqs_url when matching_faqs_count == 0")
         collection = IFAQCollection(self.context)
         return canonical_url(collection) + '/+faqs?' + urlencode({
-            'field.search_text': self.search_text,
+            'field.search_text': self.search_text.encode('utf-8'),
             'field.actions.search': 'Search',
             })
 
@@ -464,7 +464,8 @@
         """
         self.search_params = dict(self.getDefaultFilter())
         self.search_params.update(**data)
-        if self.search_params.get('search_text', None) is not None:
+        search_text = self.search_params.get('search_text', None)
+        if search_text is not None:
             self.search_params['search_text'] = (
                 self.search_params['search_text'].strip())
 

=== modified file 'lib/lp/answers/browser/tests/test_questiontarget.py'
--- lib/lp/answers/browser/tests/test_questiontarget.py	2010-10-26 15:47:24 +0000
+++ lib/lp/answers/browser/tests/test_questiontarget.py	2011-05-03 14:12:42 +0000
@@ -6,16 +6,20 @@
 __metaclass__ = type
 
 import os
+from urllib import quote
 
 from BeautifulSoup import BeautifulSoup
-
 from zope.component import getUtility
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.answers.interfaces.questioncollection import IQuestionSet
 from lp.app.enums import ServiceUsage
-from lp.testing import login_person, person_logged_in, TestCaseWithFactory
+from lp.testing import (
+    login_person,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.views import create_initialized_view
 
 
@@ -23,20 +27,29 @@
 
     layer = DatabaseFunctionalLayer
 
-    def linkPackage(self, product, name):
-        # A helper to setup a legitimate Packaging link between a product
-        # and an Ubuntu source package.
-        hoary = getUtility(ILaunchpadCelebrities).ubuntu['hoary']
-        sourcepackagename = self.factory.makeSourcePackageName(name)
-        sourcepackage = self.factory.makeSourcePackage(
-            sourcepackagename=sourcepackagename, distroseries=hoary)
-        self.factory.makeSourcePackagePublishingHistory(
-            sourcepackagename=sourcepackagename, distroseries=hoary)
-        product.development_focus.setPackaging(
-            hoary, sourcepackagename, product.owner)
-
-
-class TestSearchQuestionsViewCanConfigureAnswers(TestSearchQuestionsView):
+    def test_matching_faqs_url__handles_non_ascii(self):
+        product = self.factory.makeProduct()
+        # Avoid non-ascii character in unicode literal to not upset
+        # pocket-lint. Bug #776389.
+        non_ascii_string = u'portugu\xeas'
+        with person_logged_in(product.owner):
+            self.factory.makeFAQ(product, non_ascii_string)
+        form = {
+            'field.search_text': non_ascii_string,
+            'field.status': 'OPEN',
+            'field.actions.search': 'Search',
+            }
+        view = create_initialized_view(
+            product, '+questions', form=form, method='GET')
+
+        encoded_string = quote(non_ascii_string.encode('utf-8'))
+        # This must not raise UnicodeEncodeError.
+        self.assertIn(encoded_string, view.matching_faqs_url)
+
+
+class TestSearchQuestionsViewCanConfigureAnswers(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
 
     def test_cannot_configure_answers_product_no_edit_permission(self):
         product = self.factory.makeProduct()
@@ -75,8 +88,10 @@
         self.assertEqual(False, view.can_configure_answers)
 
 
-class TestSearchQuestionsViewTemplate(TestSearchQuestionsView):
-    """Test the behaviour of SearchQuestionsView.template"""
+class TestSearchQuestionsViewTemplate(TestCaseWithFactory):
+    """Test the behavior of SearchQuestionsView.template"""
+
+    layer = DatabaseFunctionalLayer
 
     def assertViewTemplate(self, context, file_name):
         view = create_initialized_view(context, '+questions')
@@ -89,21 +104,21 @@
 
     def test_template_product_answers_usage_launchpad(self):
         product = self.factory.makeProduct()
-        with person_logged_in(product.owner) as owner:
+        with person_logged_in(product.owner):
             product.answers_usage = ServiceUsage.LAUNCHPAD
         self.assertViewTemplate(product, 'question-listing.pt')
 
     def test_template_projectgroup_answers_usage_unknown(self):
         product = self.factory.makeProduct()
         project_group = self.factory.makeProject(owner=product.owner)
-        with person_logged_in(product.owner) as owner:
+        with person_logged_in(product.owner):
             product.project = project_group
         self.assertViewTemplate(project_group, 'unknown-support.pt')
 
     def test_template_projectgroup_answers_usage_launchpad(self):
         product = self.factory.makeProduct()
         project_group = self.factory.makeProject(owner=product.owner)
-        with person_logged_in(product.owner) as owner:
+        with person_logged_in(product.owner):
             product.project = project_group
             product.answers_usage = ServiceUsage.LAUNCHPAD
         self.assertViewTemplate(project_group, 'question-listing.pt')
@@ -114,7 +129,7 @@
 
     def test_template_distribution_answers_usage_launchpad(self):
         distribution = self.factory.makeDistribution()
-        with person_logged_in(distribution.owner) as owner:
+        with person_logged_in(distribution.owner):
             distribution.answers_usage = ServiceUsage.LAUNCHPAD
         self.assertViewTemplate(distribution, 'question-listing.pt')
 
@@ -124,7 +139,7 @@
 
     def test_template_DSP_answers_usage_launchpad(self):
         dsp = self.factory.makeDistributionSourcePackage()
-        with person_logged_in(dsp.distribution.owner) as owner:
+        with person_logged_in(dsp.distribution.owner):
             dsp.distribution.answers_usage = ServiceUsage.LAUNCHPAD
         self.assertViewTemplate(dsp, 'question-listing.pt')
 
@@ -133,8 +148,22 @@
         self.assertViewTemplate(question_set, 'question-listing.pt')
 
 
-class TestSearchQuestionsViewUnknown(TestSearchQuestionsView):
-    """Test the behaviour of SearchQuestionsView unknown support."""
+class TestSearchQuestionsViewUnknown(TestCaseWithFactory):
+    """Test the behavior of SearchQuestionsView unknown support."""
+
+    layer = DatabaseFunctionalLayer
+
+    def linkPackage(self, product, name):
+        # A helper to setup a legitimate Packaging link between a product
+        # and an Ubuntu source package.
+        hoary = getUtility(ILaunchpadCelebrities).ubuntu['hoary']
+        sourcepackagename = self.factory.makeSourcePackageName(name)
+        self.factory.makeSourcePackage(
+            sourcepackagename=sourcepackagename, distroseries=hoary)
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename=sourcepackagename, distroseries=hoary)
+        product.development_focus.setPackaging(
+            hoary, sourcepackagename, product.owner)
 
     def setUp(self):
         super(TestSearchQuestionsViewUnknown, self).setUp()