Changes after review. Updates to style and import order to be more consistent with other code.

Added clarification to an error message to give better debugging information.
Removed using dao_get_reply_to_by_service_id in tests to be more consistent with other code and use the test db functions or remove the need for a call altogether making the code less complex.

app/dao/notifications_dao.py

@@ -40,7 +40,9 @@ from app.models import (
     KEY_TYPE_NORMAL, KEY_TYPE_TEST,
     LETTER_TYPE,
     NOTIFICATION_SENT,
-    NotificationEmailReplyTo, ServiceEmailReplyTo)
+    NotificationEmailReplyTo,
+    ServiceEmailReplyTo
+)
 
 from app.dao.dao_utils import transactional
 from app.statsd_decorators import statsd
@@ -619,7 +621,7 @@ def dao_get_notification_email_reply_for_notification(notification_id):
         NotificationEmailReplyTo
     ).filter(
         NotificationEmailReplyTo.notification_id == notification_id
-    ).all()
+    ).first()
 
-    if len(email_reply_to) == 1:
+    if email_reply_to:
-        return email_reply_to[0].email_address
+        return email_reply_to.email_address

app/notifications/validators.py

@@ -140,5 +140,6 @@ def check_service_email_reply_to_id(service_id, reply_to_id):
         try:
             reply_to = dao_get_reply_to_by_id(service_id, reply_to_id)
         except NoResultFound:
-            message = 'email_reply_to_id does not exist in database'
+            message = 'email_reply_to_id {} does not exist in database for service id {}'\
+                .format(reply_to_id, service_id)
             raise BadRequestError(message=message)

app/v2/notifications/post_notifications.py

@@ -19,7 +19,9 @@ from app.notifications.process_notifications import (
     persist_notification,
     send_notification_to_queue,
     simulated_recipient,
-    persist_scheduled_notification, persist_email_reply_to_id_for_notification)
+    persist_scheduled_notification,
+    persist_email_reply_to_id_for_notification
+)
 from app.notifications.process_letter_notifications import (
     create_letter_notification
 )

tests/app/dao/notification_dao/test_notification_dao.py

@@ -11,6 +11,7 @@ from app.models import (
     Notification,
     NotificationHistory,
     Job,
+    NotificationEmailReplyTo,
     NotificationStatistics,
     ScheduledNotification,
     NOTIFICATION_STATUS_TYPES,
@@ -20,15 +21,23 @@ from app.models import (
     KEY_TYPE_NORMAL,
     KEY_TYPE_TEAM,
     KEY_TYPE_TEST,
-    NotificationEmailReplyTo)
+)
 
 from app.dao.notifications_dao import (
     dao_create_notification,
+    dao_create_notification_email_reply_to_mapping,
+    dao_created_scheduled_notification,
+    dao_delete_notifications_and_history_by_id,
+    dao_get_notifications_by_to_field,
     dao_get_last_template_usage,
+    dao_get_notification_email_reply_for_notification,
     dao_get_notification_statistics_for_service_and_day,
     dao_get_potential_notification_statistics_for_day,
+    dao_get_scheduled_notifications,
     dao_get_template_usage,
+    dao_timeout_notifications,
     dao_update_notification,
+    dao_update_notifications_for_job_to_sent_to_dvla,
     delete_notifications_created_more_than_a_week_ago_by_type,
     get_notification_by_id,
     get_notification_for_job,
@@ -36,17 +45,11 @@ from app.dao.notifications_dao import (
     get_notifications_for_job,
     get_notifications_for_service,
     get_total_sent_notifications_in_date_range,
-    update_notification_status_by_id,
-    update_notification_status_by_reference,
-    dao_delete_notifications_and_history_by_id,
-    dao_timeout_notifications,
     is_delivery_slow_for_provider,
-    dao_update_notifications_for_job_to_sent_to_dvla,
-    dao_get_notifications_by_to_field,
-    dao_created_scheduled_notification,
-    dao_get_scheduled_notifications,
     set_scheduled_notification_to_processed,
-    dao_create_notification_email_reply_to_mapping, dao_get_notification_email_reply_for_notification)
+    update_notification_status_by_id,
+    update_notification_status_by_reference
+)
 
 from app.dao.services_dao import dao_update_service
 from tests.app.db import create_notification, create_api_key, create_reply_to_email
@@ -1968,15 +1971,12 @@ def test_dao_create_notification_email_reply_to_mapping(sample_service, sample_n
 
 
 def test_dao_create_multiple_notification_email_reply_to_mapping(sample_service, sample_notification):
+    reply_to_address = create_reply_to_email(sample_service, "test@test.com")
 
-    create_reply_to_email(sample_service, "test@test.com")
+    dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address.id)
-
-    reply_to_address = dao_get_reply_to_by_service_id(sample_service.id)
-
-    dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address[0].id)
 
     with pytest.raises(IntegrityError) as e:
-        dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address[0].id)
+        dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address.id)
 
     assert 'duplicate key value' in str(e.value)
 
