Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent duplicate answer_ids across different list collectors #228

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
015d500
Add test and new invalid schema for scenario
liamtoozer Dec 4, 2023
8aceaab
Add validation
liamtoozer Dec 4, 2023
ed55fca
Formatting
liamtoozer Dec 5, 2023
f9b9802
Slight wording change
liamtoozer Dec 5, 2023
45145b1
Reduce nesting logic slightly
liamtoozer Dec 5, 2023
ea84440
Linting
liamtoozer Dec 5, 2023
17d0b48
Improve error message slightly
liamtoozer Dec 5, 2023
23c9534
Remove nesting and tweak var naming
liamtoozer Dec 5, 2023
483c081
Small tidy up and add comments
liamtoozer Dec 6, 2023
89418e9
Formatting
liamtoozer Dec 6, 2023
37ebd74
Remove comment
liamtoozer Dec 6, 2023
be7dc90
Change include duplicate edit & remove answer_ids and make error generic
liamtoozer Dec 7, 2023
fbda0f4
Linting
liamtoozer Dec 7, 2023
d2e23f2
Tweak test case and fix existing
liamtoozer Dec 7, 2023
126c240
Rename vars and group similar together
liamtoozer Dec 8, 2023
e1d52e0
Formatting
liamtoozer Dec 8, 2023
f2d453c
Address PR comments
liamtoozer Dec 12, 2023
29c32f9
Add check for duplicate answer_id in list child block.
liamtoozer Dec 12, 2023
3d71208
Fix schema to get test passing. Update newly added schema to ensure i…
liamtoozer Dec 12, 2023
d0cad06
Address PR comments and small tidy up
liamtoozer Dec 12, 2023
2a4d7b7
Give meaningful var name to all schema ids excluding list collectors …
liamtoozer Dec 13, 2023
e8b5ffc
Merge branch 'main' into prevent-duplicate-answer-ids-across-differen…
liamtoozer Dec 14, 2023
fd77d55
Merge branch 'main' into prevent-duplicate-answer-ids-across-differen…
liamtoozer Dec 14, 2023
af1fc21
Merge branch 'main' into prevent-duplicate-answer-ids-across-differen…
liamtoozer Dec 19, 2023
1312ea3
Merge branch 'main' into prevent-duplicate-answer-ids-across-differen…
liamtoozer Dec 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 63 additions & 20 deletions app/validators/blocks/list_collector_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -23,7 +24,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):
Expand Down Expand Up @@ -66,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 = {
katie-gardner marked this conversation as resolved.
Show resolved Hide resolved
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
Expand All @@ -90,25 +106,52 @@ 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_name = self.block["for_list"]
add_answer_ids = self.questionnaire_schema.get_all_answer_ids(
self.block["add_block"]["id"]
)
list_blocks_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
}

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_blocks_answer_ids = {
child_block: self.questionnaire_schema.get_all_answer_ids(
other_list_collector[child_block]["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"]
):
self.add_error(
self.DIFFERENT_LIST_COLLECTOR_ADD_BLOCKS_FOR_SAME_LIST,
list_name=list_name,
katie-gardner marked this conversation as resolved.
Show resolved Hide resolved
)
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]
katie-gardner marked this conversation as resolved.
Show resolved Hide resolved
):
self.add_error(
self.DUPLICATE_ANSWER_ID_FOR_DIFFERENT_LIST_COLLECTOR,
list_name=list_name,
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"):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
Expand Down
Original file line number Diff line number Diff line change
@@ -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"
}
]
}
]
}
}
}
]
}
]
}
]
}
Loading