Merge lp:~gmb/maas/bug-1378837 into lp:~maas-committers/maas/trunk

[Graham Binns]

Rename the 'Abort operation' action to 'Abort disk erasure', which is more accurate and less confusing.

[Christian Reis (kiko)]

Looks fine -- no test actually verifies the text in the options drop-down then?

review: Approve

[Jason Hobbs (jason-hobbs)]

The point of this operation is to avoid adding additional 'abort' API calls and actions for each operation we have. Eventually, stuff like commissioning and deploying should be abortable by this.

This was something Raphael proposed actually.

review: Disapprove

[Graham Binns (gmb)]

On 8 October 2014 17:11, Christian Reis <email address hidden> wrote:
> Looks fine -- no test actually verifies the text in the options drop-down then?

Nope. It's not referenced anywhere.

[Graham Binns (gmb)]

On 8 October 2014 17:20, Jason Hobbs <email address hidden> wrote:
> The point of this operation is to avoid adding additional 'abort' API calls and actions for each operation we have. Eventually, stuff like commissioning and deploying should be abortable by this.

Right, but people see it in the UI and think "ah, I can stop this node
from deploying… oh, wait, why didn't that work?"

[Jason Hobbs (jason-hobbs)]

So the right fix for now is to not show the action when we're not in Disk
Erasing mode.

On Wed, Oct 8, 2014 at 6:24 PM, Graham Binns <email address hidden>
wrote:

> On 8 October 2014 17:20, Jason Hobbs <email address hidden> wrote:
> > The point of this operation is to avoid adding additional 'abort' API
> calls and actions for each operation we have. Eventually, stuff like
> commissioning and deploying should be abortable by this.
>
> Right, but people see it in the UI and think "ah, I can stop this node
> from deploying… oh, wait, why didn't that work?"
>
> --
> https://code.launchpad.net/~gmb/maas/bug-1378837/+merge/237627
> You are reviewing the proposed merge of lp:~gmb/maas/bug-1378837 into
> lp:maas.
>

[Graham Binns (gmb)]

> So the right fix for now is to not show the action when we're not in Disk
> Erasing mode.

Maybe, but that's more involved. Right now, this operation only applies to disk erasing.

In fact the right fix is to replace all the "abort" actions with this one and variations thereon; I don't disagree with that. But for now, let's choose the sane path to getting this confusing bit of UX fixed. Please let's not block this during the RC period.

Yes, there are "better" ways to fix it. But this way is accurate, minimally invasive, and won't require much work to revert or change once we're out of the 1.7 cycle. Doing anything else at this stage is more complicated and thus more error prone.

[Graham Binns (gmb)]

(note that I am going to file a follow-up bug about this once this change lands).

[Jason Hobbs (jason-hobbs)]

Hmm, I thought Actions should only show in their Actionable Statuses and that it would be very straightforward to list only DISK_ERASING for this one. But that's what is done already!

I guess your complaint is that it shows up in the bulk operations list all the time. Ok - fair enough!

[Christian Reis (kiko)]

Yeah, we are all on the same page that this is not the best fix, but it is the right fix for 1.7. Can you move to approve, Jason?

[Jason Hobbs (jason-hobbs)]

Sure thing. Now that I understand the issue, I can't think of a better solution for now.

review: Approve

=== modified file 'src/maasserver/node_action.py'
--- src/maasserver/node_action.py	2014-10-08 09:43:51 +0000
+++ src/maasserver/node_action.py	2014-10-08 16:08:01 +0000
@@ -230,8 +230,8 @@
 class AbortOperation(NodeAction):
     """Abort the current operation."""
     name = "abort operation"
-    display = "Abort operation"
-    display_bulk = "Abort operation"
+    display = "Abort disk erasure"
+    display_bulk = "Abort disk erasure"
     actionable_statuses = (
         NODE_STATUS.DISK_ERASING,)
     permission = NODE_PERMISSION.EDIT