Repository: ansible/tower-cli
Pull request #565 (2018-07-02 13:43:01)

Add multiple statuses processed for the list cmd
##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Feature Pull Request

##### SUMMARY
<!--- Describe the change, including rationale and design decisions -->

This PR offers to handle multiple statuses while listing Tower objects if available.

Indeed, it is not currently possible to select different statuses and list them in a single CLI call:

```
$ tower-cli job list
== ============ =========================== ========== ============ 
id job_template           created             status     elapsed    
== ============ =========================== ========== ============ 
 5           19 2018-06-30T11:04:24.420148Z failed     14.893
 8           19 2018-06-30T11:52:08.643536Z failed     12.814
13           19 2018-06-30T12:43:32.030374Z failed     9.163
16           19 2018-06-30T12:45:34.288095Z failed     1138.641
20           19 2018-06-30T23:26:41.597881Z canceled   15.289
23           19 2018-06-30T23:32:37.113179Z successful 34.476
26           19 2018-06-30T23:40:28.264561Z successful 232.25
38           20 2018-07-01T00:32:40.124790Z failed     13.7
42           20 2018-07-01T00:35:00.546425Z successful 15.154
46           19 2018-07-01T00:35:33.165234Z successful 129.117
49           21 2018-07-01T00:37:55.220272Z failed     190.465
53           20 2018-07-01T03:05:25.893312Z successful 16.277
56           19 2018-07-01T03:05:55.575079Z failed     142.901
60           20 2018-07-01T04:05:28.665761Z running    28222.825323
64           20 2018-07-01T05:05:32.076526Z canceled   0.0
67           20 2018-07-01T06:05:34.389206Z pending    0.0
69           20 2018-07-01T07:05:36.900398Z canceled   0.0
71           20 2018-07-01T08:05:42.421700Z pending    0.0
73           20 2018-07-01T09:05:42.397185Z pending    0.0
75           20 2018-07-01T10:05:44.946920Z pending    0.0
77           20 2018-07-01T11:05:06.268427Z canceled   0.0
== ============ =========================== ========== ============ 
$
$ tower-cli job list --status running
== ============ =========================== ======= ============ 
id job_template           created           status    elapsed    
== ============ =========================== ======= ============ 
60           20 2018-07-01T04:05:28.665761Z running 28233.051791
== ============ =========================== ======= ============ 
$
$ tower-cli job list --status pending
== ============ =========================== ======= ======= 
id job_template           created           status  elapsed 
== ============ =========================== ======= ======= 
67           20 2018-07-01T06:05:34.389206Z pending 0.0
71           20 2018-07-01T08:05:42.421700Z pending 0.0
73           20 2018-07-01T09:05:42.397185Z pending 0.0
75           20 2018-07-01T10:05:44.946920Z pending 0.0
== ============ =========================== ======= ======= 
$ tower-cli job list --status running,pending
No records found.
```

After implementing this feature, one can successfully list multiple statuses at a time:

```
$ tower-cli job list --status running,pending
== ============ =========================== ========== ============ 
id job_template           created             status     elapsed    
== ============ =========================== ========== ============
60           20 2018-07-01T04:05:28.665761Z running    28222.825323 
67           20 2018-07-01T06:05:34.389206Z pending    0.0
71           20 2018-07-01T08:05:42.421700Z pending    0.0
73           20 2018-07-01T09:05:42.397185Z pending    0.0
75           20 2018-07-01T10:05:44.946920Z pending    0.0
== ============ =========================== ========== ============ 
```

