Merge branch 'master' into update-job-status

Conflicts:
	app/dao/jobs_dao.py
	tests/app/dao/test_jobs_dao.py

tests/app/celery/test_scheduled_tasks.py

@@ -203,17 +203,19 @@ def test_should_update_all_scheduled_jobs_and_put_on_queue(notify_db, notify_db_
 def test_will_remove_csv_files_for_jobs_older_than_seven_days(notify_db, notify_db_session, mocker):
     mocker.patch('app.celery.scheduled_tasks.s3.remove_job_from_s3')
 
-    one_millisecond_before_midnight = datetime(2016, 10, 9, 23, 59, 59, 999)
-    midnight = datetime(2016, 10, 10, 0, 0, 0, 0)
-    one_millisecond_past_midnight = datetime(2016, 10, 10, 0, 0, 0, 1)
+    eligible_job_1 = datetime(2016, 10, 10, 23, 59, 59, 000)
+    eligible_job_2 = datetime(2016, 10, 9, 00, 00, 00, 000)
+    in_eligible_job_too_new = datetime(2016, 10, 11, 00, 00, 00, 000)
+    in_eligible_job_too_old = datetime(2016, 10, 8, 23, 59, 59, 999)
 
-    job_1 = create_sample_job(notify_db, notify_db_session, created_at=one_millisecond_before_midnight)
-    create_sample_job(notify_db, notify_db_session, created_at=midnight)
-    create_sample_job(notify_db, notify_db_session, created_at=one_millisecond_past_midnight)
+    job_1 = create_sample_job(notify_db, notify_db_session, created_at=eligible_job_1)
+    job_2 = create_sample_job(notify_db, notify_db_session, created_at=eligible_job_2)
+    create_sample_job(notify_db, notify_db_session, created_at=in_eligible_job_too_new)
+    create_sample_job(notify_db, notify_db_session, created_at=in_eligible_job_too_old)
 
-    with freeze_time('2016-10-17T00:00:00'):
+    with freeze_time('2016-10-18T10:00:00'):
         remove_csv_files()
-    s3.remove_job_from_s3.assert_called_once_with(job_1.service_id, job_1.id)
+    assert s3.remove_job_from_s3.call_args_list == [call(job_1.service_id, job_1.id), call(job_2.service_id, job_2.id)]
 
 
 def test_send_daily_performance_stats_calls_does_not_send_if_inactive(

tests/app/celery/test_tasks.py

@@ -1035,4 +1035,4 @@ def test_dvla_letter_template(sample_letter_notification):
     letter = LetterDVLATemplate(t,
                                 sample_letter_notification.personalisation,
                                 12345)
-    assert str(letter) == "140|500|001||201703230012345|23032017||||||||||||A1|A2|A3|A4|A5|A6||A_POST||||||||Template subject|Dear Sir/Madam, Hello. Yours Truly, The Government.<cr><cr>"  # noqa
+    assert str(letter) == "140|500|001||201703230012345|||||||||||||A1|A2|A3|A4|A5|A6||A_POST|||||||||23 March 2017<cr><cr><h1>Template subject<normal><cr><cr>Dear Sir/Madam, Hello. Yours Truly, The Government.<cr><cr>"  # noqa

tests/app/clients/test_firetext.py

@@ -1,10 +1,11 @@
 from requests import HTTPError
 from urllib.parse import parse_qs
+from requests.exceptions import ConnectTimeout, ReadTimeout
 
 import pytest
 import requests_mock
 
-from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException
+from app.clients.sms.firetext import get_firetext_responses, SmsClientResponseException, FiretextClientResponseException
 
 
 def test_should_return_correct_details_for_delivery():
@@ -126,3 +127,25 @@ def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetex
 
     request_args = parse_qs(request_mock.request_history[0].text)
     assert request_args['from'][0] == 'fromservice'
+
+
+def test_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_firetext_client):
+    to = content = reference = 'foo'
+
+    with pytest.raises(FiretextClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://www.firetext.co.uk/api/sendsms/json', exc=ConnectTimeout)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'
+
+
+def test_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firetext_client):
+    to = content = reference = 'foo'
+
+    with pytest.raises(FiretextClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://www.firetext.co.uk/api/sendsms/json', exc=ReadTimeout)
+        mock_firetext_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_mmg.py

@@ -3,11 +3,12 @@ import json
 import pytest
 import requests_mock
 from requests import HTTPError
+from requests.exceptions import ConnectTimeout, ReadTimeout
 
 from app import mmg_client
 from app.clients.sms import SmsClientResponseException
 
-from app.clients.sms.mmg import get_mmg_responses
+from app.clients.sms.mmg import get_mmg_responses, MMGClientResponseException
 
 
 def test_should_return_correct_details_for_delivery():
@@ -113,3 +114,25 @@ def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
     assert 'app.clients.sms.mmg.MMGClientResponseException: Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc)  # noqa
     assert exc.value.status_code == 200
     assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}'
+
+
+def test_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
+    to = content = reference = 'foo'
+
+    with pytest.raises(MMGClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://api.mmg.co.uk/json/api.php', exc=ConnectTimeout)
+        mmg_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'
+
+
+def test_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
+    to = content = reference = 'foo'
+
+    with pytest.raises(MMGClientResponseException) as exc:
+        rmock.register_uri('POST', 'https://api.mmg.co.uk/json/api.php', exc=ReadTimeout)
+        mmg_client.send_sms(to, content, reference)
+
+    assert exc.value.status_code == 504
+    assert exc.value.text == 'Gateway Time-out'

tests/app/dao/test_jobs_dao.py

@@ -12,10 +12,10 @@ from app.dao.jobs_dao import (
     dao_set_scheduled_jobs_to_pending,
     dao_get_future_scheduled_job_by_id_and_service_id,
     dao_get_notification_outcomes_for_job,
-    dao_get_jobs_older_than,
     all_notifications_are_created_for_job,
+    dao_update_job_status,
     dao_get_all_notifications_for_job,
-    dao_update_job_status)
+    dao_get_jobs_older_than_limited_by)
 from app.models import Job
 
 from tests.app.conftest import sample_notification as create_notification
@@ -269,19 +269,33 @@ def test_get_future_scheduled_job_gets_a_job_yet_to_send(sample_scheduled_job):
     assert result.id == sample_scheduled_job.id
 
 
-def test_should_get_jobs_older_than_seven_days(notify_db, notify_db_session):
-    one_millisecond_before_midnight = datetime(2016, 10, 9, 23, 59, 59, 999)
-    midnight = datetime(2016, 10, 10, 0, 0, 0, 0)
-    one_millisecond_past_midnight = datetime(2016, 10, 10, 0, 0, 0, 1)
+def test_should_get_jobs_seven_days_old(notify_db, notify_db_session):
+    # job runs at some point on each day
+    # shouldn't matter when, we are deleting things 7 days ago
+    job_run_time = '2016-10-31T10:00:00'
 
-    job_1 = create_job(notify_db, notify_db_session, created_at=one_millisecond_before_midnight)
-    create_job(notify_db, notify_db_session, created_at=midnight)
-    create_job(notify_db, notify_db_session, created_at=one_millisecond_past_midnight)
+    # running on the 31st means the previous 7 days are ignored
 
-    with freeze_time('2016-10-17T00:00:00'):
-        jobs = dao_get_jobs_older_than(7)
-        assert len(jobs) == 1
+    # 2 day window for delete jobs
+    # 7 days of files to skip includes the 30,29,28,27,26,25,24th, so the....
+    last_possible_time_for_eligible_job = '2016-10-23T23:59:59'
+    first_possible_time_for_eligible_job = '2016-10-22T00:00:00'
+
+    job_1 = create_job(notify_db, notify_db_session, created_at=last_possible_time_for_eligible_job)
+    job_2 = create_job(notify_db, notify_db_session, created_at=first_possible_time_for_eligible_job)
+
+    # bookmarks for jobs that should be ignored
+    last_possible_time_for_ineligible_job = '2016-10-24T00:00:00'
+    create_job(notify_db, notify_db_session, created_at=last_possible_time_for_ineligible_job)
+
+    first_possible_time_for_ineligible_job = '2016-10-21T23:59:59'
+    create_job(notify_db, notify_db_session, created_at=first_possible_time_for_ineligible_job)
+
+    with freeze_time(job_run_time):
+        jobs = dao_get_jobs_older_than_limited_by()
+        assert len(jobs) == 2
         assert jobs[0].id == job_1.id
+        assert jobs[1].id == job_2.id
 
 
 def test_get_jobs_for_service_is_paginated(notify_db, notify_db_session, sample_service, sample_template):

tests/app/notifications/rest/test_send_notification.py

@@ -133,6 +133,65 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t
             assert response_data['subject'] == 'Jo'
 
 
+@pytest.mark.parametrize('personalisation, expected_body, expected_subject', [
+    (
+        ['Jo', 'John', 'Josephine'],
+        (
+            'Hello \n\n'
+            '* Jo\n'
+            '* John\n'
+            '* Josephine\n'
+            'This is an email from GOV.\u200BUK'
+        ),
+        'Jo, John and Josephine',
+    ),
+    (
+        6,
+        (
+            'Hello 6\n'
+            'This is an email from GOV.\u200BUK'
+        ),
+        '6',
+    ),
+    pytest.mark.xfail((
+        None,
+        ('we consider None equivalent to missing personalisation'),
+        '',
+    )),
+])
+def test_send_notification_with_placeholders_replaced_with_unusual_types(
+    client,
+    sample_email_template_with_placeholders,
+    mocker,
+    personalisation,
+    expected_body,
+    expected_subject,
+):
+    mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async')
+
+    response = client.post(
+        path='/notifications/email',
+        data=json.dumps(
+            {
+                'to': 'ok@ok.com',
+                'template': str(sample_email_template_with_placeholders.id),
+                'personalisation': {
+                    'name': personalisation
+                }
+            }
+        ),
+        headers=[
+            ('Content-Type', 'application/json'),
+            create_authorization_header(service_id=sample_email_template_with_placeholders.service.id)
+        ]
+    )
+
+    assert response.status_code == 201
+    response_data = json.loads(response.data)['data']
+    assert response_data['body'] == expected_body
+    assert response_data['subject'] == expected_subject
+
+
 def test_should_not_send_notification_for_archived_template(notify_api, sample_template):
     with notify_api.test_request_context():
         with notify_api.test_client() as client: