Check if any jobs exist before querying jobs

At the moment the dashboard does two API calls to find out if a service
has:

1. Scheduled jobs
2. Normal jobs

API calls are slow because they are synchronous, go over the network and
touch the database. We can’t cache these API calls because:
- a scheduled job could become a normal job at any time
- the statistics on a normal job are constantly updating

However there are plenty of services which don’t have any jobs, and
probably never will. And finding out if a service has any jobs is
reliably cacheable (because as soon as a service creates its first job
it has some jobs).

So this commit:
- refactors the way we get scheduled/normal jobs into the job_api_client
  to make the view a bit slimmer
- makes an additional, Redis-wrapped call to find out if any jobs exist
  before trying to get the jobs

This should result in a speedup on the dashboard, and can be used in the
future if there’s anywhere else we want to show or hide something
depending on whether a service has created any jobs (I have some ideas).

app/main/views/dashboard.py

@@ -275,21 +275,18 @@ def aggregate_usage(template_statistics, sort_key='count'):
 
 
 def get_dashboard_partials(service_id):
-    # all but scheduled and cancelled
-    statuses_to_display = job_api_client.JOB_STATUSES - {'scheduled', 'cancelled'}
-
     template_statistics = aggregate_usage(
         template_statistics_client.get_template_statistics_for_service(service_id, limit_days=7)
     )
 
-    scheduled_jobs = sorted(
+    scheduled_jobs, immediate_jobs = [], []
-        job_api_client.get_jobs(service_id, statuses=['scheduled'])['data'],
+    if job_api_client.has_jobs(service_id):
-        key=lambda job: job['scheduled_for']
+        scheduled_jobs = job_api_client.get_scheduled_jobs(service_id)
-    )
+        immediate_jobs = [
-    immediate_jobs = [
+            add_rate_to_job(job)
-        add_rate_to_job(job)
+            for job in job_api_client.get_immediate_jobs(service_id)
-        for job in job_api_client.get_jobs(service_id, limit_days=7, statuses=statuses_to_display)['data']
+        ]
-    ]
+
     stats = service_api_client.get_service_statistics(service_id, today_only=False)
     column_width, max_notifiction_count = get_column_properties(
         number_of_columns=(

app/main/views/jobs.py

@@ -42,9 +42,7 @@ from app.utils import (
 @user_has_permissions('view_activity')
 def view_jobs(service_id):
     page = int(request.args.get('page', 1))
-    # all but scheduled and cancelled
+    jobs_response = job_api_client.get_page_of_jobs(service_id, page=page)
-    statuses_to_display = job_api_client.JOB_STATUSES - {'scheduled', 'cancelled'}
-    jobs_response = job_api_client.get_jobs(service_id, statuses=statuses_to_display, page=page)
     jobs = [
         add_rate_to_job(job) for job in jobs_response['data']
     ]

app/notify_client/job_api_client.py

@@ -1,6 +1,6 @@
 from collections import defaultdict
 
-from app.notify_client import NotifyAdminAPIClient, _attach_current_user
+from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache
 
 
 class JobApiClient(NotifyAdminAPIClient):
@@ -16,6 +16,8 @@ class JobApiClient(NotifyAdminAPIClient):
         'sent to dvla'
     }
 
+    NORMAL_JOB_STATUSES = JOB_STATUSES - {'scheduled', 'cancelled'}
+
     def __init__(self):
         super().__init__("a" * 73, "b")
 
@@ -60,8 +62,38 @@ class JobApiClient(NotifyAdminAPIClient):
 
         return jobs
 
+    def get_page_of_jobs(self, service_id, page):
+        return self.get_jobs(
+            service_id,
+            statuses=self.NORMAL_JOB_STATUSES,
+            page=page,
+        )
+
+    def get_immediate_jobs(self, service_id):
+        return self.get_jobs(
+            service_id,
+            limit_days=7,
+            statuses=self.NORMAL_JOB_STATUSES,
+        )['data']
+
+    def get_scheduled_jobs(self, service_id):
+        return sorted(
+            self.get_jobs(service_id, statuses=['scheduled'])['data'],
+            key=lambda job: job['scheduled_for']
+        )
+
+    @cache.set('has_jobs-{service_id}')
+    def has_jobs(self, service_id):
+        return bool(self.get_jobs(service_id)['data'])
+
     def create_job(self, job_id, service_id, scheduled_for=None):
 
+        self.redis_client.set(
+            'has_jobs-{}'.format(service_id),
+            True,
+            ex=cache.TTL,
+        )
+
         data = {"id": job_id}
 
         if scheduled_for:
@@ -78,6 +110,7 @@ class JobApiClient(NotifyAdminAPIClient):
 
         return job
 
+    @cache.delete('has_jobs-{service_id}')
     def cancel_job(self, service_id, job_id):
 
         job = self.post(

tests/app/main/views/test_dashboard.py

@@ -2,7 +2,7 @@ import copy
 import json
 from datetime import datetime
 from functools import partial
-from unittest.mock import ANY, call
+from unittest.mock import call
 
 import pytest
 from bs4 import BeautifulSoup
@@ -573,9 +573,9 @@ def test_should_show_upcoming_jobs_on_dashboard(
 ):
     response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID))
 
-    first_call = mock_get_jobs.call_args_list[0]
+    second_call = mock_get_jobs.call_args_list[1]
-    assert first_call[0] == (SERVICE_ONE_ID,)
+    assert second_call[0] == (SERVICE_ONE_ID,)
-    assert first_call[1]['statuses'] == ['scheduled']
+    assert second_call[1]['statuses'] == ['scheduled']
 
     assert response.status_code == 200
 
@@ -702,10 +702,10 @@ def test_should_show_recent_jobs_on_dashboard(
 ):
     response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID))
 
