Skip to content

ELI-210: Review feedback #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
acerathereinspirative-nhs-nvir merged 2 commits into from
Apr 11, 2025
Merged

ELI-210: Review feedback #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c804777
Use attribute type "PERSON", not "PERSON#5123456789".
acerathereinspirative-nhs-nvir Apr 11, 2025
ce2c02a
Ensure rules evaluation does something sensible with empty values and…
acerathereinspirative-nhs-nvir Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/eligibility_signposting_api/services/eligibility_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def get_attribute_value(iteration_rule: IterationRule, person_data: list[dict[st
match iteration_rule.attribute_level:
case RuleAttributeLevel.PERSON:
person: dict[str, Any] | None = next(
(r for r in person_data if r.get("ATTRIBUTE_TYPE", "").startswith("PERSON")), None
(r for r in person_data if r.get("ATTRIBUTE_TYPE", "") == "PERSON"), None
)
attribute_value = person.get(iteration_rule.attribute_name) if person else None
case _:
Expand All @@ -87,13 +87,13 @@ def evaluate_rule(iteration_rule: IterationRule, attribute_value: Any) -> bool:
case RuleOperator.ne:
return attribute_value != iteration_rule.comparator
case RuleOperator.lt:
return int(attribute_value) < int(iteration_rule.comparator)
return int(attribute_value or 0) < int(iteration_rule.comparator)
case RuleOperator.lte:
return int(attribute_value) <= int(iteration_rule.comparator)
return int(attribute_value or 0) <= int(iteration_rule.comparator)
case RuleOperator.gt:
return int(attribute_value) > int(iteration_rule.comparator)
return int(attribute_value or 0) > int(iteration_rule.comparator)
Copy link
Contributor

@jhadfield-NHS jhadfield-NHS Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume this logically matches the handling done in the DPS flavour of the rules.

case RuleOperator.gte:
return int(attribute_value) >= int(iteration_rule.comparator)
return int(attribute_value or 0) >= int(iteration_rule.comparator)
case RuleOperator.year_gt:
attribute_date = datetime.strptime(str(attribute_value), "%Y%m%d") if attribute_value else None # noqa: DTZ007
today = datetime.today() # noqa: DTZ002
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ def persisted_person(eligibility_table: Any, faker: Faker) -> Generator[tuple[NH
eligibility_table.put_item(
Item={
"NHS_NUMBER": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": "PERSON",
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
"POSTCODE": postcode,
}
Expand All @@ -238,7 +238,7 @@ def persisted_person(eligibility_table: Any, faker: Faker) -> Generator[tuple[NH
Item={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORTS", "COHORT_MAP": {}}
)
yield nhs_number, date_of_birth, postcode
eligibility_table.delete_item(Key={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": f"PERSON#{nhs_number}"})
eligibility_table.delete_item(Key={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "PERSON"})
eligibility_table.delete_item(Key={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORTS"})


Expand Down
2 changes: 1 addition & 1 deletion tests/integration/repo/test_eligibility_repo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def test_person_found(eligibility_table: Any, persisted_person: tuple[NHSNumber,
contains_inanyorder(
{
"NHS_NUMBER": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": "PERSON",
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
"POSTCODE": postcode,
},
Expand Down
30 changes: 21 additions & 9 deletions tests/unit/services/test_eligibility_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,11 @@ def test_simple_rule_eligible(faker: Faker):
return_value=[
{
"NHS_NUMBER": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": "PERSON",
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
"POSTCODE": postcode,
}
},
{"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORT", "COHORT_MAP": {}},
]
)
rules_repo.get_campaign_configs = MagicMock(
Expand Down Expand Up @@ -113,10 +114,11 @@ def test_simple_rule_ineligible(faker: Faker):
return_value=[
{
"NHS_NUMBER": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
"ATTRIBUTE_TYPE": "PERSON",
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
"POSTCODE": postcode,
}
},
{"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORT", "COHORT_MAP": {}},
]
)
rules_repo.get_campaign_configs = MagicMock(
Expand Down Expand Up @@ -160,12 +162,16 @@ def test_equals_rule():
rule = IterationRuleFactory.build(operator=RuleOperator.equals, comparator="42")
assert EligibilityService.evaluate_rule(rule, "42")
assert not EligibilityService.evaluate_rule(rule, "99")
assert not EligibilityService.evaluate_rule(rule, "")
assert not EligibilityService.evaluate_rule(rule, None)


def test_not_equals_rule():
rule = IterationRuleFactory.build(operator=RuleOperator.ne, comparator="42")
assert EligibilityService.evaluate_rule(rule, "99")
assert not EligibilityService.evaluate_rule(rule, "42")
assert EligibilityService.evaluate_rule(rule, "")
assert EligibilityService.evaluate_rule(rule, None)


def test_less_than_rule():
Expand All @@ -174,27 +180,35 @@ def test_less_than_rule():
assert EligibilityService.evaluate_rule(rule, "99")
assert not EligibilityService.evaluate_rule(rule, "100")
assert not EligibilityService.evaluate_rule(rule, "101")
assert EligibilityService.evaluate_rule(rule, "")
assert EligibilityService.evaluate_rule(rule, None)


def test_less_than_or_equal_rule():
rule = IterationRuleFactory.build(operator=RuleOperator.lte, comparator="100")
assert EligibilityService.evaluate_rule(rule, "99")
assert EligibilityService.evaluate_rule(rule, "100")
assert not EligibilityService.evaluate_rule(rule, "101")
assert EligibilityService.evaluate_rule(rule, "")
assert EligibilityService.evaluate_rule(rule, None)


def test_greater_than_rule():
rule = IterationRuleFactory.build(operator=RuleOperator.gt, comparator="100")
assert EligibilityService.evaluate_rule(rule, "101")
assert not EligibilityService.evaluate_rule(rule, "100")
assert not EligibilityService.evaluate_rule(rule, "99")
assert not EligibilityService.evaluate_rule(rule, "")
assert not EligibilityService.evaluate_rule(rule, None)


def test_greater_than_or_equal_rule():
rule = IterationRuleFactory.build(operator=RuleOperator.gte, comparator="100")
assert EligibilityService.evaluate_rule(rule, "100")
assert EligibilityService.evaluate_rule(rule, "101")
assert not EligibilityService.evaluate_rule(rule, "99")
assert not EligibilityService.evaluate_rule(rule, "")
assert not EligibilityService.evaluate_rule(rule, None)


def test_year_gt_rule_future_date():
Expand All @@ -204,6 +218,8 @@ def test_year_gt_rule_future_date():
attribute_value = future_date.strftime("%Y%m%d")
rule = IterationRuleFactory.build(operator=RuleOperator.year_gt, comparator=str(years_offset))
assert EligibilityService.evaluate_rule(rule, attribute_value)
assert not EligibilityService.evaluate_rule(rule, "")
assert not EligibilityService.evaluate_rule(rule, None)


def test_year_gt_rule_past_date():
Expand All @@ -213,12 +229,8 @@ def test_year_gt_rule_past_date():
attribute_value = past_date.strftime("%Y%m%d")
rule = IterationRuleFactory.build(operator=RuleOperator.year_gt, comparator=str(years_offset))
assert not EligibilityService.evaluate_rule(rule, attribute_value)


def test_year_gt_rule_empty_value():
rule = IterationRuleFactory.build(operator=RuleOperator.year_gt, comparator="2")
assert not EligibilityService.evaluate_rule(rule, None)
assert not EligibilityService.evaluate_rule(rule, "")
assert not EligibilityService.evaluate_rule(rule, None)


def test_unimplemented_operator():
Expand Down
Loading