Merge pull request #3263 from alphagov/fix-new-jobs-showing-as-deleted

Use time to determine why notifications don’t exist

app/main/views/new_password.py

@@ -1,5 +1,4 @@
 import json
-from datetime import datetime
 
 from flask import (
     current_app,
@@ -29,8 +28,7 @@ def new_password(token):
 
     email_address = json.loads(token_data)['email']
     user = User.from_email_address(email_address)
-    if user.password_changed_at and datetime.strptime(user.password_changed_at, '%Y-%m-%d %H:%M:%S.%f') > \
-            datetime.strptime(json.loads(token_data)['created_at'], '%Y-%m-%d %H:%M:%S.%f'):
+    if user.password_changed_more_recently_than(json.loads(token_data)['created_at']):
         flash('The link in the email has already been used')
         return redirect(url_for('main.index'))
 

app/main/views/service_settings.py

@@ -14,6 +14,7 @@ from flask import (
 )
 from flask_login import current_user
 from notifications_python_client.errors import HTTPError
+from notifications_utils.timezones import utc_string_to_aware_gmt_datetime
 
 from app import (
     billing_api_client,
@@ -475,8 +476,10 @@ def get_service_verify_reply_to_address_partials(service_id, notification_id):
                     email_address=notification["to"],
                     is_default=is_default
                 )
-    created_at_no_tz = notification["created_at"][:-6]
-    seconds_since_sending = (datetime.utcnow() - datetime.strptime(created_at_no_tz, '%Y-%m-%dT%H:%M:%S.%f')).seconds
+    seconds_since_sending = (
+        utc_string_to_aware_gmt_datetime(datetime.utcnow().isoformat()) -
+        utc_string_to_aware_gmt_datetime(notification['created_at'])
+    ).seconds
     if notification["status"] in FAILURE_STATUSES or (
         notification["status"] in SENDING_STATUSES and
         seconds_since_sending > current_app.config['REPLY_TO_EMAIL_ADDRESS_VALIDATION_TIMEOUT']

app/models/job.py

@@ -5,6 +5,10 @@ from notifications_utils.letter_timings import (
     get_letter_timings,
     letter_can_be_cancelled,
 )
+from notifications_utils.timezones import (
+    local_timezone,
+    utc_string_to_aware_gmt_datetime,
+)
 from werkzeug.utils import cached_property
 
 from app.models import JSONModel, ModelList
@@ -23,6 +27,7 @@ class Job(JSONModel):
         'template_version',
         'original_file_name',
         'created_at',
+        'processing_started',
         'notification_count',
         'created_by',
     }
@@ -47,6 +52,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 utc_string_to_aware_gmt_datetime(self._dict['processing_started'])
+
     def _aggregate_statistics(self, *statuses):
         return sum(
             outcome['count'] for outcome in self._dict['statistics']
@@ -94,6 +105,17 @@ class Job(JSONModel):
     def finished_processing(self):
         return self.notification_count == self.notifications_sent
 
+    @property
+    def awaiting_processing_or_recently_processed(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().astimezone(local_timezone) - self.processing_started
+        ).days < 1
+
     @property
     def template_id(self):
         return self._dict['template']
@@ -124,7 +146,8 @@ class Job(JSONModel):
             return False
 
         if not letter_can_be_cancelled(
-            'created', datetime.strptime(self.created_at[:-6], '%Y-%m-%dT%H:%M:%S.%f')
+            'created',
+            utc_string_to_aware_gmt_datetime(self.created_at).replace(tzinfo=None)
         ):
             return False
 

app/models/user.py

@@ -1,6 +1,7 @@
 from flask import abort, current_app, request, session
 from flask_login import AnonymousUserMixin, UserMixin, login_user
 from notifications_python_client.errors import HTTPError
+from notifications_utils.timezones import utc_string_to_aware_gmt_datetime
 from werkzeug.utils import cached_property
 
 from app.models import JSONModel, ModelList
@@ -108,6 +109,15 @@ class User(JSONModel, UserMixin):
         response = user_api_client.update_password(self.id, password)
         self.__init__(response)
 
+    def password_changed_more_recently_than(self, datetime_string):
+        if not self.password_changed_at:
+            return False
+        return utc_string_to_aware_gmt_datetime(
+            self.password_changed_at
+        ) > utc_string_to_aware_gmt_datetime(
+            datetime_string
+        )
+
     def set_permissions(self, service_id, permissions, folder_permissions):
         user_api_client.set_user_permissions(
             self.id,

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.status == 'finished' else 'No messages to show yet…',
+        empty_message='No messages to show yet…' if job.awaiting_processing_or_recently_processed else 'These messages have been deleted because they were sent more than {} days ago'.format(service_data_retention_days),
         field_headings=[
           'Recipient',
           'Status'

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
 
 

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,68 @@ def test_should_show_job_without_notifications(
     assert page.select_one('tbody').text.strip() == 'No messages to show yet…'
 
 
+@freeze_time("2020-01-10 1: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 just within the last 24h
+    (datetime(2020, 1, 1, 0, 0, 0), datetime(2020, 1, 9, 1, 0, 1), (
+        'No messages to show yet…'
+    )),
+    # Created a while ago, started exactly 24h ago
+    # ---
+    # It doesn’t matter that 24h (1 day) and 7 days (the service’s data
+    # retention) don’t match up. We’re testing the case of no
+    # notifications existing more than 1 day after the job started
+    # processing. In this case we assume it’s because the service’s
+    # data retention has kicked in.
+    (datetime(2020, 1, 1, 0, 0, 0), datetime(2020, 1, 9, 1, 0, 0), (
+        '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")