diff --git a/src/eligibility_signposting_api/services/processors/derived_values/add_days_handler.py b/src/eligibility_signposting_api/services/processors/derived_values/add_days_handler.py index e65529b9a..b198bf595 100644 --- a/src/eligibility_signposting_api/services/processors/derived_values/add_days_handler.py +++ b/src/eligibility_signposting_api/services/processors/derived_values/add_days_handler.py @@ -27,7 +27,6 @@ class AddDaysHandler(DerivedValueHandler): function_name: str = "ADD_DAYS" - # Mapping of derived attribute names to their source attributes DERIVED_ATTRIBUTE_SOURCES: ClassVar[dict[str, str]] = { "NEXT_DOSE_DUE": "LAST_SUCCESSFUL_DATE", } @@ -62,7 +61,6 @@ def get_source_attribute(self, target_attribute: str, function_args: str | None The source attribute name (e.g., 'LAST_SUCCESSFUL_DATE') """ if function_args and "," in function_args: - # Extract source from args if present (second argument) parts = [p.strip() for p in function_args.split(",")] if len(parts) > 1 and parts[1]: return parts[1].upper() @@ -98,6 +96,9 @@ def calculate(self, context: DerivedValueContext) -> str: def _find_source_date(self, context: DerivedValueContext) -> str | None: """Find the source date value from person data. + For PERSON/COHORT-level attributes, looks for ATTRIBUTE_TYPE == attribute_level. + For TARGET-level attributes, looks for ATTRIBUTE_TYPE == context.attribute_name (e.g., "COVID"). + Args: context: The derived value context @@ -108,8 +109,13 @@ def _find_source_date(self, context: DerivedValueContext) -> str | None: if not source_attr: return None + if context.attribute_level in ("PERSON", "COHORT"): + attribute_type_to_match = context.attribute_level + else: + attribute_type_to_match = context.attribute_name + for attribute in context.person_data: - if attribute.get("ATTRIBUTE_TYPE") == context.attribute_name: + if attribute.get("ATTRIBUTE_TYPE") == attribute_type_to_match: return attribute.get(source_attr) return None @@ -128,7 +134,6 @@ def _get_days_to_add(self, context: DerivedValueContext) -> int: Returns: Number of days to add """ - # Priority 1: Token argument (if non-empty) if context.function_args: args = context.function_args.split(",")[0].strip() if args: @@ -138,11 +143,9 @@ def _get_days_to_add(self, context: DerivedValueContext) -> int: message = f"Invalid days argument '{args}' for ADD_DAYS function. Expected an integer." raise ValueError(message) from e - # Priority 2: Vaccine-specific configuration if context.attribute_name in self.vaccine_type_days: return self.vaccine_type_days[context.attribute_name] - # Priority 3: Default return self.default_days def _add_days_to_date(self, date_str: str, days: int) -> datetime: diff --git a/src/eligibility_signposting_api/services/processors/derived_values/base.py b/src/eligibility_signposting_api/services/processors/derived_values/base.py index 6e75312d1..d3de12cea 100644 --- a/src/eligibility_signposting_api/services/processors/derived_values/base.py +++ b/src/eligibility_signposting_api/services/processors/derived_values/base.py @@ -9,10 +9,12 @@ class DerivedValueContext: Attributes: person_data: List of person attribute dictionaries - attribute_name: The condition/vaccine type (e.g., 'COVID', 'RSV') + attribute_name: The condition/vaccine type (e.g., 'COVID', 'RSV') or person/cohort attribute + (e.g., 'DATE_OF_BIRTH') source_attribute: The source attribute to derive from (e.g., 'LAST_SUCCESSFUL_DATE') function_args: Arguments passed to the function (e.g., number of days) date_format: Optional date format string for output formatting + attribute_level: The level of the attribute ('TARGET', 'PERSON' or 'COHORT') """ person_data: list[dict[str, Any]] @@ -20,6 +22,7 @@ class DerivedValueContext: source_attribute: str | None function_args: str | None date_format: str | None + attribute_level: str = "TARGET" class DerivedValueHandler(ABC): diff --git a/src/eligibility_signposting_api/services/processors/token_processor.py b/src/eligibility_signposting_api/services/processors/token_processor.py index 574b9dd57..a97b69c8e 100644 --- a/src/eligibility_signposting_api/services/processors/token_processor.py +++ b/src/eligibility_signposting_api/services/processors/token_processor.py @@ -90,15 +90,15 @@ def get_token_replacement(token: str, person_data: list[dict], present_attribute if TokenProcessor.should_replace_with_empty(parsed_token, present_attributes): return "" - # Check if this is a derived value (has a function like ADD_DAYS) if parsed_token.function_name: return TokenProcessor.get_derived_value(parsed_token, person_data, present_attributes, token) + TokenProcessor.validate_target_attribute(parsed_token, token) + found_attribute, key_to_replace = TokenProcessor.find_matching_attribute(parsed_token, person_data) if not found_attribute or not key_to_replace: TokenProcessor.handle_token_not_found(parsed_token, token) - # handle_token_not_found always raises, but the type checker needs help msg = "Unreachable" raise RuntimeError(msg) # pragma: no cover @@ -113,6 +113,11 @@ def get_derived_value( ) -> str: """Calculate a derived value using the registered handler. + For TARGET level tokens, validates that the condition is allowed before processing. + If the vaccine type is not in person data, returns an empty string. + For derived values, any target attribute name is allowed (e.g., NEXT_BOOKING_AVAILABLE) + since it's just a placeholder that may be surfaced in the future. + Args: parsed_token: The parsed token containing function information person_data: List of person attribute dictionaries @@ -136,20 +141,14 @@ def get_derived_value( message = f"Unknown function '{function_name}' in token '{token}'." raise ValueError(message) - # For TARGET level tokens, validate the condition is allowed if parsed_token.attribute_level == TARGET_ATTRIBUTE_LEVEL: is_allowed_condition = parsed_token.attribute_name in ALLOWED_CONDITIONS.__args__ - is_allowed_target_attr = parsed_token.attribute_value in ALLOWED_TARGET_ATTRIBUTES - # If condition is not allowed, raise error if not is_allowed_condition: TokenProcessor.handle_token_not_found(parsed_token, token) - # If vaccine type is not in person data but is allowed, return empty if parsed_token.attribute_name not in present_attributes: - if is_allowed_target_attr: - return "" - TokenProcessor.handle_token_not_found(parsed_token, token) + return "" try: target_attribute = parsed_token.attribute_value or parsed_token.attribute_name @@ -165,6 +164,7 @@ def get_derived_value( source_attribute=source_attribute, function_args=parsed_token.function_args, date_format=parsed_token.format, + attribute_level=parsed_token.attribute_level, ) return registry.calculate( @@ -172,7 +172,6 @@ def get_derived_value( context=context, ) except ValueError as e: - # Re-raise with more context message = f"Error calculating derived value for token '{token}': {e}" raise ValueError(message) from e @@ -185,6 +184,26 @@ def should_replace_with_empty(parsed_token: ParsedToken, present_attributes: set return all([is_target_level, is_allowed_condition, is_allowed_target_attr, is_attr_not_present]) + @staticmethod + def validate_target_attribute(parsed_token: ParsedToken, token: str) -> None: + """Validate that target attribute is allowed for non-derived tokens. + + For regular (non-derived) tokens, only allow known target attributes. + Derived values with functions can use any custom target attribute name. + + Args: + parsed_token: The parsed token to validate + token: The original token string for error messages + + Raises: + ValueError: If the target attribute is not in ALLOWED_TARGET_ATTRIBUTES + """ + if ( + parsed_token.attribute_level == TARGET_ATTRIBUTE_LEVEL + and parsed_token.attribute_value not in ALLOWED_TARGET_ATTRIBUTES + ): + TokenProcessor.handle_token_not_found(parsed_token, token) + @staticmethod def find_matching_attribute(parsed_token: ParsedToken, person_data: list[dict]) -> tuple[dict | None, str | None]: attribute_level_map = { diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 9ba0968d3..6be20801c 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1162,6 +1162,48 @@ def campaign_config_with_multiple_add_days( s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") +@pytest.fixture +def campaign_config_with_custom_target_attributes( + s3_client: BaseClient, rules_bucket: BucketName +) -> Generator[CampaignConfig]: + """Campaign config with custom target attribute names for derived values.""" + campaign: CampaignConfig = rule.CampaignConfigFactory.build( + target="COVID", + iterations=[ + rule.IterationFactory.build( + default_comms_routing="CUSTOM_BOOKING_DATE", + actions_mapper=rule.ActionsMapperFactory.build( + root={ + "CUSTOM_BOOKING_DATE": AvailableAction( + ActionType="DataValue", + ExternalRoutingCode="NextBookingAvailable", + ActionDescription=( + "[[TARGET.COVID.NEXT_BOOKING_AVAILABLE:ADD_DAYS(71, LAST_SUCCESSFUL_DATE):" + "DATE(%d %B %Y)]]" + ), + ), + } + ), + iteration_rules=[], + iteration_cohorts=[ + rule.IterationCohortFactory.build( + cohort_label="cohort_label1", + cohort_group="cohort_group1", + positive_description="Positive Description", + negative_description="Negative Description", + ) + ], + ) + ], + ) + campaign_data = {"CampaignConfig": campaign.model_dump(by_alias=True)} + s3_client.put_object( + Bucket=rules_bucket, Key=f"{campaign.name}.json", Body=json.dumps(campaign_data), ContentType="application/json" + ) + yield campaign + s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") + + @pytest.fixture(scope="class") def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list[CampaignConfig]]: """Create and upload multiple campaign configs to S3, then clean up after tests.""" @@ -1521,6 +1563,20 @@ def consumer_to_active_campaign_config_with_multiple_add_days_mapping( s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping.json") +@pytest.fixture +def consumer_to_active_campaign_config_with_custom_target_attributes_mapping( + s3_client: BaseClient, + consumer_mapping_bucket: ConsumerMapping, + campaign_config_with_custom_target_attributes: CampaignConfig, + consumer_id: ConsumerId, +): + consumer_mapping = create_and_put_consumer_mapping_in_s3( + campaign_config_with_custom_target_attributes, consumer_id, consumer_mapping_bucket, s3_client + ) + yield consumer_mapping + s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping.json") + + @pytest.fixture def consumer_to_campaign_having_inactive_iteration_mapping( s3_client: BaseClient, diff --git a/tests/integration/in_process/test_derived_values.py b/tests/integration/in_process/test_derived_values.py index 2996b0daa..4e146eb31 100644 --- a/tests/integration/in_process/test_derived_values.py +++ b/tests/integration/in_process/test_derived_values.py @@ -246,3 +246,65 @@ def test_multiple_actions_with_different_add_days_parameters( has_item(has_entries(actionCode="DateOfNextDoseAt61Days", description="20260330")), ), ) + + +class TestCustomTargetAttributeNames: + """Test that custom target attribute names work with derived values in integration.""" + + def test_custom_target_attribute_with_derived_value( + self, + client: FlaskClient, + person_with_covid_vaccination: NHSNumber, + consumer_id: ConsumerId, + consumer_to_active_campaign_config_with_custom_target_attributes_mapping: ConsumerMapping, # noqa: ARG002 + secretsmanager_client: BaseClient, # noqa: ARG002 + ): + """ + Test that custom target attribute names like NEXT_BOOKING_AVAILABLE work with derived values. + + This tests the issue reported in production where: + [[TARGET.COVID.NEXT_BOOKING_AVAILABLE:ADD_DAYS(71, LAST_SUCCESSFUL_DATE):DATE(%d %B %Y)]] + was raising a ValueError. + + Given: + - A person with COVID vaccination on 2026-01-28 + - A campaign config using a custom target attribute: NEXT_BOOKING_AVAILABLE + - The token: [[TARGET.COVID.NEXT_BOOKING_AVAILABLE:ADD_DAYS(71, LAST_SUCCESSFUL_DATE):DATE(%d %B %Y)]] + + Expected: + - Should calculate 2026-01-28 + 71 days = 2026-04-09 + - Should format as "09 April 2026" + - Should NOT raise ValueError about invalid attribute name + """ + # Given + headers = {"nhs-login-nhs-number": str(person_with_covid_vaccination), UNIQUE_CONSUMER_HEADER: str(consumer_id)} + + # When + response = client.get( + f"/patient-check/{person_with_covid_vaccination}?includeActions=Y", + headers=headers, + ) + + # Then + assert_that( + response, + is_response().with_status_code(HTTPStatus.OK).and_text(is_json_that(has_key("processedSuggestions"))), + ) + + body = response.get_json() + assert_that(body, is_not(none())) + processed_suggestions = body.get("processedSuggestions", []) + + covid_suggestion = next( + (s for s in processed_suggestions if s.get("condition") == "COVID"), + None, + ) + assert_that(covid_suggestion, is_not(none())) + + actions = covid_suggestion.get("actions", []) # type: ignore[union-attr] + + # Verify the custom target attribute with derived value works correctly + assert_that( + actions, + has_item(has_entries(actionCode="NextBookingAvailable", description="09 April 2026")), + ) diff --git a/tests/unit/services/processors/test_derived_values.py b/tests/unit/services/processors/test_derived_values.py index 9fc64c54b..5d82e6afd 100644 --- a/tests/unit/services/processors/test_derived_values.py +++ b/tests/unit/services/processors/test_derived_values.py @@ -260,6 +260,38 @@ def test_function_args_priority_over_vaccine_config(self): # 2025-01-01 + 30 days = 2025-01-31 assert_that(result, is_(equal_to("20250131"))) + def test_calculate_returns_empty_when_source_attribute_is_none(self): + """Test that empty string is returned when source_attribute is None.""" + handler = AddDaysHandler(default_days=91) + context = DerivedValueContext( + person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}], + attribute_name="COVID", + source_attribute=None, # This should cause early return None in _find_source_date + function_args=None, + date_format=None, + attribute_level="TARGET", + ) + + result = handler.calculate(context) + + assert_that(result, is_(equal_to(""))) + + def test_calculate_returns_empty_when_source_attribute_is_empty(self): + """Test that empty string is returned when source_attribute is empty string.""" + handler = AddDaysHandler(default_days=91) + context = DerivedValueContext( + person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}], + attribute_name="COVID", + source_attribute="", # This should cause early return None in _find_source_date + function_args=None, + date_format=None, + attribute_level="TARGET", + ) + + result = handler.calculate(context) + + assert_that(result, is_(equal_to(""))) + class TestDerivedValueRegistry: """Tests for the DerivedValueRegistry class.""" diff --git a/tests/unit/services/processors/test_token_parser.py b/tests/unit/services/processors/test_token_parser.py index 8223b9252..b2a9d2682 100644 --- a/tests/unit/services/processors/test_token_parser.py +++ b/tests/unit/services/processors/test_token_parser.py @@ -1,4 +1,5 @@ import pytest +from hamcrest import assert_that, calling, equal_to, is_, raises from eligibility_signposting_api.services.processors.token_parser import TokenParser @@ -21,10 +22,10 @@ class TestTokenParser: ) def test_parse_valid_tokens(self, token, expected_level, expected_name, expected_value, expected_format): parsed_token = TokenParser.parse(token) - assert parsed_token.attribute_level == expected_level - assert parsed_token.attribute_name == expected_name - assert parsed_token.attribute_value == expected_value - assert parsed_token.format == expected_format + assert_that(parsed_token.attribute_level, is_(equal_to(expected_level))) + assert_that(parsed_token.attribute_name, is_(equal_to(expected_name))) + assert_that(parsed_token.attribute_value, is_(equal_to(expected_value))) + assert_that(parsed_token.format, is_(equal_to(expected_format))) @pytest.mark.parametrize( "token", @@ -38,8 +39,10 @@ def test_parse_valid_tokens(self, token, expected_level, expected_name, expected ], ) def test_parse_invalid_tokens_raises_error(self, token): - with pytest.raises(ValueError, match=r"Invalid token\."): - TokenParser.parse(token) + assert_that( + calling(TokenParser.parse).with_args(token), + raises(ValueError, pattern=r"Invalid token\."), + ) @pytest.mark.parametrize( "token", @@ -52,12 +55,14 @@ def test_parse_invalid_tokens_raises_error(self, token): ], ) def test_parse_invalid_token_format_raises_error(self, token): - with pytest.raises(ValueError, match=r"Invalid token format\."): - TokenParser.parse(token) + assert_that( + calling(TokenParser.parse).with_args(token), + raises(ValueError, pattern=r"Invalid token format\."), + ) def test_parse_function_token_valid(self): """Test that valid function tokens are parsed correctly.""" # This used to be invalid, but now we support custom functions parsed = TokenParser.parse("[[PERSON.DATE_OF_BIRTH:SOME_FUNC(abc)]]") - assert parsed.function_name == "SOME_FUNC" - assert parsed.function_args == "abc" + assert_that(parsed.function_name, is_(equal_to("SOME_FUNC"))) + assert_that(parsed.function_args, is_(equal_to("abc"))) diff --git a/tests/unit/services/processors/test_token_processor.py b/tests/unit/services/processors/test_token_processor.py index 4ce8c0148..964aa5e62 100644 --- a/tests/unit/services/processors/test_token_processor.py +++ b/tests/unit/services/processors/test_token_processor.py @@ -1,6 +1,7 @@ import re import pytest +from hamcrest import assert_that, calling, equal_to, is_, raises from eligibility_signposting_api.model import eligibility_status from eligibility_signposting_api.model.eligibility_status import ( @@ -15,6 +16,7 @@ StatusText, ) from eligibility_signposting_api.model.person import Person +from eligibility_signposting_api.services.processors.token_parser import ParsedToken from eligibility_signposting_api.services.processors.token_processor import TokenProcessor @@ -42,7 +44,7 @@ def test_simple_token_replacement(self): actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual == expected + assert_that(actual, is_(equal_to(expected))) def test_deep_nesting_token_replacement(self): person = Person([{"ATTRIBUTE_TYPE": "PERSON", "AGE": "30", "DEGREE": "DOCTOR", "QUALITY": "NICE"}]) @@ -83,9 +85,9 @@ def test_deep_nesting_token_replacement(self): actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual.cohort_results[0].description == "Results for cohort 30." - assert actual.cohort_results[0].reasons[1].rule_text == "Rule 30 here." - assert actual.status_text == StatusText("Everything is NICE.") + assert_that(actual.cohort_results[0].description, is_(equal_to("Results for cohort 30."))) + assert_that(actual.cohort_results[0].reasons[1].rule_text, is_(equal_to("Rule 30 here."))) + assert_that(actual.status_text, is_(equal_to(StatusText("Everything is NICE.")))) def test_invalid_token_on_person_attribute_should_raise_error(self): person = Person([{"ATTRIBUTE_TYPE": "PERSON", "AGE": "30"}]) @@ -101,8 +103,10 @@ def test_invalid_token_on_person_attribute_should_raise_error(self): expected_error = re.escape("Invalid attribute name 'ICECREAM' in token '[[PERSON.ICECREAM]]'.") - with pytest.raises(ValueError, match=expected_error): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=expected_error), + ) def test_invalid_token_should_raise_error(self): person = Person([{"ATTRIBUTE_TYPE": "PERSON", "AGE": "30"}]) @@ -117,8 +121,10 @@ def test_invalid_token_should_raise_error(self): ) expected_error = re.escape("Invalid attribute level 'ICECREAM' in token '[[ICECREAM.FLAVOR]]'.") - with pytest.raises(ValueError, match=expected_error): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=expected_error), + ) def test_invalid_token_on_target_attribute_should_raise_error(self): person = Person([{"ATTRIBUTE_TYPE": "RSV", "LAST_SUCCESSFUL_DATE": "20250101"}]) @@ -133,8 +139,10 @@ def test_invalid_token_on_target_attribute_should_raise_error(self): ) expected_error = re.escape("Invalid attribute name 'ICECREAM' in token '[[TARGET.RSV.ICECREAM]]'.") - with pytest.raises(ValueError, match=expected_error): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=expected_error), + ) def test_missing_target_attribute_and_invalid_token_should_raise_error(self): person = Person([{"ATTRIBUTE_TYPE": "PERSON", "AGE": "30"}]) @@ -149,8 +157,10 @@ def test_missing_target_attribute_and_invalid_token_should_raise_error(self): ) expected_error = re.escape("Invalid attribute name 'ICECREAM' in token '[[TARGET.RSV.ICECREAM]]'.") - with pytest.raises(ValueError, match=expected_error): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=expected_error), + ) def test_missing_patient_vaccine_data_on_target_attribute_should_replace_with_empty(self): person = Person([{"ATTRIBUTE_TYPE": "PERSON", "AGE": "30"}]) @@ -166,7 +176,7 @@ def test_missing_patient_vaccine_data_on_target_attribute_should_replace_with_em actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual.condition_name == "Last successful date: " + assert_that(actual.condition_name, is_(equal_to("Last successful date: "))) def test_not_allowed_target_conditions_token_should_raise_error(self): person = Person( @@ -188,8 +198,10 @@ def test_not_allowed_target_conditions_token_should_raise_error(self): expected_error = re.escape( "Invalid attribute name 'LAST_SUCCESSFUL_DATE' in token '[[TARGET.YELLOW_FEVER.LAST_SUCCESSFUL_DATE]]'." ) - with pytest.raises(ValueError, match=expected_error): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=expected_error), + ) def test_valid_token_but_missing_attribute_data_to_replace(self): person = Person( @@ -222,8 +234,8 @@ def test_valid_token_but_missing_attribute_data_to_replace(self): actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual.status_text == expected.status_text - assert actual.condition_name == expected.condition_name + assert_that(actual.status_text, is_(equal_to(expected.status_text))) + assert_that(actual.condition_name, is_(equal_to(expected.condition_name))) def test_valid_token_but_missing_attribute_in_multiple_vacc_data_to_replace(self): person = Person( @@ -258,8 +270,8 @@ def test_valid_token_but_missing_attribute_in_multiple_vacc_data_to_replace(self actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual.status_text == expected.status_text - assert actual.condition_name == expected.condition_name + assert_that(actual.status_text, is_(equal_to(expected.status_text))) + assert_that(actual.condition_name, is_(equal_to(expected.condition_name))) def test_simple_string_with_multiple_tokens(self): person = Person( @@ -291,7 +303,7 @@ def test_simple_string_with_multiple_tokens(self): actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual == expected + assert_that(actual, is_(equal_to(expected))) def test_valid_token_valid_format_should_replace_with_date_formatting(self): person = Person( @@ -324,8 +336,8 @@ def test_valid_token_valid_format_should_replace_with_date_formatting(self): actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual.condition_name == expected.condition_name - assert actual.status_text == expected.status_text + assert_that(actual.condition_name, is_(equal_to(expected.condition_name))) + assert_that(actual.status_text, is_(equal_to(expected.status_text))) @pytest.mark.parametrize( "token_format", @@ -347,8 +359,10 @@ def test_valid_token_invalid_format_should_raise_error(self, token_format: str): actions=[], ) - with pytest.raises(ValueError, match=r"Invalid token format\."): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=r"Invalid token format\."), + ) @pytest.mark.parametrize( ("token_format", "func_name"), @@ -372,8 +386,10 @@ def test_unknown_function_raises_error(self, token_format: str, func_name: str): actions=[], ) - with pytest.raises(ValueError, match=f"Unknown function '{func_name}'"): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=f"Unknown function '{func_name}'"), + ) @pytest.mark.parametrize( ("token_format", "expected"), @@ -408,7 +424,7 @@ def test_valid_date_format(self, token_format: str, expected: str): actual = TokenProcessor.find_and_replace_tokens(person, condition) - assert actual.condition_name == f"Date: {expected}" + assert_that(actual.condition_name, is_(equal_to(f"Date: {expected}"))) @pytest.mark.parametrize( ("token", "expected"), @@ -442,5 +458,281 @@ def test_token_replace_is_case_insensitive(self, token: str, expected: str): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == f"Your DOB is: {expected}." - assert result.condition_name == f"FLU vaccine on: {expected}." + assert_that(result.status_text, is_(equal_to(f"Your DOB is: {expected}."))) + assert_that(result.condition_name, is_(equal_to(f"FLU vaccine on: {expected}."))) + + +class TestCustomTargetAttributeNames: + """Test that custom target attribute names work with derived values.""" + + def test_custom_target_attribute_with_add_days_when_data_present(self): + """Test that custom target attributes like NEXT_BOOKING_AVAILABLE work when data is present.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20260128"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText( + "Next booking: [[TARGET.COVID.NEXT_BOOKING_AVAILABLE:ADD_DAYS(71, LAST_SUCCESSFUL_DATE)]]" + ), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + # 2026-01-28 + 71 days = 2026-04-09 + assert_that(result.status_text, is_(equal_to("Next booking: 20260409"))) + + def test_custom_target_attribute_with_add_days_and_formatting(self): + """Test that custom target attributes work with both ADD_DAYS and DATE formatting.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20260128"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText( + "Date: [[TARGET.COVID.NEXT_BOOKING_AVAILABLE:ADD_DAYS(71, LAST_SUCCESSFUL_DATE):DATE(%d %B %Y)]]" + ), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + # 2026-01-28 + 71 days = 2026-04-09, formatted as "09 April 2026" + assert_that(result.status_text, is_(equal_to("Date: 09 April 2026"))) + + def test_custom_target_attribute_returns_empty_when_condition_not_present(self): + """Test that custom target attributes return empty string when condition data not present.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "PERSON", "AGE": "30"}, + # No COVID data present + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText( + "Next booking: [[TARGET.COVID.NEXT_BOOKING_AVAILABLE:ADD_DAYS(71, LAST_SUCCESSFUL_DATE)]]" + ), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + # Should return empty string when condition data is not present + assert_that(result.status_text, is_(equal_to("Next booking: "))) + + def test_multiple_custom_target_attributes_with_different_functions(self): + """Test multiple custom target attributes with different parameters.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20260128"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText( + "First: [[TARGET.COVID.CUSTOM_FIELD_A:ADD_DAYS(30, LAST_SUCCESSFUL_DATE)]] " + "Second: [[TARGET.COVID.CUSTOM_FIELD_B:ADD_DAYS(60, LAST_SUCCESSFUL_DATE)]]" + ), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + # 2026-01-28 + 30 = 2026-02-27, + 60 = 2026-03-29 + assert_that(result.status_text, is_(equal_to("First: 20260227 Second: 20260329"))) + + def test_custom_target_attribute_raises_error_for_invalid_condition(self): + """Test that invalid condition names still raise errors even with custom target attributes.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "PERSON", "AGE": "30"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText("Invalid: [[TARGET.INVALID_CONDITION.CUSTOM_FIELD:ADD_DAYS(30)]]"), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern="Invalid attribute name 'CUSTOM_FIELD'"), + ) + + def test_non_derived_token_with_invalid_target_attribute_raises_error(self): + """Test that non-derived tokens (without functions) validate target attributes strictly.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20260128"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText("Invalid: [[TARGET.COVID.CUSTOM_INVALID_FIELD]]"), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + # Non-derived tokens should only allow ALLOWED_TARGET_ATTRIBUTES + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern="Invalid attribute name 'CUSTOM_INVALID_FIELD'"), + ) + + def test_non_derived_token_with_valid_target_attribute_works(self): + """Test that non-derived tokens with valid target attributes work correctly.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20260128"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText("Last date: [[TARGET.COVID.LAST_SUCCESSFUL_DATE]]"), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + assert_that(result.status_text, is_(equal_to("Last date: 20260128"))) + + def test_person_level_attribute_with_add_days_without_explicit_source(self): + """Test that ADD_DAYS works on PERSON-level attributes without explicit source.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "PERSON", "DATE_OF_BIRTH": "19900327"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText("Future date: [[PERSON.DATE_OF_BIRTH:ADD_DAYS(91)]]"), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + # 1990-03-27 + 91 days = 1990-06-26 + assert_that(result.status_text, is_(equal_to("Future date: 19900626"))) + + def test_person_level_attribute_with_add_days_explicit_source(self): + """Test that ADD_DAYS works on PERSON-level attributes with explicit source.""" + person = Person( + [ + {"ATTRIBUTE_TYPE": "PERSON", "DATE_OF_BIRTH": "19900327"}, + ] + ) + + condition = Condition( + condition_name=ConditionName("Test"), + status=Status.actionable, + status_text=StatusText("Future date: [[PERSON.DATE_OF_BIRTH:ADD_DAYS(91, DATE_OF_BIRTH)]]"), + cohort_results=[], + suitability_rules=[], + actions=[], + ) + + result = TokenProcessor.find_and_replace_tokens(person, condition) + + # 1990-03-27 + 91 days = 1990-06-26 + assert_that(result.status_text, is_(equal_to("Future date: 19900626"))) + + def test_derived_value_with_no_function_name_raises_error(self): + """Test that derived tokens without function name raise ValueError.""" + # This would happen if token parsing goes wrong somehow + parsed_token = ParsedToken( + attribute_level="TARGET", + attribute_name="COVID", + attribute_value="NEXT_DOSE_DUE", + function_name=None, # This should cause the error + function_args=None, + format=None, + ) + + assert_that( + calling(TokenProcessor.get_derived_value).with_args( + parsed_token=parsed_token, + person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}], + present_attributes={"COVID"}, + token="[[TARGET.COVID.NEXT_DOSE_DUE:]]", # Malformed token + ), + raises(ValueError, pattern="No function specified in token"), + ) + + def test_derived_value_with_unknown_function_raises_error(self): + """Test that derived tokens with unknown function raise ValueError.""" + parsed_token = ParsedToken( + attribute_level="TARGET", + attribute_name="COVID", + attribute_value="NEXT_DOSE_DUE", + function_name="UNKNOWN_FUNCTION", # This function doesn't exist + function_args="30", + format=None, + ) + + assert_that( + calling(TokenProcessor.get_derived_value).with_args( + parsed_token=parsed_token, + person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}], + present_attributes={"COVID"}, + token="[[TARGET.COVID.NEXT_DOSE_DUE:UNKNOWN_FUNCTION(30)]]", + ), + raises(ValueError, pattern="Unknown function 'UNKNOWN_FUNCTION' in token"), + ) + + def test_derived_value_handler_exception_gets_wrapped(self): + """Test that exceptions from derived value handlers are wrapped with context.""" + parsed_token = ParsedToken( + attribute_level="TARGET", + attribute_name="COVID", + attribute_value="NEXT_DOSE_DUE", + function_name="ADD_DAYS", + function_args="invalid_arg", # This will cause the handler to raise ValueError + format=None, + ) + + assert_that( + calling(TokenProcessor.get_derived_value).with_args( + parsed_token=parsed_token, + person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}], + present_attributes={"COVID"}, + token="[[TARGET.COVID.NEXT_DOSE_DUE:ADD_DAYS(invalid_arg)]]", + ), + raises(ValueError, pattern=r"Error calculating derived value for token.*Invalid days argument"), + ) diff --git a/tests/unit/services/processors/test_token_processor_derived.py b/tests/unit/services/processors/test_token_processor_derived.py index b72fd7a9f..1111fecd2 100644 --- a/tests/unit/services/processors/test_token_processor_derived.py +++ b/tests/unit/services/processors/test_token_processor_derived.py @@ -2,7 +2,7 @@ import re -import pytest +from hamcrest import assert_that, calling, equal_to, is_, raises from eligibility_signposting_api.model.eligibility_status import ( Condition, @@ -37,7 +37,7 @@ def test_next_dose_due_basic_replacement(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 2025-01-01 + 91 days = 2025-04-02 - assert result.status_text == "Next dose due: 20250402" + assert_that(result.status_text, is_(equal_to("Next dose due: 20250402"))) def test_next_dose_due_with_date_format(self): """Test NEXT_DOSE_DUE with date formatting.""" @@ -58,7 +58,7 @@ def test_next_dose_due_with_date_format(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "You can book from 02 April 2025" + assert_that(result.status_text, is_(equal_to("You can book from 02 April 2025"))) def test_next_dose_due_different_days(self): """Test NEXT_DOSE_DUE with different number of days.""" @@ -80,7 +80,7 @@ def test_next_dose_due_different_days(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 2025-06-01 + 365 days = 2026-06-01 - assert result.status_text == "Next dose: 01/06/2026" + assert_that(result.status_text, is_(equal_to("Next dose: 01/06/2026"))) def test_missing_vaccine_data_returns_empty(self): """Test that missing vaccine data returns empty string for derived values.""" @@ -101,7 +101,7 @@ def test_missing_vaccine_data_returns_empty(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.condition_name == "Next COVID dose: " + assert_that(result.condition_name, is_(equal_to("Next COVID dose: "))) def test_missing_last_successful_date_returns_empty(self): """Test that missing source date returns empty string.""" @@ -122,7 +122,7 @@ def test_missing_last_successful_date_returns_empty(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "Next dose: " + assert_that(result.status_text, is_(equal_to("Next dose: "))) def test_custom_target_without_mapping_returns_empty(self): """Test unknown target without source override returns empty string.""" @@ -143,7 +143,7 @@ def test_custom_target_without_mapping_returns_empty(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "Due: " + assert_that(result.status_text, is_(equal_to("Due: "))) def test_custom_target_with_source_override_uses_override(self): """Test custom target with explicit source override derives date.""" @@ -164,7 +164,7 @@ def test_custom_target_with_source_override_uses_override(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "Next dose: 20250131" + assert_that(result.status_text, is_(equal_to("Next dose: 20250131"))) def test_mixed_regular_and_derived_tokens(self): """Test mixing regular tokens with derived value tokens.""" @@ -189,7 +189,7 @@ def test_mixed_regular_and_derived_tokens(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "At age 65, your next dose is from 02 April 2025" + assert_that(result.status_text, is_(equal_to("At age 65, your next dose is from 02 April 2025"))) def test_unknown_function_raises_error(self): """Test that unknown function name raises ValueError.""" @@ -208,8 +208,10 @@ def test_unknown_function_raises_error(self): actions=[], ) - with pytest.raises(ValueError, match="Unknown function 'UNKNOWN_FUNC'"): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern="Unknown function 'UNKNOWN_FUNC'"), + ) def test_multiple_derived_tokens(self): """Test multiple derived value tokens in same text.""" @@ -233,7 +235,7 @@ def test_multiple_derived_tokens(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "COVID: 20250402, FLU: 20260601" + assert_that(result.status_text, is_(equal_to("COVID: 20250402, FLU: 20260601"))) def test_derived_value_uses_default_days_without_args(self): """Test that empty function args uses default days from handler config.""" @@ -256,7 +258,7 @@ def test_derived_value_uses_default_days_without_args(self): # Should use the default 91 days configured in __init__.py # 2025-01-01 + 91 days = 2025-04-02 - assert result.status_text == "Next dose: 20250402" + assert_that(result.status_text, is_(equal_to("Next dose: 20250402"))) def test_case_insensitive_function_name(self): """Test that function names are case insensitive.""" @@ -277,7 +279,7 @@ def test_case_insensitive_function_name(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "20250402" + assert_that(result.status_text, is_(equal_to("20250402"))) def test_not_allowed_condition_with_derived_raises_error(self): """Test that non-allowed conditions raise error for derived values.""" @@ -299,5 +301,7 @@ def test_not_allowed_condition_with_derived_raises_error(self): expected_error = re.escape( "Invalid attribute name 'NEXT_DOSE_DUE' in token '[[TARGET.YELLOW_FEVER.NEXT_DOSE_DUE:ADD_DAYS(91)]]'." ) - with pytest.raises(ValueError, match=expected_error): - TokenProcessor.find_and_replace_tokens(person, condition) + assert_that( + calling(TokenProcessor.find_and_replace_tokens).with_args(person, condition), + raises(ValueError, pattern=expected_error), + )