Merge lp:~frankban/juju-quickstart/env-manage-remove-environment into lp:juju-quickstart

[Francesco Banconi]

Implement the remove env functionality.

It is now possible to remove environments
using the interactive session.

A confirmation dialog prevents the user
to accidentally remove an environment.

Tests: `make check`.
QA: start the demo app
(`make` and `./cli-app-demo.py).
Remove environments at will: exiting
from the app you should see the
new environments YAML without the
environments you deleted.
Any UX suggestion welcome.
Thank you!

https://codereview.appspot.com/44310043/

[Francesco Banconi (frankban)]

Reviewers: mp+199698_code.launchpad.net,

Message:
Please take a look.

Description:
Implement the remove env functionality.

It is now possible to remove environments
using the interactive session.

A confirmation dialog prevents the user
to accidentally remove an environment.

Tests: `make check`.
QA: start the demo app
(`make` and `./cli-app-demo.py).
Remove environments at will: exiting
from the app you should see the
new environments YAML without the
environments you deleted.
Any UX suggestion welcome.
Thank you!

https://code.launchpad.net/~frankban/juju-quickstart/env-manage-remove-environment/+merge/199698

(do not edit description out of merge proposal)

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

Affected files (+306, -30 lines):
   A [revision details]
   M quickstart/cli/ui.py
   M quickstart/cli/views.py
   M quickstart/models/envs.py
   M quickstart/tests/cli/helpers.py
   M quickstart/tests/cli/test_ui.py
   M quickstart/tests/cli/test_views.py
   M quickstart/tests/models/test_envs.py

[Gary Poster (gary)]

LGTM.

The warning says "This action cannot be undone" but really it won't take
effect if the person exits early (^X), right? Because we only will save
at the end? If I'm right, maybe the following would work?

"""
Remove the private-cloud-errors-environment

This change will be saved once you choose an environment to deploy. The
action cannot be undone.
"""

I deleted my default. What is supposed to happen then? You don't have
a default anymore? (That's what happens now.)

QA good with those notes.

Thanks!

Gary

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py#newcode129
quickstart/cli/ui.py:129: original_contents = app.get_contents()
The way you can use this with the dismiss thunk is cool. Feels nice.

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/views.py#newcode277
quickstart/cli/views.py:277: ('control alert', 'remove'),
ui.thunk(confirm_removal, env_data)),
cool. I really like how this and the show_dialog actions work.

https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.py
File quickstart/tests/cli/helpers.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.py#newcode39
quickstart/tests/cli/helpers.py:39: def emit(self, widget):
Don't you have this in another base class? If so, maybe this ought to
be a standalone function? it does not require "self."

https://codereview.appspot.com/44310043/

[Brad Crittenden (bac)]

LGTM

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py#newcode125
quickstart/cli/ui.py:125: the function tries to generate suitable
defaults.
If the units are pixels please say so.

https://codereview.appspot.com/44310043/

[Francesco Banconi (frankban)]

On 2013/12/19 19:11:13, gary.poster wrote:
> LGTM.

> The warning says "This action cannot be undone" but really it won't
take effect
> if the person exits early (^X), right? Because we only will save at
the end?
> If I'm right, maybe the following would work?

In the current implementation, you pass a save_callable to the views,
which is
called when required, e.g. when the user removes an environment.
The save_callable accepts an env_db and it's not yet implemented:
it will likely be something like functools.partial(envs.save, env_file).
We can decide to change this, but I think this strategy gives us two
things:
1) the UX is less surprising for the user: when he performs an action,
that
action is done, and not just scheduled;
2) it is possible to use the quickstart interactive session to manage
environments without starting Juju: the user starts quickstart,
creates/edits/removes environments and then exits (^X).

> """
> Remove the private-cloud-errors-environment

> This change will be saved once you choose an environment to deploy.
The action
> cannot be undone.
> """

