Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion backend/src/models/fhir_immunization_pre_validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,7 @@ def pre_validate_expiration_date(self, values: dict) -> dict:
"""
try:
field_value = values["expirationDate"]
PreValidation.for_date(field_value, "expirationDate")
PreValidation.for_date(field_value, "expirationDate", future_date_allowed=True)
except KeyError:
pass

Expand Down
39 changes: 31 additions & 8 deletions backend/src/models/utils/pre_validator_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from datetime import datetime, timedelta
from decimal import Decimal
from typing import Union
from datetime import datetime, date

from .generic_utils import nhs_number_mod11_check, is_valid_simple_snomed

Expand Down Expand Up @@ -82,7 +83,7 @@ def for_list(
raise ValueError(f"{field_location} must be an array of non-empty objects")

@staticmethod
def for_date(field_value: str, field_location: str):
def for_date(field_value: str, field_location: str, future_date_allowed: bool = False):
"""
Apply pre-validation to a date field to ensure that it is a string (JSON dates must be
written as strings) containing a valid date in the format "YYYY-MM-DD"
Expand All @@ -91,12 +92,16 @@ def for_date(field_value: str, field_location: str):
raise TypeError(f"{field_location} must be a string")

try:
datetime.strptime(field_value, "%Y-%m-%d").date()
parsed_date = datetime.strptime(field_value, "%Y-%m-%d").date()
except ValueError as value_error:
raise ValueError(
f'{field_location} must be a valid date string in the format "YYYY-MM-DD"'
) from value_error

# Enforce future date rule using central checker after successful parse
if not future_date_allowed and PreValidation.check_if_future_date(parsed_date):
raise ValueError(f"{field_location} must not be in the future")

@staticmethod
def for_date_time(field_value: str, field_location: str, strict_timezone: bool = True):
"""
Expand All @@ -116,11 +121,13 @@ def for_date_time(field_value: str, field_location: str, strict_timezone: bool =
"- 'YYYY-MM-DD' — Full date only"
"- 'YYYY-MM-DDThh:mm:ss%z' — Full date and time with timezone (e.g. +00:00 or +01:00)"
"- 'YYYY-MM-DDThh:mm:ss.f%z' — Full date and time with milliseconds and timezone"
"- Date must not be in the future."
)

if strict_timezone:
error_message += "Only '+00:00' and '+01:00' are accepted as valid timezone offsets.\n"
error_message += f"Note that partial dates are not allowed for {field_location} in this service."
error_message += (
"Only '+00:00' and '+01:00' are accepted as valid timezone offsets.\n"
f"Note that partial dates are not allowed for {field_location} in this service.\n"
)

allowed_suffixes = {"+00:00", "+01:00", "+0000", "+0100",}

Expand All @@ -133,10 +140,13 @@ def for_date_time(field_value: str, field_location: str, strict_timezone: bool =
for fmt in formats:
try:
fhir_date = datetime.strptime(field_value, fmt)

# Enforce future-date rule using central checker after successful parse
if PreValidation.check_if_future_date(fhir_date):
raise ValueError(f"{field_location} must not be in the future")
# After successful parse, enforce timezone and future-date rules
if strict_timezone and fhir_date.tzinfo is not None:
if not any(field_value.endswith(suffix) for suffix in allowed_suffixes):
raise ValueError(error_message)
if not any(field_value.endswith(suffix) for suffix in allowed_suffixes):
raise ValueError(error_message)
return fhir_date.isoformat()
except ValueError:
continue
Expand Down Expand Up @@ -234,3 +244,16 @@ def for_nhs_number(nhs_number: str, field_location: str):
"""
if not nhs_number_mod11_check(nhs_number):
raise ValueError(f"{field_location} is not a valid NHS number")

@staticmethod
def check_if_future_date(parsed_value: date | datetime):
"""
Ensure a parsed date or datetime object is not in the future.
"""
if isinstance(parsed_value, datetime):
now = datetime.now(parsed_value.tzinfo) if parsed_value.tzinfo else datetime.now()
elif isinstance(parsed_value, date):
now = datetime.now().date()
if parsed_value > now:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This code will crash if parsed_value is not a datetime or date and now will not be assigned a value. I have checked and your code is always called in a safe way - i.e. it is only ever provided a datetime or date.

