From 1ad2f62fb371dcf1ed3df60409fdb291cc9bfe95 Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Fri, 30 Jan 2026 15:46:00 +0000 Subject: [PATCH 1/7] eli-579 - handling 'unknown' targets when used in derivation functions --- .../services/processors/token_processor.py | 32 +++- tests/integration/conftest.py | 56 ++++++ .../in_process/test_derived_values.py | 62 +++++++ .../processors/test_token_processor.py | 164 ++++++++++++++++++ 4 files changed, 309 insertions(+), 5 deletions(-) diff --git a/src/eligibility_signposting_api/services/processors/token_processor.py b/src/eligibility_signposting_api/services/processors/token_processor.py index 574b9dd57..30625a1f2 100644 --- a/src/eligibility_signposting_api/services/processors/token_processor.py +++ b/src/eligibility_signposting_api/services/processors/token_processor.py @@ -94,6 +94,9 @@ def get_token_replacement(token: str, person_data: list[dict], present_attribute if parsed_token.function_name: return TokenProcessor.get_derived_value(parsed_token, person_data, present_attributes, token) + # For non-derived tokens, validate that target attributes are allowed + 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: @@ -139,17 +142,16 @@ def get_derived_value( # 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 vaccine type is not in person data, return 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 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 @@ -185,6 +187,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_token_processor.py b/tests/unit/services/processors/test_token_processor.py index 4ce8c0148..88e18a33b 100644 --- a/tests/unit/services/processors/test_token_processor.py +++ b/tests/unit/services/processors/test_token_processor.py @@ -444,3 +444,167 @@ def test_token_replace_is_case_insensitive(self, token: str, expected: str): assert result.status_text == f"Your DOB is: {expected}." assert result.condition_name == 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 result.status_text == "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 result.status_text == "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 result.status_text == "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 result.status_text == "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=[], + ) + + with pytest.raises(ValueError, match="Invalid attribute name 'CUSTOM_FIELD'"): + TokenProcessor.find_and_replace_tokens(person, condition) + + 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 + with pytest.raises(ValueError, match="Invalid attribute name 'CUSTOM_INVALID_FIELD'"): + TokenProcessor.find_and_replace_tokens(person, condition) + + 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 result.status_text == "Last date: 20260128" From 448ae65dee0ed95a0816753ec6b5f6f9ec198d52 Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Fri, 30 Jan 2026 16:14:25 +0000 Subject: [PATCH 2/7] eli-5779 fixing person level application of function --- .../derived_values/add_days_handler.py | 6 ++- .../processors/derived_values/base.py | 4 +- .../services/processors/token_processor.py | 1 + .../processors/test_token_processor.py | 44 +++++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) 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..fd70754cb 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 @@ -108,8 +108,12 @@ def _find_source_date(self, context: DerivedValueContext) -> str | None: if not source_attr: return None + # For PERSON-level attributes, look for ATTRIBUTE_TYPE == "PERSON" + # For TARGET-level attributes, look for ATTRIBUTE_TYPE == context.attribute_name (e.g., "COVID") + attribute_type_to_match = "PERSON" if context.attribute_level == "PERSON" else 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 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..c2b93ea73 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,11 @@ 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 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' or 'PERSON') """ person_data: list[dict[str, Any]] @@ -20,6 +21,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 30625a1f2..08469bc41 100644 --- a/src/eligibility_signposting_api/services/processors/token_processor.py +++ b/src/eligibility_signposting_api/services/processors/token_processor.py @@ -167,6 +167,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( diff --git a/tests/unit/services/processors/test_token_processor.py b/tests/unit/services/processors/test_token_processor.py index 88e18a33b..497dd6265 100644 --- a/tests/unit/services/processors/test_token_processor.py +++ b/tests/unit/services/processors/test_token_processor.py @@ -608,3 +608,47 @@ def test_non_derived_token_with_valid_target_attribute_works(self): result = TokenProcessor.find_and_replace_tokens(person, condition) assert result.status_text == "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 result.status_text == "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 result.status_text == "Future date: 19900626" From 67af227725ae1265007f306d5187ea7a01509202 Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Mon, 2 Feb 2026 14:56:49 +0000 Subject: [PATCH 3/7] eli-579 removing comments and adding info to docstring, where appropriate --- .../processors/derived_values/add_days_handler.py | 10 +++------- .../services/processors/token_processor.py | 14 +++++--------- 2 files changed, 8 insertions(+), 16 deletions(-) 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 fd70754cb..d866be1e3 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-level attributes, looks for ATTRIBUTE_TYPE == "PERSON". + For TARGET-level attributes, looks for ATTRIBUTE_TYPE == context.attribute_name (e.g., "COVID"). + Args: context: The derived value context @@ -108,8 +109,6 @@ def _find_source_date(self, context: DerivedValueContext) -> str | None: if not source_attr: return None - # For PERSON-level attributes, look for ATTRIBUTE_TYPE == "PERSON" - # For TARGET-level attributes, look for ATTRIBUTE_TYPE == context.attribute_name (e.g., "COVID") attribute_type_to_match = "PERSON" if context.attribute_level == "PERSON" else context.attribute_name for attribute in context.person_data: @@ -132,7 +131,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: @@ -142,11 +140,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/token_processor.py b/src/eligibility_signposting_api/services/processors/token_processor.py index 08469bc41..a97b69c8e 100644 --- a/src/eligibility_signposting_api/services/processors/token_processor.py +++ b/src/eligibility_signposting_api/services/processors/token_processor.py @@ -90,18 +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) - # For non-derived tokens, validate that target attributes are allowed 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 @@ -116,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 @@ -139,17 +141,12 @@ 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__ - # 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, return 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 if parsed_token.attribute_name not in present_attributes: return "" @@ -175,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 From 2591a124f9a9a47088ea88c3a3001b8cfb32317d Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Mon, 2 Feb 2026 15:06:18 +0000 Subject: [PATCH 4/7] eli-579 adding support for COHORT data being used with ADD_DAYS --- .../services/processors/derived_values/add_days_handler.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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 d866be1e3..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 @@ -96,7 +96,7 @@ 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-level attributes, looks for ATTRIBUTE_TYPE == "PERSON". + 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: @@ -109,7 +109,10 @@ def _find_source_date(self, context: DerivedValueContext) -> str | None: if not source_attr: return None - attribute_type_to_match = "PERSON" if context.attribute_level == "PERSON" else context.attribute_name + 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") == attribute_type_to_match: From a41290689e3aa1577a5822dedfd5d9c298f75dd7 Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Mon, 2 Feb 2026 16:09:43 +0000 Subject: [PATCH 5/7] eli-579 upping test coverage --- .../processors/test_derived_values.py | 32 ++++++++++ .../processors/test_token_processor.py | 59 +++++++++++++++++++ 2 files changed, 91 insertions(+) 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_processor.py b/tests/unit/services/processors/test_token_processor.py index 497dd6265..f45e66500 100644 --- a/tests/unit/services/processors/test_token_processor.py +++ b/tests/unit/services/processors/test_token_processor.py @@ -15,6 +15,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 @@ -652,3 +653,61 @@ def test_person_level_attribute_with_add_days_explicit_source(self): # 1990-03-27 + 91 days = 1990-06-26 assert result.status_text == "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, + ) + + with pytest.raises(ValueError, match="No function specified in token"): + TokenProcessor.get_derived_value( + parsed_token=parsed_token, + person_data=[{"ATTRIBUTE_TYPE": "COVID", "LAST_SUCCESSFUL_DATE": "20250101"}], + present_attributes={"COVID"}, + token="[[TARGET.COVID.NEXT_DOSE_DUE:]]", # Malformed 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, + ) + + with pytest.raises(ValueError, match="Unknown function 'UNKNOWN_FUNCTION' in token"): + TokenProcessor.get_derived_value( + 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)]]", + ) + + 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, + ) + + with pytest.raises(ValueError, match=r"Error calculating derived value for token.*Invalid days argument"): + TokenProcessor.get_derived_value( + 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)]]", + ) From 55d9f86b2040c6549816263723628c0e4f6ef1ea Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Mon, 2 Feb 2026 16:36:00 +0000 Subject: [PATCH 6/7] eli-579 updating docstring --- .../services/processors/derived_values/base.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 c2b93ea73..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,11 +9,12 @@ class DerivedValueContext: Attributes: person_data: List of person attribute dictionaries - attribute_name: The condition/vaccine type (e.g., 'COVID', 'RSV') or person attribute (e.g., 'DATE_OF_BIRTH') + 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' or 'PERSON') + attribute_level: The level of the attribute ('TARGET', 'PERSON' or 'COHORT') """ person_data: list[dict[str, Any]] From cd5cd06b36bcb433dc3ee9c0579e013124333c65 Mon Sep 17 00:00:00 2001 From: Edd Almond <102675624+eddalmond1@users.noreply.github.com> Date: Mon, 2 Feb 2026 16:48:26 +0000 Subject: [PATCH 7/7] eli-579 switched tests to using hamcrest matchers, to match other test suites --- .../services/processors/test_token_parser.py | 25 ++-- .../processors/test_token_processor.py | 123 +++++++++++------- .../test_token_processor_derived.py | 36 ++--- 3 files changed, 109 insertions(+), 75 deletions(-) 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 f45e66500..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 ( @@ -43,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"}]) @@ -84,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"}]) @@ -102,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"}]) @@ -118,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"}]) @@ -134,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"}]) @@ -150,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"}]) @@ -167,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( @@ -189,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( @@ -223,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( @@ -259,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( @@ -292,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( @@ -325,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", @@ -348,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"), @@ -373,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"), @@ -409,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"), @@ -443,8 +458,8 @@ 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: @@ -472,7 +487,7 @@ def test_custom_target_attribute_with_add_days_when_data_present(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 2026-01-28 + 71 days = 2026-04-09 - assert result.status_text == "Next booking: 20260409" + 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.""" @@ -496,7 +511,7 @@ def test_custom_target_attribute_with_add_days_and_formatting(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 2026-01-28 + 71 days = 2026-04-09, formatted as "09 April 2026" - assert result.status_text == "Date: 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.""" @@ -521,7 +536,7 @@ def test_custom_target_attribute_returns_empty_when_condition_not_present(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # Should return empty string when condition data is not present - assert result.status_text == "Next booking: " + 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.""" @@ -546,7 +561,7 @@ def test_multiple_custom_target_attributes_with_different_functions(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 2026-01-28 + 30 = 2026-02-27, + 60 = 2026-03-29 - assert result.status_text == "First: 20260227 Second: 20260329" + 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.""" @@ -565,8 +580,10 @@ def test_custom_target_attribute_raises_error_for_invalid_condition(self): actions=[], ) - with pytest.raises(ValueError, match="Invalid attribute name 'CUSTOM_FIELD'"): - TokenProcessor.find_and_replace_tokens(person, condition) + 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.""" @@ -586,8 +603,10 @@ def test_non_derived_token_with_invalid_target_attribute_raises_error(self): ) # Non-derived tokens should only allow ALLOWED_TARGET_ATTRIBUTES - with pytest.raises(ValueError, match="Invalid attribute name 'CUSTOM_INVALID_FIELD'"): - TokenProcessor.find_and_replace_tokens(person, condition) + 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.""" @@ -608,7 +627,7 @@ def test_non_derived_token_with_valid_target_attribute_works(self): result = TokenProcessor.find_and_replace_tokens(person, condition) - assert result.status_text == "Last date: 20260128" + 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.""" @@ -630,7 +649,7 @@ def test_person_level_attribute_with_add_days_without_explicit_source(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 1990-03-27 + 91 days = 1990-06-26 - assert result.status_text == "Future date: 19900626" + 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.""" @@ -652,7 +671,7 @@ def test_person_level_attribute_with_add_days_explicit_source(self): result = TokenProcessor.find_and_replace_tokens(person, condition) # 1990-03-27 + 91 days = 1990-06-26 - assert result.status_text == "Future date: 19900626" + 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.""" @@ -666,13 +685,15 @@ def test_derived_value_with_no_function_name_raises_error(self): format=None, ) - with pytest.raises(ValueError, match="No function specified in token"): - TokenProcessor.get_derived_value( + 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.""" @@ -685,13 +706,15 @@ def test_derived_value_with_unknown_function_raises_error(self): format=None, ) - with pytest.raises(ValueError, match="Unknown function 'UNKNOWN_FUNCTION' in token"): - TokenProcessor.get_derived_value( + 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.""" @@ -704,10 +727,12 @@ def test_derived_value_handler_exception_gets_wrapped(self): format=None, ) - with pytest.raises(ValueError, match=r"Error calculating derived value for token.*Invalid days argument"): - TokenProcessor.get_derived_value( + 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), + )