> I deleted my default. What is supposed to happen then? You don't
have a
> default anymore? (That's what happens now.)

Yes, in that case there is no longer a default in the environments.yaml
file, and
I have to check if juju-core is ok with this. If not, we need to change
this behavior. It's also important to define the concept of "default"
in the quickstart context: for now in Urwid it is just the YAML default,
but
in the real world it can be overwritten by juju switch (which, in turn,
looks for that value in the JUJU_ENV env var and in the current_env
file,
the latter being an implementation detail). When you run quickstart
without specifying -e, the default environment from juju switch is
started.
In the interactive session, the default environment is instead the one
in the
YAML file, and that's because the interactive session is intended to be
a CRUD for envs.yaml. We might want to either unify those two concepts
or
keep them separate and show both in the UX.

> QA good with those notes.

> Thanks!

Thank you!

https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.py#newcode39
> quickstart/tests/cli/helpers.py:39: def emit(self, widget):
> Don't you have this in another base class? If so, maybe this ought to
be a
> standalone function? it does not require "self."

We had that, but now it is abstracted in this Mixin, used by
both test cases. That said, you are right, this does not
require self, so it can be just a function. The same for
get_button_caption and inspect_dialog. Fixed.

https://codereview.appspot.com/44310043/

[lp:~frankban/juju-quickstart/env-manage-remove-environment]

52. By Francesco Banconi on 2013-12-20

Changes as per review.

[Francesco Banconi (frankban)]

*** Submitted:

Implement the remove env functionality.

It is now possible to remove environments
using the interactive session.

A confirmation dialog prevents the user
to accidentally remove an environment.

Tests: `make check`.
QA: start the demo app
(`make` and `./cli-app-demo.py).
Remove environments at will: exiting
from the app you should see the
new environments YAML without the
environments you deleted.
Any UX suggestion welcome.
Thank you!

R=gary.poster, bac
CC=
https://codereview.appspot.com/44310043

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py#newcode125
quickstart/cli/ui.py:125: the function tries to generate suitable
defaults.
On 2013/12/19 20:29:17, bac wrote:
> If the units are pixels please say so.

Those are Urwid specific values: if you pass
a number, the units are rows/columns, but you
can provide also other values: see
http://excess.org/urwid/docs/reference/widget.html#overlay
Updated the docstring.

https://codereview.appspot.com/44310043/

[Francesco Banconi (frankban)]

Thank you both for the reviews!

https://codereview.appspot.com/44310043/

[Gary Poster (gary)]

On 12/20/2013 05:24 AM, Francesco Banconi wrote:
> On 2013/12/19 19:11:13, gary.poster wrote:
>> LGTM.
>
>> The warning says "This action cannot be undone" but really it won't
> take effect
>> if the person exits early (^X), right? Because we only will save at
> the end?
>> If I'm right, maybe the following would work?
>
> In the current implementation, you pass a save_callable to the views,
> which is
> called when required, e.g. when the user removes an environment.
> The save_callable accepts an env_db and it's not yet implemented:
> it will likely be something like functools.partial(envs.save, env_file).
> We can decide to change this, but I think this strategy gives us two
> things:
> 1) the UX is less surprising for the user: when he performs an action,
> that
> action is done, and not just scheduled;

+1. I misunderstood the plan.

> 2) it is possible to use the quickstart interactive session to manage
> environments without starting Juju: the user starts quickstart,
> creates/edits/removes environments and then exits (^X).
>
>
>> """
>> Remove the private-cloud-errors-environment
>
>> This change will be saved once you choose an environment to deploy.
> The action
>> cannot be undone.
>> """
>
>> I deleted my default. What is supposed to happen then? You don't
> have a
>> default anymore? (That's what happens now.)
>
> Yes, in that case there is no longer a default in the environments.yaml
> file, and
> I have to check if juju-core is ok with this. If not, we need to change
> this behavior. It's also important to define the concept of "default"
> in the quickstart context: for now in Urwid it is just the YAML default,
> but
> in the real world it can be overwritten by juju switch (which, in turn,
> looks for that value in the JUJU_ENV env var and in the current_env
> file,
> the latter being an implementation detail). When you run quickstart
> without specifying -e, the default environment from juju switch is
> started.
> In the interactive session, the default environment is instead the one
> in the
> YAML file, and that's because the interactive session is intended to be
> a CRUD for envs.yaml. We might want to either unify those two concepts
> or
> keep them separate and show both in the UX.

Ah, yeah, that's a bit confusing (in Juju itself).

>
>
>> QA good with those notes.
>
>> Thanks!
>
> Thank you!
>
>
>
> https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.py#newcode39
>> quickstart/tests/cli/helpers.py:39: def emit(self, widget):
>> Don't you have this in another base class? If so, maybe this ought to
> be a
>> standalone function? it does not require "self."
>
> We had that, but now it is abstracted in this Mixin, used by
> both test cases. That said, you are right, this does not
> require self, so it can be just a function. The same for
> get_button_caption and inspect_dialog. Fixed.
>
>
>
> https://codereview.appspot.com/44310043/
>

=== modified file 'quickstart/cli/ui.py'
--- quickstart/cli/ui.py	2013-12-18 11:34:12 +0000
+++ quickstart/cli/ui.py	2013-12-20 10:30:54 +0000
@@ -31,6 +31,9 @@
     # See <http://excess.org/urwid/docs/reference/constants.html
     # foreground-and-background-colors>.
     (None, 'light gray', 'black'),
+    ('dialog', 'dark gray', 'light gray'),
+    ('dialog header', 'light gray,bold', 'dark blue'),
+    ('control alert', 'light red', 'light gray'),
     ('controls', 'dark gray', 'light gray'),
     ('edit', 'white,underline', 'black'),
     ('error', 'light red', 'black'),
@@ -47,6 +50,7 @@
 # Map attributes to new attributes to apply when the widget is selected.
 FOCUS_MAP = {
     None: 'selected',
+    'control alert': 'error selected',
     'error': 'error selected',
 }
 # Define a default padding for the Urwid application.
@@ -100,6 +104,69 @@
         self._w = urwid.AttrMap(icon, None, FOCUS_MAP)
 
 
+def show_dialog(
+        app, title, message, actions=None, dismissable=True,
+        width=None, height=None):
+    """Display an interactive modal dialog.
+
+    This function receives the following arguments:
+        - app: the App named tuple used to configure the current interactive
+          session (see the quickstart.cli.base module);
+        - title: the title of the message dialog;
+        - message: the help message displayed in the dialog;
+        - actions: the actions which can be executed from the dialog, as a
+          sequence of (caption, callback) tuples. Those pairs are used to
+          generate the clickable controls (MenuButton instances) displayed
+          in the dialog;
+        - dismissable: if set to True, a "cancel" button is prepended to the
+          list of controls. Clicking the "cancel" button, the dialog just
+          disappears without further changes to the state of the application;
+        - width and height: optional dialog width and height as defined in
+          Urwid, e.g. 'pack', 'relative' or the number of rows/columns.
+          If not provided, the function tries to generate suitable defaults.
+
+    Return a function that can be called to dismiss the dialog.
+    """
+    original_contents = app.get_contents()
+    # Set up the dialog's header.
+    header = urwid.Pile([
+        urwid.Divider(),
+        urwid.Text(title, align='center'),
+        urwid.Divider(),
+    ])
+    # Set up the controls displayed in the dialog.
+    if actions is None:
+        controls = []
+    else:
+        controls = [
+            MenuButton(caption, callback) for caption, callback in actions]
+    # The dialog is removed by restoring the view original contents.
+    dismiss = thunk(app.set_contents, original_contents)
+    if dismissable:
+        controls.insert(0, MenuButton('cancel', dismiss))
+    # Create the listbox that replaces the original view contents.
+    widgets = [
+        urwid.AttrMap(header, 'dialog header'),
+        urwid.Divider(),
+        urwid.Text(message, align='center'),
+        create_controls(*controls),
+    ]
+    listbox = urwid.ListBox(urwid.SimpleFocusListWalker(widgets))
+    if width is None:
+        # Calculate the dialog width: the max from the title/message length +
+        # two padding spaces for each side.
+        width = max(len(title), len(message)) + 4
+    if height is None:
+        # A height of eleven is usually enough for the dialog.
+        height = 11
+    contents = urwid.Overlay(
+        urwid.AttrMap(listbox, 'dialog'), original_contents,
+        align='center', width=width,
+        valign='middle', height=height)
+    app.set_contents(contents)
+    return dismiss
+
+
 def thunk(function, *args, **kwargs):
     """Create and return a callable binding the given method and args/kwargs.
 

=== modified file 'quickstart/cli/views.py'
--- quickstart/cli/views.py	2013-12-19 11:10:46 +0000
+++ quickstart/cli/views.py	2013-12-20 10:30:54 +0000
@@ -228,6 +228,23 @@
         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.
+        envs.remove_env(env_db, env_data['name'])
+        save_callable(env_db)
+        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(env_type_db, env_data)
     app.set_title(envs.get_env_short_description(env_data))
     app.set_status('')
@@ -254,8 +271,11 @@
     if not env_data['is-default']:
         controls.append(
             ui.MenuButton('set default', ui.thunk(set_default, env_data)))
-    # XXX frankban 2013-12-18: implement the "remove env" functionality.
-    # XXX frankban 2013-12-18: implement the env modification view.
+    controls.extend([
+        # XXX frankban 2013-12-18: implement the env modification view.
+        ui.MenuButton(
+            ('control alert', 'remove'), ui.thunk(confirm_removal, env_data)),
+    ])
     widgets.append(ui.create_controls(*controls))
     listbox = urwid.ListBox(urwid.SimpleFocusListWalker(widgets))
     app.set_contents(listbox)

=== modified file 'quickstart/models/envs.py'
--- quickstart/models/envs.py	2013-12-17 15:30:48 +0000
+++ quickstart/models/envs.py	2013-12-20 10:30:54 +0000
@@ -296,6 +296,21 @@
         del env_db['default']
 
 
+def remove_env(env_db, env_name):
+    """Remove the environment named env_name from the environments database.
+
+    Raise a ValueError if the environment is not present in env_db.
+    Without errors, the env_db is modified in place and None is returned.
+    """
+    try:
+        del env_db['environments'][env_name]
+    except KeyError:
+        raise ValueError(
+            b'the environment named {!r} does not exist'.format(env_name))
+    if env_db.get('default') == env_name:
+        del env_db['default']
+
+
 def get_env_type_db():
     """Return the environments meta information based on provider types.
 

=== modified file 'quickstart/tests/cli/helpers.py'
--- quickstart/tests/cli/helpers.py	2013-12-19 17:41:40 +0000
+++ quickstart/tests/cli/helpers.py	2013-12-20 10:30:54 +0000
@@ -18,6 +18,8 @@
 
 from __future__ import unicode_literals
 
+import urwid
+
 from quickstart.cli import ui
 
 
@@ -29,3 +31,36 @@
         with self.assertRaises(ui.AppExit) as context_manager:
             loop.unhandled_input(ui.EXIT_KEY)
         return context_manager.exception.return_value
+
+
+def get_button_caption(button):
+    """Return the button caption as a string."""
+    return button._w.original_widget.text
+
+
+def emit(widget):
+    """Emit the first signal associated withe the given widget.
+
+    This is usually invoked to click buttons.
+    """
+    # Retrieve the first signal name (usually is 'click').
+    signal_name = widget.signals[0]
+    urwid.emit_signal(widget, signal_name, widget)
+
+
+def inspect_dialog(contents):
+    """Inspect the widgets composing the dialog.
+
+    Return a tuple (title_widget, messaqe_widget, buttons) where:
+        - title_widget is the Text widget displaying the dialog title;
+        - message_widget is the Text widget displaying the dialog message;
+        - buttons is a list of MenuButton widgets included in the dialog.
+    """
+    listbox = contents.top_w.base_widget
+    header, _, message_widget, controls = listbox.body
+    # The title widget is the second one in the header Pile.
+    title_widget = header.base_widget.contents[1][0]
+    # The button columns is the second widget in the Pile.
+    columns = controls.contents[1][0].base_widget
+    buttons = [content[0].base_widget for content in columns.contents]
+    return title_widget, message_widget, buttons

=== modified file 'quickstart/tests/cli/test_ui.py'
--- quickstart/tests/cli/test_ui.py	2013-12-18 14:04:14 +0000
+++ quickstart/tests/cli/test_ui.py	2013-12-20 10:30:54 +0000
@@ -27,6 +27,7 @@
     base,
     ui,
 )
+from quickstart.tests.cli import helpers as cli_helpers
 
 
 class TestAppExit(unittest.TestCase):
@@ -95,6 +96,75 @@
         callback.assert_called_once_with(button)
 
 
+class TestShowDialog(cli_helpers.CliAppTestsMixin, unittest.TestCase):
+
+    def setUp(self):
+        # Set up the base Urwid application.
+        _, self.app = base.setup_urwid_app()
+        self.original_contents = self.app.get_contents()
+
+    def test_dialog_rendering(self):
+        # The dialog is correctly displayed without additional controls.
+        ui.show_dialog(self.app, 'my title', 'my message')
+        contents = self.app.get_contents()
+        self.assertIsNot(contents, self.original_contents)
+        title_widget, message_widget, buttons = cli_helpers.inspect_dialog(
+            contents)
+        self.assertEqual('my title', title_widget.text)
+        self.assertEqual('my message', message_widget.text)
+        # The dialog only includes the "cancel" button.
+        self.assertEqual(1, len(buttons))
+        caption = cli_helpers.get_button_caption(buttons[0])
+        self.assertEqual('cancel', caption)
+
+    def test_dialog_cancel_action(self):
+        # A dialog can be properly dismissed clicking the cancel button.
+        ui.show_dialog(self.app, 'my title', 'my message')
+        contents = self.app.get_contents()
+        buttons = cli_helpers.inspect_dialog(contents)[2]
+        cancel_button = buttons[0]
+        cli_helpers.emit(cancel_button)
+        # The original contents have been restored.
+        self.assertIs(self.original_contents, self.app.get_contents())
+
+    def test_dialog_customized_actions(self):
+        # A button is displayed for each customized action provided.
+        performed = []
+        actions = [
+            ('push me', ui.thunk(performed.append, 'push')),
+            ('pull me', ui.thunk(performed.append, 'pull')),
+        ]
+        ui.show_dialog(self.app, 'my title', 'my message', actions=actions)
+        contents = self.app.get_contents()
+        buttons = cli_helpers.inspect_dialog(contents)[2]
+        # Three buttons are displayed: "cancel", "push me" and "pull me".
+        self.assertEqual(3, len(buttons))
+        captions = [
+            cli_helpers.get_button_caption(button) for button in buttons]
+        self.assertEqual(['cancel', 'push me', 'pull me'], captions)
+        push, pull = buttons[1:]
+        # Click the "push me" button.
+        cli_helpers.emit(push)
+        self.assertEqual(['push'], performed)
+        # Click the "pull me" button two times.
+        cli_helpers.emit(pull)
+        cli_helpers.emit(pull)
+        self.assertEqual(['push', 'pull', 'pull'], performed)
+
+    def test_not_dismissable(self):
+        # The cancel button is not present if the dialog is not dismissable.
+        actions = [('push me', lambda *args: None)]
+        ui.show_dialog(
+            self.app, 'my title', 'my message',
+            actions=actions, dismissable=False)
+        contents = self.app.get_contents()
+        buttons = cli_helpers.inspect_dialog(contents)[2]
+        # Only the "push me" button is displayed.
+        self.assertEqual(1, len(buttons))
+        caption = cli_helpers.get_button_caption(buttons[0])
+        self.assertEqual('push me', caption)
+
+
 class TestThunk(unittest.TestCase):
 
     widget = 'test-widget'

=== modified file 'quickstart/tests/cli/test_views.py'
--- quickstart/tests/cli/test_views.py	2013-12-19 10:59:57 +0000
+++ quickstart/tests/cli/test_views.py	2013-12-20 10:30:54 +0000
@@ -119,19 +119,6 @@
         """
         return lambda arg: isinstance(arg, cls)
 
-    def get_button_caption(self, button):
-        """Return the button caption as a string."""
-        return button._w.original_widget.text
-
-    def emit(self, widget):
-        """Emit the first signal associated withe the given widget.
-
-        This is usually invoked to click buttons.
-        """
-        # Retrieve the first signal name (usually is 'click').
-        signal_name = widget.signals[0]
-        urwid.emit_signal(widget, signal_name, widget)
-
 
 class TestEnvIndex(EnvViewTestsMixin, unittest.TestCase):
 
@@ -177,10 +164,11 @@
             env_data = envs.get_env_data(env_db, env_name)
             # The caption includes the environment description.
             env_description = envs.get_env_short_description(env_data)
-            self.assertIn(env_description, self.get_button_caption(button))
+            self.assertIn(
+                env_description, cli_helpers.get_button_caption(button))
             # When the button is clicked, the detail view is called passing the
             # corresponding environment data.
-            self.emit(button)
+            cli_helpers.emit(button)
             mock_env_detail.assert_called_once_with(
                 self.app, self.env_type_db, env_db, self.save_callable,
                 env_data)
@@ -197,7 +185,8 @@
         contents = self.app.get_contents()
         focused_widget = contents.get_focus()[0]
         self.assertIsInstance(focused_widget, ui.MenuButton)
-        self.assertIn(env_description, self.get_button_caption(focused_widget))
+        self.assertIn(
+            env_description, cli_helpers.get_button_caption(focused_widget))
 
     def test_status_with_errors(self):
         # The status message explains how errors are displayed.
@@ -272,35 +261,36 @@
             self.assertEqual(expected_text, widget.text)
 
     def test_view_buttons(self):
-        # The following buttons are displayed: "back", "use" and "set default".
+        # The following buttons are displayed: "back", "use", "set default" and
+        # "remove".
         self.call_view(env_name='ec2-west')
         buttons = self.get_control_buttons()
-        captions = [self.get_button_caption(button) for button in buttons]
-        self.assertEqual(['back', 'use', 'set default'], captions)
+        captions = map(cli_helpers.get_button_caption, buttons)
+        self.assertEqual(['back', 'use', 'set default', 'remove'], captions)
 
     def test_view_buttons_default(self):
         # If the environment is the default one, the "set default" button is
-        # not displayed. The buttons we expect are "back" and "use".
+        # not displayed. The buttons we expect are "back", "use" and "remove".
         self.call_view(env_name='lxc')
         buttons = self.get_control_buttons()
-        captions = [self.get_button_caption(button) for button in buttons]
-        self.assertEqual(['back', 'use'], captions)
+        captions = map(cli_helpers.get_button_caption, buttons)
+        self.assertEqual(['back', 'use', 'remove'], captions)
 
     def test_view_buttons_error(self):
         # If the environment is not valid, the "use" button is not displayed.
-        # The buttons we expect are "back" and "set default".
+        # The buttons we expect are "back", "set default" and "remove".
         self.call_view(env_name='local-with-errors')
         buttons = self.get_control_buttons()
-        captions = [self.get_button_caption(button) for button in buttons]
-        self.assertEqual(['back', 'set default'], captions)
+        captions = map(cli_helpers.get_button_caption, buttons)
+        self.assertEqual(['back', 'set default', 'remove'], captions)
 
     @mock.patch('quickstart.cli.views.env_index')
     def test_back_button(self, mock_env_index):
         # The index view is called if the "back" button is clicked.
         self.call_view(env_name='ec2-west')
-        # The control buttons are: "back", "use" and "set default".
+        # The "back" button is the first one.
         back_button = self.get_control_buttons()[0]
-        self.emit(back_button)
+        cli_helpers.emit(back_button)
         mock_env_index.assert_called_once_with(
             self.app, self.env_type_db, self.env_db, self.save_callable)
 
@@ -308,10 +298,10 @@
         # The application exits if the "use" button is clicked.
         # The env_db and the current environment data are returned.
         self.call_view(env_name='ec2-west')
-        # The control buttons are: "back", "use" and "set default".
+        # The "use" button is the second one.
         use_button = self.get_control_buttons()[1]
         with self.assertRaises(ui.AppExit) as context_manager:
-            self.emit(use_button)
+            cli_helpers.emit(use_button)
         expected_return_value = (self.env_db, self.env_data)
         self.assertEqual(
             expected_return_value, context_manager.exception.return_value)
@@ -321,9 +311,9 @@
         # The current environment is set as default if the "set default" button
         # is clicked. Subsequently the application switches to the index view.
         self.call_view(env_name='ec2-west')
-        # The control buttons are: "back", "use" and "set default".
+        # The "set default" button is the third one.
         set_default_button = self.get_control_buttons()[2]
-        self.emit(set_default_button)
+        cli_helpers.emit(set_default_button)
         # The index view has been called passing the modified env_db as third
         # argument.
         self.assertTrue(mock_env_index.called)
@@ -333,6 +323,64 @@
         # The new env_db has been saved.
         self.save_callable.assert_called_once_with(new_env_db)
 
+    def test_remove_button(self):
+        # A confirmation dialog is displayed if the "remove" button is clicked.
+        self.call_view(env_name='ec2-west')
+        original_contents = self.app.get_contents()
+        # The "remove" button is the last one.
+        remove_button = self.get_control_buttons()[-1]
+        cli_helpers.emit(remove_button)
+        # The original env detail contents have been replaced.
+        contents = self.app.get_contents()
+        self.assertIsNot(contents, original_contents)
+        # A "remove" confirmation dialog is displayed.
+        title_widget, message_widget, buttons = cli_helpers.inspect_dialog(
+            contents)
+        self.assertEqual('Remove the ec2-west environment', title_widget.text)
+        self.assertEqual('This action cannot be undone!', message_widget.text)
+        # The dialog includes the "cancel" and "confirm" buttons.
+        self.assertEqual(2, len(buttons))
+        captions = map(cli_helpers.get_button_caption, buttons)
+        self.assertEqual(['cancel', 'confirm'], captions)
+
+    def test_remove_cancelled(self):
+        # The "remove" confirmation dialog can be safely dismissed.
+        self.call_view(env_name='ec2-west')
+        original_contents = self.app.get_contents()
+        # The "remove" button is the last one.
+        remove_button = self.get_control_buttons()[-1]
+        cli_helpers.emit(remove_button)
+        contents = self.app.get_contents()
+        buttons = cli_helpers.inspect_dialog(contents)[2]
+        # The "cancel" button is the first one in the dialog.
+        cancel_button = buttons[0]
+        cli_helpers.emit(cancel_button)
+        # The original contents have been restored.
+        self.assertIs(original_contents, self.app.get_contents())
+
+    @mock.patch('quickstart.cli.views.env_index')
+    def test_remove_confirmed(self, mock_env_index):
+        # The current environment is removed if the "remove" button is clicked
+        # and then the deletion is confirmed. Subsequently the application
+        # switches to the index view.
+        self.call_view(env_name='ec2-west')
+        # The "remove" button is the last one.
+        remove_button = self.get_control_buttons()[-1]
+        cli_helpers.emit(remove_button)
+        contents = self.app.get_contents()
+        buttons = cli_helpers.inspect_dialog(contents)[2]
+        # The "confirm" button is the second one in the dialog.
+        confirm_button = buttons[1]
+        cli_helpers.emit(confirm_button)
+        # The index view has been called passing the modified env_db as third
+        # argument.
+        self.assertTrue(mock_env_index.called)
+        new_env_db = mock_env_index.call_args[0][2]
+        # The new env_db no longer includes the "ec2-west" environment.
+        self.assertNotIn('ec2-west', new_env_db['environments'])
+        # The new env_db has been saved.
+        self.save_callable.assert_called_once_with(new_env_db)
+
     def test_status_with_errors(self):
         # The status message explains how field errors are displayed.
         self.call_view(env_name='local-with-errors')

=== modified file 'quickstart/tests/models/test_envs.py'
--- quickstart/tests/models/test_envs.py	2013-12-17 15:30:48 +0000
+++ quickstart/tests/models/test_envs.py	2013-12-20 10:30:54 +0000
@@ -225,7 +225,7 @@
         self.assertIn('new: contents', contents)
 
 
-class GetSetEnvDataTestsMixin(object):
+class EnvDataTestsMixin(object):
     """Set up an initial environments dictionary."""
 
     def setUp(self):
@@ -245,7 +245,7 @@
             },
         }
         self.original = copy.deepcopy(self.env_db)
-        super(GetSetEnvDataTestsMixin, self).setUp()
+        super(EnvDataTestsMixin, self).setUp()
 
     def assert_env_db_not_modified(self):
         """Ensure the stored env_db is not modified."""
@@ -253,8 +253,7 @@
 
 
 class TestGetEnvData(
-        GetSetEnvDataTestsMixin, helpers.ValueErrorTestsMixin,
-        unittest.TestCase):
+        EnvDataTestsMixin, helpers.ValueErrorTestsMixin, unittest.TestCase):
 
     def test_env_not_found(self):
         # A ValueError is raised if an environment with the given name is not
@@ -301,8 +300,7 @@
 
 
 class TestSetEnvData(
-        GetSetEnvDataTestsMixin, helpers.ValueErrorTestsMixin,
-        unittest.TestCase):
+        EnvDataTestsMixin, helpers.ValueErrorTestsMixin, unittest.TestCase):
 
     def test_error_no_name_key(self):
         # A ValueError is raised if the env_data dictionary does not include
@@ -469,6 +467,36 @@
         self.assertNotIn('default', self.env_db)
 
 
+class TestRemoveEnv(
+        EnvDataTestsMixin, helpers.ValueErrorTestsMixin, unittest.TestCase):
+
+    def test_removal(self):
+        # An environment is successfully removed from the environments db.
+        envs.remove_env(self.env_db, 'aws')
+        environments = self.env_db['environments']
+        self.assertNotIn('aws', environments)
+        # The other environments are not removed.
+        self.assertIn('lxc', environments)
+        # The default environment is not modified.
+        self.assertEqual('lxc', self.env_db['default'])
+
+    def test_default_environment_removal(self):
+        # An environment is successfully removed even if it is the default one.
+        envs.remove_env(self.env_db, 'lxc')
+        environments = self.env_db['environments']
+        self.assertNotIn('lxc', environments)
+        # The other environments are not removed.
+        self.assertIn('aws', environments)
+        # The environments database no longer includes a default environment.
+        self.assertNotIn('default', self.env_db)
+
+    def test_invalid_environment_name(self):
+        # A ValueError is raised if the environment is not present in env_db.
+        expected = "the environment named u'no-such' does not exist"
+        with self.assert_value_error(expected):
+            envs.remove_env(self.env_db, 'no-such')
+
+
 class TestGetEnvTypeDb(unittest.TestCase):
 
     def setUp(self):