Code review comment for lp:~frankban/juju-quickstart/env-manage-remove-environment

Revision history for this message
Gary Poster (gary) 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?

"""
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/

« Back to merge proposal