launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/question-title-0 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #583623 impossible length titles cause oopes
  https://bugs.launchpad.net/bugs/583623


This is my branch to prevent users from entering long question summaries.

    lp:~sinzui/launchpad/question-title-0
    Diff size: 84
    Launchpad bug:
          https://bugs.launchpad.net/bugs/583623
    Test command: ./bin/test -vv -t TestQuestionAddView
    Pre-implementation: no one
    Target release: 10.09


Prevent users from entering long question summaries
----------------------------------------------------

OOPS-1601O1172 was caused when the user pasted an error log into the title
field. The field should restrict the title to a reasonable

Discovering a reasonable length took some time, looking at question titles
in the db, and at bug titles (because we want bugs to be convertible to
questions). Changing the field definition has a secondary complication--we
would need to update all the existing questions that have titles that are
too long. Since we really want comments to work on way in Launchpad, a
better approach is to ensure the view does not let the user provide too
much information.


Rules
-----

The simplest fix for this issue is to use displayMaxWidth=250 in the
QuestionAddView to tell the browser to ignore characters after 250.
     custom_widget('title', TextWidget, displayWidth=40, displayMaxWidth=250)

But in the case where users are posting from a non-lp form (I am speculating)
we can add a constraint to the view to return a field error too. This is also
easier to test than browser compliance.


QA
--

    * On staging, as a question with more than 250 characters.
    * Verify the page says the question summary cannot exceed 250 characters.


Lint
----

Linting changed files:
  lib/lp/answers/browser/question.py
  lib/lp/answers/browser/tests/test_question.py


Test
----

I renamed the existing module to test_views since it it is loading doctests
for question and faq views. 

    * lib/lp/answers/browser/tests/test_question.py
      * Added tests for the max length of the title field.


Implementation
--------------

    * lib/lp/answers/browser/question.py
      * Added a validation rule to report an error if the question title is
        exceeds 250 characters.
      * Added displayMaxWidth to the html element so that users can see that
        the excessive content is ignored.
-- 
https://code.launchpad.net/~sinzui/launchpad/question-title-0/+merge/34555
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/question-title-0 into lp:launchpad/devel.

=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py	2010-08-24 10:45:57 +0000
+++ lib/lp/answers/browser/question.py	2010-09-03 17:08:51 +0000
@@ -539,7 +539,7 @@
     # The fields displayed on the search page.
     search_field_names = ['language', 'title']
 
-    custom_widget('title', TextWidget, displayWidth=40)
+    custom_widget('title', TextWidget, displayWidth=40, displayMaxWidth=250)
 
     search_template = ViewPageTemplateFile(
         '../templates/question-add-search.pt')
@@ -601,6 +601,10 @@
         if 'title' not in data:
             self.setFieldError(
                 'title', _('You must enter a summary of your problem.'))
+        else:
+            if len(data['title']) > 250:
+                self.setFieldError(
+                    'title', _('The summary cannot exceed 250 characters.'))
         if self.widgets.get('description'):
             if 'description' not in data:
                 self.setFieldError(

=== added file 'lib/lp/answers/browser/tests/test_question.py'
--- lib/lp/answers/browser/tests/test_question.py	1970-01-01 00:00:00 +0000
+++ lib/lp/answers/browser/tests/test_question.py	2010-09-03 17:08:51 +0000
@@ -0,0 +1,53 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the question module."""
+
+__metaclass__ = type
+
+__all__ = []
+
+from canonical.testing import DatabaseFunctionalLayer
+from lp.answers.publisher import AnswersLayer
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
+from lp.testing.views import create_initialized_view
+
+
+class TestQuestionAddView(TestCaseWithFactory):
+    """Verify the behavior of the QuestionAddView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestQuestionAddView, self).setUp()
+        self.question_target = self.factory.makeProduct()
+        self.user = self.factory.makePerson()
+        login_person(self.user)
+
+    def getSearchForm(self, title, language='en'):
+        return {
+            'field.title': title,
+            'field.language': language,
+            'field.actions.continue': 'Continue',
+            }
+
+    def test_question_title_within_max_display_width(self):
+        # Titles (summary in the view) less than 250 characters are accepted.
+        form = self.getSearchForm('123456789 ' * 10)
+        view = create_initialized_view(
+            self.question_target, name='+addquestion', layer=AnswersLayer,
+            form=form, principal=self.user)
+        self.assertEqual([], view.errors)
+
+    def test_question_title_exceeds_max_display_width(self):
+        # Titles (summary in the view) cannot exceed 250 characters.
+        form = self.getSearchForm('123456789 ' * 26)
+        view = create_initialized_view(
+            self.question_target, name='+addquestion', layer=AnswersLayer,
+            form=form, principal=self.user)
+        self.assertEqual(1, len(view.errors))
+        self.assertEqual(
+            'The summary cannot exceed 250 characters.', view.errors[0])

=== renamed file 'lib/lp/answers/browser/tests/test_question.py' => 'lib/lp/answers/browser/tests/test_views.py'