Fix handling of invalid values in form numerical inputs

app/forms/fields/decimal_field_with_separator.py

@@ -1,9 +1,8 @@
 from decimal import Decimal, InvalidOperation
 
-from babel import numbers
 from wtforms import DecimalField
 
-from app.settings import DEFAULT_LOCALE
+from app.helpers.form_helpers import sanitise_number
 
 
 class DecimalFieldWithSeparator(DecimalField):
@@ -23,8 +22,6 @@ def __init__(self, **kwargs):
     def process_formdata(self, valuelist):
         if valuelist:
             try:
-                self.data = Decimal(
-                    valuelist[0].replace(numbers.get_group_symbol(DEFAULT_LOCALE), "")
-                )
+                self.data = Decimal(sanitise_number(valuelist[0]))
             except (ValueError, TypeError, InvalidOperation):
                 pass

app/forms/fields/integer_field_with_separator.py

@@ -1,7 +1,6 @@
-from babel import numbers
 from wtforms import IntegerField
 
-from app.settings import DEFAULT_LOCALE
+from app.helpers.form_helpers import sanitise_number
 
 
 class IntegerFieldWithSeparator(IntegerField):
@@ -21,8 +20,6 @@ def __init__(self, **kwargs):
     def process_formdata(self, valuelist):
         if valuelist:
             try:
-                self.data = int(
-                    valuelist[0].replace(numbers.get_group_symbol(DEFAULT_LOCALE), "")
-                )
+                self.data = int(sanitise_number(valuelist[0]))
             except ValueError:
                 pass

app/forms/validators.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+import math
 import re
 from datetime import datetime, timezone
 from decimal import Decimal, InvalidOperation
@@ -19,10 +20,14 @@
     DecimalFieldWithSeparator,
     IntegerFieldWithSeparator,
 )
-from app.jinja_filters import format_number, get_formatted_currency
+from app.helpers.form_helpers import (
+    format_message_with_title,
+    format_playback_value,
+    sanitise_mobile_number,
+    sanitise_number,
+)
 from app.questionnaire.questionnaire_store_updater import QuestionnaireStoreUpdater
 from app.questionnaire.rules.utils import parse_datetime
-from app.utilities import safe_content
 
 if TYPE_CHECKING:
     from app.forms.questionnaire_form import QuestionnaireForm  # pragma: no cover
@@ -49,15 +54,15 @@ def __call__(
         field: Union[DecimalFieldWithSeparator, IntegerFieldWithSeparator],
     ) -> None:
         try:
-            Decimal(
-                field.raw_data[0].replace(
-                    numbers.get_group_symbol(flask_babel.get_locale()), ""
-                )
-            )
+            Decimal(sanitise_number(number=field.raw_data[0]))
         except (ValueError, TypeError, InvalidOperation, AttributeError) as exc:
             raise validators.StopValidation(self.message) from exc
 
-        if "e" in field.raw_data[0].lower():
+        if "e" in field.raw_data[0].lower() or math.isnan(
+            Decimal(
+                sanitise_number(number=field.raw_data[0])
+            )  # number is sanitised to guard against inputs like `,NaN_` etc
+        ):
             raise validators.StopValidation(self.message)
 
 
@@ -126,6 +131,7 @@ def __call__(
         field: Union[DecimalFieldWithSeparator, IntegerFieldWithSeparator],
     ) -> None:
         value: Union[int, Decimal] = field.data
+
         if value is not None:
             error_message = self.validate_minimum(value) or self.validate_maximum(value)
             if error_message:
@@ -179,11 +185,7 @@ def __init__(self, max_decimals: int = 0, messages: OptionalMessage = None):
     def __call__(
         self, form: "QuestionnaireForm", field: DecimalFieldWithSeparator
     ) -> None:
-        data = (
-            field.raw_data[0]
-            .replace(numbers.get_group_symbol(flask_babel.get_locale()), "")
-            .replace(" ", "")
-        )
+        data = sanitise_number(field.raw_data[0])
         decimal_symbol = numbers.get_decimal_symbol(flask_babel.get_locale())
         if data and decimal_symbol in data:
             if self.max_decimals == 0:
