launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/recipe-owner-widget-1 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #714527 Owner widget on Branch:+new-recipe doesn't render structured strings properly
  https://bugs.launchpad.net/bugs/714527

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/recipe-owner-widget-1/+merge/48937

Properly escape labels in suggestion widgets

    Launchpad bug: https://bugs.launchpad.net/bugs/714527
    Pre-implementation: no one
    Test command: ./bin/test -vv \
      -t test_suggestion -t test_itemwidgets

On a page like https://code.launchpad.net/~launchpad-pqm/launchpad/devel/+new-
recipe, the Owner widget has HTML like '<structured-string'. It clearly isn't
being printed properly, but the contents seem to be fine.

ADDENDUM: While writing the test to see the problem, I discovered that
all SuggestionWidgets render invalid HTML markup. They create nested
<labels>.

--------------------------------------------------------------------

RULES

    * Update renderItems() to escape() the call to self._renderLabel()
      to get the IStructured.escapedtext value.
    * Redefine _renderItem() to so that a wrapping label is not added,
      or make _renderItem() in the base class smart enough know not to
      add a wrapping label if the text contains <label.


QA

    * Visit https://code.qastaging.launchpad.net/~launchpad-pqm/launchpad/devel/+new-recipe
    * Verify the owner widget renders the Other label element by clicking the
      text and see that the item is selected.
    * View the source/inspector and verify that labels are not nested.


LINT

    lib/lp/app/widgets/itemswidgets.py
    lib/lp/app/widgets/suggestion.py
    lib/lp/app/widgets/tests/test_itemswidgets.py
    lib/lp/app/widgets/tests/test_suggestion.py


IMPLEMENTATION

Added a rule to omit the wrapping label if the text already has a label. I
tried the simple approach of redefining _renderItem() in SuggestionWidget,
but the code did not look DRY, and the tests were repeated. I decided that
making the base class smarter would prevent extra work for other subclasses.
    lib/lp/app/widgets/itemswidgets.py
    lib/lp/app/widgets/tests/test_itemswidgets.py


I updated the rule that makes the label to only escape the passed text, then
uses escape() to get the IStructured.escapedtext. Setting up a test for the
base widget class was a lot of work for what is actually a two line fix.
Added future tests will be easy :)
    lib/lp/app/widgets/suggestion.py
    lib/lp/app/widgets/tests/test_suggestion.py
-- 
https://code.launchpad.net/~sinzui/launchpad/recipe-owner-widget-1/+merge/48937
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/recipe-owner-widget-1 into lp:launchpad.

=== modified file 'lib/lp/app/widgets/itemswidgets.py'
--- lib/lp/app/widgets/itemswidgets.py	2011-02-03 22:52:06 +0000
+++ lib/lp/app/widgets/itemswidgets.py	2011-02-08 15:57:57 +0000
@@ -110,9 +110,12 @@
                              cssClass=cssClass,
                              type='radio',
                              **kw)
-        return renderElement(u'label',
-                             contents='%s&nbsp;%s' % (elem, text),
-                             **{'style': 'font-weight: normal'})
+        if '<label' in text:
+            return '%s&nbsp;%s' % (elem, text)
+        else:
+            return renderElement(u'label',
+                                 contents='%s&nbsp;%s' % (elem, text),
+                                 **{'style': 'font-weight: normal'})
 
     def _div(self, cssClass, contents, **kw):
         return contents

=== modified file 'lib/lp/app/widgets/suggestion.py'
--- lib/lp/app/widgets/suggestion.py	2011-02-03 22:52:06 +0000
+++ lib/lp/app/widgets/suggestion.py	2011-02-08 15:57:57 +0000
@@ -136,9 +136,10 @@
 
     def _renderLabel(self, text, index):
         """Render a label for the option with the specified index."""
-        label = u'<label for="%s" style="font-weight: normal">%s</label>' % (
+        label = structured(
+            u'<label for="%s" style="font-weight: normal">%s</label>',
             self._optionId(index), text)
-        return structured(label)
+        return label
 
     def _renderSuggestionLabel(self, value, index):
         """Render a label for the option based on a branch."""
@@ -173,7 +174,7 @@
         # Lastly render the other option.
         index = len(items)
         other_selection_text = "%s %s" % (
-            self._renderLabel("Other:", index),
+            escape(self._renderLabel("Other:", index)),
             self.other_selection_widget())
         other_selection_onclick = (
             "this.form['%s'].focus()" % self.other_selection_widget.name)

=== modified file 'lib/lp/app/widgets/tests/test_itemswidgets.py'
--- lib/lp/app/widgets/tests/test_itemswidgets.py	2011-02-03 16:50:09 +0000
+++ lib/lp/app/widgets/tests/test_itemswidgets.py	2011-02-08 15:57:57 +0000
@@ -18,6 +18,7 @@
     Item,
     )
 
+from canonical.launchpad.webapp.menu import structured
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import DatabaseFunctionalLayer
 from lp.app.widgets.itemswidgets import (
@@ -136,6 +137,13 @@
             '&nbsp;&lt;unsafe&gt; &amp;nbsp; title</label>')
         self.assertRenderItem(expected, self.UNSAFE_TERM, checked=False)
 
