-
Notifications
You must be signed in to change notification settings - Fork 14
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
Resolve Cyclic Imports #1500
Resolve Cyclic Imports #1500
Changes from 77 commits
ec11df9
5943cc7
625ddc6
fd4b580
63a006e
aa2d551
9827787
4618a71
9f21590
aa24fc7
6f22786
951d42f
7f187f9
012f3a4
170f0b9
3c70289
e527b92
ca4e7f7
5b9220c
2469d8b
2acff21
0043e9a
d59afbb
bb42b8c
3151ce4
52878b8
026c8cc
6b9b9e4
62de00c
6673b5b
bf07a26
9de3d1c
4f7f443
cfcaa2b
1409742
5cb5bd8
9b56536
738da58
066670e
a394f86
1771ce9
528b500
d1e050e
149626a
4e592be
8775209
a4bf9e1
0645c23
e60816b
ece8538
9de28d5
12c0071
c638d8e
003fdca
2350cbf
690e247
9d84a27
76d365c
685d565
045bdc3
a54b5c6
568da7b
880a566
51d98ef
66625f8
f0c8fe0
21ce288
61867f2
31584c5
d172410
7f793b9
2b778b0
2f9d91f
016f091
ae64bb2
2e3f96e
162ede8
a7c6386
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -314,19 +314,19 @@ def _get_period_range_for_single_date( | |
data_stores=self.data_stores, | ||
schema=self.schema, | ||
location=self.location, | ||
use_default_answer=True, | ||
list_item_id=list_item_id, | ||
escape_answer_values=False, | ||
) | ||
|
||
rule_evaluator = RuleEvaluator( | ||
value_source_resolver=value_source_resolver, | ||
data_stores=self.data_stores, | ||
schema=self.schema, | ||
location=self.location, | ||
) | ||
|
||
handler = DateHandler( | ||
date_from, value_source_resolver, rule_evaluator, error_messages | ||
) | ||
handler = DateHandler(date_from, rule_evaluator, error_messages) | ||
|
||
min_period_date = handler.get_date_value("minimum") or handler.get_date_value( | ||
"maximum" | ||
|
@@ -480,32 +480,47 @@ def get_answer_fields( | |
if block_ids_by_section: | ||
block_ids = get_flattened_mapping_values(block_ids_by_section) | ||
|
||
def _get_value_source_resolver(list_item: str | None = None) -> ValueSourceResolver: | ||
def _get_value_source_resolver( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this private method does not use vars from the higher scope of the class implicitly (is only passing parameters explicitly), there is no reason why to nest it inside and in the middle of another method of this class. Should be separated out from the parent method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it still uses the higher scope of the function. If not I think the function would not be needed |
||
list_item: str | None = None, | ||
use_default_answer: bool = False, | ||
assess_routing_path: bool = False, | ||
) -> ValueSourceResolver: | ||
return ValueSourceResolver( | ||
data_stores=data_stores, | ||
schema=schema, | ||
location=location, | ||
list_item_id=list_item, | ||
escape_answer_values=False, | ||
routing_path_block_ids=block_ids, | ||
assess_routing_path=False, | ||
assess_routing_path=assess_routing_path, | ||
use_default_answer=use_default_answer, | ||
) | ||
|
||
rule_evaluator = RuleEvaluator( | ||
value_source_resolver=_get_value_source_resolver( | ||
list_item=location.list_item_id if location else None, | ||
use_default_answer=True, | ||
assess_routing_path=True, | ||
), | ||
schema=schema, | ||
data_stores=data_stores, | ||
location=location, | ||
routing_path_block_ids=block_ids, | ||
) | ||
|
||
answer_fields = {} | ||
question_title = question.get("title") | ||
|
||
value_source_resolved_for_location = _get_value_source_resolver(list_item_id) | ||
value_source_resolved_for_location = _get_value_source_resolver( | ||
list_item=list_item_id | ||
) | ||
for answer in question.get("answers", []): | ||
if "list_item_id" in answer: | ||
value_source_resolver = _get_value_source_resolver(answer["list_item_id"]) | ||
rule_evaluator.value_source_resolver = _get_value_source_resolver( | ||
list_item=answer["list_item_id"] | ||
) | ||
else: | ||
value_source_resolver = value_source_resolved_for_location | ||
rule_evaluator.value_source_resolver = value_source_resolved_for_location | ||
|
||
for option in answer.get("options", []): | ||
if "detail_answer" in option: | ||
|
@@ -518,15 +533,13 @@ def _get_value_source_resolver(list_item: str | None = None) -> ValueSourceResol | |
|
||
answer_fields[option["detail_answer"]["id"]] = get_field_handler( | ||
answer_schema=detail_answer, | ||
value_source_resolver=value_source_resolver, | ||
rule_evaluator=rule_evaluator, | ||
error_messages=schema.error_messages, | ||
disable_validation=disable_validation, | ||
question_title=question_title, | ||
).get_field() | ||
answer_fields[answer["id"]] = get_field_handler( | ||
answer_schema=answer, | ||
value_source_resolver=value_source_resolver, | ||
rule_evaluator=rule_evaluator, | ||
error_messages=schema.error_messages, | ||
question_title=question_title, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
from app.questionnaire.questionnaire_schema import QuestionnaireSchema | ||
from app.questionnaire.routing_path import RoutingPath | ||
from app.questionnaire.rules.rule_evaluator import RuleEvaluator, RuleEvaluatorTypes | ||
from app.questionnaire.value_source_resolver import ValueSourceResolver | ||
from app.utilities.types import LocationType | ||
|
||
|
||
|
@@ -181,10 +182,18 @@ def _evaluate_routing_rules( | |
) | ||
|
||
when_rule_evaluator = RuleEvaluator( | ||
self.schema, | ||
self.data_stores, | ||
schema=self.schema, | ||
data_stores=self.data_stores, | ||
location=this_location, | ||
routing_path_block_ids=block_ids_for_dependencies, | ||
value_source_resolver=ValueSourceResolver( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could probably call the same instance of VSR here and on line 245, if we initialised VSR when the PathFinder class instance is first created, but that requires some more thought as we would need to refactor how we initialise PathFinder class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will bring this up in Tech Session once everyone has had a look |
||
list_item_id=this_location.list_item_id if this_location else None, | ||
schema=self.schema, | ||
data_stores=self.data_stores, | ||
location=this_location, | ||
routing_path_block_ids=block_ids_for_dependencies, | ||
use_default_answer=True, | ||
), | ||
) | ||
for rule in routing_rules: | ||
rule_valid = ( | ||
|
@@ -223,7 +232,7 @@ def evaluate_skip_conditions( | |
) -> RuleEvaluatorTypes: | ||
if not skip_conditions: | ||
return False | ||
|
||
list_item_id = current_location.list_item_id if current_location else None | ||
block_ids_for_dependencies = ( | ||
list(routing_path_block_ids) + when_rules_block_dependencies | ||
) | ||
|
@@ -233,6 +242,14 @@ def evaluate_skip_conditions( | |
data_stores=self.data_stores, | ||
location=current_location, | ||
routing_path_block_ids=block_ids_for_dependencies, | ||
value_source_resolver=ValueSourceResolver( | ||
list_item_id=list_item_id, | ||
schema=self.schema, | ||
data_stores=self.data_stores, | ||
routing_path_block_ids=block_ids_for_dependencies, | ||
location=current_location, | ||
use_default_answer=True, | ||
), | ||
) | ||
|
||
return when_rule_evaluator.evaluate(skip_conditions["when"]) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,14 @@ def _resolve_summary_with_calculation( | |
data_stores=self.data_stores, | ||
location=self.location, | ||
routing_path_block_ids=self.routing_path_block_ids, | ||
value_source_resolver=ValueSourceResolver( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We definitely don't want to instantiate another There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @berroar this probably needs some more thought and some kind of structured approach/plan/proposal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this felt akward to do when refactoring, Will have a look |
||
data_stores=self.data_stores, | ||
schema=self.schema, | ||
location=self.location, | ||
list_item_id=self.list_item_id, | ||
routing_path_block_ids=self.routing_path_block_ids, | ||
use_default_answer=True, | ||
), | ||
) | ||
|
||
return evaluator.evaluate(calculation["operation"]) # type: ignore | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some inconsistencies taking this approach, as we seem to mix where we call rule evaluator and where we are calling value source resolver directly. For example here, if we were to do this we'd be better of getting the rule_evaluator as it already has a value source resolver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth having a chat about this approach with the team anyway.