From 79c6671500bdf32ae17e03689378f6e615b59aeb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 11:46:58 +0100 Subject: [PATCH 1/3] Normalise whitespace and use client fixtures Using the client fixture means that fewer nested indentation is needed. Which, along with consistent indenting, makes the code easier to read. --- tests/app/job/test_rest.py | 811 ++++++++++++++++++------------------- 1 file changed, 390 insertions(+), 421 deletions(-) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index a7736d5c6..51cac4d06 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -1,6 +1,7 @@ import json import uuid from datetime import datetime, timedelta +from functools import partial from freezegun import freeze_time import pytest @@ -17,143 +18,131 @@ from app.dao.templates_dao import dao_update_template from app.models import NOTIFICATION_STATUS_TYPES, JOB_STATUS_TYPES, JOB_STATUS_PENDING -def test_get_job_with_invalid_service_id_returns404(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job'.format(sample_service.id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 0 +def test_get_job_with_invalid_service_id_returns404(client, sample_service): + path = '/service/{}/job'.format(sample_service.id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 0 -def test_get_job_with_invalid_job_id_returns404(notify_api, sample_template): +def test_get_job_with_invalid_job_id_returns404(client, sample_template): service_id = sample_template.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job/{}'.format(service_id, "bad-id") - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 404 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['result'] == 'error' - assert resp_json['message'] == 'No result found' + path = '/service/{}/job/{}'.format(service_id, "bad-id") + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'No result found' -def test_get_job_with_unknown_id_returns404(notify_api, sample_template, fake_uuid): +def test_get_job_with_unknown_id_returns404(client, sample_template, fake_uuid): service_id = sample_template.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job/{}'.format(service_id, fake_uuid) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 404 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json == { - 'message': 'No result found', - 'result': 'error' - } + path = '/service/{}/job/{}'.format(service_id, fake_uuid) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json == { + 'message': 'No result found', + 'result': 'error' + } -def test_cancel_job(notify_api, sample_scheduled_job): +def test_cancel_job(client, sample_scheduled_job): job_id = str(sample_scheduled_job.id) service_id = sample_scheduled_job.service.id - with notify_api.test_request_context(), notify_api.test_client() as client: - path = '/service/{}/job/{}/cancel'.format(service_id, job_id) - auth_header = create_authorization_header() - response = client.post(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == job_id - assert resp_json['data']['job_status'] == 'cancelled' + path = '/service/{}/job/{}/cancel'.format(service_id, job_id) + auth_header = create_authorization_header() + response = client.post(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['data']['id'] == job_id + assert resp_json['data']['job_status'] == 'cancelled' -def test_cant_cancel_normal_job(notify_api, sample_job, mocker): +def test_cant_cancel_normal_job(client, sample_job, mocker): job_id = str(sample_job.id) service_id = sample_job.service.id - with notify_api.test_request_context(), notify_api.test_client() as client: - mock_update = mocker.patch('app.dao.jobs_dao.dao_update_job') - path = '/service/{}/job/{}/cancel'.format(service_id, job_id) - auth_header = create_authorization_header() - response = client.post(path, headers=[auth_header]) - assert response.status_code == 404 - assert mock_update.call_count == 0 + mock_update = mocker.patch('app.dao.jobs_dao.dao_update_job') + path = '/service/{}/job/{}/cancel'.format(service_id, job_id) + auth_header = create_authorization_header() + response = client.post(path, headers=[auth_header]) + assert response.status_code == 404 + assert mock_update.call_count == 0 -def test_create_unscheduled_job(notify_api, sample_template, mocker, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.process_job.apply_async') - data = { - 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, - 'created_by': str(sample_template.created_by.id) - } - path = '/service/{}/job'.format(sample_template.service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] +def test_create_unscheduled_job(client, sample_template, mocker, fake_uuid): + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + 'id': fake_uuid, + 'service': str(sample_template.service.id), + 'template': str(sample_template.id), + 'original_file_name': 'thisisatest.csv', + 'notification_count': 1, + 'created_by': str(sample_template.created_by.id) + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) - assert response.status_code == 201 + response = client.post( + path, + data=json.dumps(data), + headers=headers) + assert response.status_code == 201 - app.celery.tasks.process_job.apply_async.assert_called_once_with( - ([str(fake_uuid)]), - queue="job-tasks" - ) + app.celery.tasks.process_job.apply_async.assert_called_once_with( + ([str(fake_uuid)]), + queue="job-tasks" + ) - resp_json = json.loads(response.get_data(as_text=True)) + resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == fake_uuid - assert resp_json['data']['statistics'] == [] - assert resp_json['data']['job_status'] == 'pending' - assert not resp_json['data']['scheduled_for'] - assert resp_json['data']['job_status'] == 'pending' - assert resp_json['data']['template'] == str(sample_template.id) - assert resp_json['data']['original_file_name'] == 'thisisatest.csv' + assert resp_json['data']['id'] == fake_uuid + assert resp_json['data']['statistics'] == [] + assert resp_json['data']['job_status'] == 'pending' + assert not resp_json['data']['scheduled_for'] + assert resp_json['data']['job_status'] == 'pending' + assert resp_json['data']['template'] == str(sample_template.id) + assert resp_json['data']['original_file_name'] == 'thisisatest.csv' -def test_create_scheduled_job(notify_api, sample_template, mocker, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - with freeze_time("2016-01-01 12:00:00.000000"): - scheduled_date = (datetime.utcnow() + timedelta(hours=95, minutes=59)).isoformat() - mocker.patch('app.celery.tasks.process_job.apply_async') - data = { - 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, - 'created_by': str(sample_template.created_by.id), - 'scheduled_for': scheduled_date - } - path = '/service/{}/job'.format(sample_template.service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] +@freeze_time("2016-01-01 12:00:00.000000") +def test_create_scheduled_job(client, sample_template, mocker, fake_uuid): + scheduled_date = (datetime.utcnow() + timedelta(hours=95, minutes=59)).isoformat() + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + 'id': fake_uuid, + 'service': str(sample_template.service.id), + 'template': str(sample_template.id), + 'original_file_name': 'thisisatest.csv', + 'notification_count': 1, + 'created_by': str(sample_template.created_by.id), + 'scheduled_for': scheduled_date + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) - assert response.status_code == 201 + response = client.post( + path, + data=json.dumps(data), + headers=headers) + assert response.status_code == 201 - app.celery.tasks.process_job.apply_async.assert_not_called() + app.celery.tasks.process_job.apply_async.assert_not_called() - resp_json = json.loads(response.get_data(as_text=True)) + resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == fake_uuid - assert resp_json['data']['scheduled_for'] == datetime(2016, 1, 5, 11, 59, 0, - tzinfo=pytz.UTC).isoformat() - assert resp_json['data']['job_status'] == 'scheduled' - assert resp_json['data']['template'] == str(sample_template.id) - assert resp_json['data']['original_file_name'] == 'thisisatest.csv' + assert resp_json['data']['id'] == fake_uuid + assert resp_json['data']['scheduled_for'] == datetime(2016, 1, 5, 11, 59, 0, + tzinfo=pytz.UTC).isoformat() + assert resp_json['data']['job_status'] == 'scheduled' + assert resp_json['data']['template'] == str(sample_template.id) + assert resp_json['data']['original_file_name'] == 'thisisatest.csv' def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, sample_service, mocker): @@ -172,7 +161,8 @@ def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, samp def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( - client, fake_uuid, sample_trial_letter_template, mocker): + client, fake_uuid, sample_trial_letter_template, mocker +): data = { 'id': fake_uuid, 'service': str(sample_trial_letter_template.service.id), @@ -194,166 +184,152 @@ def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( 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: - with freeze_time("2016-01-01 11:09:00.061258"): - scheduled_date = (datetime.utcnow() + timedelta(hours=96, minutes=1)).isoformat() +@freeze_time("2016-01-01 11:09:00.061258") +def test_should_not_create_scheduled_job_more_then_24_hours_hence(client, sample_template, mocker, fake_uuid): + scheduled_date = (datetime.utcnow() + timedelta(hours=96, minutes=1)).isoformat() + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + 'id': fake_uuid, + 'service': str(sample_template.service.id), + 'template': str(sample_template.id), + 'original_file_name': 'thisisatest.csv', + 'notification_count': 1, + 'created_by': str(sample_template.created_by.id), + 'scheduled_for': scheduled_date + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] - mocker.patch('app.celery.tasks.process_job.apply_async') - data = { - 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, - 'created_by': str(sample_template.created_by.id), - 'scheduled_for': scheduled_date - } - path = '/service/{}/job'.format(sample_template.service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) + assert response.status_code == 400 - response = client.post( - path, - data=json.dumps(data), - headers=headers) - assert response.status_code == 400 + app.celery.tasks.process_job.apply_async.assert_not_called() - app.celery.tasks.process_job.apply_async.assert_not_called() - - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['result'] == 'error' - assert 'scheduled_for' in resp_json['message'] - assert resp_json['message']['scheduled_for'] == ['Date cannot be more than 96hrs in the future'] + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert 'scheduled_for' in resp_json['message'] + assert resp_json['message']['scheduled_for'] == ['Date cannot be more than 96hrs in the future'] -def test_should_not_create_scheduled_job_in_the_past(notify_api, sample_template, mocker, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - with freeze_time("2016-01-01 11:09:00.061258"): - scheduled_date = (datetime.utcnow() - timedelta(minutes=1)).isoformat() +@freeze_time("2016-01-01 11:09:00.061258") +def test_should_not_create_scheduled_job_in_the_past(client, sample_template, mocker, fake_uuid): + scheduled_date = (datetime.utcnow() - timedelta(minutes=1)).isoformat() + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + 'id': fake_uuid, + 'service': str(sample_template.service.id), + 'template': str(sample_template.id), + 'original_file_name': 'thisisatest.csv', + 'notification_count': 1, + 'created_by': str(sample_template.created_by.id), + 'scheduled_for': scheduled_date + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] - mocker.patch('app.celery.tasks.process_job.apply_async') - data = { - 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, - 'created_by': str(sample_template.created_by.id), - 'scheduled_for': scheduled_date - } - path = '/service/{}/job'.format(sample_template.service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) + assert response.status_code == 400 - response = client.post( - path, - data=json.dumps(data), - headers=headers) - assert response.status_code == 400 + app.celery.tasks.process_job.apply_async.assert_not_called() - app.celery.tasks.process_job.apply_async.assert_not_called() - - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['result'] == 'error' - assert 'scheduled_for' in resp_json['message'] - assert resp_json['message']['scheduled_for'] == ['Date cannot be in the past'] + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert 'scheduled_for' in resp_json['message'] + assert resp_json['message']['scheduled_for'] == ['Date cannot be in the past'] -def test_create_job_returns_400_if_missing_data(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.process_job.apply_async') - data = { - 'template': str(sample_template.id) - } - path = '/service/{}/job'.format(sample_template.service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) +def test_create_job_returns_400_if_missing_data(client, sample_template, mocker): + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + 'template': str(sample_template.id) + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) - resp_json = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 - app.celery.tasks.process_job.apply_async.assert_not_called() - assert resp_json['result'] == 'error' - assert 'Missing data for required field.' in resp_json['message']['original_file_name'] - assert 'Missing data for required field.' in resp_json['message']['notification_count'] + app.celery.tasks.process_job.apply_async.assert_not_called() + assert resp_json['result'] == 'error' + assert 'Missing data for required field.' in resp_json['message']['original_file_name'] + assert 'Missing data for required field.' in resp_json['message']['notification_count'] -def test_create_job_returns_404_if_template_does_not_exist(notify_api, sample_service, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.process_job.apply_async') - data = { - 'template': str(sample_service.id) - } - path = '/service/{}/job'.format(sample_service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) +def test_create_job_returns_404_if_template_does_not_exist(client, sample_service, mocker): + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + 'template': str(sample_service.id) + } + path = '/service/{}/job'.format(sample_service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) - resp_json = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 - app.celery.tasks.process_job.apply_async.assert_not_called() - assert resp_json['result'] == 'error' - assert resp_json['message'] == 'No result found' + app.celery.tasks.process_job.apply_async.assert_not_called() + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'No result found' -def test_create_job_returns_404_if_missing_service(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.process_job.apply_async') - random_id = str(uuid.uuid4()) - data = {'template': str(sample_template.id)} - path = '/service/{}/job'.format(random_id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) +def test_create_job_returns_404_if_missing_service(client, sample_template, mocker): + mocker.patch('app.celery.tasks.process_job.apply_async') + random_id = str(uuid.uuid4()) + data = {'template': str(sample_template.id)} + path = '/service/{}/job'.format(random_id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) - resp_json = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 - app.celery.tasks.process_job.apply_async.assert_not_called() - assert resp_json['result'] == 'error' - assert resp_json['message'] == 'No result found' + app.celery.tasks.process_job.apply_async.assert_not_called() + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'No result found' -def test_create_job_returns_400_if_archived_template(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.process_job.apply_async') - sample_template.archived = True - dao_update_template(sample_template) - data = { - 'template': str(sample_template.id) - } - path = '/service/{}/job'.format(sample_template.service.id) - auth_header = create_authorization_header() - headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) +def test_create_job_returns_400_if_archived_template(client, sample_template, mocker): + mocker.patch('app.celery.tasks.process_job.apply_async') + sample_template.archived = True + dao_update_template(sample_template) + data = { + 'template': str(sample_template.id) + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) - resp_json = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 - app.celery.tasks.process_job.apply_async.assert_not_called() - assert resp_json['result'] == 'error' - assert 'Template has been deleted' in resp_json['message']['template'] + app.celery.tasks.process_job.apply_async.assert_not_called() + assert resp_json['result'] == 'error' + assert 'Template has been deleted' in resp_json['message']['template'] def _setup_jobs(notify_db, notify_db_session, template, number_of_jobs=5): @@ -365,55 +341,53 @@ def _setup_jobs(notify_db, notify_db_session, template, number_of_jobs=5): template=template) -def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, - notify_db, - notify_db_session, - sample_service): - with notify_api.test_request_context(), notify_api.test_client() as client: - main_job = create_job(notify_db, notify_db_session, service=sample_service) - another_job = create_job(notify_db, notify_db_session, service=sample_service) +def test_get_all_notifications_for_job_in_order_of_job_number( + client, notify_db, notify_db_session, sample_service +): + main_job = create_job(notify_db, notify_db_session, service=sample_service) + another_job = create_job(notify_db, notify_db_session, service=sample_service) - notification_1 = create_notification( - notify_db, - notify_db_session, - job=main_job, - to_field="1", - created_at=datetime.utcnow(), - job_row_number=1 - ) - notification_2 = create_notification( - notify_db, - notify_db_session, - job=main_job, - to_field="2", - created_at=datetime.utcnow(), - job_row_number=2 - ) - notification_3 = create_notification( - notify_db, - notify_db_session, - job=main_job, - to_field="3", - created_at=datetime.utcnow(), - job_row_number=3 - ) - create_notification(notify_db, notify_db_session, job=another_job) + notification_1 = create_notification( + notify_db, + notify_db_session, + job=main_job, + to_field="1", + created_at=datetime.utcnow(), + job_row_number=1 + ) + notification_2 = create_notification( + notify_db, + notify_db_session, + job=main_job, + to_field="2", + created_at=datetime.utcnow(), + job_row_number=2 + ) + notification_3 = create_notification( + notify_db, + notify_db_session, + job=main_job, + to_field="3", + created_at=datetime.utcnow(), + job_row_number=3 + ) + create_notification(notify_db, notify_db_session, job=another_job) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - path='/service/{}/job/{}/notifications'.format(sample_service.id, main_job.id), - headers=[auth_header]) + response = client.get( + path='/service/{}/job/{}/notifications'.format(sample_service.id, main_job.id), + headers=[auth_header]) - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == 3 - assert resp['notifications'][0]['to'] == notification_1.to - assert resp['notifications'][0]['job_row_number'] == notification_1.job_row_number - assert resp['notifications'][1]['to'] == notification_2.to - assert resp['notifications'][1]['job_row_number'] == notification_2.job_row_number - assert resp['notifications'][2]['to'] == notification_3.to - assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number - assert response.status_code == 200 + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 3 + assert resp['notifications'][0]['to'] == notification_1.to + assert resp['notifications'][0]['job_row_number'] == notification_1.job_row_number + assert resp['notifications'][1]['to'] == notification_2.to + assert resp['notifications'][1]['job_row_number'] == notification_2.job_row_number + assert resp['notifications'][2]['to'] == notification_3.to + assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number + assert response.status_code == 200 @pytest.mark.parametrize( @@ -426,33 +400,32 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, ] ) def test_get_all_notifications_for_job_filtered_by_status( - notify_api, + client, notify_db, notify_db_session, sample_service, expected_notification_count, status_args ): - with notify_api.test_request_context(), notify_api.test_client() as client: - job = create_job(notify_db, notify_db_session, service=sample_service) + job = create_job(notify_db, notify_db_session, service=sample_service) - create_notification( - notify_db, - notify_db_session, - job=job, - to_field="1", - created_at=datetime.utcnow(), - status=NOTIFICATION_STATUS_TYPES[0], - job_row_number=1 - ) + create_notification( + notify_db, + notify_db_session, + job=job, + to_field="1", + created_at=datetime.utcnow(), + status=NOTIFICATION_STATUS_TYPES[0], + job_row_number=1 + ) - response = client.get( - path='/service/{}/job/{}/notifications{}'.format(sample_service.id, job.id, status_args), - headers=[create_authorization_header()] - ) - resp = json.loads(response.get_data(as_text=True)) - assert len(resp['notifications']) == expected_notification_count - assert response.status_code == 200 + response = client.get( + path='/service/{}/job/{}/notifications{}'.format(sample_service.id, job.id, status_args), + headers=[create_authorization_header()] + ) + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == expected_notification_count + assert response.status_code == 200 def test_get_all_notifications_for_job_returns_correct_format( @@ -487,85 +460,83 @@ def test_get_job_by_id(notify_api, sample_job): assert resp_json['data']['created_by']['name'] == 'Test User' -def test_get_job_by_id_should_return_statistics(notify_db, notify_db_session, notify_api, sample_job): +def test_get_job_by_id_should_return_statistics(client, notify_db, notify_db_session, notify_api, sample_job): job_id = str(sample_job.id) service_id = sample_job.service.id + partial_notification = partial( + create_notification, notify_db, notify_db_session, service=sample_job.service, job=sample_job + ) + partial_notification(status='created') + partial_notification(status='sending') + partial_notification(status='delivered') + partial_notification(status='pending') + partial_notification(status='failed') + partial_notification(status='technical-failure') # noqa + partial_notification(status='temporary-failure') # noqa + partial_notification(status='permanent-failure') # noqa - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='sending') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='delivered') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='pending') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='technical-failure') # noqa - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='permanent-failure') # noqa - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job/{}'.format(service_id, job_id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == job_id - assert {'status': 'created', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'sending', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'delivered', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'pending', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'failed', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'technical-failure', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'temporary-failure', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'permanent-failure', 'count': 1} in resp_json['data']['statistics'] - assert resp_json['data']['created_by']['name'] == 'Test User' + path = '/service/{}/job/{}'.format(service_id, job_id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['data']['id'] == job_id + assert {'status': 'created', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'sending', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'delivered', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'pending', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'failed', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'technical-failure', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'temporary-failure', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'permanent-failure', 'count': 1} in resp_json['data']['statistics'] + assert resp_json['data']['created_by']['name'] == 'Test User' -def test_get_job_by_id_should_return_summed_statistics(notify_db, notify_db_session, notify_api, sample_job): +def test_get_job_by_id_should_return_summed_statistics(client, notify_db, notify_db_session, notify_api, sample_job): job_id = str(sample_job.id) service_id = sample_job.service.id + partial_notification = partial( + create_notification, notify_db, notify_db_session, service=sample_job.service, job=sample_job + ) + partial_notification(status='created') + partial_notification(status='created') + partial_notification(status='created') + partial_notification(status='sending') + partial_notification(status='failed') + partial_notification(status='failed') + partial_notification(status='failed') + partial_notification(status='technical-failure') + partial_notification(status='temporary-failure') + partial_notification(status='temporary-failure') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='created') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='sending') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='failed') - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='technical-failure') # noqa - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa - create_notification(notify_db, notify_db_session, service=sample_job.service, job=sample_job, status='temporary-failure') # noqa - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job/{}'.format(service_id, job_id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == job_id - assert {'status': 'created', 'count': 3} in resp_json['data']['statistics'] - assert {'status': 'sending', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'failed', 'count': 3} in resp_json['data']['statistics'] - assert {'status': 'technical-failure', 'count': 1} in resp_json['data']['statistics'] - assert {'status': 'temporary-failure', 'count': 2} in resp_json['data']['statistics'] - assert resp_json['data']['created_by']['name'] == 'Test User' + path = '/service/{}/job/{}'.format(service_id, job_id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['data']['id'] == job_id + assert {'status': 'created', 'count': 3} in resp_json['data']['statistics'] + assert {'status': 'sending', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'failed', 'count': 3} in resp_json['data']['statistics'] + assert {'status': 'technical-failure', 'count': 1} in resp_json['data']['statistics'] + assert {'status': 'temporary-failure', 'count': 2} in resp_json['data']['statistics'] + assert resp_json['data']['created_by']['name'] == 'Test User' -def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): +def test_get_jobs(client, notify_db, notify_db_session, sample_template): _setup_jobs(notify_db, notify_db_session, sample_template) service_id = sample_template.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job'.format(service_id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 5 + path = '/service/{}/job'.format(service_id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 5 -def test_get_jobs_with_limit_days(notify_api, notify_db, notify_db_session, sample_template): +def test_get_jobs_with_limit_days(client, notify_db, notify_db_session, sample_template): create_job( notify_db, notify_db_session, @@ -581,28 +552,26 @@ def test_get_jobs_with_limit_days(notify_api, notify_db, notify_db_session, samp service_id = sample_template.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job'.format(service_id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header], query_string={'limit_days': 5}) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 1 + path = '/service/{}/job'.format(service_id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header], query_string={'limit_days': 5}) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 1 -def test_get_jobs_should_return_statistics(notify_db, notify_db_session, notify_api, sample_service): +def test_get_jobs_should_return_statistics(client, notify_db, notify_db_session, notify_api, sample_service): now = datetime.utcnow() earlier = datetime.utcnow() - timedelta(days=1) job_1 = create_job(notify_db, notify_db_session, service=sample_service, created_at=earlier) job_2 = create_job(notify_db, notify_db_session, service=sample_service, created_at=now) - - create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - create_notification(notify_db, notify_db_session, service=sample_service, job=job_1, status='created') - create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') - create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') - create_notification(notify_db, notify_db_session, service=sample_service, job=job_2, status='sending') + partial_notification = partial(create_notification, notify_db, notify_db_session, service=sample_service) + partial_notification(job=job_1, status='created') + partial_notification(job=job_1, status='created') + partial_notification(job=job_1, status='created') + partial_notification(job=job_2, status='sending') + partial_notification(job=job_2, status='sending') + partial_notification(job=job_2, status='sending') with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -619,35 +588,35 @@ def test_get_jobs_should_return_statistics(notify_db, notify_db_session, notify_ def test_get_jobs_should_return_no_stats_if_no_rows_in_notifications( - notify_db, - notify_db_session, - notify_api, - sample_service): + client, + notify_db, + notify_db_session, + notify_api, + sample_service, +): now = datetime.utcnow() earlier = datetime.utcnow() - timedelta(days=1) job_1 = create_job(notify_db, notify_db_session, service=sample_service, created_at=earlier) job_2 = create_job(notify_db, notify_db_session, service=sample_service, created_at=now) - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = '/service/{}/job'.format(sample_service.id) - auth_header = create_authorization_header() - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 2 - assert resp_json['data'][0]['id'] == str(job_2.id) - assert resp_json['data'][0]['statistics'] == [] - assert resp_json['data'][1]['id'] == str(job_1.id) - assert resp_json['data'][1]['statistics'] == [] + path = '/service/{}/job'.format(sample_service.id) + auth_header = create_authorization_header() + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 2 + assert resp_json['data'][0]['id'] == str(job_2.id) + assert resp_json['data'][0]['statistics'] == [] + assert resp_json['data'][1]['id'] == str(job_1.id) + assert resp_json['data'][1]['statistics'] == [] def test_get_jobs_should_paginate( - notify_db, - notify_db_session, - client, - sample_template + notify_db, + notify_db_session, + client, + sample_template ): create_10_jobs(notify_db, notify_db_session, sample_template.service, sample_template) @@ -669,10 +638,10 @@ def test_get_jobs_should_paginate( def test_get_jobs_accepts_page_parameter( - notify_db, - notify_db_session, - client, - sample_template + notify_db, + notify_db_session, + client, + sample_template ): create_10_jobs(notify_db, notify_db_session, sample_template.service, sample_template) @@ -702,12 +671,12 @@ def test_get_jobs_accepts_page_parameter( ('foo', []) ]) def test_get_jobs_can_filter_on_statuses( - notify_db, - notify_db_session, - client, - sample_service, - statuses_filter, - expected_statuses + notify_db, + notify_db_session, + client, + sample_service, + statuses_filter, + expected_statuses ): create_job(notify_db, notify_db_session, job_status='pending') create_job(notify_db, notify_db_session, job_status='in progress') From a4857c08ab66c2a869a6e717df95b15b3527efde Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 11:47:13 +0100 Subject: [PATCH 2/3] Read job metadata from S3 metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All of our uploads now have the metadata about the job set on them in S3. So this commit moves to using that metadata, if it’s there, instead of the data in the body of the post request. The aim of this is to stop the admin app having to post this data, which means that it won’t have to keep this data in the session for the while doing the file upload flow. --- app/aws/s3.py | 20 +++++++---- app/job/rest.py | 8 +++++ tests/app/job/test_rest.py | 72 +++++++++++++++++++++++++++++++++++--- 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index aa1bbed97..b23cca71d 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -18,17 +18,25 @@ def get_s3_object(bucket_name, file_location): return s3.Object(bucket_name, file_location) +def get_job_location(service_id, job_id): + return ( + current_app.config['CSV_UPLOAD_BUCKET_NAME'], + FILE_LOCATION_STRUCTURE.format(service_id, job_id), + ) + + def get_job_from_s3(service_id, job_id): - bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] - file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - obj = get_s3_object(bucket_name, file_location) + obj = get_s3_object(*get_job_location(service_id, job_id)) return obj.get()['Body'].read().decode('utf-8') +def get_job_metadata_from_s3(service_id, job_id): + obj = get_s3_object(*get_job_location(service_id, job_id)) + return obj.get()['Metadata'] + + def remove_job_from_s3(service_id, job_id): - bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] - file_location = FILE_LOCATION_STRUCTURE.format(service_id, job_id) - return remove_s3_object(bucket_name, file_location) + return remove_s3_object(*get_job_location(service_id, job_id)) def get_s3_bucket_objects(bucket_name, subfolder='', older_than=7, limit_days=2): diff --git a/app/job/rest.py b/app/job/rest.py index 4d0f045f7..111834b32 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -5,6 +5,7 @@ from flask import ( current_app ) +from app.aws.s3 import get_job_metadata_from_s3 from app.dao.jobs_dao import ( dao_create_job, dao_update_job, @@ -119,6 +120,13 @@ def create_job(service_id): data.update({ "service": service_id }) + try: + data.update( + **get_job_metadata_from_s3(service_id, data['id']) + ) + except KeyError: + raise InvalidRequest({'id': ['Missing data for required field.']}, status_code=400) + template = dao_get_template_by_id(data['template']) if template.template_type == LETTER_TYPE and service.restricted: diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 51cac4d06..84ee9369e 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -76,6 +76,11 @@ def test_cant_cancel_normal_job(client, sample_job, mocker): def test_create_unscheduled_job(client, sample_template, mocker, 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), + 'original_file_name': 'thisisatest.csv', + 'notification_count': '1', + }) data = { 'id': fake_uuid, 'service': str(sample_template.service.id), @@ -108,12 +113,18 @@ def test_create_unscheduled_job(client, sample_template, mocker, fake_uuid): assert resp_json['data']['job_status'] == 'pending' assert resp_json['data']['template'] == str(sample_template.id) assert resp_json['data']['original_file_name'] == 'thisisatest.csv' + assert resp_json['data']['notification_count'] == 1 @freeze_time("2016-01-01 12:00:00.000000") def test_create_scheduled_job(client, sample_template, mocker, fake_uuid): scheduled_date = (datetime.utcnow() + timedelta(hours=95, minutes=59)).isoformat() 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), + 'original_file_name': 'thisisatest.csv', + 'notification_count': '1', + }) data = { 'id': fake_uuid, 'service': str(sample_template.service.id), @@ -143,6 +154,7 @@ def test_create_scheduled_job(client, sample_template, mocker, fake_uuid): assert resp_json['data']['job_status'] == 'scheduled' assert resp_json['data']['template'] == str(sample_template.id) assert resp_json['data']['original_file_name'] == 'thisisatest.csv' + assert resp_json['data']['notification_count'] == 1 def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, sample_service, mocker): @@ -163,6 +175,11 @@ def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, samp def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( client, fake_uuid, sample_trial_letter_template, mocker ): + mocker.patch('app.job.rest.get_job_metadata_from_s3', return_value={ + 'template_id': str(sample_trial_letter_template.id), + 'original_file_name': 'thisisatest.csv', + 'notification_count': '1', + }) data = { 'id': fake_uuid, 'service': str(sample_trial_letter_template.service.id), @@ -188,6 +205,11 @@ def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( def test_should_not_create_scheduled_job_more_then_24_hours_hence(client, sample_template, mocker, fake_uuid): scheduled_date = (datetime.utcnow() + timedelta(hours=96, minutes=1)).isoformat() 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), + 'original_file_name': 'thisisatest.csv', + 'notification_count': '1', + }) data = { 'id': fake_uuid, 'service': str(sample_template.service.id), @@ -219,6 +241,11 @@ def test_should_not_create_scheduled_job_more_then_24_hours_hence(client, sample def test_should_not_create_scheduled_job_in_the_past(client, sample_template, mocker, fake_uuid): scheduled_date = (datetime.utcnow() - timedelta(minutes=1)).isoformat() 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), + 'original_file_name': 'thisisatest.csv', + 'notification_count': '1', + }) data = { 'id': fake_uuid, 'service': str(sample_template.service.id), @@ -246,10 +273,36 @@ def test_should_not_create_scheduled_job_in_the_past(client, sample_template, mo assert resp_json['message']['scheduled_for'] == ['Date cannot be in the past'] -def test_create_job_returns_400_if_missing_data(client, sample_template, mocker): +def test_create_job_returns_400_if_missing_id(client, sample_template, mocker): 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), + }) + data = {} + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) + + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + + app.celery.tasks.process_job.apply_async.assert_not_called() + assert resp_json['result'] == 'error' + assert 'Missing data for required field.' in resp_json['message']['id'] + + +def test_create_job_returns_400_if_missing_data(client, sample_template, mocker, 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), + }) data = { - 'template': str(sample_template.id) + 'id': fake_uuid, + 'template': str(sample_template.id), } path = '/service/{}/job'.format(sample_template.service.id) auth_header = create_authorization_header() @@ -268,9 +321,13 @@ def test_create_job_returns_400_if_missing_data(client, sample_template, mocker) assert 'Missing data for required field.' in resp_json['message']['notification_count'] -def test_create_job_returns_404_if_template_does_not_exist(client, sample_service, mocker): +def test_create_job_returns_404_if_template_does_not_exist(client, sample_service, mocker, 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_service.id), + }) data = { + 'id': fake_uuid, 'template': str(sample_service.id) } path = '/service/{}/job'.format(sample_service.id) @@ -291,6 +348,9 @@ def test_create_job_returns_404_if_template_does_not_exist(client, sample_servic def test_create_job_returns_404_if_missing_service(client, sample_template, mocker): 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), + }) random_id = str(uuid.uuid4()) data = {'template': str(sample_template.id)} path = '/service/{}/job'.format(random_id) @@ -309,11 +369,15 @@ def test_create_job_returns_404_if_missing_service(client, sample_template, mock assert resp_json['message'] == 'No result found' -def test_create_job_returns_400_if_archived_template(client, sample_template, mocker): +def test_create_job_returns_400_if_archived_template(client, sample_template, mocker, fake_uuid): mocker.patch('app.celery.tasks.process_job.apply_async') sample_template.archived = True dao_update_template(sample_template) + mocker.patch('app.job.rest.get_job_metadata_from_s3', return_value={ + 'template_id': str(sample_template.id), + }) data = { + 'id': fake_uuid, 'template': str(sample_template.id) } path = '/service/{}/job'.format(sample_template.service.id) From 84024a9efc108d861558e5c3e820c9a165f62766 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 30 Apr 2018 11:47:27 +0100 Subject: [PATCH 3/3] Refuse to process invalid files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the admin app won’t be checking the metadata when it starts a job now it’s possible that someone could make a post request which attempts to start a job for an invalid file. This commit adds a check to make sure that can’t happen. This is more of an extra safety thing, rather than something that the admin app or a user will see. --- app/job/rest.py | 4 ++ tests/app/job/test_rest.py | 75 ++++++++++++++++++++++++-------------- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 111834b32..fd1ee56ef 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -127,11 +127,15 @@ def create_job(service_id): except KeyError: raise InvalidRequest({'id': ['Missing data for required field.']}, status_code=400) + data['template'] = data.pop('template_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) + if data.get('valid') != 'True': + raise InvalidRequest("File is not valid, can't create job", 400) + 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 84ee9369e..a4fd20117 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -80,14 +80,11 @@ def test_create_unscheduled_job(client, sample_template, mocker, fake_uuid): 'template_id': str(sample_template.id), 'original_file_name': 'thisisatest.csv', 'notification_count': '1', + 'valid': 'True', }) data = { 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, - 'created_by': str(sample_template.created_by.id) + 'created_by': str(sample_template.created_by.id), } path = '/service/{}/job'.format(sample_template.service.id) auth_header = create_authorization_header() @@ -124,15 +121,12 @@ def test_create_scheduled_job(client, sample_template, mocker, fake_uuid): 'template_id': str(sample_template.id), 'original_file_name': 'thisisatest.csv', 'notification_count': '1', + 'valid': 'True', }) data = { 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, 'created_by': str(sample_template.created_by.id), - 'scheduled_for': scheduled_date + 'scheduled_for': scheduled_date, } path = '/service/{}/job'.format(sample_template.service.id) auth_header = create_authorization_header() @@ -172,6 +166,42 @@ def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, samp mock_job_dao.assert_not_called() +@pytest.mark.parametrize('extra_metadata', ( + {}, + {'valid': 'anything not the string True'}, +)) +def test_create_job_returns_400_if_file_is_invalid( + client, + fake_uuid, + sample_template, + mocker, + extra_metadata, +): + mock_job_dao = mocker.patch("app.dao.jobs_dao.dao_create_job") + auth_header = create_authorization_header() + metadata = dict( + template_id=str(sample_template.id), + original_file_name='thisisatest.csv', + notification_count=1, + **extra_metadata + ) + mocker.patch('app.job.rest.get_job_metadata_from_s3', return_value=metadata) + data = {'id': fake_uuid} + response = client.post( + '/service/{}/job'.format(sample_template.service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 400 + print(response.get_data(as_text=True)) + resp_json = json.loads(response.get_data(as_text=True)) + print(resp_json) + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'File is not valid, can\'t create job' + mock_job_dao.assert_not_called() + + def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( client, fake_uuid, sample_trial_letter_template, mocker ): @@ -182,11 +212,7 @@ def test_create_job_returns_403_if_letter_template_type_and_service_in_trial( }) 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) + '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() @@ -209,15 +235,12 @@ def test_should_not_create_scheduled_job_more_then_24_hours_hence(client, sample 'template_id': str(sample_template.id), 'original_file_name': 'thisisatest.csv', 'notification_count': '1', + 'valid': 'True', }) data = { 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, 'created_by': str(sample_template.created_by.id), - 'scheduled_for': scheduled_date + 'scheduled_for': scheduled_date, } path = '/service/{}/job'.format(sample_template.service.id) auth_header = create_authorization_header() @@ -245,13 +268,10 @@ def test_should_not_create_scheduled_job_in_the_past(client, sample_template, mo 'template_id': str(sample_template.id), 'original_file_name': 'thisisatest.csv', 'notification_count': '1', + 'valid': 'True', }) data = { 'id': fake_uuid, - 'service': str(sample_template.service.id), - 'template': str(sample_template.id), - 'original_file_name': 'thisisatest.csv', - 'notification_count': 1, 'created_by': str(sample_template.created_by.id), 'scheduled_for': scheduled_date } @@ -302,7 +322,7 @@ def test_create_job_returns_400_if_missing_data(client, sample_template, mocker, }) data = { 'id': fake_uuid, - 'template': str(sample_template.id), + 'valid': 'True', } path = '/service/{}/job'.format(sample_template.service.id) auth_header = create_authorization_header() @@ -328,7 +348,6 @@ def test_create_job_returns_404_if_template_does_not_exist(client, sample_servic }) data = { 'id': fake_uuid, - 'template': str(sample_service.id) } path = '/service/{}/job'.format(sample_service.id) auth_header = create_authorization_header() @@ -352,7 +371,7 @@ def test_create_job_returns_404_if_missing_service(client, sample_template, mock 'template_id': str(sample_template.id), }) random_id = str(uuid.uuid4()) - data = {'template': str(sample_template.id)} + data = {} path = '/service/{}/job'.format(random_id) auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] @@ -378,7 +397,7 @@ def test_create_job_returns_400_if_archived_template(client, sample_template, mo }) data = { 'id': fake_uuid, - 'template': str(sample_template.id) + 'valid': 'True', } path = '/service/{}/job'.format(sample_template.service.id) auth_header = create_authorization_header()