From 28f8340ff27facc1558878bee794780f8ea925e0 Mon Sep 17 00:00:00 2001 From: Mebin Abraham Date: Wed, 24 Jan 2024 17:02:53 +0000 Subject: [PATCH 1/5] Bug fix: Backwards routing does not work when dependencies exist --- app/questionnaire/path_finder.py | 22 +++++++++---------- ...nfirmation_question_backwards_routing.json | 18 ++++++++++++--- tests/app/questionnaire/test_path_finder.py | 22 ++++++++++++------- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/app/questionnaire/path_finder.py b/app/questionnaire/path_finder.py index 4b7e82d099..19119e4645 100644 --- a/app/questionnaire/path_finder.py +++ b/app/questionnaire/path_finder.py @@ -1,3 +1,4 @@ +from copy import deepcopy from typing import Iterable, Mapping, Sequence from werkzeug.datastructures import ImmutableDict @@ -105,7 +106,7 @@ def _build_routing_path_block_ids( when_rules_block_dependencies: list[str], ) -> list[str]: # Keep going unless we've hit the last block - + # routing_path_block_ids can be mutated by _evaluate_routing_rules routing_path_block_ids: list[str] = [] block_index = 0 repeating_list = self.schema.get_repeating_list_for_section( @@ -175,16 +176,16 @@ def _evaluate_routing_rules( routing_path_block_ids: list[str], when_rules_block_dependencies: list[str], ) -> int | None: - if when_rules_block_dependencies: - routing_path_block_ids = ( - when_rules_block_dependencies + routing_path_block_ids - ) + # Use `list` to create a shallow copy since routing_path_block_ids is mutated hence we don't to update its memory reference + block_ids_for_dependencies = list(routing_path_block_ids) + ( + when_rules_block_dependencies + ) when_rule_evaluator = RuleEvaluator( self.schema, self.data_stores, location=this_location, - routing_path_block_ids=routing_path_block_ids, + routing_path_block_ids=block_ids_for_dependencies, ) for rule in routing_rules: rule_valid = ( @@ -224,16 +225,15 @@ def evaluate_skip_conditions( if not skip_conditions: return False - if when_rules_block_dependencies: - routing_path_block_ids = ( - when_rules_block_dependencies + routing_path_block_ids - ) + block_ids_for_dependencies = list(routing_path_block_ids) + ( + when_rules_block_dependencies + ) when_rule_evaluator = RuleEvaluator( schema=self.schema, data_stores=self.data_stores, location=current_location, - routing_path_block_ids=routing_path_block_ids, + routing_path_block_ids=block_ids_for_dependencies, ) return when_rule_evaluator.evaluate(skip_conditions["when"]) diff --git a/schemas/test/en/test_confirmation_question_backwards_routing.json b/schemas/test/en/test_confirmation_question_backwards_routing.json index 53555fb1cc..9bf7cf7188 100644 --- a/schemas/test/en/test_confirmation_question_backwards_routing.json +++ b/schemas/test/en/test_confirmation_question_backwards_routing.json @@ -40,11 +40,11 @@ "sections": [ { "id": "default-section", - "title": "Questions", + "title": "Section 1", "groups": [ { "id": "confirmation", - "title": "Confirmation Question Test", + "title": "Confirmation Driver", "blocks": [ { "type": "Question", @@ -71,7 +71,19 @@ } ] } - }, + } + ] + } + ] + }, + { + "id": "section-2", + "title": "Section 2", + "groups": [ + { + "id": "group=2", + "title": "Confirmation Question", + "blocks": [ { "id": "number-of-employees-total-block", "question": { diff --git a/tests/app/questionnaire/test_path_finder.py b/tests/app/questionnaire/test_path_finder.py index 1a84a43d16..6a55eb9d65 100644 --- a/tests/app/questionnaire/test_path_finder.py +++ b/tests/app/questionnaire/test_path_finder.py @@ -415,7 +415,14 @@ def test_remove_answer_and_block_if_routing_backwards(): "list_item_id": None, "status": CompletionStatus.COMPLETED, "block_ids": [ - "route-backwards-block", + "route-backwards-block" + ], + }, + { + "section_id": "section-2", + "list_item_id": None, + "status": CompletionStatus.COMPLETED, + "block_ids": [ "number-of-employees-total-block", "confirm-zero-employees-block", ], @@ -443,10 +450,10 @@ def test_remove_answer_and_block_if_routing_backwards(): assert ( len( path_finder.data_stores.progress_store.get_completed_block_ids( - SectionKey("default-section") + SectionKey("section-2") ) ) - == 3 + == 2 ) assert len(path_finder.data_stores.answer_store) == 3 @@ -454,17 +461,16 @@ def test_remove_answer_and_block_if_routing_backwards(): expected_path = RoutingPath( block_ids=[ - "route-backwards-block", "number-of-employees-total-block", "confirm-zero-employees-block", "number-of-employees-total-block", ], - section_id="default-section", + section_id="section-2", ) assert routing_path == expected_path assert path_finder.data_stores.progress_store.get_completed_block_ids( - SectionKey("default-section") - ) == progress_store.get_completed_block_ids(SectionKey("default-section")) + SectionKey("section-2") + ) == progress_store.get_completed_block_ids(SectionKey("section-2")) assert len(path_finder.data_stores.answer_store) == 2 assert not path_finder.data_stores.answer_store.get_answer( @@ -472,7 +478,7 @@ def test_remove_answer_and_block_if_routing_backwards(): ) assert ( path_finder.data_stores.progress_store.get_section_status( - SectionKey("default-section") + SectionKey("section-2") ) == CompletionStatus.IN_PROGRESS ) From 9d99b112586b2a21fefc6c82744aa9f242df86a0 Mon Sep 17 00:00:00 2001 From: Mebin Abraham Date: Wed, 24 Jan 2024 17:19:22 +0000 Subject: [PATCH 2/5] Fix group id --- .../test/en/test_confirmation_question_backwards_routing.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/schemas/test/en/test_confirmation_question_backwards_routing.json b/schemas/test/en/test_confirmation_question_backwards_routing.json index 9bf7cf7188..b8bc14b8fe 100644 --- a/schemas/test/en/test_confirmation_question_backwards_routing.json +++ b/schemas/test/en/test_confirmation_question_backwards_routing.json @@ -81,7 +81,7 @@ "title": "Section 2", "groups": [ { - "id": "group=2", + "id": "group-2", "title": "Confirmation Question", "blocks": [ { From fc7a19d44a781eef65f3735166ba6b5a43f4a5d7 Mon Sep 17 00:00:00 2001 From: Mebin Abraham Date: Wed, 24 Jan 2024 17:19:39 +0000 Subject: [PATCH 3/5] Remove unused imports --- app/questionnaire/path_finder.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/questionnaire/path_finder.py b/app/questionnaire/path_finder.py index 19119e4645..0f8301580c 100644 --- a/app/questionnaire/path_finder.py +++ b/app/questionnaire/path_finder.py @@ -1,4 +1,3 @@ -from copy import deepcopy from typing import Iterable, Mapping, Sequence from werkzeug.datastructures import ImmutableDict From 23d3a01c6b2448279ed72e5a46231486c0924cd7 Mon Sep 17 00:00:00 2001 From: Mebin Abraham Date: Wed, 24 Jan 2024 17:33:43 +0000 Subject: [PATCH 4/5] Fix formatting --- app/questionnaire/path_finder.py | 8 ++++---- tests/app/questionnaire/test_path_finder.py | 6 ++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/app/questionnaire/path_finder.py b/app/questionnaire/path_finder.py index 0f8301580c..138fdb9083 100644 --- a/app/questionnaire/path_finder.py +++ b/app/questionnaire/path_finder.py @@ -176,8 +176,8 @@ def _evaluate_routing_rules( when_rules_block_dependencies: list[str], ) -> int | None: # Use `list` to create a shallow copy since routing_path_block_ids is mutated hence we don't to update its memory reference - block_ids_for_dependencies = list(routing_path_block_ids) + ( - when_rules_block_dependencies + block_ids_for_dependencies = ( + list(routing_path_block_ids) + when_rules_block_dependencies ) when_rule_evaluator = RuleEvaluator( @@ -224,8 +224,8 @@ def evaluate_skip_conditions( if not skip_conditions: return False - block_ids_for_dependencies = list(routing_path_block_ids) + ( - when_rules_block_dependencies + block_ids_for_dependencies = ( + list(routing_path_block_ids) + when_rules_block_dependencies ) when_rule_evaluator = RuleEvaluator( diff --git a/tests/app/questionnaire/test_path_finder.py b/tests/app/questionnaire/test_path_finder.py index 6a55eb9d65..f47562f025 100644 --- a/tests/app/questionnaire/test_path_finder.py +++ b/tests/app/questionnaire/test_path_finder.py @@ -414,9 +414,7 @@ def test_remove_answer_and_block_if_routing_backwards(): "section_id": "default-section", "list_item_id": None, "status": CompletionStatus.COMPLETED, - "block_ids": [ - "route-backwards-block" - ], + "block_ids": ["route-backwards-block"], }, { "section_id": "section-2", @@ -426,7 +424,7 @@ def test_remove_answer_and_block_if_routing_backwards(): "number-of-employees-total-block", "confirm-zero-employees-block", ], - } + }, ] ) From 0d7ffa09384768fb80cc7ae2b841bac4d6c399c5 Mon Sep 17 00:00:00 2001 From: Mebin Abraham <35296336+MebinAbraham@users.noreply.github.com> Date: Thu, 25 Jan 2024 09:03:42 +0000 Subject: [PATCH 5/5] Update app/questionnaire/path_finder.py Co-authored-by: Rhys Berrow <47635349+berroar@users.noreply.github.com> --- app/questionnaire/path_finder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/questionnaire/path_finder.py b/app/questionnaire/path_finder.py index 138fdb9083..466c0aaf6c 100644 --- a/app/questionnaire/path_finder.py +++ b/app/questionnaire/path_finder.py @@ -175,7 +175,7 @@ def _evaluate_routing_rules( routing_path_block_ids: list[str], when_rules_block_dependencies: list[str], ) -> int | None: - # Use `list` to create a shallow copy since routing_path_block_ids is mutated hence we don't to update its memory reference + # Use `list` to create a shallow copy since routing_path_block_ids is mutated hence we don't want to update its memory reference block_ids_for_dependencies = ( list(routing_path_block_ids) + when_rules_block_dependencies )