coveralls: 
[![Coverage Status](https://coveralls.io/builds/17790881/badge)](https://coveralls.io/builds/17790881)

Coverage decreased (-12.3%) to 51.543% when pulling **13825027d5e776f1c615c358a7f9456218416f3f on piroux:feature_add_multiple_statuses_list_handling** into **9e6f48bbf61da5c16bf5716abe00390850a6aa34 on ansible:master**.

piroux: If you are interested, I will add some tests. :hammer_and_wrench:
AlanCoding: I'm happy to have this addition, but your approach here will disregard the ordering & pagination flags in order to obtain the behavior. This isn't necessary. All you want is to OR the statuses together, and for that you should put that in the language of the querystrings and let the server / database do the work.

Click the "?" in the jobs list in your API browser for documentation:

> (Added in AWX 1.4) By default, all query string filters are AND'ed together, so only the results matching all filters will be returned. To combine results matching any one of multiple criteria, prefix each query string parameter with or__:

> ?or__field=value&or__field=othervalue
> ?or__not__field=value&or__field=othervalue

For example, I am able to craft the URL `/api/v2/jobs/?or__status=running&or__status=error`, which gives me jobs that are running and errored.

In tower-cli:

```
tower-cli job list --query or__status failed --query or__status running -v
```

This doesn't work because of 

https://github.com/ansible/tower-cli/blob/828234df230efc94f2b7abbceb3ae2309ccaf235/tower_cli/models/base.py#L284

So, it would actually be better to remove that so the `--query` syntax works. But I don't disagree that a shortcut would be good.

I am happy to have such a shortcut added, it would just involve some minimal tweaks to your logic so that you use the `or__`, and don't mess with the all_pages or concatenate the list.

Also feel free to remove the exception I mentioned, or else I'll need to make an issue for it.

One last point, if you don't mind, put a TODO on this code to move this to a field callback method, I would like to introduce this sort of thing generally.

Patch:
diff --git a/tower_cli/models/base.py b/tower_cli/models/base.py
index 74b785e..63298d2 100644
--- a/tower_cli/models/base.py
+++ b/tower_cli/models/base.py
@@ -31,6 +31,7 @@ from click._compat import isatty as is_tty
 from tower_cli import resources, exceptions as exc
 from tower_cli.api import client
 from tower_cli.conf import settings
+from tower_cli.constants import STATUS_CHOICES
 from tower_cli.models.fields import Field, ManyToManyField
 from tower_cli.utils import parser, debug, secho
 from tower_cli.utils.data_structures import OrderedDict
@@ -279,13 +280,12 @@ class BaseResource(six.with_metaclass(ResourceMeta)):
                 kwargs.pop(field.name)
 
         # If queries were provided, process them.
+        params = list(kwargs.items())
         for query in queries:
-            if query[0] in kwargs:
-                raise exc.BadRequest('Attempted to set %s twice.' % query[0].replace('_', '-'))
-            kwargs[query[0]] = query[1]
+            params.append((query[0], query[1]))
 
         # Make the request to the Ansible Tower API.
-        r = client.get(url, params=kwargs)
+        r = client.get(url, params=params)
         resp = r.json()
 
         # If this was a request with a primary key included, then at the
@@ -519,6 +519,18 @@ class BaseResource(six.with_metaclass(ResourceMeta)):
 
         =====API DOCS=====
         """
+        # TODO: Move to a field callback method to make it generic
+        # If multiple statuses where given, add OR queries for each of them
+        if kwargs.get('status', None) and ',' in kwargs['status']:
+            all_status = kwargs.pop('status').strip(',').split(',')
+            queries = list(kwargs.pop('query', ()))
+            for status in all_status:
+                if status in STATUS_CHOICES:
+                    queries.append(('or__status', status))
+                else:
+                    raise exc.TowerCLIError('This status does not exist: {}'.format(status))
+            kwargs['query'] = tuple(queries)
+
         # If the `all_pages` flag is set, then ignore any page that might also be sent.
         if all_pages:
             kwargs.pop('page', None)
@@ -545,6 +557,7 @@ class BaseResource(six.with_metaclass(ResourceMeta)):
             while cursor['next']:
                 cursor = self.list(**dict(kwargs, page=cursor['next']))
                 response['results'] += cursor['results']
+                response['count'] += cursor['count']
 
         # Done; return the response
         return response


Test patch:
diff --git a/tests/test_models_base.py b/tests/test_models_base.py
index e9dcbf7..3024c2e 100644
--- a/tests/test_models_base.py
+++ b/tests/test_models_base.py
@@ -279,15 +279,6 @@ class ResourceTests(unittest.TestCase):
             self.res.list(query=[('bar', 'baz')])
             self.assertTrue(t.requests[0].url.endswith('bar=baz'))
 
-    def test_list_duplicate_kwarg(self):
-        """Establish that if we attempt to query on the same field twice,
-        that we get an error.
-        """
-        with client.test_mode as t:
-            with self.assertRaises(exc.BadRequest):
-                self.res.list(name='Batman', query=[('name', 'Robin')])
-            self.assertEqual(len(t.requests), 0)
-
     def test_get_unexpected_zero_results(self):
         """Establish that if a read method gets 0 results when it should have
         gotten one or more, that it raises NotFound.
diff --git a/tests/test_resources_job.py b/tests/test_resources_job.py
index e45c53b..f73a960 100644
--- a/tests/test_resources_job.py
+++ b/tests/test_resources_job.py
@@ -27,6 +27,7 @@ from tower_cli.cli.resource import ResSubcommand
 
 from tests.compat import unittest, mock
 from tower_cli.conf import settings
+from tower_cli.constants import CUR_API_VERSION
 
 
 # Standard functions used for space and readability
@@ -365,6 +366,248 @@ class StatusTests(unittest.TestCase):
             self.assertEqual(len(t.requests), 1)
 
 
+class ListStatusesTests(unittest.TestCase):
+    """A set of tests to establish that the job list command works in the
+    way that we expect when passing a single status or multiple statuses.
+    """
+    def setUp(self):
+        self.res = tower_cli.get_resource('job')
+
+    def test_list_lone_status(self):
+        """Establish that the list command is still able to handle single
+        status.
+        """
+        with client.test_mode as t:
+            t.register_json('/jobs/?status=running', {
+                'elapsed': 4567.0,
+                'extra': 'ignored',
+                'failed': False,
+                'status': 'running',
+                'extra': 'ignored'
+            })
+            result = self.res.list(status='running')
+            self.assertEqual(result, {
+                'elapsed': 4567.0,
+                'failed': False,
+                'status': 'running',
+                'extra': 'ignored'
+            })
+            self.assertEqual(len(t.requests), 1)
+
+    def test_list_multiple_statuses_with_bad_one(self):
+        """Establish that when passing multiple statuses, the list command errors
+        when a status is not found among the registered statuses.
+        """
+        with client.test_mode as t:
+            with self.assertRaises(exc.TowerCLIError) as e:
+                self.res.list(status='pending,runin')
+            self.assertEqual(len(t.requests), 0)
+            self.assertEqual(str(e.exception), 'This status does not exist: runin')
+
+    def test_list_multiple_statuses_first_page(self):
+        """Establish that when passing multiple statuses, the list command returns
+        only the first page of jobs matching the requested statuses.
+        """
+        with client.test_mode as t:
+            t.register_json('/jobs/?or__status=pending&or__status=running', {
+                'count': 3,
+                'previous': None,
+                'next': '/api/%s/jobs/?or__status=pending&or__status=running&page=2' % CUR_API_VERSION,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 7823.0,
+                    'failed': False,
+                    'status': 'running',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            t.register_json('/jobs/?or__status=pending&or__status=running&page=2', {
+                'count': 2,
+                'previous': '/api/%s/jobs/?or__status=pending&or__status=running&page=2' % CUR_API_VERSION,
+                'next': None,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            result = self.res.list(status='pending,running')
+            self.assertEqual(result, {
+                'count': 3,
+                'previous': None,
+                'next': 2,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 7823.0,
+                    'failed': False,
+                    'status': 'running',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            self.assertEqual(len(t.requests), 1)
+
+    def test_list_multiple_statuses_second_page(self):
+        """Establish that when passing multiple statuses, the list command returns
+        only the second page of jobs matching the requested statuses.
+        """
+        with client.test_mode as t:
+            t.register_json('/jobs/?or__status=pending&or__status=running', {
+                'count': 3,
+                'previous': None,
+                'next': '/api/%s/jobs/?or__status=pending&or__status=running&page=2' % CUR_API_VERSION,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 7823.0,
+                    'failed': False,
+                    'status': 'running',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            t.register_json('/jobs/?or__status=pending&or__status=running&page=2', {
+                'count': 2,
+                'previous': '/api/%s/jobs/?or__status=pending&or__status=running&page=1' % CUR_API_VERSION,
+                'next': None,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            result = self.res.list(status='pending,running', page=2)
+            self.assertEqual(result, {
+                'count': 2,
+                'previous': 1,
+                'next': None,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            self.assertEqual(len(t.requests), 1)
+
+    def test_list_multiple_statuses_all_pages(self):
+        """Establish that when passing multiple statuses, the list command returns
+        the entire set of jobs matching the requested statuses.
+        """
+        with client.test_mode as t:
+            t.register_json('/jobs/?or__status=pending&or__status=running', {
+                'count': 3,
+                'previous': None,
+                'next': '/api/%s/jobs/?or__status=pending&or__status=running&page=2' % CUR_API_VERSION,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 7823.0,
+                    'failed': False,
+                    'status': 'running',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            t.register_json('/jobs/?or__status=pending&or__status=running&page=2', {
+                'count': 2,
+                'previous': '/api/%s/jobs/?or__status=pending&or__status=running&page=1' % CUR_API_VERSION,
+                'next': None,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            result = self.res.list(status='pending,running', all_pages=True)
+            self.assertEqual(result, {
+                'count': 5,
+                'previous': None,
+                'next': 2,
+                'results': [{
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 7823.0,
+                    'failed': False,
+                    'status': 'running',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }, {
+                    'elapsed': 0.0,
+                    'failed': False,
+                    'status': 'pending',
+                    'extra': 'ignored',
+                }]
+            })
+            self.assertEqual(len(t.requests), 2)
+
+
 class MonitorWaitTests(unittest.TestCase):
     """A set of tests to establish that the job monitor and wait commands
     works in the way that we expect.