From 5692a8596d81684ab8dc71ccb3892141bb988816 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 22 May 2019 16:07:27 +0100 Subject: [PATCH] Code refactors and corrections, details below: Change method name to be more relevant Check if verification notification we send is a correct one pass in notify db session for tests instead of sample_user Refactor tests by using admin_request instead of client Refactor all tests for affected reply-to endpoints for good measure Allow overwriting reply-to address with the same address Skip checking for duplicates if it's an reply-to email update Fix refactored tests Verify duplicates exception not needed --- app/service/rest.py | 5 +- tests/app/service/test_rest.py | 211 +++++++++++++++++---------------- 2 files changed, 108 insertions(+), 108 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 7c1b3fb0b..e3f627cb6 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -651,7 +651,7 @@ def get_email_reply_to_address(service_id, reply_to_id): @service_blueprint.route('//email-reply-to/verify', methods=['POST']) -def verify_new_service_reply_to_email_address(service_id): +def verify_reply_to_email_address(service_id): email_address, errors = email_data_request_schema.load(request.get_json()) check_if_reply_to_address_already_in_use(service_id, email_address["email"]) template = dao_get_template_by_id(current_app.config['REPLY_TO_EMAIL_ADDRESS_VERIFICATION_TEMPLATE_ID']) @@ -690,7 +690,6 @@ def update_service_reply_to_email_address(service_id, reply_to_email_id): # validate the service exists, throws ResultNotFound exception. dao_fetch_service_by_id(service_id) form = validate(request.get_json(), add_service_email_reply_to_request) - check_if_reply_to_address_already_in_use(service_id, form['email_address']) new_reply_to = update_reply_to_email_address(service_id=service_id, reply_to_id=reply_to_email_id, email_address=form['email_address'], @@ -910,5 +909,5 @@ def check_if_reply_to_address_already_in_use(service_id, email_address): existing_reply_to_addresses = dao_get_reply_to_by_service_id(service_id) if email_address in [i.email_address for i in existing_reply_to_addresses]: raise InvalidRequest( - "Your service already uses '{}' as an email reply-to address.".format(email_address), status_code=400 + "Your service already uses ‘{}’ as an email reply-to address.".format(email_address), status_code=400 ) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ecde5f097..6121d94a1 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -2492,157 +2492,158 @@ def test_get_email_reply_to_addresses_with_multiple_email_addresses(client, noti @freeze_time("2016-01-01 11:09:00.061258") -def test_verify_new_service_reply_to_email_address_should_send_verification_email( - client, sample_user, mocker, verify_reply_to_address_email_template +def test_verify_reply_to_email_address_should_send_verification_email( + admin_request, notify_db, notify_db_session, mocker, verify_reply_to_address_email_template ): service = create_service() mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - data = json.dumps({'email': 'reply-here@example.gov.uk'}) - auth_header = create_authorization_header() + data = {'email': 'reply-here@example.gov.uk'} notify_service = verify_reply_to_address_email_template.service - response = client.post( - url_for('service.verify_new_service_reply_to_email_address', service_id=service.id), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) + response = admin_request.post( + 'service.verify_reply_to_email_address', + service_id=service.id, + _data=data, + _expected_status=201 + ) - assert response.status_code == 201 notification = Notification.query.first() - json_response = json.loads(response.get_data(as_text=True)) - assert json_response["data"] == {"id": str(notification.id)} + assert notification.template_id == verify_reply_to_address_email_template.id + assert response["data"] == {"id": str(notification.id)} mocked.assert_called_once_with([str(notification.id)], queue="notify-internal-tasks") assert notification.reply_to_text == notify_service.get_default_reply_to_email_address() -def test_verify_new_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): - data = json.dumps({'email': 'reply-here@example.gov.uk'}) +def test_verify_reply_to_email_address_doesnt_allow_duplicates(admin_request, notify_db, notify_db_session, mocker): + data = {'email': 'reply-here@example.gov.uk'} service = create_service() create_reply_to_email(service, 'reply-here@example.gov.uk') - auth_header = create_authorization_header() - response = client.post( - url_for('service.verify_new_service_reply_to_email_address', service_id=service.id), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 400 - assert response.json[ - "message" - ] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + response = admin_request.post( + 'service.verify_reply_to_email_address', + service_id=service.id, + _data=data, + _expected_status=400 + ) + assert response["message"] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." -def test_add_service_reply_to_email_address(client, sample_service): - data = json.dumps({"email_address": "new@reply.com", "is_default": True}) - response = client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) +def test_add_service_reply_to_email_address(admin_request, sample_service): + data = {"email_address": "new@reply.com", "is_default": True} + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=data, + _expected_status=201 + ) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) results = ServiceEmailReplyTo.query.all() assert len(results) == 1 - assert json_resp['data'] == results[0].serialize() + assert response['data'] == results[0].serialize() -def test_add_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): - data = json.dumps({"email_address": "reply-here@example.gov.uk", "is_default": True}) +def test_add_service_reply_to_email_address_doesnt_allow_duplicates( + admin_request, notify_db, notify_db_session, mocker +): + data = {"email_address": "reply-here@example.gov.uk", "is_default": True} service = create_service() create_reply_to_email(service, 'reply-here@example.gov.uk') - auth_header = create_authorization_header() - response = client.post('/service/{}/email-reply-to'.format(service.id), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 400 - assert response.json[ - "message" - ] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=service.id, + _data=data, + _expected_status=400 + ) + assert response["message"] == "Your service already uses 'reply-here@example.gov.uk' as an email reply-to address." -def test_add_service_reply_to_email_address_can_add_multiple_addresses(client, sample_service): - data = json.dumps({"email_address": "first@reply.com", "is_default": True}) - client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - - second = json.dumps({"email_address": "second@reply.com", "is_default": True}) - response = client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=second, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) +def test_add_service_reply_to_email_address_can_add_multiple_addresses(admin_request, sample_service): + data = {"email_address": "first@reply.com", "is_default": True} + admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=data, + _expected_status=201 + ) + second = {"email_address": "second@reply.com", "is_default": True} + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=second, + _expected_status=201 + ) results = ServiceEmailReplyTo.query.all() assert len(results) == 2 default = [x for x in results if x.is_default] - assert json_resp['data'] == default[0].serialize() + assert response['data'] == default[0].serialize() first_reply_to_not_default = [x for x in results if not x.is_default] assert first_reply_to_not_default[0].email_address == 'first@reply.com' -def test_add_service_reply_to_email_address_raise_exception_if_no_default(client, sample_service): - data = json.dumps({"email_address": "first@reply.com", "is_default": False}) - response = client.post('/service/{}/email-reply-to'.format(sample_service.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['message'] == 'You must have at least one reply to email address as the default.' +def test_add_service_reply_to_email_address_raise_exception_if_no_default(admin_request, sample_service): + data = {"email_address": "first@reply.com", "is_default": False} + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=sample_service.id, + _data=data, + _expected_status=400 + ) + assert response['message'] == 'You must have at least one reply to email address as the default.' -def test_add_service_reply_to_email_address_404s_when_invalid_service_id(client, notify_db, notify_db_session): - response = client.post('/service/{}/email-reply-to'.format(uuid.uuid4()), - data={}, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) +def test_add_service_reply_to_email_address_404s_when_invalid_service_id(admin_request, notify_db, notify_db_session): + response = admin_request.post( + 'service.add_service_reply_to_email_address', + service_id=uuid.uuid4(), + _data={}, + _expected_status=404 + ) - assert response.status_code == 404 - result = json.loads(response.get_data(as_text=True)) - assert result['result'] == 'error' - assert result['message'] == 'No result found' + assert response['result'] == 'error' + assert response['message'] == 'No result found' -def test_update_service_reply_to_email_address(client, sample_service): +def test_update_service_reply_to_email_address(admin_request, sample_service): original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com", "is_default": True}) - response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + data = {"email_address": "changed@reply.com", "is_default": True} + response = admin_request.post( + 'service.update_service_reply_to_email_address', + service_id=sample_service.id, + reply_to_email_id=original_reply_to.id, + _data=data, + _expected_status=200 + ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) results = ServiceEmailReplyTo.query.all() assert len(results) == 1 - assert json_resp['data'] == results[0].serialize() + assert response['data'] == results[0].serialize() -def test_update_service_reply_to_email_address_doesnt_allow_duplicates(client, sample_user, mocker): - service = create_service() - original_reply_to = create_reply_to_email(service=service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com", "is_default": True}) - create_reply_to_email(service, 'changed@reply.com') - response = client.post('/service/{}/email-reply-to/{}'.format(service.id, original_reply_to.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) - assert response.status_code == 400 - assert response.json["message"] == "Your service already uses 'changed@reply.com' as an email reply-to address." - - -def test_update_service_reply_to_email_address_returns_400_when_no_default(client, sample_service): +def test_update_service_reply_to_email_address_returns_400_when_no_default(admin_request, sample_service): original_reply_to = create_reply_to_email(service=sample_service, email_address="some@email.com") - data = json.dumps({"email_address": "changed@reply.com", "is_default": False}) - response = client.post('/service/{}/email-reply-to/{}'.format(sample_service.id, original_reply_to.id), - data=data, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) + data = {"email_address": "changed@reply.com", "is_default": False} + response = admin_request.post( + 'service.update_service_reply_to_email_address', + service_id=sample_service.id, + reply_to_email_id=original_reply_to.id, + _data=data, + _expected_status=400 + ) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['message'] == 'You must have at least one reply to email address as the default.' + assert response['message'] == 'You must have at least one reply to email address as the default.' -def test_update_service_reply_to_email_address_404s_when_invalid_service_id(client, notify_db, notify_db_session): - response = client.post('/service/{}/email-reply-to/{}'.format(uuid.uuid4(), uuid.uuid4()), - data={}, - headers=[('Content-Type', 'application/json'), create_authorization_header()]) +def test_update_service_reply_to_email_address_404s_when_invalid_service_id( + admin_request, notify_db, notify_db_session +): + response = admin_request.post( + 'service.update_service_reply_to_email_address', + service_id=uuid.uuid4(), + reply_to_email_id=uuid.uuid4(), + _data={}, + _expected_status=404 + ) - assert response.status_code == 404 - result = json.loads(response.get_data(as_text=True)) - assert result['result'] == 'error' - assert result['message'] == 'No result found' + assert response['result'] == 'error' + assert response['message'] == 'No result found' def test_delete_service_reply_to_email_address_archives_an_email_reply_to(