Merge branch 'add-reply-to-notifications' of github.com:alphagov/notifications-api into add-reply-to-notifications

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:

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 = {

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)
+    reply_to = get_reply_to_text(notification_type, form)
-    sms_sender = check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type)
 
     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

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)

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',
+    assert {'error': 'ValidationError',
-                                     'message': "template_id is a required property"
+            '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,
+        client,
-    notify_db,
+        notify_db,
-    notify_db_session,
+        notify_db_session,
-    mocker,
+        mocker,
-    notification_type,
+        notification_type,
-    key_send_to,
+        key_send_to,
-    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,
+        client,
-    notify_db,
+        notify_db,
-    notify_db_session,
+        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', [
+@pytest.mark.parametrize('recipient,label,template_factory,expected_error', [
-    (sample_template_without_sms_permission, 'Cannot send text messages'),
+    ('07700 900000', 'phone_number', sample_template_without_sms_permission, 'Cannot send text messages'),
-    (sample_template_without_email_permission, 'Cannot send emails')])
+    ('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',
+        label: recipient,
-        'email_address': 'someone@test.com',
         '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_service,
-    sample_template,
+        sample_template,
-    client,
+        client,
-    mocker,
+        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'.\
+    assert 'Additional properties are not allowed ({} was unexpected)'.format(form_label) \
-        format(form_label, notification_type) in resp_json['errors'][0]['message']
+           in resp_json['errors'][0]['message']
-    assert 'BadRequestError' in resp_json['errors'][0]['error']
+    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']