Group the letters by postage class.

In this PR the letters are grouped and sent by postage type.
This means we can group the first class letters first and start sending them.

This PR is to share an idea... tests have not been written and an order to postage hasn't been added. There is also a PR in the works to add the new postage classes so we should consider that.

app/celery/letters_pdf_tasks.py

@@ -52,7 +52,7 @@ from app.models import (
     NOTIFICATION_TECHNICAL_FAILURE,
     NOTIFICATION_VALIDATION_FAILED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
-)
+    POSTAGE_TYPES)
 from app.cronitor import cronitor
 
 
@@ -136,51 +136,55 @@ def collate_letter_pdfs_to_be_sent():
         hour=17, minute=30, second=0, microsecond=0
     )
 
-    letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline)
+    for postage in POSTAGE_TYPES:
+        letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage)
 
-    for i, letters in enumerate(group_letters(letters_to_print)):
-        filenames = [letter['Key'] for letter in letters]
+        for i, letters in enumerate(group_letters(letters_to_print)):
+            filenames = [letter['Key'] for letter in letters]
 
-        hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode()
-        # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP
-        dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format(
-            date=print_run_deadline.strftime("%Y-%m-%d"),
-            num=i + 1,
-            hash=hash
-        )
-
-        current_app.logger.info(
-            'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format(
-                len(filenames),
-                dvla_filename,
-                sum(letter['Size'] for letter in letters)
+            hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode()
+            # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP
+            dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format(
+                date=print_run_deadline.strftime("%Y-%m-%d"),
+                num=i + 1,
+                hash=hash
+            )
+
+            current_app.logger.info(
+                'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format(
+                    len(filenames),
+                    dvla_filename,
+                    sum(letter['Size'] for letter in letters)
+                )
+            )
+            notify_celery.send_task(
+                name=TaskNames.ZIP_AND_SEND_LETTER_PDFS,
+                kwargs={
+                    'filenames_to_zip': filenames,
+                    'upload_filename': dvla_filename
+                },
+                queue=QueueNames.PROCESS_FTP,
+                compression='zlib'
             )
-        )
-        notify_celery.send_task(
-            name=TaskNames.ZIP_AND_SEND_LETTER_PDFS,
-            kwargs={
-                'filenames_to_zip': filenames,
-                'upload_filename': dvla_filename
-            },
-            queue=QueueNames.PROCESS_FTP,
-            compression='zlib'
-        )
 
 
-def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline):
-    letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline)
+def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage):
+    letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline, postage)
 
     letter_pdfs = []
     for letter in letters_awaiting_sending:
-        letter_file_name = get_letter_pdf_filename(
-            reference=letter.reference,
-            crown=letter.service.crown,
-            sending_date=letter.created_at,
-            postage=letter.postage
-        )
-
-        letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name)
-        letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']})
+        try:
+            letter_file_name = get_letter_pdf_filename(
+                reference=letter.reference,
+                crown=letter.service.crown,
+                sending_date=letter.created_at,
+                postage=letter.postage
+            )
+            letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name)
+            letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']})
+        except BotoClientError as e:
+            current_app.logger.exception(
+                f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}")
 
     return letter_pdfs
 

app/celery/scheduled_tasks.py

@@ -205,7 +205,7 @@ def replay_created_notifications():
 
         current_app.logger.info(msg)
         for letter in letters:
-            create_letters_pdf.apply_async([letter.id], queue=QueueNames.LETTERS)
+            create_letters_pdf.apply_async([str(letter.id)], queue=QueueNames.LETTERS)
 
 
 @notify_celery.task(name='check-precompiled-letter-state')

app/dao/notifications_dao.py

@@ -731,7 +731,7 @@ def notifications_not_yet_sent(should_be_sending_after_seconds, notification_typ
     return notifications
 
 
-def dao_get_letters_to_be_printed(print_run_deadline):
+def dao_get_letters_to_be_printed(print_run_deadline, postage):
     """
     Return all letters created before the print run deadline that have not yet been sent
     """
@@ -739,7 +739,8 @@ def dao_get_letters_to_be_printed(print_run_deadline):
         Notification.created_at < convert_bst_to_utc(print_run_deadline),
         Notification.notification_type == LETTER_TYPE,
         Notification.status == NOTIFICATION_CREATED,
-        Notification.key_type == KEY_TYPE_NORMAL
+        Notification.key_type == KEY_TYPE_NORMAL,
+        Notification.postage == postage
     ).order_by(
         Notification.created_at
     ).all()