@@ -1984,15 +1984,14 @@ def test_dao_create_multiple_notification_email_reply_to_mapping(sample_service,
 
     assert len(email_reply_to) == 1
     assert email_reply_to[0].notification_id == sample_notification.id
-    assert email_reply_to[0].service_email_reply_to_id == reply_to_address[0].id
+    assert email_reply_to[0].service_email_reply_to_id == reply_to_address.id
 
 
-def test_dao_get_notification_email_reply_for_notification(sample_service, sample_notification):
+def test_dao_get_notification_ememail_reply_toail_reply_for_notification(sample_service, sample_notification):
-    create_reply_to_email(sample_service, "test@test.com")
+    reply_to_address = create_reply_to_email(sample_service, "test@test.com")
-    reply_to_address = dao_get_reply_to_by_service_id(sample_service.id)
+    dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address.id)
-    dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address[0].id)
     assert dao_get_notification_email_reply_for_notification(sample_notification.id) == "test@test.com"
 
 
-def test_dao_get_notification_email_reply_for_notification_where_no_mapping(fake_uuid):
+def test_dao_get_notification_email_reply_for_notification_where_no_mapping(notify_db_session, fake_uuid):
     assert dao_get_notification_email_reply_for_notification(fake_uuid) is None

tests/app/notifications/test_validators.py

@@ -330,43 +330,23 @@ def test_check_service_email_reply_to_id_where_reply_to_id_is_none():
     assert check_service_email_reply_to_id(None, None) is None
 
 
-def test_check_service_email_reply_to_id_where_reply_to_id_is_not_found(sample_service, fake_uuid):
-    with pytest.raises(BadRequestError) as e:
-        check_service_email_reply_to_id(sample_service.id, fake_uuid)
-    assert e.value.status_code == 400
-    assert e.value.message == 'email_reply_to_id does not exist in database'
-
-
-def test_check_service_email_reply_to_id_where_reply_to_id_is_found(sample_service):
-    reply_to_email = create_reply_to_email(sample_service, 'test@test.com')
-    assert check_service_email_reply_to_id(sample_service.id, reply_to_email.id) is None
-
-
 def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_service, fake_uuid):
+    reply_to_address = create_reply_to_email(sample_service, "test@test.com")
     with pytest.raises(BadRequestError) as e:
-        check_service_email_reply_to_id(fake_uuid, fake_uuid)
+        check_service_email_reply_to_id(fake_uuid, reply_to_address.id)
     assert e.value.status_code == 400
-    assert e.value.message == 'email_reply_to_id does not exist in database'
+    assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}'\
-
+        .format(reply_to_address.id, fake_uuid)
-
-def test_check_service_email_reply_to_id_where_reply_to_id_is_none():
-    assert check_service_email_reply_to_id(None, None) is None
 
 
 def test_check_service_email_reply_to_id_where_reply_to_id_is_not_found(sample_service, fake_uuid):
     with pytest.raises(BadRequestError) as e:
         check_service_email_reply_to_id(sample_service.id, fake_uuid)
     assert e.value.status_code == 400
-    assert e.value.message == 'email_reply_to_id does not exist in database'
+    assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}'\
+        .format(fake_uuid, sample_service.id)
 
 
 def test_check_service_email_reply_to_id_where_reply_to_id_is_found(sample_service):
     reply_to_email = create_reply_to_email(sample_service, 'test@test.com')
     assert check_service_email_reply_to_id(sample_service.id, reply_to_email.id) is None
-
-
-def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_service, fake_uuid):
-    with pytest.raises(BadRequestError) as e:
-        check_service_email_reply_to_id(fake_uuid, fake_uuid)
-    assert e.value.status_code == 400
-    assert e.value.message == 'email_reply_to_id does not exist in database'

tests/app/v2/notifications/test_post_notifications.py

@@ -4,9 +4,13 @@ import pytest
 from freezegun import freeze_time
 
 from app.models import (
-    Notification, ScheduledNotification, SCHEDULE_NOTIFICATIONS,
+    NotificationEmailReplyTo,
-    EMAIL_TYPE, INTERNATIONAL_SMS_TYPE, SMS_TYPE,
+    ScheduledNotification,
-    NotificationEmailReplyTo)
+    SCHEDULE_NOTIFICATIONS,
+    EMAIL_TYPE,
+    INTERNATIONAL_SMS_TYPE,
+    SMS_TYPE
+)
 from flask import json, current_app
 
 from app.models import Notification
@@ -15,8 +19,10 @@ 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 as create_sample_template,
-    sample_template_without_sms_permission, sample_template_without_email_permission
+    sample_service,
+    sample_template_without_email_permission,
+    sample_template_without_sms_permission
 )
 
 from tests.app.db import create_inbound_number, create_service, create_template, create_reply_to_email
@@ -501,7 +507,8 @@ def test_post_sms_notification_with_invalid_reply_to_email_id(
         headers=[('Content-Type', 'application/json'), auth_header])
     assert response.status_code == 400
     resp_json = json.loads(response.get_data(as_text=True))
-    assert 'reply_to_id does not exist in database' in resp_json['errors'][0]['message']
+    assert 'reply_to_id {} does not exist in database for service id {}'.\
+        format(fake_uuid, sample_template_with_placeholders.service_id) in resp_json['errors'][0]['message']
     assert 'BadRequestError' in resp_json['errors'][0]['error']
 
 
@@ -540,7 +547,8 @@ 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 'reply_to_id does not exist in database' in resp_json['errors'][0]['message']
+    assert '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']