diff --git a/app/models/job.py b/app/models/job.py index 1515eda46..8685136aa 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -1,3 +1,4 @@ +from collections import defaultdict from datetime import datetime from notifications_utils.letter_timings import ( @@ -24,10 +25,7 @@ class Job(JSONModel): 'original_file_name', 'created_at', 'notification_count', - 'notifications_sent', - 'notifications_requested', 'job_status', - 'statistics', 'created_by', 'scheduled_for', } @@ -48,17 +46,37 @@ class Job(JSONModel): def scheduled(self): return self.status == 'scheduled' - @property - def notification_count(self): - return self._dict.get('notification_count', 0) + @cached_property + def statistics(self): + results = defaultdict(int) + for outcome in self._dict['statistics']: + if outcome['status'] in [ + 'failed', 'technical-failure', 'temporary-failure', + 'permanent-failure', 'cancelled' + ]: + results['failed'] += outcome['count'] + if outcome['status'] in ['sending', 'pending', 'created']: + results['sending'] += outcome['count'] + if outcome['status'] in ['delivered', 'sent']: + results['delivered'] += outcome['count'] + results['requested'] += outcome['count'] + return results @property def notifications_delivered(self): - return self._dict.get('notifications_delivered', 0) + return self.statistics['delivered'] @property def notifications_failed(self): - return self._dict.get('notifications_failed', 0) + return self.statistics['failed'] + + @property + def notifications_requested(self): + return self.statistics['requested'] + + @property + def notifications_sent(self): + return self.notifications_delivered + self.notifications_failed @property def notifications_processed(self): @@ -68,11 +86,7 @@ class Job(JSONModel): def notifications_sending(self): if self.scheduled: return 0 - return ( - self.notification_count - - self.notifications_delivered - - self.notifications_failed - ) + return self.notification_count - self.notifications_sent @property def notifications_created(self): @@ -83,7 +97,7 @@ class Job(JSONModel): @property def still_processing(self): return ( - self.status != 'finished' or self.percentage_complete < 100 + self.percentage_complete < 100 and self.status != 'finished' ) @cached_property @@ -189,15 +203,15 @@ class PaginatedJobs(ImmediateJobs): client = job_api_client.get_page_of_jobs - def __init__(self, service_id, page): + def __init__(self, service_id, page=None): try: self.current_page = int(page) except TypeError: self.current_page = 1 response = self.client(service_id, page=self.current_page) self.items = response['data'] - self.prev_page = response['links'].get('prev', None) - self.next_page = response['links'].get('next', None) + self.prev_page = response.get('links', {}).get('prev', None) + self.next_page = response.get('links', {}).get('next', None) class PaginatedUploads(PaginatedJobs): diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 6b584fb5c..7fa93d52f 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -1,5 +1,3 @@ -from collections import defaultdict - from app.extensions import redis_client from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache @@ -19,28 +17,9 @@ class JobApiClient(NotifyAdminAPIClient): NON_SCHEDULED_JOB_STATUSES = JOB_STATUSES - {'scheduled', 'cancelled'} - @staticmethod - def __convert_statistics(job): - results = defaultdict(int) - for outcome in job['statistics']: - if outcome['status'] in ['failed', 'technical-failure', 'temporary-failure', - 'permanent-failure', 'cancelled']: - results['failed'] += outcome['count'] - if outcome['status'] in ['sending', 'pending', 'created']: - results['sending'] += outcome['count'] - if outcome['status'] in ['delivered', 'sent']: - results['delivered'] += outcome['count'] - results['requested'] += outcome['count'] - return results - def get_job(self, service_id, job_id): params = {} job = self.get(url='/service/{}/job/{}'.format(service_id, job_id), params=params) - stats = self.__convert_statistics(job['data']) - job['data']['notifications_sent'] = stats['delivered'] + stats['failed'] - job['data']['notifications_delivered'] = stats['delivered'] - job['data']['notifications_failed'] = stats['failed'] - job['data']['notifications_requested'] = stats['requested'] return job @@ -51,29 +30,13 @@ class JobApiClient(NotifyAdminAPIClient): if statuses is not None: params['statuses'] = ','.join(statuses) - jobs = self.get(url='/service/{}/job'.format(service_id), params=params) - for job in jobs['data']: - stats = self.__convert_statistics(job) - job['notifications_sent'] = stats['delivered'] + stats['failed'] - job['notifications_delivered'] = stats['delivered'] - job['notifications_failed'] = stats['failed'] - job['notifications_requested'] = stats['requested'] - - return jobs + return self.get(url='/service/{}/job'.format(service_id), params=params) def get_uploads(self, service_id, limit_days=None, page=1): params = {'page': page} if limit_days is not None: params['limit_days'] = limit_days - uploads = self.get(url='/service/{}/upload'.format(service_id), params=params) - for upload in uploads['data']: - stats = self.__convert_statistics(upload) - upload['notifications_sent'] = stats['delivered'] + stats['failed'] - upload['notifications_delivered'] = stats['delivered'] - upload['notifications_failed'] = stats['failed'] - upload['notifications_requested'] = stats['requested'] - - return uploads + return self.get(url='/service/{}/upload'.format(service_id), params=params) def has_sent_previously(self, service_id, template_id, template_version, original_file_name): return ( @@ -125,30 +88,15 @@ class JobApiClient(NotifyAdminAPIClient): ex=cache.TTL, ) - stats = self.__convert_statistics(job['data']) - job['data']['notifications_sent'] = stats['delivered'] + stats['failed'] - job['data']['notifications_delivered'] = stats['delivered'] - job['data']['notifications_failed'] = stats['failed'] - job['data']['notifications_requested'] = stats['requested'] - return job @cache.delete('has_jobs-{service_id}') def cancel_job(self, service_id, job_id): - - job = self.post( + return self.post( url='/service/{}/job/{}/cancel'.format(service_id, job_id), data={} ) - stats = self.__convert_statistics(job['data']) - job['data']['notifications_sent'] = stats['delivered'] + stats['failed'] - job['data']['notifications_delivered'] = stats['delivered'] - job['data']['notifications_failed'] = stats['failed'] - job['data']['notifications_requested'] = stats['requested'] - - return job - @cache.delete('has_jobs-{service_id}') def cancel_letter_job(self, service_id, job_id): return self.post( diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index b12322168..b3bffbbe2 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/tests/app/notify_client/test_job_client.py @@ -3,6 +3,7 @@ from unittest.mock import ANY import pytest +from app.models.job import Job, PaginatedJobs from app.notify_client.job_api_client import JobApiClient @@ -113,16 +114,15 @@ def test_client_parses_job_stats(mocker): expected_url = '/service/{}/job/{}'.format(service_id, job_id) - client = JobApiClient() mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get', return_value=expected_data) - result = client.get_job(service_id, job_id) + result = Job.from_id(job_id, service_id=service_id) mock_get.assert_called_once_with(url=expected_url, params={}) - assert result['data']['notifications_requested'] == 80 - assert result['data']['notifications_sent'] == 50 - assert result['data']['notification_count'] == 80 - assert result['data']['notifications_failed'] == 40 + assert result.notifications_requested == 80 + assert result.notifications_sent == 50 + assert result.notification_count == 80 + assert result.notifications_failed == 40 def test_client_parses_empty_job_stats(mocker): @@ -149,16 +149,15 @@ def test_client_parses_empty_job_stats(mocker): expected_url = '/service/{}/job/{}'.format(service_id, job_id) - client = JobApiClient() mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get', return_value=expected_data) - result = client.get_job(service_id, job_id) + result = Job.from_id(job_id, service_id=service_id) mock_get.assert_called_once_with(url=expected_url, params={}) - assert result['data']['notifications_requested'] == 0 - assert result['data']['notifications_sent'] == 0 - assert result['data']['notification_count'] == 80 - assert result['data']['notifications_failed'] == 0 + assert result.notifications_requested == 0 + assert result.notifications_sent == 0 + assert result.notification_count == 80 + assert result.notifications_failed == 0 def test_client_parses_job_stats_for_service(mocker): @@ -221,22 +220,21 @@ def test_client_parses_job_stats_for_service(mocker): expected_url = '/service/{}/job'.format(service_id) - client = JobApiClient() mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get', return_value=expected_data) - result = client.get_jobs(service_id) + result = PaginatedJobs(service_id) - mock_get.assert_called_once_with(url=expected_url, params={'page': 1}) - assert result['data'][0]['id'] == job_1_id - assert result['data'][0]['notifications_requested'] == 80 - assert result['data'][0]['notifications_sent'] == 50 - assert result['data'][0]['notification_count'] == 80 - assert result['data'][0]['notifications_failed'] == 40 - assert result['data'][1]['id'] == job_2_id - assert result['data'][1]['notifications_requested'] == 40 - assert result['data'][1]['notifications_sent'] == 25 - assert result['data'][1]['notification_count'] == 40 - assert result['data'][1]['notifications_failed'] == 20 + mock_get.assert_called_once_with(url=expected_url, params={'page': 1, 'statuses': ANY}) + assert result[0].id == job_1_id + assert result[0].notifications_requested == 80 + assert result[0].notifications_sent == 50 + assert result[0].notification_count == 80 + assert result[0].notifications_failed == 40 + assert result[1].id == job_2_id + assert result[1].notifications_requested == 40 + assert result[1].notifications_sent == 25 + assert result[1].notification_count == 40 + assert result[1].notifications_failed == 20 def test_client_parses_empty_job_stats_for_service(mocker): @@ -281,22 +279,21 @@ def test_client_parses_empty_job_stats_for_service(mocker): expected_url = '/service/{}/job'.format(service_id) - client = JobApiClient() mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get', return_value=expected_data) - result = client.get_jobs(service_id) + result = PaginatedJobs(service_id) - mock_get.assert_called_once_with(url=expected_url, params={'page': 1}) - assert result['data'][0]['id'] == job_1_id - assert result['data'][0]['notifications_requested'] == 0 - assert result['data'][0]['notifications_sent'] == 0 - assert result['data'][0]['notification_count'] == 80 - assert result['data'][0]['notifications_failed'] == 0 - assert result['data'][1]['id'] == job_2_id - assert result['data'][1]['notifications_requested'] == 0 - assert result['data'][1]['notifications_sent'] == 0 - assert result['data'][1]['notification_count'] == 40 - assert result['data'][1]['notifications_failed'] == 0 + mock_get.assert_called_once_with(url=expected_url, params={'page': 1, 'statuses': ANY}) + assert result[0].id == job_1_id + assert result[0].notifications_requested == 0 + assert result[0].notifications_sent == 0 + assert result[0].notification_count == 80 + assert result[0].notifications_failed == 0 + assert result[1].id == job_2_id + assert result[1].notifications_requested == 0 + assert result[1].notifications_sent == 0 + assert result[1].notification_count == 40 + assert result[1].notifications_failed == 0 def test_cancel_job(mocker): diff --git a/tests/app/test_statistics_utils.py b/tests/app/test_statistics_utils.py index 8abde1cf2..9ce41a8f0 100644 --- a/tests/app/test_statistics_utils.py +++ b/tests/app/test_statistics_utils.py @@ -128,25 +128,25 @@ def test_service_statistics_by_state(): (1, 4, 20) ]) def test_add_rate_to_job_calculates_rate(failed, delivered, expected_failure_rate): - resp = Job( - { - 'notifications_failed': failed, - 'notifications_delivered': delivered, - 'id': 'foo' - } - ) + resp = Job({ + 'statistics': [ + {'status': 'failed', 'count': failed}, + {'status': 'delivered', 'count': delivered}, + ], + 'id': 'foo', + }) assert resp.failure_rate == expected_failure_rate def test_add_rate_to_job_preserves_initial_fields(): - resp = Job( - { - 'notifications_failed': 0, - 'notifications_delivered': 0, - 'id': 'foo' - } - ) + resp = Job({ + 'statistics': [ + {'status': 'failed', 'count': 0}, + {'status': 'delivered', 'count': 0}, + ], + 'id': 'foo', + }) assert resp.notifications_failed == resp.notifications_delivered == resp.failure_rate == 0 assert resp.id == 'foo'