From 57bd6494e1c4e5042fd31a556de0076544997bec Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 4 Apr 2017 16:05:25 +0100 Subject: [PATCH 01/10] WIP - adding request timeout --- app/celery/research_mode_tasks.py | 3 ++- app/clients/sms/firetext.py | 3 ++- app/clients/sms/mmg.py | 8 ++++++-- app/notifications/notifications_sms_callback.py | 2 ++ 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index d7ecb399e..274fa3fe4 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -55,7 +55,8 @@ def make_request(notification_type, provider, data, headers): "POST", api_call, headers=headers, - data=data + data=data, + timeout=5 ) response.raise_for_status() except RequestException as e: diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 911c88bda..17e09e967 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -97,7 +97,8 @@ class FiretextClient(SmsClient): response = request( "POST", self.url, - data=data + data=data, + timeout=60 ) response.raise_for_status() try: diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 642a247a9..7e3a18e47 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -1,7 +1,7 @@ import json from monotonic import monotonic from requests import (request, RequestException) - +from requests.exceptions import HTTPError from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE) from app.clients.sms import (SmsClient, SmsClientResponseException) @@ -104,7 +104,8 @@ class MMGClient(SmsClient): headers={ 'Content-Type': 'application/json', 'Authorization': 'Basic {}'.format(self.api_key) - } + }, + timeout=60 ) response.raise_for_status() @@ -117,6 +118,9 @@ class MMGClient(SmsClient): except RequestException as e: self.record_outcome(False, e.response) raise MMGClientResponseException(response=e.response, exception=e) + except HTTPError as e: + self.record_outcome(False, e.response) + raise MMGClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time self.statsd_client.timing("clients.mmg.request-time", elapsed_time) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index 3c569e158..c1eacdee4 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -11,6 +11,8 @@ register_errors(sms_callback_blueprint) @sms_callback_blueprint.route('/mmg', methods=['POST']) def process_mmg_response(): + from time import sleep + sleep(20) client_name = 'MMG' data = json.loads(request.data) errors = validate_callback_data(data=data, From 7943b6819b5ba4efefe9b62d41c58ea279c8f528 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Apr 2017 09:18:58 +0100 Subject: [PATCH 02/10] Revert "Revert "Bump utils to allow lists as placeholder values"" --- requirements.txt | 2 +- .../rest/test_send_notification.py | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 73cf5b94b..783686de9 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@13.9.6#egg=notifications-utils==13.9.6 +git+https://github.com/alphagov/notifications-utils.git@14.0.1#egg=notifications-utils==14.0.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 90e2c64e7..7c90f0c99 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -133,6 +133,42 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t assert response_data['subject'] == 'Jo' +def test_send_notification_with_placeholders_replaced_with_list( + client, + sample_email_template_with_placeholders, + mocker +): + 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': ['Jo', 'John', 'Josephine'] + } + } + ), + headers=[ + ('Content-Type', 'application/json'), + create_authorization_header(service_id=sample_email_template_with_placeholders.service.id) + ] + ) + + response_data = json.loads(response.data)['data'] + assert response.status_code == 201 + assert response_data['body'] == ( + 'Hello \n\n' + '* Jo\n' + '* John\n' + '* Josephine\n' + 'This is an email from GOV.\u200BUK' + ) + assert response_data['subject'] == 'Jo, John and Josephine' + + 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: From 1802c84b8ad6c02fbe5e23c11ec69a43020f58a8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Apr 2017 09:28:17 +0100 Subject: [PATCH 03/10] Add handling for data types other than list/string Brings in: - [ ] https://github.com/alphagov/notifications-utils/pull/135 Also adds extra tests for: - the exact issue that we saw in production when #867 was deployed - what happens when `None` is passed as a placeholder value, because this should never get as far as the relevant bit of utils --- requirements.txt | 2 +- .../rest/test_send_notification.py | 47 ++++++++++++++----- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/requirements.txt b/requirements.txt index 783686de9..6224a91e8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@14.0.1#egg=notifications-utils==14.0.1 +git+https://github.com/alphagov/notifications-utils.git@14.0.2#egg=notifications-utils==14.0.2 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 7c90f0c99..d6a4e9c31 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -133,10 +133,39 @@ def test_send_notification_with_placeholders_replaced(notify_api, sample_email_t assert response_data['subject'] == 'Jo' -def test_send_notification_with_placeholders_replaced_with_list( +@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 + mocker, + personalisation, + expected_body, + expected_subject, ): mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') @@ -147,7 +176,7 @@ def test_send_notification_with_placeholders_replaced_with_list( 'to': 'ok@ok.com', 'template': str(sample_email_template_with_placeholders.id), 'personalisation': { - 'name': ['Jo', 'John', 'Josephine'] + 'name': personalisation } } ), @@ -157,16 +186,10 @@ def test_send_notification_with_placeholders_replaced_with_list( ] ) - response_data = json.loads(response.data)['data'] assert response.status_code == 201 - assert response_data['body'] == ( - 'Hello \n\n' - '* Jo\n' - '* John\n' - '* Josephine\n' - 'This is an email from GOV.\u200BUK' - ) - assert response_data['subject'] == 'Jo, John and Josephine' + 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): From f98f220e74697ac22593c8d9d7c04d97dd6b83e5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 5 Apr 2017 12:27:27 +0100 Subject: [PATCH 04/10] Update utils to fix letter output Includes: - [x] https://github.com/alphagov/notifications-utils/pull/136 - [ ] https://github.com/alphagov/notifications-utils/pull/131 --- requirements.txt | 2 +- tests/app/celery/test_tasks.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements.txt b/requirements.txt index 6224a91e8..98b9ef1da 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,6 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@14.0.2#egg=notifications-utils==14.0.2 +git+https://github.com/alphagov/notifications-utils.git@15.0.0#egg=notifications-utils==15.0.0 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index eae030386..41c35b299 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1005,4 +1005,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." # noqa + assert str(letter) == "140|500|001||201703230012345|||||||||||||A1|A2|A3|A4|A5|A6||A_POST|||||||||23 March 2017

Template subjectDear Sir/Madam, Hello. Yours Truly, The Government." # noqa From 72ecca4dab544fb59e29bc213d4e815ba37c0316 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 15:31:37 +0100 Subject: [PATCH 05/10] Added a 60 second timeout to the MMG and fire text clients. - This is a CONNECT and a READ timeout. - Gets wrapped in the standard client exception, with a status code of 504, message Gateway timeout. - Is quiet noisy in logs to allow us to see it - Ensures we flick across the provider. To test change the timeout to 0 and it will timeout. --- app/clients/sms/firetext.py | 10 +++++++--- app/clients/sms/mmg.py | 14 +++++++------- tests/app/clients/test_firetext.py | 25 ++++++++++++++++++++++++- tests/app/clients/test_mmg.py | 25 ++++++++++++++++++++++++- 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 17e09e967..e97091c0b 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -42,8 +42,10 @@ def get_firetext_responses(status): class FiretextClientResponseException(SmsClientResponseException): def __init__(self, response, exception): - self.status_code = response.status_code - self.text = response.text + status_code = response.status_code if response is not None else 504 + text = response.text if response is not None else "Gateway Time-out" + self.status_code = status_code + self.text = text self.exception = exception def __str__(self): @@ -68,11 +70,13 @@ class FiretextClient(SmsClient): return self.name def record_outcome(self, success, response): + status_code = response.status_code if response else 503 + log_message = "API {} request {} on {} response status_code {}".format( "POST", "succeeded" if success else "failed", self.url, - response.status_code + status_code ) if success: diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index 7e3a18e47..3c8cf6338 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -1,7 +1,6 @@ import json from monotonic import monotonic from requests import (request, RequestException) -from requests.exceptions import HTTPError from app.clients import (STATISTICS_DELIVERED, STATISTICS_FAILURE) from app.clients.sms import (SmsClient, SmsClientResponseException) @@ -45,8 +44,11 @@ def get_mmg_responses(status): class MMGClientResponseException(SmsClientResponseException): def __init__(self, response, exception): - self.status_code = response.status_code - self.text = response.text + status_code = response.status_code if response is not None else 504 + text = response.text if response is not None else "Gateway Time-out" + + self.status_code = status_code + self.text = text self.exception = exception def __str__(self): @@ -68,11 +70,12 @@ class MMGClient(SmsClient): self.mmg_url = current_app.config.get('MMG_URL') def record_outcome(self, success, response): + status_code = response.status_code if response else 503 log_message = "API {} request {} on {} response status_code {}".format( "POST", "succeeded" if success else "failed", self.mmg_url, - response.status_code + status_code ) if success: @@ -118,9 +121,6 @@ class MMGClient(SmsClient): except RequestException as e: self.record_outcome(False, e.response) raise MMGClientResponseException(response=e.response, exception=e) - except HTTPError as e: - self.record_outcome(False, e.response) - raise MMGClientResponseException(response=e.response, exception=e) finally: elapsed_time = monotonic() - start_time self.statsd_client.timing("clients.mmg.request-time", elapsed_time) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 5685db48b..4251f89bb 100644 --- a/tests/app/clients/test_firetext.py +++ b/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_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_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' diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index d87f2f914..2b4ce7bd8 100644 --- a/tests/app/clients/test_mmg.py +++ b/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_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_firetext_rejects_with_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' From 43a31fa49771dac5f653044fdd9f29918ad83c1f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 15:33:02 +0100 Subject: [PATCH 06/10] Removed a stray sleep used for testing. --- app/notifications/notifications_sms_callback.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/notifications/notifications_sms_callback.py b/app/notifications/notifications_sms_callback.py index c1eacdee4..3c569e158 100644 --- a/app/notifications/notifications_sms_callback.py +++ b/app/notifications/notifications_sms_callback.py @@ -11,8 +11,6 @@ register_errors(sms_callback_blueprint) @sms_callback_blueprint.route('/mmg', methods=['POST']) def process_mmg_response(): - from time import sleep - sleep(20) client_name = 'MMG' data = json.loads(request.data) errors = validate_callback_data(data=data, From 832005efef733ed1d485535888c7b12964495c6c Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 16:23:41 +0100 Subject: [PATCH 07/10] Updates to the delete CSV file job to reduce the number of eligible jobs in any run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - previously this was unbounded, so it got all jobs older then 7 days. In excess of 75,000 🔥 - this meant that the job took (a) a long time and (b) a lot memory and (c) doing the same thing every day These changes mean that the job has a 2 day eligible window for jobs, minimising the number of eligible jobs in a run, whilst still retaining some leeway in event if it failing one night. In principle the job runs early morning on a given day. The previous 7 days are left along, and then the previous 2 days worth of files are deleted: so: runs on 31st 30,29,28,27,26,25,24 are ignored 23,22 jobs here have files deleted 21 and earlier are ignored. --- app/celery/scheduled_tasks.py | 4 +-- app/dao/jobs_dao.py | 5 ++-- tests/app/celery/test_scheduled_tasks.py | 18 +++++++----- tests/app/dao/test_jobs_dao.py | 37 ++++++++++++++++-------- 4 files changed, 40 insertions(+), 24 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index b8e4eb3e8..b76122819 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -10,7 +10,7 @@ from app.aws import s3 from app import notify_celery from app import performance_platform_client from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago -from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than +from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than_limited_by from app.dao.notifications_dao import ( delete_notifications_created_more_than_a_week_ago, dao_timeout_notifications, @@ -28,7 +28,7 @@ from app.celery.tasks import process_job @notify_celery.task(name="remove_csv_files") @statsd(namespace="tasks") def remove_csv_files(): - jobs = dao_get_jobs_older_than(7) + jobs = dao_get_jobs_older_than_limited_by() for job in jobs: s3.remove_job_from_s3(job.service_id, job.id) current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 1387bb65f..fa30fb41a 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -116,7 +116,8 @@ def dao_update_job(job): db.session.commit() -def dao_get_jobs_older_than(limit_days): +def dao_get_jobs_older_than_limited_by(older_than=7, limit_days=2): return Job.query.filter( - cast(Job.created_at, sql_date) < days_ago(limit_days) + cast(Job.created_at, sql_date) < days_ago(older_than), + cast(Job.created_at, sql_date) >= days_ago(older_than + limit_days) ).order_by(desc(Job.created_at)).all() diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index eb94839aa..b534e7df0 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/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) + s3.remove_job_from_s3.assert_has_calls([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( diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index b51480237..34b92b1d5 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -12,9 +12,8 @@ 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_get_all_notifications_for_job) + dao_get_all_notifications_for_job, dao_get_jobs_older_than_limited_by) from app.models import Job from tests.app.conftest import sample_notification as create_notification @@ -268,19 +267,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): From c8a26fcd4050348792243812edb62dd1ca8a371f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 16:30:53 +0100 Subject: [PATCH 08/10] Fixed test names --- tests/app/clients/test_firetext.py | 4 ++-- tests/app/clients/test_mmg.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/app/clients/test_firetext.py b/tests/app/clients/test_firetext.py index 4251f89bb..1edcab564 100644 --- a/tests/app/clients/test_firetext.py +++ b/tests/app/clients/test_firetext.py @@ -129,7 +129,7 @@ def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetex assert request_args['from'][0] == 'fromservice' -def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock, mock_firetext_client): +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: @@ -140,7 +140,7 @@ def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock, mock_firetext_c assert exc.value.text == 'Gateway Time-out' -def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock, mock_firetext_client): +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: diff --git a/tests/app/clients/test_mmg.py b/tests/app/clients/test_mmg.py index 2b4ce7bd8..73bbc012e 100644 --- a/tests/app/clients/test_mmg.py +++ b/tests/app/clients/test_mmg.py @@ -116,7 +116,7 @@ def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker): assert exc.value.text == 'NOT AT ALL VALID JSON {"key" : "value"}}' -def test_send_sms_raises_if_mmg_rejects_with_timeout(rmock): +def test_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock): to = content = reference = 'foo' with pytest.raises(MMGClientResponseException) as exc: @@ -127,7 +127,7 @@ def test_send_sms_raises_if_mmg_rejects_with_timeout(rmock): assert exc.value.text == 'Gateway Time-out' -def test_send_sms_raises_if_firetext_rejects_with_timeout(rmock): +def test_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock): to = content = reference = 'foo' with pytest.raises(MMGClientResponseException) as exc: From bce75ae0db31c2d1bf6d5b8791f1df1a2e433285 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 16:39:46 +0100 Subject: [PATCH 09/10] Bump timeout on research mode to match normal behaviour --- app/celery/research_mode_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 274fa3fe4..ced35b1d4 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -56,7 +56,7 @@ def make_request(notification_type, provider, data, headers): api_call, headers=headers, data=data, - timeout=5 + timeout=60 ) response.raise_for_status() except RequestException as e: From 80965e4651294248bfe5c789a2d82d7726fe87b9 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 5 Apr 2017 16:50:32 +0100 Subject: [PATCH 10/10] Good spot by Leo on has_calls not being exclusive, Using call_args_list to get accurate list of args --- tests/app/celery/test_scheduled_tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b534e7df0..cbec1e43c 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -215,7 +215,7 @@ def test_will_remove_csv_files_for_jobs_older_than_seven_days(notify_db, notify_ with freeze_time('2016-10-18T10:00:00'): remove_csv_files() - s3.remove_job_from_s3.assert_has_calls([call(job_1.service_id, job_1.id), call(job_2.service_id, job_2.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(