From 7d52ac97f111f6538b5cbe5cae1d369aac30cd04 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 8 Jan 2020 16:32:08 +0000 Subject: [PATCH] Move stats to the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API clients should just deal with calling the API and returning the data from it. Inferring things from the data is more logically done at the model layer. But we couldn’t do that before, because we didn’t have a model layer for jobs. --- app/models/job.py | 48 +++++++++----- app/notify_client/job_api_client.py | 58 +---------------- tests/app/notify_client/test_job_client.py | 73 +++++++++++----------- tests/app/test_statistics_utils.py | 28 ++++----- 4 files changed, 83 insertions(+), 124 deletions(-) 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'