-    second_call = mock_get_jobs.call_args_list[1]
+    third_call = mock_get_jobs.call_args_list[2]
-    assert second_call[0] == (SERVICE_ONE_ID,)
+    assert third_call[0] == (SERVICE_ONE_ID,)
-    assert second_call[1]['limit_days'] == 7
+    assert third_call[1]['limit_days'] == 7
-    assert 'scheduled' not in second_call[1]['statuses']
+    assert 'scheduled' not in third_call[1]['statuses']
 
     assert response.status_code == 200
 
@@ -1266,11 +1266,14 @@ def test_should_show_all_jobs_with_valid_statuses(
     logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID))
 
     first_call = mock_get_jobs.call_args_list[0]
-    # first call - scheduled jobs only
+    # first call - checking for any jobs
-    assert first_call == call(ANY, statuses=['scheduled'])
+    assert first_call == call(SERVICE_ONE_ID)
-    # second call - everything but scheduled and cancelled
     second_call = mock_get_jobs.call_args_list[1]
-    assert second_call == call(ANY, limit_days=ANY, statuses={
+    # second call - scheduled jobs only
+    assert second_call == call(SERVICE_ONE_ID, statuses=['scheduled'])
+    # third call - everything but scheduled and cancelled
+    third_call = mock_get_jobs.call_args_list[2]
+    assert third_call == call(SERVICE_ONE_ID, limit_days=7, statuses={
         'pending',
         'in progress',
         'finished',

tests/app/notify_client/test_job_client.py

@@ -1,6 +1,8 @@
 import uuid
 from unittest.mock import ANY
 
+import pytest
+
 from app.notify_client.job_api_client import JobApiClient
 
 
@@ -8,6 +10,7 @@ def test_client_creates_job_data_correctly(mocker, fake_uuid):
     job_id = fake_uuid
     service_id = fake_uuid
     mocker.patch('app.notify_client.current_user', id='1')
+    mock_redis_set = mocker.patch('app.notify_client.RedisClient.set')
 
     expected_data = {
         "id": job_id,
@@ -24,6 +27,11 @@ def test_client_creates_job_data_correctly(mocker, fake_uuid):
 
     client.create_job(service_id, job_id)
     mock_post.assert_called_once_with(url=expected_url, data=expected_data)
+    mock_redis_set.assert_called_once_with(
+        'has_jobs-{}'.format(service_id),
+        True,
+        ex=604800,
+    )
 
 
 def test_client_schedules_job(mocker, fake_uuid):
@@ -300,3 +308,59 @@ def test_cancel_job(mocker):
         url='/service/{}/job/{}/cancel'.format('service_id', 'job_id'),
         data={}
     )
+
+
+@pytest.mark.parametrize('job_data, expected_cache_value', [
+    (
+        [{'data': [1, 2, 3], 'statistics': []}],
+        'true',
+    ),
+    (
+        [],
+        'false',
+    ),
+])
+def test_has_jobs_sets_cache(
+    mocker,
+    fake_uuid,
+    job_data,
+    expected_cache_value,
+):
+    mock_get = mocker.patch(
+        'app.notify_client.job_api_client.JobApiClient.get',
+        return_value={'data': job_data}
+    )
+    mock_redis_set = mocker.patch('app.notify_client.RedisClient.set')
+
+    JobApiClient().has_jobs(fake_uuid)
+
+    mock_get.assert_called_once_with(
+        url='/service/{}/job'.format(fake_uuid),
+        params={'page': 1}
+    )
+    mock_redis_set.assert_called_once_with(
+        'has_jobs-{}'.format(fake_uuid),
+        expected_cache_value,
+        ex=604800,
+    )
+
+
+@pytest.mark.parametrize('cache_value, return_value', [
+    (b'true', True),
+    (b'false', False),
+])
+def test_has_jobs_returns_from_cache(
+    mocker,
+    fake_uuid,
+    cache_value,
+    return_value,
+):
+    mock_redis_get = mocker.patch(
+        'app.notify_client.RedisClient.get',
+        return_value=cache_value,
+    )
+
+    assert JobApiClient().has_jobs(fake_uuid) is return_value
+    mock_redis_get.assert_called_once_with(
+        'has_jobs-{}'.format(fake_uuid)
+    )