diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5aee7c55b..344f05ebe 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -138,11 +138,8 @@ def validate_template(template_id, personalisation, service, notification_type): def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): if reply_to_id: - if notification_type != EMAIL_TYPE: - message = 'email_reply_to_id is not a valid option for {} notification'.format(notification_type) - raise BadRequestError(message=message) try: - dao_get_reply_to_by_id(service_id, reply_to_id) + return dao_get_reply_to_by_id(service_id, reply_to_id).email_address except NoResultFound: message = 'email_reply_to_id {} does not exist in database for service id {}'\ .format(reply_to_id, service_id) @@ -151,9 +148,6 @@ def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): def check_service_sms_sender_id(service_id, sms_sender_id, notification_type): if sms_sender_id: - if notification_type != SMS_TYPE: - message = 'sms_sender_id is not a valid option for {} notification'.format(notification_type) - raise BadRequestError(message=message) try: return dao_get_service_sms_senders_by_id(service_id, sms_sender_id).sms_sender except NoResultFound: diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 1a7959589..56b5512e5 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -127,7 +127,8 @@ post_sms_request = { "scheduled_for": {"type": ["string", "null"], "format": "datetime"}, "sms_sender_id": uuid }, - "required": ["phone_number", "template_id"] + "required": ["phone_number", "template_id"], + "additionalProperties": False } sms_content = { @@ -172,7 +173,8 @@ post_email_request = { "scheduled_for": {"type": ["string", "null"], "format": "datetime"}, "email_reply_to_id": uuid }, - "required": ["email_address", "template_id"] + "required": ["email_address", "template_id"], + "additionalProperties": False } email_content = { @@ -215,7 +217,8 @@ post_letter_request = { "template_id": uuid, "personalisation": letter_personalisation }, - "required": ["template_id", "personalisation"] + "required": ["template_id", "personalisation"], + "additionalProperties": False } letter_content = { diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 72e25fa57..0afd7fdfa 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -63,15 +63,12 @@ def post_notification(notification_type): check_service_has_permission(notification_type, authenticated_service.permissions) scheduled_for = form.get("scheduled_for", None) - service_email_reply_to_id = form.get("email_reply_to_id", None) - service_sms_sender_id = form.get("sms_sender_id", None) check_service_can_schedule_notification(authenticated_service.permissions, scheduled_for) check_rate_limiting(authenticated_service, api_user) - check_service_email_reply_to_id(str(authenticated_service.id), service_email_reply_to_id, notification_type) - sms_sender = check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type) + reply_to = get_reply_to_text(notification_type, form) template, template_with_content = validate_template( form['template_id'], @@ -93,13 +90,13 @@ def post_notification(notification_type): api_key=api_user, template=template, service=authenticated_service, - reply_to_text=sms_sender, + reply_to_text=reply_to ) if notification_type == SMS_TYPE: create_resp_partial = functools.partial( create_post_sms_response_from_notification, - from_number=sms_sender or authenticated_service.get_default_sms_sender() + from_number=reply_to ) elif notification_type == EMAIL_TYPE: create_resp_partial = functools.partial( @@ -196,3 +193,23 @@ def process_letter_notification(*, letter_data, api_key, template): ) return notification + + +def get_reply_to_text(notification_type, form): + service_email_reply_to_id = form.get("email_reply_to_id", None) + service_sms_sender_id = form.get("sms_sender_id", None) + reply_to = None + if notification_type == EMAIL_TYPE: + reply_to = check_service_email_reply_to_id( + str(authenticated_service.id), service_email_reply_to_id, notification_type + ) or authenticated_service.get_default_reply_to_email_address() + + elif notification_type == SMS_TYPE: + reply_to = check_service_sms_sender_id( + str(authenticated_service.id), service_sms_sender_id, notification_type + ) or authenticated_service.get_default_sms_sender() + + elif notification_type == LETTER_TYPE: + reply_to = authenticated_service.get_default_letter_contact() + + return reply_to diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index a20251790..cb841e431 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -338,7 +338,7 @@ def test_check_service_email_reply_to_id_where_reply_to_id_is_none(notification_ def test_check_service_email_reply_to_where_email_reply_to_is_found(sample_service): reply_to_address = create_reply_to_email(sample_service, "test@test.com") - assert check_service_email_reply_to_id(sample_service.id, reply_to_address.id, EMAIL_TYPE) is None + assert check_service_email_reply_to_id(sample_service.id, reply_to_address.id, EMAIL_TYPE) == "test@test.com" def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_service, fake_uuid): @@ -358,15 +358,6 @@ def test_check_service_email_reply_to_id_where_reply_to_id_is_not_found(sample_s .format(fake_uuid, sample_service.id) -@pytest.mark.parametrize('notification_type', ['sms', 'letter']) -def test_check_service_email_reply_to_id_when_channel_type_is_wrong(sample_service, notification_type): - reply_to_address = create_reply_to_email(sample_service, "test@test.com") - with pytest.raises(BadRequestError) as e: - check_service_email_reply_to_id(sample_service.id, reply_to_address.id, notification_type) - assert e.value.status_code == 400 - assert e.value.message == 'email_reply_to_id is not a valid option for {} notification'.format(notification_type) - - @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) def test_check_service_sms_sender_id_where_sms_sender_id_is_none(notification_type): assert check_service_sms_sender_id(None, None, notification_type) is None @@ -392,12 +383,3 @@ def test_check_service_sms_sender_id_where_sms_sender_is_not_found(sample_servic assert e.value.status_code == 400 assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}' \ .format(fake_uuid, sample_service.id) - - -@pytest.mark.parametrize('notification_type', ['email', 'letter']) -def test_check_service_sms_sender_id_when_channel_type_is_wrong(sample_service, notification_type): - sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') - with pytest.raises(BadRequestError) as e: - check_service_sms_sender_id(sample_service.id, sms_sender.id, notification_type) - assert e.value.status_code == 400 - assert e.value.message == 'sms_sender_id is not a valid option for {} notification'.format(notification_type) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index beb70e49a..f29dbcf49 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -229,9 +229,13 @@ def test_notification_returns_400_and_for_schema_problems(client, sample_templat assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 400 - assert error_resp['errors'] == [{'error': 'ValidationError', - 'message': "template_id is a required property" - }] + assert {'error': 'ValidationError', + 'message': "template_id is a required property" + } in error_resp['errors'] + assert {'error': 'ValidationError', + 'message': + 'Additional properties are not allowed (template was unexpected)' + } in error_resp['errors'] @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -256,6 +260,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference assert notification.reference is None + assert notification.reply_to_text is None assert resp_json['content']['body'] == sample_email_template_with_placeholders.content \ .replace('((name))', 'Bob').replace('GOV.UK', u'GOV.\u200bUK') assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject \ @@ -322,7 +327,6 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori notification_type, key_send_to, send_to): - mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) sample = create_sample_template( @@ -356,13 +360,13 @@ def test_send_notification_uses_priority_queue_when_template_is_marked_as_priori [("sms", "phone_number", "07700 900 855"), ("email", "email_address", "sample@email.com")] ) def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded( - client, - notify_db, - notify_db_session, - mocker, - notification_type, - key_send_to, - send_to + client, + notify_db, + notify_db_session, + mocker, + notification_type, + key_send_to, + send_to ): sample = create_sample_template( notify_db, @@ -400,11 +404,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, + 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) @@ -430,15 +433,14 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms( ] -@pytest.mark.parametrize('template_factory,expected_error', [ - (sample_template_without_sms_permission, 'Cannot send text messages'), - (sample_template_without_email_permission, 'Cannot send emails')]) +@pytest.mark.parametrize('recipient,label,template_factory,expected_error', [ + ('07700 900000', 'phone_number', sample_template_without_sms_permission, 'Cannot send text messages'), + ('someone@test.com', 'email_address', sample_template_without_email_permission, 'Cannot send emails')]) def test_post_sms_notification_returns_400_if_not_allowed_to_send_notification( - client, template_factory, expected_error, notify_db, notify_db_session): + client, template_factory, recipient, label, expected_error, notify_db, notify_db_session): sample_template_without_permission = template_factory(notify_db, notify_db_session) data = { - 'phone_number': '07700 900000', - 'email_address': 'someone@test.com', + label: recipient, 'template_id': sample_template_without_permission.id } auth_header = create_authorization_header(service_id=sample_template_without_permission.service.id) @@ -459,12 +461,11 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_notification( def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms( - sample_service, - sample_template, - client, - mocker, + sample_service, + sample_template, + client, + mocker, ): - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { @@ -599,9 +600,9 @@ def test_post_notification_with_wrong_type_of_sender( headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 resp_json = json.loads(response.get_data(as_text=True)) - assert '{} is not a valid option for {} notification'.\ - format(form_label, notification_type) in resp_json['errors'][0]['message'] - assert 'BadRequestError' in resp_json['errors'][0]['error'] + assert 'Additional properties are not allowed ({} was unexpected)'.format(form_label) \ + in resp_json['errors'][0]['message'] + assert 'ValidationError' in resp_json['errors'][0]['error'] def test_post_email_notification_with_valid_reply_to_id_returns_201(client, sample_email_template, mocker): @@ -621,6 +622,7 @@ def test_post_email_notification_with_valid_reply_to_id_returns_201(client, samp resp_json = json.loads(response.get_data(as_text=True)) assert validate(resp_json, post_email_response) == resp_json notification = Notification.query.first() + assert notification.reply_to_text == 'test@test.com' assert resp_json['id'] == str(notification.id) assert mocked.called @@ -639,7 +641,7 @@ def test_post_email_notification_with_invalid_reply_to_id_returns_400(client, sa headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 resp_json = json.loads(response.get_data(as_text=True)) - assert 'email_reply_to_id {} does not exist in database for service id {}'.\ + assert 'email_reply_to_id {} does not exist in database for service id {}'. \ format(fake_uuid, sample_email_template.service_id) in resp_json['errors'][0]['message'] assert 'BadRequestError' in resp_json['errors'][0]['error']