app/job/rest.py

@@ -141,7 +141,6 @@ def create_job(service_id):
         raise InvalidRequest("Create job is not allowed: service is inactive ", 403)
 
     data = request.get_json()
-
     data.update({
         "service": service_id
     })

app/notifications/receive_notifications.py

@@ -149,7 +149,7 @@ def fetch_potential_service(inbound_number, provider_name):
     service = dao_fetch_service_by_inbound_number(inbound_number)
 
     if not service:
-        current_app.logger.warning('Inbound number "{}" from {} not associated with a service'.format(
+        current_app.logger.error('Inbound number "{}" from {} not associated with a service'.format(
             inbound_number, provider_name
         ))
         statsd_client.incr('inbound.{}.failed'.format(provider_name))

app/notifications/validators.py

@@ -50,7 +50,8 @@ def check_service_over_daily_message_limit(key_type, service):
 
 def check_rate_limiting(service, api_key):
     check_service_over_api_rate_limit(service, api_key)
-    check_service_over_daily_message_limit(api_key.key_type, service)
+    # Reduce queries to the notifications table
+    # check_service_over_daily_message_limit(api_key.key_type, service)
 
 
 def check_template_is_for_notification_type(notification_type, template_type):

app/schemas.py

@@ -381,6 +381,7 @@ class JobSchema(BaseSchema):
         ServiceSchema, attribute="service", dump_to="service_name", only=["name"], dump_only=True)
 
     template_type = fields.Method('get_template_type', dump_only=True)
+    contact_list_id = field_for(models.Job, 'contact_list_id')
 
     def get_template_type(self, job):
         return job.template.template_type

tests/app/celery/test_letters_pdf_tasks.py

@@ -315,6 +315,48 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam
     ]
 
 
