Merge lp:~frankban/juju-quickstart/no-closure-for-you into lp:juju-quickstart

[Francesco Banconi]

Reorganize view code.

Replace most of the view closures with external
private functions.
This has several good side effects in my opinion:
- view functions are shorter;
- we know which objects are used for each callback
  function, because we need to explicitly pass them
  as arguments;
- external functions can be reused by multiple views
  if required (and we have the _use example here);
- if an external callback become less straightforward,
  it is now possible to directly unit test it.

Tests: `make check`.

QA:
If unit tests pass we have some confidence that
the interactive session is not broken.
However, some time spent on it would be great,
creating/editing/destroying environments, setting
them as default, canceling the changes, starting
one of them, etc...
Thanks a lot.

https://codereview.appspot.com/194080043/

[Francesco Banconi (frankban)]

Reviewers: mp+245868_code.launchpad.net,

Message:
Please take a look.

Description:
Reorganize view code.

Replace most of the view closures with external
private functions.
This has several good side effects in my opinion:
- view functions are shorter;
- we know which objects are used for each callback
   function, because we need to explicitly pass them
   as arguments;
- external functions can be reused by multiple views
   if required (and we have the _use example here);
- if an external callback become less straightforward,
   it is now possible to directly unit test it.

Tests: `make check`.

QA:
If unit tests pass we have some confidence that
the interactive session is not broken.
However, some time spent on it would be great,
creating/editing/destroying environments, setting
them as default, canceling the changes, starting
one of them, etc...
Thanks a lot.

https://code.launchpad.net/~frankban/juju-quickstart/no-closure-for-you/+merge/245868

(do not edit description out of merge proposal)

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

Affected files (+140, -101 lines):
   A [revision details]
   M quickstart/cli/views.py

[Madison Scott-Clary (makyo)]

LGTM, no QA yet (if bac doesn't get to it, I can do it later in the day)

https://codereview.appspot.com/194080043/

[Brad Crittenden (bac)]

LGTM. WIll QA now.

https://codereview.appspot.com/194080043/

[Brad Crittenden (bac)]

QA was OK.

I changed defaults, created a new environment, and launched a local env.

https://codereview.appspot.com/194080043/

[Francesco Banconi (frankban)]

*** Submitted:

Reorganize view code.

Replace most of the view closures with external
private functions.
This has several good side effects in my opinion:
- view functions are shorter;
- we know which objects are used for each callback
   function, because we need to explicitly pass them
   as arguments;
- external functions can be reused by multiple views
   if required (and we have the _use example here);
- if an external callback become less straightforward,
   it is now possible to directly unit test it.

Tests: `make check`.

QA:
If unit tests pass we have some confidence that
the interactive session is not broken.
However, some time spent on it would be great,
creating/editing/destroying environments, setting
them as default, canceling the changes, starting
one of them, etc...
Thanks a lot.

R=matthew.scott, bac
CC=
https://codereview.appspot.com/194080043

https://codereview.appspot.com/194080043/

[Francesco Banconi (frankban)]

Thank you both for the reviews!

https://codereview.appspot.com/194080043/

=== modified file 'quickstart/cli/views.py'
--- quickstart/cli/views.py	2015-01-07 16:46:28 +0000
+++ quickstart/cli/views.py	2015-01-08 14:53:36 +0000
@@ -184,24 +184,6 @@
         for env_name in params.env_db['environments']
     ], key=operator.itemgetter('name'))
 
-    def create_and_start_local_env():
-        # Automatically create and use a local environment named "local".
-        # This closure can only be called when there are no environments in the
-        # database. For this reason, the new environment is set as default.
-        # Exit the interactive session selecting the newly created environment.
-        env_data = envs.create_local_env_data(
-            params.env_type_db, 'local', is_default=True)
-        _save_and_exit(params.env_db, env_data, params.save_callable)
-
-    def create_and_start_maas_env(name, server, api_key):
-        # Automatically create and use a MAAS environment.
-        # This closure can only be called when there are no environments in the
-        # database. For this reason, the new environment is set as default.
-        # Exit the interactive session selecting the newly created environment.
-        env_data = envs.create_maas_env_data(
-            params.env_type_db, name, server, api_key, is_default=True)
-        _save_and_exit(params.env_db, env_data, params.save_callable)
-
     platform = platform_support.get_platform()
     supports_local = platform_support.supports_local(platform)
 
