Use a ModelList for lists of jobs

This follows the pattern of what we’ve done with services, users and
events.

It gives us a way of neatly instantiating a model for each item in the
list we get back from the API and reduces the complexity of the view
layer code.

Now is a good time to do this because we’re going to be making a bunch
of changes to the jobs pages, and those changes will be easier to code
and understand with a sensible model behind them.

app/main/views/dashboard.py

@@ -20,12 +20,11 @@ from app import (
     current_service,
     format_date_numeric,
     format_datetime_numeric,
-    job_api_client,
     service_api_client,
     template_statistics_client,
 )
 from app.main import main
-from app.statistics_utils import add_rate_to_job, get_formatted_percentage
+from app.statistics_utils import get_formatted_percentage
 from app.utils import (
     DELIVERED_STATUSES,
     FAILURE_STATUSES,
@@ -281,14 +280,6 @@ def get_dashboard_partials(service_id):
     all_statistics = template_statistics_client.get_template_statistics_for_service(service_id, limit_days=7)
     template_statistics = aggregate_template_usage(all_statistics)
 
-    scheduled_jobs, immediate_jobs = [], []
-    if job_api_client.has_jobs(service_id):
-        scheduled_jobs = job_api_client.get_scheduled_jobs(service_id)
-        immediate_jobs = [
-            add_rate_to_job(job)
-            for job in job_api_client.get_immediate_jobs(service_id)
-        ]
-
     stats = aggregate_notifications_stats(all_statistics)
     column_width, max_notifiction_count = get_column_properties(3)
 
@@ -310,7 +301,6 @@ def get_dashboard_partials(service_id):
     return {
         'upcoming': render_template(
             'views/dashboard/_upcoming.html',
-            scheduled_jobs=scheduled_jobs
         ),
         'inbox': render_template(
             'views/dashboard/_inbox.html',
@@ -337,9 +327,8 @@ def get_dashboard_partials(service_id):
         ),
         'jobs': render_template(
             'views/dashboard/_jobs.html',
-            jobs=immediate_jobs
+            jobs=current_service.immediate_jobs,
         ),
-        'has_jobs': bool(immediate_jobs),
         'usage': render_template(
             'views/dashboard/_usage.html',
             column_width=column_width,

app/main/views/jobs.py

@@ -19,14 +19,12 @@ from app import (
     current_service,
     format_datetime_short,
     format_thousands,
-    job_api_client,
     notification_api_client,
     service_api_client,
 )
 from app.main import main
 from app.main.forms import SearchNotificationsForm
 from app.models.job import Job
-from app.statistics_utils import add_rate_to_job
 from app.utils import (
     generate_next_dict,
     generate_notifications_csv,
@@ -44,31 +42,25 @@ from app.utils import (
 @main.route("/services/<uuid:service_id>/jobs")
 @user_has_permissions()
 def view_jobs(service_id):
-    page = int(request.args.get('page', 1))
-    jobs_response = job_api_client.get_page_of_jobs(service_id, page=page)
-    jobs = [
-        add_rate_to_job(job) for job in jobs_response['data']
-    ]
+    jobs = current_service.get_page_of_jobs(page=request.args.get('page'))
 
     prev_page = None
-    if jobs_response['links'].get('prev', None):
-        prev_page = generate_previous_dict('main.view_jobs', service_id, page)
+    if jobs.prev_page:
+        prev_page = generate_previous_dict('main.view_jobs', service_id, jobs.current_page)
     next_page = None
-    if jobs_response['links'].get('next', None):
-        next_page = generate_next_dict('main.view_jobs', service_id, page)
+    if jobs.next_page:
+        next_page = generate_next_dict('main.view_jobs', service_id, jobs.current_page)
 
     scheduled_jobs = ''
-    if not current_user.has_permissions('view_activity') and page == 1:
+    if not current_user.has_permissions('view_activity') and jobs.current_page == 1:
         scheduled_jobs = render_template(
             'views/dashboard/_upcoming.html',
-            scheduled_jobs=job_api_client.get_scheduled_jobs(service_id),
             hide_heading=True,
         )
 
     return render_template(
         'views/jobs/jobs.html',
         jobs=jobs,
-        page=page,
         prev_page=prev_page,
         next_page=next_page,
         scheduled_jobs=scheduled_jobs,

app/main/views/uploads.py

@@ -15,12 +15,7 @@ from notifications_utils.pdf import pdf_page_count
 from PyPDF2.utils import PdfReadError
 from requests import RequestException
 
-from app import (
-    current_service,
-    job_api_client,
-    notification_api_client,
-    service_api_client,
-)
+from app import current_service, notification_api_client, service_api_client
 from app.extensions import antivirus_client
 from app.main import main
 from app.main.forms import LetterUploadPostageForm, PDFUploadForm
@@ -47,20 +42,18 @@ MAX_FILE_UPLOAD_SIZE = 2 * 1024 * 1024  # 2MB
 def uploads(service_id):
     # No tests have been written, this has been quickly prepared for user research.
     # It's also very like that a new view will be created to show uploads.
-    page = int(request.args.get('page', 1))
-    uploads_response = job_api_client.get_uploads(service_id, page=page)
+    uploads = current_service.get_page_of_uploads(page=request.args.get('page'))
 
     prev_page = None
-    if uploads_response['links'].get('prev', None):
-        prev_page = generate_previous_dict('main.uploads', service_id, page)
+    if uploads.next_page:
+        prev_page = generate_previous_dict('main.uploads', service_id, uploads.current_page)
     next_page = None
-    if uploads_response['links'].get('next', None):
-        next_page = generate_next_dict('main.uploads', service_id, page)
+    if uploads.prev_page:
+        next_page = generate_next_dict('main.uploads', service_id, uploads.current_page)
 
     return render_template(
         'views/jobs/jobs.html',
-        jobs=uploads_response['data'],
-        page=page,
+        jobs=uploads,
         prev_page=prev_page,
         next_page=next_page,
         scheduled_jobs='',

app/models/job.py

@@ -7,7 +7,7 @@ from notifications_utils.letter_timings import (
 )
 from werkzeug.utils import cached_property
 
-from app.models import JSONModel
+from app.models import JSONModel, ModelList
 from app.notify_client.job_api_client import job_api_client
 from app.notify_client.notification_api_client import notification_api_client
 from app.notify_client.service_api_client import service_api_client
@@ -147,6 +147,20 @@ class Job(JSONModel):
     def letter_timings(self):
         return get_letter_timings(self.created_at, postage=self.postage)
 
+    @property
+    def failure_rate(self):
+        if not self.notifications_delivered:
+            return 100 if self.notifications_failed else 0
+        return (
+            self.notifications_failed / (
+                self.notifications_failed + self.notifications_delivered
+            ) * 100
+        )
+
+    @property
+    def high_failure_rate(self):
+        return self.failure_rate > 30
+
     def get_notifications(self, status):
         return notification_api_client.get_notifications_for_service(
             self.service, self.id, status=status,
@@ -157,3 +171,31 @@ class Job(JSONModel):
             return job_api_client.cancel_letter_job(self.service, self.id)
         else:
             return job_api_client.cancel_job(self.service, self.id)
+
+
+class ImmediateJobs(ModelList):
+    client = job_api_client.get_immediate_jobs
+    model = Job
+
+
+class ScheduledJobs(ImmediateJobs):
+    client = job_api_client.get_scheduled_jobs
+
+
+class PaginatedJobs(ImmediateJobs):
+
+    client = job_api_client.get_page_of_jobs
+
+    def __init__(self, service_id, page):
+        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)
+
+
+class PaginatedUploads(PaginatedJobs):
+    client = job_api_client.get_uploads

app/models/service.py

@@ -5,6 +5,12 @@ from notifications_utils.take import Take
 from werkzeug.utils import cached_property
 
 from app.models import JSONModel
+from app.models.job import (
+    ImmediateJobs,
+    PaginatedJobs,
+    PaginatedUploads,
+    ScheduledJobs,
+)
 from app.models.organisation import Organisation
 from app.models.user import InvitedUsers, User, Users
 from app.notify_client.api_key_api_client import api_key_api_client
@@ -103,10 +109,28 @@ class Service(JSONModel):
     def has_permission(self, permission):
         return permission in self.permissions
 
+    def get_page_of_jobs(self, page):
+        return PaginatedJobs(self.id, page=page)
+
+    def get_page_of_uploads(self, page):
+        return PaginatedUploads(self.id, page=page)
+
     @cached_property
     def has_jobs(self):
         return job_api_client.has_jobs(self.id)
 
+    @cached_property
+    def immediate_jobs(self):
+        if not self.has_jobs:
+            return []
+        return ImmediateJobs(self.id)
+
+    @cached_property
+    def scheduled_jobs(self):
+        if not self.has_jobs:
+            return []
+        return ScheduledJobs(self.id)
+
     @cached_property
     def invited_users(self):
         return InvitedUsers(self.id)

app/statistics_utils.py

@@ -78,19 +78,3 @@ def statistics_by_state(statistics):
             'failed': statistics['emails_failed']
         }
     }
-
-
-def get_failure_rate_for_job(job):
-    if not job.get('notifications_delivered'):
-        return 1 if job.get('notifications_failed') else 0
-    return (
-        job.get('notifications_failed', 0) /
-        (job.get('notifications_failed', 0) + job.get('notifications_delivered', 0))
-    )
-
-
-def add_rate_to_job(job):
-    return dict(
-        failure_rate=(get_failure_rate_for_job(job)) * 100,
-        **job
-    )

app/templates/views/dashboard/_jobs.html

@@ -26,22 +26,22 @@
         {% endif %}
         <span class="file-list-hint">
           Sent {{
-            (item.scheduled_for if item.scheduled_for else item.created_at)|format_datetime_relative
+            (item.scheduled_for or item.created_at)|format_datetime_relative
           }}
         </span>
       </div>
     {% endcall %}
     {% call field() %}
       {{ big_number(
-        item.get('notification_count', 0) - item.get('notifications_delivered', 0) - item.get('notifications_failed', 0),
+        item.notifications_sending,
         smallest=True
       ) }}
     {% endcall %}
     {% call field() %}
-      {{ big_number(item.get('notifications_delivered', 0), smallest=True) }}
+      {{ big_number(item.notifications_delivered, smallest=True) }}
     {% endcall %}
-    {% call field(status='error' if item.get('failure_rate', 0) > 3 else '') %}
-      {{ big_number(item.get('notifications_failed', 0), smallest=True) }}
+    {% call field(status='error' if item.high_failure_rate else '') %}
+      {{ big_number(item.notifications_failed, smallest=True) }}
     {% endcall %}
   {% endcall %}
 </div>

app/templates/views/dashboard/_upcoming.html

@@ -3,7 +3,7 @@
 {% from "components/show-more.html" import show_more %}
 
 <div class="ajax-block-container">
-  {% if scheduled_jobs %}
+  {% if current_service.scheduled_jobs %}
     <div class='dashboard-table'>
       {% if not hide_heading %}
         <h2 class="heading-medium heading-upcoming-jobs">
@@ -11,7 +11,7 @@
         </h2>
       {% endif %}
       {% call(item, row_number) list_table(
-        scheduled_jobs,
+        current_service.scheduled_jobs,
         caption="In the next few days",
         caption_visible=False,
         empty_message='Nothing to see here',

app/templates/views/dashboard/dashboard.html

@@ -35,7 +35,7 @@
 
     {{ ajax_block(partials, updates_url, 'template-statistics', interval=5) }}
 
-    {% if partials['has_jobs'] %}
+    {% if current_service.immediate_jobs %}
       {{ ajax_block(partials, updates_url, 'jobs', interval=5) }}
       {{ show_more(
         url_for('.view_jobs', service_id=current_service.id),

tests/app/main/views/test_dashboard.py

@@ -1366,7 +1366,8 @@ def test_should_show_all_jobs_with_valid_statuses(
     mock_get_service_templates_when_no_templates_exist,
     mock_get_jobs,
     mock_get_usage,
-    mock_get_inbound_sms_summary
+    mock_get_inbound_sms_summary,
+    mock_get_free_sms_fragment_limit,
 ):
     logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID))
 

tests/app/main/views/test_jobs.py

@@ -518,7 +518,7 @@ def test_should_cancel_job(
     mock_get_service_template,
     mocker,
 ):
-    mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_job')
+    mock_cancel = mocker.patch('app.job_api_client.cancel_job')
     client_request.post(
         'main.cancel_job',
         service_id=SERVICE_ONE_ID,
@@ -687,7 +687,7 @@ def test_dont_cancel_letter_job_when_to_early_to_cancel(
         'app.notification_api_client.get_notification_count_for_job_id', return_value=number_of_processed_notifications
     )
 
-    mock_cancel = mocker.patch('app.main.jobs.job_api_client.cancel_letter_job')
+    mock_cancel = mocker.patch('app.job_api_client.cancel_letter_job')
     page = client_request.post(
         'main.cancel_letter_job',
         service_id=SERVICE_ONE_ID,

tests/app/test_statistics_utils.py

@@ -1,7 +1,7 @@
 import pytest
 
+from app.models.job import Job
 from app.statistics_utils import (
-    add_rate_to_job,
     add_rates_to,
     statistics_by_state,
     sum_of_statistics,
@@ -128,7 +128,7 @@ def test_service_statistics_by_state():
     (1, 4, 20)
 ])
 def test_add_rate_to_job_calculates_rate(failed, delivered, expected_failure_rate):
-    resp = add_rate_to_job(
+    resp = Job(
         {
             'notifications_failed': failed,
             'notifications_delivered': delivered,
@@ -136,11 +136,11 @@ def test_add_rate_to_job_calculates_rate(failed, delivered, expected_failure_rat
         }
     )
 
-    assert resp['failure_rate'] == expected_failure_rate
+    assert resp.failure_rate == expected_failure_rate
 
 
 def test_add_rate_to_job_preserves_initial_fields():
-    resp = add_rate_to_job(
+    resp = Job(
         {
             'notifications_failed': 0,
             'notifications_delivered': 0,
@@ -148,4 +148,5 @@ def test_add_rate_to_job_preserves_initial_fields():
         }
     )
 
-    assert set(resp.keys()) == {'notifications_failed', 'notifications_delivered', 'id', 'failure_rate'}
+    assert resp.notifications_failed == resp.notifications_delivered == resp.failure_rate == 0
+    assert resp.id == 'foo'

tests/conftest.py

@@ -1732,12 +1732,16 @@ def mock_get_uploads(mocker, api_user_active):
                     'notification_count': 10,
                     'created_at': '2016-01-01 11:09:00.061258',
                     'statistics': [{'count': 8, 'status': 'delivered'}, {'count': 2, 'status': 'temporary-failure'}],
+                    'scheduled_for': None,
+                    'job_status': 'finished',
                     'upload_type': 'job'},
                    {'id': 'job_id_1',
                     'original_file_name': 'some.csv',
                     'notification_count': 1,
                     'created_at': '2016-01-01 11:09:00.061258',
                     'statistics': [{'count': 1, 'status': 'delivered'}],
+                    'scheduled_for': None,
+                    'job_status': 'finished',
                     'upload_type': 'letter'}
                    ]
         return {
@@ -1747,8 +1751,8 @@ def mock_get_uploads(mocker, api_user_active):
                 'next': 'services/{}/uploads?page={}'.format(service_id, page + 1)
             }
         }
-
-    return mocker.patch('app.job_api_client.get_uploads', side_effect=_get_uploads)
+    # Why is mocking on the model needed?
+    return mocker.patch('app.models.job.PaginatedUploads.client', side_effect=_get_uploads)
 
 
 @pytest.fixture(scope='function')