diff --git a/app/dao/service_sms_sender_dao.py b/app/dao/service_sms_sender_dao.py index fcb5ff92c..0cddd606d 100644 --- a/app/dao/service_sms_sender_dao.py +++ b/app/dao/service_sms_sender_dao.py @@ -19,12 +19,16 @@ def insert_service_sms_sender(service, sms_sender): def dao_get_service_sms_senders_by_id(service_id, service_sms_sender_id): return ServiceSmsSender.query.filter_by( id=service_sms_sender_id, - service_id=service_id + service_id=service_id, + archived=False ).one() def dao_get_sms_senders_by_service_id(service_id): - return ServiceSmsSender.query.filter_by(service_id=service_id).order_by(desc(ServiceSmsSender.is_default)).all() + return ServiceSmsSender.query.filter_by( + service_id=service_id, + archived=False + ).order_by(desc(ServiceSmsSender.is_default)).all() @transactional diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 1aba31259..08bf6dd37 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -90,14 +90,15 @@ def send_one_off_notification(service_id, post_data): def get_reply_to_text(notification_type, sender_id, service, template): reply_to = None if sender_id: - if notification_type == EMAIL_TYPE: - try: - reply_to = dao_get_reply_to_by_id(service.id, sender_id).email_address - except NoResultFound: + try: + if notification_type == EMAIL_TYPE: message = 'Reply to email address not found' - raise BadRequestError(message=message) - elif notification_type == SMS_TYPE: - reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).get_reply_to_text() + reply_to = dao_get_reply_to_by_id(service.id, sender_id).email_address + elif notification_type == SMS_TYPE: + message = 'SMS sender not found' + reply_to = dao_get_service_sms_senders_by_id(service.id, sender_id).get_reply_to_text() + except NoResultFound: + raise BadRequestError(message=message) else: reply_to = template.get_reply_to_text() return reply_to diff --git a/tests/app/dao/test_service_sms_sender_dao.py b/tests/app/dao/test_service_sms_sender_dao.py index 5b9c9296d..414c7899a 100644 --- a/tests/app/dao/test_service_sms_sender_dao.py +++ b/tests/app/dao/test_service_sms_sender_dao.py @@ -10,7 +10,7 @@ from app.dao.service_sms_sender_dao import ( dao_get_sms_senders_by_service_id, update_existing_sms_sender_with_inbound_number) from app.models import ServiceSmsSender -from tests.app.db import create_service, create_inbound_number +from tests.app.db import create_service, create_inbound_number, create_service_sms_sender def test_dao_get_service_sms_senders_id(notify_db_session): @@ -32,6 +32,18 @@ def test_dao_get_service_sms_senders_id_raise_exception_when_not_found(notify_db service_sms_sender_id=uuid.uuid4()) +def test_dao_get_service_sms_senders_id_raises_exception_with_archived_sms_sender(notify_db_session): + service = create_service() + archived_sms_sender = create_service_sms_sender( + service=service, + sms_sender="second", + is_default=False, + archived=True) + with pytest.raises(expected_exception=SQLAlchemyError): + dao_get_service_sms_senders_by_id(service_id=service.id, + service_sms_sender_id=archived_sms_sender.id) + + def test_dao_get_sms_senders_by_service_id(notify_db_session): service = create_service() second_sender = dao_add_sms_sender_for_service(service_id=service.id, @@ -47,6 +59,19 @@ def test_dao_get_sms_senders_by_service_id(notify_db_session): assert x == second_sender +def test_dao_get_sms_senders_by_service_id_does_not_return_archived_senders(notify_db_session): + service = create_service() + archived_sms_sender = create_service_sms_sender( + service=service, + sms_sender="second", + is_default=False, + archived=True) + results = dao_get_sms_senders_by_service_id(service_id=service.id) + + assert len(results) == 1 + assert archived_sms_sender not in results + + def test_dao_add_sms_sender_for_service(notify_db_session): service = create_service() new_sms_sender = dao_add_sms_sender_for_service(service_id=service.id, diff --git a/tests/app/db.py b/tests/app/db.py index 998997730..b7b618ca9 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -418,16 +418,18 @@ def create_reply_to_email( def create_service_sms_sender( - service, - sms_sender, - is_default=True, - inbound_number_id=None + service, + sms_sender, + is_default=True, + inbound_number_id=None, + archived=False ): data = { 'service_id': service.id, 'sms_sender': sms_sender, 'is_default': is_default, - 'inbound_number_id': inbound_number_id + 'inbound_number_id': inbound_number_id, + 'archived': archived, } service_sms_sender = ServiceSmsSender(**data) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 92e8ddd41..eb6907850 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -10,6 +10,7 @@ from freezegun import freeze_time from app.celery.scheduled_tasks import daily_stats_template_usage_by_month from app.dao.organisation_dao import dao_add_service_to_organisation +from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id from app.dao.services_dao import dao_remove_user_from_service from app.dao.templates_dao import dao_redact_template from app.dao.users_dao import save_model_user @@ -2715,9 +2716,10 @@ def test_add_service_sms_sender_can_add_multiple_senders(client, notify_db_sessi assert len(senders) == 2 -def test_add_service_sms_sender_when_it_is_an_inbound_number_updates_the_only_existing_sms_sender( +def test_add_service_sms_sender_when_it_is_an_inbound_number_updates_the_only_existing_non_archived_sms_sender( client, notify_db_session): service = create_service_with_defined_sms_sender(sms_sender_value='GOVUK') + create_service_sms_sender(service=service, sms_sender="archived", is_default=False, archived=True) inbound_number = create_inbound_number(number='12345') data = { "sms_sender": str(inbound_number.id), @@ -2736,7 +2738,7 @@ def test_add_service_sms_sender_when_it_is_an_inbound_number_updates_the_only_ex assert resp_json['inbound_number_id'] == str(inbound_number.id) assert resp_json['is_default'] - senders = ServiceSmsSender.query.all() + senders = dao_get_sms_senders_by_service_id(service.id) assert len(senders) == 1 diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index b03eed941..2e3bac985 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -3,7 +3,6 @@ from unittest.mock import Mock import pytest from notifications_utils.recipients import InvalidPhoneError -from sqlalchemy.exc import SQLAlchemyError from app.v2.errors import BadRequestError, TooManyRequestsError from app.config import QueueNames @@ -334,5 +333,6 @@ def test_send_one_off_notification_should_throw_exception_if_sms_sender_id_doesn 'created_by': str(sample_template.service.created_by_id) } - with pytest.raises(expected_exception=SQLAlchemyError): + with pytest.raises(expected_exception=BadRequestError) as e: send_one_off_notification(service_id=sample_template.service.id, post_data=data) + assert e.value.message == 'SMS sender not found' diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index eeb629d91..ce2ae1ec0 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -485,6 +485,30 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms( ] +def test_post_sms_notification_with_archived_reply_to_id_returns_400(client, sample_template, mocker): + archived_sender = create_service_sms_sender( + sample_template.service, + '12345', + is_default=False, + archived=True) + mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + data = { + "phone_number": '+447700900855', + "template_id": sample_template.id, + 'sms_sender_id': archived_sender.id + } + auth_header = create_authorization_header(service_id=sample_template.service_id) + response = client.post( + path="v2/notifications/sms", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 400 + resp_json = json.loads(response.get_data(as_text=True)) + assert 'sms_sender_id {} does not exist in database for service id {}'. \ + format(archived_sender.id, sample_template.service_id) in resp_json['errors'][0]['message'] + 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, 'Cannot send text messages'), ('someone@test.com', 'email_address', sample_template_without_email_permission, 'Cannot send emails')])