diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index d37aa83f..e852e1df 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -23,7 +23,9 @@ class ListCollectorValidator(BlockValidator, ValidateListCollectorQuestionsMixin ) LIST_COLLECTOR_FOR_SUPPLEMENTARY_LIST_IS_INVALID = "Non content list collectors cannot be for a list which comes from supplementary data" LIST_COLLECTOR_ADD_EDIT_IDS_DONT_MATCH = "The list collector block contains an add block and edit block with different answer ids" - NON_UNIQUE_ANSWER_ID_FOR_LIST_COLLECTOR_ADD = "Multiple list collectors populate a list using different answer_ids in the add block" + DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST = "Multiple list collectors with same name populate a list using different answer_ids in add block" + DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR = "Different list collectors populate a list using duplicate answer_ids in a list block" + LIST_COLLECTOR_ANSWER_ID_USED_ELSEWHERE = "List collector child block answer_id is already used elsewhere outside the list collector" NON_SINGLE_REPEATING_BLOCKS_LIST_COLLECTOR = "List may only have one List Collector, if the List Collector features Repeating Blocks" def validate(self): @@ -66,19 +68,32 @@ def validate_list_collector_answer_ids(self, block): """ - Ensure that answer_ids on add and edit blocks match between all blocks that populate a single list. - Enforce the same answer_ids on add and edit sub-blocks + - Ensure that that child block answer_ids are not used elsewhere in the schema that's not another list collector """ - add_answer_ids = self.questionnaire_schema.get_all_answer_ids( - block["add_block"]["id"] - ) - edit_answer_ids = self.questionnaire_schema.get_all_answer_ids( - block["edit_block"]["id"] + list_answer_ids = ( + self.questionnaire_schema.get_list_collector_answer_ids_by_child_block( + block["id"] + ) ) - if add_answer_ids.symmetric_difference(edit_answer_ids): + if list_answer_ids["add_block"].symmetric_difference( + list_answer_ids["edit_block"] + ): self.add_error( self.LIST_COLLECTOR_ADD_EDIT_IDS_DONT_MATCH, block_id=block["id"] ) + all_schema_ids_excluding_list_collectors = self.questionnaire_schema.ids + for child_block in list_answer_ids: + if list_answer_ids[child_block].intersection( + all_schema_ids_excluding_list_collectors + ): + self.add_error( + self.LIST_COLLECTOR_ANSWER_ID_USED_ELSEWHERE, + block_id=block["id"], + list_child_block_name=child_block, + ) + def validate_not_for_supplementary_list(self): """ Standard list collectors cannot be used for a supplementary list, as these may not be edited @@ -90,25 +105,48 @@ def validate_not_for_supplementary_list(self): ) def validate_other_list_collectors(self): - list_name = self.block["for_list"] - add_answer_ids = self.questionnaire_schema.get_all_answer_ids( - self.block["add_block"]["id"] + """ + Checks other list collectors for: + - non-unique answer id in add block for any other same-named list collectors + - duplicate answer id in add, edit, or remove block for other different-named list collectors + """ + list_answer_ids = ( + self.questionnaire_schema.get_list_collector_answer_ids_by_child_block( + self.block["id"] + ) ) - other_list_collectors = self.questionnaire_schema.get_other_blocks( - self.block["id"], for_list=list_name, type="ListCollector" + self.block["id"], type="ListCollector" ) for other_list_collector in other_list_collectors: - other_add_ids = self.questionnaire_schema.get_all_answer_ids( - other_list_collector["add_block"]["id"] - ) - difference = add_answer_ids.symmetric_difference(other_add_ids) - if difference: - self.add_error( - self.NON_UNIQUE_ANSWER_ID_FOR_LIST_COLLECTOR_ADD, - list_name=list_name, + other_list_answer_ids = ( + self.questionnaire_schema.get_list_collector_answer_ids_by_child_block( + other_list_collector["id"] ) + ) + + if self.block["for_list"] == other_list_collector["for_list"]: + if list_answer_ids["add_block"].symmetric_difference( + other_list_answer_ids["add_block"] + ): + self.add_error( + self.DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST, + list_name=self.block["for_list"], + other_list_block_id=other_list_collector["id"], + ) + else: + for child_block in other_list_answer_ids: + if other_list_answer_ids[child_block].intersection( + list_answer_ids[child_block] + ): + self.add_error( + self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, + list_name=self.block["for_list"], + list_child_block_name=child_block, + other_list_name=other_list_collector["for_list"], + other_list_block_id=other_list_collector["id"], + ) def validate_single_repeating_blocks_list_collector(self): if not self.block.get("repeating_blocks"): diff --git a/app/validators/questionnaire_schema.py b/app/validators/questionnaire_schema.py index a0e8e692..f952c868 100644 --- a/app/validators/questionnaire_schema.py +++ b/app/validators/questionnaire_schema.py @@ -396,6 +396,14 @@ def get_list_collector_answer_ids(self, block_id): edit_answer_ids = self.get_all_answer_ids(block["edit_block"]["id"]) return add_answer_ids | edit_answer_ids + @lru_cache + def get_list_collector_answer_ids_by_child_block(self, block_id: str): + block = self.blocks_by_id[block_id] + return { + child_block: self.get_all_answer_ids(block[child_block]["id"]) + for child_block in ["add_block", "edit_block", "remove_block"] + } + @lru_cache def get_all_answer_ids(self, block_id): questions = self.get_all_questions_for_block(self.blocks_by_id[block_id]) diff --git a/tests/schemas/invalid/test_invalid_list_collector_section_summary_items.json b/tests/schemas/invalid/test_invalid_list_collector_section_summary_items.json index 80145c02..af223086 100644 --- a/tests/schemas/invalid/test_invalid_list_collector_section_summary_items.json +++ b/tests/schemas/invalid/test_invalid_list_collector_section_summary_items.json @@ -380,7 +380,7 @@ "warning": "All of the information about this person will be deleted", "answers": [ { - "id": "remove-confirmation", + "id": "remove-person-confirmation", "mandatory": true, "type": "Radio", "options": [ diff --git a/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_id_used_elsewhere.json b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_id_used_elsewhere.json new file mode 100644 index 00000000..22ff32fb --- /dev/null +++ b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_id_used_elsewhere.json @@ -0,0 +1,198 @@ +{ + "mime_type": "application/json/ons/eq", + "language": "en", + "schema_version": "0.0.1", + "data_version": "0.0.3", + "survey_id": "0", + "title": "Test ListCollector", + "theme": "default", + "description": "A questionnaire to test ListCollector", + "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": "section", + "groups": [ + { + "id": "group", + "title": "List", + "blocks": [ + { + "type": "Question", + "id": "name-block", + "question": { + "description": ["Testing"], + "answers": [ + { + "id": "name-answer", + "label": "What is your name?", + "max_length": 20, + "mandatory": false, + "type": "TextField" + } + ], + "id": "name-question", + "title": "Title", + "type": "General" + } + }, + { + "id": "list-collector", + "type": "ListCollector", + "for_list": "people", + "question": { + "id": "confirmation-question", + "type": "General", + "title": "Does anyone else live here?", + "answers": [ + { + "id": "anyone-else", + "mandatory": true, + "type": "Radio", + "options": [ + { + "label": "Yes", + "value": "Yes", + "action": { + "type": "RedirectToListAddBlock" + } + }, + { + "label": "No", + "value": "No" + } + ] + } + ] + }, + "add_block": { + "id": "add-person", + "type": "ListAddQuestion", + "question": { + "id": "add-question", + "type": "General", + "title": "What is the name of the person?", + "answers": [ + { + "id": "name-answer", + "label": "First name", + "mandatory": true, + "type": "TextField" + }, + { + "id": "last-name", + "label": "Last name", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + "edit_block": { + "id": "edit-person", + "type": "ListEditQuestion", + "question": { + "id": "edit-question", + "type": "General", + "title": "What is the name of the person?", + "answers": [ + { + "id": "name-answer", + "label": "First name", + "mandatory": true, + "type": "TextField" + }, + { + "id": "last-name", + "label": "Last name", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + "remove_block": { + "id": "remove-person", + "type": "ListRemoveQuestion", + "question": { + "id": "remove-question", + "type": "General", + "title": "Are you sure you want to remove this person?", + "answers": [ + { + "id": "name-answer", + "mandatory": true, + "type": "Radio", + "options": [ + { + "label": "Yes", + "value": "Yes", + "action": { + "type": "RemoveListItemAndAnswers" + } + }, + { + "label": "No", + "value": "No" + } + ] + } + ] + } + }, + "summary": { + "title": "Household members", + "item_title": { + "text": "{person_name}", + "placeholders": [ + { + "placeholder": "person_name", + "transforms": [ + { + "arguments": { + "delimiter": " ", + "list_to_concatenate": [ + { + "source": "answers", + "identifier": "name-answer" + }, + { + "source": "answers", + "identifier": "name-answer" + } + ] + }, + "transform": "concatenate_list" + } + ] + } + ] + } + } + } + ] + } + ] + } + ] +} diff --git a/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json new file mode 100644 index 00000000..a4c08f16 --- /dev/null +++ b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json @@ -0,0 +1,325 @@ +{ + "mime_type": "application/json/ons/eq", + "language": "en", + "schema_version": "0.0.1", + "data_version": "0.0.3", + "survey_id": "0", + "title": "Test ListCollector", + "theme": "default", + "description": "A questionnaire to test ListCollector", + "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": "section", + "groups": [ + { + "id": "group", + "title": "List", + "blocks": [ + { + "id": "list-collector", + "type": "ListCollector", + "for_list": "people", + "question": { + "id": "confirmation-question", + "type": "General", + "title": "Does anyone else live here?", + "answers": [ + { + "id": "anyone-else", + "mandatory": true, + "type": "Radio", + "options": [ + { + "label": "Yes", + "value": "Yes", + "action": { + "type": "RedirectToListAddBlock" + } + }, + { + "label": "No", + "value": "No" + } + ] + } + ] + }, + "add_block": { + "id": "add-person", + "type": "ListAddQuestion", + "question": { + "id": "add-question", + "type": "General", + "title": "What is the name of the person?", + "answers": [ + { + "id": "first-name", + "label": "First name", + "mandatory": true, + "type": "TextField" + }, + { + "id": "last-name", + "label": "Last name", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + "edit_block": { + "id": "edit-person", + "type": "ListEditQuestion", + "question": { + "id": "edit-question", + "type": "General", + "title": "What is the name of the person?", + "answers": [ + { + "id": "first-name", + "label": "First name", + "mandatory": true, + "type": "TextField" + }, + { + "id": "last-name", + "label": "Last name", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + "remove_block": { + "id": "remove-person", + "type": "ListRemoveQuestion", + "question": { + "id": "remove-question", + "type": "General", + "title": "Are you sure you want to remove this person?", + "answers": [ + { + "id": "remove-confirmation", + "mandatory": true, + "type": "Radio", + "options": [ + { + "label": "Yes", + "value": "Yes", + "action": { + "type": "RemoveListItemAndAnswers" + } + }, + { + "label": "No", + "value": "No" + } + ] + } + ] + } + }, + "summary": { + "title": "Household members", + "item_title": { + "text": "{person_name}", + "placeholders": [ + { + "placeholder": "person_name", + "transforms": [ + { + "arguments": { + "delimiter": " ", + "list_to_concatenate": [ + { + "source": "answers", + "identifier": "first-name" + }, + { + "source": "answers", + "identifier": "last-name" + } + ] + }, + "transform": "concatenate_list" + } + ] + } + ] + } + } + }, + { + "id": "visitor-list-collector", + "type": "ListCollector", + "for_list": "visitor", + "question": { + "id": "visitor-question", + "type": "General", + "title": "Any visitors?", + "answers": [ + { + "id": "any-visitors", + "mandatory": true, + "type": "Radio", + "options": [ + { + "label": "Yes", + "value": "Yes", + "action": { + "type": "RedirectToListAddBlock" + } + }, + { + "label": "No", + "value": "No" + } + ] + } + ] + }, + "add_block": { + "id": "add-visitor", + "type": "ListAddQuestion", + "question": { + "id": "visitor-add-person", + "type": "General", + "title": "What is the name of the visitor?", + "answers": [ + { + "id": "first-name", + "label": "First name", + "mandatory": true, + "type": "TextField" + }, + { + "id": "visitor-middle-names", + "label": "Middle names", + "mandatory": false, + "type": "TextField" + }, + { + "id": "visitor-last-name", + "label": "Last name", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + "edit_block": { + "id": "visitor-edit-person", + "type": "ListEditQuestion", + "question": { + "id": "visitor-edit-question", + "type": "General", + "title": "What is the name of the visitor?", + "answers": [ + { + "id": "first-name", + "label": "First name", + "mandatory": true, + "type": "TextField" + }, + { + "id": "visitor-middle-names", + "label": "Middle names", + "mandatory": false, + "type": "TextField" + }, + { + "id": "visitor-last-name", + "label": "Last name", + "mandatory": true, + "type": "TextField" + } + ] + } + }, + "remove_block": { + "id": "visitor-remove-person", + "type": "ListRemoveQuestion", + "question": { + "id": "visitor-remove-question", + "type": "General", + "title": "Are you sure you want to remove this visitor?", + "answers": [ + { + "id": "remove-confirmation", + "mandatory": true, + "type": "Radio", + "options": [ + { + "label": "Yes", + "value": "Yes", + "action": { + "type": "RemoveListItemAndAnswers" + } + }, + { + "label": "No", + "value": "No" + } + ] + } + ] + } + }, + "summary": { + "title": "Visitors", + "item_title": { + "text": "{visitor_name}", + "placeholders": [ + { + "placeholder": "visitor_name", + "transforms": [ + { + "arguments": { + "delimiter": " ", + "list_to_concatenate": [ + { + "source": "answers", + "identifier": "first-name" + }, + { + "source": "answers", + "identifier": "visitor-last-name" + } + ] + }, + "transform": "concatenate_list" + } + ] + } + ] + } + } + } + ] + } + ] + } + ] +} diff --git a/tests/schemas/invalid/test_invalid_progress_value_source_block_in_past_repeating_section.json b/tests/schemas/invalid/test_invalid_progress_value_source_block_in_past_repeating_section.json index 1392bfaa..3c326e66 100644 --- a/tests/schemas/invalid/test_invalid_progress_value_source_block_in_past_repeating_section.json +++ b/tests/schemas/invalid/test_invalid_progress_value_source_block_in_past_repeating_section.json @@ -224,10 +224,11 @@ "question": { "answers": [ { - "id": "first-name", - "label": "First name", + "id": "date-of-birth-answer", + "description": "Enter your date of birth", + "label": "Date of Birth", "mandatory": true, - "type": "TextField" + "type": "Date" } ], "guidance": { diff --git a/tests/schemas/valid/test_answer_codes_list_collector.json b/tests/schemas/valid/test_answer_codes_list_collector.json index 59cd684b..ef8c60a7 100644 --- a/tests/schemas/valid/test_answer_codes_list_collector.json +++ b/tests/schemas/valid/test_answer_codes_list_collector.json @@ -276,7 +276,7 @@ "title": "Are you sure you want to remove this company or UK branch?", "answers": [ { - "id": "remove-confirmation", + "id": "remove-company-confirmation", "mandatory": true, "type": "Radio", "options": [ @@ -453,7 +453,7 @@ "warning": "All of the information about this person will be deleted", "answers": [ { - "id": "remove-confirmation", + "id": "remove-person-confirmation", "mandatory": true, "type": "Radio", "options": [ diff --git a/tests/test_answer_code_validator.py b/tests/test_answer_code_validator.py index 1c1b4713..6e5c3401 100644 --- a/tests/test_answer_code_validator.py +++ b/tests/test_answer_code_validator.py @@ -744,7 +744,7 @@ def test_invalid_answer_codes_for_list_collector_remove_question(): {"answer_id": "confirmation-checkbox-answer", "code": "3"}, {"answer_id": "anyone-else", "code": "4"}, {"answer_id": "householder-checkbox-answer", "code": "5"}, - {"answer_id": "remove-confirmation", "code": "5a"}, + {"answer_id": "remove-company-confirmation", "code": "5a"}, ] validator = AnswerCodeValidator("0.0.3", answer_codes, questionnaire_schema) @@ -754,7 +754,7 @@ def test_invalid_answer_codes_for_list_collector_remove_question(): expected_errors = [ { "message": validator.INVALID_ANSWER_CODE_FOR_LIST_COLLECTOR, - "answer_id": "remove-confirmation", + "answer_id": "remove-company-confirmation", }, { "message": validator.MISSING_ANSWER_CODE, diff --git a/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index e3f46897..86a1f973 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -22,6 +22,36 @@ def test_invalid_list_collector_with_different_answer_ids_in_add_and_edit(): assert validator.errors == expected_errors +def test_invalid_list_collector_with_answer_id_used_elsewhere(): + filename = "schemas/invalid/test_invalid_list_collector_with_duplicate_answer_id_used_elsewhere.json" + + questionnaire_schema = QuestionnaireSchema(_open_and_load_schema_file(filename)) + block = questionnaire_schema.get_block("list-collector") + validator = ListCollectorValidator(block, questionnaire_schema) + + expected_errors = [ + { + "message": validator.LIST_COLLECTOR_ANSWER_ID_USED_ELSEWHERE, + "block_id": "list-collector", + "list_child_block_name": "add_block", + }, + { + "message": validator.LIST_COLLECTOR_ANSWER_ID_USED_ELSEWHERE, + "block_id": "list-collector", + "list_child_block_name": "edit_block", + }, + { + "message": validator.LIST_COLLECTOR_ANSWER_ID_USED_ELSEWHERE, + "block_id": "list-collector", + "list_child_block_name": "remove_block", + }, + ] + + validator.validate() + + assert validator.errors == expected_errors + + def test_invalid_list_collector_with_different_add_block_answer_ids(): filename = "schemas/invalid/test_invalid_list_collector_with_different_add_block_answer_ids.json" @@ -32,15 +62,54 @@ def test_invalid_list_collector_with_different_add_block_answer_ids(): expected_errors = [ { - "message": validator.NON_UNIQUE_ANSWER_ID_FOR_LIST_COLLECTOR_ADD, + "message": validator.DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST, "list_name": "people", "block_id": "list-collector", + "other_list_block_id": "another-list-collector", } ] assert expected_errors == validator.errors +def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector(): + filename = "schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json" + + questionnaire_schema = QuestionnaireSchema(_open_and_load_schema_file(filename)) + block = questionnaire_schema.get_block("list-collector") + validator = ListCollectorValidator(block, questionnaire_schema) + validator.validate() + + expected_errors = [ + { + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, + "list_name": "people", + "list_child_block_name": "add_block", + "block_id": "list-collector", + "other_list_name": "visitor", + "other_list_block_id": "visitor-list-collector", + }, + { + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, + "list_name": "people", + "list_child_block_name": "edit_block", + "block_id": "list-collector", + "other_list_name": "visitor", + "other_list_block_id": "visitor-list-collector", + }, + { + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, + "list_name": "people", + "list_child_block_name": "remove_block", + "block_id": "list-collector", + "other_list_name": "visitor", + "other_list_block_id": "visitor-list-collector", + }, + ] + + assert expected_errors == validator.errors + + def test_invalid_list_collector_non_radio(): filename = "schemas/invalid/test_invalid_list_collector_non_radio.json" questionnaire_schema = QuestionnaireSchema(_open_and_load_schema_file(filename))