Merge lp:~wallyworld/launchpad/subscription-policy-text-912159 into lp:launchpad

[Ian Booth]

[r=sinzui][bug=912159] Add informative text to team edit page explaining why subscription policy choices are limited.

== 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

[Curtis Hovey (sinzui)]

I like the implementation (I really like the reuse of the exception). Why did you define the 'inline-informational' class? It looks like "sprite info"...sprite is inline and info is informational. Did you try that and was the layout broken?

review: Needs Information (code + ui)

[Curtis Hovey (sinzui)]

We talked about this and your can land it as is. You can try "sprite info" and if it is 100% compatible the CSS addition can be reverted.

review: Approve (code)

[Ian Booth (wallyworld)]

"sprite info" works great, thanks for pointing it out.

=== 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-18 00:19:25 +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-18 00:19:25 +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-18 00:19:25 +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-18 00:19:25 +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 = (
+                'sprite info')
+            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 = (
+                'sprite info')
+            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-18 00:19:25 +0000
@@ -266,6 +266,7 @@
             self.assertEqual(
                 TeamSubscriptionPolicy,
                 view.widgets['subscriptionpolicy'].vocabulary)
+            self.assertIsNone(view.widgets['subscriptionpolicy'].extra_hint)
             self.assertEqual(
                 TeamMembershipRenewalPolicy.NONE,
                 view.widgets['renewal_policy']._data)
@@ -286,6 +287,11 @@
                 expected_items,
                 [term.value
                  for term in view.widgets['subscriptionpolicy'].vocabulary])
+            self.assertEqual(
+                'sprite info',
+                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