diff --git a/src/eligibility_signposting_api/services/eligibility_services.py b/src/eligibility_signposting_api/services/eligibility_services.py index cd98285b..a4e5e2f8 100644 --- a/src/eligibility_signposting_api/services/eligibility_services.py +++ b/src/eligibility_signposting_api/services/eligibility_services.py @@ -1,6 +1,8 @@ import logging from collections import defaultdict -from collections.abc import Collection, Mapping +from collections.abc import Collection, Iterator, Mapping +from itertools import groupby +from operator import attrgetter from typing import Any from hamcrest.core.string_description import StringDescription @@ -136,6 +138,8 @@ def evaluate_for_base_eligible_campaigns( or not eligible (due to "S" rules"). For each condition, evaluate all iterations for inclusion or exclusion.""" + priority_getter = attrgetter("priority") + base_eligible_evaluations: dict[ eligibility.ConditionName, dict[eligibility.Status, list[eligibility.Reason]] ] = defaultdict(dict) @@ -144,29 +148,81 @@ def evaluate_for_base_eligible_campaigns( for cc in base_eligible_campaigns if cc.current_iteration ]: - status = eligibility.Status.actionable + # Until we see a worse status, we assume someone is actionable for this iteration. + worst_status_so_far_for_condition = eligibility.Status.actionable exclusion_reasons, actionable_reasons = [], [] - for iteration_rule in iteration.iteration_rules: - if iteration_rule.type not in (rules.RuleType.filter, rules.RuleType.suppression): - continue - exclusion, reason = EligibilityService.evaluate_exclusion(iteration_rule, person_data) - if exclusion: - status = min( - status, - eligibility.Status.not_eligible - if iteration_rule.type == rules.RuleType.filter - else eligibility.Status.not_actionable, + for _priority, iteration_rule_group in groupby( + sorted(iteration.iteration_rules, key=priority_getter), key=priority_getter + ): + worst_status_so_far_for_condition, group_actionable_reasons, group_exclusion_reasons = ( + EligibilityService.evaluate_priority_group( + iteration_rule_group, person_data, worst_status_so_far_for_condition ) - exclusion_reasons.append(reason) - else: - actionable_reasons.append(reason) + ) + actionable_reasons.extend(group_actionable_reasons) + exclusion_reasons.extend(group_exclusion_reasons) condition_entry = base_eligible_evaluations.setdefault(condition_name, {}) - condition_status_entry = condition_entry.setdefault(status, []) + condition_status_entry = condition_entry.setdefault(worst_status_so_far_for_condition, []) condition_status_entry.extend( - actionable_reasons if status is eligibility.Status.actionable else exclusion_reasons + actionable_reasons + if worst_status_so_far_for_condition is eligibility.Status.actionable + else exclusion_reasons ) return base_eligible_evaluations + @staticmethod + def evaluate_priority_group( + iteration_rule_group: Iterator[rules.IterationRule], + person_data: Collection[Mapping[str, Any]], + worst_status_so_far_for_condition: eligibility.Status, + ) -> tuple[eligibility.Status, list[eligibility.Reason], list[eligibility.Reason]]: + actionable_reasons, exclusion_reasons = [], [] + exclude_capable_rules = [ + ir for ir in iteration_rule_group if ir.type in (rules.RuleType.filter, rules.RuleType.suppression) + ] + best_status_so_far_for_priority_group = ( + eligibility.Status.not_eligible if exclude_capable_rules else eligibility.Status.actionable + ) + for iteration_rule in exclude_capable_rules: + exclusion, reason = EligibilityService.evaluate_exclusion(iteration_rule, person_data) + if exclusion: + best_status_so_far_for_priority_group = EligibilityService.best_status( + iteration_rule.type, best_status_so_far_for_priority_group + ) + exclusion_reasons.append(reason) + else: + best_status_so_far_for_priority_group = eligibility.Status.actionable + actionable_reasons.append(reason) + return ( + EligibilityService.worst_status(best_status_so_far_for_priority_group, worst_status_so_far_for_condition), + actionable_reasons, + exclusion_reasons, + ) + + @staticmethod + def worst_status(*statuses: eligibility.Status) -> eligibility.Status: + """Pick the worst status from those given. + + Here "worst" means furthest from being able to access vaccination, so not-eligible is "worse" than + not-actionable, and not-actionable is "worse" than actionable. + """ + return min(statuses) + + @staticmethod + def best_status(rule_type: rules.RuleType, status: eligibility.Status) -> eligibility.Status: + """Pick the best status between the existing status, and the status implied by + the rule excluding the person from vaccination. + + Here "best" means closest to being able to access vaccination, so not-actionable is "better" than + not-eligible, and actionable is "better" than not-actionable. + """ + return max( + status, + eligibility.Status.not_eligible + if rule_type == rules.RuleType.filter + else eligibility.Status.not_actionable, + ) + @staticmethod def get_base_eligible_conditions( base_eligible_evaluations: Mapping[ diff --git a/tests/fixtures/builders/model/rule.py b/tests/fixtures/builders/model/rule.py index 4b72e26d..9ad985eb 100644 --- a/tests/fixtures/builders/model/rule.py +++ b/tests/fixtures/builders/model/rule.py @@ -5,7 +5,6 @@ from polyfactory.factories.pydantic_factory import ModelFactory from eligibility_signposting_api.model import rules -from eligibility_signposting_api.model.rules import RulePriority def past_date(days_behind: int = 365) -> date: @@ -37,10 +36,21 @@ class CampaignConfigFactory(ModelFactory[rules.CampaignConfig]): class PersonAgeSuppressionRuleFactory(IterationRuleFactory): type = rules.RuleType.suppression - name = "Exclude too young less than 75" - description = "Exclude too young less than 75" - priority = RulePriority(10) + name = rules.RuleName("Exclude too young less than 75") + description = rules.RuleDescription("Exclude too young less than 75") + priority = rules.RulePriority(10) operator = rules.RuleOperator.year_gt attribute_level = rules.RuleAttributeLevel.PERSON - attribute_name = "DATE_OF_BIRTH" - comparator = "-75" + attribute_name = rules.RuleAttributeName("DATE_OF_BIRTH") + comparator = rules.RuleComparator("-75") + + +class PostcodeSuppressionRuleFactory(IterationRuleFactory): + type = rules.RuleType.suppression + name = rules.RuleName("In SW19") + description = rules.RuleDescription("In SW19") + priority = rules.RulePriority(10) + operator = rules.RuleOperator.starts_with + attribute_level = rules.RuleAttributeLevel.PERSON + attribute_name = rules.RuleAttributeName("POSTCODE") + comparator = rules.RuleComparator("SW19") diff --git a/tests/fixtures/builders/repos/eligibility.py b/tests/fixtures/builders/repos/eligibility.py index bc5ddd5b..191fb6c1 100644 --- a/tests/fixtures/builders/repos/eligibility.py +++ b/tests/fixtures/builders/repos/eligibility.py @@ -5,21 +5,21 @@ from faker import Faker from eligibility_signposting_api.model import eligibility -from eligibility_signposting_api.model.eligibility import DateOfBirth def eligibility_rows_builder( nhs_number: eligibility.NHSNumber, *, date_of_birth: eligibility.DateOfBirth | None = None, + postcode: eligibility.Postcode | None = None, cohorts: Sequence[str] | None = None, vaccines: Sequence[str] | None = None, ) -> list[dict[str, Any]]: faker = Faker("en_UK") key = f"PERSON#{nhs_number}" - date_of_birth = date_of_birth or DateOfBirth(faker.date_of_birth(minimum_age=18, maximum_age=99)) - postcode = eligibility.Postcode(faker.postcode()) + date_of_birth = date_of_birth or eligibility.DateOfBirth(faker.date_of_birth(minimum_age=18, maximum_age=99)) + postcode = postcode or eligibility.Postcode(faker.postcode()) cohorts = cohorts if cohorts is not None else ["cohort-a", "cohort-b"] vaccines = vaccines if vaccines is not None else ["RSV", "COVID"] rows: list[dict[str, Any]] = [ diff --git a/tests/unit/services/test_eligibility_services.py b/tests/unit/services/test_eligibility_services.py index 613080c8..6821fef1 100644 --- a/tests/unit/services/test_eligibility_services.py +++ b/tests/unit/services/test_eligibility_services.py @@ -6,8 +6,8 @@ from freezegun import freeze_time from hamcrest import assert_that, empty, has_item -from eligibility_signposting_api.model.eligibility import ConditionName, DateOfBirth, NHSNumber, Status -from eligibility_signposting_api.model.rules import IterationDate, RuleType +from eligibility_signposting_api.model.eligibility import ConditionName, DateOfBirth, NHSNumber, Postcode, Status +from eligibility_signposting_api.model.rules import IterationDate, IterationRule, RuleComparator, RulePriority, RuleType from eligibility_signposting_api.repos import EligibilityRepo, NotFoundError, RulesRepo from eligibility_signposting_api.services import EligibilityService, UnknownPersonError from tests.fixtures.builders.model import rule as rule_builder @@ -359,9 +359,15 @@ def test_multiple_rule_types_cause_correct_status(faker: Faker): iterations=[ rule_builder.IterationFactory.build( iteration_rules=[ - rule_builder.PersonAgeSuppressionRuleFactory.build(type=RuleType.suppression), - rule_builder.PersonAgeSuppressionRuleFactory.build(type=RuleType.filter), - rule_builder.PersonAgeSuppressionRuleFactory.build(type=RuleType.suppression), + rule_builder.PersonAgeSuppressionRuleFactory.build( + priority=RulePriority(5), type=RuleType.suppression + ), + rule_builder.PersonAgeSuppressionRuleFactory.build( + priority=RulePriority(10), type=RuleType.filter + ), + rule_builder.PersonAgeSuppressionRuleFactory.build( + priority=RulePriority(15), type=RuleType.suppression + ), ], iteration_cohorts=[rule_builder.IterationCohortFactory.build(cohort_label="cohort1")], ) @@ -382,3 +388,95 @@ def test_multiple_rule_types_cause_correct_status(faker: Faker): has_item(is_condition().with_condition_name(ConditionName("RSV")).and_status(Status.not_eligible)) ), ) + + +@pytest.mark.parametrize( + ("test_comment", "rule1", "rule2", "expected_status"), + [ + ( + "two rules, both exclude, same priority, should exclude", + rule_builder.PersonAgeSuppressionRuleFactory.build(priority=RulePriority(5)), + rule_builder.PostcodeSuppressionRuleFactory.build(priority=RulePriority(5)), + Status.not_actionable, + ), + ( + "two rules, rule 1 excludes, same priority, should allow", + rule_builder.PersonAgeSuppressionRuleFactory.build(priority=RulePriority(5)), + rule_builder.PostcodeSuppressionRuleFactory.build( + priority=RulePriority(5), comparator=RuleComparator("NW1") + ), + Status.actionable, + ), + ( + "two rules, rule 2 excludes, same priority, should allow", + rule_builder.PersonAgeSuppressionRuleFactory.build( + priority=RulePriority(5), comparator=RuleComparator("-65") + ), + rule_builder.PostcodeSuppressionRuleFactory.build(priority=RulePriority(5)), + Status.actionable, + ), + ( + "two rules, rule 1 excludes, different priority, should exclude", + rule_builder.PersonAgeSuppressionRuleFactory.build(priority=RulePriority(5)), + rule_builder.PostcodeSuppressionRuleFactory.build( + priority=RulePriority(10), comparator=RuleComparator("NW1") + ), + Status.not_actionable, + ), + ( + "two rules, rule 2 excludes, different priority, should exclude", + rule_builder.PersonAgeSuppressionRuleFactory.build( + priority=RulePriority(5), comparator=RuleComparator("-65") + ), + rule_builder.PostcodeSuppressionRuleFactory.build(priority=RulePriority(10)), + Status.not_actionable, + ), + ( + "two rules, both excludes, different priority, should exclude", + rule_builder.PersonAgeSuppressionRuleFactory.build(priority=RulePriority(5)), + rule_builder.PostcodeSuppressionRuleFactory.build(priority=RulePriority(10)), + Status.not_actionable, + ), + ], +) +def test_rules_with_same_priority_must_all_match_to_exclude( + test_comment: str, rule1: IterationRule, rule2: IterationRule, expected_status: Status, faker: Faker +): + # Given + nhs_number = NHSNumber(f"5{faker.random_int(max=999999999):09d}") + date_of_birth = DateOfBirth(faker.date_of_birth(minimum_age=66, maximum_age=74)) + + eligibility_repo = MagicMock(spec=EligibilityRepo) + rules_repo = MagicMock(spec=RulesRepo) + eligibility_repo.get_eligibility_data = MagicMock( + return_value=eligibility_rows_builder( + nhs_number, date_of_birth=date_of_birth, postcode=Postcode("SW19 2BH"), cohorts=["cohort1"] + ) + ) + rules_repo.get_campaign_configs = MagicMock( + return_value=[ + rule_builder.CampaignConfigFactory.build( + target="RSV", + iterations=[ + rule_builder.IterationFactory.build( + iteration_rules=[rule1, rule2], + iteration_cohorts=[rule_builder.IterationCohortFactory.build(cohort_label="cohort1")], + ) + ], + ) + ] + ) + + service = EligibilityService(eligibility_repo, rules_repo) + + # When + actual = service.get_eligibility_status(NHSNumber(nhs_number)) + + # Then + assert_that( + actual, + is_eligibility_status().with_conditions( + has_item(is_condition().with_condition_name(ConditionName("RSV")).and_status(expected_status)) + ), + test_comment, + )