Merge pull request #134 from alphagov/share-phone-validation

Moved mobile validation to utils module for use in csv upload as well.

app/main/forms.py

@@ -12,7 +12,12 @@ from wtforms import (
 from wtforms.validators import DataRequired, Email, Length, Regexp
 
 from app.main.validators import Blacklist, CsvFileValidator
-from app.main.encryption import check_hash
+
+from app.main.utils import (
+    validate_phone_number,
+    format_phone_number,
+    InvalidPhoneError
+)
 
 
 def email_address():
@@ -28,33 +33,10 @@ def email_address():
 class UKMobileNumber(StringField):
 
     def pre_validate(self, form):
-
-        self.data = self.data.replace('(', '')
-        self.data = self.data.replace(')', '')
-        self.data = self.data.replace(' ', '')
-        self.data = self.data.replace('-', '')
-
-        if self.data.startswith('+'):
-            self.data = self.data[1:]
-
-        if not sum(
-            self.data.startswith(prefix) for prefix in ['07', '447', '4407', '00447']
-        ):
-            raise ValidationError('Must be a UK mobile number (eg 07700 900460)')
-
-        for digit in self.data:
-            try:
-                int(digit)
-            except(ValueError):
-                raise ValidationError('Must not contain letters or symbols')
-
-        self.data = self.data.split('7', 1)[1]
-
-        if len(self.data) > 9:
-            raise ValidationError('Too many digits')
-
-        if len(self.data) < 9:
-            raise ValidationError('Not enough digits')
+        try:
+            self.data = validate_phone_number(self.data)
+        except InvalidPhoneError as e:
+            raise ValidationError(e.message)
 
     def post_validate(self, form, validation_stopped):
 
@@ -63,7 +45,7 @@ class UKMobileNumber(StringField):
         # TODO implement in the render field method.
         # API's require no spaces in the number
         # self.data = '+44 7{} {} {}'.format(*re.findall('...', self.data))
-        self.data = '+447{}{}{}'.format(*re.findall('...', self.data))
+        self.data = format_phone_number(self.data)
 
 
 def mobile_number():

app/main/utils.py

@@ -24,3 +24,50 @@ class BrowsableItem(object):
     @property
     def destructive(self):
         pass
+
+
+class InvalidPhoneError(Exception):
+    def __init__(self, message):
+        self.message = message
+
+
+def validate_phone_number(number):
+
+    sanitised_number = number.replace('(', '')
+    sanitised_number = sanitised_number.replace(')', '')
+    sanitised_number = sanitised_number.replace(' ', '')
+    sanitised_number = sanitised_number.replace('-', '')
+
+    if sanitised_number.startswith('+'):
+        sanitised_number = sanitised_number[1:]
+
+    valid_prefixes = ['07', '447', '4407', '00447']
+    if not sum(sanitised_number.startswith(prefix) for prefix in valid_prefixes):
+        raise InvalidPhoneError('Must be a UK mobile number (eg 07700 900460)')
+
+    for digit in sanitised_number:
+        try:
+            int(digit)
+        except(ValueError):
+            raise InvalidPhoneError('Must not contain letters or symbols')
+
+    # Split number on first 7
+    sanitised_number = sanitised_number.split('7', 1)[1]
+
+    if len(sanitised_number) > 9:
+        raise InvalidPhoneError('Too many digits')
+
+    if len(sanitised_number) < 9:
+        raise InvalidPhoneError('Not enough digits')
+
+    return sanitised_number
+
+
+def format_phone_number(number):
+    import re
+    if len(number) > 9:
+        raise InvalidPhoneError('Too many digits')
+
+    if len(number) < 9:
+        raise InvalidPhoneError('Not enough digits')
+    return '+447{}{}{}'.format(*re.findall('...', number))

app/main/views/sms.py

@@ -1,5 +1,4 @@
 import csv
-import re
 import uuid
 
 from datetime import date
@@ -26,6 +25,10 @@ from app.main.uploader import (
 )
 from app.main.dao import templates_dao
 from app import job_api_client
+from app.main.utils import (
+    validate_phone_number,
+    InvalidPhoneError
+)
 
 
 @main.route("/services/<int:service_id>/sms/send", methods=['GET', 'POST'])
@@ -116,15 +119,15 @@ def _format_filename(filename):
 
 
 def _get_numbers(contents):
-    pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$')  # TODO need better validation
     reader = csv.DictReader(
         contents.split('\n'),
         lineterminator='\n',
         quoting=csv.QUOTE_NONE)
     valid, rejects = [], []
     for i, row in enumerate(reader):
-        if pattern.match(row['phone']):
+        try:
+            validate_phone_number(row['phone'])
             valid.append(row)
-        else:
+        except InvalidPhoneError:
             rejects.append({"line_number": i+2, "phone": row['phone']})
     return {"valid": valid, "rejects": rejects}

tests/app/main/test_phone_validation.py

@@ -0,0 +1,68 @@
+from app.main.utils import (
+    validate_phone_number,
+    InvalidPhoneError
+)
+
+import pytest
+
+valid_phone_numbers = [
+    '07123456789',
+    '07123 456789',
+    '07123-456-789',
+    '00447123456789',
+    '00 44 7123456789',
+    '+447123456789',
+    '+44 7123 456 789',
+    '+44 (0)7123 456 789'
+]
+
+invalid_phone_numbers = sum([
+    [
+        (phone_number, error) for phone_number in group
+    ] for error, group in [
+        ('Too many digits', (
+            '0712345678910',
+            '0044712345678910',
+            '0044712345678910',
+            '+44 (0)7123 456 789 10',
+        )),
+        ('Not enough digits', (
+            '0712345678',
+            '004471234567',
+            '00447123456',
+            '+44 (0)7123 456 78',
+        )),
+        ('Must be a UK mobile number (eg 07700 900460)', (
+            '08081 570364',
+            '+44 8081 570364',
+            '0117 496 0860',
+            '+44 117 496 0860',
+            '020 7946 0991',
+            '+44 20 7946 0991',
+            '71234567890',
+        )),
+        ('Must not contain letters or symbols', (
+            '07890x32109',
+            '07123 456789...',
+            '07123 ☟☜⬇⬆☞☝',
+            '07123☟☜⬇⬆☞☝',
+            '07";DROP TABLE;"',
+            '+44 07ab cde fgh',
+        ))
+    ]
+], [])
+
+
+@pytest.mark.parametrize("phone_number", valid_phone_numbers)
+def test_phone_number_accepts_valid_values(phone_number):
+    try:
+        validate_phone_number(phone_number)
+    except InvalidPhoneError:
+        pytest.fail('Unexpected InvalidPhoneError')
+
+
+@pytest.mark.parametrize("phone_number, error_message", invalid_phone_numbers)
+def test_phone_number_rejects_invalid_values(phone_number, error_message):
+    with pytest.raises(InvalidPhoneError) as e:
+        validate_phone_number(phone_number)
+    assert error_message == str(e.value)