From 1f9aade749eec1eb980de2d866263d6d6e4afeeb Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 11 Jun 2019 16:45:35 +0100 Subject: [PATCH 1/3] - Adding test where post_notification can't send to a member outside of the team or whitelist. - Updating some tests to be pytest4 compliant. --- tests/app/conftest.py | 14 ++- tests/app/dao/test_templates_dao.py | 77 ++++--------- tests/app/db.py | 4 +- tests/app/test_model.py | 6 +- .../notifications/test_post_notifications.py | 108 +++++++++++------- 5 files changed, 102 insertions(+), 107 deletions(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 9ca2f2646..56ba4e099 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -59,6 +59,7 @@ from tests.app.db import ( create_inbound_number, create_letter_contact, create_invited_org_user, + create_job ) @@ -502,13 +503,14 @@ def sample_notification_with_job( api_key=None, key_type=KEY_TYPE_NORMAL ): + if not service: + service = create_service() + if not template: + template = create_template(service=service) if job is None: - job = sample_job(notify_db, notify_db_session, service=service, template=template) - return sample_notification( - notify_db, - notify_db_session, - service, - template, + job = create_job(template=template) + return create_notification( + template=template, job=job, job_row_number=job_row_number if job_row_number is not None else None, to_field=to_field, diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 3f528e1fd..05df996be 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -20,7 +20,6 @@ from app.models import ( TemplateRedacted ) -from tests.app.conftest import sample_template as create_sample_template from tests.app.db import create_template, create_letter_contact @@ -222,7 +221,7 @@ def test_redact_template(sample_template): assert redacted.updated_by_id == sample_template.created_by_id -def test_get_all_templates_for_service(notify_db, notify_db_session, service_factory): +def test_get_all_templates_for_service(notify_db_session, service_factory): service_1 = service_factory.get('service 1', email_from='service.1') service_2 = service_factory.get('service 2', email_from='service.2') @@ -230,29 +229,23 @@ def test_get_all_templates_for_service(notify_db, notify_db_session, service_fac assert len(dao_get_all_templates_for_service(service_1.id)) == 1 assert len(dao_get_all_templates_for_service(service_2.id)) == 1 - create_sample_template( - notify_db, - notify_db_session, + create_template( + service=service_1, template_name='Sample Template 1', template_type="sms", content="Template content", - service=service_1 ) - create_sample_template( - notify_db, - notify_db_session, + create_template( + service=service_1, template_name='Sample Template 2', template_type="sms", content="Template content", - service=service_1 ) - create_sample_template( - notify_db, - notify_db_session, + create_template( + service=service_2, template_name='Sample Template 3', template_type="sms", content="Template content", - service=service_2 ) assert Template.query.count() == 5 @@ -260,26 +253,20 @@ def test_get_all_templates_for_service(notify_db, notify_db_session, service_fac assert len(dao_get_all_templates_for_service(service_2.id)) == 2 -def test_get_all_templates_for_service_is_alphabetised(notify_db, notify_db_session, sample_service): - create_sample_template( - notify_db, - notify_db_session, +def test_get_all_templates_for_service_is_alphabetised(notify_db_session, sample_service): + create_template( template_name='Sample Template 1', template_type="sms", content="Template content", service=sample_service ) - template_2 = create_sample_template( - notify_db, - notify_db_session, + template_2 = create_template( template_name='Sample Template 2', template_type="sms", content="Template content", service=sample_service ) - create_sample_template( - notify_db, - notify_db_session, + create_template( template_name='Sample Template 3', template_type="sms", content="Template content", @@ -302,17 +289,13 @@ def test_get_all_returns_empty_list_if_no_templates(sample_service): assert len(dao_get_all_templates_for_service(sample_service.id)) == 0 -def test_get_all_templates_ignores_archived_templates(notify_db, notify_db_session, sample_service): - normal_template = create_sample_template( - notify_db, - notify_db_session, +def test_get_all_templates_ignores_archived_templates(notify_db_session, sample_service): + normal_template = create_template( template_name='Normal Template', service=sample_service, archived=False ) - archived_template = create_sample_template( - notify_db, - notify_db_session, + archived_template = create_template( template_name='Archived Template', service=sample_service ) @@ -326,18 +309,14 @@ def test_get_all_templates_ignores_archived_templates(notify_db, notify_db_sessi assert templates[0] == normal_template -def test_get_all_templates_ignores_hidden_templates(notify_db, notify_db_session, sample_service): - normal_template = create_sample_template( - notify_db, - notify_db_session, +def test_get_all_templates_ignores_hidden_templates(notify_db_session, sample_service): + normal_template = create_template( template_name='Normal Template', service=sample_service, archived=False ) - create_sample_template( - notify_db, - notify_db_session, + create_template( template_name='Hidden Template', hidden=True, service=sample_service @@ -349,10 +328,8 @@ def test_get_all_templates_ignores_hidden_templates(notify_db, notify_db_session assert templates[0] == normal_template -def test_get_template_by_id_and_service(notify_db, notify_db_session, sample_service): - sample_template = create_sample_template( - notify_db, - notify_db_session, +def test_get_template_by_id_and_service(notify_db_session, sample_service): + sample_template = create_template( template_name='Test Template', service=sample_service) template = dao_get_template_by_id_and_service_id( @@ -364,10 +341,8 @@ def test_get_template_by_id_and_service(notify_db, notify_db_session, sample_ser assert not template.redact_personalisation -def test_get_template_by_id_and_service_returns_none_for_hidden_templates(notify_db, notify_db_session, sample_service): - sample_template = create_sample_template( - notify_db, - notify_db_session, +def test_get_template_by_id_and_service_returns_none_for_hidden_templates(notify_db_session, sample_service): + sample_template = create_template( template_name='Test Template', hidden=True, service=sample_service @@ -380,10 +355,8 @@ def test_get_template_by_id_and_service_returns_none_for_hidden_templates(notify ) -def test_get_template_version_returns_none_for_hidden_templates(notify_db, notify_db_session, sample_service): - sample_template = create_sample_template( - notify_db, - notify_db_session, +def test_get_template_version_returns_none_for_hidden_templates(notify_db_session, sample_service): + sample_template = create_template( template_name='Test Template', hidden=True, service=sample_service @@ -505,9 +478,7 @@ def test_get_template_versions(sample_template): def test_get_template_versions_is_empty_for_hidden_templates(notify_db, notify_db_session, sample_service): - sample_template = create_sample_template( - notify_db, - notify_db_session, + sample_template = create_template( template_name='Test Template', hidden=True, service=sample_service diff --git a/tests/app/db.py b/tests/app/db.py index bb388272b..b4213bf01 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -176,6 +176,7 @@ def create_template( archived=False, folder=None, postage=None, + process_type='normal', ): data = { 'name': template_name or '{} Template Name'.format(template_type), @@ -185,7 +186,8 @@ def create_template( 'created_by': service.created_by, 'reply_to': reply_to, 'hidden': hidden, - 'folder': folder + 'folder': folder, + 'process_type': process_type } if template_type == LETTER_TYPE: data["postage"] = postage or "second" diff --git a/tests/app/test_model.py b/tests/app/test_model.py index afea955d9..0c7b78bce 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -181,9 +181,9 @@ def test_notification_subject_fills_in_placeholders_for_email(sample_email_templ assert noti.subject == 'hello' -def test_notification_subject_fills_in_placeholders_for_letter(sample_letter_template): - noti = create_notification(sample_letter_template, personalisation={'name': 'hello'}) - noti.template.subject = '((name))' +def test_notification_subject_fills_in_placeholders_for_letter(sample_service): + template = create_template(service=sample_service, template_type='email', subject='((name))') + noti = create_notification(template=template, personalisation={'name': 'hello'}) assert noti.subject == 'hello' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 35efb3be0..df3945b9a 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -10,7 +10,8 @@ from app.models import ( NOTIFICATION_CREATED, SCHEDULE_NOTIFICATIONS, SMS_TYPE, - UPLOAD_DOCUMENT + UPLOAD_DOCUMENT, + INTERNATIONAL_SMS_TYPE ) from flask import json, current_app @@ -19,19 +20,14 @@ from app.schema_validation import validate from app.v2.errors import RateLimitError from app.v2.notifications.notification_schemas import post_sms_response, post_email_response from tests import create_authorization_header -from tests.app.conftest import ( - sample_template as create_sample_template, - sample_service, - sample_template_without_email_permission, - sample_template_without_sms_permission -) from tests.app.db import ( create_service, create_template, create_reply_to_email, create_service_sms_sender, - create_service_with_inbound_number + create_service_with_inbound_number, + create_api_key ) @@ -375,17 +371,18 @@ def test_should_not_persist_or_send_notification_if_simulated_recipient( @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "07700 900 855"), ("email", "email_address", "sample@email.com")]) -def test_send_notification_uses_priority_queue_when_template_is_marked_as_priority(client, notify_db, - notify_db_session, - mocker, - notification_type, - key_send_to, - send_to): +def test_send_notification_uses_priority_queue_when_template_is_marked_as_priority( + client, + sample_service, + mocker, + notification_type, + key_send_to, + send_to +): mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) - sample = create_sample_template( - notify_db, - notify_db_session, + sample = create_template( + service=sample_service, template_type=notification_type, process_type='priority' ) @@ -415,18 +412,13 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori ) def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( client, - notify_db, - notify_db_session, + sample_service, mocker, notification_type, key_send_to, send_to ): - sample = create_sample_template( - notify_db, - notify_db_session, - template_type=notification_type - ) + sample = create_template(service=sample_service, template_type=notification_type) persist_mock = mocker.patch('app.v2.notifications.post_notifications.persist_notification') deliver_mock = mocker.patch('app.v2.notifications.post_notifications.send_notification_to_queue') mocker.patch( @@ -459,11 +451,10 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms( client, - notify_db, notify_db_session, ): - service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE]) - template = create_sample_template(notify_db, notify_db_session, service=service) + service = create_service(service_permissions=[SMS_TYPE]) + template = create_template(service=service) data = { 'phone_number': '20-12-1234-1234', @@ -511,12 +502,14 @@ def test_post_sms_notification_with_archived_reply_to_id_returns_400(client, sam assert 'BadRequestError' in resp_json['errors'][0]['error'] -@pytest.mark.parametrize('recipient,label,template_factory,expected_error', [ - ('07700 900000', 'phone_number', sample_template_without_sms_permission, 'text messages'), - ('someone@test.com', 'email_address', sample_template_without_email_permission, 'emails')]) +@pytest.mark.parametrize('recipient,label,permission_type, notification_type,expected_error', [ + ('07700 900000', 'phone_number', 'email', 'sms', 'text messages'), + ('someone@test.com', 'email_address', 'sms', 'email', 'emails')]) def test_post_sms_notification_returns_400_if_not_allowed_to_send_notification( - client, template_factory, recipient, label, expected_error, notify_db, notify_db_session): - sample_template_without_permission = template_factory(notify_db, notify_db_session) + notify_db_session, client, recipient, label, permission_type, notification_type, expected_error +): + service = create_service(service_permissions=[permission_type]) + sample_template_without_permission = create_template(service=service, template_type=notification_type) data = { label: recipient, 'template_id': sample_template_without_permission.id @@ -538,6 +531,33 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_notification( ] +@pytest.mark.parametrize('restricted', [True, False]) +def test_post_sms_notification_returns_400_if_number_not_whitelisted( + notify_db_session, client, restricted +): + service = create_service(restricted=restricted, service_permissions=[SMS_TYPE, INTERNATIONAL_SMS_TYPE]) + template = create_template(service=service) + create_api_key(service=service, key_type='team') + + data = { + "phone_number": '+327700900855', + "template_id": template.id, + } + auth_header = create_authorization_header(service_id=service.id, key_type='team') + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['status_code'] == 400 + assert error_json['errors'] == [ + {"error": "BadRequestError", "message": 'Can’t send to this recipient using a team-only API key'} + ] + + def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms( sample_service, sample_template, @@ -750,10 +770,10 @@ def test_post_email_notification_with_archived_reply_to_id_returns_400(client, s assert 'BadRequestError' in resp_json['errors'][0]['error'] -def test_post_notification_with_document_upload(client, notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) - template = create_sample_template( - notify_db, notify_db_session, service=service, +def test_post_notification_with_document_upload(client, notify_db_session, mocker): + service = create_service(service_permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) + template = create_template( + service=service, template_type='email', content="Document: ((document))" ) @@ -785,10 +805,10 @@ def test_post_notification_with_document_upload(client, notify_db, notify_db_ses assert resp_json['content']['body'] == 'Document: https://document-url/' -def test_post_notification_with_document_upload_simulated(client, notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) - template = create_sample_template( - notify_db, notify_db_session, service=service, +def test_post_notification_with_document_upload_simulated(client, notify_db_session, mocker): + service = create_service(service_permissions=[EMAIL_TYPE, UPLOAD_DOCUMENT]) + template = create_template( + service=service, template_type='email', content="Document: ((document))" ) @@ -816,10 +836,10 @@ def test_post_notification_with_document_upload_simulated(client, notify_db, not assert resp_json['content']['body'] == 'Document: https://document-url/test-document' -def test_post_notification_without_document_upload_permission(client, notify_db, notify_db_session, mocker): - service = sample_service(notify_db, notify_db_session, permissions=[EMAIL_TYPE]) - template = create_sample_template( - notify_db, notify_db_session, service=service, +def test_post_notification_without_document_upload_permission(client, notify_db_session, mocker): + service = create_service(service_permissions=[EMAIL_TYPE]) + template = create_template( + service=service, template_type='email', content="Document: ((document))" ) From 35d2e099f733eeb2af4a526ad3a8feb9a1a4e0ff Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Jun 2019 16:16:50 +0100 Subject: [PATCH 2/3] - Removed unused method parameters. - Use parametrize for test. --- tests/app/dao/test_templates_dao.py | 16 ++++++++-------- tests/app/test_model.py | 14 +++++--------- .../v2/notifications/test_post_notifications.py | 7 ++++--- 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 05df996be..01f4ddaa1 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -221,7 +221,7 @@ def test_redact_template(sample_template): assert redacted.updated_by_id == sample_template.created_by_id -def test_get_all_templates_for_service(notify_db_session, service_factory): +def test_get_all_templates_for_service(service_factory): service_1 = service_factory.get('service 1', email_from='service.1') service_2 = service_factory.get('service 2', email_from='service.2') @@ -253,7 +253,7 @@ def test_get_all_templates_for_service(notify_db_session, service_factory): assert len(dao_get_all_templates_for_service(service_2.id)) == 2 -def test_get_all_templates_for_service_is_alphabetised(notify_db_session, sample_service): +def test_get_all_templates_for_service_is_alphabetised(sample_service): create_template( template_name='Sample Template 1', template_type="sms", @@ -289,7 +289,7 @@ def test_get_all_returns_empty_list_if_no_templates(sample_service): assert len(dao_get_all_templates_for_service(sample_service.id)) == 0 -def test_get_all_templates_ignores_archived_templates(notify_db_session, sample_service): +def test_get_all_templates_ignores_archived_templates(sample_service): normal_template = create_template( template_name='Normal Template', service=sample_service, @@ -309,7 +309,7 @@ def test_get_all_templates_ignores_archived_templates(notify_db_session, sample_ assert templates[0] == normal_template -def test_get_all_templates_ignores_hidden_templates(notify_db_session, sample_service): +def test_get_all_templates_ignores_hidden_templates(sample_service): normal_template = create_template( template_name='Normal Template', service=sample_service, @@ -328,7 +328,7 @@ def test_get_all_templates_ignores_hidden_templates(notify_db_session, sample_se assert templates[0] == normal_template -def test_get_template_by_id_and_service(notify_db_session, sample_service): +def test_get_template_by_id_and_service(sample_service): sample_template = create_template( template_name='Test Template', service=sample_service) @@ -341,7 +341,7 @@ def test_get_template_by_id_and_service(notify_db_session, sample_service): assert not template.redact_personalisation -def test_get_template_by_id_and_service_returns_none_for_hidden_templates(notify_db_session, sample_service): +def test_get_template_by_id_and_service_returns_none_for_hidden_templates(sample_service): sample_template = create_template( template_name='Test Template', hidden=True, @@ -355,7 +355,7 @@ def test_get_template_by_id_and_service_returns_none_for_hidden_templates(notify ) -def test_get_template_version_returns_none_for_hidden_templates(notify_db_session, sample_service): +def test_get_template_version_returns_none_for_hidden_templates(sample_service): sample_template = create_template( template_name='Test Template', hidden=True, @@ -477,7 +477,7 @@ def test_get_template_versions(sample_template): assert len(v) == 2 -def test_get_template_versions_is_empty_for_hidden_templates(notify_db, notify_db_session, sample_service): +def test_get_template_versions_is_empty_for_hidden_templates(sample_service): sample_template = create_template( template_name='Test Template', hidden=True, diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 0c7b78bce..bb2961a6b 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -176,15 +176,11 @@ def test_notification_subject_is_none_for_sms(): assert Notification(notification_type=SMS_TYPE).subject is None -def test_notification_subject_fills_in_placeholders_for_email(sample_email_template_with_placeholders): - noti = create_notification(sample_email_template_with_placeholders, personalisation={'name': 'hello'}) - assert noti.subject == 'hello' - - -def test_notification_subject_fills_in_placeholders_for_letter(sample_service): - template = create_template(service=sample_service, template_type='email', subject='((name))') - noti = create_notification(template=template, personalisation={'name': 'hello'}) - assert noti.subject == 'hello' +@pytest.mark.parametrize('template_type', ['email', 'letter']) +def test_notification_subject_fills_in_placeholders(sample_service, template_type): + template = create_template(service=sample_service, template_type=template_type, subject='((name))') + notification = create_notification(template=template, personalisation={'name': 'hello'}) + assert notification.subject == 'hello' def test_letter_notification_serializes_with_address(client, sample_letter_notification): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index df3945b9a..9c4440619 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -609,8 +609,9 @@ def test_post_sms_should_persist_supplied_sms_number(client, sample_template_wit [("sms", "phone_number", "07700 900 855"), ("email", "email_address", "sample@email.com")]) @freeze_time("2017-05-14 14:00:00") -def test_post_notification_with_scheduled_for(client, notify_db, notify_db_session, - notification_type, key_send_to, send_to): +def test_post_notification_with_scheduled_for( + client, notify_db_session, notification_type, key_send_to, send_to +): service = create_service(service_name=str(uuid.uuid4()), service_permissions=[EMAIL_TYPE, SMS_TYPE, SCHEDULE_NOTIFICATIONS]) template = create_template(service=service, template_type=notification_type) @@ -863,7 +864,7 @@ def test_post_notification_without_document_upload_permission(client, notify_db_ assert response.status_code == 400, response.get_data(as_text=True) -def test_post_notification_returns_400_when_get_json_throws_exception(client, sample_email_template, rmock, mocker): +def test_post_notification_returns_400_when_get_json_throws_exception(client, sample_email_template): auth_header = create_authorization_header(service_id=sample_email_template.service_id) response = client.post( path="v2/notifications/email", From 2cd657cec6fb8d18a6663ecef021cfb03cac47b4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Jun 2019 16:18:48 +0100 Subject: [PATCH 3/3] Raise given exception --- app/user/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/user/rest.py b/app/user/rest.py index 444dac914..d4e140fb9 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -68,7 +68,7 @@ def handle_integrity_error(exc): current_app.logger.exception('Check constraint ck_users_mobile_or_email_auth triggered') return jsonify(result='error', message='Mobile number must be set if auth_type is set to sms_auth'), 400 - raise + raise exc @user_blueprint.route('', methods=['POST'])