diff --git a/app/validators/questions/__init__.py b/app/validators/questions/__init__.py index 5627387d..45149946 100644 --- a/app/validators/questions/__init__.py +++ b/app/validators/questions/__init__.py @@ -10,9 +10,13 @@ from app.validators.questions.question_validator import QuestionValidator -def get_question_validator(question): +def get_question_validator(question, schema): + if question["type"] == "Calculated": + validators = { + "Calculated": CalculatedQuestionValidator, + } + return validators.get(question["type"], QuestionValidator)(question, schema) validators = { - "Calculated": CalculatedQuestionValidator, "DateRange": DateRangeQuestionValidator, "MutuallyExclusive": MutuallyExclusiveQuestionValidator, } diff --git a/app/validators/questions/calculated_question_validator.py b/app/validators/questions/calculated_question_validator.py index 7afbc125..3705bc65 100644 --- a/app/validators/questions/calculated_question_validator.py +++ b/app/validators/questions/calculated_question_validator.py @@ -1,12 +1,15 @@ from app.validators.questions.question_validator import QuestionValidator +from app.validators.routing.types import ANSWER_TYPE_TO_JSON_TYPE, TYPE_NUMBER class CalculatedQuestionValidator(QuestionValidator): ANSWER_NOT_IN_QUESTION = "Answer does not exist within this question" + ANSWER_TYPE_FOR_CALCULATION_TYPE_INVALID = "Expected the answer type for calculation to be type 'number' but got type '{answer_type}'" def validate(self): super().validate() self.validate_calculations() + self.validate_calculations_value_source_is_numeric() return self.errors def validate_calculations(self): @@ -19,3 +22,31 @@ def validate_calculations(self): for answer_id in calculation["answers_to_calculate"]: if answer_id not in answer_ids: self.add_error(self.ANSWER_NOT_IN_QUESTION, answer_id=answer_id) + + def _validate_answer_is_numeric(self, answer_id): + answer_type = self.schema.get_answer_type(answer_id) + + if ANSWER_TYPE_TO_JSON_TYPE[answer_type.value] != TYPE_NUMBER: + self.add_error( + self.ANSWER_TYPE_FOR_CALCULATION_TYPE_INVALID.format( + answer_type=ANSWER_TYPE_TO_JSON_TYPE[answer_type.value], + ), + referenced_answer=answer_id, + ) + + def validate_calculations_value_source_is_numeric(self): + """ + Validates that source answer is of number type + """ + for calculation in self.question.get("calculations"): + value = calculation.get("value") + + if answer_id := calculation.get("answer_id"): + self._validate_answer_is_numeric(answer_id) + + elif value and value.get("source"): + answer_id = value.get("identifier") + # Calculated summary value source is validated elsewhere and must be of a number type + + if value.get("source") == "answers": + self._validate_answer_is_numeric(answer_id) diff --git a/app/validators/questions/question_validator.py b/app/validators/questions/question_validator.py index 2d118597..f9235079 100644 --- a/app/validators/questions/question_validator.py +++ b/app/validators/questions/question_validator.py @@ -7,11 +7,12 @@ class QuestionValidator(Validator): ) question = {} - def __init__(self, schema_element): + def __init__(self, schema_element, schema=None): super().__init__(schema_element) self.question = schema_element self.answers = self.question.get("answers", []) self.context["question_id"] = schema_element["id"] + self.schema = schema def validate(self): if self.question["type"] != "MutuallyExclusive": diff --git a/app/validators/sections/section_validator.py b/app/validators/sections/section_validator.py index 62d45e6d..e57b8727 100644 --- a/app/validators/sections/section_validator.py +++ b/app/validators/sections/section_validator.py @@ -134,7 +134,9 @@ def validate_question(self, block_or_variant): question = block_or_variant.get("question") if question: - question_validator = get_question_validator(question) + question_validator = get_question_validator( + question, self.questionnaire_schema + ) self.errors += question_validator.validate() diff --git a/schemas/questions/types/calculated.json b/schemas/questions/types/calculated.json index a0ef1637..997d6ddb 100644 --- a/schemas/questions/types/calculated.json +++ b/schemas/questions/types/calculated.json @@ -43,8 +43,16 @@ "description": "The id of an answer from which to obtain the total to validate against" }, "value": { - "type": "integer", - "description": "A hard coded total to validate against" + "oneOf": [ + { + "type": "number", + "description": "A hard coded total to validate against" + }, + { + "$ref": "https://eq.ons.gov.uk/value_sources.json#/value_source_for_calculations", + "description": "A value source to validate against that resolves to a number." + } + ] }, "answers_to_calculate": { "type": "array", diff --git a/schemas/value_sources.json b/schemas/value_sources.json index 312a15b8..a93c7421 100644 --- a/schemas/value_sources.json +++ b/schemas/value_sources.json @@ -161,5 +161,17 @@ "$ref": "https://eq.ons.gov.uk/value_sources.json#/list_value_source" } ] + }, + "value_source_for_calculations": { + "description": "Only answer and calculated summary value source are supported in current implementation", + "anyOf": [ + { + "$ref": "https://eq.ons.gov.uk/value_sources.json#/answer_value_source", + "description": "Only answer types that return numeric answers are valid" + }, + { + "$ref": "https://eq.ons.gov.uk/value_sources.json#/calculated_summary_value_source" + } + ] } } diff --git a/tests/schemas/invalid/test_invalid_calculations_value_source.json b/tests/schemas/invalid/test_invalid_calculations_value_source.json new file mode 100644 index 00000000..e9d6aab5 --- /dev/null +++ b/tests/schemas/invalid/test_invalid_calculations_value_source.json @@ -0,0 +1,109 @@ +{ + "mime_type": "application/json/ons/eq", + "language": "en", + "schema_version": "0.0.1", + "data_version": "0.0.3", + "survey_id": "0", + "title": "Calculated question with value sources test survey", + "theme": "default", + "description": "A survey that tests validation against value sources, answer value source is not one of Number types which is expected for calculations", + "metadata": [ + { + "name": "user_id", + "type": "string" + }, + { + "name": "period_id", + "type": "string" + }, + { + "name": "ru_name", + "type": "string" + } + ], + "questionnaire_flow": { + "type": "Linear", + "options": { + "summary": { + "collapsible": false + } + } + }, + "sections": [ + { + "id": "default-section", + "groups": [ + { + "id": "group", + "title": "Validate sum against answer, calculated summary source", + "blocks": [ + { + "type": "Question", + "id": "total-block", + "question": { + "id": "total-question", + "title": "Total", + "description": [ + "Enter a number to breakdown in subsequent questions and calculated summary." + ], + "type": "General", + "answers": [ + { + "id": "total-answer", + "label": "Total", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + { + "type": "Question", + "id": "breakdown-block", + "question": { + "id": "breakdown-question", + "title": "Breakdown validated against an answer value source", + "description": [ + "This is a breakdown of the total number from the previous question." + ], + "type": "Calculated", + "calculations": [ + { + "calculation_type": "sum", + "value": { + "source": "answers", + "identifier": "total-answer" + }, + "answers_to_calculate": [ + "breakdown-1", + "breakdown-2", + "breakdown-3", + "breakdown-4" + ], + "conditions": ["equals"] + } + ], + "answers": [ + { + "id": "breakdown-1", + "label": "Breakdown 1", + "mandatory": false, + "decimal_places": 2, + "type": "Number" + }, + { + "id": "breakdown-2", + "label": "Breakdown 2", + "mandatory": false, + "decimal_places": 2, + "type": "Number" + } + ] + } + } + ] + } + ] + } + ] +} diff --git a/tests/schemas/valid/test_question_validation_messages.json b/tests/schemas/valid/test_question_validation_messages.json index a5dac0f3..d2418c28 100644 --- a/tests/schemas/valid/test_question_validation_messages.json +++ b/tests/schemas/valid/test_question_validation_messages.json @@ -35,6 +35,31 @@ "groups": [ { "blocks": [ + { + "type": "Question", + "id": "total-block", + "question": { + "id": "total-question", + "title": "Total", + "description": [ + "Enter a number to breakdown in subsequent questions and calculated summary." + ], + "type": "General", + "answers": [ + { + "id": "total-answer", + "label": "Total", + "mandatory": true, + "type": "Number", + "decimal_places": 2, + "minimum": { + "value": 0, + "exclusive": true + } + } + ] + } + }, { "type": "Question", "id": "date-block", diff --git a/tests/validators/blocks/test_calculated_question_validator.py b/tests/validators/blocks/test_calculated_question_validator.py index b4495947..dc694e23 100644 --- a/tests/validators/blocks/test_calculated_question_validator.py +++ b/tests/validators/blocks/test_calculated_question_validator.py @@ -1,42 +1,50 @@ -from app.validators.questions.calculated_question_validator import ( - CalculatedQuestionValidator, -) +from app.validators.questionnaire_schema import QuestionnaireSchema +from app.validators.questions import get_question_validator +from tests.test_questionnaire_validator import _open_and_load_schema_file def test_invalid_id_in_answers_to_calculate(): + filename = "schemas/invalid/test_invalid_calculations_value_source.json" + schema = QuestionnaireSchema(_open_and_load_schema_file(filename)) question = { - "answers": [ - { - "decimal_places": 2, - "id": "breakdown-1", - "label": "Breakdown 1", - "type": "Number", - }, - { - "decimal_places": 2, - "id": "breakdown-2", - "label": "Breakdown 2", - "type": "Number", - }, + "id": "breakdown-question", + "title": "Breakdown validated against an answer value source", + "description": [ + "This is a breakdown of the total number from the previous question." ], + "type": "Calculated", "calculations": [ { - "answer_id": "total-answer", + "calculation_type": "sum", + "value": {"source": "answers", "identifier": "total-answer"}, "answers_to_calculate": [ "breakdown-1", "breakdown-2", "breakdown-3", "breakdown-4", ], - "calculation_type": "sum", "conditions": ["equals"], } ], - "id": "breakdown-question", - "title": "Breakdown", - "type": "Calculated", + "answers": [ + { + "id": "breakdown-1", + "label": "Breakdown 1", + "mandatory": False, + "decimal_places": 2, + "type": "Number", + }, + { + "id": "breakdown-2", + "label": "Breakdown 2", + "mandatory": False, + "decimal_places": 2, + "type": "Number", + }, + ], } - validator = CalculatedQuestionValidator(question) + + validator = get_question_validator(question, schema) validator.validate() expected_error_messages = [ @@ -50,6 +58,13 @@ def test_invalid_id_in_answers_to_calculate(): "question_id": "breakdown-question", "answer_id": "breakdown-4", }, + { + "message": validator.ANSWER_TYPE_FOR_CALCULATION_TYPE_INVALID.format( + answer_type="string" + ), + "referenced_answer": "total-answer", + "question_id": "breakdown-question", + }, ] assert expected_error_messages == validator.errors diff --git a/tests/validators/questions/test_question_validator.py b/tests/validators/questions/test_question_validator.py index 7bf87413..767177e3 100644 --- a/tests/validators/questions/test_question_validator.py +++ b/tests/validators/questions/test_question_validator.py @@ -1,4 +1,5 @@ from app.validators.questions import get_question_validator +from tests.conftest import get_mock_schema def test_no_answer_label_single_answer(): @@ -9,7 +10,7 @@ def test_no_answer_label_single_answer(): "answers": [{"id": "number-1", "mandatory": False, "type": "Number"}], } - validator = get_question_validator(question) + validator = get_question_validator(question, schema=get_mock_schema()) validator.validate() assert not validator.errors @@ -31,7 +32,7 @@ def test_no_answer_label_multiple_answers(): ], } - validator = get_question_validator(question) + validator = get_question_validator(question, schema=get_mock_schema()) validator.validate() expected_error_messages = [ @@ -74,7 +75,7 @@ def test_no_answer_label_mutually_exclusive(): ], } - validator = get_question_validator(question) + validator = get_question_validator(question, schema=get_mock_schema()) validator.validate() assert not validator.errors @@ -106,7 +107,7 @@ def test_no_answer_label_two_answers_last_answer_single_checkbox(): ], } - validator = get_question_validator(question) + validator = get_question_validator(question, schema=get_mock_schema()) validator.validate() assert not validator.errors