Prevent duplicate answer_ids across different list collectors (#228)

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"):

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])

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": [

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"
+                        }
+                      ]
+                    }
+                  ]
+                }
+              }
+            }
+          ]
+        }
+      ]
+    }
+  ]
+}