make persist_notification require kwargs

when functions get as big as that, it's confusing to try and work out what
things are what. By including a * as the first arg, we require that anyone
calling the function has to use kwargs to reference the parameters

app/notifications/process_notifications.py

@@ -35,6 +35,7 @@ def check_placeholders(template_object):
 
 
 def persist_notification(
+    *,
     template_id,
     template_version,
     recipient,

app/notifications/validators.py

@@ -9,7 +9,7 @@ from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_
 
 from app.dao import services_dao, templates_dao
 from app.models import (
-    INTERNATIONAL_SMS_TYPE, SMS_TYPE,
+    INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE,
     KEY_TYPE_TEST, KEY_TYPE_TEAM, SCHEDULE_NOTIFICATIONS
 )
 from app.service.utils import service_allowed_to_send_to
@@ -104,7 +104,7 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type)
             number=send_to,
             international=international_phone_info.international
         )
-    else:
+    elif notification_type == EMAIL_TYPE:
         return validate_and_format_email_address(email_address=send_to)
 
 

tests/app/notifications/test_process_notification.py

@@ -51,10 +51,18 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api
 
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
-    notification = persist_notification(sample_template.id, sample_template.version, '+447111111111',
+    notification = persist_notification(
-                                        sample_template.service, {}, 'sms', sample_api_key.id,
+        template_id=sample_template.id,
-                                        sample_api_key.key_type, job_id=sample_job.id,
+        template_version=sample_template.version,
-                                        job_row_number=100, reference="ref")
+        recipient='+447111111111',
+        service=sample_template.service,
+        personalisation={},
+        notification_type='sms',
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
+        job_id=sample_job.id,
+        job_row_number=100,
+        reference="ref")
 
     assert Notification.query.get(notification.id) is not None
     assert NotificationHistory.query.get(notification.id) is not None
@@ -127,14 +135,14 @@ def test_persist_notification_does_not_increment_cache_if_test_key(
     assert Notification.query.count() == 0
     assert NotificationHistory.query.count() == 0
     persist_notification(
-        sample_template.id,
+        template_id=sample_template.id,
-        sample_template.version,
+        template_version=sample_template.version,
-        '+447111111111',
+        recipient='+447111111111',
-        sample_template.service,
+        service=sample_template.service,
-        {},
+        personalisation={},
-        'sms',
+        notification_type='sms',
-        api_key.id,
+        api_key_id=api_key.id,
-        api_key.key_type,
+        key_type=api_key.key_type,
         job_id=sample_job.id,
         job_row_number=100,
         reference="ref",
@@ -193,18 +201,33 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa
     mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr')
     mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value')
 
-    persist_notification(sample_template.id, sample_template.version, '+447111111111',
+    persist_notification(
-                         sample_template.service, {}, 'sms', sample_api_key.id,
+        template_id=sample_template.id,
-                         sample_api_key.key_type, reference="ref")
+        template_version=sample_template.version,
+        recipient='+447111111111',
+        service=sample_template.service,
+        personalisation={},
+        notification_type='sms',
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
+        reference="ref"
+    )
     mock_incr.assert_not_called()
     mock_incr_hash_value.assert_not_called()
 
     mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1)
     mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash',
                  return_value={sample_template.id, 1})
-    persist_notification(sample_template.id, sample_template.version, '+447111111122',
+    persist_notification(
-                         sample_template.service, {}, 'sms', sample_api_key.id,
+        template_id=sample_template.id,
-                         sample_api_key.key_type, reference="ref2")
+        template_version=sample_template.version,
+        recipient='+447111111122',
+        service=sample_template.service,
+        personalisation={},
+        notification_type='sms',
+        api_key_id=sample_api_key.id,
+        key_type=sample_api_key.key_type,
+        reference="ref2")
     mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", )
     mock_incr_hash_value.assert_called_once_with(cache_key_for_service_template_counter(sample_template.service_id),
                                                  sample_template.id)

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -10,14 +10,17 @@ from tests import create_authorization_header
 from tests.app.db import create_service, create_template
 
 
+pytestmark = pytest.mark.skip('Leters not currently implemented')
+
+
 def letter_request(client, data, service_id, _expected_status=201):
     resp = client.post(
         url_for('v2_notifications.post_notification', notification_type='letter'),
         data=json.dumps(data),
         headers=[('Content-Type', 'application/json'), create_authorization_header(service_id=service_id)]
     )
-    assert resp.status_code == _expected_status
     json_resp = json.loads(resp.get_data(as_text=True))
+    assert resp.status_code == _expected_status, json_resp
     return json_resp
 
 
@@ -76,7 +79,6 @@ def test_post_letter_notification_returns_400_and_missing_template(
     assert error_json['errors'] == [{'error': 'BadRequestError', 'message': 'Template not found'}]
 
 
-
 def test_post_notification_returns_403_and_well_formed_auth_error(
     client,
     sample_letter_template
@@ -139,11 +141,16 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
     assert not persist_mock.called
 
 
+@pytest.mark.parametrize('service_args', [
+    {'service_permissions': [EMAIL_TYPE, SMS_TYPE]},
+    {'restricted': True}
+])
 def test_post_letter_notification_returns_400_if_not_allowed_to_send_notification(
     client,
-    notify_db_session
+    notify_db_session,
+    service_args
 ):
-    service = create_service(service_permissions=[EMAIL_TYPE, SMS_TYPE])
+    service = create_service(**service_args)
     template = create_template(service, template_type=LETTER_TYPE)
 
     data = {
@@ -154,5 +161,5 @@ def test_post_letter_notification_returns_400_if_not_allowed_to_send_notificatio
     error_json = letter_request(client, data, service_id=service.id, _expected_status=400)
     assert error_json['status_code'] == 400
     assert error_json['errors'] == [
-        {'error': 'BadRequestError', 'message': 'Cannot send text letters'}
+        {'error': 'BadRequestError', 'message': 'Cannot send letters'}
     ]

tests/app/v2/notifications/test_post_notifications.py

@@ -433,3 +433,17 @@ def test_post_notification_raises_bad_request_if_service_not_invited_to_schedule
     error_json = json.loads(response.get_data(as_text=True))
     assert error_json['errors'] == [
         {"error": "BadRequestError", "message": 'Cannot schedule notifications (this feature is invite-only)'}]
+
+
+def test_post_notification_raises_bad_request_if_not_valid_notification_type(client, sample_service):
+    auth_header = create_authorization_header(service_id=sample_service.id)
+    response = client.post(
+        '/v2/notifications/foo',
+        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['errors'] == [
+        {'error': 'BadRequestError', 'message': 'Unknown notification type foo'}
+    ]