Merge branch 'master' of https://github.com/alphagov/notifications-api into raise-alert-when-no-ack-file

app/celery/letters_pdf_tasks.py

@@ -13,8 +13,10 @@ from app.config import QueueNames, TaskNames
 from app.dao.notifications_dao import (
     get_notification_by_id,
     update_notification_status_by_id,
-    dao_update_notification
+    dao_update_notification,
+    dao_get_notifications_by_references,
 )
+from app.models import NOTIFICATION_CREATED
 from app.statsd_decorators import statsd
 
 
@@ -112,7 +114,7 @@ def group_letters(letter_pdfs):
     running_filesize = 0
     list_of_files = []
     for letter in letter_pdfs:
-        if letter['Key'].lower().endswith('.pdf'):
+        if letter['Key'].lower().endswith('.pdf') and letter_in_created_state(letter['Key']):
             if (
                 running_filesize + letter['Size'] > current_app.config['MAX_LETTER_PDF_ZIP_FILESIZE'] or
                 len(list_of_files) >= current_app.config['MAX_LETTER_PDF_COUNT_PER_ZIP']
@@ -126,3 +128,19 @@ def group_letters(letter_pdfs):
 
     if list_of_files:
         yield list_of_files
+
+
+def letter_in_created_state(filename):
+    # filename looks like '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF'
+    subfolder = filename.split('/')[0]
+    ref = filename.split('.')[1]
+    notifications = dao_get_notifications_by_references([ref])
+    if notifications:
+        if notifications[0].status == NOTIFICATION_CREATED:
+            return True
+        current_app.logger.info('Collating letters for {} but notification with reference {} already in {}'.format(
+            subfolder,
+            ref,
+            notifications[0].status
+        ))
+    return False

app/notifications/validators.py

@@ -98,6 +98,9 @@ def check_service_can_schedule_notification(permissions, scheduled_for):
 
 
 def validate_and_format_recipient(send_to, key_type, service, notification_type):
+    if send_to is None:
+        raise BadRequestError(message="Recipient can't be empty")
+
     service_can_send_to_recipient(send_to, key_type, service)
 
     if notification_type == SMS_TYPE:

requirements.txt

@@ -1,4 +1,4 @@
-boto3==1.5.6
+boto3==1.5.15
 cffi==1.11.0 # pyup: != 1.11.1, != 1.11.2 # These versions are missing .whl
 celery==3.1.25 # pyup: <4
 docopt==0.6.2
@@ -17,12 +17,12 @@ monotonic==1.4
 psycopg2==2.7.3.2
 PyJWT==1.5.3
 SQLAlchemy-Utils==0.32.21
-SQLAlchemy==1.2.0
+SQLAlchemy==1.2.1
 
 notifications-python-client==4.7.1
 
 # PaaS
-awscli==1.14.16
+awscli==1.14.25
 awscli-cwlogs>=1.4,<1.5
 
 git+https://github.com/alphagov/notifications-utils.git@23.4.1#egg=notifications-utils==23.4.1

requirements_for_test.txt

@@ -3,7 +3,7 @@ flake8==3.5.0
 pytest==3.3.2
 pytest-mock==1.6.3
 pytest-cov==2.5.1
-pytest-xdist==1.21.0
+pytest-xdist==1.22.0
 coveralls==1.2.0
 freezegun==0.3.9
 requests-mock==1.4.0

tests/app/celery/test_letters_pdf_tasks.py

@@ -11,9 +11,10 @@ from app.celery.letters_pdf_tasks import (
     create_letters_pdf,
     get_letters_pdf,
     collate_letter_pdfs_for_day,
-    group_letters
+    group_letters,
+    letter_in_created_state,
 )
-from app.models import Notification
+from app.models import Notification, NOTIFICATION_SENDING
 
 from tests.conftest import set_config_values
 
@@ -166,7 +167,8 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker):
     )
 
 
-def test_group_letters_splits_on_file_size(notify_api):
+def test_group_letters_splits_on_file_size(notify_api, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True)
     letters = [
         # ends under max but next one is too big
         {'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2},
@@ -192,7 +194,8 @@ def test_group_letters_splits_on_file_size(notify_api):
         assert next(x, None) is None
 
 
-def test_group_letters_splits_on_file_count(notify_api):
+def test_group_letters_splits_on_file_count(notify_api, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True)
     letters = [
         {'Key': 'A.pdf', 'Size': 1},
         {'Key': 'B.pdf', 'Size': 2},
@@ -215,7 +218,8 @@ def test_group_letters_splits_on_file_count(notify_api):
         assert next(x, None) is None
 
 
-def test_group_letters_splits_on_file_size_and_file_count(notify_api):
+def test_group_letters_splits_on_file_size_and_file_count(notify_api, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True)
     letters = [
         # ends under max file size but next file is too big
         {'Key': 'A.pdf', 'Size': 1},
@@ -249,10 +253,39 @@ def test_group_letters_splits_on_file_size_and_file_count(notify_api):
         assert next(x, None) is None
 
 
-def test_group_letters_ignores_non_pdfs(notify_api):
+def test_group_letters_ignores_non_pdfs(notify_api, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True)
     letters = [{'Key': 'A.zip'}]
     assert list(group_letters(letters)) == []
 
 
-def test_group_letters_with_no_letters(notify_api):
+def test_group_letters_ignores_notifications_already_sent(notify_api, mocker):
+    mock = mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=False)
+    letters = [{'Key': 'A.pdf'}]
+    assert list(group_letters(letters)) == []
+    mock.assert_called_once_with('A.pdf')
+
+
+def test_group_letters_with_no_letters(notify_api, mocker):
+    mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True)
     assert list(group_letters([])) == []
+
+
+def test_letter_in_created_state(sample_notification):
+    sample_notification.reference = 'ABCDEF1234567890'
+    filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF'
+
+    assert letter_in_created_state(filename) is True
+
+
+def test_letter_in_created_state_fails_if_notification_not_in_created(sample_notification):
+    sample_notification.reference = 'ABCDEF1234567890'
+    sample_notification.status = NOTIFICATION_SENDING
+    filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF'
+    assert letter_in_created_state(filename) is False
+
+
+def test_letter_in_created_state_fails_if_notification_doesnt_exist(sample_notification):
+    sample_notification.reference = 'QWERTY1234567890'
+    filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF'
+    assert letter_in_created_state(filename) is False

tests/app/notifications/test_validators.py

@@ -347,6 +347,13 @@ def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_s
     assert result == '201212341234'
 
 
+def test_rejects_api_calls_with_no_recipient():
+    with pytest.raises(BadRequestError) as e:
+        validate_and_format_recipient(None, 'key_type', 'service', 'SMS_TYPE')
+    assert e.value.status_code == 400
+    assert e.value.message == "Recipient can't be empty"
+
+
 @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter'])
 def test_check_service_email_reply_to_id_where_reply_to_id_is_none(notification_type):
     assert check_service_email_reply_to_id(None, None, notification_type) is None