From 03fcfd9cd66bc9e2022a8c18784b33106fe8c7bc Mon Sep 17 00:00:00 2001 From: TOEL2 Date: Tue, 5 Aug 2025 17:11:05 +0100 Subject: [PATCH 1/4] work in progress --- .../services/processors/rule_processor.py | 15 +++++- tests/integration/conftest.py | 34 +++++++++++++ .../in_process/test_eligibility_endpoint.py | 51 +++++++++++++++++++ .../processors/test_rule_processor.py | 41 +++++++++++++++ 4 files changed, 140 insertions(+), 1 deletion(-) diff --git a/src/eligibility_signposting_api/services/processors/rule_processor.py b/src/eligibility_signposting_api/services/processors/rule_processor.py index 05addf584..2e924faca 100644 --- a/src/eligibility_signposting_api/services/processors/rule_processor.py +++ b/src/eligibility_signposting_api/services/processors/rule_processor.py @@ -78,9 +78,22 @@ def is_actionable( priority_getter = attrgetter("priority") suppression_reasons = [] - sorted_rules_by_priority = sorted(self.get_exclusion_rules(cohort, suppression_rules), key=priority_getter) + sorted_rules_by_priority = sorted(suppression_rules, key=priority_getter) for _, rule_group in groupby(sorted_rules_by_priority, key=priority_getter): + group_rules = list(rule_group) + cohort_specific_rules = [rule for rule in group_rules if rule.cohort_label is not None] + matching_specific_rules = [ + rule for rule in cohort_specific_rules if rule.cohort_label == cohort.cohort_label + ] + if cohort_specific_rules and not matching_specific_rules: + continue + + applicable_rules = list(self.get_exclusion_rules(cohort, group_rules)) + + if not applicable_rules: + continue + status, group_exclusion_reasons, rule_stop = self.evaluate_rules_priority_group(person, rule_group) if status.is_exclusion: is_actionable = False diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 82059511c..0acedd6d2 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -480,6 +480,40 @@ def campaign_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generato yield campaign s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") +@pytest.fixture(scope="class") +def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: + campaign: CampaignConfig = rule.CampaignConfigFactory.build( + target="RSV", + iterations=[ + rule.IterationFactory.build( + iteration_rules=[ + rule.PostcodeSuppressionRuleFactory.build(cohort_label="cohort2",), + rule.PersonAgeSuppressionRuleFactory.build(), + ], + iteration_cohorts=[ + rule.IterationCohortFactory.build( + cohort_label="cohort1", + cohort_group="cohort_group1", + positive_description="positive_description", + negative_description="negative_description", + ), + rule.IterationCohortFactory.build( + cohort_label="cohort2", + cohort_group="cohort_group2", + positive_description="positive_description", + negative_description="negative_description", + ) + ], + ) + ], + ) + campaign_data = {"CampaignConfig": campaign.model_dump(by_alias=True)} + s3_client.put_object( + Bucket=rules_bucket, Key=f"{campaign.name}.json", Body=json.dumps(campaign_data), ContentType="application/json" + ) + yield campaign + s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") + @pytest.fixture(scope="class") def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list[CampaignConfig]]: diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index ec911c1fb..b92717c2f 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -238,6 +238,57 @@ def test_actionable( ) + def test_actionable_with_and_rule( + self, + client: FlaskClient, + persisted_person: NHSNumber, + campaign_config_with_and_rule: CampaignConfig, # noqa: ARG002 + ): + # Given + + # When + response = client.get(f"/patient-check/{persisted_person}?includeActions=Y") + + # Then + assert_that( + response, + is_response() + .with_status_code(HTTPStatus.OK) + .and_text( + is_json_that( + has_entry( + "processedSuggestions", + equal_to( + [ + { + "condition": "RSV", + "status": "Actionable", + "eligibilityCohorts": [ + { + "cohortCode": "cohort_group1", + "cohortStatus": "Actionable", + "cohortText": "positive_description", + } + ], + "actions": [ + { + "actionCode": "action_code", + "actionType": "defaultcomms", + "description": "", + "urlLabel": "", + "urlLink": "", + } + ], + "suitabilityRules": [], + "statusText": "You should have the RSV vaccine", + } + ] + ), + ) + ) + ), + ) + class TestMagicCohortResponse: def test_not_eligible_by_rule_when_only_magic_cohort_is_present( self, diff --git a/tests/unit/services/processors/test_rule_processor.py b/tests/unit/services/processors/test_rule_processor.py index b97dea167..09dabb34e 100644 --- a/tests/unit/services/processors/test_rule_processor.py +++ b/tests/unit/services/processors/test_rule_processor.py @@ -144,6 +144,47 @@ def test_evaluate_rules_priority_group_with_rule_stop(mock_rule_calculator_class assert_that(is_rule_stop, is_(True)) +@patch.object(RuleProcessor, "evaluate_rules_priority_group") +def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific_rule( + mock_evaluate_rules_priority_group, + rule_processor, +): + # Person is in COHORT_B + cohort = rule_builder.IterationCohortFactory.build( + cohort_label="COHORT_B", + positive_description="Eligible" + ) + cohort_results = {} + + # Rule 1: cohort-specific to COHORT_A — should be filtered out + rule_specific = rule_builder.IterationRuleFactory.build( + priority=510, + type=RuleType.suppression, + cohort_label="COHORT_A", + name="SPECIFIC_RULE" + ) + + # Rule 2: General rule + rule_general = rule_builder.IterationRuleFactory.build( + priority=510, + type=RuleType.suppression, + cohort_label=None, + name="GENERAL_RULE" + ) + + suppression_rules = [rule_specific, rule_general] + + # Act + rule_processor.is_actionable(MOCK_PERSON_DATA, cohort, cohort_results, suppression_rules) + + # ❌ BUG: General rule should not be evaluated in isolation. + mock_evaluate_rules_priority_group.assert_not_called() + + # Cohort remains actionable + assert_that(cohort_results["COHORT_B"].status, is_(Status.actionable)) + + + @patch.object(RuleProcessor, "evaluate_rules_priority_group") @patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 def test_is_eligible_by_filter_rules_eligible( From a5f5f1e1368bf3a603a3e34beb3f748d2cdea82d Mon Sep 17 00:00:00 2001 From: TOEL2 Date: Thu, 7 Aug 2025 11:10:26 +0100 Subject: [PATCH 2/4] all tests passing --- .../services/processors/rule_processor.py | 4 +++- tests/integration/conftest.py | 7 ++++-- .../in_process/test_eligibility_endpoint.py | 2 +- .../processors/test_rule_processor.py | 22 +++++-------------- 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/src/eligibility_signposting_api/services/processors/rule_processor.py b/src/eligibility_signposting_api/services/processors/rule_processor.py index 2e924faca..47227ec15 100644 --- a/src/eligibility_signposting_api/services/processors/rule_processor.py +++ b/src/eligibility_signposting_api/services/processors/rule_processor.py @@ -94,7 +94,9 @@ def is_actionable( if not applicable_rules: continue - status, group_exclusion_reasons, rule_stop = self.evaluate_rules_priority_group(person, rule_group) + status, group_exclusion_reasons, rule_stop = self.evaluate_rules_priority_group( + person, iter(applicable_rules) + ) if status.is_exclusion: is_actionable = False suppression_reasons.extend(group_exclusion_reasons) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 0acedd6d2..3c57b5028 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -480,6 +480,7 @@ def campaign_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generato yield campaign s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") + @pytest.fixture(scope="class") def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( @@ -487,7 +488,9 @@ def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketNam iterations=[ rule.IterationFactory.build( iteration_rules=[ - rule.PostcodeSuppressionRuleFactory.build(cohort_label="cohort2",), + rule.PostcodeSuppressionRuleFactory.build( + cohort_label="cohort2", + ), rule.PersonAgeSuppressionRuleFactory.build(), ], iteration_cohorts=[ @@ -502,7 +505,7 @@ def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketNam cohort_group="cohort_group2", positive_description="positive_description", negative_description="negative_description", - ) + ), ], ) ], diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index b92717c2f..5290563f1 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -237,7 +237,6 @@ def test_actionable( ), ) - def test_actionable_with_and_rule( self, client: FlaskClient, @@ -289,6 +288,7 @@ def test_actionable_with_and_rule( ), ) + class TestMagicCohortResponse: def test_not_eligible_by_rule_when_only_magic_cohort_is_present( self, diff --git a/tests/unit/services/processors/test_rule_processor.py b/tests/unit/services/processors/test_rule_processor.py index 09dabb34e..32b789254 100644 --- a/tests/unit/services/processors/test_rule_processor.py +++ b/tests/unit/services/processors/test_rule_processor.py @@ -150,26 +150,17 @@ def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific rule_processor, ): # Person is in COHORT_B - cohort = rule_builder.IterationCohortFactory.build( - cohort_label="COHORT_B", - positive_description="Eligible" - ) + cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_B", positive_description="Eligible") cohort_results = {} # Rule 1: cohort-specific to COHORT_A — should be filtered out rule_specific = rule_builder.IterationRuleFactory.build( - priority=510, - type=RuleType.suppression, - cohort_label="COHORT_A", - name="SPECIFIC_RULE" + priority=510, type=RuleType.suppression, cohort_label="COHORT_A", name="SPECIFIC_RULE" ) # Rule 2: General rule rule_general = rule_builder.IterationRuleFactory.build( - priority=510, - type=RuleType.suppression, - cohort_label=None, - name="GENERAL_RULE" + priority=510, type=RuleType.suppression, cohort_label=None, name="GENERAL_RULE" ) suppression_rules = [rule_specific, rule_general] @@ -184,7 +175,6 @@ def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific assert_that(cohort_results["COHORT_B"].status, is_(Status.actionable)) - @patch.object(RuleProcessor, "evaluate_rules_priority_group") @patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 def test_is_eligible_by_filter_rules_eligible( @@ -305,8 +295,8 @@ def test_evaluate_suppression_rules_stops_on_rule_stop( assert_that(cohort_results["COHORT_A"].status, is_(Status.not_actionable)) assert_that(cohort_results["COHORT_A"].reasons, is_([mock_reason_p1])) assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason_p1])) - mock_evaluate_rules_priority_group.assert_called_once() - mock_get_exclusion_rules.assert_called_once_with(cohort, suppression_rules) + assert_that(mock_evaluate_rules_priority_group.call_count, is_(1)) + mock_get_exclusion_rules.assert_called_once_with(cohort, [suppression_rule_p1]) @patch.object(RuleProcessor, "evaluate_rules_priority_group") @@ -338,7 +328,7 @@ def test_evaluate_suppression_rules_does_not_stop_on_rule_stop_when_status_is_ac assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason_p2])) assert_that(mock_evaluate_rules_priority_group.call_count, is_(2)) - assert_that(mock_get_exclusion_rules.call_count, is_(1)) + assert_that(mock_get_exclusion_rules.call_count, is_(2)) def test_is_base_eligible(mock_person_data_reader): From 4fb471173b65650ed150c8e197658a03f33c5dbb Mon Sep 17 00:00:00 2001 From: TOEL2 Date: Thu, 7 Aug 2025 11:38:22 +0100 Subject: [PATCH 3/4] extracting method for readability --- .../services/processors/rule_processor.py | 21 +++++++++---------- .../processors/test_rule_processor.py | 8 +++---- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/eligibility_signposting_api/services/processors/rule_processor.py b/src/eligibility_signposting_api/services/processors/rule_processor.py index 47227ec15..adfdf9c16 100644 --- a/src/eligibility_signposting_api/services/processors/rule_processor.py +++ b/src/eligibility_signposting_api/services/processors/rule_processor.py @@ -82,20 +82,11 @@ def is_actionable( for _, rule_group in groupby(sorted_rules_by_priority, key=priority_getter): group_rules = list(rule_group) - cohort_specific_rules = [rule for rule in group_rules if rule.cohort_label is not None] - matching_specific_rules = [ - rule for rule in cohort_specific_rules if rule.cohort_label == cohort.cohort_label - ] - if cohort_specific_rules and not matching_specific_rules: - continue - - applicable_rules = list(self.get_exclusion_rules(cohort, group_rules)) - - if not applicable_rules: + if self._should_skip_rule_group(cohort, group_rules): continue status, group_exclusion_reasons, rule_stop = self.evaluate_rules_priority_group( - person, iter(applicable_rules) + person, iter(group_rules) ) if status.is_exclusion: is_actionable = False @@ -118,6 +109,14 @@ def is_actionable( suppression_reasons, ) + @staticmethod + def _should_skip_rule_group(cohort: IterationCohort, group_rules: list[IterationRule]) -> bool: + cohort_specific_rules = [rule for rule in group_rules if rule.cohort_label is not None] + matching_specific_rules = [ + rule for rule in cohort_specific_rules if rule.cohort_label == cohort.cohort_label + ] + return bool(cohort_specific_rules and not matching_specific_rules) + def evaluate_rules_priority_group( self, person: Person, rules_group: Iterator[IterationRule] ) -> tuple[eligibility_status.Status, list[eligibility_status.Reason], bool]: diff --git a/tests/unit/services/processors/test_rule_processor.py b/tests/unit/services/processors/test_rule_processor.py index 32b789254..3471b92bf 100644 --- a/tests/unit/services/processors/test_rule_processor.py +++ b/tests/unit/services/processors/test_rule_processor.py @@ -153,12 +153,13 @@ def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_B", positive_description="Eligible") cohort_results = {} - # Rule 1: cohort-specific to COHORT_A — should be filtered out + # Rule 1: Non-matching rule cohort-specific to COHORT_A — should not be evaluated rule_specific = rule_builder.IterationRuleFactory.build( priority=510, type=RuleType.suppression, cohort_label="COHORT_A", name="SPECIFIC_RULE" ) - # Rule 2: General rule + # Rule 2: Matching general rule of the same priority as cohort-specific rule + # - should also not be evaluated rule_general = rule_builder.IterationRuleFactory.build( priority=510, type=RuleType.suppression, cohort_label=None, name="GENERAL_RULE" ) @@ -168,9 +169,8 @@ def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific # Act rule_processor.is_actionable(MOCK_PERSON_DATA, cohort, cohort_results, suppression_rules) - # ❌ BUG: General rule should not be evaluated in isolation. + # None of the rules should be evaluated mock_evaluate_rules_priority_group.assert_not_called() - # Cohort remains actionable assert_that(cohort_results["COHORT_B"].status, is_(Status.actionable)) From b66a8dc3e574e8258e2ae42aa0809ff0780cd4bf Mon Sep 17 00:00:00 2001 From: TOEL2 Date: Thu, 7 Aug 2025 16:37:29 +0100 Subject: [PATCH 4/4] applying to filter rules and adding test --- .../services/processors/rule_processor.py | 16 ++-- .../processors/test_rule_processor.py | 93 ++++++++++++------- 2 files changed, 70 insertions(+), 39 deletions(-) diff --git a/src/eligibility_signposting_api/services/processors/rule_processor.py b/src/eligibility_signposting_api/services/processors/rule_processor.py index adfdf9c16..8b49d2778 100644 --- a/src/eligibility_signposting_api/services/processors/rule_processor.py +++ b/src/eligibility_signposting_api/services/processors/rule_processor.py @@ -50,10 +50,13 @@ def is_eligible( ) -> bool: is_eligible = True priority_getter = attrgetter("priority") - sorted_rules_by_priority = sorted(self.get_exclusion_rules(cohort, filter_rules), key=priority_getter) + sorted_rules_by_priority = sorted(filter_rules, key=priority_getter) for _, rule_group in groupby(sorted_rules_by_priority, key=priority_getter): - status, group_exclusion_reasons, _ = self.evaluate_rules_priority_group(person, rule_group) + group_rules = list(rule_group) + if self._should_skip_rule_group(cohort, group_rules): + continue + status, group_exclusion_reasons, _ = self.evaluate_rules_priority_group(person, iter(group_rules)) if status.is_exclusion: if cohort.cohort_label is not None: cohort_results[cohort.cohort_label] = CohortGroupResult( @@ -65,6 +68,7 @@ def is_eligible( ) is_eligible = False break + return is_eligible def is_actionable( @@ -85,9 +89,7 @@ def is_actionable( if self._should_skip_rule_group(cohort, group_rules): continue - status, group_exclusion_reasons, rule_stop = self.evaluate_rules_priority_group( - person, iter(group_rules) - ) + status, group_exclusion_reasons, rule_stop = self.evaluate_rules_priority_group(person, iter(group_rules)) if status.is_exclusion: is_actionable = False suppression_reasons.extend(group_exclusion_reasons) @@ -112,9 +114,7 @@ def is_actionable( @staticmethod def _should_skip_rule_group(cohort: IterationCohort, group_rules: list[IterationRule]) -> bool: cohort_specific_rules = [rule for rule in group_rules if rule.cohort_label is not None] - matching_specific_rules = [ - rule for rule in cohort_specific_rules if rule.cohort_label == cohort.cohort_label - ] + matching_specific_rules = [rule for rule in cohort_specific_rules if rule.cohort_label == cohort.cohort_label] return bool(cohort_specific_rules and not matching_specific_rules) def evaluate_rules_priority_group( diff --git a/tests/unit/services/processors/test_rule_processor.py b/tests/unit/services/processors/test_rule_processor.py index 3471b92bf..592322770 100644 --- a/tests/unit/services/processors/test_rule_processor.py +++ b/tests/unit/services/processors/test_rule_processor.py @@ -145,7 +145,7 @@ def test_evaluate_rules_priority_group_with_rule_stop(mock_rule_calculator_class @patch.object(RuleProcessor, "evaluate_rules_priority_group") -def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific_rule( +def test_general_suppression_rule_should_not_evaluate_in_isolation_without_matching_specific_rule( mock_evaluate_rules_priority_group, rule_processor, ): @@ -176,9 +176,38 @@ def test_general_rule_should_not_evaluate_in_isolation_without_matching_specific @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +def test_general_filter_rule_should_not_evaluate_in_isolation_without_matching_specific_rule( + mock_evaluate_rules_priority_group, + rule_processor, +): + # Person is in COHORT_B + cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_B", positive_description="Eligible") + cohort_results = {} + + # Rule 1: Non-matching rule cohort-specific to COHORT_A — should not be evaluated + rule_specific = rule_builder.IterationRuleFactory.build( + priority=510, type=RuleType.filter, cohort_label="COHORT_A", name="SPECIFIC_RULE" + ) + + # Rule 2: Matching general rule of the same priority as cohort-specific rule + # - should also not be evaluated + rule_general = rule_builder.IterationRuleFactory.build( + priority=510, type=RuleType.filter, cohort_label=None, name="GENERAL_RULE" + ) + + filter_rules = [rule_specific, rule_general] + + # Act + rule_processor.is_eligible(MOCK_PERSON_DATA, cohort, cohort_results, filter_rules) + + # None of the rules should be evaluated + mock_evaluate_rules_priority_group.assert_not_called() + + +@patch.object(RuleProcessor, "evaluate_rules_priority_group") +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_is_eligible_by_filter_rules_eligible( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A") cohort_results = {} @@ -191,14 +220,14 @@ def test_is_eligible_by_filter_rules_eligible( assert_that(is_eligible, is_(True)) assert_that(cohort_results, is_({})) - mock_get_exclusion_rules.assert_called_once_with(cohort, filter_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, filter_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_is_eligible_by_filter_rules_not_eligible( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A", negative_description="Not Eligible") cohort_results = {} @@ -215,14 +244,14 @@ def test_is_eligible_by_filter_rules_not_eligible( assert_that(cohort_results["COHORT_A"].status, is_(Status.not_eligible)) assert_that(cohort_results["COHORT_A"].description, is_("Not Eligible")) assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason])) - mock_get_exclusion_rules.assert_called_once_with(cohort, filter_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, filter_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_evaluate_suppression_rules_actionable( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A", positive_description="Actionable") cohort_results = {} @@ -238,14 +267,14 @@ def test_evaluate_suppression_rules_actionable( assert_that(cohort_results["COHORT_A"].description, is_("Actionable")) assert_that(cohort_results["COHORT_A"].reasons, is_([])) assert_that(cohort_results["COHORT_A"].audit_rules, is_([])) - mock_get_exclusion_rules.assert_called_once_with(cohort, suppression_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, suppression_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_evaluate_suppression_rules_not_actionable( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build( cohort_label="COHORT_A", positive_description="Positive Description" @@ -264,14 +293,14 @@ def test_evaluate_suppression_rules_not_actionable( assert_that(cohort_results["COHORT_A"].description, is_("Positive Description")) assert_that(cohort_results["COHORT_A"].reasons, is_([mock_reason])) assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason])) - mock_get_exclusion_rules.assert_called_once_with(cohort, suppression_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, suppression_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_evaluate_suppression_rules_stops_on_rule_stop( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A") cohort_results = {} @@ -296,13 +325,13 @@ def test_evaluate_suppression_rules_stops_on_rule_stop( assert_that(cohort_results["COHORT_A"].reasons, is_([mock_reason_p1])) assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason_p1])) assert_that(mock_evaluate_rules_priority_group.call_count, is_(1)) - mock_get_exclusion_rules.assert_called_once_with(cohort, [suppression_rule_p1]) + mock_should_skip_rule_group.assert_called_once_with(cohort, [suppression_rule_p1]) @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_evaluate_suppression_rules_does_not_stop_on_rule_stop_when_status_is_actionable( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A") cohort_results = {} @@ -328,7 +357,7 @@ def test_evaluate_suppression_rules_does_not_stop_on_rule_stop_when_status_is_ac assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason_p2])) assert_that(mock_evaluate_rules_priority_group.call_count, is_(2)) - assert_that(mock_get_exclusion_rules.call_count, is_(2)) + assert_that(mock_should_skip_rule_group.call_count, is_(2)) def test_is_base_eligible(mock_person_data_reader): @@ -395,8 +424,8 @@ def test_rules_get_group_by_types_of_rules(rule_processor): @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 -def test_is_eligible_by_filter_rules(mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor): +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) +def test_is_eligible_by_filter_rules(mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A") cohort_results = {} filter_rule = rule_builder.IterationRuleFactory.build(priority=1, type=RuleType.filter) @@ -408,13 +437,15 @@ def test_is_eligible_by_filter_rules(mock_get_exclusion_rules, mock_evaluate_rul assert_that(is_eligible, is_(True)) assert_that(cohort_results, is_({})) - mock_get_exclusion_rules.assert_called_once_with(cohort, filter_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, filter_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 -def test_is_not_eligible_by_filter_rules(mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor): +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) +def test_is_not_eligible_by_filter_rules( + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor +): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A", negative_description="Not Eligible") cohort_results = {} filter_rule = rule_builder.IterationRuleFactory.build(priority=1, type=RuleType.filter, name="F1") @@ -440,14 +471,14 @@ def mock_evaluate_side_effect(person, rules_group): # noqa: ARG001 assert_that(cohort_results["COHORT_A"].status, is_(Status.not_eligible)) assert_that(cohort_results["COHORT_A"].description, is_("Not Eligible")) assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason])) - mock_get_exclusion_rules.assert_called_once_with(cohort, filter_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, filter_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_is_actionable_by_suppression_rules( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build(cohort_label="COHORT_A", positive_description="Actionable") cohort_results = {} @@ -463,14 +494,14 @@ def test_is_actionable_by_suppression_rules( assert_that(cohort_results["COHORT_A"].description, is_("Actionable")) assert_that(cohort_results["COHORT_A"].reasons, is_(empty())) assert_that(cohort_results["COHORT_A"].audit_rules, is_(empty())) - mock_get_exclusion_rules.assert_called_once_with(cohort, suppression_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, suppression_rules) mock_evaluate_rules_priority_group.assert_called_once() @patch.object(RuleProcessor, "evaluate_rules_priority_group") -@patch.object(RuleProcessor, "get_exclusion_rules", side_effect=lambda cohort, rules_to_filter: rules_to_filter) # noqa: ARG005 +@patch.object(RuleProcessor, "_should_skip_rule_group", return_value=False) def test_is_not_actionable_by_suppression_rules( - mock_get_exclusion_rules, mock_evaluate_rules_priority_group, rule_processor + mock_should_skip_rule_group, mock_evaluate_rules_priority_group, rule_processor ): cohort = rule_builder.IterationCohortFactory.build( cohort_label="COHORT_A", positive_description="Positive Description" @@ -499,7 +530,7 @@ def mock_evaluate_side_effect(person, rules_group): # noqa: ARG001 assert_that(cohort_results["COHORT_A"].description, is_("Positive Description")) assert_that(cohort_results["COHORT_A"].reasons, is_([mock_reason])) assert_that(cohort_results["COHORT_A"].audit_rules, is_([mock_reason])) - mock_get_exclusion_rules.assert_called_once_with(cohort, suppression_rules) + mock_should_skip_rule_group.assert_called_once_with(cohort, suppression_rules) mock_evaluate_rules_priority_group.assert_called_once()