make user mobile num nullable if user has email_auth enabled

app/errors.py

@@ -94,7 +94,7 @@ def register_errors(blueprint):
         current_app.logger.exception(e)
         if hasattr(e, 'orig') and hasattr(e.orig, 'pgerror') and e.orig.pgerror and \
             ('duplicate key value violates unique constraint "services_name_key"' in e.orig.pgerror or
-                'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror):
+             'duplicate key value violates unique constraint "services_email_from_key"' in e.orig.pgerror):
             return jsonify(
                 result='error',
                 message={'name': ["Duplicate service name '{}'".format(

app/models.py

@@ -9,7 +9,7 @@ from sqlalchemy.dialects.postgresql import (
     UUID,
     JSON
 )
-from sqlalchemy import UniqueConstraint, and_
+from sqlalchemy import UniqueConstraint, CheckConstraint, and_
 from sqlalchemy.orm import foreign, remote
 from notifications_utils.recipients import (
     validate_email_address,
@@ -97,7 +97,7 @@ class User(db.Model):
         nullable=True,
         onupdate=datetime.datetime.utcnow)
     _password = db.Column(db.String, index=False, unique=False, nullable=False)
-    mobile_number = db.Column(db.String, index=False, unique=False, nullable=False)
+    mobile_number = db.Column(db.String, index=False, unique=False, nullable=True)
     password_changed_at = db.Column(db.DateTime, index=False, unique=False, nullable=False,
                                     default=datetime.datetime.utcnow)
     logged_in_at = db.Column(db.DateTime, nullable=True)
@@ -107,6 +107,9 @@ class User(db.Model):
     current_session_id = db.Column(UUID(as_uuid=True), nullable=True)
     auth_type = db.Column(db.String, db.ForeignKey('auth_type.name'), index=True, nullable=False, default=SMS_AUTH_TYPE)
 
+    # either email auth or a mobile number must be provided
+    CheckConstraint("auth_type = 'email_auth' or mobile_number is not null")
+
     services = db.relationship(
         'Service',
         secondary='user_to_service',
@@ -560,7 +563,7 @@ class Template(db.Model):
         nullable=False,
         default=NORMAL
     )
-
+    
     redact_personalisation = association_proxy('template_redacted', 'redact_personalisation')
 
     def get_link(self):

app/schemas.py

@@ -105,6 +105,26 @@ class UserSchema(BaseSchema):
             "_password", "verify_codes")
         strict = True
 
+    @validates('name')
+    def validate_name(self, value):
+        if not value:
+            raise ValidationError('Invalid name')
+
+    @validates('email_address')
+    def validate_email_address(self, value):
+        try:
+            validate_email_address(value)
+        except InvalidEmailError as e:
+            raise ValidationError(str(e))
+
+    @validates('mobile_number')
+    def validate_mobile_number(self, value):
+        try:
+            if value is not None:
+                validate_phone_number(value, international=True)
+        except InvalidPhoneError as error:
+            raise ValidationError('Invalid phone number: {}'.format(error))
+
 
 class UserUpdateAttributeSchema(BaseSchema):
     auth_type = field_for(models.User, 'auth_type')
@@ -132,7 +152,8 @@ class UserUpdateAttributeSchema(BaseSchema):
     @validates('mobile_number')
     def validate_mobile_number(self, value):
         try:
-            validate_phone_number(value, international=True)
+            if value is not None:
+                validate_phone_number(value, international=True)
         except InvalidPhoneError as error:
             raise ValidationError('Invalid phone number: {}'.format(error))
 

app/user/rest.py

@@ -4,6 +4,7 @@ from datetime import datetime
 from urllib.parse import urlencode
 
 from flask import (jsonify, request, Blueprint, current_app, abort)
+from sqlalchemy.exc import IntegrityError
 
 from app.config import QueueNames
 from app.dao.users_dao import (
@@ -52,6 +53,19 @@ user_blueprint = Blueprint('user', __name__)
 register_errors(user_blueprint)
 
 
+@user_blueprint.errorhandler(IntegrityError)
+def handle_integrity_error(exc):
+    """
+    Handle integrity errors caused by the auth type/mobile number check constraint
+    """
+    if 'ck_users_mobile_or_email_auth' in str(exc):
+        # we don't expect this to trip, so still log error
+        current_app.logger.exception('Check constraint ck_users_mobile_or_email_auth triggered')
+        return jsonify(result='error', message='Mobile number must be set if auth_type is set to sms_auth'), 400
+
+    raise
+
+
 @user_blueprint.route('', methods=['POST'])
 def create_user():
     user_to_create, errors = user_schema.load(request.get_json())

migrations/versions/0136_user_mobile_nullable.py

@@ -0,0 +1,28 @@
+"""
+
+Revision ID: 0136_user_mobile_nullable
+Revises: 0135_stats_template_usage
+Create Date: 2017-11-08 11:49:05.773974
+
+"""
+from alembic import op
+import sqlalchemy as sa
+from sqlalchemy.sql import column
+from sqlalchemy.dialects import postgresql
+
+revision = '0136_user_mobile_nullable'
+down_revision = '0135_stats_template_usage'
+
+
+def upgrade():
+    op.alter_column('users', 'mobile_number', nullable=True)
+
+    op.create_check_constraint(
+        'ck_users_mobile_or_email_auth',
+        'users',
+        "auth_type = 'email_auth' or mobile_number is not null"
+    )
+
+def downgrade():
+    op.alter_column('users', 'mobile_number', nullable=False)
+    op.drop_constraint('ck_users_mobile_or_email_auth', 'users')

tests/app/user/test_rest.py

@@ -159,6 +159,55 @@ def test_create_user_missing_attribute_password(client, notify_db, notify_db_ses
     assert {'password': ['Missing data for required field.']} == json_resp['message']
 
 
+def test_can_create_user_with_email_auth_and_no_mobile(admin_request, notify_db_session):
+    data = {
+        'name': 'Test User',
+        'email_address': 'user@digital.cabinet-office.gov.uk',
+        'password': 'password',
+        'mobile_number': None,
+        'auth_type': EMAIL_AUTH_TYPE
+    }
+
+    json_resp = admin_request.post('user.create_user', _data=data, _expected_status=201)
+
+    assert json_resp['data']['auth_type'] == EMAIL_AUTH_TYPE
+    assert json_resp['data']['mobile_number'] is None
+
+
+def test_cannot_create_user_with_sms_auth_and_no_mobile(admin_request, notify_db_session):
+    data = {
+        'name': 'Test User',
+        'email_address': 'user@digital.cabinet-office.gov.uk',
+        'password': 'password',
+        'mobile_number': None,
+        'auth_type': SMS_AUTH_TYPE
+    }
+
+    json_resp = admin_request.post('user.create_user', _data=data, _expected_status=400)
+
+    assert json_resp['message'] == 'Mobile number must be set if auth_type is set to sms_auth'
+
+
+def test_cannot_create_user_with_empty_strings(admin_request, notify_db_session):
+    data = {
+        'name': '',
+        'email_address': '',
+        'password': 'password',
+        'mobile_number': '',
+        'auth_type': EMAIL_AUTH_TYPE
+    }
+    resp = admin_request.post(
+        'user.create_user',
+        _data=data,
+        _expected_status=400
+    )
+    assert resp['message'] == {
+        'email_address': ['Not a valid email address'],
+        'mobile_number': ['Invalid phone number: Not enough digits'],
+        'name': ['Invalid name']
+    }
+
+
 def test_put_user(client, sample_service):
     """
     Tests PUT endpoint '/' to update a user.
@@ -548,18 +597,6 @@ def test_update_user_resets_failed_login_count_if_updating_password(client, samp
     assert user.failed_login_count == 0
 
 
-def test_update_user_auth_type(admin_request, sample_user):
-    assert sample_user.auth_type == 'sms_auth'
-    resp = admin_request.post(
-        'user.update_user_attribute',
-        user_id=sample_user.id,
-        _data={'auth_type': 'email_auth'},
-    )
-
-    assert resp['data']['id'] == str(sample_user.id)
-    assert resp['data']['auth_type'] == 'email_auth'
-
-
 def test_activate_user(admin_request, sample_user):
     sample_user.state = 'pending'
 
@@ -574,3 +611,68 @@ def test_activate_user_fails_if_already_active(admin_request, sample_user):
     resp = admin_request.post('user.activate_user', user_id=sample_user.id, _expected_status=400)
     assert resp['message'] == 'User already active'
     assert sample_user.state == 'active'
+
+
+def test_update_user_auth_type(admin_request, sample_user):
+    assert sample_user.auth_type == 'sms_auth'
+    resp = admin_request.post(
+        'user.update_user_attribute',
+        user_id=sample_user.id,
+        _data={'auth_type': 'email_auth'},
+    )
+
+    assert resp['data']['id'] == str(sample_user.id)
+    assert resp['data']['auth_type'] == 'email_auth'
+
+
+def test_can_set_email_auth_and_remove_mobile_at_same_time(admin_request, sample_user):
+    sample_user.auth_type = SMS_AUTH_TYPE
+
+    admin_request.post(
+        'user.update_user_attribute',
+        user_id=sample_user.id,
+        _data={
+            'mobile_number': None,
+            'auth_type': EMAIL_AUTH_TYPE,
+        }
+    )
+
+    assert sample_user.mobile_number is None
+    assert sample_user.auth_type == EMAIL_AUTH_TYPE
+
+
+def test_cannot_remove_mobile_if_sms_auth(admin_request, sample_user):
+    sample_user.auth_type = SMS_AUTH_TYPE
+
+    json_resp = admin_request.post(
+        'user.update_user_attribute',
+        user_id=sample_user.id,
+        _data={'mobile_number': None},
+        _expected_status=400
+    )
+
+    assert json_resp['message'] == 'Mobile number must be set if auth_type is set to sms_auth'
+
+
+def test_can_remove_mobile_if_email_auth(admin_request, sample_user):
+    sample_user.auth_type = EMAIL_AUTH_TYPE
+
+    admin_request.post(
+        'user.update_user_attribute',
+        user_id=sample_user.id,
+        _data={'mobile_number': None},
+    )
+
+    assert sample_user.mobile_number is None
+
+
+def test_cannot_update_user_with_mobile_number_as_empty_string(admin_request, sample_user):
+    sample_user.auth_type = EMAIL_AUTH_TYPE
+
+    resp = admin_request.post(
+        'user.update_user_attribute',
+        user_id=sample_user.id,
+        _data={'mobile_number': ''},
+        _expected_status=400
+    )
+    assert resp['message']['mobile_number'] == ['Invalid phone number: Not enough digits']