From 09715b9abaf3f4944602fe6655861e8c6459b393 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 8 Aug 2025 16:53:20 +0100 Subject: [PATCH 1/8] changed grouping from name to priority --- .../calculators/eligibility_calculator.py | 8 +- .../test_eligibility_calculator.py | 95 +++++++++++++++++++ 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 985613fcb..dc83d037a 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -28,8 +28,8 @@ from eligibility_signposting_api.model.campaign_config import ( CampaignConfig, CohortLabel, - IterationName, - ) + IterationName, RuleType, +) from eligibility_signposting_api.model.person import Person logger = logging.getLogger(__name__) @@ -165,8 +165,8 @@ def build_condition_results(iteration_result: IterationResult, condition_name: C deduplicated_reasons = [] for cohort in group: for reason in cohort.reasons: - if reason.rule_name not in unique_rule_codes and reason.rule_description: - unique_rule_codes.add(reason.rule_name) + if reason.rule_priority not in unique_rule_codes and reason.rule_description: + unique_rule_codes.add(reason.rule_priority) deduplicated_reasons.append(reason) non_empty_description = next((c.description for c in group if c.description), group[0].description) diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index 610b88437..0f95fe13b 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -845,3 +845,98 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se assert_that(len(result.cohort_results), is_(1)) assert_that(result.cohort_results[0].cohort_code, is_("COHORT_X")) assert_that(result.cohort_results[0].status, is_(Status.not_eligible)) + + @pytest.mark.parametrize( + ("reason_2", "expected_reasons"), + [ + # Same rule name and priority, different description + ( + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("1"), + RuleDescription("Matching"), + matcher_matched=True, + ), + [ # Only reason_1 expected + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("1"), + RuleDescription("Not matching"), + matcher_matched=True, + ) + ], + ), + # Different rule name, same priority + ( + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 2"), + RulePriority("1"), + RuleDescription("Matching"), + matcher_matched=True, + ), + [ # Only reason_1 expected + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("1"), + RuleDescription("Not matching"), + matcher_matched=True, + ) + ], + ), + # Same rule name, different priority + ( + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("2"), + RuleDescription("Matching"), + matcher_matched=True, + ), + [ # Both reasons expected + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("1"), + RuleDescription("Not matching"), + matcher_matched=True, + ), + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("2"), + RuleDescription("Matching"), + matcher_matched=True, + ), + ], + ), + ], + ) + def test_build_condition_results_grouping_reasons(self, reason_2, expected_reasons): + reason_1 = Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("1"), + RuleDescription("Not matching"), + matcher_matched=True, + ) + + cohort_group_results = [ + CohortGroupResult( + "COHORT_Y", + Status.not_actionable, + [reason_1, reason_2], + "Cohort Y Description", + [], + ) + ] + + iteration_result = IterationResult(Status.not_actionable, cohort_group_results, []) + + result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + + assert_that(len(result.cohort_results), is_(1)) + assert_that(result.cohort_results[0].reasons, contains_inanyorder(*expected_reasons)) From beab1dcb56a4f0bd856e841d88a0c2bafc7dfa38 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 8 Aug 2025 17:06:57 +0100 Subject: [PATCH 2/8] changed grouping from name to priority, type --- .../calculators/eligibility_calculator.py | 11 ++--- .../test_eligibility_calculator.py | 44 +++++++++++++++---- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index dc83d037a..32766d8ac 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -28,8 +28,8 @@ from eligibility_signposting_api.model.campaign_config import ( CampaignConfig, CohortLabel, - IterationName, RuleType, -) + IterationName, + ) from eligibility_signposting_api.model.person import Person logger = logging.getLogger(__name__) @@ -161,12 +161,13 @@ def build_condition_results(iteration_result: IterationResult, condition_name: C for group_cohort_code, group in grouped_cohort_results.items(): if group: - unique_rule_codes = set() + unique_reasons = set() deduplicated_reasons = [] for cohort in group: for reason in cohort.reasons: - if reason.rule_priority not in unique_rule_codes and reason.rule_description: - unique_rule_codes.add(reason.rule_priority) + key = (reason.rule_type, reason.rule_priority) + if key not in unique_reasons: + unique_reasons.add(key) deduplicated_reasons.append(reason) non_empty_description = next((c.description for c in group if c.description), group[0].description) diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index 0f95fe13b..0b49fa505 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -735,14 +735,14 @@ def test_build_condition_results_single_condition_single_cohort_not_eligible_wit def test_build_condition_results_single_condition_multiple_cohorts_same_cohort_code_same_status(self): reason_1 = Reason( - RuleType.filter, + RuleType.suppression, eligibility_status.RuleName("Filter Rule 1"), RulePriority("1"), RuleDescription("Filter Rule Description 2"), matcher_matched=True, ) reason_2 = Reason( - RuleType.filter, + RuleType.suppression, eligibility_status.RuleName("Filter Rule 2"), RulePriority("2"), RuleDescription("Filter Rule Description 2"), @@ -779,14 +779,14 @@ def test_build_condition_results_single_condition_multiple_cohorts_same_cohort_c def test_build_condition_results_multiple_cohorts_different_cohort_code_same_status(self): reason_1 = Reason( - RuleType.filter, + RuleType.suppression, eligibility_status.RuleName("Filter Rule 1"), RulePriority("1"), RuleDescription("Filter Rule Description 2"), matcher_matched=True, ) reason_2 = Reason( - RuleType.filter, + RuleType.suppression, eligibility_status.RuleName("Filter Rule 2"), RulePriority("2"), RuleDescription("Filter Rule Description 2"), @@ -820,14 +820,14 @@ def test_build_condition_results_multiple_cohorts_different_cohort_code_same_sta def test_build_condition_results_cohorts_status_not_matching_iteration_status(self): reason_1 = Reason( - RuleType.filter, + RuleType.suppression, eligibility_status.RuleName("Filter Rule 1"), RulePriority("1"), RuleDescription("Matching"), matcher_matched=True, ) reason_2 = Reason( - RuleType.filter, + RuleType.suppression, eligibility_status.RuleName("Filter Rule 2"), RulePriority("2"), RuleDescription("Not matching"), @@ -849,7 +849,7 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se @pytest.mark.parametrize( ("reason_2", "expected_reasons"), [ - # Same rule name and priority, different description + # Same rule name , type, and priority, different description ( Reason( RuleType.suppression, @@ -868,7 +868,7 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se ) ], ), - # Different rule name, same priority + # Different rule name, same type, same priority ( Reason( RuleType.suppression, @@ -887,7 +887,7 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se ) ], ), - # Same rule name, different priority + # Same rule name, same type, different priority ( Reason( RuleType.suppression, @@ -913,6 +913,32 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se ), ], ), + # Same rule name, same priority, different type + ( + Reason( + RuleType.filter, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("2"), + RuleDescription("Matching"), + matcher_matched=True, + ), + [ # Both reasons expected + Reason( + RuleType.suppression, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("1"), + RuleDescription("Not matching"), + matcher_matched=True, + ), + Reason( + RuleType.filter, + eligibility_status.RuleName("Supress Rule 1"), + RulePriority("2"), + RuleDescription("Matching"), + matcher_matched=True, + ), + ], + ), ], ) def test_build_condition_results_grouping_reasons(self, reason_2, expected_reasons): From 179948fc45f75831aa8a28986c778a60f5970729 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Sat, 9 Aug 2025 00:53:20 +0100 Subject: [PATCH 3/8] sonar code complexity fix --- .../calculators/eligibility_calculator.py | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 32766d8ac..be1501d3e 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -157,8 +157,23 @@ def build_condition_results(iteration_result: IterationResult, condition_name: C if iteration_result.status == cohort_result.status: grouped_cohort_results[cohort_result.cohort_code].append(cohort_result) - deduplicated_cohort_results = [] + deduplicated_cohort_results: list[CohortGroupResult] = EligibilityCalculator.deduplicate_cohort_results( + grouped_cohort_results + ) + + return Condition( + condition_name=condition_name, + status=iteration_result.status, + cohort_results=list(deduplicated_cohort_results), + actions=iteration_result.actions, + status_text=iteration_result.status.get_status_text(condition_name), + ) + @staticmethod + def deduplicate_cohort_results( + grouped_cohort_results: dict[str, list[CohortGroupResult]], + ) -> list[CohortGroupResult]: + deduplicated_cohort_results = [] for group_cohort_code, group in grouped_cohort_results.items(): if group: unique_reasons = set() @@ -179,11 +194,4 @@ def build_condition_results(iteration_result: IterationResult, condition_name: C audit_rules=[], ) deduplicated_cohort_results.append(cohort_group_result) - - return Condition( - condition_name=condition_name, - status=iteration_result.status, - cohort_results=list(deduplicated_cohort_results), - actions=iteration_result.actions, - status_text=iteration_result.status.get_status_text(condition_name), - ) + return deduplicated_cohort_results From 9b768b342fb8a701063a64ae9fc220ea0a132d21 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Sat, 9 Aug 2025 01:09:34 +0100 Subject: [PATCH 4/8] sonar code complexity fix --- .../calculators/eligibility_calculator.py | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index be1501d3e..c3734e1ec 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -3,6 +3,7 @@ import logging from collections import defaultdict from dataclasses import dataclass, field +from itertools import chain from typing import TYPE_CHECKING from wireup import service @@ -173,25 +174,28 @@ def build_condition_results(iteration_result: IterationResult, condition_name: C def deduplicate_cohort_results( grouped_cohort_results: dict[str, list[CohortGroupResult]], ) -> list[CohortGroupResult]: - deduplicated_cohort_results = [] - for group_cohort_code, group in grouped_cohort_results.items(): - if group: - unique_reasons = set() - deduplicated_reasons = [] - for cohort in group: - for reason in cohort.reasons: - key = (reason.rule_type, reason.rule_priority) - if key not in unique_reasons: - unique_reasons.add(key) - deduplicated_reasons.append(reason) - - non_empty_description = next((c.description for c in group if c.description), group[0].description) - cohort_group_result = CohortGroupResult( - cohort_code=group_cohort_code, + results = [] + + for cohort_code, group in grouped_cohort_results.items(): + if not group: + continue + + all_reasons = chain.from_iterable(cohort.reasons for cohort in group) + deduped = {} + for reason in all_reasons: + key = (reason.rule_type, reason.rule_priority) + deduped.setdefault(key, reason) + + description = next((c.description for c in group if c.description), group[0].description) + + results.append( + CohortGroupResult( + cohort_code=cohort_code, status=group[0].status, - reasons=deduplicated_reasons, - description=non_empty_description, + reasons=list(deduped.values()), + description=description, audit_rules=[], ) - deduplicated_cohort_results.append(cohort_group_result) - return deduplicated_cohort_results + ) + + return results From a0ec3cdb99734f65b3dd7911e66d105a64aee711 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 11 Aug 2025 13:14:55 +0100 Subject: [PATCH 5/8] updated names in the code for better clarity --- .../services/calculators/eligibility_calculator.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index c3734e1ec..79c4760b0 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -176,22 +176,22 @@ def deduplicate_cohort_results( ) -> list[CohortGroupResult]: results = [] - for cohort_code, group in grouped_cohort_results.items(): - if not group: + for cohort_code, group_results in grouped_cohort_results.items(): + if not group_results: continue - all_reasons = chain.from_iterable(cohort.reasons for cohort in group) + all_reasons = chain.from_iterable(group_result.reasons for group_result in group_results) deduped = {} for reason in all_reasons: key = (reason.rule_type, reason.rule_priority) deduped.setdefault(key, reason) - description = next((c.description for c in group if c.description), group[0].description) + description = next((c.description for c in group_results if c.description), group_results[0].description) results.append( CohortGroupResult( cohort_code=cohort_code, - status=group[0].status, + status=group_results[0].status, reasons=list(deduped.values()), description=description, audit_rules=[], From 69672a41f9748c0c40b910105743cc42caa84b94 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 11 Aug 2025 15:56:26 +0100 Subject: [PATCH 6/8] regroup the suitability tests --- .../model/eligibility_status.py | 1 + .../calculators/eligibility_calculator.py | 13 +++++++--- .../views/eligibility.py | 25 +++++++------------ tests/fixtures/builders/model/eligibility.py | 1 + .../test_eligibility_calculator.py | 17 ++++++++----- 5 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/eligibility_signposting_api/model/eligibility_status.py b/src/eligibility_signposting_api/model/eligibility_status.py index 4026d552a..9cc8809fc 100644 --- a/src/eligibility_signposting_api/model/eligibility_status.py +++ b/src/eligibility_signposting_api/model/eligibility_status.py @@ -113,6 +113,7 @@ class Condition: condition_name: ConditionName status: Status cohort_results: list[CohortGroupResult] + suitability_rules: list[Reason] status_text: StatusText actions: list[SuggestedAction] | None = None diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 79c4760b0..6308ee93d 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -101,8 +101,10 @@ def get_eligibility_status(self, include_actions: str, conditions: list[str], ca condition_results[condition_name] = best_iteration_result.iteration_result condition_results[condition_name].actions = matched_action_detail.actions - condition_result = self.build_condition_results(condition_results[condition_name], condition_name) - final_result.append(condition_result) + condition: Condition = self.build_condition( + iteration_result=condition_results[condition_name], condition_name=condition_name + ) + final_result.append(condition) AuditContext.append_audit_condition( condition_name, @@ -151,7 +153,7 @@ def get_iteration_results(self, campaign_group: list[CampaignConfig]) -> dict[It return iteration_results @staticmethod - def build_condition_results(iteration_result: IterationResult, condition_name: ConditionName) -> Condition: + def build_condition(iteration_result: IterationResult, condition_name: ConditionName) -> Condition: grouped_cohort_results = defaultdict(list) for cohort_result in iteration_result.cohort_results: @@ -162,10 +164,15 @@ def build_condition_results(iteration_result: IterationResult, condition_name: C grouped_cohort_results ) + deduplicated_suitability_rules_for_condition = list( + {(r.rule_type, r.rule_priority): r for g in deduplicated_cohort_results for r in g.reasons}.values() + ) + return Condition( condition_name=condition_name, status=iteration_result.status, cohort_results=list(deduplicated_cohort_results), + suitability_rules=deduplicated_suitability_rules_for_condition, actions=iteration_result.actions, status_text=iteration_result.status.get_status_text(condition_name), ) diff --git a/src/eligibility_signposting_api/views/eligibility.py b/src/eligibility_signposting_api/views/eligibility.py index 7dee05144..383d73d13 100644 --- a/src/eligibility_signposting_api/views/eligibility.py +++ b/src/eligibility_signposting_api/views/eligibility.py @@ -158,22 +158,15 @@ def build_eligibility_cohorts(condition: Condition) -> list[eligibility_response def build_suitability_results(condition: Condition) -> list[eligibility_response.SuitabilityRule]: - """Make only one entry if there are duplicate rules""" if condition.status != Status.not_actionable: return [] - suitability_results = [] - - for cohort_result in condition.cohort_results: - if cohort_result.status == Status.not_actionable: - suitability_results.extend( - eligibility_response.SuitabilityRule( - ruleType=eligibility_response.RuleType(reason.rule_type.value), - ruleCode=eligibility_response.RuleCode(reason.rule_name), - ruleText=eligibility_response.RuleText(reason.rule_description), - ) - for reason in cohort_result.reasons - if reason.rule_description - ) - - return suitability_results + return [ + eligibility_response.SuitabilityRule( + ruleType=eligibility_response.RuleType(reason.rule_type.value), + ruleCode=eligibility_response.RuleCode(reason.rule_name), + ruleText=eligibility_response.RuleText(reason.rule_description), + ) + for reason in condition.suitability_rules + if reason.rule_description + ] diff --git a/tests/fixtures/builders/model/eligibility.py b/tests/fixtures/builders/model/eligibility.py index 894b61c47..7406bc38e 100644 --- a/tests/fixtures/builders/model/eligibility.py +++ b/tests/fixtures/builders/model/eligibility.py @@ -33,6 +33,7 @@ class CohortResultFactory(DataclassFactory[eligibility_status.CohortGroupResult] class ConditionFactory(DataclassFactory[eligibility_status.Condition]): actions = Use(SuggestedActionFactory.batch, size=2) cohort_results = Use(CohortResultFactory.batch, size=2) + suitability_rules = Use(ReasonFactory.batch, size=2) class EligibilityStatusFactory(DataclassFactory[eligibility_status.EligibilityStatus]): diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index 0b49fa505..96038273b 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -26,6 +26,7 @@ ActionDescription, ActionType, CohortGroupResult, + Condition, ConditionName, DateOfBirth, InternalActionCode, @@ -689,7 +690,7 @@ def test_build_condition_results_single_condition_single_cohort_actionable(self) ] iteration_result = IterationResult(Status.actionable, cohort_group_results, suggested_actions) - result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + result = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(result.condition_name, is_(ConditionName("RSV"))) assert_that(result.status, is_(Status.actionable)) @@ -703,6 +704,7 @@ def test_build_condition_results_single_condition_single_cohort_actionable(self) assert_that(deduplicated_cohort.reasons, is_([])) assert_that(deduplicated_cohort.description, is_("Cohort A Description")) assert_that(deduplicated_cohort.audit_rules, is_([])) + assert_that(result.suitability_rules, is_([])) def test_build_condition_results_single_condition_single_cohort_not_eligible_with_reasons(self): cohort_group_results = [CohortGroupResult("COHORT_A", Status.not_eligible, [], "Cohort A Description", [])] @@ -718,7 +720,7 @@ def test_build_condition_results_single_condition_single_cohort_not_eligible_wit ] iteration_result = IterationResult(Status.not_eligible, cohort_group_results, suggested_actions) - result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + result = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(result.condition_name, is_(ConditionName("RSV"))) assert_that(result.status, is_(Status.not_eligible)) @@ -732,6 +734,7 @@ def test_build_condition_results_single_condition_single_cohort_not_eligible_wit assert_that(deduplicated_cohort.reasons, is_([])) assert_that(deduplicated_cohort.description, is_("Cohort A Description")) assert_that(deduplicated_cohort.audit_rules, is_([])) + assert_that(result.suitability_rules, is_([])) def test_build_condition_results_single_condition_multiple_cohorts_same_cohort_code_same_status(self): reason_1 = Reason( @@ -766,7 +769,7 @@ def test_build_condition_results_single_condition_multiple_cohorts_same_cohort_c ] iteration_result = IterationResult(Status.not_eligible, cohort_group_results, suggested_actions) - result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + result: Condition = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(len(result.cohort_results), is_(1)) @@ -776,6 +779,7 @@ def test_build_condition_results_single_condition_multiple_cohorts_same_cohort_c assert_that(deduplicated_cohort.reasons, contains_inanyorder(reason_1, reason_2)) assert_that(deduplicated_cohort.description, is_("Cohort A Description 2")) assert_that(deduplicated_cohort.audit_rules, is_([])) + assert_that(result.suitability_rules, contains_inanyorder(reason_1, reason_2)) def test_build_condition_results_multiple_cohorts_different_cohort_code_same_status(self): reason_1 = Reason( @@ -808,7 +812,7 @@ def test_build_condition_results_multiple_cohorts_different_cohort_code_same_sta ] iteration_result = IterationResult(Status.not_eligible, cohort_group_results, suggested_actions) - result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + result = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(len(result.cohort_results), is_(2)) @@ -840,7 +844,7 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se iteration_result = IterationResult(Status.not_eligible, cohort_group_results, []) - result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + result = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(len(result.cohort_results), is_(1)) assert_that(result.cohort_results[0].cohort_code, is_("COHORT_X")) @@ -962,7 +966,8 @@ def test_build_condition_results_grouping_reasons(self, reason_2, expected_reaso iteration_result = IterationResult(Status.not_actionable, cohort_group_results, []) - result = EligibilityCalculator.build_condition_results(iteration_result, ConditionName("RSV")) + result: Condition = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(len(result.cohort_results), is_(1)) assert_that(result.cohort_results[0].reasons, contains_inanyorder(*expected_reasons)) + assert_that(result.suitability_rules, contains_inanyorder(*expected_reasons)) From 29314bc4d187f7249cb23015a163a15b59a9c8e1 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 11 Aug 2025 17:39:20 +0100 Subject: [PATCH 7/8] fix - ordering regroup the suitability tests --- .../calculators/eligibility_calculator.py | 24 ++- .../test_eligibility_calculator.py | 179 ++++++------------ 2 files changed, 77 insertions(+), 126 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 6308ee93d..66cba4664 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -17,6 +17,7 @@ ConditionName, EligibilityStatus, IterationResult, + Reason, Status, ) from eligibility_signposting_api.services.processors.action_rule_handler import ActionRuleHandler @@ -164,15 +165,15 @@ def build_condition(iteration_result: IterationResult, condition_name: Condition grouped_cohort_results ) - deduplicated_suitability_rules_for_condition = list( - {(r.rule_type, r.rule_priority): r for g in deduplicated_cohort_results for r in g.reasons}.values() + overall_deduplicated_reasons_for_condition = EligibilityCalculator.deduplicate_reasons( + deduplicated_cohort_results ) return Condition( condition_name=condition_name, status=iteration_result.status, cohort_results=list(deduplicated_cohort_results), - suitability_rules=deduplicated_suitability_rules_for_condition, + suitability_rules=list(overall_deduplicated_reasons_for_condition), actions=iteration_result.actions, status_text=iteration_result.status.get_status_text(condition_name), ) @@ -187,11 +188,7 @@ def deduplicate_cohort_results( if not group_results: continue - all_reasons = chain.from_iterable(group_result.reasons for group_result in group_results) - deduped = {} - for reason in all_reasons: - key = (reason.rule_type, reason.rule_priority) - deduped.setdefault(key, reason) + deduped_reasons: list[Reason] = EligibilityCalculator.deduplicate_reasons(group_results) description = next((c.description for c in group_results if c.description), group_results[0].description) @@ -199,10 +196,19 @@ def deduplicate_cohort_results( CohortGroupResult( cohort_code=cohort_code, status=group_results[0].status, - reasons=list(deduped.values()), + reasons=list(deduped_reasons), description=description, audit_rules=[], ) ) return results + + @staticmethod + def deduplicate_reasons(group_results: list[CohortGroupResult]) -> list[Reason]: + all_reasons = chain.from_iterable(group_result.reasons for group_result in group_results) + deduped = {} + for reason in all_reasons: + key = (reason.rule_type, reason.rule_priority) + deduped.setdefault(key, reason) + return list(deduped.values()) diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index 96038273b..cb2dc1695 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -41,6 +41,7 @@ ) from eligibility_signposting_api.services.calculators.eligibility_calculator import EligibilityCalculator from tests.fixtures.builders.model import rule as rule_builder +from tests.fixtures.builders.model.eligibility import ReasonFactory from tests.fixtures.builders.repos.person import person_rows_builder from tests.fixtures.matchers.eligibility import ( is_cohort_result, @@ -850,124 +851,68 @@ def test_build_condition_results_cohorts_status_not_matching_iteration_status(se assert_that(result.cohort_results[0].cohort_code, is_("COHORT_X")) assert_that(result.cohort_results[0].status, is_(Status.not_eligible)) - @pytest.mark.parametrize( - ("reason_2", "expected_reasons"), - [ - # Same rule name , type, and priority, different description - ( - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("1"), - RuleDescription("Matching"), - matcher_matched=True, - ), - [ # Only reason_1 expected - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("1"), - RuleDescription("Not matching"), - matcher_matched=True, - ) - ], - ), - # Different rule name, same type, same priority - ( - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 2"), - RulePriority("1"), - RuleDescription("Matching"), - matcher_matched=True, - ), - [ # Only reason_1 expected - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("1"), - RuleDescription("Not matching"), - matcher_matched=True, - ) - ], - ), - # Same rule name, same type, different priority - ( - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("2"), - RuleDescription("Matching"), - matcher_matched=True, - ), - [ # Both reasons expected - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("1"), - RuleDescription("Not matching"), - matcher_matched=True, - ), - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("2"), - RuleDescription("Matching"), - matcher_matched=True, - ), - ], - ), - # Same rule name, same priority, different type - ( - Reason( - RuleType.filter, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("2"), - RuleDescription("Matching"), - matcher_matched=True, - ), - [ # Both reasons expected - Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("1"), - RuleDescription("Not matching"), - matcher_matched=True, - ), - Reason( - RuleType.filter, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("2"), - RuleDescription("Matching"), - matcher_matched=True, - ), - ], - ), - ], - ) - def test_build_condition_results_grouping_reasons(self, reason_2, expected_reasons): - reason_1 = Reason( - RuleType.suppression, - eligibility_status.RuleName("Supress Rule 1"), - RulePriority("1"), - RuleDescription("Not matching"), - matcher_matched=True, - ) - cohort_group_results = [ - CohortGroupResult( - "COHORT_Y", - Status.not_actionable, - [reason_1, reason_2], - "Cohort Y Description", - [], - ) - ] +@pytest.mark.parametrize( + ("reason_1", "reason_2", "reason_3", "expected_reasons"), + [ + # Same rule name, type, and priority, different description + ( + ReasonFactory.build(rule_description="description1", matcher_matched=True), + ReasonFactory.build(rule_description="description2", matcher_matched=True), + ReasonFactory.build(rule_description="description3", matcher_matched=True), + [ReasonFactory.build(rule_description="description1", matcher_matched=True)], + ), + # Different rule name, same type, same priority + ( + ReasonFactory.build(rule_name="Supress Rule 1", rule_description="description1", matcher_matched=True), + ReasonFactory.build(rule_name="Supress Rule 2", rule_description="description2", matcher_matched=True), + ReasonFactory.build(rule_name="Supress Rule 1", rule_description="description3", matcher_matched=True), + [ReasonFactory.build(rule_name="Supress Rule 1", rule_description="description1", matcher_matched=True)], + ), + # Same rule name, same type, different priority + ( + ReasonFactory.build(rule_priority="1", rule_description="description1", matcher_matched=True), + ReasonFactory.build(rule_priority="2", rule_description="description2", matcher_matched=True), + ReasonFactory.build(rule_priority="1", rule_description="description3", matcher_matched=True), + [ + ReasonFactory.build(rule_priority="1", rule_description="description1", matcher_matched=True), + ReasonFactory.build(rule_priority="2", rule_description="description2", matcher_matched=True), + ], + ), + # Same rule name, same priority, different type + ( + ReasonFactory.build(rule_type=RuleType.suppression, rule_description="description1", matcher_matched=True), + ReasonFactory.build(rule_type=RuleType.filter, rule_description="description2", matcher_matched=True), + ReasonFactory.build(rule_type=RuleType.suppression, rule_description="description3", matcher_matched=True), + [ + ReasonFactory.build( + rule_type=RuleType.suppression, rule_description="description1", matcher_matched=True + ), + ReasonFactory.build(rule_type=RuleType.filter, rule_description="description2", matcher_matched=True), + ], + ), + ], +) +def test_build_condition_results_grouping_reasons(reason_1, reason_2, reason_3, expected_reasons): + cohort_group_results = [ + CohortGroupResult( + "COHORT_X", + Status.not_actionable, + [reason_1, reason_3], + "Cohort X Description", + [], + ), + CohortGroupResult( + "COHORT_Y", + Status.not_actionable, + [reason_2, reason_3], + "Cohort Y Description", + [], + ), + ] - iteration_result = IterationResult(Status.not_actionable, cohort_group_results, []) + iteration_result = IterationResult(Status.not_actionable, cohort_group_results, []) - result: Condition = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) + result: Condition = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) - assert_that(len(result.cohort_results), is_(1)) - assert_that(result.cohort_results[0].reasons, contains_inanyorder(*expected_reasons)) - assert_that(result.suitability_rules, contains_inanyorder(*expected_reasons)) + assert_that(result.suitability_rules, contains_inanyorder(*expected_reasons)) From 0ffb900f4459572e9fd745255ae8d2529e9aa756 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 11 Aug 2025 18:21:33 +0100 Subject: [PATCH 8/8] test for cohort groups --- .../test_eligibility_calculator.py | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/unit/services/calculators/test_eligibility_calculator.py b/tests/unit/services/calculators/test_eligibility_calculator.py index cb2dc1695..37a13e8ff 100644 --- a/tests/unit/services/calculators/test_eligibility_calculator.py +++ b/tests/unit/services/calculators/test_eligibility_calculator.py @@ -916,3 +916,118 @@ def test_build_condition_results_grouping_reasons(reason_1, reason_2, reason_3, result: Condition = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) assert_that(result.suitability_rules, contains_inanyorder(*expected_reasons)) + + +@pytest.mark.parametrize( + ("reason_2", "expected_reasons"), + [ + # Same rule name, type, and priority, different description + ( + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Matching", + rule_name="Supress Rule 1", + rule_priority="1", + matcher_matched=True, + ), + [ + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Not matching", + rule_name="Supress Rule 1", + rule_priority="1", + matcher_matched=True, + ) + ], + ), + # Different rule name + ( + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Matching", + rule_name="Supress Rule 2", + rule_priority="1", + matcher_matched=True, + ), + [ + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Not matching", + rule_name="Supress Rule 1", + rule_priority="1", + matcher_matched=True, + ) + ], + ), + # Different priority + ( + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Matching", + rule_name="Supress Rule 1", + rule_priority="2", + matcher_matched=True, + ), + [ + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Not matching", + rule_name="Supress Rule 1", + rule_priority="1", + matcher_matched=True, + ), + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Matching", + rule_name="Supress Rule 1", + rule_priority="2", + matcher_matched=True, + ), + ], + ), + # Different type + ( + ReasonFactory.build( + rule_type=RuleType.filter, + rule_description="Matching", + rule_name="Supress Rule 1", + rule_priority="2", + matcher_matched=True, + ), + [ + ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Not matching", + rule_name="Supress Rule 1", + rule_priority="1", + matcher_matched=True, + ), + ReasonFactory.build( + rule_type=RuleType.filter, + rule_description="Matching", + rule_name="Supress Rule 1", + rule_priority="2", + matcher_matched=True, + ), + ], + ), + ], +) +def test_build_condition_results_single_cohort(reason_2, expected_reasons): + reason_1 = ReasonFactory.build( + rule_type=RuleType.suppression, + rule_description="Not matching", + rule_name="Supress Rule 1", + rule_priority="1", + matcher_matched=True, + ) + + cohort_group_results = [ + CohortGroupResult("COHORT_Y", Status.not_actionable, [reason_1, reason_2], "Cohort Y Description", []) + ] + + iteration_result = IterationResult(Status.not_actionable, cohort_group_results, []) + result = EligibilityCalculator.build_condition(iteration_result, ConditionName("RSV")) + + assert_that(len(result.cohort_results), is_(1)) + assert_that(result.cohort_results[0].reasons, contains_inanyorder(*expected_reasons))