From 7324b30dbc6d3d1fa0c13569194ab92ba439c786 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Aug 2016 14:50:38 +0100 Subject: [PATCH 1/4] Re-add a 'requested' stat to a job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s useful to know how many notifications we’ve handed off to our providers. This is a measure of how complete the processing of the job is. This is important, because once the job processing is complete then you can accurately reconcile the report with the CSV file that you’ve uploaded. --- app/notify_client/job_api_client.py | 6 +++++- tests/app/notify_client/test_job_client.py | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index ddd08ad9b..dc3250ef4 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -19,7 +19,8 @@ class JobApiClient(BaseAPIClient): results = { 'sending': 0, 'delivered': 0, - 'failed': 0 + 'failed': 0, + 'requested': 0 } if 'statistics' in job: for outcome in job['statistics']: @@ -29,6 +30,7 @@ class JobApiClient(BaseAPIClient): results['sending'] += outcome['count'] if outcome['status'] in ['delivered']: results['delivered'] += outcome['count'] + results['requested'] += outcome['count'] return results def get_job(self, service_id, job_id=None, limit_days=None, status=None): @@ -42,6 +44,7 @@ class JobApiClient(BaseAPIClient): 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 params = {} @@ -55,6 +58,7 @@ class JobApiClient(BaseAPIClient): job['notifications_sent'] = stats['delivered'] + stats['failed'] job['notifications_delivered'] = stats['delivered'] job['notifications_failed'] = stats['failed'] + job['notifications_requested'] = stats['requested'] return jobs diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index a6ba9c937..5009c7c6a 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/tests/app/notify_client/test_job_client.py @@ -154,6 +154,7 @@ def test_client_parses_job_stats(mocker): result = client.get_job(service_id, job_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 @@ -191,6 +192,7 @@ def test_client_parses_empty_job_stats(mocker): result = client.get_job(service_id, job_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 @@ -265,10 +267,12 @@ def test_client_parses_job_stats_for_service(mocker): mock_get.assert_called_once_with(url=expected_url, params={}) 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 @@ -325,10 +329,12 @@ def test_client_parses_empty_job_stats_for_service(mocker): mock_get.assert_called_once_with(url=expected_url, params={}) 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 From 37a1eb22b07cd589ae8cb51f78b833a6196e2951 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Aug 2016 14:54:33 +0100 Subject: [PATCH 2/4] Use default dict for default job stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is just some refactoring. `defaultdict` is a data structure which won’t raise a `KeyError` if you try to access a key that doesn’t exist. By passing `int` as the first argument, trying to access the value of any key that doesn’t exists will return the value of `int()`, ie `0` --- app/notify_client/job_api_client.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index dc3250ef4..bd9f06d57 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -1,3 +1,4 @@ +from collections import defaultdict from notifications_python_client.base import BaseAPIClient from app.notify_client import _attach_current_user @@ -16,12 +17,7 @@ class JobApiClient(BaseAPIClient): @staticmethod def __convert_statistics(job): - results = { - 'sending': 0, - 'delivered': 0, - 'failed': 0, - 'requested': 0 - } + results = defaultdict(int) if 'statistics' in job: for outcome in job['statistics']: if outcome['status'] in ['failed', 'technical-failure', 'temporary-failure', 'permanent-failure']: From bf872ab3421b668aedcc1780d4305dbd86ef7055 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 Aug 2016 15:13:37 +0100 Subject: [PATCH 3/4] =?UTF-8?q?Base=20=E2=80=98%=20complete=E2=80=99=20on?= =?UTF-8?q?=20notifications=20requested?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job is complete when all notifications are delivered or failed. The report is complete once we have all notifications are in the database. This commit changes the meaning of the percentage from the former to the latter. This is how it was before we removed the aggregate stats for jobs. --- app/main/views/jobs.py | 2 +- tests/__init__.py | 2 ++ tests/conftest.py | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 90e0c0429..1b9c5cc8a 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -326,7 +326,7 @@ def get_job_partials(job): 'partials/jobs/notifications.html', notifications=notifications['notifications'], more_than_one_page=bool(notifications.get('links', {}).get('next')), - percentage_complete=(job['notifications_sent'] / job['notification_count'] * 100), + percentage_complete=(job['notifications_requested'] / job['notification_count'] * 100), download_link=url_for( '.view_job_csv', service_id=current_service['id'], diff --git a/tests/__init__.py b/tests/__init__.py index bfd085d3d..cc7540638 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -156,6 +156,7 @@ def job_json(service_id, original_file_name="thisisatest.csv", notification_count=1, notifications_sent=1, + notifications_requested=1, status=None): if job_id is None: job_id = str(generate_uuid()) @@ -174,6 +175,7 @@ def job_json(service_id, 'created_at': created_at, 'notification_count': notification_count, 'notifications_sent': notifications_sent, + 'notifications_requested': notifications_requested, 'status': status, 'created_by': created_by_json( created_by.id, diff --git a/tests/conftest.py b/tests/conftest.py index b86f829c0..854b44baf 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -864,7 +864,7 @@ def mock_get_job_in_progress(mocker, api_user_active): return {"data": job_json( service_id, api_user_active, job_id=job_id, notification_count=10, - notifications_sent=5 + notifications_requested=5 )} return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) From 40e22d8258dc336f52f64b54f78d3912dce48feb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 31 Aug 2016 10:46:43 +0100 Subject: [PATCH 4/4] Remove check for statistics not being on job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A job will always have statistics. It’s always assigned: https://github.com/alphagov/notifications-api/blob/bcfa83de79e11856f12a7581b6519ec4a66dad36/app/job/rest.py#L48 And the structure should always be the same, even if the counts are zero because they’re generated from this query: https://github.com/alphagov/notifications-api/blob/668e6c9716cfffdb987cbf8abe20c14576864a1c/app/dao/jobs_dao.py#L11-L22 This line suggests that it’s a hangover from the aggregate tables: https://github.com/alphagov/notifications-admin/commit/8c159da3eaf3a8100bdc2603b4addf25b9fe573d#diff-9886486bf41b0680d23588b190c252eaL24 Since it’s no long necessary this commit removes it. --- app/notify_client/job_api_client.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index bd9f06d57..10fe9c2f1 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -18,15 +18,14 @@ class JobApiClient(BaseAPIClient): @staticmethod def __convert_statistics(job): results = defaultdict(int) - 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'] - results['requested'] += outcome['count'] + 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'] + results['requested'] += outcome['count'] return results def get_job(self, service_id, job_id=None, limit_days=None, status=None):