Merge pull request #84 from alphagov/refactor-user-rest

Refactor user/<user_id>/code into two endpoints.

app/errors.py

@@ -2,6 +2,8 @@ from flask import (
     jsonify,
     current_app
 )
+from sqlalchemy.exc import SQLAlchemyError, DataError
+from sqlalchemy.orm.exc import NoResultFound
 
 
 def register_errors(blueprint):
@@ -32,3 +34,18 @@ def register_errors(blueprint):
     def internal_server_error(e):
         current_app.logger.exception(e)
         return jsonify(error="Internal error"), 500
+
+    @blueprint.app_errorhandler(NoResultFound)
+    def no_result_found(e):
+        current_app.logger.error(e)
+        return jsonify(error="No result found"), 404
+
+    @blueprint.app_errorhandler(DataError)
+    def no_result_found(e):
+        current_app.logger.error(e)
+        return jsonify(error="No result found"), 404
+
+    @blueprint.app_errorhandler(SQLAlchemyError)
+    def db_error(e):
+        current_app.logger.error(e)
+        return jsonify(error="Database error occurred"), 500

app/schemas.py

@@ -62,7 +62,8 @@ class JobSchema(BaseSchema):
         model = models.Job
 
 
-class RequestVerifyCodeSchema(ma.Schema):
+# TODO: Remove this schema once the admin app has stopped using the /user/<user_id>code endpoint
+class OldRequestVerifyCodeSchema(ma.Schema):
 
     code_type = fields.Str(required=True)
     to = fields.Str(required=False)
@@ -73,6 +74,10 @@ class RequestVerifyCodeSchema(ma.Schema):
             raise ValidationError('Invalid code type')
 
 
+class RequestVerifyCodeSchema(ma.Schema):
+    to = fields.Str(required=False)
+
+
 # TODO main purpose to be added later
 # when processing templates, template will be
 # common for all notifications.
@@ -154,6 +159,8 @@ api_keys_schema = ApiKeySchema(many=True)
 job_schema = JobSchema()
 job_schema_load_json = JobSchema(load_json=True)
 jobs_schema = JobSchema(many=True)
+# TODO: Remove this schema once the admin app has stopped using the /user/<user_id>code endpoint
+old_request_verify_code_schema = OldRequestVerifyCodeSchema()
 request_verify_code_schema = RequestVerifyCodeSchema()
 sms_admin_notification_schema = SmsAdminNotificationSchema()
 sms_template_notification_schema = SmsTemplateNotificationSchema()

app/user/rest.py

@@ -1,7 +1,5 @@
 from datetime import datetime
 from flask import (jsonify, request, abort, Blueprint, current_app)
-from sqlalchemy.exc import DataError
-from sqlalchemy.orm.exc import NoResultFound
 
 from app import encryption
 from app.dao.services_dao import get_model_services
@@ -17,7 +15,8 @@ from app.dao.users_dao import (
 )
 from app.schemas import (
     user_schema, users_schema, service_schema, services_schema,
-    request_verify_code_schema, user_schema_load_json)
+    old_request_verify_code_schema, user_schema_load_json,
+    request_verify_code_schema)
 from app.celery.tasks import (send_sms_code, send_email_code)
 
 user = Blueprint('user', __name__)
@@ -42,15 +41,11 @@ def create_user():
 
 @user.route('/<int:user_id>', methods=['PUT', 'DELETE'])
 def update_user(user_id):
-    try:
-        user = get_model_users(user_id=user_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid user id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="User not found"), 404
+    user_to_update = get_model_users(user_id=user_id)
+
     if request.method == 'DELETE':
         status_code = 202
-        delete_model_user(user)
+        delete_model_user(user_to_update)
     else:
         req_json = request.get_json()
         update_dct, errors = user_schema_load_json.load(req_json)
@@ -62,18 +57,14 @@ def update_user(user_id):
         if errors:
             return jsonify(result="error", message=errors), 400
         status_code = 200
-        save_model_user(user, update_dict=update_dct, pwd=pwd)
-    return jsonify(data=user_schema.dump(user).data), status_code
+        save_model_user(user_to_update, update_dict=update_dct, pwd=pwd)
+    return jsonify(data=user_schema.dump(user_to_update).data), status_code
 
 
 @user.route('/<int:user_id>/verify/password', methods=['POST'])
 def verify_user_password(user_id):
-    try:
-        user = get_model_users(user_id=user_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid user id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="User not found"), 404
+    user_to_verify = get_model_users(user_id=user_id)
+
     txt_pwd = None
     try:
         txt_pwd = request.get_json()['password']
