Merge lp:~frankban/juju-quickstart/improve-field-choices into lp:juju-quickstart

[Francesco Banconi]

Improve field choices handling.

Implement clickable choices: the user can update
the corresponding edit widget by clicking choices.

Also changed the default series field behavior so
that unset values default to "precise".

Tests: `make check`.

QA: start the demo app
(`make` and `./cli-app-demo.py).
Use it to edit existing environments and
to create new ones (ec2 and local).
Check that choice fields
(e.g. ec2's regions or default series)
can be updated by clicking the choices.
Also check that, if the default series
is not set, it defaults to "precise".

https://codereview.appspot.com/34630044/

[Francesco Banconi (frankban)]

Reviewers: mp+200658_code.launchpad.net,

Message:
Please take a look.

Description:
Improve field choices handling.

Implement clickable choices: the user can update
the corresponding edit widget by clicking choices.

Also changed the default series field behavior so
that unset values default to "precise".

Tests: `make check`.

QA: start the demo app
(`make` and `./cli-app-demo.py).
Use it to edit existing environments and
to create new ones (ec2 and local).
Check that choice fields
(e.g. ec2's regions or default series)
can be updated by clicking the choices.
Also check that, if the default series
is not set, it defaults to "precise".

https://code.launchpad.net/~frankban/juju-quickstart/improve-field-choices/+merge/200658

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/34630044/

Affected files (+236, -22 lines):
   A [revision details]
   M quickstart/cli/forms.py
   M quickstart/models/envs.py
   M quickstart/models/fields.py
   M quickstart/tests/cli/test_forms.py
   M quickstart/tests/models/test_fields.py

[Gary Poster (gary)]

QA first:

- Very cool. Looks good generally.

- It looks like Urwid can support an "underline" setting. I think that
would make it easier to see the clickable help text (for both
autogeneration and these choices). WDYT?

- In my default terminal width (80x24) I see "possible values are:
precise, quantal, raring, saucy". If I use the arrow keys then I can
see there is a final option (", but this field can also be _left
empty_"). With the mouse I am out of luck. I'd like to improve that if
we could. Options include letting the line wrap, if Urwid supports
that; showing a clickable "MORE->" or "..." or something; or simply
trying to reduce the length of the clickable text.

In line with the last, simplest idea, what about replacing the four
lines with something like the following? I'm assuming we can make the
first text wrappable easily, but if not maybe this idea needs tweaking.

To select a value, type above, _clear the field_ to get the default
value (precise), or click one of the following choices:
_precise_, _quantal_, _raring_, _saucy_, _trusty_

Mm. Darn. I see in the region field that trying to reduce the length
of the clickable text is insufficient. Those options go on for days.
That will need line wrap or something else similar

- Relatedly, and probably for the future (and maybe never), it would be
nice if the fields that can be automatically generated are allowed to be
empty, which defaults to using the autogeneration.

- I see that we actually explicitly set precise in the file if it is
empty (as you describe in the cover letter). However, IIUC, Juju will
work fine if you do not specify a series. The current *Juju* default
behavior is to default to precise, but probably will change to
defaulting to trusty eventually. That makes me think for our
opinionated usage that we should begin rendering the field with
"precise" but if someone explicitly deletes the field then it is simply
empty (and the "empty" semantics are described in the fields help text).
  WDYT?

Thank you!

https://codereview.appspot.com/34630044/

[lp:~frankban/juju-quickstart/improve-field-choices]

48. By Francesco Banconi on 2014-01-07

Some changes as per review.

49. By Francesco Banconi on 2014-01-07

Checkpoint.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/34630044/

[Gary Poster (gary)]

LGTM. Thank you!

https://codereview.appspot.com/34630044/diff/20001/quickstart/tests/models/test_fields.py
File quickstart/tests/models/test_fields.py (left):

https://codereview.appspot.com/34630044/diff/20001/quickstart/tests/models/test_fields.py#oldcode443
quickstart/tests/models/test_fields.py:443: # in the specified choices/
Sorry for not catching this and other similar ones earlier.

https://codereview.appspot.com/34630044/

[lp:~frankban/juju-quickstart/improve-field-choices]

50. By Francesco Banconi on 2014-01-08

Checkpoint.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/34630044/

[Francesco Banconi (frankban)]

On 2014/01/07 13:50:48, gary.poster wrote:
> - It looks like Urwid can support an "underline" setting. I think
that would
> make it easier to see the clickable help text (for both autogeneration
and these
> choices). WDYT?

Sounds good, done.

> - In my default terminal width (80x24) I see "possible values are:
precise,
> quantal, raring, saucy". If I use the arrow keys then I can see there
is a
> final option (", but this field can also be _left empty_"). With the
mouse I am
> out of luck. I'd like to improve that if we could. Options include
letting the
> line wrap, if Urwid supports that; showing a clickable "MORE->" or
"..." or
> something; or simply trying to reduce the length of the clickable
text.

> In line with the last, simplest idea, what about replacing the four
lines with
> something like the following? I'm assuming we can make the first text
wrappable
> easily, but if not maybe this idea needs tweaking.

> To select a value, type above, _clear the field_ to get the default
value
> (precise), or click one of the following choices:
> _precise_, _quantal_, _raring_, _saucy_, _trusty_

> Mm. Darn. I see in the region field that trying to reduce the length
of the
> clickable text is insufficient. Those options go on for days. That
will need
> line wrap or something else similar

Fixed using a GridFlow in place of the Columns widget (which does not
support automatic line wrapping). This way the choices adapt better to
the screen size.

> - I see that we actually explicitly set precise in the file if it is
empty (as
> you describe in the cover letter). However, IIUC, Juju will work fine
if you do
> not specify a series. The current *Juju* default behavior is to
default to
> precise, but probably will change to defaulting to trusty eventually.
That
> makes me think for our opinionated usage that we should begin
rendering the
> field with "precise" but if someone explicitly deletes the field then
it is
> simply empty (and the "empty" semantics are described in the fields
help text).
> WDYT?

I think the current behavior is wrong and what you suggested is better.
I was wrong thinking the default series (if unset) is the host series.
Fixed, now we pre-fill precise when creating a new environment.

Thank you!

https://codereview.appspot.com/34630044/

[Gary Poster (gary)]

QA great. ;-)

Thanks!

https://codereview.appspot.com/34630044/

[Francesco Banconi (frankban)]

*** Submitted:

Improve field choices handling.

Implement clickable choices: the user can update
the corresponding edit widget by clicking choices.

Also changed the default series field behavior so
that unset values default to "precise".

Tests: `make check`.

QA: start the demo app
(`make` and `./cli-app-demo.py).
Use it to edit existing environments and
to create new ones (ec2 and local).
Check that choice fields
(e.g. ec2's regions or default series)
can be updated by clicking the choices.
Also check that, if the default series
is not set, it defaults to "precise".

R=gary.poster
CC=
https://codereview.appspot.com/34630044

https://codereview.appspot.com/34630044/

=== modified file 'quickstart/cli/forms.py'
--- quickstart/cli/forms.py	2014-01-02 17:04:25 +0000
+++ quickstart/cli/forms.py	2014-01-08 12:01:09 +0000
@@ -48,7 +48,7 @@
     """
     generate_callback = ui.thunk(_generate, generate_callable, edit_widget)
     generate_button = ui.MenuButton(
-        ('highlight', 'Click here to automatically generate'),
+        ('field button', 'Click here to automatically generate'),
         generate_callback)
     return urwid.Columns([
         ('pack', generate_button),
@@ -56,6 +56,32 @@
     ])
 
 
+def _create_choices_widget(choices, required, edit_widget):
+    """Create and return a choices widget.
+
+    The widget displays the given choices for a specific form field. Clicking a
+    choice updates the corresponding edit widget.
+    """
+    widgets = [urwid.Text('possible values are:')]
+    callback = edit_widget.set_edit_text
+    buttons = [
+        ui.MenuButton(('field button', choice), ui.thunk(callback, choice))
+        for choice in choices
+    ]
+    cell_width = max(button.base_widget.pack()[0] for button in buttons)
+    widgets.append(urwid.GridFlow(buttons, cell_width, 1, 0, 'left'))
+    if not required:
+        button = ui.MenuButton(
+            ('field button', 'left empty'), ui.thunk(callback, ''))
+        widgets.append(urwid.Columns([
+            ('pack', urwid.Text('but this field can also be ')), button,
+        ]))
+    help = urwid.Text(
+        'click the choices to auto-fill the field with the standard options')
+    widgets.append(help)
+    return urwid.Pile(widgets)
+
+
 def create_string_widget(field, value, error):
     """Create a string widget and return a tuple (widget, value_getter).
 
@@ -101,12 +127,9 @@
     # as part of the help message.
     choices = getattr(field, 'choices', None)
     if choices is not None:
-        # XXX frankban 2014-01-02: make the choices clickable and update the
-        # edit widget when a choice is clicked.
-        text = 'possible values are {}'.format(', '.join(choices))
-        if not field.required:
-            text = '{}, but this field can also be left empty'.format(text)
-        widgets.append(urwid.Text(text))
+        choices_widget = _create_choices_widget(
+            tuple(choices), field.required, edit_widget)
+        widgets.append(choices_widget)
     if field.default is not None:
         widgets.append(
             urwid.Text('default if not set: {}'.format(field.default)))
 
=== modified file 'quickstart/cli/ui.py'
--- quickstart/cli/ui.py	2014-01-03 13:32:25 +0000
+++ quickstart/cli/ui.py	2014-01-08 12:01:09 +0000
@@ -39,6 +39,8 @@
     ('error', 'light red', 'black'),
     ('error status', 'light red', 'light gray'),
     ('error selected', 'light red', 'dark blue'),
+    ('field button', 'white,underline', 'black'),
+    ('field button selected', 'white,underline', 'dark blue'),
     ('footer', 'black', 'light gray'),
     ('message', 'white', 'dark green'),
     ('header', 'white', 'dark magenta'),
@@ -54,6 +56,7 @@
     None: 'selected',
     'control alert': 'error selected',
     'error': 'error selected',
+    'field button': 'field button selected',
     'highlight': 'selected',
 }
 # Define a default padding for the Urwid application.
 
=== modified file 'quickstart/cli/views.py'
--- quickstart/cli/views.py	2014-01-03 13:55:57 +0000
+++ quickstart/cli/views.py	2014-01-08 12:01:09 +0000
@@ -102,6 +102,7 @@
 
 import urwid
 
+from quickstart import settings
 from quickstart.cli import (
     base,
     forms,
@@ -188,10 +189,14 @@
             'highlight', 'Use the links below to create new environments')),
         urwid.Divider(),
     ])
+    # The Juju GUI can be safely installed in the bootstrap node only if its
+    # series is "precise". Suggest this setting by pre-filling the value.
+    preferred_series = settings.JUJU_GUI_PREFERRED_SERIES
     widgets.extend([
         ui.MenuButton(
             ['\N{BULLET} new ', ('highlight', env_type), ' environment'],
-            ui.thunk(edit_view, {'type': env_type})
+            ui.thunk(edit_view, {
+                'type': env_type, 'default-series': preferred_series})
         )
         for env_type in envs.get_supported_env_types(env_type_db)
     ])
@@ -317,7 +322,7 @@
 
     The last value (env_data) indicates whether this view is used to create a
     new environment or to change an existing one. In the former case, env_data
-    only includes the "type" key. If instead the environment already exists,
+    does not include the "name" key. If instead the environment already exists,
     env_data includes the "name" key and all the other environment info.
     """
     env_db = copy.deepcopy(env_db)
 
=== modified file 'quickstart/models/envs.py'
--- quickstart/models/envs.py	2014-01-03 11:38:43 +0000
+++ quickstart/models/envs.py	2014-01-08 12:01:09 +0000
@@ -334,6 +334,7 @@
     default_series_field = fields.ChoiceField(
         'default-series', choices=settings.JUJU_DEFAULT_SERIES,
         label='default series', required=False,
+        default=settings.JUJU_GUI_PREFERRED_SERIES,
         help='the default Ubuntu series to use for the bootstrap node')
     is_default_field = fields.BoolField(
         'is-default', label='default', allow_mixed=False, required=True,
 
=== modified file 'quickstart/tests/cli/test_forms.py'
--- quickstart/tests/cli/test_forms.py	2014-01-02 17:04:25 +0000
+++ quickstart/tests/cli/test_forms.py	2014-01-08 12:01:09 +0000
@@ -56,7 +56,89 @@
         mock_edit_widget.set_edit_text.assert_called_once_with('generated')
 
 
-class TestCreateStringWidget(unittest.TestCase):
+class ChoicesTestsMixin(object):
+    """Helpers for inspecting the choices widget."""
+
+    def get_buttons(self, choices_widget):
+        """Return the MenuButton instances included in the choices_widget."""
+        # The grid widget including choices is the second widget in the pile.
+        grid = choices_widget.contents[1][0]
+        buttons = [
+            widget for widget, _ in grid.contents
+            if isinstance(widget, ui.MenuButton)
+        ]
+        # Check if the unset button is present.
+        columns_or_text = choices_widget.contents[2][0]
+        if isinstance(columns_or_text, urwid.Columns):
+            buttons.append(columns_or_text.contents[1][0])
+        return buttons
+
+    def assert_choices(self, expected_choices, choices_widget):
+        """Ensure the given choices widget is well formed.
+
+        The message displayed should equal the given expected_message.
+        """
+        obtained_choices = [
+            cli_helpers.get_button_caption(button) for
+            button in self.get_buttons(choices_widget)
+        ]
+        self.assertEqual(expected_choices, obtained_choices)
+
+
+class TestCreateChoicesWidget(ChoicesTestsMixin, unittest.TestCase):
+
+    def setUp(self):
+        # Set up a mock edit widget and choices.
+        self.mock_edit_widget = mock.Mock()
+        self.choices = ['Kirk', 'Picard', 'Sisko']
+
+    def test_widget_message(self):
+        # The resulting widget includes all the choices.
+        widget = forms._create_choices_widget(
+            self.choices, True, self.mock_edit_widget)
+        # The resulting pile widget is composed by choices and help message.
+        self.assert_choices(self.choices, widget)
+        help_widget = widget.contents[2][0]
+        self.assertEqual(
+            'click the choices to auto-fill the field with the '
+            'standard options',
+            help_widget.text)
+
+    def test_widget_buttons(self):
+        # The widget includes a button for each choice.
+        widget = forms._create_choices_widget(
+            self.choices, True, self.mock_edit_widget)
+        buttons = self.get_buttons(widget)
+        self.assertEqual(3, len(buttons))
+        self.assertEqual(
+            self.choices, map(cli_helpers.get_button_caption, buttons))
+
+    def test_choice_click(self):
+        # Clicking a choice updates the edit widget.
+        widget = forms._create_choices_widget(
+            self.choices, True, self.mock_edit_widget)
+        buttons = self.get_buttons(widget)
+        for button in buttons:
+            choice = cli_helpers.get_button_caption(button)
+            # Click the button.
+            cli_helpers.emit(button)
+            # Ensure the edit widget has been updated accordingly.
+            self.mock_edit_widget.set_edit_text.assert_called_with(choice)
+
+    def test_not_required(self):
+        # If the field is not required, an option to unset the field is
+        # displayed.
+        widget = forms._create_choices_widget(
+            self.choices, False, self.mock_edit_widget)
+        self.assert_choices(self.choices + ['left empty'], widget)
+        buttons = self.get_buttons(widget)
+        # The last button is used to unset the edit widget.
+        self.assertEqual(4, len(buttons))
+        cli_helpers.emit(buttons[-1])
+        self.mock_edit_widget.set_edit_text.assert_called_once_with('')
+
+
+class TestCreateStringWidget(ChoicesTestsMixin, unittest.TestCase):
 
     def inspect_widget(self, widget, field):
         """Return a list of sub-widgets composing the given string widget.
@@ -165,10 +247,7 @@
         field = fields.ChoiceField('captain', choices=('Kirk', 'Picard'))
         widget, _ = forms.create_string_widget(field, 'Luc', 'invalid name')
         choices = self.inspect_widget(widget, field)[6]
-        self.assertEqual(
-            'possible values are Kirk, Picard, '
-            'but this field can also be left empty',
-            choices.text)
+        self.assert_choices(['Kirk', 'Picard', 'left empty'], choices)
 
     def test_choices_required_field(self):
         # Possible choices are properly displayed below the edit widget for
@@ -177,7 +256,7 @@
             'captain', required=True, choices=('Janeway', 'Sisko'))
         widget, _ = forms.create_string_widget(field, 'Luc', 'invalid name')
         choices = self.inspect_widget(widget, field)[6]
-        self.assertEqual('possible values are Janeway, Sisko', choices.text)
+        self.assert_choices(['Janeway', 'Sisko'], choices)
 
 
 class TestCreateBoolWidget(unittest.TestCase):
 
=== modified file 'quickstart/tests/cli/test_views.py'
--- quickstart/tests/cli/test_views.py	2014-01-03 14:13:34 +0000
+++ quickstart/tests/cli/test_views.py	2014-01-08 12:01:09 +0000
@@ -24,6 +24,7 @@
 import mock
 import urwid
 
+from quickstart import settings
 from quickstart.cli import (
     base,
     forms,
@@ -209,7 +210,8 @@
             cli_helpers.emit(button)
             mock_env_edit.assert_called_once_with(
                 self.app, self.env_type_db, env_db, self.save_callable,
-                {'type': env_type})
+                {'type': env_type,
+                 'default-series': settings.JUJU_GUI_PREFERRED_SERIES})
             # Reset the mock so that it does not include any calls on the next
             # loop cycle.
             mock_env_edit.reset_mock()
@@ -457,8 +459,8 @@
     def call_view(self, env_name='lxc', env_type=None):
         """Call the view passing the env_data corresponding to env_name.
 
-        If env_type provider, the view is a creation form, env_name is ignored
-        and a new env_data is passed to the view.
+        If env_type is provided, the view is a creation form, env_name is
+        ignored and a new env_data is passed to the view.
         """
         if env_type is None:
             self.env_data = envs.get_env_data(self.env_db, env_name)
 
=== modified file 'quickstart/tests/models/test_fields.py'
--- quickstart/tests/models/test_fields.py	2013-12-17 11:39:55 +0000
+++ quickstart/tests/models/test_fields.py	2014-01-08 12:01:09 +0000
@@ -440,7 +440,7 @@
 
     def test_validation_error_not_in_choices(self):
         # A ValueError is raised by choice fields if the value is not included
-        # in the specified choices/
+        # in the specified choices.
         field = self.field_class(
             'word', choices=self.choices, label='selected word')
         expected = ('the selected word requires the value to be one of the '