Merge lp:~henninge/launchpad/devel-644872-unicode-error-in-search-text into lp:launchpad

[Henning Eggers]

[r=gmb][bug=644872] Handle non-ascii strings in links to FAQs on an answers search page.

= 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

[Graham Binns (gmb)]

review: Approve (code)

[Robert Collins (lifeless)]

This is potentially buggy:
>>> print u'foo\xce'.encode('utf8').encode('utf8')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position
3: ordinal not in range(128)

Do we know that the thing being encoded is always unicode? If so, the
patch is fine. If it /might be/ bytes already re-encoding would go
boom.

[Henning Eggers (henninge)]

I had thought about that but did not reach that conclusion (i.e. that it might already be utf-8). I suggest it should only re-encode unicode strings, or should it do some "trying out"?

Somehow I wonder, though, if this might or should not be solved globally. Shouldn't the widget code take care of these issues and always present me with, say, a unicode object?

[Robert Collins (lifeless)]

On Wed, May 4, 2011 at 8:50 PM, Henning Eggers
<email address hidden> wrote:
> I had thought about that but did not reach that conclusion (i.e. that it might already be utf-8). I suggest it should only re-encode unicode strings, or should it do some "trying out"?
>
> Somehow I wonder, though, if this might or should not be solved globally. Shouldn't the widget code take care of these issues and always present me with, say, a unicode object?

Guaranteeing that the strings inside the webapp code are unicode
objects would be pretty sane(1)(2). Alternatively programming
defensively and doing 'if type(thing) is unicode: thing =
thing.encode('utf8')' would also be sane.

1) but possibly hard: form data might be bogus or whatever. We can
probably afford to just raise UFD on those.
2) it may already be guaranteed; I'm not sure.

-Rob

[Henning Eggers (henninge)]

Am 04.05.2011 11:41, schrieb Robert Collins:
> Alternatively programming
> defensively and doing 'if type(thing) is unicode: thing =
> thing.encode('utf8')' would also be sane.

Yeah, I'll add that.

> 2) it may already be guaranteed; I'm not sure.

That's actually what I meant.

Henning

[Henning Eggers (henninge)]

While writing a test to try out passing in utf-8 strings, it became clear that zope.form uses unicode internally and does not like utf-8 being passed in. So, I think it is safe to assume that search_text is unicode. No change needed.

Tail of traceback:

  File "/home/henning/canonical/lp-sourcedeps/eggs/zope.app.form-3.8.1-py2.6.egg/zope/app/form/browser/widget.py", line 317, in getInputValue
    value = self._toFieldValue(self._getFormInput())
  File "/home/henning/canonical/lp-sourcedeps/eggs/zope.app.form-3.8.1-py2.6.egg/zope/app/form/browser/textwidgets.py", line 139, in _toFieldValue
    if self.convert_missing_value and input == self._missing:
UnicodeWarning: Unicode equal comparison failed to convert both arguments to Unicode - interpreting them as being unequal

=== 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-04 10:28:05 +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,9 +464,9 @@
         """
         self.search_params = dict(self.getDefaultFilter())
         self.search_params.update(**data)
-        if self.search_params.get('search_text', None) is not None:
-            self.search_params['search_text'] = (
-                self.search_params['search_text'].strip())
+        search_text = self.search_params.get('search_text', None)
+        if search_text is not None:
+            self.search_params['search_text'] = search_text.strip()
 
     def searchResults(self):
         """Return the questions corresponding to the search."""

=== 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-04 10:28:05 +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()