+    def test__renderItem_without_label(self):
+        # Render item omits a wrapping label if the text contains a label.
+        html_term = SimpleTerm(
+            'object-3', 'token-3', structured('<label>title</label>'))
+        expected = ('<input ... />&nbsp;<label>title</label>')
+        self.assertRenderItem(expected, html_term, checked=False)
+
 
 class TestLaunchpadRadioWidgetWithDescription(TestCaseWithFactory):
     """Test the LaunchpadRadioWidgetWithDescription class."""

=== modified file 'lib/lp/app/widgets/tests/test_suggestion.py'
--- lib/lp/app/widgets/tests/test_suggestion.py	2011-02-01 21:08:27 +0000
+++ lib/lp/app/widgets/tests/test_suggestion.py	2011-02-08 15:57:57 +0000
@@ -8,20 +8,114 @@
     datetime,
     timedelta,
     )
+import doctest
 
 from pytz import utc
+
+from zope.component import provideUtility
+from zope.interface import implements
 from zope.schema import Choice
+from zope.schema.interfaces import IVocabularyFactory
+from zope.schema.vocabulary import (
+    SimpleTerm,
+    SimpleVocabulary,
+    )
 from zope.security.proxy import removeSecurityProxy
 
+from testtools.matchers import DocTestMatches
+
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
+from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
 from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.app.widgets.suggestion import TargetBranchWidget
+from lp.app.widgets.suggestion import (
+    SuggestionWidget,
+    TargetBranchWidget,
+    )
 from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
     )
 
 
+class Simple:
+    """A simple class to test fields ans widgets."""
+
+    def __init__(self, name, displayname):
+        self.name = name
+        self.displayname = displayname
+
+
+class SimpleHugeVocabulary(SimpleVocabulary):
+    implements(IHugeVocabulary)
+    displayname = "Simple objects"
+    step_title = "Select something"
+
+    def __call__(self, context):
+        # Allow an instance to be used as a utility.
+        return self
+
+
+class TestSuggestionWidget(TestCaseWithFactory):
+    """Test the SuggestionWidget class."""
+
+    layer = DatabaseFunctionalLayer
+
+    SAFE_OBJECT = Simple('token-1', 'Safe title')
+    UNSAFE_OBJECT = Simple('token-2', '<unsafe> &nbsp; title')
+
+    SAFE_TERM = SimpleTerm(
+        SAFE_OBJECT, SAFE_OBJECT.name, SAFE_OBJECT.displayname)
+    UNSAFE_TERM = SimpleTerm(
+        UNSAFE_OBJECT, UNSAFE_OBJECT.name, UNSAFE_OBJECT.displayname)
+
+    class ExampleSuggestionWidget(SuggestionWidget):
+
+        @staticmethod
+        def _getSuggestions(context):
+            return SimpleVocabulary([TestSuggestionWidget.SAFE_TERM])
+
+        def _autoselectOther(self):
+            on_key_press = "selectWidget('%s', event);" % self._otherId()
+            self.other_selection_widget.onKeyPress = on_key_press
+
+    def setUp(self):
+        super(TestSuggestionWidget, self).setUp()
+        request = LaunchpadTestRequest()
+        vocabulary = SimpleHugeVocabulary(
+            [self.SAFE_TERM, self.UNSAFE_TERM])
+        provideUtility(
+            vocabulary, provides=IVocabularyFactory,
+            name='SimpleHugeVocabulary')
+        field = Choice(
+            __name__='test_field', vocabulary="SimpleHugeVocabulary")
+        field = field.bind(object())
+        self.widget = self.ExampleSuggestionWidget(
+            field, vocabulary, request)
+
+    def test_renderItems(self):
+        # Render all vocabulary and the other option as items.
+        markups = self.widget.renderItems(None)
+        self.assertEqual(2, len(markups))
+        expected = (
+            """<input class="radioType" checked="checked" ...
+            value="token-1" />&nbsp;<label ...>Safe title</label>""")
+        expected_matcher = DocTestMatches(
+            expected, (doctest.NORMALIZE_WHITESPACE |
+                       doctest.REPORT_NDIFF | doctest.ELLIPSIS))
+        self.assertThat(markups[0], expected_matcher)
+        expected = (
+            """<input class="radioType" ...
+             onClick="this.form['field.test_field.test_field'].focus()" ...
+             value="other" />&nbsp;<label ...>Other:</label>
+             <input type="text" value="" ...
+             onKeyPress="selectWidget('field.test_field.1', event);"
+             .../>...""")
+        expected_matcher = DocTestMatches(
+            expected, (doctest.NORMALIZE_WHITESPACE |
+                       doctest.REPORT_NDIFF | doctest.ELLIPSIS))
+        self.assertThat(markups[1], expected_matcher)
+
+
 def make_target_branch_widget(branch):
     """Given a branch, return a widget for selecting where to land it."""
     choice = Choice(vocabulary='Branch').bind(branch)