@@ -81,22 +72,18 @@ def verify_user_password(user_id):
         return jsonify(
             result="error",
             message={'password': ['Required field missing data']}), 400
-    if user.check_password(txt_pwd):
-        reset_failed_login_count(user)
+    if user_to_verify.check_password(txt_pwd):
+        reset_failed_login_count(user_to_verify)
         return jsonify({}), 204
     else:
-        increment_failed_login_count(user)
+        increment_failed_login_count(user_to_verify)
         return jsonify(result='error', message={'password': ['Incorrect password']}), 400
 
 
 @user.route('/<int:user_id>/verify/code', methods=['POST'])
 def verify_user_code(user_id):
-    try:
-        user = get_model_users(user_id=user_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid user id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="User not found"), 404
+    user_to_verify = get_model_users(user_id=user_id)
+
     txt_code = None
     resp_json = request.get_json()
     txt_type = None
@@ -111,7 +98,7 @@ def verify_user_code(user_id):
         errors.update({'code_type': ['Required field missing data']})
     if errors:
         return jsonify(result="error", message=errors), 400
-    code = get_user_code(user, txt_code, txt_type)
+    code = get_user_code(user_to_verify, txt_code, txt_type)
     if not code:
         return jsonify(result="error", message="Code not found"), 404
     if datetime.now() > code.expiry_datetime or code.code_used:
@@ -120,14 +107,9 @@ def verify_user_code(user_id):
     return jsonify({}), 204
 
 
-@user.route('/<int:user_id>/code', methods=['POST'])
-def send_user_code(user_id):
-    try:
-        user = get_model_users(user_id=user_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid user id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="User not found"), 404
+@user.route('/<int:user_id>/sms-code', methods=['POST'])
+def send_user_sms_code(user_id):
+    user_to_send_to = get_model_users(user_id=user_id)
 
     verify_code, errors = request_verify_code_schema.load(request.get_json())
     if errors:
@@ -135,13 +117,54 @@ def send_user_code(user_id):
 
     from app.dao.users_dao import create_secret_code
     secret_code = create_secret_code()
-    create_user_code(user, secret_code, verify_code.get('code_type'))
+    create_user_code(user_to_send_to, secret_code, 'sms')
+
+    mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to')
+    verification_message = {'to': mobile, 'secret_code': secret_code}
+
+    send_sms_code.apply_async([encryption.encrypt(verification_message)], queue='sms-code')
+
+    return jsonify({}), 204
+
+
+@user.route('/<int:user_id>/email-code', methods=['POST'])
+def send_user_email_code(user_id):
+    user_to_send_to = get_model_users(user_id=user_id)
+
+    verify_code, errors = request_verify_code_schema.load(request.get_json())
+    if errors:
+        return jsonify(result="error", message=errors), 400
+
+    from app.dao.users_dao import create_secret_code
+    secret_code = create_secret_code()
+    create_user_code(user_to_send_to, secret_code, 'email')
+
+    email = user_to_send_to.email_address if verify_code.get('to', None) is None else verify_code.get('to')
+    verification_message = {'to': email, 'secret_code': secret_code}
+
+    send_email_code.apply_async([encryption.encrypt(verification_message)], queue='email-code')
+
+    return jsonify({}), 204
+
+
+# TODO: Remove this method once the admin app has stopped using it.
+@user.route('/<int:user_id>/code', methods=['POST'])
+def send_user_code(user_id):
+    user_to_send_to = get_model_users(user_id=user_id)
+
+    verify_code, errors = old_request_verify_code_schema.load(request.get_json())
+    if errors:
+        return jsonify(result="error", message=errors), 400
+
+    from app.dao.users_dao import create_secret_code
+    secret_code = create_secret_code()
+    create_user_code(user_to_send_to, secret_code, verify_code.get('code_type'))
     if verify_code.get('code_type') == 'sms':
-        mobile = user.mobile_number if verify_code.get('to', None) is None else verify_code.get('to')
+        mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to')
         verification_message = {'to': mobile, 'secret_code': secret_code}
         send_sms_code.apply_async([encryption.encrypt(verification_message)], queue='sms-code')
     elif verify_code.get('code_type') == 'email':
