launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/subscription-policy-text-912159 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #912159 in Launchpad itself: "ITeam:+edit does not explain why some subscription policies are hidden"
  https://bugs.launchpad.net/launchpad/+bug/912159

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/subscription-policy-text-912159/+merge/88808

== Implementation ==

The tal template uses a form macro to render all the widgets, including the subscription policy radio buttons, so I decided to extend the LaunchpadRadioWidgetWithDescription widget to allow an optional context specific hint to be rendered. This is done by setting optional attributes on the widget:
- extra_hint
- extra_hint_class

I could have simply added text to the widget's hint text. However, I feel the extra information required here is different to the standard (static) widget hint attribute which is derived from the form field definition. The extra hint value is designed to provide the user with more than just text describing what a field does. It needs to convey info about the available choices and will not be static.

The text for the extra hint comes from the message in the exception raised when checking what vocab to use for the widget.

== Demo and QA ==

See screenshot:
http://people.canonical.com/~ianb/team-subscription-policy-hint.png

I'm not sure if the extra hint text should be rendered the same lighter grey as the other text - opinions welcome.

== Tests ==

Tests were written for the new LaunchpadRadioWidgetWithDescription functionality.
Add to doc test:
- launchpad-radio-widget.txt

Add new test case to TestLaunchpadRadioWidgetWithDescription:
- test_renderExtraHint

Extend the TestTeamEditView tests to check the extra hint attributes have been set (or not) on the widget:
- _test_edit_team_view_expected_subscription_vocab
- test_edit_team_view_data

== Lint ==
Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/widgets/itemswidgets.py
  lib/lp/app/widgets/doc/launchpad-radio-widget.txt
  lib/lp/app/widgets/tests/test_itemswidgets.py
  lib/lp/registry/browser/team.py
  lib/lp/registry/browser/tests/test_team_view.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/subscription-policy-text-912159/+merge/88808
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/subscription-policy-text-912159 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/css/forms.css'
--- lib/canonical/launchpad/icing/css/forms.css	2011-12-12 17:07:23 +0000
+++ lib/canonical/launchpad/icing/css/forms.css	2012-01-17 04:50:28 +0000
@@ -332,6 +332,13 @@
     color: red;
     font-weight: bold;
     }
+.inline-informational {
+    padding: 2px 2px 0 0;
+    }
+.inline-informational::before {
+    content: url('/@@/info');
+    padding-right: 5px;
+    }
 .sml-informational {
     background: #d4e8ff url('/+icing/blue-fade-to-grey');
     border: solid #666;

=== modified file 'lib/lp/app/widgets/doc/launchpad-radio-widget.txt'
--- lib/lp/app/widgets/doc/launchpad-radio-widget.txt	2011-12-24 17:49:30 +0000
+++ lib/lp/app/widgets/doc/launchpad-radio-widget.txt	2012-01-17 04:50:28 +0000
@@ -124,6 +124,19 @@
     </table>
     <input name="field.branch_type-empty-marker" type="hidden" value="1" />
 
+Sometimes, it is desirable to display to the user additional, context specific
+information to explain the choices available for selection. This can be done
+by setting the optional extra_hint and extra_hint_class attributes on the
+widget.
+    >>> radio_widget.extra_hint = 'Some additional information'
+    >>> radio_widget.extra_hint_class = 'inline-informational'
+    >>> print radio_widget()
+    <div class="inline-informational">Some additional information</div>
+    <table class="radio-button-widget"><tr>
+    ...
+    </table>
+    <input name="field.branch_type-empty-marker" type="hidden" value="1" />
+
 
 LaunchpadBooleanRadioWidget
 ---------------------------

=== modified file 'lib/lp/app/widgets/itemswidgets.py'
--- lib/lp/app/widgets/itemswidgets.py	2011-12-24 17:49:30 +0000
+++ lib/lp/app/widgets/itemswidgets.py	2012-01-17 04:50:28 +0000
@@ -152,6 +152,8 @@
             'The vocabulary must implement IEnumeratedType')
         super(LaunchpadRadioWidgetWithDescription, self).__init__(
             field, vocabulary, request)
+        self.extra_hint = None
+        self.extra_hint_class = None
 
     def _renderRow(self, text, form_value, id, elem):
         """Render the table row for the widget depending on description."""
@@ -192,12 +194,25 @@
                              type='radio')
         return self._renderRow(text, value, id, elem)
 
