From 4a551329c59695b2f6b4827364b730db34c5ea96 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 23 Aug 2016 16:58:50 +0100 Subject: [PATCH 1/4] Parse the JOB API response converting new style response to old style object - allows migration of API --- app/notify_client/job_api_client.py | 37 +++++++++++++++++++++++++++-- tests/app/main/views/test_jobs.py | 1 - 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 1111f8c9e..9225d7b69 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -14,15 +14,41 @@ class JobApiClient(BaseAPIClient): self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] self.secret = app.config['ADMIN_CLIENT_SECRET'] + @staticmethod + def __convert_statistics(job): + results = { + 'sending': 0, + 'delivered': 0, + 'failed': 0 + } + if 'statistics' in job: + for outcome in job['statistics']: + if outcome['status'] in ['failed', 'technical-failure', 'temporary-failure', 'permanent-failure']: + results['failed'] += outcome['count'] + if outcome['status'] in ['sending', 'pending', 'created']: + results['sending'] += outcome['count'] + if outcome['status'] in ['delivered']: + results['delivered'] += outcome['count'] + return results + def get_job(self, service_id, job_id=None, limit_days=None, status=None): if job_id: params = {} if status is not None: params['status'] = status - return self.get(url='/service/{}/job/{}'.format(service_id, job_id), params=params) + job = self.get(url='/service/{}/job/{}'.format(service_id, job_id), params=params) + + if 'notifications_sent' not in job['data']: + stats = self.__convert_statistics(job) + job['data']['notifications_sent'] = stats['sending'] + job['data']['notifications_delivered'] = stats['delivered'] + job['data']['notifications_failed'] = stats['failed'] + return job + params = {} if limit_days is not None: params['limit_days'] = limit_days + return self.get(url='/service/{}/job'.format(service_id), params=params) def create_job(self, job_id, service_id, template_id, original_file_name, notification_count): @@ -34,4 +60,11 @@ class JobApiClient(BaseAPIClient): } data = _attach_current_user(data) resp = self.post(url='/service/{}/job'.format(service_id), data=data) - return resp['data'] + + if 'notifications_sent' not in resp['data']: + stats = self.__convert_statistics(resp) + resp['data']['notifications_sent'] = stats['sending'] + resp['data']['notifications_delivered'] = stats['delivered'] + resp['data']['notifications_failed'] = stats['failed'] + + return resp diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 7bfbcefb2..a31f51edb 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -184,7 +184,6 @@ def test_should_show_updates_for_one_job_as_json( assert 'Recipient' in content['notifications'] assert '07123456789' in content['notifications'] assert 'Status' in content['notifications'] - print(content['notifications']) assert 'Delivered' in content['notifications'] assert '11:10' in content['notifications'] assert 'Uploaded by Test User on 1 January at 11:09' in content['status'] From 5b2ae43d5d7d1cc85ed70445bbc465a76adbe251 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Aug 2016 10:35:04 +0100 Subject: [PATCH 2/4] Wrote some tests around the stats conversions. Properly named the finished parameter. Handled the root JSON key --- app/main/views/jobs.py | 11 +-- app/notify_client/job_api_client.py | 21 +++-- tests/app/notify_client/test_job_client.py | 93 ++++++++++++++++++++++ 3 files changed, 109 insertions(+), 16 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 749fbfa31..f26203d75 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -78,13 +78,14 @@ def view_job(service_id, job_id): filter_args = _parse_filter_args(request.args) filter_args['status'] = _set_status_filters(filter_args) + finished = ( + bool(job.get('notification_count', 0) and (( + job.get('notifications_delivered', 0) + + job.get('notifications_failed', 0) + ) == job.get('notification_count', 0)))) return render_template( 'views/jobs/job.html', - finished=job.get('notifications_sent', 0) and (( - job.get('notifications_sent', 0) - - job.get('notifications_delivered', 0) - - job.get('notifications_failed', 0) - ) == 0), + finished=finished, uploaded_file_name=job['original_file_name'], template=Template( service_api_client.get_service_template( diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 9225d7b69..4cd4eadf4 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -21,8 +21,8 @@ class JobApiClient(BaseAPIClient): 'delivered': 0, 'failed': 0 } - if 'statistics' in job: - for outcome in job['statistics']: + if 'statistics' in job['data']: + for outcome in job['data']['statistics']: if outcome['status'] in ['failed', 'technical-failure', 'temporary-failure', 'permanent-failure']: results['failed'] += outcome['count'] if outcome['status'] in ['sending', 'pending', 'created']: @@ -37,10 +37,9 @@ class JobApiClient(BaseAPIClient): if status is not None: params['status'] = status job = self.get(url='/service/{}/job/{}'.format(service_id, job_id), params=params) - if 'notifications_sent' not in job['data']: stats = self.__convert_statistics(job) - job['data']['notifications_sent'] = stats['sending'] + job['data']['notifications_sent'] = stats['delivered'] + stats['failed'] job['data']['notifications_delivered'] = stats['delivered'] job['data']['notifications_failed'] = stats['failed'] return job @@ -59,12 +58,12 @@ class JobApiClient(BaseAPIClient): "notification_count": notification_count } data = _attach_current_user(data) - resp = self.post(url='/service/{}/job'.format(service_id), data=data) + job = self.post(url='/service/{}/job'.format(service_id), data=data) - if 'notifications_sent' not in resp['data']: - stats = self.__convert_statistics(resp) - resp['data']['notifications_sent'] = stats['sending'] - resp['data']['notifications_delivered'] = stats['delivered'] - resp['data']['notifications_failed'] = stats['failed'] + if 'notifications_sent' not in job['data']: + stats = self.__convert_statistics(job) + job['data']['notifications_sent'] = stats['delivered'] + stats['failed'] + job['data']['notifications_delivered'] = stats['delivered'] + job['data']['notifications_failed'] = stats['failed'] - return resp + return job diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index 442997135..154732d55 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/tests/app/notify_client/test_job_client.py @@ -25,3 +25,96 @@ def test_client_creates_job_data_correctly(mocker, fake_uuid): client.create_job(job_id, service_id, template_id, original_file_name, notification_count) mock_post.assert_called_once_with(url=expected_url, data=expected_data) + + +def test_client_gets_job_by_service_and_job(mocker): + mocker.patch('app.notify_client.current_user', id='1') + + service_id = 'service_id' + job_id = 'job_id' + + expected_url = '/service/{}/job/{}'.format(service_id, job_id) + + client = JobApiClient() + mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get') + + client.get_job(service_id, job_id) + + mock_get.assert_called_once_with(url=expected_url, params={}) + + +def test_client_gets_job_by_service_and_job_filtered_by_status(mocker): + mocker.patch('app.notify_client.current_user', id='1') + + service_id = 'service_id' + job_id = 'job_id' + + expected_url = '/service/{}/job/{}'.format(service_id, job_id) + + client = JobApiClient() + mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get') + + client.get_job(service_id, job_id, limit_days=1, status='failed') + + mock_get.assert_called_once_with(url=expected_url, params={'status': 'failed'}) + + +def test_client_gets_job_by_service_filtered_by_status(mocker): + mocker.patch('app.notify_client.current_user', id='1') + + service_id = 'service_id' + + expected_url = '/service/{}/job'.format(service_id) + + client = JobApiClient() + mock_get = mocker.patch('app.notify_client.job_api_client.JobApiClient.get') + + client.get_job(service_id, limit_days=1, status='failed') + + mock_get.assert_called_once_with(url=expected_url, params={'limit_days': 1}) + + +def test_client_parses_job_stats(mocker): + mocker.patch('app.notify_client.current_user', id='1') + + service_id = 'service_id' + job_id = 'job_id' + expected_data = {'data': { + 'status': 'finished', + 'template_version': 3, + 'id': job_id, + 'updated_at': '2016-08-24T08:29:28.332972+00:00', + 'service': service_id, + 'processing_finished': '2016-08-24T08:11:48.676365+00:00', + 'statistics': [ + {'status': 'failed', 'count': 10}, + {'status': 'technical-failure', 'count': 10}, + {'status': 'temporary-failure', 'count': 10}, + {'status': 'permanent-failure', 'count': 10}, + {'status': 'created', 'count': 10}, + {'status': 'sending', 'count': 10}, + {'status': 'pending', 'count': 10}, + {'status': 'delivered', 'count': 10} + ], + 'original_file_name': 'test-notify-email.csv', + 'created_by': { + 'name': 'test-user@digital.cabinet-office.gov.uk', + 'id': '3571f2ae-7a39-4fb4-9ad7-8453f5257072' + }, + 'created_at': '2016-08-24T08:09:56.371073+00:00', + 'template': 'c0309261-9c9e-4530-8fed-5f67b02260d2', + 'notification_count': 80, + 'processing_started': '2016-08-24T08:09:57.661246+00:00' + }} + + 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) + + mock_get.assert_called_once_with(url=expected_url, params={}) + assert result['data']['notifications_sent'] == 50 + assert result['data']['notification_count'] == 80 + assert result['data']['notifications_failed'] == 40 From 9b348880ef516af7620c202112e300e01cbd7345 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Aug 2016 10:42:57 +0100 Subject: [PATCH 3/4] Test for old style stats - make sure we don't override / fail if no new block present. --- tests/app/notify_client/test_job_client.py | 39 ++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index 154732d55..43c911205 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/tests/app/notify_client/test_job_client.py @@ -74,6 +74,45 @@ def test_client_gets_job_by_service_filtered_by_status(mocker): mock_get.assert_called_once_with(url=expected_url, params={'limit_days': 1}) +def test_client_parses_handles_aggregate_table_job_stats(mocker): + mocker.patch('app.notify_client.current_user', id='1') + + service_id = 'service_id' + job_id = 'job_id' + expected_data = {'data': { + 'status': 'finished', + 'template_version': 3, + 'id': job_id, + 'updated_at': '2016-08-24T08:29:28.332972+00:00', + 'service': service_id, + 'processing_finished': '2016-08-24T08:11:48.676365+00:00', + 'original_file_name': 'test-notify-email.csv', + 'created_by': { + 'name': 'test-user@digital.cabinet-office.gov.uk', + 'id': '3571f2ae-7a39-4fb4-9ad7-8453f5257072' + }, + 'created_at': '2016-08-24T08:09:56.371073+00:00', + 'template': 'c0309261-9c9e-4530-8fed-5f67b02260d2', + 'notification_count': 30, + 'notifications_sent': 10, + 'notifications_failed': 10, + 'notifications_delivered': 10, + 'processing_started': '2016-08-24T08:09:57.661246+00:00' + }} + + 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) + + mock_get.assert_called_once_with(url=expected_url, params={}) + assert result['data']['notifications_sent'] == 10 + assert result['data']['notification_count'] == 30 + assert result['data']['notifications_failed'] == 10 + + def test_client_parses_job_stats(mocker): mocker.patch('app.notify_client.current_user', id='1') From c61199d17d7c87673d905e5efbaa3bfe16dd4d6b Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 25 Aug 2016 09:15:55 +0100 Subject: [PATCH 4/4] Rewrote some slighlt over complicated logic --- app/main/views/jobs.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index f26203d75..0982f1a7a 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -78,14 +78,12 @@ def view_job(service_id, job_id): filter_args = _parse_filter_args(request.args) filter_args['status'] = _set_status_filters(filter_args) - finished = ( - bool(job.get('notification_count', 0) and (( - job.get('notifications_delivered', 0) + - job.get('notifications_failed', 0) - ) == job.get('notification_count', 0)))) + total_notifications = job.get('notification_count', 0) + processed_notifications = job.get('notifications_delivered', 0) + job.get('notifications_failed', 0) + return render_template( 'views/jobs/job.html', - finished=finished, + finished=(total_notifications == processed_notifications), uploaded_file_name=job['original_file_name'], template=Template( service_api_client.get_service_template(