-        email = user.email_address if verify_code.get('to', None) is None else verify_code.get('to')
+        email = user_to_send_to.email_address if verify_code.get('to', None) is None else verify_code.get('to')
         verification_message = {
             'to_address': email,
             'from_address': current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'],
@@ -156,12 +179,8 @@ def send_user_code(user_id):
 @user.route('/<int:user_id>', methods=['GET'])
 @user.route('', methods=['GET'])
 def get_user(user_id=None):
-    try:
-        users = get_model_users(user_id=user_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid user id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="User not found"), 404
+    users = get_model_users(user_id=user_id)
+
     result = users_schema.dump(users) if isinstance(users, list) else user_schema.dump(users)
     return jsonify(data=result.data)
 
@@ -169,18 +188,9 @@ def get_user(user_id=None):
 @user.route('/<int:user_id>/service', methods=['GET'])
 @user.route('/<int:user_id>/service/<service_id>', methods=['GET'])
 def get_service_by_user_id(user_id, service_id=None):
-    try:
-        user = get_model_users(user_id=user_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid user id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="User not found"), 404
+    ret_user = get_model_users(user_id=user_id)
+
+    services = get_model_services(user_id=ret_user.id, service_id=service_id)
 
-    try:
-        services = get_model_services(user_id=user.id, service_id=service_id)
-    except DataError:
-        return jsonify(result="error", message="Invalid service id"), 400
-    except NoResultFound:
-        return jsonify(result="error", message="Service not found"), 404
     services, errors = services_schema.dump(services) if isinstance(services, list) else service_schema.dump(services)
     return jsonify(data=services)

tests/app/user/test_rest.py

@@ -261,7 +261,7 @@ def test_put_user_not_exists(notify_api, notify_db, notify_db_session, sample_us
             assert User.query.count() == 2
             user = User.query.filter_by(id=sample_user.id).first()
             json_resp = json.loads(resp.get_data(as_text=True))
-            assert json_resp == {'result': 'error', 'message': 'User not found'}
+            assert json_resp == {'error': 'No result found'}
             assert user == sample_user
             assert user.email_address != new_email
 
@@ -318,8 +318,7 @@ def test_get_user_service(notify_api, notify_db, notify_db_session, sample_servi
             assert json_resp['data']['id'] == str(another_service.id)
 
 
-def test_get_user_service_user_not_exists(notify_api, notify_db, notify_db_session, sample_service,
-                                          sample_admin_service_id):
+def test_get_user_service_user_not_exists(notify_api, sample_service, sample_admin_service_id):
     """
     Tests GET endpoint "/<user_id>/service/<service_id>" 404 is returned for invalid user.
     """
@@ -330,28 +329,25 @@ def test_get_user_service_user_not_exists(notify_api, notify_db, notify_db_sessi
                                                       path=url_for('user.get_service_by_user_id', user_id="123423",
                                                                    service_id=sample_service.id),
                                                       method='GET')
-            print('** service users{}'.format(sample_service.users[0].id))
             resp = client.get(
                 url_for('user.get_service_by_user_id', user_id="123423", service_id=sample_service.id),
                 headers=[('Content-Type', 'application/json'), auth_header])
             assert resp.status_code == 404
             json_resp = json.loads(resp.get_data(as_text=True))
-            assert "User not found" in json_resp['message']
+            assert json_resp == {'error': 'No result found'}
 
 
-def test_get_user_service_service_not_exists(notify_api, notify_db, notify_db_session, sample_service,
-                                             sample_admin_service_id):
+def test_get_user_service_service_not_exists(notify_api, sample_service):
     """
     Tests GET endpoint "/<user_id>/service/<service_id>" 404 is returned for invalid service.
     """
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             user = User.query.first()
-            assert Service.query.count() == 2
+            assert Service.query.count() == 1
             import uuid
             missing_service_id = uuid.uuid4()
-            auth_header = create_authorization_header(service_id=sample_admin_service_id,
-                                                      path=url_for('user.get_service_by_user_id', user_id=user.id,
+            auth_header = create_authorization_header(path=url_for('user.get_service_by_user_id', user_id=user.id,
                                                                    service_id=missing_service_id),
                                                       method='GET')
             resp = client.get(
@@ -359,10 +355,10 @@ def test_get_user_service_service_not_exists(notify_api, notify_db, notify_db_se
                 headers=[('Content-Type', 'application/json'), auth_header])
             assert resp.status_code == 404
             json_resp = json.loads(resp.get_data(as_text=True))
-            assert "Service not found" in json_resp['message']
+            assert json_resp == {'error': 'No result found'}
 
 
-def test_delete_user(notify_api, notify_db, notify_db_session, sample_user, sample_admin_service_id):
+def test_delete_user(notify_api, sample_user, sample_admin_service_id):
     """
     Tests DELETE endpoint '/<user_id>' delete user.
     """
@@ -407,3 +403,5 @@ def test_delete_user_not_exists(notify_api, notify_db, notify_db_session, sample
                 headers=[('Content-Type', 'application/json'), auth_header])
             assert resp.status_code == 404
             assert User.query.count() == 2
+            json_resp = json.loads(resp.get_data(as_text=True))
+            assert json_resp == {'error': 'No result found'}

tests/app/user/test_rest_verify.py

@@ -2,9 +2,7 @@ import json
 import moto
 from datetime import (datetime, timedelta)
 from flask import url_for
-
 from app.models import (VerifyCode)
-
 import app.celery.tasks
 from app import db, encryption
 from tests import create_authorization_header
@@ -191,7 +189,6 @@ def test_user_verify_password_valid_password_resets_failed_logins(notify_api,
                                                                   notify_db,
                                                                   notify_db_session,
                                                                   sample_user):
-
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             data = json.dumps({'password': 'bad password'})
@@ -249,9 +246,10 @@ def test_user_verify_password_missing_password(notify_api,
             assert 'Required field missing data' in json_resp['message']['password']
 
 
+# TODO: Remove this test once the admin app has stopped using it.
 def test_send_user_code_for_sms(notify_api,
                                 sample_sms_code,
-                                mock_secret_code,
+                                mock_encryption,
                                 mock_celery_send_sms_code):
     """
    Tests POST endpoint '/<user_id>/code' successful sms
@@ -269,35 +267,11 @@ def test_send_user_code_for_sms(notify_api,
                 headers=[('Content-Type', 'application/json'), auth_header])
 
             assert resp.status_code == 204
-            encrpyted = encryption.encrypt({'to': sample_sms_code.user.mobile_number, 'secret_code': '11111'})
-            app.celery.tasks.send_sms_code.apply_async.assert_called_once_with([encrpyted], queue='sms-code')
-
-
-def test_send_user_code_for_sms_with_optional_to_field(notify_api,
-                                                       sample_sms_code,
-                                                       mock_secret_code,
-                                                       mock_celery_send_sms_code):
-    """
-   Tests POST endpoint '/<user_id>/code' successful sms with optional to field
-   """
-    with notify_api.test_request_context():
-        with notify_api.test_client() as client:
-
-            data = json.dumps({'code_type': 'sms', 'to': '+441119876757'})
-            auth_header = create_authorization_header(
-                path=url_for('user.send_user_code', user_id=sample_sms_code.user.id),
-                method='POST',
-                request_body=data)
-            resp = client.post(
-                url_for('user.send_user_code', user_id=sample_sms_code.user.id),
-                data=data,
-                headers=[('Content-Type', 'application/json'), auth_header])
-
-            assert resp.status_code == 204
-            encrypted = encryption.encrypt({'to': '+441119876757', 'secret_code': '11111'})
-            app.celery.tasks.send_sms_code.apply_async.assert_called_once_with([encrypted], queue='sms-code')
+            app.celery.tasks.send_sms_code.apply_async.assert_called_once_with(['something_encrypted'],
+                                                                               queue='sms-code')
 
 
+# TODO: Remove this test once the admin app has stopped using it.
 def test_send_user_code_for_email(notify_api,
                                   sample_email_code,
                                   mock_secret_code,
@@ -323,6 +297,7 @@ def test_send_user_code_for_email(notify_api,
                                                                                  queue='email-code')
 
 
+# TODO: Remove this test once the admin app has stopped using it.
 def test_send_user_code_for_email_uses_optional_to_field(notify_api,
                                                          sample_email_code,
                                                          mock_secret_code,
@@ -348,16 +323,130 @@ def test_send_user_code_for_email_uses_optional_to_field(notify_api,
                                                                                  queue='email-code')
 
 
-def test_request_verify_code_schema_invalid_code_type(notify_api, notify_db, notify_db_session, sample_user):
-    from app.schemas import request_verify_code_schema
+# TODO: Remove this test once the admin app has stopped using it.
+def test_request_verify_code_schema_invalid_code_type(notify_api, sample_user):
+    from app.schemas import old_request_verify_code_schema
     data = json.dumps({'code_type': 'not_sms'})
-    code, error = request_verify_code_schema.loads(data)
+    code, error = old_request_verify_code_schema.loads(data)
     assert error == {'code_type': ['Invalid code type']}
 
 
-def test_request_verify_code_schema_with_to(notify_api, notify_db, notify_db_session, sample_user):
-    from app.schemas import request_verify_code_schema
+# TODO: Remove this method once the admin app has stopped using it.
+def test_request_verify_code_schema_with_to(notify_api, sample_user):
+    from app.schemas import old_request_verify_code_schema
     data = json.dumps({'code_type': 'sms', 'to': 'some@one.gov.uk'})
-    code, error = request_verify_code_schema.loads(data)
+    code, error = old_request_verify_code_schema.loads(data)
     assert code == {'code_type': 'sms', 'to': 'some@one.gov.uk'}
     assert error == {}
+
+
+def test_send_user_sms_code(notify_api,
+                            sample_sms_code,
+                            mock_celery_send_sms_code,
+                            mock_encryption):
+    """
+    Tests POST endpoint /user/<user_id>/sms-code
+    """
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            data = json.dumps({})
+            auth_header = create_authorization_header(
+                path=url_for('user.send_user_sms_code', user_id=sample_sms_code.user.id),
+                method='POST',
+                request_body=data)
+            resp = client.post(
+                url_for('user.send_user_sms_code', user_id=sample_sms_code.user.id),
+                data=data,
+                headers=[('Content-Type', 'application/json'), auth_header])
+            print(resp.get_data(as_text=True))
+            assert resp.status_code == 204
+            app.celery.tasks.send_sms_code.apply_async.assert_called_once_with(['something_encrypted'],
+                                                                               queue='sms-code')
+
+
+def test_send_user_code_for_sms_with_optional_to_field(notify_api,
+                                                       sample_sms_code,
+                                                       mock_secret_code,
+                                                       mock_celery_send_sms_code):
+    """
+   Tests POST endpoint '/<user_id>/code' successful sms with optional to field
+   """
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            data = json.dumps({'to': '+441119876757'})
+            auth_header = create_authorization_header(
+                path=url_for('user.send_user_sms_code', user_id=sample_sms_code.user.id),
+                method='POST',
+                request_body=data)
+            resp = client.post(
+                url_for('user.send_user_sms_code', user_id=sample_sms_code.user.id),
+                data=data,
+                headers=[('Content-Type', 'application/json'), auth_header])
+
+            assert resp.status_code == 204
+            encrypted = encryption.encrypt({'to': '+441119876757', 'secret_code': '11111'})
+            app.celery.tasks.send_sms_code.apply_async.assert_called_once_with([encrypted], queue='sms-code')
+
+
+def test_send_sms_code_returns_404_for_bad_input_data(notify_api, notify_db, notify_db_session):
+    """
+    Tests POST endpoint /user/<user_id>/sms-code return 404 for bad input data
+    """
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            data = json.dumps({})
+            import uuid
+            uuid_ = uuid.uuid4()
+            auth_header = create_authorization_header(
+                path=url_for('user.send_user_sms_code', user_id=uuid_),
+                method='POST',
+                request_body=data)
+            resp = client.post(
+                url_for('user.send_user_sms_code', user_id=uuid_),
+                data=data,
+                headers=[('Content-Type', 'application/json'), auth_header])
+            assert resp.status_code == 404
+            assert json.loads(resp.get_data(as_text=True))['error'] == 'No result found'
+
+
+def test_send_user_email_code(notify_api,
+                              sample_email_code,
+                              mock_celery_send_email_code,
+                              mock_encryption):
+    """
+    Tests POST endpoint /user/<user_id>/email-code
+    """
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            data = json.dumps({})
+            auth_header = create_authorization_header(
+                path=url_for('user.send_user_email_code', user_id=sample_email_code.user.id),
+                method='POST',
+                request_body=data)
+            resp = client.post(
+                url_for('user.send_user_email_code', user_id=sample_email_code.user.id),
+                data=data,
+                headers=[('Content-Type', 'application/json'), auth_header])
+            print(resp.get_data(as_text=True))
+            assert resp.status_code == 204
+            app.celery.tasks.send_email_code.apply_async.assert_called_once_with(['something_encrypted'],
+                                                                                 queue='email-code')
+
+
+def test_send_user_email_code_returns_404_for_when_user_does_not_exist(notify_api, notify_db, notify_db_session):
+    """
+    Tests POST endpoint /user/<user_id>/email-code return 404 for missing user
+    """
+    with notify_api.test_request_context():
+        with notify_api.test_client() as client:
+            data = json.dumps({})
+            auth_header = create_authorization_header(
+                path=url_for('user.send_user_email_code', user_id=1),
+                method='POST',
+                request_body=data)
+            resp = client.post(
+                url_for('user.send_user_email_code', user_id=1),
+                data=data,
+                headers=[('Content-Type', 'application/json'), auth_header])
+            assert resp.status_code == 404
+            assert json.loads(resp.get_data(as_text=True))['error'] == 'No result found'