Skip to content

Commit 45eb8b1

Browse files
acerathereinspirative-nhs-nviracerathereinspirative-nhs-nvir
authored
Merge pull request #46 from NHSDigital/feature/eli-21-review-feedback
ELI-210: Review feedback
2 parents 607f29a + ce2c02a commit 45eb8b1

File tree

4 files changed

+29
-17
lines changed

4 files changed

+29
-17
lines changed

‎src/eligibility_signposting_api/services/eligibility_services.py‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ def get_attribute_value(iteration_rule: IterationRule, person_data: list[dict[st
7171
match iteration_rule.attribute_level:
7272
case RuleAttributeLevel.PERSON:
7373
person: dict[str, Any] | None = next(
74-
(r for r in person_data if r.get("ATTRIBUTE_TYPE", "").startswith("PERSON")), None
74+
(r for r in person_data if r.get("ATTRIBUTE_TYPE", "") == "PERSON"), None
7575
)
7676
attribute_value = person.get(iteration_rule.attribute_name) if person else None
7777
case _:
@@ -87,13 +87,13 @@ def evaluate_rule(iteration_rule: IterationRule, attribute_value: Any) -> bool:
8787
case RuleOperator.ne:
8888
return attribute_value != iteration_rule.comparator
8989
case RuleOperator.lt:
90-
return int(attribute_value) < int(iteration_rule.comparator)
90+
return int(attribute_value or 0) < int(iteration_rule.comparator)
9191
case RuleOperator.lte:
92-
return int(attribute_value) <= int(iteration_rule.comparator)
92+
return int(attribute_value or 0) <= int(iteration_rule.comparator)
9393
case RuleOperator.gt:
94-
return int(attribute_value) > int(iteration_rule.comparator)
94+
return int(attribute_value or 0) > int(iteration_rule.comparator)
9595
case RuleOperator.gte:
96-
return int(attribute_value) >= int(iteration_rule.comparator)
96+
return int(attribute_value or 0) >= int(iteration_rule.comparator)
9797
case RuleOperator.year_gt:
9898
attribute_date = datetime.strptime(str(attribute_value), "%Y%m%d") if attribute_value else None # noqa: DTZ007
9999
today = datetime.today() # noqa: DTZ002

‎tests/integration/conftest.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,7 +229,7 @@ def persisted_person(eligibility_table: Any, faker: Faker) -> Generator[tuple[NH
229229
eligibility_table.put_item(
230230
Item={
231231
"NHS_NUMBER": f"PERSON#{nhs_number}",
232-
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
232+
"ATTRIBUTE_TYPE": "PERSON",
233233
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
234234
"POSTCODE": postcode,
235235
}
@@ -238,7 +238,7 @@ def persisted_person(eligibility_table: Any, faker: Faker) -> Generator[tuple[NH
238238
Item={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORTS", "COHORT_MAP": {}}
239239
)
240240
yield nhs_number, date_of_birth, postcode
241-
eligibility_table.delete_item(Key={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": f"PERSON#{nhs_number}"})
241+
eligibility_table.delete_item(Key={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "PERSON"})
242242
eligibility_table.delete_item(Key={"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORTS"})
243243

244244

‎tests/integration/repo/test_eligibility_repo.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ def test_person_found(eligibility_table: Any, persisted_person: tuple[NHSNumber,
2323
contains_inanyorder(
2424
{
2525
"NHS_NUMBER": f"PERSON#{nhs_number}",
26-
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
26+
"ATTRIBUTE_TYPE": "PERSON",
2727
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
2828
"POSTCODE": postcode,
2929
},

‎tests/unit/services/test_eligibility_services.py‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,11 @@ def test_simple_rule_eligible(faker: Faker):
5858
return_value=[
5959
{
6060
"NHS_NUMBER": f"PERSON#{nhs_number}",
61-
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
61+
"ATTRIBUTE_TYPE": "PERSON",
6262
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
6363
"POSTCODE": postcode,
64-
}
64+
},
65+
{"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORT", "COHORT_MAP": {}},
6566
]
6667
)
6768
rules_repo.get_campaign_configs = MagicMock(
@@ -113,10 +114,11 @@ def test_simple_rule_ineligible(faker: Faker):
113114
return_value=[
114115
{
115116
"NHS_NUMBER": f"PERSON#{nhs_number}",
116-
"ATTRIBUTE_TYPE": f"PERSON#{nhs_number}",
117+
"ATTRIBUTE_TYPE": "PERSON",
117118
"DATE_OF_BIRTH": date_of_birth.strftime("%Y%m%d"),
118119
"POSTCODE": postcode,
119-
}
120+
},
121+
{"NHS_NUMBER": f"PERSON#{nhs_number}", "ATTRIBUTE_TYPE": "COHORT", "COHORT_MAP": {}},
120122
]
121123
)
122124
rules_repo.get_campaign_configs = MagicMock(
@@ -160,12 +162,16 @@ def test_equals_rule():
160162
rule = IterationRuleFactory.build(operator=RuleOperator.equals, comparator="42")
161163
assert EligibilityService.evaluate_rule(rule, "42")
162164
assert not EligibilityService.evaluate_rule(rule, "99")
165+
assert not EligibilityService.evaluate_rule(rule, "")
166+
assert not EligibilityService.evaluate_rule(rule, None)
163167

164168

165169
def test_not_equals_rule():
166170
rule = IterationRuleFactory.build(operator=RuleOperator.ne, comparator="42")
167171
assert EligibilityService.evaluate_rule(rule, "99")
168172
assert not EligibilityService.evaluate_rule(rule, "42")
173+
assert EligibilityService.evaluate_rule(rule, "")
174+
assert EligibilityService.evaluate_rule(rule, None)
169175

170176

171177
def test_less_than_rule():
@@ -174,27 +180,35 @@ def test_less_than_rule():
174180
assert EligibilityService.evaluate_rule(rule, "99")
175181
assert not EligibilityService.evaluate_rule(rule, "100")
176182
assert not EligibilityService.evaluate_rule(rule, "101")
183+
assert EligibilityService.evaluate_rule(rule, "")
184+
assert EligibilityService.evaluate_rule(rule, None)
177185

178186

179187
def test_less_than_or_equal_rule():
180188
rule = IterationRuleFactory.build(operator=RuleOperator.lte, comparator="100")
181189
assert EligibilityService.evaluate_rule(rule, "99")
182190
assert EligibilityService.evaluate_rule(rule, "100")
183191
assert not EligibilityService.evaluate_rule(rule, "101")
192+
assert EligibilityService.evaluate_rule(rule, "")
193+
assert EligibilityService.evaluate_rule(rule, None)
184194

185195

186196
def test_greater_than_rule():
187197
rule = IterationRuleFactory.build(operator=RuleOperator.gt, comparator="100")
188198
assert EligibilityService.evaluate_rule(rule, "101")
189199
assert not EligibilityService.evaluate_rule(rule, "100")
190200
assert not EligibilityService.evaluate_rule(rule, "99")
201+
assert not EligibilityService.evaluate_rule(rule, "")
202+
assert not EligibilityService.evaluate_rule(rule, None)
191203

192204

193205
def test_greater_than_or_equal_rule():
194206
rule = IterationRuleFactory.build(operator=RuleOperator.gte, comparator="100")
195207
assert EligibilityService.evaluate_rule(rule, "100")
196208
assert EligibilityService.evaluate_rule(rule, "101")
197209
assert not EligibilityService.evaluate_rule(rule, "99")
210+
assert not EligibilityService.evaluate_rule(rule, "")
211+
assert not EligibilityService.evaluate_rule(rule, None)
198212

199213

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

208224

209225
def test_year_gt_rule_past_date():
@@ -213,12 +229,8 @@ def test_year_gt_rule_past_date():
213229
attribute_value = past_date.strftime("%Y%m%d")
214230
rule = IterationRuleFactory.build(operator=RuleOperator.year_gt, comparator=str(years_offset))
215231
assert not EligibilityService.evaluate_rule(rule, attribute_value)
216-
217-
218-
def test_year_gt_rule_empty_value():
219-
rule = IterationRuleFactory.build(operator=RuleOperator.year_gt, comparator="2")
220-
assert not EligibilityService.evaluate_rule(rule, None)
221232
assert not EligibilityService.evaluate_rule(rule, "")
233+
assert not EligibilityService.evaluate_rule(rule, None)
222234

223235

224236
def test_unimplemented_operator():

0 commit comments

Comments
 (0)