@@ -233,7 +215,8 @@
             if maas_api_data is not None:
                 maas_name, maas_server, maas_api_key = maas_api_data
                 maas_callback = ui.thunk(
-                    create_and_start_maas_env,
+                    _create_and_start_maas_env,
+                    params.env_type_db, params.env_db, params.save_callable,
                     maas_name, maas_server, maas_api_key)
                 widgets.extend([
                     urwid.Text([
@@ -254,6 +237,9 @@
         # If the current platform supports local Juju environments, add an
         # option to automatically create and bootstrap one.
         if supports_local:
+            local_callback = ui.thunk(
+                _create_and_start_local_env,
+                params.env_type_db, params.env_db, params.save_callable)
             widgets.extend([
                 urwid.Text([
                     '\nAt the bottom of the page you can find links to '
@@ -264,7 +250,7 @@
                 urwid.Divider(),
                 ui.MenuButton(
                     '\N{BULLET} automatically create and bootstrap a local '
-                    'environment', ui.thunk(create_and_start_local_env)),
+                    'environment', local_callback),
             ])
 
     app.set_title(title)
@@ -369,6 +355,31 @@
     app.set_contents(contents)
 
 
+def _create_and_start_local_env(env_type_db, env_db, save_callable):
+    """Automatically create and use a local environment named "local".
+
+    This function can only be called when there are no environments in the
+    database. For this reason, the new environment is set as default.
+    Exit the interactive session selecting the newly created environment.
+    """
+    env_data = envs.create_local_env_data(
+        env_type_db, 'local', is_default=True)
+    _save_and_exit(env_db, env_data, save_callable)
+
+
+def _create_and_start_maas_env(
+        env_type_db, env_db, save_callable, name, server, api_key):
+    """Automatically create and use a MAAS environment.
+
+    This function can only be called when there are no environments in the
+    database. For this reason, the new environment is set as default.
+    Exit the interactive session selecting the newly created environment.
+    """
+    env_data = envs.create_maas_env_data(
+        env_type_db, name, server, api_key, is_default=True)
+    _save_and_exit(env_db, env_data, save_callable)
+
+
 def env_detail(app, params, env_data):
     """Show details on a Juju environment.
 
@@ -391,40 +402,6 @@
     index_view = functools.partial(env_index, app, params)
     edit_view = functools.partial(env_edit, app, params, env_data)
 
-    def use(env_data):
-        # Quit the interactive session returning the (possibly modified)
-        # environment database and the environment data corresponding to the
-        # selected environment.
-        raise ui.AppExit((params.env_db, env_data))
-
-    def set_default(env_data):
-        # Set this environment as the default one, save the env_db and return
-        # to the index view.
-        env_name = env_data['name']
-        params.env_db['default'] = env_name
-        params.save_callable(params.env_db)
-        app.set_message('{} successfully set as default'.format(env_name))
-        index_view()
-
-    def remove(env_data):
-        # The environment deletion is confirmed: remove the environment from
-        # the database, save the new env_db and return to the index view.
-        env_name = env_data['name']
-        envs.remove_env(params.env_db, env_name)
-        params.save_callable(params.env_db)
-        app.set_message('{} successfully removed'.format(env_name))
-        index_view()
-
-    def confirm_removal(env_data):
-        # Ask confirmation before removing an environment.
-        ui.show_dialog(
-            app, 'Remove the {} environment'.format(env_data['name']),
-            'This action cannot be undone!',
-            actions=[
-                (('control alert', 'confirm'), ui.thunk(remove, env_data)),
-            ],
-        )
-
     env_metadata = envs.get_env_metadata(params.env_type_db, env_data)
     app.set_title(envs.get_env_short_description(env_data))
     # Validate the environment.
@@ -447,21 +424,76 @@
         ])
     else:
         # Without errors, it is possible to use/start this environment.
-        controls.append(ui.MenuButton('use', ui.thunk(use, env_data)))
+        use_callback = ui.thunk(_use, params.env_db, env_data)
+        controls.append(ui.MenuButton('use', use_callback))
     app.set_status(status)
     if not env_data['is-default']:
-        controls.append(
-            ui.MenuButton('set default', ui.thunk(set_default, env_data)))
+        # If the environment is not the default one, it is possible to set it
+        # as default.
+        set_default_callback = ui.thunk(
+            _set_default, params.env_db, env_data, params.save_callable,
+            app.set_message, index_view)
+        controls.append(ui.MenuButton('set default', set_default_callback))
+    # Add the controls to edit and remove the environment.
+    remove_callback = ui.thunk(
+        _remove, params.env_db, env_data, params.save_callable,
+        app.set_message, index_view)
+    confirm_removal_callback = ui.thunk(
+        _confirm_removal, app, env_data, remove_callback)
     controls.extend([
         ui.MenuButton('edit', ui.thunk(edit_view)),
-        ui.MenuButton(
-            ('control alert', 'remove'), ui.thunk(confirm_removal, env_data)),
+        ui.MenuButton(('control alert', 'remove'), confirm_removal_callback),
     ])
     widgets.append(ui.create_controls(*controls))
     listbox = urwid.ListBox(urwid.SimpleFocusListWalker(widgets))
     app.set_contents(listbox)
 
 
+def _use(env_db, env_data):
+    """Use the selected environment.
+
+    Quit the interactive session returning the (possibly modified)
+    environment database and the environment data corresponding to the
+    selected environment.
+    """
+    raise ui.AppExit((env_db, env_data))
+
+
+def _set_default(env_db, env_data, save_callable, set_message, redirect_view):
+    """Set this environment as the default one.
+
+    Save the env_db and return to the given view.
+    Also output a notification using the given set_message callable.
+    """
+    env_name = env_data['name']
+    env_db['default'] = env_name
+    save_callable(env_db)
+    set_message('{} successfully set as default'.format(env_name))
+    redirect_view()
+
+
+def _confirm_removal(app, env_data, remove_callable):
+    """Ask confirmation before removing an environment."""
+    ui.show_dialog(
+        app, 'Remove the {} environment'.format(env_data['name']),
+        'This action cannot be undone!',
+        actions=[(('control alert', 'confirm'), remove_callable)],
+    )
+
+
+def _remove(env_db, env_data, save_callable, set_message, redirect_view):
+    """Remove the environment represented by env_data from the database.
+
+    Save the new env_db and return to the given view.
+    Also output a notification using the given set_message callable.
+    """
+    env_name = env_data['name']
+    envs.remove_env(env_db, env_name)
+    save_callable(env_db)
+    set_message('{} successfully removed'.format(env_name))
+    redirect_view()
+
+
 def jenv_detail(app, params, env_data):
     """Show details on a Juju imported environment.
 
@@ -484,12 +516,6 @@
     app.set_return_value_on_exit((params.env_db, None))
     index_view = functools.partial(env_index, app, params)
 
-    def use(env_data):
-        # Quit the interactive session returning the (possibly modified)
-        # environment database and the environment data corresponding to the
-        # selected environment.
-        raise ui.AppExit((params.env_db, env_data))
-
     app.set_title(jenv.get_env_short_description(env_data))
     widgets = []
     for key, value in jenv.get_env_details(env_data):
@@ -508,7 +534,7 @@
 
     controls = [
         ui.MenuButton('back', ui.thunk(index_view)),
-        ui.MenuButton('use', ui.thunk(use, env_data)),
+        ui.MenuButton('use', ui.thunk(_use, params.env_db, env_data)),
     ]
     widgets.append(ui.create_controls(*controls))
     listbox = urwid.ListBox(urwid.SimpleFocusListWalker(widgets))
@@ -566,39 +592,6 @@
         ' field errors ',
     ])
 
-    def save(env_data, get_new_env_data):
-        # Create a new environment or save changes for an existing one.
-        # The new values are saved only if the new env_data is valid, in which
-        # case also redirect to the environment detail view.
-        new_env_data = get_new_env_data()
-        # Validate the new env_data.
-        errors = envs.validate(env_metadata, new_env_data)
-        new_name = new_env_data['name']
-        initial_name = env_data.get('name')
-        env_db = params.env_db
-        if (new_name != initial_name) and new_name in env_db['environments']:
-            errors['name'] = 'an environment with this name already exists'
-        # If errors are found, re-render the form passing the errors. This way
-        # the errors are displayed as part of the form and the user is given
-        # the opportunity to fix the invalid values.
-        if errors:
-            return render_form(new_env_data, errors)
-        # Without errors, normalize the new values, update the env_db and save
-        # the resulting environments database.
-        env_data = envs.normalize(env_metadata, new_env_data)
-        # If this is the only environment in the db, set it as the default one.
-        if not env_db['environments']:
-            env_data['is-default'] = True
-        envs.set_env_data(env_db, initial_name, env_data)
-        params.save_callable(env_db)
-        verb = 'modified' if exists else 'created'
-        app.set_message('{} successfully {}'.format(new_name, verb))
-        return detail_view(env_data)
-
-    def cancel(env_data):
-        # Dismiss any changes and return to the index or detail view.
-        return detail_view(env_data) if exists else index_view()
-
     def render_form(data, errors):
         # Render the environment edit form.
         widgets = [
@@ -612,9 +605,15 @@
         form_widgets, get_new_env_data = forms.create_form(
             field_value_pairs, errors)
         widgets.extend(form_widgets)
+        save_callback = ui.thunk(
+            _save, params.env_db, env_data, env_metadata, params.save_callable,
+            exists, get_new_env_data, render_form, app.set_message,
+            detail_view)
+        cancel_callback = ui.thunk(
+            _cancel, env_data, exists, index_view, detail_view)
         actions = (
-            ('save', ui.thunk(save, env_data, get_new_env_data)),
-            ('cancel', ui.thunk(cancel, env_data)),
+            ('save', save_callback),
+            ('cancel', cancel_callback),
             ('restore', ui.thunk(render_form, env_data, initial_errors)),
         )
         widgets.append(forms.create_actions(actions))
@@ -624,3 +623,41 @@
 
     # Render the initial form.
     render_form(env_data, initial_errors)
+
+
+def _save(
+        env_db, env_data, env_metadata, save_callable, exists,
+        get_new_env_data, render_form, set_message, redirect_view):
+    """Create a new environment or save changes for an existing one.
+
+    The new values are saved only if the new env_data is valid, in which
+    case also redirect to the given view.
+    """
+    new_env_data = get_new_env_data()
+    # Validate the new env_data.
+    errors = envs.validate(env_metadata, new_env_data)
+    new_name = new_env_data['name']
+    initial_name = env_data.get('name')
+    if (new_name != initial_name) and new_name in env_db['environments']:
+        errors['name'] = 'an environment with this name already exists'
+    # If errors are found, re-render the form passing the errors. This way
+    # the errors are displayed as part of the form and the user is given
+    # the opportunity to fix the invalid values.
+    if errors:
+        return render_form(new_env_data, errors)
+    # Without errors, normalize the new values, update the env_db and save
+    # the resulting environments database.
+    env_data = envs.normalize(env_metadata, new_env_data)
+    # If this is the only environment in the db, set it as the default one.
+    if not env_db['environments']:
+        env_data['is-default'] = True
+    envs.set_env_data(env_db, initial_name, env_data)
+    save_callable(env_db)
+    verb = 'modified' if exists else 'created'
+    set_message('{} successfully {}'.format(new_name, verb))
+    return redirect_view(env_data)
+
+
+def _cancel(env_data, exists, index_view, detail_view):
+    """Dismiss any changes and return to the index or detail view."""
+    return detail_view(env_data) if exists else index_view()