+    def renderExtraHint(self):
+        extra_hint_html = ''
+        extra_hint_class = ''
+        if self.extra_hint_class:
+            extra_hint_class = ' class="%s"' % self.extra_hint_class
+        if self.extra_hint:
+            extra_hint_html = ('<div%s>%s</div>'
+                % (extra_hint_class, escape(self.extra_hint)))
+        return extra_hint_html
+
     def renderValue(self, value):
         # Render the items in a table to align the descriptions.
         rendered_items = self.renderItems(value)
+        extra_hint = self.renderExtraHint()
         return (
-            '<table class="radio-button-widget">%s</table>'
-            % ''.join(rendered_items))
+            '%(extra_hint)s\n'
+            '<table class="radio-button-widget">%(items)s</table>'
+            % {'extra_hint': extra_hint,
+               'items': ''.join(rendered_items)})
 
 
 class LaunchpadBooleanRadioWidget(LaunchpadRadioWidget):

=== modified file 'lib/lp/app/widgets/tests/test_itemswidgets.py'
--- lib/lp/app/widgets/tests/test_itemswidgets.py	2012-01-01 02:58:52 +0000
+++ lib/lp/app/widgets/tests/test_itemswidgets.py	2012-01-17 04:50:28 +0000
@@ -203,3 +203,12 @@
             '<...>item-&lt;2&gt;<...>&lt;unsafe&gt; &amp;nbsp; title<...>')
         self.assertRenderItem(
             expected, self.widget.renderItem, self.TestEnum.UNSAFE_TERM)
+
+    def test_renderExtraHint(self):
+        # If an extra hint is specified, it is rendered.
+        self.widget.extra_hint = "Hello World"
+        self.widget.extra_hint_class = 'hint_class'
+        expected = (
+            '<div class="hint_class">Hello World</div>')
+        hint_html = self.widget.renderExtraHint()
+        self.assertEqual(expected, hint_html)

=== modified file 'lib/lp/registry/browser/team.py'
--- lib/lp/registry/browser/team.py	2012-01-05 21:53:52 +0000
+++ lib/lp/registry/browser/team.py	2012-01-17 04:50:28 +0000
@@ -303,7 +303,7 @@
         # Do we need to only show open subscription policy choices?
         try:
             team.checkClosedSubscriptionPolicyAllowed()
-        except TeamSubscriptionPolicyError:
+        except TeamSubscriptionPolicyError as e:
             # Ideally SimpleVocabulary.fromItems() would accept 3-tuples but
             # it doesn't so we need to be a bit more verbose.
             self.widgets['subscriptionpolicy'].vocabulary = (
@@ -311,10 +311,14 @@
                     policy, policy.name, policy.title)
                     for policy in OPEN_TEAM_POLICY])
                 )
+            self.widgets['subscriptionpolicy'].extra_hint_class = (
+                'inline-informational')
+            self.widgets['subscriptionpolicy'].extra_hint = e.message
+
         # Do we need to only show closed subscription policy choices?
         try:
             team.checkOpenSubscriptionPolicyAllowed()
-        except TeamSubscriptionPolicyError:
+        except TeamSubscriptionPolicyError as e:
             # Ideally SimpleVocabulary.fromItems() would accept 3-tuples but
             # it doesn't so we need to be a bit more verbose.
             self.widgets['subscriptionpolicy'].vocabulary = (
@@ -322,6 +326,9 @@
                     policy, policy.name, policy.title)
                     for policy in CLOSED_TEAM_POLICY])
                 )
+            self.widgets['subscriptionpolicy'].extra_hint_class = (
+                'inline-informational')
+            self.widgets['subscriptionpolicy'].extra_hint = e.message
 
     @action('Save', name='save')
     def action_save(self, action, data):

=== modified file 'lib/lp/registry/browser/tests/test_team_view.py'
--- lib/lp/registry/browser/tests/test_team_view.py	2012-01-01 02:58:52 +0000
+++ lib/lp/registry/browser/tests/test_team_view.py	2012-01-17 04:50:28 +0000
@@ -286,6 +286,11 @@
                 expected_items,
                 [term.value
                  for term in view.widgets['subscriptionpolicy'].vocabulary])
+            self.assertEqual(
+                'inline-informational',
+                view.widgets['subscriptionpolicy'].extra_hint_class)
+            self.assertIsNotNone(
+                view.widgets['subscriptionpolicy'].extra_hint)
 
     def test_edit_team_view_pillar_owner(self):
         # The edit view renders only closed subscription policy choices when