Some code clean up.

Clean up the unit test.

app/notifications/rest.py

@@ -23,33 +23,17 @@ def get_notifications(notification_id):
     return jsonify(notify_alpha_client.fetch_notification_by_id(notification_id)), 200
 
 
-def _add_notification_to_queue(template_id, service, msg_type, to):
-    q = boto3.resource('sqs', region_name=current_app.config['AWS_REGION']).create_queue(
-        QueueName=str(service.queue_name))
-    import uuid
-    notification = json.dumps({'message_id': str(uuid.uuid4()),
-                               'service_id': service.id,
-                               'to': to,
-                               'message_type': msg_type,
-                               'template_id': template_id})
-    serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY'))
-    encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT'))
-    q.send_message(MessageBody=encrypted,
-                   MessageAttributes={'type': {'StringValue': msg_type, 'DataType': 'String'},
-                                      'service_id': {'StringValue': str(service.id), 'DataType': 'String'},
-                                      'template_id': {'StringValue': str(template_id), 'DataType': 'String'}})
-
-
 @notifications.route('/sms', methods=['POST'])
 def create_sms_notification():
     notification = request.get_json()['notification']
     errors = {}
-    # TODO: should create a different endpoint for the admin client to send verify codes.
-    if api_user['client'] == current_app.config.get('ADMIN_CLIENT_USER_NAME'):
-        to, to_errors = validate_to_for_admin_client(notification)
-        content, content_errors = validate_content_for_admin_client(notification)
+    to, to_errors = validate_to(notification)
     if to_errors['to']:
         errors.update(to_errors)
+
+    # TODO: should create a different endpoint for the admin client to send verify codes.
+    if api_user['client'] == current_app.config.get('ADMIN_CLIENT_USER_NAME'):
+        content, content_errors = validate_content_for_admin_client(notification)
         if content_errors['content']:
             errors.update(content_errors)
         if errors:
@@ -58,10 +42,11 @@ def create_sms_notification():
         return jsonify(notify_alpha_client.send_sms(mobile_number=to, message=content)), 200
 
     else:
-        to, to_errors = validate_to(notification, api_user['client'])
+        to, restricted_errors = validate_to_for_service(notification, api_user['client'])
+        if restricted_errors['restricted']:
+            errors.update(restricted_errors)
+
         template, template_errors = validate_template(notification, api_user['client'])
-        if to_errors['to']:
-            errors.update(to_errors)
         if template_errors['template']:
             errors.update(template_errors)
         if errors:
@@ -92,15 +77,8 @@ def create_email_notification():
         notification['subject']))
 
 
-def validate_to(json_body, service_id):
-    errors = {"to": []}
-    mob = json_body.get('to', None)
-    if not mob:
-        errors['to'].append('Required data missing')
-    else:
-        if not mobile_regex.match(mob):
-            errors['to'].append('invalid phone number, must be of format +441234123123')
-
+def validate_to_for_service(mob, service_id):
+    errors = {"restricted": []}
     service = services_dao.get_model_services(service_id=service_id)
     if service.restricted:
         valid = False
@@ -109,11 +87,11 @@ def validate_to(json_body, service_id):
                 valid = True
                 break
         if not valid:
-                errors['to'].append('Invalid phone number for restricted service')
+            errors['restricted'].append('Invalid phone number for restricted service')
     return mob, errors
 
 
-def validate_to_for_admin_client(json_body):
+def validate_to(json_body):
     errors = {"to": []}
     mob = json_body.get('to', None)
     if not mob:
@@ -154,3 +132,20 @@ def validate_required_and_something(json_body, field):
     if field not in json_body and json_body[field]:
         errors.append('Required data for field.')
     return {field: errors} if errors else None
+
+
+def _add_notification_to_queue(template_id, service, msg_type, to):
+    q = boto3.resource('sqs', region_name=current_app.config['AWS_REGION']).create_queue(
+        QueueName=str(service.queue_name))
+    import uuid
+    notification = json.dumps({'message_id': str(uuid.uuid4()),
+                               'service_id': service.id,
+                               'to': to,
+                               'message_type': msg_type,
+                               'template_id': template_id})
+    serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY'))
+    encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT'))
+    q.send_message(MessageBody=encrypted,
+                   MessageAttributes={'type': {'StringValue': msg_type, 'DataType': 'String'},
+                                      'service_id': {'StringValue': str(service.id), 'DataType': 'String'},
+                                      'template_id': {'StringValue': str(template_id), 'DataType': 'String'}})

