From ddce83d7757c56d6ff2307dd984a39f6143f6aab Mon Sep 17 00:00:00 2001
From: Alexander Maretskiy <amaretskiy@mirantis.com>
Date: Thu, 18 Aug 2016 18:02:18 +0300
Subject: [PATCH] [API] Allow to delete stopped tasks without force=True

It is reasonable to protect deletion of running tasks
(statuses INIT, VERIFYING, RUNNING, ABORTING and so on...)
but it is strange to protect deletion for stopped tasks
(statuses FAILED and ABORTED).
Also this is annoyning in CLI usage.

This patch proposes deletion of all stopped tasks
without forcing

Change-Id: I3c540d1b11c2ef3cc99293dbc50a229cfbf3af17
---
 rally/api.py           | 16 ++++++++---
 tests/unit/test_api.py | 60 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/rally/api.py b/rally/api.py
index 6f0c23b784..c133028b94 100644
--- a/rally/api.py
+++ b/rally/api.py
@@ -361,11 +361,19 @@ class Task(object):
         :param force: If set to True, then delete the task despite to the
                       status
         :raises TaskInvalidStatus: when the status of the task is not
-                                   FINISHED and the force argument
-                                   is not True
+                                   in FINISHED, FAILED or ABORTED and
+                                   the force argument is not True
         """
-        status = None if force else consts.TaskStatus.FINISHED
-        objects.Task.delete_by_uuid(task_uuid, status=status)
+        if force:
+            objects.Task.delete_by_uuid(task_uuid, status=None)
+        elif objects.Task.get_status(task_uuid) in (
+                consts.TaskStatus.ABORTED,
+                consts.TaskStatus.FINISHED,
+                consts.TaskStatus.FAILED):
+            objects.Task.delete_by_uuid(task_uuid, status=None)
+        else:
+            objects.Task.delete_by_uuid(
+                task_uuid, status=consts.TaskStatus.FINISHED)
 
 
 class Verification(object):
diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py
index d774207094..96d8988061 100644
--- a/tests/unit/test_api.py
+++ b/tests/unit/test_api.py
@@ -244,18 +244,56 @@ class TaskAPITestCase(test.TestCase):
         self.assertFalse(mock_task.get_status.called)
         self.assertFalse(mock_time.sleep.called)
 
-    @mock.patch("rally.common.objects.task.db.task_delete")
-    def test_delete(self, mock_task_delete):
-        api.Task.delete(self.task_uuid)
-        mock_task_delete.assert_called_once_with(
+    @ddt.data({"task_status": "strange value",
+               "expected_status": consts.TaskStatus.FINISHED},
+              {"task_status": consts.TaskStatus.INIT,
+               "expected_status": consts.TaskStatus.FINISHED},
+              {"task_status": consts.TaskStatus.VERIFYING,
+               "expected_status": consts.TaskStatus.FINISHED},
+              {"task_status": consts.TaskStatus.ABORTING,
+               "expected_status": consts.TaskStatus.FINISHED},
+              {"task_status": consts.TaskStatus.SOFT_ABORTING,
+               "expected_status": consts.TaskStatus.FINISHED},
+              {"task_status": consts.TaskStatus.RUNNING,
+               "expected_status": consts.TaskStatus.FINISHED},
+              {"task_status": consts.TaskStatus.ABORTED,
+               "expected_status": None},
+              {"task_status": consts.TaskStatus.FINISHED,
+               "expected_status": None},
+              {"task_status": consts.TaskStatus.FAILED,
+               "expected_status": None},
+              {"task_status": "strange value",
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.INIT,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.VERIFYING,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.RUNNING,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.ABORTING,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.SOFT_ABORTING,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.ABORTED,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.FINISHED,
+               "force": True, "expected_status": None},
+              {"task_status": consts.TaskStatus.FAILED,
+               "force": True, "expected_status": None})
+    @ddt.unpack
+    @mock.patch("rally.api.objects.Task.get_status")
+    @mock.patch("rally.api.objects.Task.delete_by_uuid")
+    def test_delete(self, mock_task_delete_by_uuid, mock_task_get_status,
+                    task_status, expected_status, force=False, raises=None):
+        mock_task_get_status.return_value = task_status
+        api.Task.delete(self.task_uuid, force=force)
+        if force:
+            self.assertFalse(mock_task_get_status.called)
+        else:
+            mock_task_get_status.assert_called_once_with(self.task_uuid)
+        mock_task_delete_by_uuid.assert_called_once_with(
             self.task_uuid,
-            status=consts.TaskStatus.FINISHED)
-
-    @mock.patch("rally.common.objects.task.db.task_delete")
-    def test_delete_force(self, mock_task_delete):
-        api.Task.delete(self.task_uuid, force=True)
-        mock_task_delete.assert_called_once_with(
-            self.task_uuid, status=None)
+            status=expected_status)
 
     @mock.patch("rally.api.objects.Task")
     def test_get_detailed(self, mock_task):