+@freeze_time('2020-02-17 18:00:00')
+def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception(
+    notify_api, mocker, sample_letter_template
+):
+    create_notification(
+        template=sample_letter_template,
+        status='created',
+        reference='ref0',
+        created_at=(datetime.now() - timedelta(hours=2))
+    )
+
+    create_notification(
+        template=sample_letter_template,
+        status='created',
+        reference='ref1',
+        created_at=(datetime.now() - timedelta(hours=3))
+    )
+    error_response = {
+        'Error': {
+            'Code': 'FileNotFound',
+            'Message': 'some error message from amazon',
+            'Type': 'Sender'
+        }
+    }
+    mock_head_s3_object = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[
+        {'ContentLength': 2},
+        ClientError(error_response, "File not found")
+    ])
+
+    results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30))
+
+    assert mock_head_s3_object.call_count == 2
+    mock_head_s3_object.assert_has_calls(
+        [
+            call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF'),
+            call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF'),
+        ]
+    )
+
+    assert results == [{'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 2}]
+
+
 @pytest.mark.parametrize('time_to_run_task', [
     "2020-02-17 18:00:00",  # after 5:30pm
     "2020-02-18 02:00:00",  # the next day after midnight, before 5:30pm we expect the same results

tests/app/celery/test_scheduled_tasks.py

@@ -329,8 +329,8 @@ def test_replay_created_notifications_create_letters_pdf_tasks_for_letters_not_r
 
     replay_created_notifications()
 
-    calls = [call([notification_1.id], queue=QueueNames.LETTERS),
-             call([notification_2.id], queue=QueueNames.LETTERS),
+    calls = [call([str(notification_1.id)], queue=QueueNames.LETTERS),
+             call([str(notification_2.id)], queue=QueueNames.LETTERS),
              ]
     mock_task.assert_has_calls(calls, any_order=True)
 

tests/app/job/test_rest.py

@@ -13,7 +13,12 @@ from app.models import JOB_STATUS_TYPES, JOB_STATUS_PENDING
 
 from tests import create_authorization_header
 from tests.conftest import set_config
-from tests.app.db import create_ft_notification_status, create_job, create_notification
+from tests.app.db import (
+    create_ft_notification_status,
+    create_job,
+    create_notification,
+    create_service_contact_list
+)
 
 
 def test_get_job_with_invalid_service_id_returns404(client, sample_service):
@@ -233,6 +238,30 @@ def test_create_scheduled_job(client, sample_template, mocker, fake_uuid):
     assert resp_json['data']['notification_count'] == 1
 
 
+def test_create_job_with_contact_list_id(client, mocker, sample_template, fake_uuid):
+    mocker.patch('app.celery.tasks.process_job.apply_async')
+    mocker.patch('app.job.rest.get_job_metadata_from_s3', return_value={
+        'template_id': str(sample_template.id)
+    })
+    contact_list = create_service_contact_list()
+    data = {
+        'id': fake_uuid,
+        'valid': 'True',
+        'original_file_name': contact_list.original_file_name,
+        'created_by': str(sample_template.service.users[0].id),
+        'notification_count': 100,
+        'contact_list_id': str(contact_list.id),
+    }
+    response = client.post(
+        f'/service/{sample_template.service_id}/job',
+        data=json.dumps(data),
+        headers=[('Content-Type', 'application/json'), create_authorization_header()])
+    resp_json = response.get_json()
+    assert response.status_code == 201
+    assert resp_json['data']['contact_list_id'] == str(contact_list.id)
+    assert resp_json['data']['original_file_name'] == 'EmergencyContactList.xls'
+
+
 def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, sample_service, mocker):
     sample_service.active = False
     mock_job_dao = mocker.patch("app.dao.jobs_dao.dao_create_job")
@@ -683,6 +712,7 @@ def test_get_jobs(admin_request, sample_template):
         'template_type': 'sms',
         'template_version': 1,
         'updated_at': None,
+        'contact_list_id': None
     }
 
 

tests/app/notifications/rest/test_send_notification.py

@@ -18,7 +18,7 @@ from app.dao.services_dao import dao_update_service
 from app.dao.api_key_dao import save_model_api_key
 from app.errors import InvalidRequest
 from app.models import Template
-from app.v2.errors import RateLimitError, TooManyRequestsError
+from app.v2.errors import RateLimitError
 
 from tests import create_authorization_header
 from tests.app.db import (
@@ -404,69 +404,6 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template
             assert response_data['template_version'] == sample_email_template.version
 
 
-@freeze_time("2016-01-01 12:00:00.061258")
-def test_should_block_api_call_if_over_day_limit_for_live_service(
-        notify_db_session,
-        notify_api,
-        mocker):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            mocker.patch(
-                'app.notifications.validators.check_service_over_daily_message_limit',
-                side_effect=TooManyRequestsError(1)
-            )
-            mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
-            service = create_service(message_limit=1)
-            email_template = create_template(service, template_type=EMAIL_TYPE)
-            create_notification(template=email_template)
-
-            data = {
-                'to': 'ok@ok.com',
-                'template': str(email_template.id)
-            }
-
-            auth_header = create_authorization_header(service_id=service.id)
-
-            response = client.post(
-                path='/notifications/email',
-                data=json.dumps(data),
-                headers=[('Content-Type', 'application/json'), auth_header])
-            json.loads(response.get_data(as_text=True))
-            assert response.status_code == 429
-
-
-@freeze_time("2016-01-01 12:00:00.061258")
-def test_should_block_api_call_if_over_day_limit_for_restricted_service(
-        notify_db_session,
-        notify_api,
-        mocker):
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-            mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
-            mocker.patch(
-                'app.notifications.validators.check_service_over_daily_message_limit',
-                side_effect=TooManyRequestsError(1)
-            )
-            service = create_service(restricted=True, message_limit=1)
-            email_template = create_template(service, template_type=EMAIL_TYPE)
-            create_notification(template=email_template)
-
-            data = {
-                'to': 'ok@ok.com',
-                'template': str(email_template.id)
-            }
-
-            auth_header = create_authorization_header(service_id=service.id)
-
-            response = client.post(
-                path='/notifications/email',
-                data=json.dumps(data),
-                headers=[('Content-Type', 'application/json'), auth_header])
-            json.loads(response.get_data(as_text=True))
-
-            assert response.status_code == 429
-
-
 @pytest.mark.parametrize('restricted', [True, False])
 @freeze_time("2016-01-01 12:00:00.061258")
 def test_should_allow_api_call_if_under_day_limit_regardless_of_type(