tests/app/notifications/test_rest.py

@@ -8,7 +8,7 @@ from app.models import Service
 
 
 def test_get_notifications(
-        notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id, mocker):
+        notify_api, notify_db, notify_db_session, sample_api_key, mocker):
     """
     Tests GET endpoint '/' to retrieve entire service list.
     """
@@ -27,7 +27,7 @@ def test_get_notifications(
             )
 
             auth_header = create_authorization_header(
-                service_id=sample_admin_service_id,
+                service_id=sample_api_key.service_id,
                 path=url_for('notifications.get_notifications', notification_id=123),
                 method='GET')
 
@@ -44,7 +44,7 @@ def test_get_notifications(
 
 
 def test_get_notifications_empty_result(
-        notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id, mocker):
+        notify_api, notify_db, notify_db_session, sample_api_key, mocker):
     """
     Tests GET endpoint '/' to retrieve entire service list.
     """
@@ -59,7 +59,7 @@ def test_get_notifications_empty_result(
             )
 
             auth_header = create_authorization_header(
-                service_id=sample_admin_service_id,
+                service_id=sample_api_key.service_id,
                 path=url_for('notifications.get_notifications', notification_id=123),
                 method='GET')
 
@@ -73,15 +73,13 @@ def test_get_notifications_empty_result(
             notify_alpha_client.fetch_notification_by_id.assert_called_with("123")
 
 
-@moto.mock_sqs
-def test_should_reject_if_no_phone_numbers(
-        notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id, mocker):
+def test_create_sms_should_reject_if_no_phone_numbers(
+        notify_api, notify_db, notify_db_session, sample_api_key, mocker):
     """
     Tests GET endpoint '/' to retrieve entire service list.
     """
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            set_up_mock_queue()
             mocker.patch(
                 'app.notify_alpha_client.send_sms',
                 return_value='success'
@@ -92,7 +90,7 @@ def test_should_reject_if_no_phone_numbers(
                 }
             }
             auth_header = create_authorization_header(
-                service_id=sample_admin_service_id,
+                service_id=sample_api_key.service_id,
                 request_body=json.dumps(data),
                 path=url_for('notifications.create_sms_notification'),
                 method='POST')
@@ -109,15 +107,13 @@ def test_should_reject_if_no_phone_numbers(
             assert not notify_alpha_client.send_sms.called
 
 
-@moto.mock_sqs
 def test_should_reject_bad_phone_numbers(
-        notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id, mocker):
+        notify_api, notify_db, notify_db_session, mocker):
     """
     Tests GET endpoint '/' to retrieve entire service list.
     """
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            set_up_mock_queue()
             mocker.patch(
                 'app.notify_alpha_client.send_sms',
                 return_value='success'
@@ -129,7 +125,6 @@ def test_should_reject_bad_phone_numbers(
                 }
             }
             auth_header = create_authorization_header(
-                service_id=sample_admin_service_id,
                 request_body=json.dumps(data),
                 path=url_for('notifications.create_sms_notification'),
                 method='POST')
@@ -146,15 +141,13 @@ def test_should_reject_bad_phone_numbers(
             assert not notify_alpha_client.send_sms.called
 
 
-@moto.mock_sqs
-def test_should_reject_missing_template(
-        notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id, mocker):
+def test_should_reject_missing_content(
+        notify_api, notify_db, notify_db_session, mocker):
     """
     Tests GET endpoint '/' to retrieve entire service list.
     """
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            set_up_mock_queue()
             mocker.patch(
                 'app.notify_alpha_client.send_sms',
                 return_value='success'
@@ -165,7 +158,6 @@ def test_should_reject_missing_template(
                 }
             }
             auth_header = create_authorization_header(
-                service_id=sample_admin_service_id,
                 request_body=json.dumps(data),
                 path=url_for('notifications.create_sms_notification'),
                 method='POST')
@@ -178,7 +170,7 @@ def test_should_reject_missing_template(
             json_resp = json.loads(response.get_data(as_text=True))
             assert response.status_code == 400
             assert json_resp['result'] == 'error'
-            assert 'Required data missing' in json_resp['message']['template']
+            assert 'Required content' in json_resp['message']['content']
             assert not notify_alpha_client.send_sms.called
 
 
@@ -186,9 +178,6 @@ def test_should_reject_missing_template(
 def test_send_template_content(notify_api,
                                notify_db,
                                notify_db_session,
-                               sample_api_key,
-                               sample_template,
-                               sample_user,
                                mocker):
     """
     Test POST endpoint '/sms' with service notification.
@@ -196,6 +185,8 @@ def test_send_template_content(notify_api,
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             set_up_mock_queue()
+            mobile = '+447719087678'
+            msg = 'Message content'
             mocker.patch(
                 'app.notify_alpha_client.send_sms',
                 return_value={
@@ -203,21 +194,20 @@ def test_send_template_content(notify_api,
                         "createdAt": "2015-11-03T09:37:27.414363Z",
                         "id": 100,
                         "jobId": 65,
-                        "message": sample_template.content,
+                        "message": msg,
                         "method": "sms",
                         "status": "created",
-                        "to": sample_user.mobile_number
+                        "to": mobile
                     }
                 }
             )
             data = {
                 'notification': {
-                    'to': sample_user.mobile_number,
-                    'template': sample_template.id
+                    'to': mobile,
+                    'template': msg
                 }
             }
             auth_header = create_authorization_header(
-                service_id=sample_template.service.id,
                 request_body=json.dumps(data),
                 path=url_for('notifications.create_sms_notification'),
                 method='POST')
@@ -231,11 +221,10 @@ def test_send_template_content(notify_api,
             assert response.status_code == 200
             assert json_resp['notification']['id'] == 100
             notify_alpha_client.send_sms.assert_called_with(
-                mobile_number=sample_user.mobile_number,
-                message=sample_template.content)
+                mobile_number=mobile,
+                message=msg)
 
 
-@moto.mock_sqs
 def test_send_notification_restrict_mobile(notify_api,
                                            notify_db,
                                            notify_db_session,
@@ -249,7 +238,7 @@ def test_send_notification_restrict_mobile(notify_api,
     """
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            set_up_mock_queue()
+
             Service.query.filter_by(
                 id=sample_template.service.id).update({'restricted': True})
             invalid_mob = '+449999999999'
@@ -277,12 +266,12 @@ def test_send_notification_restrict_mobile(notify_api,
 
             json_resp = json.loads(response.get_data(as_text=True))
             assert response.status_code == 400
-            assert 'Invalid phone number for restricted service' in json_resp['message']['to']
+            assert 'Invalid phone number for restricted service' in json_resp['message']['restricted']
+            assert not notify_alpha_client.send_sms.called
 
 
 @moto.mock_sqs
-def test_should_allow_valid_message(
-        notify_api, notify_db, notify_db_session, sample_service, mocker):
+def test_should_allow_valid_message(notify_api, notify_db, notify_db_session, mocker):
     """
     Tests POST endpoint '/sms' with notifications-admin notification.
     """
@@ -296,7 +285,7 @@ def test_should_allow_valid_message(
                         "createdAt": "2015-11-03T09:37:27.414363Z",
                         "id": 100,
                         "jobId": 65,
-                        "message": "This is the message",
+                        "message": "valid",
                         "method": "sms",
                         "status": "created",
                         "to": "+449999999999"
@@ -378,13 +367,6 @@ def test_send_email_valid_data(notify_api,
                 to_address, message, from_address, subject)
 
 
-@moto.mock_sqs
-def test_add_notification_to_queue(notify_api, notify_db, notify_db_session, sample_service):
-    set_up_mock_queue()
-    from app.notifications.rest import _add_notification_to_queue
-    _add_notification_to_queue('some message', sample_service, 'sms', '+447515349060')
-
-
 def set_up_mock_queue():
     # set up mock queue
     boto3.setup_default_session(region_name='eu-west-1')