From 5496516ec9ce7207bcbef790dd7d76e2bfcbf2c5 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 25 Nov 2025 13:07:10 +0000 Subject: [PATCH 01/21] ELI 557 - Iteration Date must be >= Campaign Start Date AND <= Campaign End Date --- .../validators/campaign_config_validator.py | 16 ++++++++++------ .../validation/test_campaign_config_validator.py | 12 ++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index 94b007346..02f985ba7 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -14,15 +14,19 @@ def validate_iterations(cls, iterations: list[Iteration]) -> list[IterationValid return [IterationValidation(**i.model_dump()) for i in iterations] @model_validator(mode="after") - def check_has_iteration_from_start(self) -> typing.Self: + def validate_campaign_has_iteration_within_schedule(self) -> typing.Self: iterations_by_date = sorted(self.iterations, key=attrgetter("iteration_date")) if first_iteration := next(iter(iterations_by_date), None): - if first_iteration.iteration_date > self.start_date: - message = ( - f"campaign {self.id} starts on {self.start_date}, " - f"1st iteration starts later - {first_iteration.iteration_date}" + if first_iteration.iteration_date < self.start_date: + raise ValueError( + f"Iteration {first_iteration.id} starts before campaign {self.id} " + f"start date {self.start_date}." + ) + if first_iteration.iteration_date > self.end_date: + raise ValueError( + f"Iteration {first_iteration.id} starts after campaign {self.id} " + f"end date {self.end_date}." ) - raise ValueError(message) return self # Should never happen, since we are constraining self.iterations with a min_length of 1 message = f"campaign {self.id} has no iterations." diff --git a/tests/unit/validation/test_campaign_config_validator.py b/tests/unit/validation/test_campaign_config_validator.py index 61bb75ca7..a4d7c2f00 100644 --- a/tests/unit/validation/test_campaign_config_validator.py +++ b/tests/unit/validation/test_campaign_config_validator.py @@ -196,8 +196,8 @@ class TestBUCValidations: ("start_date", "end_date"), [ ("20250101", "20250331"), # valid range - ("20250601", "20250630"), # valid short range - ("20250101", "20250101"), # same day + ("20250201", "20250228"), # valid short range + ("20250202", "20250202"), # same day ], ) def test_valid_start_and_end_dates_and_iteration_dates_relation( @@ -206,14 +206,14 @@ def test_valid_start_and_end_dates_and_iteration_dates_relation( data = valid_campaign_config_with_only_mandatory_fields.copy() data["StartDate"] = start_date data["EndDate"] = end_date - data["Iterations"][0]["IterationDate"] = "20241231" + data["Iterations"][0]["IterationDate"] = "20250202" CampaignConfigValidation(**data) @pytest.mark.parametrize( ("start_date", "end_date"), [ - ("20241230", "20250101"), # campaign start date is after the iteration date - ("20250331", "20250101"), # end before start + ("20241230", "20250101"), # Campaign start date is after the iteration date + ("20240729", "20241228"), # Campaign ends date is before the iteration date ], ) def test_invalid_start_and_end_dates_and_iteration_dates_relation( @@ -222,7 +222,7 @@ def test_invalid_start_and_end_dates_and_iteration_dates_relation( data = valid_campaign_config_with_only_mandatory_fields.copy() data["StartDate"] = start_date data["EndDate"] = end_date - data["Iterations"][0]["IterationDate"] = "20241231" + data["Iterations"][0]["IterationDate"] = "20241229" with pytest.raises(ValidationError): CampaignConfigValidation(**data) From 825f8651c5389565d96a0471f431e3cde071c93a Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 25 Nov 2025 13:51:38 +0000 Subject: [PATCH 02/21] ELI 557 - Iteration id must be unique --- .../validators/campaign_config_validator.py | 11 ++++++++++ tests/unit/validation/conftest.py | 20 +++++++++++++++++++ .../test_campaign_config_validator.py | 14 +++++++++++++ 3 files changed, 45 insertions(+) diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index 02f985ba7..5cc95ecda 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -1,4 +1,5 @@ import typing +from collections import Counter from operator import attrgetter from pydantic import field_validator, model_validator @@ -13,6 +14,16 @@ class CampaignConfigValidation(CampaignConfig): def validate_iterations(cls, iterations: list[Iteration]) -> list[IterationValidation]: return [IterationValidation(**i.model_dump()) for i in iterations] + @model_validator(mode="after") + def validate_iterations_have_unique_id(self) -> typing.Self: + ids = [iteration.id for iteration in self.iterations] + duplicates = {i_id for i_id, count in Counter(ids).items() if count > 1} + if duplicates: + raise ValueError( + f"Iterations contain duplicate IDs: {', '.join(duplicates)}" + ) + return self + @model_validator(mode="after") def validate_campaign_has_iteration_within_schedule(self) -> typing.Self: iterations_by_date = sorted(self.iterations, key=attrgetter("iteration_date")) diff --git a/tests/unit/validation/conftest.py b/tests/unit/validation/conftest.py index 8b95bef44..2870b29a9 100644 --- a/tests/unit/validation/conftest.py +++ b/tests/unit/validation/conftest.py @@ -34,6 +34,26 @@ def valid_campaign_config_with_only_mandatory_fields(): } +@pytest.fixture +def valid_iteration_with_only_mandatory_fields(): + return { + "ID": "ITER001", + "Version": 1, + "Name": "Mid-January Push", + "IterationDate": "20250102", + "IterationNumber": 1, + "ApprovalMinimum": 10, + "ApprovalMaximum": 100, + "Type": "A", + "DefaultCommsRouting": "", + "DefaultNotEligibleRouting": "", + "DefaultNotActionableRouting": "", + "IterationCohorts": [], + "IterationRules": [], + "ActionsMapper": {}, + } + + @pytest.fixture def valid_iteration_rule_with_only_mandatory_fields(): return { diff --git a/tests/unit/validation/test_campaign_config_validator.py b/tests/unit/validation/test_campaign_config_validator.py index a4d7c2f00..fcf30bd52 100644 --- a/tests/unit/validation/test_campaign_config_validator.py +++ b/tests/unit/validation/test_campaign_config_validator.py @@ -233,3 +233,17 @@ def test_validate_iterations_non_empty(self, valid_campaign_config_with_only_man CampaignConfigValidation(**data) errors = error.value.errors() assert any(e["loc"][-1] == "Iterations" for e in errors), "Expected validation error on 'Iterations'" + + def test_unique_iteration_ids(self, valid_campaign_config_with_only_mandatory_fields, valid_iteration_with_only_mandatory_fields): + data = valid_campaign_config_with_only_mandatory_fields.copy() + data["Iterations"].append(valid_iteration_with_only_mandatory_fields.copy()) + data["Iterations"][1]["ID"] = data["Iterations"][0]["ID"] + with pytest.raises(ValidationError) as exc_info: + CampaignConfigValidation(**data) + + # Extract the error message + error_message = str(exc_info.value) + + # Assert that the duplicate ID appears in the message + duplicate_id = data["Iterations"][0]["ID"] + assert f"Iterations contain duplicate IDs: {duplicate_id}" in error_message From 336d2847fc52044d30a4bc1e027ba128c1778e32 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 26 Nov 2025 10:50:04 +0000 Subject: [PATCH 03/21] validate approval minimum and maximum --- .../validators/campaign_config_validator.py | 9 +++++++ .../test_campaign_config_validator.py | 26 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index 5cc95ecda..be3c80d39 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -9,11 +9,20 @@ class CampaignConfigValidation(CampaignConfig): + @field_validator("iterations") @classmethod def validate_iterations(cls, iterations: list[Iteration]) -> list[IterationValidation]: return [IterationValidation(**i.model_dump()) for i in iterations] + @model_validator(mode="after") + def validate_approval_minimum_is_less_than_or_equal_to_approval_maximum(self) -> typing.Self: + if self.approval_minimum > self.approval_maximum: + raise ValueError( + f"approval_minimum {self.approval_minimum} is greater than approval_maximum {self.approval_maximum}" + ) + return self + @model_validator(mode="after") def validate_iterations_have_unique_id(self) -> typing.Self: ids = [iteration.id for iteration in self.iterations] diff --git a/tests/unit/validation/test_campaign_config_validator.py b/tests/unit/validation/test_campaign_config_validator.py index fcf30bd52..29ed079a0 100644 --- a/tests/unit/validation/test_campaign_config_validator.py +++ b/tests/unit/validation/test_campaign_config_validator.py @@ -247,3 +247,29 @@ def test_unique_iteration_ids(self, valid_campaign_config_with_only_mandatory_fi # Assert that the duplicate ID appears in the message duplicate_id = data["Iterations"][0]["ID"] assert f"Iterations contain duplicate IDs: {duplicate_id}" in error_message + + + def test_error_approval_minimum_is_greater_than_approval_maximum(self, valid_campaign_config_with_only_mandatory_fields): + data = valid_campaign_config_with_only_mandatory_fields.copy() + data["ApprovalMinimum"] = 2 + data["ApprovalMaximum"] = 1 + with pytest.raises(ValidationError): + CampaignConfigValidation(**data) + + @pytest.mark.parametrize( + ("approval_min", "approval_max"), + [ + (1, 2), + (1, 1), + ] + ) + def test_approval_minimum_greater_than_approval_maximum_is_invalid( + self, valid_campaign_config_with_only_mandatory_fields, + approval_min, + approval_max, + ): + data = valid_campaign_config_with_only_mandatory_fields.copy() + data["ApprovalMinimum"] = approval_min + data["ApprovalMaximum"] = approval_max + CampaignConfigValidation(**data) + From cf482df33484d7256211ec9a3bbc3e8f12ba73c9 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 26 Nov 2025 17:17:12 +0000 Subject: [PATCH 04/21] lint fix --- .../validators/campaign_config_validator.py | 34 +++++++++---------- .../test_campaign_config_validator.py | 17 ++++++---- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index be3c80d39..394ac14df 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -9,7 +9,6 @@ class CampaignConfigValidation(CampaignConfig): - @field_validator("iterations") @classmethod def validate_iterations(cls, iterations: list[Iteration]) -> list[IterationValidation]: @@ -17,10 +16,11 @@ def validate_iterations(cls, iterations: list[Iteration]) -> list[IterationValid @model_validator(mode="after") def validate_approval_minimum_is_less_than_or_equal_to_approval_maximum(self) -> typing.Self: - if self.approval_minimum > self.approval_maximum: - raise ValueError( - f"approval_minimum {self.approval_minimum} is greater than approval_maximum {self.approval_maximum}" - ) + if self.approval_minimum and self.approval_maximum: + if self.approval_minimum > self.approval_maximum: + msg = f"approval_minimum {self.approval_minimum} > approval_maximum {self.approval_maximum}" + raise ValueError(msg) + return self return self @model_validator(mode="after") @@ -28,9 +28,8 @@ def validate_iterations_have_unique_id(self) -> typing.Self: ids = [iteration.id for iteration in self.iterations] duplicates = {i_id for i_id, count in Counter(ids).items() if count > 1} if duplicates: - raise ValueError( - f"Iterations contain duplicate IDs: {', '.join(duplicates)}" - ) + msg = f"Iterations contain duplicate IDs: {', '.join(duplicates)}" + raise ValueError(msg) return self @model_validator(mode="after") @@ -38,16 +37,15 @@ def validate_campaign_has_iteration_within_schedule(self) -> typing.Self: iterations_by_date = sorted(self.iterations, key=attrgetter("iteration_date")) if first_iteration := next(iter(iterations_by_date), None): if first_iteration.iteration_date < self.start_date: - raise ValueError( - f"Iteration {first_iteration.id} starts before campaign {self.id} " - f"start date {self.start_date}." - ) + msg = f"Iteration {first_iteration.id} starts before campaign {self.id} start date {self.start_date}." + raise ValueError(msg) + if first_iteration.iteration_date > self.end_date: - raise ValueError( - f"Iteration {first_iteration.id} starts after campaign {self.id} " - f"end date {self.end_date}." - ) + msg = f"Iteration {first_iteration.id} starts after campaign {self.id} end date {self.end_date}." + raise ValueError(msg) + return self + # Should never happen, since we are constraining self.iterations with a min_length of 1 - message = f"campaign {self.id} has no iterations." - raise ValueError(message) + msg = f"campaign {self.id} has no iterations." + raise ValueError(msg) diff --git a/tests/unit/validation/test_campaign_config_validator.py b/tests/unit/validation/test_campaign_config_validator.py index 29ed079a0..c7321aef4 100644 --- a/tests/unit/validation/test_campaign_config_validator.py +++ b/tests/unit/validation/test_campaign_config_validator.py @@ -234,10 +234,12 @@ def test_validate_iterations_non_empty(self, valid_campaign_config_with_only_man errors = error.value.errors() assert any(e["loc"][-1] == "Iterations" for e in errors), "Expected validation error on 'Iterations'" - def test_unique_iteration_ids(self, valid_campaign_config_with_only_mandatory_fields, valid_iteration_with_only_mandatory_fields): + def test_unique_iteration_ids( + self, valid_campaign_config_with_only_mandatory_fields, valid_iteration_with_only_mandatory_fields + ): data = valid_campaign_config_with_only_mandatory_fields.copy() data["Iterations"].append(valid_iteration_with_only_mandatory_fields.copy()) - data["Iterations"][1]["ID"] = data["Iterations"][0]["ID"] + data["Iterations"][1]["ID"] = data["Iterations"][0]["ID"] with pytest.raises(ValidationError) as exc_info: CampaignConfigValidation(**data) @@ -248,8 +250,9 @@ def test_unique_iteration_ids(self, valid_campaign_config_with_only_mandatory_fi duplicate_id = data["Iterations"][0]["ID"] assert f"Iterations contain duplicate IDs: {duplicate_id}" in error_message - - def test_error_approval_minimum_is_greater_than_approval_maximum(self, valid_campaign_config_with_only_mandatory_fields): + def test_error_approval_minimum_is_greater_than_approval_maximum( + self, valid_campaign_config_with_only_mandatory_fields + ): data = valid_campaign_config_with_only_mandatory_fields.copy() data["ApprovalMinimum"] = 2 data["ApprovalMaximum"] = 1 @@ -261,10 +264,11 @@ def test_error_approval_minimum_is_greater_than_approval_maximum(self, valid_cam [ (1, 2), (1, 1), - ] + ], ) def test_approval_minimum_greater_than_approval_maximum_is_invalid( - self, valid_campaign_config_with_only_mandatory_fields, + self, + valid_campaign_config_with_only_mandatory_fields, approval_min, approval_max, ): @@ -272,4 +276,3 @@ def test_approval_minimum_greater_than_approval_maximum_is_invalid( data["ApprovalMinimum"] = approval_min data["ApprovalMaximum"] = approval_max CampaignConfigValidation(**data) - From ad28d051ba59e5e49f7962715265a4f0f571e326 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 27 Nov 2025 10:27:08 +0000 Subject: [PATCH 05/21] fix for approve minimum and maximum validator --- .../validators/campaign_config_validator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index 394ac14df..3b5cf509d 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -16,7 +16,7 @@ def validate_iterations(cls, iterations: list[Iteration]) -> list[IterationValid @model_validator(mode="after") def validate_approval_minimum_is_less_than_or_equal_to_approval_maximum(self) -> typing.Self: - if self.approval_minimum and self.approval_maximum: + if self.approval_minimum is not None and self.approval_maximum is not None: if self.approval_minimum > self.approval_maximum: msg = f"approval_minimum {self.approval_minimum} > approval_maximum {self.approval_maximum}" raise ValueError(msg) From 69473d1d461845dc327f36c6698ea845c41b1187 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 27 Nov 2025 16:29:13 +0000 Subject: [PATCH 06/21] date fix --- .../model/campaign_config.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index 6c4eaeeea..a107d669b 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import re import typing from collections import Counter from datetime import UTC, date, datetime @@ -273,7 +274,12 @@ class Iteration(BaseModel): def parse_dates(cls, v: str | date) -> date: if isinstance(v, date): return v - return datetime.strptime(v, "%Y%m%d").date() # noqa: DTZ007 + if not re.fullmatch(r"\d{8}", str(v)): + raise ValueError(f"Invalid format: {v}. Must be YYYYMMDD with 8 digits.") + try: + return datetime.strptime(str(v), "%Y%m%d").date() # noqa: DTZ007 + except ValueError: + raise ValueError(f"Invalid date value: {v}. Must be a valid calendar date in YYYYMMDD format.") @field_serializer("iteration_date", when_used="always") @staticmethod @@ -316,7 +322,12 @@ class CampaignConfig(BaseModel): def parse_dates(cls, v: str | date) -> date: if isinstance(v, date): return v - return datetime.strptime(v, "%Y%m%d").date() # noqa: DTZ007 + if not re.fullmatch(r"\d{8}", str(v)): + raise ValueError(f"Invalid format: {v}. Must be YYYYMMDD with 8 digits.") + try: + return datetime.strptime(str(v), "%Y%m%d").date() # noqa: DTZ007 + except ValueError: + raise ValueError(f"Invalid date value: {v}. Must be a valid calendar date in YYYYMMDD format.") @field_serializer("start_date", "end_date", when_used="always") @staticmethod From 2fd830d9dae82a1e687105631ddd992d7458c181 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 27 Nov 2025 16:44:30 +0000 Subject: [PATCH 07/21] attribute name is optional only for cohort attribte level --- .../model/campaign_config.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index a107d669b..9ae26280f 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -163,6 +163,17 @@ def parse_yn_to_bool(cls, v: str | bool) -> bool: # noqa: N805 return v.upper() == "Y" return v + @model_validator( mode="after") + def validate_attribute_name_is_optional_only_for_cohort_attribute_level(self) -> typing.Self: # noqa: N805 + if self.attribute_name: + return self + else: + if self.attribute_level == RuleAttributeLevel.COHORT: + return self + else: + raise ValueError(f"AttributeName must be set for {self.attribute_level} AttributeLevel.") + + _parent: Iteration | None = PrivateAttr(default=None) def set_parent(self, parent: Iteration) -> None: From 2f5ae0a2a231b40e33ffbe9aad8d6a12f4329185 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 27 Nov 2025 16:59:04 +0000 Subject: [PATCH 08/21] attribute level and attribute target relation --- .../model/campaign_config.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index 9ae26280f..fcb185ee3 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -171,7 +171,17 @@ def validate_attribute_name_is_optional_only_for_cohort_attribute_level(self) - if self.attribute_level == RuleAttributeLevel.COHORT: return self else: - raise ValueError(f"AttributeName must be set for {self.attribute_level} AttributeLevel.") + raise ValueError(f"AttributeName must be set where AttributeLevel is {self.attribute_level} .") + + @model_validator(mode="after") + def validate_attribute_target_is_mandatory_for_target_attribute_level(self) -> typing.Self: # noqa: N805 + if self.attribute_target: + return self + else: + if self.attribute_level != RuleAttributeLevel.TARGET: + return self + else: + raise ValueError(f"AttributeTarget is mandatory where AttributeLevel is {self.attribute_level}.") _parent: Iteration | None = PrivateAttr(default=None) From 9153de09e8ea37e30678b7d67627e6065cab40f9 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Thu, 27 Nov 2025 17:06:58 +0000 Subject: [PATCH 09/21] cohort label md --- src/rules_validation_api/validations.md | 116 ++++++++++++------------ 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/src/rules_validation_api/validations.md b/src/rules_validation_api/validations.md index a54dc9d6c..2fcddc633 100644 --- a/src/rules_validation_api/validations.md +++ b/src/rules_validation_api/validations.md @@ -1,60 +1,60 @@ # CampaignConfig Validation Rules -| Key | Validation | Data Type | Field Uniqueness | Enum/Allowed Values | Cross-field/Conditional | -|-----------------------------------------------------------------------------|------------|--------------|---------------------|---------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------| -| CampaignConfig.ID | Mandatory | string | Unique | - | - | -| CampaignConfig.Version | Mandatory | int | - | - | - | -| CampaignConfig.Name | Mandatory | string | - | - | - | -| CampaignConfig.Type | Mandatory | string | - | V, S | - | -| CampaignConfig.Target | Mandatory | string | - | See [`config/constants.py`](../eligibility_signposting_api/config/constants.py) for the definition of `ALLOWED_CONDITIONS`. | - | -| CampaignConfig.Manager | Optional | list[string] | - | - | - | -| CampaignConfig.Approver | Optional | list[string] | - | - | - | -| CampaignConfig.Reviewer | Optional | list[string] | - | - | - | -| CampaignConfig.IterationFrequency | Mandatory | string | - | X, D, W, M, Q, A | - | -| CampaignConfig.IterationType | Mandatory | string | - | A, M, S, O | - | -| CampaignConfig.IterationTime | Optional | string | - | - | - | -| CampaignConfig.DefaultCommsRouting | Optional | string | - | - | - | -| CampaignConfig.StartDate | Mandatory | date | - | - | StartDate < EndDate | -| CampaignConfig.EndDate | Mandatory | date | - | - | EndDate > StartDate | -| CampaignConfig.ApprovalMinimum | Optional | int | - | - | ApprovalMinimum ≤ ApprovalMaximum | -| CampaignConfig.ApprovalMaximum | Optional | int | - | - | ApprovalMaximum ≥ ApprovalMinimum | -| CampaignConfig.Iterations[x].ID | Mandatory | string | Unique | - | - | -| CampaignConfig.Iterations[x].Version | Mandatory | int | - | - | - | -| CampaignConfig.Iterations[x].Name | Mandatory | string | - | - | - | -| CampaignConfig.Iterations[x].IterationDate | Mandatory | date | Unique | - | No overlapping dates | -| CampaignConfig.Iterations[x].IterationNumber | Optional | int | - | - | - | -| CampaignConfig.Iterations[x].ApprovalMinimum | Optional | int | - | - | - | -| CampaignConfig.Iterations[x].ApprovalMaximum | Optional | int | - | - | - | -| CampaignConfig.Iterations[x].Type | Mandatory | string | - | A, M, S, O | - | -| CampaignConfig.Iterations[x].DefaultCommsRouting | Mandatory | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].DefaultNotEligibleRouting | Mandatory | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].DefaultNotActionableRouting | Mandatory | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].StatusText.NotEligible | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].StatusText.NotActionable | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].StatusText.Actionable | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].RulesMapper.RULE_ENTRY.RuleNames | Optional | list[string] | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].RulesMapper.RULE_ENTRY.RuleCode | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].RulesMapper.RULE_ENTRY.RuleText | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | -| CampaignConfig.Iterations[x].IterationCohorts[y].CohortLabel | Mandatory | string | Unique in iteration | - | - | -| CampaignConfig.Iterations[x].IterationCohorts[y].CohortGroup | Mandatory | string | - | - | - | -| CampaignConfig.Iterations[x].IterationCohorts[y].PositiveDescription | Optional | string | - | - | - | -| CampaignConfig.Iterations[x].IterationCohorts[y].NegativeDescription | Optional | string | - | - | - | -| CampaignConfig.Iterations[x].IterationCohorts[y].Priority | Optional | int | - | - | - | -| CampaignConfig.Iterations[x].IterationCohorts[y].Virtual | Optional | string | - | Y, N | - | -| CampaignConfig.Iterations[x].IterationRules[z].Type | Mandatory | string | - | F, S, R, X, Y | - | -| CampaignConfig.Iterations[x].IterationRules[z].Name | Mandatory | string | - | - | - | -| CampaignConfig.Iterations[x].IterationRules[z].Description | Mandatory | string | - | - | - | -| CampaignConfig.Iterations[x].IterationRules[z].Operator | Mandatory | string | - | See [`model/campaign_config.py`](../eligibility_signposting_api/model/campaign_config.py) for the definition of `RuleOperator`. | - | -| CampaignConfig.Iterations[x].IterationRules[z].Comparator | Mandatory | string | - | - | - | -| CampaignConfig.Iterations[x].IterationRules[z].AttributeTarget | Optional | string | - | - | Required and non-null if AttributeLevel == TARGET | -| CampaignConfig.Iterations[x].IterationRules[z].AttributeLevel | Mandatory | string | - | PERSON, TARGET, COHORT | - | -| CampaignConfig.Iterations[x].IterationRules[z].AttributeName | Optional | string | - | - | If AttributeLevel is COHORT, AttributeName must be COHORT_LABEL or left blank. | -| CampaignConfig.Iterations[x].IterationRules[z].CohortLabel | Optional | string | - | - | Required if AttributeLevel == COHORT and CohortLabel only matters for Type 'F'/'S' | -| CampaignConfig.Iterations[x].IterationRules[z].Priority | Mandatory | int | - | - | - | -| CampaignConfig.Iterations[x].IterationRules[z].RuleStop | Optional | bool | - | - | - | -| CampaignConfig.Iterations[x].IterationRules[z].CommsRouting | Optional | string | - | - | - | -| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.ExternalRoutingCode | Optional | string | - | - | - | -| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.ActionDescription | Optional | string | - | Must be valid Markdown and follow style rules (headers, lists, blank lines) | - | -| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.ActionType | Optional | string | - | - | - | -| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.UrlLink | Optional | string | - | - | - | -| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.UrlLabel | Optional | string | - | - | - | +| Key | Validation | Data Type | Field Uniqueness | Enum/Allowed Values | Cross-field/Conditional | +|-----------------------------------------------------------------------------|------------|--------------|---------------------|---------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------| +| CampaignConfig.ID | Mandatory | string | Unique | - | - | +| CampaignConfig.Version | Mandatory | int | - | - | - | +| CampaignConfig.Name | Mandatory | string | - | - | - | +| CampaignConfig.Type | Mandatory | string | - | V, S | - | +| CampaignConfig.Target | Mandatory | string | - | See [`config/constants.py`](../eligibility_signposting_api/config/constants.py) for the definition of `ALLOWED_CONDITIONS`. | - | +| CampaignConfig.Manager | Optional | list[string] | - | - | - | +| CampaignConfig.Approver | Optional | list[string] | - | - | - | +| CampaignConfig.Reviewer | Optional | list[string] | - | - | - | +| CampaignConfig.IterationFrequency | Mandatory | string | - | X, D, W, M, Q, A | - | +| CampaignConfig.IterationType | Mandatory | string | - | A, M, S, O | - | +| CampaignConfig.IterationTime | Optional | string | - | - | - | +| CampaignConfig.DefaultCommsRouting | Optional | string | - | - | - | +| CampaignConfig.StartDate | Mandatory | date | - | - | StartDate < EndDate | +| CampaignConfig.EndDate | Mandatory | date | - | - | EndDate > StartDate | +| CampaignConfig.ApprovalMinimum | Optional | int | - | - | ApprovalMinimum ≤ ApprovalMaximum | +| CampaignConfig.ApprovalMaximum | Optional | int | - | - | ApprovalMaximum ≥ ApprovalMinimum | +| CampaignConfig.Iterations[x].ID | Mandatory | string | Unique | - | - | +| CampaignConfig.Iterations[x].Version | Mandatory | int | - | - | - | +| CampaignConfig.Iterations[x].Name | Mandatory | string | - | - | - | +| CampaignConfig.Iterations[x].IterationDate | Mandatory | date | Unique | - | No overlapping dates | +| CampaignConfig.Iterations[x].IterationNumber | Optional | int | - | - | - | +| CampaignConfig.Iterations[x].ApprovalMinimum | Optional | int | - | - | - | +| CampaignConfig.Iterations[x].ApprovalMaximum | Optional | int | - | - | - | +| CampaignConfig.Iterations[x].Type | Mandatory | string | - | A, M, S, O | - | +| CampaignConfig.Iterations[x].DefaultCommsRouting | Mandatory | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].DefaultNotEligibleRouting | Mandatory | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].DefaultNotActionableRouting | Mandatory | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].StatusText.NotEligible | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].StatusText.NotActionable | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].StatusText.Actionable | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].RulesMapper.RULE_ENTRY.RuleNames | Optional | list[string] | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].RulesMapper.RULE_ENTRY.RuleCode | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].RulesMapper.RULE_ENTRY.RuleText | Optional | string | - | - | Each value in this field must match a key in ActionsMapper. | +| CampaignConfig.Iterations[x].IterationCohorts[y].CohortLabel | Mandatory | string | Unique in iteration | - | - | +| CampaignConfig.Iterations[x].IterationCohorts[y].CohortGroup | Mandatory | string | - | - | - | +| CampaignConfig.Iterations[x].IterationCohorts[y].PositiveDescription | Optional | string | - | - | - | +| CampaignConfig.Iterations[x].IterationCohorts[y].NegativeDescription | Optional | string | - | - | - | +| CampaignConfig.Iterations[x].IterationCohorts[y].Priority | Optional | int | - | - | - | +| CampaignConfig.Iterations[x].IterationCohorts[y].Virtual | Optional | string | - | Y, N | - | +| CampaignConfig.Iterations[x].IterationRules[z].Type | Mandatory | string | - | F, S, R, X, Y | - | +| CampaignConfig.Iterations[x].IterationRules[z].Name | Mandatory | string | - | - | - | +| CampaignConfig.Iterations[x].IterationRules[z].Description | Mandatory | string | - | - | - | +| CampaignConfig.Iterations[x].IterationRules[z].Operator | Mandatory | string | - | See [`model/campaign_config.py`](../eligibility_signposting_api/model/campaign_config.py) for the definition of `RuleOperator`. | - | +| CampaignConfig.Iterations[x].IterationRules[z].Comparator | Mandatory | string | - | - | - | +| CampaignConfig.Iterations[x].IterationRules[z].AttributeTarget | Optional | string | - | - | Required and non-null if AttributeLevel == TARGET | +| CampaignConfig.Iterations[x].IterationRules[z].AttributeLevel | Mandatory | string | - | PERSON, TARGET, COHORT | - | +| CampaignConfig.Iterations[x].IterationRules[z].AttributeName | Optional | string | - | - | If AttributeLevel is COHORT, AttributeName must be COHORT_LABEL or left blank. | +| CampaignConfig.Iterations[x].IterationRules[z].CohortLabel | Optional | string | - | - | CohortLabel only works for Type 'F'/'S' | +| CampaignConfig.Iterations[x].IterationRules[z].Priority | Mandatory | int | - | - | - | +| CampaignConfig.Iterations[x].IterationRules[z].RuleStop | Optional | bool | - | - | - | +| CampaignConfig.Iterations[x].IterationRules[z].CommsRouting | Optional | string | - | - | - | +| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.ExternalRoutingCode | Optional | string | - | - | - | +| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.ActionDescription | Optional | string | - | Must be valid Markdown and follow style rules (headers, lists, blank lines) | - | +| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.ActionType | Optional | string | - | - | - | +| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.UrlLink | Optional | string | - | - | - | +| CampaignConfig.Iterations[x].ActionsMapper.ACTION_ENTRY.UrlLabel | Optional | string | - | - | - | From 6d61ec8c147fb3ac6c9290f9637520eb047aaf77 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 28 Nov 2025 07:55:28 +0000 Subject: [PATCH 10/21] refactored --- .../model/campaign_config.py | 21 ------ .../validators/iteration_rules_validator.py | 22 +++++- .../test_iteration_rules_validator.py | 73 ++++++++++++++++++- 3 files changed, 88 insertions(+), 28 deletions(-) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index fcb185ee3..a107d669b 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -163,27 +163,6 @@ def parse_yn_to_bool(cls, v: str | bool) -> bool: # noqa: N805 return v.upper() == "Y" return v - @model_validator( mode="after") - def validate_attribute_name_is_optional_only_for_cohort_attribute_level(self) -> typing.Self: # noqa: N805 - if self.attribute_name: - return self - else: - if self.attribute_level == RuleAttributeLevel.COHORT: - return self - else: - raise ValueError(f"AttributeName must be set where AttributeLevel is {self.attribute_level} .") - - @model_validator(mode="after") - def validate_attribute_target_is_mandatory_for_target_attribute_level(self) -> typing.Self: # noqa: N805 - if self.attribute_target: - return self - else: - if self.attribute_level != RuleAttributeLevel.TARGET: - return self - else: - raise ValueError(f"AttributeTarget is mandatory where AttributeLevel is {self.attribute_level}.") - - _parent: Iteration | None = PrivateAttr(default=None) def set_parent(self, parent: Iteration) -> None: diff --git a/src/rules_validation_api/validators/iteration_rules_validator.py b/src/rules_validation_api/validators/iteration_rules_validator.py index 74f6ce12d..2acd19ded 100644 --- a/src/rules_validation_api/validators/iteration_rules_validator.py +++ b/src/rules_validation_api/validators/iteration_rules_validator.py @@ -1,4 +1,4 @@ -from typing import Self +import typing from pydantic import model_validator @@ -12,7 +12,7 @@ class IterationRuleValidation(IterationRule): @model_validator(mode="after") - def check_cohort_attribute_name(self) -> Self: + def check_cohort_attribute_name(self) -> typing.Self: if ( self.attribute_level == RuleAttributeLevel.COHORT and self.attribute_name @@ -23,7 +23,7 @@ def check_cohort_attribute_name(self) -> Self: return self @model_validator(mode="after") - def check_cohort_label_for_non_f_and_s_types(self) -> Self: + def check_cohort_label_for_non_f_and_s_types(self) -> typing.Self: allowed_types = {RuleType("F"), RuleType("S")} if self.cohort_label is not None and self.type not in allowed_types: msg = ( @@ -32,3 +32,19 @@ def check_cohort_label_for_non_f_and_s_types(self) -> Self: ) raise ValueError(msg) return self + + @model_validator(mode="after") + def validate_attribute_name_is_optional_only_for_cohort_attribute_level(self) -> typing.Self: + if self.attribute_name: + return self + if self.attribute_level == RuleAttributeLevel.COHORT: + return self + raise ValueError(f"AttributeName must be set where AttributeLevel is {self.attribute_level} .") + + @model_validator(mode="after") + def validate_attribute_target_is_mandatory_for_target_attribute_level(self) -> typing.Self: + if self.attribute_target: + return self + if self.attribute_level != RuleAttributeLevel.TARGET: + return self + raise ValueError(f"AttributeTarget is mandatory where AttributeLevel is {self.attribute_level}.") diff --git a/tests/unit/validation/test_iteration_rules_validator.py b/tests/unit/validation/test_iteration_rules_validator.py index 2e6dcfd21..33c66abc7 100644 --- a/tests/unit/validation/test_iteration_rules_validator.py +++ b/tests/unit/validation/test_iteration_rules_validator.py @@ -1,6 +1,7 @@ import pytest from pydantic import ValidationError +from eligibility_signposting_api.model.campaign_config import RuleAttributeLevel from rules_validation_api.validators.iteration_validator import IterationRuleValidation @@ -80,8 +81,18 @@ def test_invalid_priority(self, priority_value, valid_iteration_rule_with_only_m with pytest.raises(ValidationError): IterationRuleValidation(**data) - @pytest.mark.parametrize("attribute_level", ["PERSON", "TARGET", "COHORT"]) - def test_valid_attribute_level(self, attribute_level, valid_iteration_rule_with_only_mandatory_fields): + @pytest.mark.parametrize("attribute_level", ["PERSON", "TARGET"]) + def test_valid_attribute_level_person_and_targer( + self, attribute_level, valid_iteration_rule_with_only_mandatory_fields + ): + data = valid_iteration_rule_with_only_mandatory_fields.copy() + data["AttributeLevel"] = attribute_level + data["AttributeName"] = "Something" # Ignoring the validation constraint btw AttributeLevel and AttributeName + result = IterationRuleValidation(**data) + assert result.attribute_level == attribute_level + + @pytest.mark.parametrize("attribute_level", ["COHORT"]) + def test_valid_attribute_level_cohort(self, attribute_level, valid_iteration_rule_with_only_mandatory_fields): data = valid_iteration_rule_with_only_mandatory_fields.copy() data["AttributeLevel"] = attribute_level data["AttributeName"] = None # Ignoring the validation constraint btw AttributeLevel and AttributeName @@ -148,7 +159,7 @@ def test_rule_stop_boolean_resolution( class TestOptionalFieldsSchemaValidations: # AttributeName - @pytest.mark.parametrize("attr_name", ["status", "user_type", None]) + @pytest.mark.parametrize("attr_name", ["status", "user_type"]) def test_valid_attribute_name(self, attr_name, valid_iteration_rule_with_only_mandatory_fields): data = valid_iteration_rule_with_only_mandatory_fields.copy() data["AttributeName"] = attr_name @@ -192,7 +203,7 @@ def test_invalid_cohort_label(self, label, valid_iteration_rule_with_only_mandat IterationRuleValidation(**data) # AttributeTarget - @pytest.mark.parametrize("target", ["target_value", None]) + @pytest.mark.parametrize("target", ["target_value"]) def test_valid_attribute_target(self, target, valid_iteration_rule_with_only_mandatory_fields): data = valid_iteration_rule_with_only_mandatory_fields.copy() data["AttributeTarget"] = target @@ -291,3 +302,57 @@ def test_valid_when_cohort_label_absent_for_non_f_s_types( data.pop("CohortLabel", None) result = IterationRuleValidation(**data) assert result.cohort_label is None + + @pytest.mark.parametrize( + ("attribute_level", "attribute_name", "is_valid"), + [ + (RuleAttributeLevel.COHORT, None, True), # Allowed: name optional for cohort + (RuleAttributeLevel.COHORT, "COHORT_LABEL", True), # Allowed: name provided + (RuleAttributeLevel.TARGET, "RSV", True), # Allowed: name provided for target + (RuleAttributeLevel.TARGET, None, False), # NOT allowed: missing for non-cohort + ], + ) + def test_attribute_name_optional_only_for_cohort( + self, attribute_level, attribute_name, is_valid, valid_iteration_rule_with_only_mandatory_fields + ): + data = valid_iteration_rule_with_only_mandatory_fields.copy() + data["AttributeLevel"] = attribute_level + data["AttributeName"] = attribute_name + + if is_valid: + # Should validate with no exceptions + IterationRuleValidation(**data) + else: + with pytest.raises(ValidationError) as exc: + IterationRuleValidation(**data) + + assert "AttributeName must be set" in str(exc.value) + + @pytest.mark.parametrize( + ("attribute_level", "attribute_target", "attribute_name", "is_valid"), + [ + (RuleAttributeLevel.TARGET, "RSV", "BOOKED_APPOINTMENT_DATE", True), # Valid: required for TARGET + (RuleAttributeLevel.TARGET, None, "BOOKED_APPOINTMENT_DATE", False), # Invalid: missing + (RuleAttributeLevel.COHORT, None, "COHORT_LABEL", True), # Valid: not required + ], + ) + def test_attribute_target_mandatory_for_target_level( + self, + attribute_level, + attribute_target, + attribute_name, + is_valid, + valid_iteration_rule_with_only_mandatory_fields, + ): + data = valid_iteration_rule_with_only_mandatory_fields.copy() + data["AttributeLevel"] = attribute_level + data["AttributeTarget"] = attribute_target + data["AttributeName"] = attribute_name + + if is_valid: + IterationRuleValidation(**data) + else: + with pytest.raises(ValidationError) as exc: + IterationRuleValidation(**data) + + assert "AttributeTarget is mandatory" in str(exc.value) From 39856a5a9f5a2ec9ac1381e997c68e832af1f060 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 28 Nov 2025 07:59:01 +0000 Subject: [PATCH 11/21] lint fix --- .../model/campaign_config.py | 32 +++++++++++++------ .../validators/iteration_rules_validator.py | 8 +++-- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index a107d669b..492094dfc 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -274,12 +274,18 @@ class Iteration(BaseModel): def parse_dates(cls, v: str | date) -> date: if isinstance(v, date): return v - if not re.fullmatch(r"\d{8}", str(v)): - raise ValueError(f"Invalid format: {v}. Must be YYYYMMDD with 8 digits.") + + v_str = str(v) + + if not re.fullmatch(r"\d{8}", v_str): + msg = f"Invalid format: {v_str}. Must be YYYYMMDD with 8 digits." + raise ValueError(msg) + try: - return datetime.strptime(str(v), "%Y%m%d").date() # noqa: DTZ007 - except ValueError: - raise ValueError(f"Invalid date value: {v}. Must be a valid calendar date in YYYYMMDD format.") + return datetime.strptime(v_str, "%Y%m%d").date() # noqa: DTZ007 + except ValueError as err: + msg = f"Invalid date value: {v_str}. Must be a valid calendar date in YYYYMMDD format." + raise ValueError(msg) from err @field_serializer("iteration_date", when_used="always") @staticmethod @@ -322,12 +328,18 @@ class CampaignConfig(BaseModel): def parse_dates(cls, v: str | date) -> date: if isinstance(v, date): return v - if not re.fullmatch(r"\d{8}", str(v)): - raise ValueError(f"Invalid format: {v}. Must be YYYYMMDD with 8 digits.") + + v_str = str(v) + + if not re.fullmatch(r"\d{8}", v_str): + msg = f"Invalid format: {v_str}. Must be YYYYMMDD with 8 digits." + raise ValueError(msg) + try: - return datetime.strptime(str(v), "%Y%m%d").date() # noqa: DTZ007 - except ValueError: - raise ValueError(f"Invalid date value: {v}. Must be a valid calendar date in YYYYMMDD format.") + return datetime.strptime(v_str, "%Y%m%d").date() # noqa: DTZ007 + except ValueError as err: + msg = f"Invalid date value: {v_str}. Must be a valid calendar date in YYYYMMDD format." + raise ValueError(msg) from err @field_serializer("start_date", "end_date", when_used="always") @staticmethod diff --git a/src/rules_validation_api/validators/iteration_rules_validator.py b/src/rules_validation_api/validators/iteration_rules_validator.py index 2acd19ded..ccce344e8 100644 --- a/src/rules_validation_api/validators/iteration_rules_validator.py +++ b/src/rules_validation_api/validators/iteration_rules_validator.py @@ -27,7 +27,7 @@ def check_cohort_label_for_non_f_and_s_types(self) -> typing.Self: allowed_types = {RuleType("F"), RuleType("S")} if self.cohort_label is not None and self.type not in allowed_types: msg = ( - f"CohortLabel is only allowed for rule types F and S. " + "CohortLabel is only allowed for rule types F and S. " f"Found type: {self.type} with cohort_label: {self.cohort_label}" ) raise ValueError(msg) @@ -39,7 +39,8 @@ def validate_attribute_name_is_optional_only_for_cohort_attribute_level(self) -> return self if self.attribute_level == RuleAttributeLevel.COHORT: return self - raise ValueError(f"AttributeName must be set where AttributeLevel is {self.attribute_level} .") + msg = f"AttributeName must be set where AttributeLevel is {self.attribute_level}." + raise ValueError(msg) @model_validator(mode="after") def validate_attribute_target_is_mandatory_for_target_attribute_level(self) -> typing.Self: @@ -47,4 +48,5 @@ def validate_attribute_target_is_mandatory_for_target_attribute_level(self) -> t return self if self.attribute_level != RuleAttributeLevel.TARGET: return self - raise ValueError(f"AttributeTarget is mandatory where AttributeLevel is {self.attribute_level}.") + msg = f"AttributeTarget is mandatory where AttributeLevel is {self.attribute_level}." + raise ValueError(msg) From 6ff59e4ba6d3ac56d6884bda4a72be68b4dcb80d Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 28 Nov 2025 08:40:52 +0000 Subject: [PATCH 12/21] code cleanup --- .../validators/available_action_validator.py | 4 +--- src/rules_validation_api/validators/iteration_validator.py | 6 +----- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/src/rules_validation_api/validators/available_action_validator.py b/src/rules_validation_api/validators/available_action_validator.py index 745fcd853..07e80c315 100644 --- a/src/rules_validation_api/validators/available_action_validator.py +++ b/src/rules_validation_api/validators/available_action_validator.py @@ -1,13 +1,11 @@ import re -from pydantic import Field, field_validator +from pydantic import field_validator from eligibility_signposting_api.model.campaign_config import AvailableAction class AvailableActionValidation(AvailableAction): - action_description: str | None = Field(None, alias="ActionDescription") - @field_validator("action_description") @classmethod def validate_description_style(cls, text: str) -> str: diff --git a/src/rules_validation_api/validators/iteration_validator.py b/src/rules_validation_api/validators/iteration_validator.py index 0e9e46b9b..725065187 100644 --- a/src/rules_validation_api/validators/iteration_validator.py +++ b/src/rules_validation_api/validators/iteration_validator.py @@ -1,6 +1,6 @@ import typing -from pydantic import Field, ValidationError, field_validator, model_validator +from pydantic import ValidationError, field_validator, model_validator from pydantic_core import InitErrorDetails from eligibility_signposting_api.model.campaign_config import ( @@ -18,10 +18,6 @@ class IterationValidation(Iteration): - iteration_cohorts: list[IterationCohort] = Field(..., alias="IterationCohorts") - iteration_rules: list[IterationRule] = Field(..., alias="IterationRules") - actions_mapper: ActionsMapper = Field(..., alias="ActionsMapper") - @field_validator("iteration_rules") @classmethod def validate_iteration_rules(cls, iteration_rules: list[IterationRule]) -> list[IterationRuleValidation]: From b572f6c7479cd6eff75317279a97651361adbfd2 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 28 Nov 2025 11:28:21 +0000 Subject: [PATCH 13/21] cohort priority should be unique --- .../validators/iteration_validator.py | 40 +++++++++++++++---- tests/unit/validation/conftest.py | 4 +- .../validation/test_iteration_validator.py | 29 ++++++++++++++ 3 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/rules_validation_api/validators/iteration_validator.py b/src/rules_validation_api/validators/iteration_validator.py index 725065187..5f5f32143 100644 --- a/src/rules_validation_api/validators/iteration_validator.py +++ b/src/rules_validation_api/validators/iteration_validator.py @@ -25,22 +25,46 @@ def validate_iteration_rules(cls, iteration_rules: list[IterationRule]) -> list[ @field_validator("iteration_cohorts") @classmethod - def validate_iteration_cohorts(cls, iteration_cohorts: list[IterationCohort]) -> list[IterationCohortValidation]: + def validate_iteration_cohorts( + cls, iteration_cohorts: list[IterationCohort] + ) -> list[IterationCohortValidation]: seen_labels = set() + seen_priorities = set() errors = [] + for cohort in iteration_cohorts: label = cohort.cohort_label + priority = cohort.priority + + # Duplicate label check if label in seen_labels: - error = InitErrorDetails( - type="value_error", - loc=("iteration_cohort",), - input=label, - ctx={"error": f"Duplicate iteration_cohort: {label}"}, + errors.append( + InitErrorDetails( + type="value_error", + loc=("iteration_cohort", "cohort_label"), + input=label, + ctx={"error": f"Duplicate iteration_cohort label: {label}"}, + ) ) - errors.append(error) seen_labels.add(label) + + # Duplicate priority check + if priority in seen_priorities: + errors.append( + InitErrorDetails( + type="value_error", + loc=("iteration_cohort", "priority"), + input=priority, + ctx={"error": f"Duplicate iteration_cohort priority: {priority}"}, + ) + ) + seen_priorities.add(priority) + if errors: - raise ValidationError.from_exception_data(title="IterationValidation", line_errors=errors) + raise ValidationError.from_exception_data( + title="IterationValidation", line_errors=errors + ) + return [IterationCohortValidation(**i.model_dump()) for i in iteration_cohorts] @field_validator("actions_mapper", mode="after") diff --git a/tests/unit/validation/conftest.py b/tests/unit/validation/conftest.py index 2870b29a9..41f8014ed 100644 --- a/tests/unit/validation/conftest.py +++ b/tests/unit/validation/conftest.py @@ -82,13 +82,13 @@ def valid_available_action(): @pytest.fixture def valid_iteration_cohorts(): - def _cohort(label: str = "label_1", group: str = "group_1"): + def _cohort(label: str = "label_1", group: str = "group_1", priority: int = 0): return { "CohortLabel": label, "CohortGroup": group, "PositiveDescription": "are a member of eli_399_cohort_group [[PERSON.POSTCODE:DATE(%d %B %Y)]]", "NegativeDescription": "are not a member of eli_399_cohort_group [[PERSON.POSTCODE:DATE(%d %B %Y)]]", - "Priority": 0, + "Priority": priority, } return _cohort diff --git a/tests/unit/validation/test_iteration_validator.py b/tests/unit/validation/test_iteration_validator.py index 7475c60db..5c08bb4cc 100644 --- a/tests/unit/validation/test_iteration_validator.py +++ b/tests/unit/validation/test_iteration_validator.py @@ -441,3 +441,32 @@ def test_invalid_iteration_if_more_than_one_cohort_has_the_same_cohort_label( assert label_counts["label_2"] == expected_label_2_error_count, ( f"Expected {expected_label_2_error_count} error for label_2, got {label_counts['label_2']}" ) + + def test_invalid_iteration_if_more_than_one_cohort_has_the_same_priority( + self, valid_campaign_config_with_only_mandatory_fields, valid_iteration_cohorts + ): + iteration_data = { + **valid_campaign_config_with_only_mandatory_fields["Iterations"][0], + "IterationCohorts": [ + valid_iteration_cohorts(label="label_1", priority=1, group="group_1"), + valid_iteration_cohorts(label="label_2", priority=1, group="group_1"), + valid_iteration_cohorts(label="label_3", priority=1, group="group_1"), + ], + } + + with pytest.raises(ValidationError) as error: + IterationValidation(**iteration_data) + + errors = error.value.errors() + # Extract all cohort_label mentions from error inputs + label_mentions = [err["input"] for err in errors if err.get("ctx")] + + # Count occurrences + priority_counts = Counter(label_mentions) + + # Assert expected counts + expected_priority_1_error_count = 2 + + assert priority_counts[1] == expected_priority_1_error_count, ( + f"Expected {expected_priority_1_error_count} errors for priority '1' , got {priority_counts['label_1']}" + ) From 336f90da4244a8817da20b79b053c492e58b8541 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 28 Nov 2025 11:28:45 +0000 Subject: [PATCH 14/21] cohort priority should be unique --- .../validators/iteration_validator.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/rules_validation_api/validators/iteration_validator.py b/src/rules_validation_api/validators/iteration_validator.py index 5f5f32143..87950689d 100644 --- a/src/rules_validation_api/validators/iteration_validator.py +++ b/src/rules_validation_api/validators/iteration_validator.py @@ -25,9 +25,7 @@ def validate_iteration_rules(cls, iteration_rules: list[IterationRule]) -> list[ @field_validator("iteration_cohorts") @classmethod - def validate_iteration_cohorts( - cls, iteration_cohorts: list[IterationCohort] - ) -> list[IterationCohortValidation]: + def validate_iteration_cohorts(cls, iteration_cohorts: list[IterationCohort]) -> list[IterationCohortValidation]: seen_labels = set() seen_priorities = set() errors = [] @@ -61,9 +59,7 @@ def validate_iteration_cohorts( seen_priorities.add(priority) if errors: - raise ValidationError.from_exception_data( - title="IterationValidation", line_errors=errors - ) + raise ValidationError.from_exception_data(title="IterationValidation", line_errors=errors) return [IterationCohortValidation(**i.model_dump()) for i in iteration_cohorts] From 78422d4a5540b7680fa4fcdbcdca59411c5758e8 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 1 Dec 2025 17:35:40 +0000 Subject: [PATCH 15/21] rules mapper has its own class now --- .../model/campaign_config.py | 12 ++++++++++-- tests/fixtures/builders/model/rule.py | 10 +++++++++- tests/integration/conftest.py | 19 +++++++++++-------- .../calculators/test_rule_calculator.py | 6 +++++- 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index 492094dfc..8c6e8c7c6 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -238,7 +238,7 @@ class StatusText(BaseModel): not_actionable: str | None = Field(None, alias="NotActionable") actionable: str | None = Field(None, alias="Actionable") - model_config = {"populate_by_name": True, "extra": "ignore"} + model_config = {"populate_by_name": True} class RuleEntry(BaseModel): @@ -249,6 +249,14 @@ class RuleEntry(BaseModel): model_config = {"populate_by_name": True} +class RulesMapper(RootModel[dict[str, RuleEntry]]): + def get(self, key: str, default: RuleEntry | None = None) -> RuleEntry | None: + return self.root.get(key, default) + + def values(self) -> list[RuleEntry]: + return list(self.root.values()) + + class Iteration(BaseModel): id: IterationID = Field(..., alias="ID") version: IterationVersion = Field(..., alias="Version") @@ -264,7 +272,7 @@ class Iteration(BaseModel): iteration_cohorts: list[IterationCohort] = Field(..., alias="IterationCohorts") iteration_rules: list[IterationRule] = Field(..., alias="IterationRules") actions_mapper: ActionsMapper = Field(..., alias="ActionsMapper") - rules_mapper: dict[str, RuleEntry] | None = Field(None, alias="RulesMapper") + rules_mapper: RulesMapper | None = Field(None, alias="RulesMapper") status_text: StatusText | None = Field(None, alias="StatusText") model_config = {"populate_by_name": True, "arbitrary_types_allowed": True, "extra": "ignore"} diff --git a/tests/fixtures/builders/model/rule.py b/tests/fixtures/builders/model/rule.py index 5fafec8ba..bf62de900 100644 --- a/tests/fixtures/builders/model/rule.py +++ b/tests/fixtures/builders/model/rule.py @@ -24,6 +24,7 @@ RuleName, RuleOperator, RulePriority, + RulesMapper, RuleType, StatusText, Virtual, @@ -71,7 +72,14 @@ class StatusTextFactory(ModelFactory[StatusText]): actionable = "Actionable status text" -class RuleEntryFactory(ModelFactory[RuleEntry]): ... +class RuleEntryFactory(ModelFactory[RuleEntry]): + RuleNames = ([RuleName("age_rule_name1")],) + RuleCode = ("Age rule code from mapper",) + RuleText = "Age Rule Description from mapper" + + +class RulesMapperFactory(ModelFactory[RulesMapper]): + root = Use(lambda: {"OTHER_SETTINGS": RuleEntryFactory.build()}) class IterationFactory(ModelFactory[Iteration]): diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 8c8b4715f..71e28142b 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -33,6 +33,7 @@ from eligibility_signposting_api.repos.campaign_repo import BucketName from eligibility_signposting_api.repos.person_repo import TableName from tests.fixtures.builders.model import rule +from tests.fixtures.builders.model.rule import RulesMapperFactory from tests.fixtures.builders.repos.person import person_rows_builder if TYPE_CHECKING: @@ -603,14 +604,16 @@ def campaign_config_with_rules_having_rule_mapper( negative_description="negative_description", ) ], - rules_mapper={ - "OTHER_SETTINGS": RuleEntry( - RuleNames=[RuleName("age_rule_name1")], - RuleCode=RuleCode("Age rule code from mapper"), - RuleText=RuleText("Age Rule Description from mapper"), - ), - "ALREADY_JABBED": RuleEntry(RuleNames=[], RuleCode=RuleCode(""), RuleText=RuleText("")), - }, + rules_mapper=RulesMapperFactory.build( + root={ + "OTHER_SETTINGS": RuleEntry( + RuleNames=[RuleName("age_rule_name1")], + RuleCode=RuleCode("Age rule code from mapper"), + RuleText=RuleText("Age Rule Description from mapper"), + ), + "ALREADY_JABBED": RuleEntry(RuleNames=[], RuleCode=RuleCode(""), RuleText=RuleText("")), + } + ), status_text=None, ) ], diff --git a/tests/unit/services/calculators/test_rule_calculator.py b/tests/unit/services/calculators/test_rule_calculator.py index d35ead47a..ad3a363ab 100644 --- a/tests/unit/services/calculators/test_rule_calculator.py +++ b/tests/unit/services/calculators/test_rule_calculator.py @@ -10,6 +10,7 @@ RuleCode, RuleEntry, RuleName, + RulesMapper, RuleText, ) from eligibility_signposting_api.model.eligibility_status import Status @@ -102,11 +103,14 @@ def test_rule_code_resolution_in_evaluate_exclusion_function_for_rule_code_and_r RuleCode=RuleCode("POSTCODE_RULE_CODE_FROM_MAPPER"), RuleText=RuleText("POSTCODE_RULE_TEXT_FROM_MAPPER"), ) - rules_mapper = { + + rules_mapper_data = { "OTHER_SETTINGS": rule_entry, "ALREADY_JABBED": RuleEntry(RuleNames=[], RuleCode=RuleCode(""), RuleText=RuleText("")), } + rules_mapper: RulesMapper = RulesMapper(root=rules_mapper_data) + rule = rule_builder.IterationRuleFactory.build( name="POSTCODE_RULE_NAME", attribute_level=RuleAttributeLevel.PERSON, From e3048fcb0ac03842630e6822021ab39731a1664d Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 1 Dec 2025 18:10:39 +0000 Subject: [PATCH 16/21] Iteration dates are checked for each iteration together --- .../validators/campaign_config_validator.py | 23 +-- .../test_config/test_config_v1.2.0.json | 176 +++++++++++++++++- tests/unit/validation/conftest.py | 18 +- .../test_campaign_config_validator.py | 50 ++++- 4 files changed, 249 insertions(+), 18 deletions(-) diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index 3b5cf509d..0baf3419a 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -34,18 +34,19 @@ def validate_iterations_have_unique_id(self) -> typing.Self: @model_validator(mode="after") def validate_campaign_has_iteration_within_schedule(self) -> typing.Self: + errors: list[str] = [] iterations_by_date = sorted(self.iterations, key=attrgetter("iteration_date")) - if first_iteration := next(iter(iterations_by_date), None): - if first_iteration.iteration_date < self.start_date: - msg = f"Iteration {first_iteration.id} starts before campaign {self.id} start date {self.start_date}." - raise ValueError(msg) - if first_iteration.iteration_date > self.end_date: - msg = f"Iteration {first_iteration.id} starts after campaign {self.id} end date {self.end_date}." - raise ValueError(msg) + for iteration in iterations_by_date: + if iteration.iteration_date < self.start_date: + errors.append( + f"\nIteration {iteration.id} starts before campaign {self.id} start date {self.start_date}." + ) + if iteration.iteration_date > self.end_date: + errors.append(f"\nIteration {iteration.id} starts after campaign {self.id} end date {self.end_date}.") - return self + if errors: + # Raise one exception with all messages joined + raise ValueError("".join(errors)) - # Should never happen, since we are constraining self.iterations with a min_length of 1 - msg = f"campaign {self.id} has no iterations." - raise ValueError(msg) + return self diff --git a/tests/test_data/test_config/test_config_v1.2.0.json b/tests/test_data/test_config/test_config_v1.2.0.json index 9c1aa161f..15c568a20 100644 --- a/tests/test_data/test_config/test_config_v1.2.0.json +++ b/tests/test_data/test_config/test_config_v1.2.0.json @@ -27,7 +27,7 @@ "Version": "1", "Name": "Test Config", "Type": "M", - "IterationDate": "20250101", + "IterationDate": "20250103", "IterationNumber": 1, "CommsType": "R", "ApprovalMinimum": 1, @@ -97,7 +97,179 @@ "CohortGroup": "rsv_age_catchup", "PositiveDescription": "You turned 80 after 1 September 2024, so are eligible for the RSV vaccine until 31 August 2025", "NegativeDescription": "You did not turn 80 after 1 September 2024 and get vaccinated by 31 August 2025", - "Priority": 10, + "Priority": 101, + "Virtual": "Y" + } + ], + "IterationRules": [ + { + "Type": "F", + "Name": "Test Rule", + "Description": "Test Rule Desc", + "Priority": 20, + "AttributeLevel": "PERSON", + "AttributeNames": "DATE_OF_BIRTH", + "Operator": "=", + "Comparator": "19000101" + }, + { + "Type": "F", + "Name": "Test Rule", + "Description": "Test Rule Desc", + "Priority": 30, + "AttributeLevel": "PERSON", + "AttributeName": "place of birth", + "Operator": "=", + "Comparator": "london" + }, + { + "Type": "S", + "Name": "Already Vaccinated", + "Description": "Already Vaccinated|You have already been Vaccinated", + "Priority": 30, + "AttributeLevel": "TARGET", + "AttributeTarget": "RSV", + "AttributeName": "LAST_SUCCESSFUL_DATE", + "CohortLabel": "rsv_75to79_2024", + "Operator": "is_not_null", + "Comparator": "" + }, + { + "Type": "S", + "Name": "In Supressed Cohort", + "Description": "In Supressed Cohort|You Are In Supressed Cohort", + "Priority": 40, + "AttributeLevel": "COHORT", + "AttributeName": "COHORT_LABEL", + "Operator": "=", + "Comparator": "rsv_75to79_2024" + }, + { + "Type": "R", + "Name": "Test Redirect Rule", + "Description": "Test Redirect Rule Desc", + "Priority": 20, + "AttributeLevel": "PERSON", + "AttributeName": "DATE_OF_BIRTH", + "Operator": ">", + "Comparator": "19000101", + "CommsRouting": "INTERNALCONTACTGP1|INTERNALTESCO" + }, + { + "Type": "X", + "Name": "Test X Rule for not eligible", + "Description": "Test X Rule Desc", + "Priority": 20, + "AttributeLevel": "PERSON", + "AttributeName": "DATE_OF_BIRTH", + "Operator": ">", + "Comparator": "19000101", + "CommsRouting": "XRULEID1|INTERNALTESCO" + }, + { + "Type": "Y", + "Name": "Test Y Rule for not actionable", + "Description": "Test Y Rule Desc", + "Priority": 20, + "AttributeLevel": "PERSON", + "AttributeName": "DATE_OF_BIRTH", + "Operator": ">", + "Comparator": "19000101", + "CommsRouting": "YRULEID1|INTERNALTESCO" + } + ], + "RulesMapper": { + "ALREADY_JABBED": { + "RuleNames": [ + "Already Vaccinated" + ], + "RuleCode": "Already Jabbed", + "RuleText": "Already Vaccinated|You have already been Vaccinated" + }, + "OTHER_SETTINGS": { + "RuleNames": [ + "In Supressed Cohort" + ], + "RuleCode": "Present in Supressed Cohort" + } + } + }, + { + "ID": "id_101", + "Version": "1", + "Name": "Test Config", + "Type": "M", + "IterationDate": "20250102", + "IterationNumber": 1, + "CommsType": "R", + "ApprovalMinimum": 1, + "ApprovalMaximum": 5000000, + "DefaultCommsRouting": "INTERNALCONTACTGP1", + "DefaultNotActionableRouting": "INTERNALCONTACTGP1", + "DefaultNotEligibleRouting": "INTERNALCONTACTGP1", + "StatusText": { + "NotEligible": "You are not eligible to take RSV vaccines", + "NotActionable": "You have taken RSV vaccine in the last 90 days", + "Actionable": "You can take RSV vaccine" + }, + "ActionsMapper": { + "INTERNALCONTACTGP1": { + "ExternalRoutingCode": "CONTACTGP", + "ActionDescription": "Contact GP Text1 description", + "ActionType": "text1" + }, + "INTERNALCONTACTGP2": { + "ExternalRoutingCode": "CONTACTGP", + "ActionDescription": "Contact GP Link description", + "ActionType": "link", + "UrlLink": "https://www.link123.example", + "UrlLabel": "link label" + }, + "INTERNALTESCO": { + "ExternalRoutingCode": "TESCO", + "ActionDescription": "Tesco description", + "ActionType": "link", + "UrlLink": "https://www.tesco_link.example", + "UrlLabel": "link label" + }, + "INTERNALFINDWALKIN": { + "ExternalRoutingCode": "FINDWALKIN", + "ActionDescription": "Find walkin description", + "ActionType": "button" + }, + "XRULEID1": { + "ExternalRoutingCode": "FINDWALKIN", + "ActionDescription": "Find walkin description", + "ActionType": "button" + }, + "YRULEID1": { + "ExternalRoutingCode": "FINDWALKIN", + "ActionDescription": "Find walkin description", + "ActionType": "button" + } + }, + "IterationCohorts": [ + { + "CohortLabel": "rsv_75_rolling", + "CohortGroup": "rsv_age_range", + "PositiveDescription": "You are currently aged 75 to 79", + "NegativeDescription": "You are not currently aged 75 to 79", + "Priority": 0, + "Virtual": "N" + }, + { + "CohortLabel": "rsv_75to79_2024", + "CohortGroup": "rsv_catch_up_age_range", + "PositiveDescription": "You turned 80 after 1 September 2024, so are eligible for the RSV vaccine until 31 August 2025", + "NegativeDescription": "You did not turn 80 after 1 September 2024 and get vaccinated by 31 August 2025", + "Priority": 10 + }, + { + "CohortLabel": "virtual_rsv_80_since_02_Sept_2024", + "CohortGroup": "rsv_age_catchup", + "PositiveDescription": "You turned 80 after 1 September 2024, so are eligible for the RSV vaccine until 31 August 2025", + "NegativeDescription": "You did not turn 80 after 1 September 2024 and get vaccinated by 31 August 2025", + "Priority": 101, "Virtual": "Y" } ], diff --git a/tests/unit/validation/conftest.py b/tests/unit/validation/conftest.py index 41f8014ed..c5f676407 100644 --- a/tests/unit/validation/conftest.py +++ b/tests/unit/validation/conftest.py @@ -29,7 +29,23 @@ def valid_campaign_config_with_only_mandatory_fields(): "IterationCohorts": [], "IterationRules": [], "ActionsMapper": {}, - } + }, + { + "ID": "ITER002", + "Version": 1, + "Name": "Mid-January Push", + "IterationDate": "20250110", + "IterationNumber": 2, + "ApprovalMinimum": 10, + "ApprovalMaximum": 100, + "Type": "A", + "DefaultCommsRouting": "", + "DefaultNotEligibleRouting": "", + "DefaultNotActionableRouting": "", + "IterationCohorts": [], + "IterationRules": [], + "ActionsMapper": {}, + }, ], } diff --git a/tests/unit/validation/test_campaign_config_validator.py b/tests/unit/validation/test_campaign_config_validator.py index c7321aef4..d81c0fa29 100644 --- a/tests/unit/validation/test_campaign_config_validator.py +++ b/tests/unit/validation/test_campaign_config_validator.py @@ -197,7 +197,6 @@ class TestBUCValidations: [ ("20250101", "20250331"), # valid range ("20250201", "20250228"), # valid short range - ("20250202", "20250202"), # same day ], ) def test_valid_start_and_end_dates_and_iteration_dates_relation( @@ -207,25 +206,68 @@ def test_valid_start_and_end_dates_and_iteration_dates_relation( data["StartDate"] = start_date data["EndDate"] = end_date data["Iterations"][0]["IterationDate"] = "20250202" + data["Iterations"][1]["IterationDate"] = "20250203" + CampaignConfigValidation(**data) + + # StartDate and EndDates + @pytest.mark.parametrize( + ("start_date", "end_date"), + [ + ("20250202", "20250202"), # same day + ], + ) + def test_valid_start_and_end_dates_and_iteration_dates_relation_for_a_one_day_campaign( + self, start_date, end_date, valid_campaign_config_with_only_mandatory_fields + ): + data = valid_campaign_config_with_only_mandatory_fields.copy() + data["StartDate"] = start_date + data["EndDate"] = end_date + data["Iterations"][0]["IterationDate"] = "20250202" + data["Iterations"].pop(1) CampaignConfigValidation(**data) @pytest.mark.parametrize( ("start_date", "end_date"), [ - ("20241230", "20250101"), # Campaign start date is after the iteration date ("20240729", "20241228"), # Campaign ends date is before the iteration date ], ) - def test_invalid_start_and_end_dates_and_iteration_dates_relation( + def test_invalid_end_dates_and_iteration_dates_relation( self, start_date, end_date, valid_campaign_config_with_only_mandatory_fields ): data = valid_campaign_config_with_only_mandatory_fields.copy() data["StartDate"] = start_date data["EndDate"] = end_date data["Iterations"][0]["IterationDate"] = "20241229" - with pytest.raises(ValidationError): + data["Iterations"][1]["IterationDate"] = "20241230" + with pytest.raises(ValidationError) as exc_info: CampaignConfigValidation(**data) + errors = exc_info.value.errors() + assert "Iteration ITER001 starts after" in errors[0]["msg"] + assert "Iteration ITER002 starts after" in errors[0]["msg"] + + @pytest.mark.parametrize( + ("start_date", "end_date"), + [ + ("20241230", "20250101"), # Campaign start date is after the iteration date + ], + ) + def test_invalid_start_date_and_iteration_dates_relation( + self, start_date, end_date, valid_campaign_config_with_only_mandatory_fields + ): + data = valid_campaign_config_with_only_mandatory_fields.copy() + data["StartDate"] = start_date + data["EndDate"] = end_date + data["Iterations"][0]["IterationDate"] = "20241229" + data["Iterations"][1]["IterationDate"] = "20241228" + with pytest.raises(ValidationError) as exc_info: + CampaignConfigValidation(**data) + + errors = exc_info.value.errors() + assert "Iteration ITER001 starts before" in errors[0]["msg"] + assert "Iteration ITER002 starts before" in errors[0]["msg"] + # Iteration def test_validate_iterations_non_empty(self, valid_campaign_config_with_only_mandatory_fields): data = {**valid_campaign_config_with_only_mandatory_fields, "Iterations": []} From 22a11278fa086847fb80445dd5ade879c9785fbf Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 1 Dec 2025 19:01:46 +0000 Subject: [PATCH 17/21] Iteration all iteration rules errors are reported together --- .../validators/iteration_validator.py | 18 +++++++++- .../test_config/test_config_v1.2.0.json | 2 +- .../validation/test_iteration_validator.py | 36 ++++++++++++++++++- 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/rules_validation_api/validators/iteration_validator.py b/src/rules_validation_api/validators/iteration_validator.py index 87950689d..61dc2376a 100644 --- a/src/rules_validation_api/validators/iteration_validator.py +++ b/src/rules_validation_api/validators/iteration_validator.py @@ -21,7 +21,23 @@ class IterationValidation(Iteration): @field_validator("iteration_rules") @classmethod def validate_iteration_rules(cls, iteration_rules: list[IterationRule]) -> list[IterationRuleValidation]: - return [IterationRuleValidation(**i.model_dump()) for i in iteration_rules] + errors: list = [] + validated: list[IterationRuleValidation] = [] + + for idx, i in enumerate(iteration_rules): + try: + validated.append(IterationRuleValidation(**i.model_dump())) + except ValidationError as ve: + for err in ve.errors(): + # adjust the location to include the index + err["loc"] = ("iteration_rules", idx, *tuple(err["loc"])) + errors.append(err) + + if errors: + # raise one ValidationError with all collected errors + raise ValidationError.from_exception_data("IterationValidation", errors) # noqa: EM101 + + return validated @field_validator("iteration_cohorts") @classmethod diff --git a/tests/test_data/test_config/test_config_v1.2.0.json b/tests/test_data/test_config/test_config_v1.2.0.json index 15c568a20..1ceb9190d 100644 --- a/tests/test_data/test_config/test_config_v1.2.0.json +++ b/tests/test_data/test_config/test_config_v1.2.0.json @@ -108,7 +108,7 @@ "Description": "Test Rule Desc", "Priority": 20, "AttributeLevel": "PERSON", - "AttributeNames": "DATE_OF_BIRTH", + "AttributeName": "DATE_OF_BIRTH", "Operator": "=", "Comparator": "19000101" }, diff --git a/tests/unit/validation/test_iteration_validator.py b/tests/unit/validation/test_iteration_validator.py index 5c08bb4cc..3c58c33c9 100644 --- a/tests/unit/validation/test_iteration_validator.py +++ b/tests/unit/validation/test_iteration_validator.py @@ -465,8 +465,42 @@ def test_invalid_iteration_if_more_than_one_cohort_has_the_same_priority( priority_counts = Counter(label_mentions) # Assert expected counts - expected_priority_1_error_count = 2 + expected_error_count = 2 + expected_priority_1_error_count = expected_error_count assert priority_counts[1] == expected_priority_1_error_count, ( f"Expected {expected_priority_1_error_count} errors for priority '1' , got {priority_counts['label_1']}" ) + + def test_invalid_iteration_collects_errors_if_iteration_rules_have_invalid_data( + self, + valid_iteration_with_only_mandatory_fields, + valid_iteration_rule_with_only_mandatory_fields, + ): + # Create two invalid rules + iteration_rule_1 = valid_iteration_rule_with_only_mandatory_fields.copy() + iteration_rule_1["AttributeName"] = None + iteration_rule_2 = valid_iteration_rule_with_only_mandatory_fields.copy() + iteration_rule_2["AttributeName"] = None + + data = { + **valid_iteration_with_only_mandatory_fields, + "IterationRules": [iteration_rule_1, iteration_rule_2], + } + + with pytest.raises(ValidationError) as exc_info: + IterationValidation(**data) + + errors = exc_info.value.errors() + + # Assert both errors are present + expected_error_count = 2 + assert len(errors) == expected_error_count + + # Assert locations are correct + assert errors[0]["loc"][0] == "IterationRules" + assert errors[1]["loc"][0] == "IterationRules" + + # Assert messages contain the expected text + assert "AttributeName must be set" in errors[0]["msg"] + assert "AttributeName must be set" in errors[1]["msg"] From 743168babe5fe909665601eca9f9761250a9a5f5 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 1 Dec 2025 22:02:33 +0000 Subject: [PATCH 18/21] refined the error messages --- src/rules_validation_api/app.py | 36 ++++++++++++++----- .../validators/campaign_config_validator.py | 14 +++++--- .../test_campaign_config_validator.py | 8 ++--- 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/rules_validation_api/app.py b/src/rules_validation_api/app.py index 1ff67b782..4c50449a9 100644 --- a/src/rules_validation_api/app.py +++ b/src/rules_validation_api/app.py @@ -4,21 +4,37 @@ import sys from pathlib import Path +from pydantic import ValidationError + from rules_validation_api.validators.rules_validator import RulesValidation logging.basicConfig( - level=logging.INFO, # or DEBUG for more detail + level=logging.INFO, format="%(asctime)s | %(levelname)s | %(name)s | %(message)s", force=True, ) -GREEN = "\033[92m" # pragma: no cover -RESET = "\033[0m" # pragma: no cover -YELLOW = "\033[93m" # pragma: no cover -RED = "\033[91m" # pragma: no cover +GREEN = "\033[92m" +RESET = "\033[0m" +YELLOW = "\033[93m" +RED = "\033[91m" + + +def refine_error(e: ValidationError) -> str: + """Return a very short, single-line error message.""" + lines = [f"Validation Error: {len(e.errors())} validation error(s)"] + + for err in e.errors(): + loc = ".".join(str(x) for x in err["loc"]) + msg = err["msg"] + type_ = err["type"] + lines.append(f"{loc} : {msg} [type={type_}]") -def main() -> None: # pragma: no cover + return "\n".join(lines) + + +def main() -> None: parser = argparse.ArgumentParser(description="Validate campaign configuration.") parser.add_argument("--config_path", required=True, help="Path to the campaign config JSON file") args = parser.parse_args() @@ -28,9 +44,11 @@ def main() -> None: # pragma: no cover json_data = json.load(file) RulesValidation(**json_data) sys.stdout.write(f"{GREEN}Valid Config{RESET}\n") - except ValueError as e: - sys.stderr.write(f"{YELLOW}Validation Error:{RESET} {RED}{e}{RESET}\n") + + except ValidationError as e: + clean = refine_error(e) + sys.stderr.write(f"{YELLOW}{clean}{RESET}\n") -if __name__ == "__main__": # pragma: no cover +if __name__ == "__main__": main() diff --git a/src/rules_validation_api/validators/campaign_config_validator.py b/src/rules_validation_api/validators/campaign_config_validator.py index 0baf3419a..d2a6bcc9b 100644 --- a/src/rules_validation_api/validators/campaign_config_validator.py +++ b/src/rules_validation_api/validators/campaign_config_validator.py @@ -37,16 +37,20 @@ def validate_campaign_has_iteration_within_schedule(self) -> typing.Self: errors: list[str] = [] iterations_by_date = sorted(self.iterations, key=attrgetter("iteration_date")) - for iteration in iterations_by_date: + for idx, iteration in enumerate(iterations_by_date): if iteration.iteration_date < self.start_date: errors.append( - f"\nIteration {iteration.id} starts before campaign {self.id} start date {self.start_date}." + f"CampaignConfig.Iterations.{idx}.IterationDate : " + f"Starts before campaign start date {self.start_date} [type=invalid]" ) if iteration.iteration_date > self.end_date: - errors.append(f"\nIteration {iteration.id} starts after campaign {self.id} end date {self.end_date}.") + errors.append( + f"CampaignConfig.Iterations.{idx}.IterationDate : " + f"Starts after campaign end date {self.end_date} [type=invalid]" + ) if errors: - # Raise one exception with all messages joined - raise ValueError("".join(errors)) + # Raise one exception with all messages joined by newlines + raise ValueError("\n".join(errors)) return self diff --git a/tests/unit/validation/test_campaign_config_validator.py b/tests/unit/validation/test_campaign_config_validator.py index d81c0fa29..216369a98 100644 --- a/tests/unit/validation/test_campaign_config_validator.py +++ b/tests/unit/validation/test_campaign_config_validator.py @@ -244,8 +244,8 @@ def test_invalid_end_dates_and_iteration_dates_relation( CampaignConfigValidation(**data) errors = exc_info.value.errors() - assert "Iteration ITER001 starts after" in errors[0]["msg"] - assert "Iteration ITER002 starts after" in errors[0]["msg"] + assert "Starts after" in errors[0]["msg"] + assert "Starts after" in errors[0]["msg"] @pytest.mark.parametrize( ("start_date", "end_date"), @@ -265,8 +265,8 @@ def test_invalid_start_date_and_iteration_dates_relation( CampaignConfigValidation(**data) errors = exc_info.value.errors() - assert "Iteration ITER001 starts before" in errors[0]["msg"] - assert "Iteration ITER002 starts before" in errors[0]["msg"] + assert "Starts before" in errors[0]["msg"] + assert "Starts before" in errors[0]["msg"] # Iteration def test_validate_iterations_non_empty(self, valid_campaign_config_with_only_mandatory_fields): From 32385ff99e8cc43db911112e105fe207650c5e0c Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 2 Dec 2025 09:30:25 +0000 Subject: [PATCH 19/21] validate attribute target --- .../validators/iteration_rules_validator.py | 16 +++++++++++++++- .../validation/test_iteration_rules_validator.py | 4 ++-- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/rules_validation_api/validators/iteration_rules_validator.py b/src/rules_validation_api/validators/iteration_rules_validator.py index ccce344e8..cd082db54 100644 --- a/src/rules_validation_api/validators/iteration_rules_validator.py +++ b/src/rules_validation_api/validators/iteration_rules_validator.py @@ -1,7 +1,8 @@ import typing -from pydantic import model_validator +from pydantic import field_validator, model_validator +from eligibility_signposting_api.config.constants import ALLOWED_CONDITIONS from eligibility_signposting_api.model.campaign_config import ( IterationRule, RuleAttributeLevel, @@ -11,6 +12,19 @@ class IterationRuleValidation(IterationRule): + @field_validator("attribute_target") + @classmethod + def validate_attribute_target(cls, value: str) -> str | None: + if value is None: + return value + + allowed = ALLOWED_CONDITIONS.__args__ + if value not in allowed: + allowed_str = ", ".join(allowed) + msg = f"Invalid attribute_target value: {value}. Allowed values: {allowed_str}" + raise ValueError(msg) + return value + @model_validator(mode="after") def check_cohort_attribute_name(self) -> typing.Self: if ( diff --git a/tests/unit/validation/test_iteration_rules_validator.py b/tests/unit/validation/test_iteration_rules_validator.py index 33c66abc7..1c33f4623 100644 --- a/tests/unit/validation/test_iteration_rules_validator.py +++ b/tests/unit/validation/test_iteration_rules_validator.py @@ -203,14 +203,14 @@ def test_invalid_cohort_label(self, label, valid_iteration_rule_with_only_mandat IterationRuleValidation(**data) # AttributeTarget - @pytest.mark.parametrize("target", ["target_value"]) + @pytest.mark.parametrize("target", ["RSV", "COVID"]) def test_valid_attribute_target(self, target, valid_iteration_rule_with_only_mandatory_fields): data = valid_iteration_rule_with_only_mandatory_fields.copy() data["AttributeTarget"] = target result = IterationRuleValidation(**data) assert result.attribute_target == target - @pytest.mark.parametrize("target", [123, [], {}]) + @pytest.mark.parametrize("target", [123, [], {}, "RSV1"]) def test_invalid_attribute_target(self, target, valid_iteration_rule_with_only_mandatory_fields): data = valid_iteration_rule_with_only_mandatory_fields.copy() data["AttributeTarget"] = target From c568b9a5f958e7a3cd9d214fdcbc16cf02160d20 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 2 Dec 2025 09:47:18 +0000 Subject: [PATCH 20/21] Json schema error refined for easy readability --- index.html | 68 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/index.html b/index.html index 111769777..ae474d1d2 100644 --- a/index.html +++ b/index.html @@ -1,7 +1,7 @@ - + Campaign Config Validator - NHS Digital