diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 78e05fa99..c8b037a84 100644 --- a/app/main/views/dashboard.py +++ b/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( - job_api_client.get_jobs(service_id, statuses=['scheduled'])['data'], - key=lambda job: job['scheduled_for'] - ) - immediate_jobs = [ - add_rate_to_job(job) - for job in job_api_client.get_jobs(service_id, limit_days=7, statuses=statuses_to_display)['data'] - ] + scheduled_jobs, immediate_jobs = [], [] + if job_api_client.has_jobs(service_id): + scheduled_jobs = job_api_client.get_scheduled_jobs(service_id) + immediate_jobs = [ + add_rate_to_job(job) + for job in job_api_client.get_immediate_jobs(service_id) + ] + stats = service_api_client.get_service_statistics(service_id, today_only=False) column_width, max_notifiction_count = get_column_properties( number_of_columns=( diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 5527c6367..3eca491d0 100644 --- a/app/main/views/jobs.py +++ b/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 - 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_response = job_api_client.get_page_of_jobs(service_id, page=page) jobs = [ add_rate_to_job(job) for job in jobs_response['data'] ] diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 3099827f7..c214cc447 100644 --- a/app/notify_client/job_api_client.py +++ b/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' } + NON_SCHEDULED_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.NON_SCHEDULED_JOB_STATUSES, + page=page, + ) + + def get_immediate_jobs(self, service_id): + return self.get_jobs( + service_id, + limit_days=7, + statuses=self.NON_SCHEDULED_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( diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index fac07b2e5..0cac648ea 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/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] - assert first_call[0] == (SERVICE_ONE_ID,) - assert first_call[1]['statuses'] == ['scheduled'] + second_call = mock_get_jobs.call_args_list[1] + assert second_call[0] == (SERVICE_ONE_ID,) + 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] - assert second_call[0] == (SERVICE_ONE_ID,) - assert second_call[1]['limit_days'] == 7 - assert 'scheduled' not in second_call[1]['statuses'] + third_call = mock_get_jobs.call_args_list[2] + assert third_call[0] == (SERVICE_ONE_ID,) + assert third_call[1]['limit_days'] == 7 + 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 - assert first_call == call(ANY, statuses=['scheduled']) - # second call - everything but scheduled and cancelled + # first call - checking for any jobs + assert first_call == call(SERVICE_ONE_ID) 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', diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index 47c86dcdb..0b39da02e 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/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,63 @@ 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_get = mocker.patch( + 'app.notify_client.job_api_client.JobApiClient.get' + ) + mock_redis_get = mocker.patch( + 'app.notify_client.RedisClient.get', + return_value=cache_value, + ) + + assert JobApiClient().has_jobs(fake_uuid) is return_value + assert not mock_get.called + mock_redis_get.assert_called_once_with( + 'has_jobs-{}'.format(fake_uuid) + )