Merge pull request #86 from NHSDigital/feature/ELI-254-rule-priority

ELI-254 Implement rule priority

Author: acerathereinspirative-nhs-nvir

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[

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")

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]] = [

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,
+    )