@@ -450,20 +452,6 @@ def _is_valid(
         raise NotImplementedError(f"Condition '{condition}' is not implemented")
 
 
-def format_playback_value(
-    value: Union[float, Decimal], currency: Optional[str] = None
-) -> str:
-    if currency:
-        return get_formatted_currency(value, currency)
-
-    formatted_number: str = format_number(value)
-    return formatted_number
-
-
-def format_message_with_title(error_message: str, question_title: str) -> str:
-    return error_message % {"question_title": safe_content(question_title)}
-
-
 class MutuallyExclusiveCheck:
     def __init__(self, question_title: str, messages: OptionalMessage = None):
         self.messages = {**error_messages, **(messages or {})}
@@ -492,11 +480,6 @@ def __call__(
             raise validators.ValidationError(message)
 
 
-def sanitise_mobile_number(data: str) -> str:
-    data = re.sub(r"[\s.,\t\-{}\[\]()/]", "", data)
-    return re.sub(r"^(0{1,2}44|\+44|0)", "", data)
-
-
 class MobileNumberCheck:
     def __init__(self, message: OptionalMessage = None):
         self.message = message or error_messages["INVALID_MOBILE_NUMBER"]

app/helpers/form_helpers.py

@@ -0,0 +1,33 @@
+import re
+from decimal import Decimal
+
+import flask_babel
+from babel import numbers
+
+from app.jinja_filters import format_number, get_formatted_currency
+from app.utilities import safe_content
+
+
+def sanitise_number(number: str) -> str:
+    return (
+        number.replace(numbers.get_group_symbol(flask_babel.get_locale()), "")
+        .replace("_", "")
+        .replace(" ", "")
+    )
+
+
+def sanitise_mobile_number(data: str) -> str:
+    data = re.sub(r"[\s.,\t\-{}\[\]()/]", "", data)
+    return re.sub(r"^(0{1,2}44|\+44|0)", "", data)
+
+
+def format_playback_value(value: float | Decimal, currency: str | None = None) -> str:
+    if currency:
+        return get_formatted_currency(value, currency)
+
+    formatted_number: str = format_number(value)
+    return formatted_number
+
+
+def format_message_with_title(error_message: str, question_title: str) -> str:
+    return error_message % {"question_title": safe_content(question_title)}

tests/app/forms/test_custom_fields.py

@@ -1,3 +1,5 @@
+from decimal import Decimal
+
 import pytest
 from wtforms.fields import Field
 
@@ -21,6 +23,7 @@ def test_text_area_supports_maxlength_property(mock_form):
     assert text_area.maxlength == 20
 
 
+@pytest.mark.usefixtures("gb_locale")
 def test_integer_field(mock_form):
     integer_field = IntegerFieldWithSeparator(_form=mock_form, name="aName")
     assert isinstance(integer_field, Field)
@@ -31,6 +34,44 @@ def test_integer_field(mock_form):
         pytest.fail("Exceptions should not thrown by CustomIntegerField")
 
 
+@pytest.mark.usefixtures("gb_locale")
+@pytest.mark.parametrize(
+    "number_input, result",
+    [
+        ("_110", 110),
+        ("1_10", 110),
+        ("1__10", 110),
+        ("1_1,0", 110),
+        ("_1_1,0,0", 1100),
+        ("1.10", None),
+    ],
+)
+def test_integer_field_inputs(mock_form, number_input, result):
+    integer_field = IntegerFieldWithSeparator(_form=mock_form, name="aName")
+    integer_field.process_formdata([number_input])
+
+    assert integer_field.data == result
+
+
+@pytest.mark.usefixtures("gb_locale")
+@pytest.mark.parametrize(
+    "number_input, result",
+    [
+        ("1_1,0", Decimal("110")),
+        ("1.10", Decimal("1.1")),
+        ("_1.1_0", Decimal("1.1")),
+        ("_1.1_0,0", Decimal("1.1")),
+        ("_1._1,0,0", Decimal("1.1")),
+    ],
+)
+def test_decimal_field_inputs(mock_form, number_input, result):
+    decimal_field = DecimalFieldWithSeparator(_form=mock_form, name="aName")
+    decimal_field.process_formdata([number_input])
+
+    assert decimal_field.data == result
+
+
+@pytest.mark.usefixtures("gb_locale")
 def test_decimal_field(mock_form):
     decimal_field = DecimalFieldWithSeparator(_form=mock_form, name="aName")
     assert isinstance(decimal_field, Field)

tests/app/forms/validation/test_number_validator.py

@@ -1,3 +1,5 @@
+from decimal import Decimal
+
 import pytest
 from wtforms.validators import StopValidation, ValidationError
 
@@ -7,12 +9,7 @@
 
 @pytest.mark.parametrize(
     "value",
-    (
-        [None],
-        [""],
-        ["a"],
-        ["2E2"],
-    ),
+    ([None], [""], ["a"], ["2E2"], ["NaN"], [",NaN_"]),
 )
 @pytest.mark.usefixtures("gb_locale")
 def test_number_validator_raises_StopValidation(
@@ -49,12 +46,13 @@ def test_decimal_validator_raises_StopValidation(
 @pytest.mark.parametrize(
     "value",
     (
-        ["0"],
-        ["10"],
-        ["-10"],
+        "0",
+        "10",
+        "-10",
     ),
 )
 @pytest.mark.usefixtures("gb_locale")
 def test_number_validator(number_check, value, mock_form, mock_field):
-    mock_field.raw_data = value
+    mock_field.raw_data = [value]
+    mock_field.data = Decimal(value)
     number_check(mock_form, mock_field)

tests/functional/spec/error_messages.spec.js

@@ -1,4 +1,5 @@
 import AboutYou from "../generated_pages/multiple_answers/about-you-block.page";
+import BlockPage from "../generated_pages/percentage/block.page";
 
 async function answerAllButOne() {
   await $(AboutYou.textfield()).setValue("John Doe");
@@ -135,3 +136,20 @@ describe("Error Messages", () => {
     await expect(await $(AboutYou.textarea()).isFocused()).to.be.true;
   });
 });
+describe("Error Message NaN value", () => {
+  beforeEach(async () => {
+    await browser.openQuestionnaire("test_percentage.json");
+  });
+  it("Given a NaN value was entered on percentage question, When the error is displayed, Then the error message is correct", async () => {
+    await $(BlockPage.answer()).setValue("NaN");
+    await $(BlockPage.submit()).click();
+    await expect(await $(BlockPage.errorHeader()).getText()).to.equal("There is a problem with your answer");
+    await expect(await $(BlockPage.answerErrorItem()).getText()).to.equal("Enter a number");
+  });
+  it("Given a NaN value with separators was entered on percentage question, When the error is displayed, Then the error message is correct", async () => {
+    await $(BlockPage.answer()).setValue(",NaN_");
+    await $(BlockPage.submit()).click();
+    await expect(await $(BlockPage.errorHeader()).getText()).to.equal("There is a problem with your answer");
+    await expect(await $(BlockPage.answerErrorItem()).getText()).to.equal("Enter a number");
+  });
+});