Move stats to the model

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

@@ -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
+    @cached_property
-    def notification_count(self):
+    def statistics(self):
-        return self._dict.get('notification_count', 0)
+        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 (
+        return self.notification_count - self.notifications_sent
-            self.notification_count -
-            self.notifications_delivered -
-            self.notifications_failed
-        )
 
     @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.prev_page = response.get('links', {}).get('prev', None)
-        self.next_page = response['links'].get('next', None)
+        self.next_page = response.get('links', {}).get('next', None)
 
 
 class PaginatedUploads(PaginatedJobs):

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)
+        return 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
 
     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)
+        return 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
 
     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):
-
+        return self.post(
-        job = 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(

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.notifications_requested == 80
-    assert result['data']['notifications_sent'] == 50
+    assert result.notifications_sent == 50
-    assert result['data']['notification_count'] == 80
+    assert result.notification_count == 80
-    assert result['data']['notifications_failed'] == 40
+    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.notifications_requested == 0
-    assert result['data']['notifications_sent'] == 0
+    assert result.notifications_sent == 0
-    assert result['data']['notification_count'] == 80
+    assert result.notification_count == 80
-    assert result['data']['notifications_failed'] == 0
+    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})
+    mock_get.assert_called_once_with(url=expected_url, params={'page': 1, 'statuses': ANY})
-    assert result['data'][0]['id'] == job_1_id
+    assert result[0].id == job_1_id
-    assert result['data'][0]['notifications_requested'] == 80
+    assert result[0].notifications_requested == 80
-    assert result['data'][0]['notifications_sent'] == 50
+    assert result[0].notifications_sent == 50
-    assert result['data'][0]['notification_count'] == 80
+    assert result[0].notification_count == 80
-    assert result['data'][0]['notifications_failed'] == 40
+    assert result[0].notifications_failed == 40
-    assert result['data'][1]['id'] == job_2_id
+    assert result[1].id == job_2_id
-    assert result['data'][1]['notifications_requested'] == 40
+    assert result[1].notifications_requested == 40
-    assert result['data'][1]['notifications_sent'] == 25
+    assert result[1].notifications_sent == 25
-    assert result['data'][1]['notification_count'] == 40
+    assert result[1].notification_count == 40
-    assert result['data'][1]['notifications_failed'] == 20
+    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})
+    mock_get.assert_called_once_with(url=expected_url, params={'page': 1, 'statuses': ANY})
-    assert result['data'][0]['id'] == job_1_id
+    assert result[0].id == job_1_id
-    assert result['data'][0]['notifications_requested'] == 0
+    assert result[0].notifications_requested == 0
-    assert result['data'][0]['notifications_sent'] == 0
+    assert result[0].notifications_sent == 0
-    assert result['data'][0]['notification_count'] == 80
+    assert result[0].notification_count == 80
-    assert result['data'][0]['notifications_failed'] == 0
+    assert result[0].notifications_failed == 0
-    assert result['data'][1]['id'] == job_2_id
+    assert result[1].id == job_2_id
-    assert result['data'][1]['notifications_requested'] == 0
+    assert result[1].notifications_requested == 0
-    assert result['data'][1]['notifications_sent'] == 0
+    assert result[1].notifications_sent == 0
-    assert result['data'][1]['notification_count'] == 40
+    assert result[1].notification_count == 40
-    assert result['data'][1]['notifications_failed'] == 0
+    assert result[1].notifications_failed == 0
 
 
 def test_cancel_job(mocker):

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(
+    resp = Job({
-        {
+        'statistics': [
-            'notifications_failed': failed,
+            {'status': 'failed', 'count': failed},
-            'notifications_delivered': delivered,
+            {'status': 'delivered', 'count': delivered},
-            'id': 'foo'
+        ],
-        }
+        'id': 'foo',
-    )
+    })
 
     assert resp.failure_rate == expected_failure_rate
 
 
 def test_add_rate_to_job_preserves_initial_fields():
-    resp = Job(
+    resp = Job({
-        {
+        'statistics': [
-            'notifications_failed': 0,
+            {'status': 'failed', 'count': 0},
-            'notifications_delivered': 0,
+            {'status': 'delivered', 'count': 0},
-            'id': 'foo'
+        ],
-        }
+        'id': 'foo',
-    )
+    })
 
     assert resp.notifications_failed == resp.notifications_delivered == resp.failure_rate == 0
     assert resp.id == 'foo'