From 87b26868750419af62b54d827970d3f1286a8e7e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 16 Jan 2020 16:58:26 +0000 Subject: [PATCH] =?UTF-8?q?Use=20time=20to=20determine=20why=20notificatio?= =?UTF-8?q?ns=20don=E2=80=99t=20exist?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Notifications won’t exist for a job if: - it’s just started - it started a long time ago (older than the retention period) We have a bug where: 1. Job starts processing, puts notifications on queue 2. Job finishes processing, sets status to `finished` 3. First notification gets picked up off the queue and put in the database In between 2. and 3. it’s possible for a job to be finished, but also to have no notifications. We’re saying this is because the notifications have been deleted, whereas really it’s because they haven’t been created yet. This commit fixes that bug by introducing the concept of recency for jobs. ‘Recent’ is defined as 1 day, which is: - a lot longer than it takes to create any notifications - a bit shorter than anyone’s retention time N.B. `processing_started` is defined here: https://github.com/alphagov/notifications-api/blob/879ba1d5f092dbf6e15bf9adadd53e4657660490/app/models.py#L1194 It can be `None` for scheduled jobs that haven’t started yet. --- app/models/job.py | 17 ++++++++ .../partials/jobs/notifications.html | 2 +- tests/__init__.py | 8 +++- tests/app/main/views/test_jobs.py | 41 ++++++++++++++++--- 4 files changed, 60 insertions(+), 8 deletions(-) diff --git a/app/models/job.py b/app/models/job.py index 855aa1ce8..76ca7d2f1 100644 --- a/app/models/job.py +++ b/app/models/job.py @@ -48,6 +48,12 @@ class Job(JSONModel): def scheduled_for(self): return self._dict.get('scheduled_for') + @property + def processing_started(self): + if not self._dict.get('processing_started'): + return None + return datetime.strptime(self._dict['processing_started'][:-6], '%Y-%m-%dT%H:%M:%S') + def _aggregate_statistics(self, *statuses): return sum( outcome['count'] for outcome in self._dict['statistics'] @@ -95,6 +101,17 @@ class Job(JSONModel): def finished_processing(self): return self.notification_count == self.notifications_sent + @property + def recently_created(self): + if not self.processing_started: + # Assume that if processing hasn’t started yet then the job + # must have been created recently enough to not have any + # notifications yet + return True + return ( + datetime.utcnow() - self.processing_started + ).days < 1 + @property def template_id(self): return self._dict['template'] diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 16b6c449a..41d55c73d 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -47,7 +47,7 @@ notifications, caption=uploaded_file_name, caption_visible=False, - empty_message='These messages have been deleted because they were sent more than {} days ago'.format(service_data_retention_days) if job.job_status == 'finished' else 'No messages to show yet…', + empty_message='No messages to show yet…' if job.recently_created else 'These messages have been deleted because they were sent more than {} days ago'.format(service_data_retention_days), field_headings=[ 'Recipient', 'Status' diff --git a/tests/__init__.py b/tests/__init__.py index 4348d7c3a..5555b853e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -361,7 +361,8 @@ def job_json( notifications_sent=1, notifications_requested=1, job_status='finished', - scheduled_for='' + scheduled_for='', + processing_started=None, ): if job_id is None: job_id = str(generate_uuid()) @@ -391,8 +392,11 @@ def job_json( created_by['name'], created_by['email_address'], ), - 'scheduled_for': scheduled_for } + if scheduled_for: + data.update(scheduled_for=scheduled_for) + if processing_started: + data.update(processing_started=processing_started) return data diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index f8723505a..b54e2431f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -1,5 +1,6 @@ import json import uuid +from datetime import datetime, timezone import pytest from flask import url_for @@ -329,28 +330,58 @@ def test_should_show_job_without_notifications( assert page.select_one('tbody').text.strip() == 'No messages to show yet…' +@freeze_time("2020-01-10 0:0:0") +@pytest.mark.parametrize('created_at, processing_started, expected_message', ( + # Recently created, not yet started + (datetime(2020, 1, 10, 0, 0, 0), None, ( + 'No messages to show yet…' + )), + # Just started + (datetime(2020, 1, 10, 0, 0, 0), datetime(2020, 1, 10, 0, 0, 1), ( + 'No messages to show yet…' + )), + # Created a while ago, just started + (datetime(2020, 1, 1, 0, 0, 0), datetime(2020, 1, 10, 0, 0, 1), ( + 'No messages to show yet…' + )), + # Created a while ago, started a couple of days ago + (datetime(2020, 1, 1, 0, 0, 0), datetime(2020, 1, 8, 0, 0, 1), ( + 'These messages have been deleted because they were sent more than 7 days ago' + )), +)) def test_should_show_old_job( client_request, service_one, active_user_with_permissions, mock_get_service_template, - mock_get_job, mocker, mock_get_notifications_with_no_notifications, mock_get_service_data_retention, fake_uuid, + created_at, + processing_started, + expected_message, ): + mocker.patch('app.job_api_client.get_job', return_value={ + "data": job_json( + SERVICE_ONE_ID, + active_user_with_permissions, + created_at=created_at.replace(tzinfo=timezone.utc).isoformat(), + processing_started=( + processing_started.replace(tzinfo=timezone.utc).isoformat() + if processing_started else None + ), + ), + }) page = client_request.get( 'main.view_job', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, job_id=fake_uuid, ) assert not page.select('.pill a') assert not page.select('p.hint') assert not page.select('a[download]') - assert page.select_one('tbody').text.strip() == ( - 'These messages have been deleted because they were sent more than 7 days ago' - ) + assert page.select_one('tbody').text.strip() == expected_message @freeze_time("2016-01-01 11:09:00.061258")