launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02570
[Merge] lp:~sinzui/launchpad/bug-description-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/bug-description-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#68203 Bug description with only whitespaces oopses
https://bugs.launchpad.net/bugs/68203
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/bug-description-0/+merge/49302
Do not accept whitespace comments when reporting a bug.
Launchpad bug: https://bugs.launchpad.net/bugs/68203
Pre-implementation: jcsackett
Test command: ./bin/test -vv \
-t test_bugtarget_filebug \
-t bugtarget-filebug-views -t xx-product-guided-filebug
Bug description with only whitespaces oopses
When reporting a bug, or editing a bug description, the form accepts ' ',
but an error is raised when the creation/update happens because the the
value will be stripped latter.
This could be fixed in one line by stripping the comment before the
existing validation rules are tested. There is a deeper issue though. The
form /thinks/ is is validating. The tests for the validation do not use
use the path that users will use, the tests do not work with the data that
the methods will receive from LaunchpadForm! Attempting to set an existing
bug's description to whitespace will cause a sane_description DB violation.
The Description field descends from StrippableField, but the schemas
are do not use it. Updating the schemas to do what we intend will not
directly fix this issue because the the DescriptionWidget does not descend
from StrippableTextWidget, so it does not provide a value for validation that
the field will actually be set to.
In summary, the schema, view, and database disagree about definition of
a bug description/comment.
PS the existing form explicitly shows "Required input is missing." if the
comment/description is invalid which is very unhelpful.
--------------------------------------------------------------------
RULES
* Update IBugAddForm.comment and IBug.description to be a Description
field that enforces stripping whitespace. Set the constraints to
be >=1 and <= 50000 to match sane_description.
* Update the validation rules in +filebug to work with schema validation.
* Replace the validation doctests with unittests that actually work with
the kind of data that users will submit.
* Revised the error message to help the users to complete the form. Maybe
"Provide details about the issue."
QA
* Visit any project and choose Report a bug link
* Enter anything in the Summary
* Enter a single space in the Further information field.
* Submit
* Verify the page asks you to provide details.
* Visit an existing bug.
* Try to set the description to a space
* Verify the form/ajax does not accept the space.
LINT
lib/lp/bugs/browser/bugtarget.py
lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt
lib/lp/bugs/browser/tests/test_bugtarget_filebug.py
lib/lp/bugs/interfaces/bug.py
lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt
lib/lp/services/fields/__init__.py
lib/lp/services/fields/tests/test_fields.py
^ lint reports issues in the doctests. I can fix these before I land the
branch.
IMPLEMENTATION
Updated the stripping rules for StrippableText, the base class for the
Description field. I extracted the striping rules to normalize() so that
they can be used in more than one method.
lib/lp/services/fields/__init__.py
lib/lp/services/fields/tests/test_fields.py
Updated IBug and IBugAddForm to use the Description field. Set the min and
max to match the rules of the database sane_description constraint. Replaced
the flawed validation doctest with a unittest that really calls
LaunchpadFormView.initialize() This ensures that formlib processed the user
data. I modified the view's validation rules to work with the form validation
because tests showed that the view could not handle cases where initialize()
mutated the form data.
lib/lp/bugs/browser/bugtarget.py
lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt
lib/lp/bugs/browser/tests/test_bugtarget_filebug.py
lib/lp/bugs/interfaces/bug.py
Fixed the error message for missing or meaningless comment input. Removed
duplicate test.
lib/lp/bugs/browser/bugtarget.py
lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt
--
https://code.launchpad.net/~sinzui/launchpad/bug-description-0/+merge/49302
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/bug-description-0 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py 2011-02-03 04:31:19 +0000
+++ lib/lp/bugs/browser/bugtarget.py 2011-02-10 21:25:13 +0000
@@ -47,6 +47,7 @@
Bool,
Choice,
)
+from zope.schema.interfaces import TooLong
from zope.schema.vocabulary import SimpleVocabulary
from zope.security.proxy import removeSecurityProxy
@@ -105,7 +106,6 @@
from lp.bugs.interfaces.apportjob import IProcessApportBlobJobSource
from lp.bugs.interfaces.bug import (
CreateBugParams,
- IBug,
IBugAddForm,
IBugSet,
IProjectGroupBugAddForm,
@@ -344,13 +344,19 @@
# The comment field is only required if filing a new bug.
if self.submit_bug_action.submitted():
comment = data.get('comment')
- if comment:
- if len(comment) > IBug['description'].max_length:
- self.setFieldError('comment',
- 'The description is too long. If you have lots '
- 'text to add, attach a file to the bug instead.')
+ # The widget only exposes the error message. The private
+ # attr contains the real error.
+ widget_error = self.widgets.get('comment')._error
+ if widget_error and isinstance(widget_error.errors, TooLong):
+ self.setFieldError('comment',
+ 'The description is too long. If you have lots '
+ 'text to add, attach a file to the bug instead.')
+ elif not comment or widget_error is not None:
+ self.setFieldError(
+ 'comment', "Provide details about the issue.")
else:
- self.setFieldError('comment', "Required input is missing.")
+ # The comment is fine.
+ pass
# Check a bug has been selected when the user wants to
# subscribe to an existing bug.
elif self.this_is_my_bug_action.submitted():
=== modified file 'lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt'
--- lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt 2011-02-03 04:31:19 +0000
+++ lib/lp/bugs/browser/tests/bugtarget-filebug-views.txt 2011-02-10 21:25:13 +0000
@@ -703,46 +703,3 @@
bug-superdude
>>> print added_bugtask.milestone.name
bug-superdude-milestone
-
-
-== Validation ==
-
-
-=== The comment field ===
-
-When filing a bug, supplying an empty comment causes a validation error:
-
- >>> from lp.bugs.browser.bugtarget import FileBugGuidedView
- >>> request = LaunchpadTestRequest(
- ... form={'field.actions.submit_bug': 'Submit Bug Request'})
- >>> filebug_view = FileBugGuidedView(ubuntu, request)
- >>> filebug_view.submit_bug_action.submitted()
- True
- >>> bug_data = dict(title='Test Title', comment='')
- >>> filebug_view.setUpFields()
- >>> filebug_view.setUpWidgets()
- >>> filebug_view.validate(bug_data)
- >>> print filebug_view.getFieldError('comment')
- Required input is missing.
-
-Comments can be up to 50000 characters in length:
-
- >>> comment = 'x' * 50000
- >>> bug_data = dict(title='Test Title', comment=comment)
- >>> filebug_view = FileBugGuidedView(ubuntu, request)
- >>> filebug_view.setUpFields()
- >>> filebug_view.setUpWidgets()
- >>> filebug_view.validate(bug_data)
- >>> print filebug_view.getFieldError('comment')
- <BLANKLINE>
-
-Supplying a comment that is too long causes a validation error:
-
- >>> comment = 'x' * 50001
- >>> bug_data = dict(title='Test Title', comment=comment)
- >>> filebug_view = FileBugGuidedView(ubuntu, request)
- >>> filebug_view.setUpFields()
- >>> filebug_view.setUpWidgets()
- >>> filebug_view.validate(bug_data)
- >>> print filebug_view.getFieldError('comment')
- The description is too long...
=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py 2011-02-03 03:12:28 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py 2011-02-10 21:25:13 +0000
@@ -4,13 +4,20 @@
__metaclass__ = type
-import unittest
+from zope.schema.interfaces import (
+ TooLong,
+ TooShort,
+ )
from canonical.launchpad.ftests import login
from canonical.launchpad.testing.pages import find_tag_by_id
from canonical.launchpad.webapp.servers import LaunchpadTestRequest
-from canonical.testing.layers import LaunchpadFunctionalLayer
-from lp.bugs.browser.bugtarget import FileBugInlineFormView
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.bugs.browser.bugtarget import (
+ FileBugInlineFormView,
+ FileBugViewBase,
+ )
+from lp.bugs.interfaces.bug import IBugAddForm
from lp.testing import (
login_person,
TestCaseWithFactory,
@@ -20,7 +27,7 @@
class TestBugTargetFileBugConfirmationMessage(TestCaseWithFactory):
- layer = LaunchpadFunctionalLayer
+ layer = DatabaseFunctionalLayer
def setUp(self):
super(TestBugTargetFileBugConfirmationMessage, self).setUp()
@@ -294,11 +301,69 @@
self.assertIs(None, find_tag_by_id(html, 'filebug-search-form'))
-def test_suite():
- suite = unittest.TestSuite()
- suite.addTest(unittest.makeSuite(TestBugTargetFileBugConfirmationMessage))
- return suite
-
-
-if __name__ == '__main__':
- unittest.TextTestRunner().run(test_suite())
+class TestFileBugViewBase(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ class FileBugTestView(FileBugViewBase):
+ """A simple subclass."""
+ schema = IBugAddForm
+
+ def showFileBugForm(self):
+ # Disable redirects on validation failure.
+ pass
+
+ def setUp(self):
+ super(TestFileBugViewBase, self).setUp()
+ self.target = self.factory.makeProduct()
+ login_person(self.target.owner)
+ self.target.official_malone = True
+
+ def get_form(self, title='Test title', comment='Test comment'):
+ return {
+ 'field.title': title,
+ 'field.comment': comment,
+ 'field.actions.submit_bug': 'Submit Bug Request',
+ }
+
+ def create_initialized_view(self, form=None):
+ """Create and initialize the class without adaption."""
+ request = LaunchpadTestRequest(form=form, method='POST')
+ view = self.FileBugTestView(self.target, request)
+ view.initialize()
+ return view
+
+ def test_submit_comment_empty_error(self):
+ # The comment cannot be an empty string.
+ form = self.get_form(comment='')
+ view = self.create_initialized_view(form=form)
+ self.assertEqual(1, len(view.errors))
+ self.assertEqual(
+ 'Provide details about the issue.', view.getFieldError('comment'))
+
+ def test_submit_comment_whitespace_only_error(self):
+ # The comment cannot be a whitespace only string.
+ form = self.get_form(comment=' ')
+ view = self.create_initialized_view(form=form)
+ self.assertEqual(2, len(view.errors))
+ self.assertIsInstance(view.errors[0].errors, TooShort)
+ self.assertEqual(
+ 'Provide details about the issue.', view.errors[1])
+
+ def test_submit_comment_too_large_error(self):
+ # The comment cannot exceed the max length of 50000.
+ comment = 'x' * 50001
+ form = self.get_form(comment=comment)
+ view = self.create_initialized_view(form=form)
+ self.assertEqual(2, len(view.errors))
+ self.assertIsInstance(view.errors[0].errors, TooLong)
+ message_start = 'The description is too long'
+ self.assertTrue(
+ view.getFieldError('comment').startswith(message_start))
+
+ def test_submit_comment_max(self):
+ # The comment can be as large as 50000.
+ form = self.get_form(comment='x' * 50000)
+ view = self.create_initialized_view(form=form)
+ self.assertEqual(0, len(view.errors))
+ self.assertTrue(view.added_bug is not None)
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2011-02-04 10:37:21 +0000
+++ lib/lp/bugs/interfaces/bug.py 2011-02-10 21:25:13 +0000
@@ -85,6 +85,7 @@
from lp.services.fields import (
BugField,
ContentNameField,
+ Description,
DuplicateBug,
PublicPersonChoice,
Tag,
@@ -212,10 +213,11 @@
Title(title=_('Summary'), required=True,
description=_("""A one-line summary of the problem.""")))
description = exported(
- Text(title=_('Description'), required=True,
+ Description(title=_('Description'), required=True,
description=_("""A detailed description of the problem,
including the steps required to reproduce it."""),
- max_length=50000))
+ strip_text=True, trailing_only=True,
+ min_length=1, max_length=50000))
ownerID = Int(title=_('Owner'), required=True, readonly=True)
owner = exported(
Reference(IPerson, title=_("The owner's IPerson"), readonly=True))
@@ -992,9 +994,10 @@
"tracker."),
vocabulary="DistributionUsingMalone")
owner = Int(title=_("Owner"), required=True)
- comment = Text(
+ comment = Description(
title=_('Further information'),
- required=False)
+ strip_text=True, trailing_only=True,
+ min_length=1, max_length=50000, required=False)
bug_already_reported_as = Choice(
title=_("This bug has already been reported as ..."), required=False,
vocabulary="Bug")
=== modified file 'lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt'
--- lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt 2010-12-02 20:37:35 +0000
+++ lib/lp/bugs/stories/guided-filebug/xx-product-guided-filebug.txt 2011-02-10 21:25:13 +0000
@@ -46,18 +46,6 @@
If the user doesn't see their bug already reported, they can proceed to
file the bug.
-If no description is entered, an error message is displayed.
-
- >>> user_browser.getControl("Further information").value
- ''
- >>> user_browser.getControl("Submit Bug Report").click()
- >>> print user_browser.url
- http://bugs.launchpad.dev/firefox/+filebug
- >>> for message in find_tags_by_class(user_browser.contents, 'message'):
- ... print message.renderContents()
- There is 1 error.
- Required input is missing.
-
If the user for some reason would erase the summary, an error message
will be displayed as well.
=== modified file 'lib/lp/services/fields/__init__.py'
--- lib/lp/services/fields/__init__.py 2011-02-07 16:37:09 +0000
+++ lib/lp/services/fields/__init__.py 2011-02-10 21:25:13 +0000
@@ -254,16 +254,31 @@
class StrippableText(Text):
"""A text that can be configured to strip when setting."""
- def __init__(self, strip_text=False, **kwargs):
+ def __init__(self, strip_text=False, trailing_only=False, **kwargs):
super(StrippableText, self).__init__(**kwargs)
self.strip_text = strip_text
+ self.trailing_only = trailing_only
- def set(self, object, value):
- """Strip the value and pass up."""
+ def normalize(self, value):
+ """Strip the leading and trailing whitespace."""
if self.strip_text and value is not None:
- value = value.strip()
+ if self.trailing_only:
+ value = value.rstrip()
+ else:
+ value = value.strip()
+ return value
+
+ def set(self, object, value):
+ """Strip the value and pass up."""
+ value = self.normalize(value)
super(StrippableText, self).set(object, value)
+ def validate(self, value):
+ """See `IField`."""
+ value = self.normalize(value)
+ return super(StrippableText, self).validate(value)
+
+
# Summary
# A field capture a Launchpad object summary
=== modified file 'lib/lp/services/fields/tests/test_fields.py'
--- lib/lp/services/fields/tests/test_fields.py 2011-02-07 16:37:09 +0000
+++ lib/lp/services/fields/tests/test_fields.py 2011-02-10 21:25:13 +0000
@@ -11,6 +11,7 @@
from zope.interface import Interface
from zope.component import getUtility
+from zope.schema.interfaces import TooShort
from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.launchpad.validators import LaunchpadValidationError
@@ -64,6 +65,15 @@
field.set(target, ' testing ')
self.assertEqual('testing', target.test)
+ def test_strips_text_trailing_only(self):
+ # The set method strips the trailing whitespace.
+ target = make_target()
+ field = StrippableText(
+ __name__='test', strip_text=True, trailing_only=True)
+ self.assertTrue(field.trailing_only)
+ field.set(target, ' testing ')
+ self.assertEqual(' testing', target.test)
+
def test_default_constructor(self):
# If strip_text is not set, or set to false, then the text is not
# stripped when set.
@@ -80,6 +90,18 @@
field.set(target, None)
self.assertIs(None, target.test)
+ def test_validate_min_contraints(self):
+ # The minimum length constraint tests the stripped string.
+ field = StrippableText(
+ __name__='test', strip_text=True, min_length=1)
+ self.assertRaises(TooShort, field.validate, u' ')
+
+ def test_validate_max_contraints(self):
+ # The minimum length constraint tests the stripped string.
+ field = StrippableText(
+ __name__='test', strip_text=True, max_length=2)
+ self.assertEqual(None, field.validate(u' a '))
+
class TestBlacklistableContentNameField(TestCaseWithFactory):