However, it might be worth stipulating this can only be called by functions which guarantee the input is date | datetime. Or handling the case where it isn't. Probably the first option is simplest and okay.

return True
return False
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor, but could use a newline.

2 changes: 1 addition & 1 deletion backend/tests/test_immunization_pre_validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ def test_pre_validate_lot_number(self):

def test_pre_validate_expiration_date(self):
"""Test pre_validate_expiration_date accepts valid values and rejects invalid values"""
ValidatorModelTests.test_date_value(self, field_location="expirationDate")
ValidatorModelTests.test_date_value(self, field_location="expirationDate", is_future_date_allowed=True)

def test_pre_validate_site_coding(self):
"""Test pre_validate_site_coding accepts valid values and rejects invalid values"""
Expand Down
17 changes: 15 additions & 2 deletions backend/tests/utils/pre_validation_test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ def test_unique_list(
def test_date_value(
test_instance: unittest.TestCase,
field_location: str,
is_future_date_allowed: bool = False,
):
"""
Test that a FHIR model accepts valid date values and rejects the following invalid values:
Expand Down Expand Up @@ -314,6 +315,15 @@ def test_date_value(
invalid_value=invalid_date_format,
expected_error_message=f"{field_location} must be a valid date string in the " + 'format "YYYY-MM-DD"',
)
if not is_future_date_allowed:
for invalid_date_format in InvalidValues.for_future_dates:
test_invalid_values_rejected(
test_instance,
valid_json_data,
field_location=field_location,
invalid_value=invalid_date_format,
expected_error_message=f"{field_location} must not be in the future",
)

@staticmethod
def test_date_time_value(
Expand All @@ -333,11 +343,14 @@ def test_date_time_value(
"- 'YYYY-MM-DD' — Full date only"
"- 'YYYY-MM-DDThh:mm:ss%z' — Full date and time with timezone (e.g. +00:00 or +01:00)"
"- 'YYYY-MM-DDThh:mm:ss.f%z' — Full date and time with milliseconds and timezone"
"- Date must not be in the future."
)

if is_occurrence_date_time:
expected_error_message += "Only '+00:00' and '+01:00' are accepted as valid timezone offsets.\n"
expected_error_message += f"Note that partial dates are not allowed for {field_location} in this service."
expected_error_message += (
"Only '+00:00' and '+01:00' are accepted as valid timezone offsets.\n"
f"Note that partial dates are not allowed for {field_location} in this service.\n"
)
valid_datetime_formats = ValidValues.for_date_times_strict_timezones
invalid_datetime_formats = InvalidValues.for_date_time_string_formats_for_strict_timezone
else:
Expand Down
8 changes: 7 additions & 1 deletion backend/tests/utils/values_for_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class ValidValues:
for_date_times_strict_timezones = [
"2000-01-01", # Full date only
"2000-01-01T00:00:00+00:00", # Time and offset all zeroes
"2025-05-20T18:26:30+01:00", # Date with Time with no milliseconds and positive offset
"2025-09-24T11:04:30+01:00", # Date with Time with no milliseconds and positive offset
"2000-01-01T00:00:00+01:00", # Time and offset all zeroes
"1933-12-31T11:11:11+01:00", # Positive offset (with hours and minutes not 0)
"1933-12-31T11:11:11.1+00:00", # DateTime with milliseconds to 1 decimal place
Expand Down Expand Up @@ -291,6 +291,12 @@ class InvalidValues:
"2000-02-30", # Invalid combination of month and day
]

for_future_dates = [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not essential, but the future-proof way of doing this would be to generate date strings with strptime() based on offsets from the time now. That would mean we can have reliable test cases for dates which were only months or days in the future as well

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm sorry, only managed to get to review this now due to some internal calls.

This is absolutely mandatory. Even if we are deferring until the year 3000, you never write unit tests that are going to fail at some point and need bumping.

Typically, I would advocate mocking datetime.now(), but the validation tests are such spaghetti, that James' suggestion to use time offsets so we can guarantee it is always a future date would be sufficient.

"2100-01-01", # Year in future
"2050-12-31", # Year in future
"2029-06-15", # Year in future
]

# Strings which are not in acceptable date time format
for_date_time_string_formats_for_relaxed_timezone = [
"", # Empty string
Expand Down
Loading