From 8f82642422288612b4e7cc0793fe26ea7ade49c8 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 25 Aug 2017 16:07:30 +0100 Subject: [PATCH 1/9] Prevent v2 notifications POST when in trial mode --- app/v2/notifications/post_notifications.py | 3 +++ .../test_post_letter_notifications.py | 23 ++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index c27ca8dc5..c1bcd168e 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -150,6 +150,9 @@ def process_letter_notification(*, letter_data, api_key, template): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) + if api_key.service.restricted: + raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) + job = create_letter_api_job(template) notification = create_letter_notification(letter_data, job, api_key) diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 429e53eb8..cb9d4bb29 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -17,13 +17,12 @@ from app.variables import LETTER_TEST_API_FILENAME from app.variables import LETTER_API_FILENAME from tests import create_authorization_header -from tests.app.db import create_service -from tests.app.db import create_template +from tests.app.db import create_service, create_template def letter_request(client, data, service_id, key_type=KEY_TYPE_NORMAL, _expected_status=201): resp = client.post( - url_for('v2_notifications.post_notification', notification_type='letter'), + url_for('v2_notifications.post_notification', notification_type=LETTER_TYPE), data=json.dumps(data), headers=[ ('Content-Type', 'application/json'), @@ -232,3 +231,21 @@ def test_post_letter_notification_doesnt_accept_team_key(client, sample_letter_t assert error_json['status_code'] == 403 assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Cannot send letters with a team api key'}] + + +def test_post_letter_notification_doesnt_send_in_trial(client, sample_trial_letter_template): + data = { + 'template_id': str(sample_trial_letter_template.id), + 'personalisation': {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'} + } + + error_json = letter_request( + client, + data, + sample_trial_letter_template.service_id, + _expected_status=403 + ) + + assert error_json['status_code'] == 403 + assert error_json['errors'] == [ + {'error': 'BadRequestError', 'message': 'Cannot send letters when service is in trial mode'}] From 41a71c703b1b5000b30de1e68303fed7a9100c37 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 25 Aug 2017 16:10:41 +0100 Subject: [PATCH 2/9] Refactor test --- tests/app/celery/test_scheduled_tasks.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index a05ea18f9..0929291b2 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -680,9 +680,7 @@ def test_populate_monthly_billing_updates_correct_month_in_bst(sample_template): def test_run_letter_jobs(client, mocker, sample_letter_template): - jobs = [create_job(template=sample_letter_template, job_status=JOB_STATUS_READY_TO_SEND), - create_job(template=sample_letter_template, job_status=JOB_STATUS_READY_TO_SEND)] - job_ids = [str(job.id) for job in jobs] + job_ids = [str(uuid.uuid4()), str(uuid.uuid4())] mocker.patch( "app.celery.scheduled_tasks.dao_get_letter_job_ids_by_status", return_value=job_ids From 7e70b4411361de52eba01f0e989e94677e5a04f5 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 25 Aug 2017 16:11:39 +0100 Subject: [PATCH 3/9] Error in task when letter template and in trial mode --- app/celery/tasks.py | 7 +++++++ tests/app/celery/test_tasks.py | 23 ++++++++++++++++++++++- tests/app/conftest.py | 6 ++++++ 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 008b0fd5e..0581d346b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -75,6 +75,13 @@ def process_job(job_id): db_template = dao_get_template_by_id(job.template_id, job.template_version) + if db_template.template_type == LETTER_TYPE and service.restricted: + job.job_status = JOB_STATUS_ERROR + dao_update_job(job) + current_app.logger.warn( + "Job {} has been set to error, service {} is in trial mode".format(job_id, service.id)) + return + TemplateClass = get_template_class(db_template.template_type) template = TemplateClass(db_template.__dict__) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 717dc7b82..e2fde2d17 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -38,7 +38,8 @@ from app.models import ( SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, - Job) + Job, + JOB_STATUS_ERROR) from tests.app import load_example_csv from tests.conftest import set_config @@ -977,6 +978,26 @@ def test_should_cancel_job_if_service_is_inactive(sample_service, mock_dvla_file_task.assert_not_called() +def test_should_error_job_if_service_is_restricted_and_letter_template_type( + sample_service, + sample_letter_job, + mocker +): + sample_service.restricted = True + + mocker.patch('app.celery.tasks.s3.get_job_from_s3') + mocker.patch('app.celery.tasks.process_row') + mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file') + + process_job(sample_letter_job.id) + + job = jobs_dao.dao_get_job_by_id(sample_letter_job.id) + assert job.job_status == JOB_STATUS_ERROR + s3.get_job_from_s3.assert_not_called() + tasks.process_row.assert_not_called() + mock_dvla_file_task.assert_not_called() + + @pytest.mark.parametrize('template_type, expected_class', [ (SMS_TYPE, SMSMessageTemplate), (EMAIL_TYPE, WithSubjectTemplate), diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 86437e7a1..13cc64ea3 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -271,6 +271,12 @@ def sample_letter_template(sample_service_full_permissions): return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) +@pytest.fixture +def sample_trial_letter_template(sample_service_full_permissions): + sample_service_full_permissions.restricted = True + return create_template(sample_service_full_permissions, template_type=LETTER_TYPE) + + @pytest.fixture(scope='function') def sample_email_template_with_placeholders(notify_db, notify_db_session): return sample_email_template( From ff860ec24231b83a4215a1f722dee8848dafc152 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Fri, 25 Aug 2017 16:12:26 +0100 Subject: [PATCH 4/9] 403 when creating a letter job in trial mode --- app/job/rest.py | 5 ++++- tests/app/job/test_rest.py | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/app/job/rest.py b/app/job/rest.py index 6a8a86ee4..8b2165760 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -32,7 +32,7 @@ from app.schemas import ( from app.celery.tasks import process_job -from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANCELLED +from app.models import JOB_STATUS_SCHEDULED, JOB_STATUS_PENDING, JOB_STATUS_CANCELLED, LETTER_TYPE from app.utils import pagination_links @@ -190,6 +190,9 @@ def create_job(service_id): }) template = dao_get_template_by_id(data['template']) + if template.template_type == LETTER_TYPE and service.restricted: + raise InvalidRequest("Create letter job is not allowed for service in trial mode ", 403) + errors = unarchived_template_schema.validate({'archived': template.archived}) if errors: diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 7ce45b51e..920bc3397 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -186,6 +186,29 @@ def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, samp mock_job_dao.assert_not_called() +def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( + client, fake_uuid, sample_service, sample_trial_letter_template, mocker): + data = { + 'id': fake_uuid, + 'service': str(sample_trial_letter_template.service.id), + 'template': str(sample_trial_letter_template.id), + 'original_file_name': 'thisisatest.csv', + 'notification_count': 1, + 'created_by': str(sample_trial_letter_template.created_by.id) + } + mock_job_dao = mocker.patch("app.dao.jobs_dao.dao_create_job") + auth_header = create_authorization_header() + response = client.post('/service/{}/job'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 403 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert resp_json['message'] == "Create letter job is not allowed for service in trial mode " + mock_job_dao.assert_not_called() + + def test_should_not_create_scheduled_job_more_then_24_hours_hence(notify_api, sample_template, mocker, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: From 01830b7e596b75c2d58e710c5fd774f880fd63ad Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 29 Aug 2017 12:24:17 +0100 Subject: [PATCH 5/9] Push letter job to research queue in research mode --- app/celery/tasks.py | 6 ++++-- tests/app/celery/test_tasks.py | 38 ++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 0581d346b..52f2fcde9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -93,9 +93,11 @@ def process_job(job_id): process_row(row_number, recipient, personalisation, template, job, service) if template.template_type == LETTER_TYPE: - build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) + build_dvla_file.apply_async( + [str(job.id)], queue=QueueNames.JOBS if not service.research_mode else QueueNames.RESEARCH_MODE) # temporary logging - current_app.logger.info("send job {} to build-dvla-file in the process-job queue".format(job_id)) + current_app.logger.info("send job {} to build-dvla-file in the {} queue".format( + job_id, QueueNames.JOBS if not service.research_mode else QueueNames.RESEARCH_MODE)) else: job.job_status = JOB_STATUS_FINISHED diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e2fde2d17..20d961f51 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -29,6 +29,7 @@ from app.celery.tasks import ( update_letter_notifications_statuses, process_updates_from_file, send_inbound_sms_to_service) +from app.config import QueueNames from app.dao import jobs_dao, services_dao from app.models import ( Notification, @@ -609,6 +610,43 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv ) +@freeze_time("2017-08-29 17:30:00") +def test_should_build_dvla_file_in_research_mode_for_letter_job( + notify_db, notify_db_session, mocker, sample_service, sample_letter_job, fake_uuid +): + test_encrypted_data = 'some encrypted data' + sample_service.research_mode = True + + csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name + A1,A2,A3,A4,A_POST,Alice + """ + s3_mock = mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) + # mocker.patch('app.celery.tasks.process_row') + mock_persist_letter = mocker.patch('app.celery.tasks.persist_letter.apply_async') + mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid) + mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data) + mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + + process_job(sample_letter_job.id) + + job = jobs_dao.dao_get_job_by_id(sample_letter_job.id) + + persist_letter.apply_async.assert_called_once_with( + ( + str(sample_service.id), + fake_uuid, + test_encrypted_data, + datetime.utcnow().strftime(DATETIME_FORMAT) + ), + queue=QueueNames.RESEARCH_MODE + ) + + build_dvla_file.apply_async.assert_called_once_with( + [str(job.id)], + queue=QueueNames.RESEARCH_MODE + ) + + def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_api_key, mocker): notification = _notification_json( sample_job.template, From 225c85832c3822d32e220a90fafc9cedeb4a0697 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 29 Aug 2017 18:15:38 +0100 Subject: [PATCH 6/9] Refactored to update job status and not build dvla file --- app/celery/tasks.py | 6 ++++-- tests/app/celery/test_tasks.py | 37 +++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 52f2fcde9..ed7811ddb 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -93,8 +93,10 @@ def process_job(job_id): process_row(row_number, recipient, personalisation, template, job, service) if template.template_type == LETTER_TYPE: - build_dvla_file.apply_async( - [str(job.id)], queue=QueueNames.JOBS if not service.research_mode else QueueNames.RESEARCH_MODE) + if service.research_mode: + update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) + else: + build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) # temporary logging current_app.logger.info("send job {} to build-dvla-file in the {} queue".format( job_id, QueueNames.JOBS if not service.research_mode else QueueNames.RESEARCH_MODE)) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 20d961f51..344ac00ab 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -610,8 +610,7 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv ) -@freeze_time("2017-08-29 17:30:00") -def test_should_build_dvla_file_in_research_mode_for_letter_job( +def test_should_not_build_dvla_file_in_research_mode_for_letter_job( notify_db, notify_db_session, mocker, sample_service, sample_letter_job, fake_uuid ): test_encrypted_data = 'some encrypted data' @@ -620,9 +619,31 @@ def test_should_build_dvla_file_in_research_mode_for_letter_job( csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name A1,A2,A3,A4,A_POST,Alice """ - s3_mock = mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) - # mocker.patch('app.celery.tasks.process_row') - mock_persist_letter = mocker.patch('app.celery.tasks.persist_letter.apply_async') + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) + mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') + mocker.patch('app.celery.tasks.persist_letter.apply_async') + mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid) + mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data) + mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + + process_job(sample_letter_job.id) + + assert not mock_dvla_file_task.called + + +@freeze_time("2017-08-29 17:30:00") +def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( + notify_db, notify_db_session, mocker, sample_service, sample_letter_job, fake_uuid +): + test_encrypted_data = 'some encrypted data' + sample_service.research_mode = True + + csv = """address_line_1,address_line_2,address_line_3,address_line_4,postcode,name + A1,A2,A3,A4,A_POST,Alice + """ + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=csv) + mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') + mocker.patch('app.celery.tasks.persist_letter.apply_async') mocker.patch('app.celery.tasks.create_uuid', return_value=fake_uuid) mocker.patch('app.celery.tasks.encryption.encrypt', return_value=test_encrypted_data) mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') @@ -641,10 +662,8 @@ def test_should_build_dvla_file_in_research_mode_for_letter_job( queue=QueueNames.RESEARCH_MODE ) - build_dvla_file.apply_async.assert_called_once_with( - [str(job.id)], - queue=QueueNames.RESEARCH_MODE - ) + update_job_to_sent_to_dvla.apply_async.assert_called_once_with( + [str(job.id)], queue=QueueNames.RESEARCH_MODE) def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_api_key, mocker): From d39191967770595883aab11177a14cdd59f36260 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 30 Aug 2017 16:03:05 +0100 Subject: [PATCH 7/9] Refactored to check trial when running scheduled job --- app/celery/scheduled_tasks.py | 19 +++++---- app/celery/tasks.py | 8 ++-- app/letters/rest.py | 7 +++- app/utils.py | 30 ++++++++++++++- tests/app/celery/test_scheduled_tasks.py | 45 +++++++++++++++++++++- tests/app/letters/test_send_letter_jobs.py | 8 +++- 6 files changed, 100 insertions(+), 17 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 5614c361f..9c4dbb5c5 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -39,7 +39,7 @@ from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd from app.celery.tasks import process_job from app.config import QueueNames, TaskNames -from app.utils import convert_utc_to_bst +from app.utils import convert_utc_to_bst, get_unrestricted_letter_ids @notify_celery.task(name="remove_csv_files") @@ -308,9 +308,14 @@ def populate_monthly_billing(): @statsd(namespace="tasks") def run_letter_jobs(): job_ids = dao_get_letter_job_ids_by_status(JOB_STATUS_READY_TO_SEND) - notify_celery.send_task( - name=TaskNames.DVLA_FILES, - args=(job_ids,), - queue=QueueNames.PROCESS_FTP - ) - current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) + + unrestricted_job_ids = get_unrestricted_letter_ids(job_ids) + + if unrestricted_job_ids: + notify_celery.send_task( + name=TaskNames.DVLA_FILES, + args=(unrestricted_job_ids,), + queue=QueueNames.PROCESS_FTP + ) + current_app.logger.info( + "Queued {} ready letter job ids onto {}".format(len(unrestricted_job_ids), QueueNames.PROCESS_FTP)) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ed7811ddb..a5e9f98d6 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -75,7 +75,7 @@ def process_job(job_id): db_template = dao_get_template_by_id(job.template_id, job.template_version) - if db_template.template_type == LETTER_TYPE and service.restricted: + if db_template.template_type == LETTER_TYPE and job.service.restricted: job.job_status = JOB_STATUS_ERROR dao_update_job(job) current_app.logger.warn( @@ -97,9 +97,9 @@ def process_job(job_id): update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) - # temporary logging - current_app.logger.info("send job {} to build-dvla-file in the {} queue".format( - job_id, QueueNames.JOBS if not service.research_mode else QueueNames.RESEARCH_MODE)) + # temporary logging + current_app.logger.info("send job {} to build-dvla-file in the {} queue".format( + job_id, QueueNames.JOBS)) else: job.job_status = JOB_STATUS_FINISHED diff --git a/app/letters/rest.py b/app/letters/rest.py index b9ce7bf35..458611a80 100644 --- a/app/letters/rest.py +++ b/app/letters/rest.py @@ -8,6 +8,7 @@ from app.schemas import job_schema from app.v2.errors import register_errors from app.letters.letter_schemas import letter_job_ids from app.schema_validation import validate +from app.utils import get_unrestricted_letter_ids letter_job = Blueprint("letter-job", __name__) register_errors(letter_job) @@ -16,7 +17,11 @@ register_errors(letter_job) @letter_job.route('/send-letter-jobs', methods=['POST']) def send_letter_jobs(): job_ids = validate(request.get_json(), letter_job_ids) - notify_celery.send_task(name=TaskNames.DVLA_FILES, args=(job_ids['job_ids'],), queue=QueueNames.PROCESS_FTP) + + unrestricted_job_ids = get_unrestricted_letter_ids(job_ids['job_ids']) + + notify_celery.send_task( + name=TaskNames.DVLA_FILES, args=(unrestricted_job_ids,), queue=QueueNames.PROCESS_FTP) return jsonify(data={"response": "Task created to send files to DVLA"}), 201 diff --git a/app/utils.py b/app/utils.py index cf4c28f06..538673fb3 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,7 +1,7 @@ from datetime import datetime, timedelta import pytz -from flask import url_for +from flask import current_app, url_for from sqlalchemy import func from notifications_utils.template import SMSMessageTemplate, PlainTextEmailTemplate, LetterPreviewTemplate @@ -86,3 +86,31 @@ def get_public_notify_type_text(notify_type, plural=False): notify_type_text = 'text message' return '{}{}'.format(notify_type_text, 's' if plural else '') + + +def get_unrestricted_letter_ids(job_ids): + from app.dao.jobs_dao import ( + dao_get_job_by_id, + dao_update_job + ) + + from app.models import (LETTER_TYPE, JOB_STATUS_ERROR) + from app.dao.templates_dao import dao_get_template_by_id + + unrestricted_job_ids = [] + + for job_id in job_ids: + job = dao_get_job_by_id(job_id) + + template = dao_get_template_by_id(job.template_id, job.template_version) + + if template.template_type == LETTER_TYPE: + if job.service.restricted: + job.job_status = JOB_STATUS_ERROR + dao_update_job(job) + current_app.logger.warn( + "Job {} has been set to error, service {} is in trial mode".format(job.id, job.service.id)) + else: + unrestricted_job_ids.append(job_id) + + return unrestricted_job_ids diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 0929291b2..cfcd386dd 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -41,7 +41,7 @@ from app.dao.provider_details_dao import ( from app.models import ( Service, Template, SMS_TYPE, LETTER_TYPE, - JOB_STATUS_READY_TO_SEND, + JOB_STATUS_READY_TO_SEND, JOB_STATUS_ERROR, MonthlyBilling) from app.utils import get_london_midnight_in_utc, convert_utc_to_bst from tests.app.db import create_notification, create_service, create_template, create_job, create_rate @@ -680,7 +680,9 @@ def test_populate_monthly_billing_updates_correct_month_in_bst(sample_template): def test_run_letter_jobs(client, mocker, sample_letter_template): - job_ids = [str(uuid.uuid4()), str(uuid.uuid4())] + jobs = [create_job(template=sample_letter_template, job_status=JOB_STATUS_READY_TO_SEND), + create_job(template=sample_letter_template, job_status=JOB_STATUS_READY_TO_SEND)] + job_ids = [str(j.id) for j in jobs] mocker.patch( "app.celery.scheduled_tasks.dao_get_letter_job_ids_by_status", return_value=job_ids @@ -692,3 +694,42 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): mock_celery.assert_called_once_with(name=TaskNames.DVLA_FILES, args=(job_ids,), queue=QueueNames.PROCESS_FTP) + + +def test_run_letter_jobs_in_trial_sets_job_to_error(client, mocker, sample_letter_template): + sample_letter_template.service.restricted = True + job = create_job(sample_letter_template) + job_ids = [str(job.id)] + mocker.patch( + "app.celery.scheduled_tasks.dao_get_letter_job_ids_by_status", + return_value=job_ids + ) + mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") + + run_letter_jobs() + + assert not mock_celery.called + assert job.job_status == JOB_STATUS_ERROR + + +def test_run_letter_jobs_in_trial_sets_job_to_error_and_process_live_services( + client, mocker, sample_letter_template): + live_job = create_job(sample_letter_template) + + service = create_service(service_name="Sample service 2", restricted=True) + template = create_template(service, template_type=LETTER_TYPE) + trial_job = create_job(template) + + job_ids = [str(live_job.id), str(trial_job.id)] + mocker.patch( + "app.celery.scheduled_tasks.dao_get_letter_job_ids_by_status", + return_value=job_ids + ) + mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") + + run_letter_jobs() + + mock_celery.assert_called_once_with(name=TaskNames.DVLA_FILES, + args=([str(live_job.id)],), + queue=QueueNames.PROCESS_FTP) + assert trial_job.job_status == JOB_STATUS_ERROR diff --git a/tests/app/letters/test_send_letter_jobs.py b/tests/app/letters/test_send_letter_jobs.py index d38e24c8f..d899387b6 100644 --- a/tests/app/letters/test_send_letter_jobs.py +++ b/tests/app/letters/test_send_letter_jobs.py @@ -5,11 +5,15 @@ from flask import json from app.variables import LETTER_TEST_API_FILENAME from tests import create_authorization_header +from tests.app.db import create_job -def test_send_letter_jobs(client, mocker): +def test_send_letter_jobs(client, mocker, sample_letter_template): mock_celery = mocker.patch("app.letters.rest.notify_celery.send_task") - job_ids = {"job_ids": [str(uuid.uuid4()), str(uuid.uuid4()), str(uuid.uuid4())]} + job_1 = create_job(sample_letter_template) + job_2 = create_job(sample_letter_template) + job_3 = create_job(sample_letter_template) + job_ids = {"job_ids": [str(job_1.id), str(job_2.id), str(job_3.id)]} auth_header = create_authorization_header() From 19f964a90baead41e461f8f878a1404c162896af Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 4 Sep 2017 17:24:41 +0100 Subject: [PATCH 8/9] Added a check that the call is not using a test api key. Removed the tests for trial mode service for the scheduled tasks and the process job. Having the validation in the POST notification and create job endpoint is enough. Updated the test_service_whitelist test because the order of the array is not gaurenteed. --- app/celery/scheduled_tasks.py | 19 +++----- app/celery/tasks.py | 11 +---- app/letters/rest.py | 7 +-- app/utils.py | 32 +------------ app/v2/notifications/post_notifications.py | 6 +-- tests/app/celery/test_scheduled_tasks.py | 45 +------------------ tests/app/celery/test_tasks.py | 20 --------- tests/app/service/test_service_whitelist.py | 7 ++- .../test_post_letter_notifications.py | 16 +++++++ 9 files changed, 34 insertions(+), 129 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 9c4dbb5c5..5614c361f 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -39,7 +39,7 @@ from app.notifications.process_notifications import send_notification_to_queue from app.statsd_decorators import statsd from app.celery.tasks import process_job from app.config import QueueNames, TaskNames -from app.utils import convert_utc_to_bst, get_unrestricted_letter_ids +from app.utils import convert_utc_to_bst @notify_celery.task(name="remove_csv_files") @@ -308,14 +308,9 @@ def populate_monthly_billing(): @statsd(namespace="tasks") def run_letter_jobs(): job_ids = dao_get_letter_job_ids_by_status(JOB_STATUS_READY_TO_SEND) - - unrestricted_job_ids = get_unrestricted_letter_ids(job_ids) - - if unrestricted_job_ids: - notify_celery.send_task( - name=TaskNames.DVLA_FILES, - args=(unrestricted_job_ids,), - queue=QueueNames.PROCESS_FTP - ) - current_app.logger.info( - "Queued {} ready letter job ids onto {}".format(len(unrestricted_job_ids), QueueNames.PROCESS_FTP)) + notify_celery.send_task( + name=TaskNames.DVLA_FILES, + args=(job_ids,), + queue=QueueNames.PROCESS_FTP + ) + current_app.logger.info("Queued {} ready letter job ids onto {}".format(len(job_ids), QueueNames.PROCESS_FTP)) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a5e9f98d6..e621350ce 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -75,13 +75,6 @@ def process_job(job_id): db_template = dao_get_template_by_id(job.template_id, job.template_version) - if db_template.template_type == LETTER_TYPE and job.service.restricted: - job.job_status = JOB_STATUS_ERROR - dao_update_job(job) - current_app.logger.warn( - "Job {} has been set to error, service {} is in trial mode".format(job_id, service.id)) - return - TemplateClass = get_template_class(db_template.template_type) template = TemplateClass(db_template.__dict__) @@ -97,9 +90,7 @@ def process_job(job_id): update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) - # temporary logging - current_app.logger.info("send job {} to build-dvla-file in the {} queue".format( - job_id, QueueNames.JOBS)) + current_app.logger.info("send job {} to build-dvla-file in the {} queue".format(job_id, QueueNames.JOBS)) else: job.job_status = JOB_STATUS_FINISHED diff --git a/app/letters/rest.py b/app/letters/rest.py index 458611a80..b9ce7bf35 100644 --- a/app/letters/rest.py +++ b/app/letters/rest.py @@ -8,7 +8,6 @@ from app.schemas import job_schema from app.v2.errors import register_errors from app.letters.letter_schemas import letter_job_ids from app.schema_validation import validate -from app.utils import get_unrestricted_letter_ids letter_job = Blueprint("letter-job", __name__) register_errors(letter_job) @@ -17,11 +16,7 @@ register_errors(letter_job) @letter_job.route('/send-letter-jobs', methods=['POST']) def send_letter_jobs(): job_ids = validate(request.get_json(), letter_job_ids) - - unrestricted_job_ids = get_unrestricted_letter_ids(job_ids['job_ids']) - - notify_celery.send_task( - name=TaskNames.DVLA_FILES, args=(unrestricted_job_ids,), queue=QueueNames.PROCESS_FTP) + notify_celery.send_task(name=TaskNames.DVLA_FILES, args=(job_ids['job_ids'],), queue=QueueNames.PROCESS_FTP) return jsonify(data={"response": "Task created to send files to DVLA"}), 201 diff --git a/app/utils.py b/app/utils.py index 538673fb3..49f6d61dc 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,9 +1,9 @@ from datetime import datetime, timedelta import pytz -from flask import current_app, url_for +from flask import url_for from sqlalchemy import func -from notifications_utils.template import SMSMessageTemplate, PlainTextEmailTemplate, LetterPreviewTemplate +from notifications_utils.template import SMSMessageTemplate, PlainTextEmailTemplate local_timezone = pytz.timezone("Europe/London") @@ -86,31 +86,3 @@ def get_public_notify_type_text(notify_type, plural=False): notify_type_text = 'text message' return '{}{}'.format(notify_type_text, 's' if plural else '') - - -def get_unrestricted_letter_ids(job_ids): - from app.dao.jobs_dao import ( - dao_get_job_by_id, - dao_update_job - ) - - from app.models import (LETTER_TYPE, JOB_STATUS_ERROR) - from app.dao.templates_dao import dao_get_template_by_id - - unrestricted_job_ids = [] - - for job_id in job_ids: - job = dao_get_job_by_id(job_id) - - template = dao_get_template_by_id(job.template_id, job.template_version) - - if template.template_type == LETTER_TYPE: - if job.service.restricted: - job.job_status = JOB_STATUS_ERROR - dao_update_job(job) - current_app.logger.warn( - "Job {} has been set to error, service {} is in trial mode".format(job.id, job.service.id)) - else: - unrestricted_job_ids.append(job_id) - - return unrestricted_job_ids diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index c1bcd168e..db69a5758 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -150,18 +150,16 @@ def process_letter_notification(*, letter_data, api_key, template): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) - if api_key.service.restricted: - raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) + if api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST: + raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403) job = create_letter_api_job(template) notification = create_letter_notification(letter_data, job, api_key) if api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST: - # distinguish real API jobs from test jobs by giving the test jobs a different filename job.original_file_name = LETTER_TEST_API_FILENAME dao_update_job(job) - update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE) else: build_dvla_file.apply_async([str(job.id)], queue=QueueNames.JOBS) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index cfcd386dd..2308e7247 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1,5 +1,3 @@ -import uuid - from datetime import datetime, timedelta from functools import partial from unittest.mock import call, patch, PropertyMock @@ -41,9 +39,9 @@ from app.dao.provider_details_dao import ( from app.models import ( Service, Template, SMS_TYPE, LETTER_TYPE, - JOB_STATUS_READY_TO_SEND, JOB_STATUS_ERROR, + JOB_STATUS_READY_TO_SEND, MonthlyBilling) -from app.utils import get_london_midnight_in_utc, convert_utc_to_bst +from app.utils import get_london_midnight_in_utc from tests.app.db import create_notification, create_service, create_template, create_job, create_rate from tests.app.conftest import ( sample_job as create_sample_job, @@ -694,42 +692,3 @@ def test_run_letter_jobs(client, mocker, sample_letter_template): mock_celery.assert_called_once_with(name=TaskNames.DVLA_FILES, args=(job_ids,), queue=QueueNames.PROCESS_FTP) - - -def test_run_letter_jobs_in_trial_sets_job_to_error(client, mocker, sample_letter_template): - sample_letter_template.service.restricted = True - job = create_job(sample_letter_template) - job_ids = [str(job.id)] - mocker.patch( - "app.celery.scheduled_tasks.dao_get_letter_job_ids_by_status", - return_value=job_ids - ) - mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") - - run_letter_jobs() - - assert not mock_celery.called - assert job.job_status == JOB_STATUS_ERROR - - -def test_run_letter_jobs_in_trial_sets_job_to_error_and_process_live_services( - client, mocker, sample_letter_template): - live_job = create_job(sample_letter_template) - - service = create_service(service_name="Sample service 2", restricted=True) - template = create_template(service, template_type=LETTER_TYPE) - trial_job = create_job(template) - - job_ids = [str(live_job.id), str(trial_job.id)] - mocker.patch( - "app.celery.scheduled_tasks.dao_get_letter_job_ids_by_status", - return_value=job_ids - ) - mock_celery = mocker.patch("app.celery.tasks.notify_celery.send_task") - - run_letter_jobs() - - mock_celery.assert_called_once_with(name=TaskNames.DVLA_FILES, - args=([str(live_job.id)],), - queue=QueueNames.PROCESS_FTP) - assert trial_job.job_status == JOB_STATUS_ERROR diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 344ac00ab..feca346e1 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1035,26 +1035,6 @@ def test_should_cancel_job_if_service_is_inactive(sample_service, mock_dvla_file_task.assert_not_called() -def test_should_error_job_if_service_is_restricted_and_letter_template_type( - sample_service, - sample_letter_job, - mocker -): - sample_service.restricted = True - - mocker.patch('app.celery.tasks.s3.get_job_from_s3') - mocker.patch('app.celery.tasks.process_row') - mock_dvla_file_task = mocker.patch('app.celery.tasks.build_dvla_file') - - process_job(sample_letter_job.id) - - job = jobs_dao.dao_get_job_by_id(sample_letter_job.id) - assert job.job_status == JOB_STATUS_ERROR - s3.get_job_from_s3.assert_not_called() - tasks.process_row.assert_not_called() - mock_dvla_file_task.assert_not_called() - - @pytest.mark.parametrize('template_type, expected_class', [ (SMS_TYPE, SMSMessageTemplate), (EMAIL_TYPE, WithSubjectTemplate), diff --git a/tests/app/service/test_service_whitelist.py b/tests/app/service/test_service_whitelist.py index 5edbe8e28..18c0595f8 100644 --- a/tests/app/service/test_service_whitelist.py +++ b/tests/app/service/test_service_whitelist.py @@ -30,10 +30,9 @@ def test_get_whitelist_separates_emails_and_phones(client, sample_service): response = client.get('service/{}/whitelist'.format(sample_service.id), headers=[create_authorization_header()]) assert response.status_code == 200 - assert json.loads(response.get_data(as_text=True)) == { - 'email_addresses': ['service@example.com'], - 'phone_numbers': ['+1800-555-555', '07123456789'] - } + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['email_addresses'] == ['service@example.com'] + assert sorted(json_resp['phone_numbers']) == sorted(['+1800-555-555', '07123456789']) def test_get_whitelist_404s_with_unknown_service_id(client): diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index cb9d4bb29..aa4433a59 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -249,3 +249,19 @@ def test_post_letter_notification_doesnt_send_in_trial(client, sample_trial_lett assert error_json['status_code'] == 403 assert error_json['errors'] == [ {'error': 'BadRequestError', 'message': 'Cannot send letters when service is in trial mode'}] + + +def test_post_letter_notification_calls_update_job_sent_to_dvla_when_service_is_in_trial_mode_but_using_test_key( + client, sample_trial_letter_template, mocker): + build_dvla_task = mocker.patch('app.celery.tasks.build_dvla_file.apply_async') + update_job_task = mocker.patch('app.celery.tasks.update_job_to_sent_to_dvla.apply_async') + + data = { + "template_id": sample_trial_letter_template.id, + "personalisation": {'address_line_1': 'Foo', 'address_line_2': 'Bar', 'postcode': 'Baz'} + } + letter_request(client, data=data, service_id=sample_trial_letter_template.service_id, + key_type=KEY_TYPE_TEST) + job = Job.query.one() + update_job_task.assert_called_once_with([str(job.id)], queue='research-mode-tasks') + assert not build_dvla_task.called From dbeec6c88b28dcc4371bb1ffe29d59813c6d6894 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 5 Sep 2017 11:50:39 +0100 Subject: [PATCH 9/9] Remove unused fixtures from tests --- tests/app/celery/test_tasks.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index feca346e1..a12e6c050 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -611,7 +611,7 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv def test_should_not_build_dvla_file_in_research_mode_for_letter_job( - notify_db, notify_db_session, mocker, sample_service, sample_letter_job, fake_uuid + mocker, sample_service, sample_letter_job, fake_uuid ): test_encrypted_data = 'some encrypted data' sample_service.research_mode = True @@ -633,7 +633,7 @@ def test_should_not_build_dvla_file_in_research_mode_for_letter_job( @freeze_time("2017-08-29 17:30:00") def test_should_update_job_to_sent_to_dvla_in_research_mode_for_letter_job( - notify_db, notify_db_session, mocker, sample_service, sample_letter_job, fake_uuid + mocker, sample_service, sample_letter_job, fake_uuid ): test_encrypted_data = 'some encrypted data' sample_service.research_mode = True