From 015d50098f58d8ee1658219d745d851d0862f660 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Mon, 4 Dec 2023 17:00:34 +0000 Subject: [PATCH 01/21] Add test and new invalid schema for scenario --- ...nswer_id_for_different_list_collector.json | 325 ++++++++++++++++++ .../blocks/test_list_collector_validator.py | 22 +- 2 files changed, 346 insertions(+), 1 deletion(-) create mode 100644 tests/schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json diff --git a/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json new file mode 100644 index 00000000..98db054d --- /dev/null +++ b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_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": "visitor-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": "visitor-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": "visitor-first-name" + }, + { + "source": "answers", + "identifier": "visitor-last-name" + } + ] + }, + "transform": "concatenate_list" + } + ] + } + ] + } + } + } + ] + } + ] + } + ] +} diff --git a/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index e3f46897..b834d487 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -32,7 +32,7 @@ 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.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD, "list_name": "people", "block_id": "list-collector", } @@ -41,6 +41,26 @@ def test_invalid_list_collector_with_different_add_block_answer_ids(): 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_add_block_answer_id_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_ADD, + "list_name": "people", + "block_id": "list-collector", + "other_list_collector_name": "visitor" + } + ] + + 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)) From 8aceaab92c60669e89fc867614a6d6df01babd3f Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Mon, 4 Dec 2023 17:00:53 +0000 Subject: [PATCH 02/21] Add validation --- .../blocks/list_collector_validator.py | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index d37aa83f..951acdd7 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -23,7 +23,8 @@ 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" + NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD = "Multiple list collectors with same name populate a list using different answer_ids in the add block" + DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD = "Different list collectors populate a list using duplicate answer_ids in the add block" NON_SINGLE_REPEATING_BLOCKS_LIST_COLLECTOR = "List may only have one List Collector, if the List Collector features Repeating Blocks" def validate(self): @@ -96,18 +97,31 @@ def validate_other_list_collectors(self): ) 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_list_collector_name = other_list_collector["for_list"] + is_matching_list_collector = other_list_collector_name == list_name 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: + has_other_add_id_duplicates = any( + add_answer_id in other_add_ids + for add_answer_id in add_answer_ids + ) + + if is_matching_list_collector: + if add_answer_ids.symmetric_difference(other_add_ids): + self.add_error( + self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD, + list_name=list_name, + ) + elif has_other_add_id_duplicates: self.add_error( - self.NON_UNIQUE_ANSWER_ID_FOR_LIST_COLLECTOR_ADD, + self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD, list_name=list_name, + other_list_collector_name=other_list_collector_name, ) def validate_single_repeating_blocks_list_collector(self): From ed55fcab9c23b82dd319abce742d5441f6dc987b Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 5 Dec 2023 08:05:52 +0000 Subject: [PATCH 03/21] Formatting --- app/validators/blocks/list_collector_validator.py | 3 +-- tests/validators/blocks/test_list_collector_validator.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 951acdd7..0245fb53 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -107,8 +107,7 @@ def validate_other_list_collectors(self): other_list_collector["add_block"]["id"] ) has_other_add_id_duplicates = any( - add_answer_id in other_add_ids - for add_answer_id in add_answer_ids + add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) if is_matching_list_collector: diff --git a/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index b834d487..e0668baf 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -54,7 +54,7 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD, "list_name": "people", "block_id": "list-collector", - "other_list_collector_name": "visitor" + "other_list_collector_name": "visitor", } ] From f9b9802d28c9c0f6287cd29df8988c15abce384b Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 5 Dec 2023 08:20:33 +0000 Subject: [PATCH 04/21] Slight wording change --- app/validators/blocks/list_collector_validator.py | 13 +++++++------ .../blocks/test_list_collector_validator.py | 4 ++-- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 0245fb53..7131811e 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -23,8 +23,8 @@ 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_SAME_LIST_COLLECTOR_ADD = "Multiple list collectors with same name populate a list using different answer_ids in the add block" - DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD = "Different list collectors populate a list using duplicate answer_ids in the add block" + NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK = "Multiple list collectors with same name populate a list using different answer_ids in the add block" + DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK = "Different list collectors populate a list using duplicate answer_ids in the add block" NON_SINGLE_REPEATING_BLOCKS_LIST_COLLECTOR = "List may only have one List Collector, if the List Collector features Repeating Blocks" def validate(self): @@ -102,7 +102,8 @@ def validate_other_list_collectors(self): for other_list_collector in other_list_collectors: other_list_collector_name = other_list_collector["for_list"] - is_matching_list_collector = other_list_collector_name == list_name + is_other_list_collector_name_matching = other_list_collector_name == list_name + other_add_ids = self.questionnaire_schema.get_all_answer_ids( other_list_collector["add_block"]["id"] ) @@ -110,15 +111,15 @@ def validate_other_list_collectors(self): add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) - if is_matching_list_collector: + if is_other_list_collector_name_matching: if add_answer_ids.symmetric_difference(other_add_ids): self.add_error( - self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD, + self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, ) elif has_other_add_id_duplicates: self.add_error( - self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD, + self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, other_list_collector_name=other_list_collector_name, ) diff --git a/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index e0668baf..ac0458f2 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -32,7 +32,7 @@ def test_invalid_list_collector_with_different_add_block_answer_ids(): expected_errors = [ { - "message": validator.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD, + "message": validator.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, "list_name": "people", "block_id": "list-collector", } @@ -51,7 +51,7 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different expected_errors = [ { - "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD, + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, "list_name": "people", "block_id": "list-collector", "other_list_collector_name": "visitor", From 45145b1015119e8686ddba691603ef815fa5b2c5 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 5 Dec 2023 09:14:37 +0000 Subject: [PATCH 05/21] Reduce nesting logic slightly --- app/validators/blocks/list_collector_validator.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 7131811e..e33d03a0 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -111,8 +111,7 @@ def validate_other_list_collectors(self): add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) - if is_other_list_collector_name_matching: - if add_answer_ids.symmetric_difference(other_add_ids): + if is_other_list_collector_name_matching and add_answer_ids.symmetric_difference(other_add_ids): self.add_error( self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, From ea84440af8e3e912c350c2930f557ba168e6db95 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 5 Dec 2023 10:26:02 +0000 Subject: [PATCH 06/21] Linting --- app/validators/blocks/list_collector_validator.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index e33d03a0..9c7a4977 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -23,7 +23,7 @@ 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_SAME_LIST_COLLECTOR_ADD_BLOCK = "Multiple list collectors with same name populate a list using different answer_ids in the add block" + NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK = "Multiple list collectors with same name populate a list using different answer_ids in add block" DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK = "Different list collectors populate a list using duplicate answer_ids in the add block" NON_SINGLE_REPEATING_BLOCKS_LIST_COLLECTOR = "List may only have one List Collector, if the List Collector features Repeating Blocks" @@ -102,7 +102,9 @@ def validate_other_list_collectors(self): for other_list_collector in other_list_collectors: other_list_collector_name = other_list_collector["for_list"] - is_other_list_collector_name_matching = other_list_collector_name == list_name + is_other_list_collector_name_matching = ( + other_list_collector_name == list_name + ) other_add_ids = self.questionnaire_schema.get_all_answer_ids( other_list_collector["add_block"]["id"] @@ -111,7 +113,8 @@ def validate_other_list_collectors(self): add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) - if is_other_list_collector_name_matching and add_answer_ids.symmetric_difference(other_add_ids): + if is_other_list_collector_name_matching: + if add_answer_ids.symmetric_difference(other_add_ids): self.add_error( self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, From 17d0b480ab81937d1e9febddd962ecc5d4959e93 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 5 Dec 2023 12:10:25 +0000 Subject: [PATCH 07/21] Improve error message slightly --- app/validators/blocks/list_collector_validator.py | 1 + tests/validators/blocks/test_list_collector_validator.py | 1 + 2 files changed, 2 insertions(+) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 9c7a4977..2fb54723 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -124,6 +124,7 @@ def validate_other_list_collectors(self): self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, other_list_collector_name=other_list_collector_name, + other_list_collector_block_id=other_list_collector["id"], ) def validate_single_repeating_blocks_list_collector(self): diff --git a/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index ac0458f2..424594cf 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -55,6 +55,7 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different "list_name": "people", "block_id": "list-collector", "other_list_collector_name": "visitor", + "other_list_collector_block_id": "visitor-list-collector", } ] From 23c9534dbe9ef04b67de3d5678358bea33ac071d Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 5 Dec 2023 20:37:25 +0000 Subject: [PATCH 08/21] Remove nesting and tweak var naming --- .../blocks/list_collector_validator.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 2fb54723..3b63e3e6 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -102,24 +102,24 @@ def validate_other_list_collectors(self): for other_list_collector in other_list_collectors: other_list_collector_name = other_list_collector["for_list"] - is_other_list_collector_name_matching = ( + are_list_collector_names_matching = ( other_list_collector_name == list_name ) other_add_ids = self.questionnaire_schema.get_all_answer_ids( other_list_collector["add_block"]["id"] ) - has_other_add_id_duplicates = any( + contains_duplicate_add_answer_ids = any( add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) - if is_other_list_collector_name_matching: - if add_answer_ids.symmetric_difference(other_add_ids): - self.add_error( - self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, - list_name=list_name, - ) - elif has_other_add_id_duplicates: + if are_list_collector_names_matching and add_answer_ids.symmetric_difference(other_add_ids): + self.add_error( + self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, + list_name=list_name, + ) + + if not are_list_collector_names_matching and contains_duplicate_add_answer_ids: self.add_error( self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, From 483c0810f8a95cb10de8abd2368c3a8dcaff6565 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Wed, 6 Dec 2023 08:13:37 +0000 Subject: [PATCH 09/21] Small tidy up and add comments --- app/validators/blocks/list_collector_validator.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 3b63e3e6..b10c33be 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -102,23 +102,25 @@ def validate_other_list_collectors(self): for other_list_collector in other_list_collectors: other_list_collector_name = other_list_collector["for_list"] - are_list_collector_names_matching = ( - other_list_collector_name == list_name - ) - other_add_ids = self.questionnaire_schema.get_all_answer_ids( other_list_collector["add_block"]["id"] ) + + are_list_collector_names_matching = ( + other_list_collector_name == list_name + ) contains_duplicate_add_answer_ids = any( add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) + # Check for duplicate answer IDs for the same list collector if are_list_collector_names_matching and add_answer_ids.symmetric_difference(other_add_ids): self.add_error( self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, ) + # Check for duplicate answer IDs for a different list collector if not are_list_collector_names_matching and contains_duplicate_add_answer_ids: self.add_error( self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, From 89418e94a5b2f9e4085d5146e2f5f37f494a14f1 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Wed, 6 Dec 2023 08:17:31 +0000 Subject: [PATCH 10/21] Formatting --- app/validators/blocks/list_collector_validator.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index b10c33be..8d587dac 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -106,22 +106,26 @@ def validate_other_list_collectors(self): other_list_collector["add_block"]["id"] ) - are_list_collector_names_matching = ( - other_list_collector_name == list_name - ) + are_list_collector_names_matching = other_list_collector_name == list_name contains_duplicate_add_answer_ids = any( add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) # Check for duplicate answer IDs for the same list collector - if are_list_collector_names_matching and add_answer_ids.symmetric_difference(other_add_ids): + if ( + are_list_collector_names_matching + and add_answer_ids.symmetric_difference(other_add_ids) + ): self.add_error( self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, ) # Check for duplicate answer IDs for a different list collector - if not are_list_collector_names_matching and contains_duplicate_add_answer_ids: + if ( + not are_list_collector_names_matching + and contains_duplicate_add_answer_ids + ): self.add_error( self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, From 37ebd747d8ae036338a87929d02ebeed65d7993a Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Wed, 6 Dec 2023 10:16:56 +0000 Subject: [PATCH 11/21] Remove comment --- app/validators/blocks/list_collector_validator.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 8d587dac..4e01bac2 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -111,7 +111,6 @@ def validate_other_list_collectors(self): add_answer_id in other_add_ids for add_answer_id in add_answer_ids ) - # Check for duplicate answer IDs for the same list collector if ( are_list_collector_names_matching and add_answer_ids.symmetric_difference(other_add_ids) @@ -121,7 +120,7 @@ def validate_other_list_collectors(self): list_name=list_name, ) - # Check for duplicate answer IDs for a different list collector + # Check for duplicate answer IDs in a different list collector if ( not are_list_collector_names_matching and contains_duplicate_add_answer_ids From be7dc90b82c34fa2dd0237aa658951cbe67c506c Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Thu, 7 Dec 2023 14:00:51 +0000 Subject: [PATCH 12/21] Change include duplicate edit & remove answer_ids and make error generic --- .../blocks/list_collector_validator.py | 68 +++++++++++-------- .../blocks/test_list_collector_validator.py | 7 +- 2 files changed, 45 insertions(+), 30 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 4e01bac2..b7380335 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -1,3 +1,5 @@ +from typing import Sequence + from app.validators.blocks.block_validator import BlockValidator from app.validators.blocks.validate_list_collector_quesitons_mixin import ( ValidateListCollectorQuestionsMixin, @@ -24,7 +26,7 @@ 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_SAME_LIST_COLLECTOR_ADD_BLOCK = "Multiple list collectors with same name populate a list using different answer_ids in add block" - DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK = "Different list collectors populate a list using duplicate answer_ids in the add block" + DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK = "Different list collectors populate a list using duplicate answer_ids in a list block" NON_SINGLE_REPEATING_BLOCKS_LIST_COLLECTOR = "List may only have one List Collector, if the List Collector features Repeating Blocks" def validate(self): @@ -91,46 +93,58 @@ def validate_not_for_supplementary_list(self): ) def validate_other_list_collectors(self): + """ + 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_blocks_to_validate = ["add_block", "edit_block", "remove_block"] list_name = self.block["for_list"] - add_answer_ids = self.questionnaire_schema.get_all_answer_ids( - self.block["add_block"]["id"] - ) + + list_block_answer_ids = { + list_block: self.questionnaire_schema.get_all_answer_ids( + self.block[list_block]["id"] + ) + for list_block in list_blocks_to_validate + } other_list_collectors = self.questionnaire_schema.get_other_blocks( self.block["id"], type="ListCollector" ) for other_list_collector in other_list_collectors: - other_list_collector_name = other_list_collector["for_list"] - other_add_ids = self.questionnaire_schema.get_all_answer_ids( - other_list_collector["add_block"]["id"] - ) + other_list_name = other_list_collector["for_list"] + are_list_names_matching = other_list_name == list_name - are_list_collector_names_matching = other_list_collector_name == list_name - contains_duplicate_add_answer_ids = any( - add_answer_id in other_add_ids for add_answer_id in add_answer_ids - ) + other_list_block_answer_ids = { + list_block: self.questionnaire_schema.get_all_answer_ids( + other_list_collector[list_block]["id"] + ) + for list_block in list_blocks_to_validate + } - if ( - are_list_collector_names_matching - and add_answer_ids.symmetric_difference(other_add_ids) - ): + if are_list_names_matching and list_block_answer_ids[ + "add_block" + ].symmetric_difference(other_list_block_answer_ids["add_block"]): self.add_error( self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, ) - # Check for duplicate answer IDs in a different list collector - if ( - not are_list_collector_names_matching - and contains_duplicate_add_answer_ids - ): - self.add_error( - self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, - list_name=list_name, - other_list_collector_name=other_list_collector_name, - other_list_collector_block_id=other_list_collector["id"], - ) + if not are_list_names_matching: + for list_block in list_blocks_to_validate: + has_duplicate_answer_ids = any( + answer_id in other_list_block_answer_ids[list_block] + for answer_id in list_block_answer_ids[list_block] + ) + if has_duplicate_answer_ids: + self.add_error( + self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, + list_name=list_name, + list_block=list_block, + other_list_name=other_list_name, + 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/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index 424594cf..cbe8378f 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -51,11 +51,12 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different expected_errors = [ { - "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_ADD_BLOCK, + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, "list_name": "people", + "list_block": "add_block", "block_id": "list-collector", - "other_list_collector_name": "visitor", - "other_list_collector_block_id": "visitor-list-collector", + "other_list_name": "visitor", + "other_list_block_id": "visitor-list-collector", } ] From fbda0f4d8bf6fb8c9d8f2d526dd621e2afb398ad Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Thu, 7 Dec 2023 14:02:40 +0000 Subject: [PATCH 13/21] Linting --- app/validators/blocks/list_collector_validator.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index b7380335..e6f818f2 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -1,5 +1,3 @@ -from typing import Sequence - from app.validators.blocks.block_validator import BlockValidator from app.validators.blocks.validate_list_collector_quesitons_mixin import ( ValidateListCollectorQuestionsMixin, From d2e23f2e76664dbad51e8834fc221d70e96f3647 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Thu, 7 Dec 2023 14:28:10 +0000 Subject: [PATCH 14/21] Tweak test case and fix existing --- ..._list_collector_section_summary_items.json | 2 +- ...wer_ids_for_different_list_collector.json} | 4 ++-- .../test_answer_codes_list_collector.json | 4 ++-- tests/test_answer_code_validator.py | 4 ++-- .../blocks/test_list_collector_validator.py | 20 +++++++++++++++++-- 5 files changed, 25 insertions(+), 9 deletions(-) rename tests/schemas/invalid/{test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json => test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json} (98%) 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_add_block_answer_id_for_different_list_collector.json b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json similarity index 98% rename from tests/schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json rename to tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json index 98db054d..8e9cc7e7 100644 --- a/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json +++ b/tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_ids_for_different_list_collector.json @@ -238,7 +238,7 @@ "title": "What is the name of the visitor?", "answers": [ { - "id": "visitor-first-name", + "id": "first-name", "label": "First name", "mandatory": true, "type": "TextField" @@ -267,7 +267,7 @@ "title": "Are you sure you want to remove this visitor?", "answers": [ { - "id": "visitor-remove-confirmation", + "id": "remove-confirmation", "mandatory": true, "type": "Radio", "options": [ 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 cbe8378f..d2ac0a8f 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -42,7 +42,7 @@ def test_invalid_list_collector_with_different_add_block_answer_ids(): def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector(): - filename = "schemas/invalid/test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different_list_collector.json" + 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") @@ -57,7 +57,23 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different "block_id": "list-collector", "other_list_name": "visitor", "other_list_block_id": "visitor-list-collector", - } + }, + { + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, + "list_name": "people", + "list_block": "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_BLOCK, + "list_name": "people", + "list_block": "remove_block", + "block_id": "list-collector", + "other_list_name": "visitor", + "other_list_block_id": "visitor-list-collector", + }, ] assert expected_errors == validator.errors From 126c2407b880c462c27d5499a420f566c48bb481 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Fri, 8 Dec 2023 11:35:12 +0000 Subject: [PATCH 15/21] Rename vars and group similar together --- .../blocks/list_collector_validator.py | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index e6f818f2..89004486 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -97,9 +97,9 @@ def validate_other_list_collectors(self): - duplicate answer id in add, edit, or remove block for other different-named list collectors """ list_blocks_to_validate = ["add_block", "edit_block", "remove_block"] - list_name = self.block["for_list"] - list_block_answer_ids = { + list_name = self.block["for_list"] + list_blocks_answer_ids = { list_block: self.questionnaire_schema.get_all_answer_ids( self.block[list_block]["id"] ) @@ -114,32 +114,33 @@ def validate_other_list_collectors(self): other_list_name = other_list_collector["for_list"] are_list_names_matching = other_list_name == list_name - other_list_block_answer_ids = { - list_block: self.questionnaire_schema.get_all_answer_ids( - other_list_collector[list_block]["id"] + other_list_blocks_answer_ids = { + other_list_block: self.questionnaire_schema.get_all_answer_ids( + other_list_collector[other_list_block]["id"] ) - for list_block in list_blocks_to_validate + for other_list_block in list_blocks_to_validate } - if are_list_names_matching and list_block_answer_ids[ + if are_list_names_matching and list_blocks_answer_ids[ "add_block" - ].symmetric_difference(other_list_block_answer_ids["add_block"]): + ].symmetric_difference(other_list_blocks_answer_ids["add_block"]): self.add_error( self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, list_name=list_name, ) + # Validate other different list collector if not are_list_names_matching: - for list_block in list_blocks_to_validate: + for list_block_to_validate in list_blocks_to_validate: has_duplicate_answer_ids = any( - answer_id in other_list_block_answer_ids[list_block] - for answer_id in list_block_answer_ids[list_block] + answer_id in other_list_blocks_answer_ids[list_block_to_validate] + for answer_id in list_blocks_answer_ids[list_block_to_validate] ) if has_duplicate_answer_ids: self.add_error( self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, list_name=list_name, - list_block=list_block, + list_block=list_block_to_validate, other_list_name=other_list_name, other_list_block_id=other_list_collector["id"], ) From e1d52e0fdaf0e1781159db2c930b94cce834fcad Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Fri, 8 Dec 2023 11:38:13 +0000 Subject: [PATCH 16/21] Formatting --- app/validators/blocks/list_collector_validator.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 89004486..8a484cf8 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -133,7 +133,8 @@ def validate_other_list_collectors(self): if not are_list_names_matching: for list_block_to_validate in list_blocks_to_validate: has_duplicate_answer_ids = any( - answer_id in other_list_blocks_answer_ids[list_block_to_validate] + answer_id + in other_list_blocks_answer_ids[list_block_to_validate] for answer_id in list_blocks_answer_ids[list_block_to_validate] ) if has_duplicate_answer_ids: From f2d453cbf2ac01329c0d52e43c259e8bf3f21db6 Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 12 Dec 2023 08:00:43 +0000 Subject: [PATCH 17/21] Address PR comments --- .../blocks/list_collector_validator.py | 50 ++++++++----------- .../blocks/test_list_collector_validator.py | 8 +-- 2 files changed, 26 insertions(+), 32 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 8a484cf8..ccb11559 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -23,7 +23,7 @@ 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_SAME_LIST_COLLECTOR_ADD_BLOCK = "Multiple list collectors with same name populate a list using different answer_ids in 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_BLOCK = "Different list collectors populate a list using duplicate answer_ids in a list block" NON_SINGLE_REPEATING_BLOCKS_LIST_COLLECTOR = "List may only have one List Collector, if the List Collector features Repeating Blocks" @@ -100,10 +100,10 @@ def validate_other_list_collectors(self): list_name = self.block["for_list"] list_blocks_answer_ids = { - list_block: self.questionnaire_schema.get_all_answer_ids( - self.block[list_block]["id"] + child_block: self.questionnaire_schema.get_all_answer_ids( + self.block[child_block]["id"] ) - for list_block in list_blocks_to_validate + for child_block in list_blocks_to_validate } other_list_collectors = self.questionnaire_schema.get_other_blocks( @@ -111,38 +111,32 @@ def validate_other_list_collectors(self): ) for other_list_collector in other_list_collectors: - other_list_name = other_list_collector["for_list"] - are_list_names_matching = other_list_name == list_name - other_list_blocks_answer_ids = { - other_list_block: self.questionnaire_schema.get_all_answer_ids( - other_list_collector[other_list_block]["id"] + child_block: self.questionnaire_schema.get_all_answer_ids( + other_list_collector[child_block]["id"] ) - for other_list_block in list_blocks_to_validate + for child_block in list_blocks_to_validate } - if are_list_names_matching and list_blocks_answer_ids[ - "add_block" - ].symmetric_difference(other_list_blocks_answer_ids["add_block"]): - self.add_error( - self.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, - list_name=list_name, - ) - - # Validate other different list collector - if not are_list_names_matching: - for list_block_to_validate in list_blocks_to_validate: - has_duplicate_answer_ids = any( - answer_id - in other_list_blocks_answer_ids[list_block_to_validate] - for answer_id in list_blocks_answer_ids[list_block_to_validate] + if list_name == other_list_collector["for_list"]: + if list_blocks_answer_ids["add_block"].symmetric_difference( + other_list_blocks_answer_ids["add_block"] + ): + self.add_error( + self.DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST, + list_name=list_name, ) - if has_duplicate_answer_ids: + else: + for child_block in list_blocks_to_validate: + if ( + other_list_blocks_answer_ids[child_block] + & list_blocks_answer_ids[child_block] + ): self.add_error( self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, list_name=list_name, - list_block=list_block_to_validate, - other_list_name=other_list_name, + child_block=child_block, + other_list_name=other_list_collector["for_list"], other_list_block_id=other_list_collector["id"], ) diff --git a/tests/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index d2ac0a8f..9f1470f3 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -32,7 +32,7 @@ def test_invalid_list_collector_with_different_add_block_answer_ids(): expected_errors = [ { - "message": validator.NON_UNIQUE_ANSWER_ID_FOR_SAME_LIST_COLLECTOR_ADD_BLOCK, + "message": validator.DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST, "list_name": "people", "block_id": "list-collector", } @@ -53,7 +53,7 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different { "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, "list_name": "people", - "list_block": "add_block", + "child_block": "add_block", "block_id": "list-collector", "other_list_name": "visitor", "other_list_block_id": "visitor-list-collector", @@ -61,7 +61,7 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different { "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, "list_name": "people", - "list_block": "edit_block", + "child_block": "edit_block", "block_id": "list-collector", "other_list_name": "visitor", "other_list_block_id": "visitor-list-collector", @@ -69,7 +69,7 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different { "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, "list_name": "people", - "list_block": "remove_block", + "child_block": "remove_block", "block_id": "list-collector", "other_list_name": "visitor", "other_list_block_id": "visitor-list-collector", From 29c32f9c5ad68686c1df620505a9f1c2f0351a8e Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 12 Dec 2023 13:40:09 +0000 Subject: [PATCH 18/21] Add check for duplicate answer_id in list child block. Update key names for error. Add new unit test for this scenario. --- .../blocks/list_collector_validator.py | 43 ++-- ...th_duplicate_answer_id_used_elsewhere.json | 198 ++++++++++++++++++ .../blocks/test_list_collector_validator.py | 42 +++- 3 files changed, 262 insertions(+), 21 deletions(-) create mode 100644 tests/schemas/invalid/test_invalid_list_collector_with_duplicate_answer_id_used_elsewhere.json diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index ccb11559..b4655bca 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -8,6 +8,7 @@ class ListCollectorValidator(BlockValidator, ValidateListCollectorQuestionsMixin LIST_COLLECTOR_KEY_MISSING = "Missing key in ListCollector" REDIRECT_TO_LIST_ADD_BLOCK_ACTION = "RedirectToListAddBlock" REMOVE_LIST_ITEM_AND_ANSWERS_ACTION = "RemoveListItemAndAnswers" + LIST_CHILD_BLOCKS_TO_VALIDATE = ["add_block", "edit_block", "remove_block"] NO_REDIRECT_TO_LIST_ADD_BLOCK_ACTION = ( f"{REDIRECT_TO_LIST_ADD_BLOCK_ACTION} action not found" @@ -24,7 +25,8 @@ 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" 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_BLOCK = "Different list collectors populate a list using duplicate answer_ids in a list 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): @@ -67,19 +69,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_block_answer_ids = { + child_block: self.questionnaire_schema.get_all_answer_ids( + self.block[child_block]["id"] + ) + for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE + } - if add_answer_ids.symmetric_difference(edit_answer_ids): + if list_block_answer_ids["add_block"].symmetric_difference( + list_block_answer_ids["edit_block"] + ): self.add_error( self.LIST_COLLECTOR_ADD_EDIT_IDS_DONT_MATCH, block_id=block["id"] ) + for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE: + if list_block_answer_ids[child_block].intersection( + self.questionnaire_schema.ids + ): + 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 @@ -96,14 +111,12 @@ def validate_other_list_collectors(self): - 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_blocks_to_validate = ["add_block", "edit_block", "remove_block"] - list_name = self.block["for_list"] list_blocks_answer_ids = { child_block: self.questionnaire_schema.get_all_answer_ids( self.block[child_block]["id"] ) - for child_block in list_blocks_to_validate + for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE } other_list_collectors = self.questionnaire_schema.get_other_blocks( @@ -115,7 +128,7 @@ def validate_other_list_collectors(self): child_block: self.questionnaire_schema.get_all_answer_ids( other_list_collector[child_block]["id"] ) - for child_block in list_blocks_to_validate + for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE } if list_name == other_list_collector["for_list"]: @@ -127,15 +140,15 @@ def validate_other_list_collectors(self): list_name=list_name, ) else: - for child_block in list_blocks_to_validate: + for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE: if ( other_list_blocks_answer_ids[child_block] & list_blocks_answer_ids[child_block] ): self.add_error( - self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, + self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, list_name=list_name, - child_block=child_block, + list_child_block_name=child_block, other_list_name=other_list_collector["for_list"], other_list_block_id=other_list_collector["id"], ) 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/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index 9f1470f3..1b6c57af 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" @@ -51,25 +81,25 @@ def test_invalid_list_collector_with_duplicate_add_block_answer_id_for_different expected_errors = [ { - "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR_BLOCK, + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, "list_name": "people", - "child_block": "add_block", + "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_BLOCK, + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, "list_name": "people", - "child_block": "edit_block", + "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_BLOCK, + "message": validator.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR, "list_name": "people", - "child_block": "remove_block", + "list_child_block_name": "remove_block", "block_id": "list-collector", "other_list_name": "visitor", "other_list_block_id": "visitor-list-collector", From 3d712086f83b6fa67b98bf281247f09fcbd37fda Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 12 Dec 2023 14:12:47 +0000 Subject: [PATCH 19/21] Fix schema to get test passing. Update newly added schema to ensure it only fails validation where expected --- ..._duplicate_answer_ids_for_different_list_collector.json | 2 +- ...gress_value_source_block_in_past_repeating_section.json | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) 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 index 8e9cc7e7..a4c08f16 100644 --- 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 @@ -301,7 +301,7 @@ "list_to_concatenate": [ { "source": "answers", - "identifier": "visitor-first-name" + "identifier": "first-name" }, { "source": "answers", 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": { From d0cad06792c54ce35dbdbd13dc383b6bbca9f99a Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Tue, 12 Dec 2023 17:01:19 +0000 Subject: [PATCH 20/21] Address PR comments and small tidy up --- .../blocks/list_collector_validator.py | 58 ++++++++----------- app/validators/questionnaire_schema.py | 8 +++ .../blocks/test_list_collector_validator.py | 1 + 3 files changed, 34 insertions(+), 33 deletions(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index b4655bca..27a35f1a 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -8,7 +8,6 @@ class ListCollectorValidator(BlockValidator, ValidateListCollectorQuestionsMixin LIST_COLLECTOR_KEY_MISSING = "Missing key in ListCollector" REDIRECT_TO_LIST_ADD_BLOCK_ACTION = "RedirectToListAddBlock" REMOVE_LIST_ITEM_AND_ANSWERS_ACTION = "RemoveListItemAndAnswers" - LIST_CHILD_BLOCKS_TO_VALIDATE = ["add_block", "edit_block", "remove_block"] NO_REDIRECT_TO_LIST_ADD_BLOCK_ACTION = ( f"{REDIRECT_TO_LIST_ADD_BLOCK_ACTION} action not found" @@ -71,24 +70,21 @@ def validate_list_collector_answer_ids(self, block): - 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 """ - list_block_answer_ids = { - child_block: self.questionnaire_schema.get_all_answer_ids( - self.block[child_block]["id"] + list_answer_ids = ( + self.questionnaire_schema.get_list_collector_answer_ids_by_child_block( + block["id"] ) - for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE - } + ) - if list_block_answer_ids["add_block"].symmetric_difference( - list_block_answer_ids["edit_block"] + 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"] ) - for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE: - if list_block_answer_ids[child_block].intersection( - self.questionnaire_schema.ids - ): + for child_block in list_answer_ids: + if list_answer_ids[child_block].intersection(self.questionnaire_schema.ids): self.add_error( self.LIST_COLLECTOR_ANSWER_ID_USED_ELSEWHERE, block_id=block["id"], @@ -111,43 +107,39 @@ def validate_other_list_collectors(self): - 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_name = self.block["for_list"] - list_blocks_answer_ids = { - child_block: self.questionnaire_schema.get_all_answer_ids( - self.block[child_block]["id"] + list_answer_ids = ( + self.questionnaire_schema.get_list_collector_answer_ids_by_child_block( + self.block["id"] ) - for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE - } - + ) other_list_collectors = self.questionnaire_schema.get_other_blocks( self.block["id"], type="ListCollector" ) for other_list_collector in other_list_collectors: - other_list_blocks_answer_ids = { - child_block: self.questionnaire_schema.get_all_answer_ids( - other_list_collector[child_block]["id"] + other_list_answer_ids = ( + self.questionnaire_schema.get_list_collector_answer_ids_by_child_block( + other_list_collector["id"] ) - for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE - } + ) - if list_name == other_list_collector["for_list"]: - if list_blocks_answer_ids["add_block"].symmetric_difference( - other_list_blocks_answer_ids["add_block"] + 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=list_name, + list_name=self.block["for_list"], + other_list_block_id=other_list_collector["id"], ) else: - for child_block in self.LIST_CHILD_BLOCKS_TO_VALIDATE: - if ( - other_list_blocks_answer_ids[child_block] - & list_blocks_answer_ids[child_block] + 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=list_name, + 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"], diff --git a/app/validators/questionnaire_schema.py b/app/validators/questionnaire_schema.py index 3a4ca792..604b8d92 100644 --- a/app/validators/questionnaire_schema.py +++ b/app/validators/questionnaire_schema.py @@ -378,6 +378,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/validators/blocks/test_list_collector_validator.py b/tests/validators/blocks/test_list_collector_validator.py index 1b6c57af..86a1f973 100644 --- a/tests/validators/blocks/test_list_collector_validator.py +++ b/tests/validators/blocks/test_list_collector_validator.py @@ -65,6 +65,7 @@ def test_invalid_list_collector_with_different_add_block_answer_ids(): "message": validator.DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST, "list_name": "people", "block_id": "list-collector", + "other_list_block_id": "another-list-collector", } ] From 2a4d7b72e89fc5030fa4785d04a04b5853955feb Mon Sep 17 00:00:00 2001 From: Liam Toozer Date: Wed, 13 Dec 2023 07:57:32 +0000 Subject: [PATCH 21/21] Give meaningful var name to all schema ids excluding list collectors for validation logic --- app/validators/blocks/list_collector_validator.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/validators/blocks/list_collector_validator.py b/app/validators/blocks/list_collector_validator.py index 27a35f1a..e852e1df 100644 --- a/app/validators/blocks/list_collector_validator.py +++ b/app/validators/blocks/list_collector_validator.py @@ -83,8 +83,11 @@ def validate_list_collector_answer_ids(self, block): 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(self.questionnaire_schema.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"],