From f02fb2854a05cdcebf09ce7bae3f216643904668 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 27 Feb 2026 18:48:47 +0000 Subject: [PATCH 01/23] ELI-615 | campaign having recent - active start_date supersedes the others sharing same best-status --- .../calculators/eligibility_calculator.py | 4 +- .../in_process/test_eligibility_endpoint.py | 141 +++++++++++++++++- 2 files changed, 142 insertions(+), 3 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index b7314bdcc..2a071b2bc 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -124,7 +124,9 @@ def get_eligibility_status(self, include_actions: str, conditions: list[str], ca return eligibility_status.EligibilityStatus(conditions=final_result) def get_best_iteration_result(self, campaign_group: list[CampaignConfig]) -> BestIterationResult | None: - iteration_results = self.get_iteration_results(campaign_group) + sorted_campaigns = sorted(campaign_group, key=lambda c: c.start_date, reverse=True) + + iteration_results = self.get_iteration_results(sorted_campaigns) if not iteration_results: return None diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 28989d2be..e0cc9d9d5 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1,4 +1,5 @@ import json +from datetime import date, timedelta, datetime, timezone from http import HTTPStatus import pytest @@ -25,6 +26,7 @@ from tests.fixtures.builders.model import rule from tests.integration.conftest import UNIQUE_CONSUMER_HEADER +today = lambda: datetime.now(timezone.utc).date() class TestBaseLine: def test_nhs_number_given( @@ -1192,10 +1194,11 @@ def test_valid_response_when_consumer_has_a_valid_campaign_config_mapping( # no ( [ # Campaign configs in S3 + # Note: Configs are uploaded in order so the start date would be newer down the order. ("RSV", "RSV_campaign_id_1"), ("RSV", "RSV_campaign_id_2"), - ("RSV", "RSV_campaign_id_3"), ("RSV", "RSV_campaign_id_4"), + ("RSV", "RSV_campaign_id_3"), ("RSV", "inactive_RSV_campaign_id_5", "inactive"), # inactive iteration ("RSV", "RSV_campaign_id_6"), ], @@ -1223,7 +1226,7 @@ def test_valid_response_when_consumer_has_a_valid_campaign_config_mapping( # no ], indirect=["campaign_configs", "consumer_mappings"], ) - def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_exists_per_target_giving_same_status( # noqa : PLR0913 + def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_exists_per_target_giving_same_status( self, client: FlaskClient, persisted_person: NHSNumber, @@ -1379,3 +1382,137 @@ def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campa ) ), ) + + @pytest.mark.parametrize( + ("campaign_1_start_date", "campaign_2_start_date", "postcode_for_comparator", "expected_campaign_id"), + [ + ( + ("RSV_campaign_id_1", today()), + ("RSV_campaign_id_2", today() - timedelta(days=1)), + "SW19", # postcode for resulting in not-actionable + "RSV_campaign_id_1", + ), + ( + ("RSV_campaign_id_1", today() - timedelta(days=1)), + ("RSV_campaign_id_2", today()), + "SW19", # postcode for resulting in not-actionable + "RSV_campaign_id_2", + ), + ( + ("RSV_campaign_id_1", today()), + ("RSV_campaign_id_2", today() - timedelta(days=1)), + "M4", # postcode for resulting in actionable + "RSV_campaign_id_1", + ), + ( + ("RSV_campaign_id_1", today() - timedelta(days=1)), + ("RSV_campaign_id_2", today()), + "M4", # postcode for resulting in actionable + "RSV_campaign_id_2", + ), + ], + ) + def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campaign_per_target_diff_start_date( + self, + client: FlaskClient, + persisted_person_pc_sw19: NHSNumber, + s3_client: BaseClient, + consumer_mapping_bucket: BucketName, + rules_bucket: BucketName, + audit_bucket: BucketName, + secretsmanager_client: BaseClient, # noqa: ARG002 + campaign_1_start_date: tuple[str, date], + campaign_2_start_date: tuple[str, date], + postcode_for_comparator: str, + expected_campaign_id: NHSNumber, + ): + # Given + consumer_id = "consumer-n3bs-jo4hn-ce4na" + headers = {"nhs-login-nhs-number": str(persisted_person_pc_sw19), UNIQUE_CONSUMER_HEADER: consumer_id} + + # Consumer Mapping Data + s3_client.put_object( + Bucket=consumer_mapping_bucket, + Key="consumer_mapping_config.json", + Body=json.dumps( + { + consumer_id: [ + {"CampaignConfigID": "RSV_campaign_id_1"}, + {"CampaignConfigID": "RSV_campaign_id_2"}, + ], + } + ), + ContentType="application/json", + ) + + # Campaign configs + campaign_1 = rule.CampaignConfigFactory.build( + id=campaign_1_start_date[0], + target="RSV", + start_date=campaign_1_start_date[1], + type="V", + iterations=[ + rule.IterationFactory.build( + iteration_rules=[ + rule.PostcodeSuppressionRuleFactory.build( + name="Exclude M4", comparator=RuleComparator(postcode_for_comparator) + ), + ], + iteration_cohorts=[ + rule.IterationCohortFactory.build( + cohort_label="cohort1", + cohort_group="cohort_group1", + positive_description="positive_description", + ) + ], + status_text=None, + ) + ], + ) + + campaign_2 = rule.CampaignConfigFactory.build( + id=campaign_2_start_date[0], + target="RSV", + type="V", + start_date=campaign_2_start_date[1], + iterations=[ + rule.IterationFactory.build( + iteration_rules=[ + rule.PostcodeSuppressionRuleFactory.build( + name="Exclude M4", comparator=RuleComparator(postcode_for_comparator) + ), + ], + iteration_cohorts=[ + rule.IterationCohortFactory.build( + cohort_label="cohort1", + cohort_group="cohort_group1", + positive_description="positive_description", + ) + ], + status_text=None, + ) + ], + ) + + for campaign in [campaign_1, campaign_2]: + s3_client.put_object( + Bucket=rules_bucket, + Key=f"{campaign.id}.json", + Body=json.dumps({"CampaignConfig": campaign.model_dump(by_alias=True)}), + ContentType="application/json", + ) + + # When + client.get(f"/patient-check/{persisted_person_pc_sw19}", headers=headers) + + objects = s3_client.list_objects_v2(Bucket=audit_bucket).get("Contents", []) + object_keys = [obj["Key"] for obj in objects] + latest_key = sorted(object_keys)[-1] + audit_data = json.loads(s3_client.get_object(Bucket=audit_bucket, Key=latest_key)["Body"].read()) + + # Then + if expected_campaign_id is not None: + assert_that(len(audit_data["response"]["condition"]), equal_to(1)) + assert_that(audit_data["response"]["condition"][0].get("campaignId"), equal_to(expected_campaign_id)) + else: + assert_that(len(audit_data["response"]["condition"]), equal_to(0)) From 582308a0e114fc2a0a7cddffa90d1d6cdab7bca4 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Fri, 27 Feb 2026 18:50:50 +0000 Subject: [PATCH 02/23] ELI-615 | more linting --- .../in_process/test_eligibility_endpoint.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index e0cc9d9d5..2ff477bb8 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1,5 +1,5 @@ import json -from datetime import date, timedelta, datetime, timezone +from datetime import UTC, date, datetime, timedelta from http import HTTPStatus import pytest @@ -26,7 +26,10 @@ from tests.fixtures.builders.model import rule from tests.integration.conftest import UNIQUE_CONSUMER_HEADER -today = lambda: datetime.now(timezone.utc).date() + +def today(): + return datetime.now(UTC).date() + class TestBaseLine: def test_nhs_number_given( @@ -1226,7 +1229,7 @@ def test_valid_response_when_consumer_has_a_valid_campaign_config_mapping( # no ], indirect=["campaign_configs", "consumer_mappings"], ) - def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_exists_per_target_giving_same_status( + def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_exists_per_target_giving_same_status( # noqa : PLR0913 self, client: FlaskClient, persisted_person: NHSNumber, @@ -1412,7 +1415,7 @@ def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campa ), ], ) - def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campaign_per_target_diff_start_date( + def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campaign_per_target_diff_start_date( # noqa : PLR0913 self, client: FlaskClient, persisted_person_pc_sw19: NHSNumber, From f8d4987d286dd475faef2ddac9d71b85f6ce883b Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 2 Mar 2026 11:06:48 +0000 Subject: [PATCH 03/23] ELI-615 | revert commit --- .../services/calculators/eligibility_calculator.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 2a071b2bc..b7314bdcc 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -124,9 +124,7 @@ def get_eligibility_status(self, include_actions: str, conditions: list[str], ca return eligibility_status.EligibilityStatus(conditions=final_result) def get_best_iteration_result(self, campaign_group: list[CampaignConfig]) -> BestIterationResult | None: - sorted_campaigns = sorted(campaign_group, key=lambda c: c.start_date, reverse=True) - - iteration_results = self.get_iteration_results(sorted_campaigns) + iteration_results = self.get_iteration_results(campaign_group) if not iteration_results: return None From 039d79c6b10a204132ab5aea63d95726dda00c25 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 2 Mar 2026 14:03:23 +0000 Subject: [PATCH 04/23] ELI-615 | wip --- .../calculators/eligibility_calculator.py | 6 +-- .../services/processors/campaign_evaluator.py | 38 +++++++++++++------ .../processors/test_campaign_evaluator.py | 16 ++++---- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index b7314bdcc..ab37a0718 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -81,13 +81,13 @@ def get_the_best_cohort_memberships( return best_status, best_cohorts - def get_eligibility_status(self, include_actions: str, conditions: list[str], category: str) -> EligibilityStatus: + def get_eligibility_status(self, include_actions: str, conditions: list[str], requested_category: str) -> EligibilityStatus: include_actions_flag = include_actions.upper() == "Y" condition_results: dict[ConditionName, IterationResult] = {} final_result = [] - requested_grouped_campaigns = self.campaign_evaluator.get_requested_grouped_campaigns( - self.campaign_configs, conditions, category + requested_grouped_campaigns = self.campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( + self.campaign_configs, conditions, requested_category ) for condition_name, campaign_group in requested_grouped_campaigns: best_iteration_result = self.get_best_iteration_result(campaign_group) diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 864d45c8c..2b4f7140a 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -15,29 +15,43 @@ class CampaignEvaluator: def get_active_campaigns(self, campaign_configs: Collection[CampaignConfig]) -> list[CampaignConfig]: return [cc for cc in campaign_configs if cc.campaign_live] - def get_requested_grouped_campaigns( - self, campaign_configs: Collection[CampaignConfig], conditions: list[str], category: str - ) -> Iterator[tuple[eligibility_status.ConditionName, list[CampaignConfig]]]: + def get_latest_campaign(self, campaign_group: list[CampaignConfig]): + if not campaign_group: + return None + + latest_date = max(c.start_date for c in campaign_group) + + latest = [c for c in campaign_group if c.start_date == latest_date] + + if len(latest) == 1: + return latest[0] + + if len(latest) > 1: + raise ValueError( + f"Multiple campaigns share the latest start_date: {latest_date}") # TODO handle it in FHIR format + + return None + + def get_campaign_with_latest_active_iteration_per_target( + self, campaign_configs: Collection[CampaignConfig], conditions: list[str], requested_category: str + ) -> Iterator[tuple[eligibility_status.ConditionName, CampaignConfig]]: mapping = { "ALL": {"V", "S"}, "VACCINATIONS": {"V"}, "SCREENING": {"S"}, } - allowed_types = mapping.get(category, set()) + allowed_types = mapping.get(requested_category, set()) filter_all_conditions = "ALL" in conditions - active_campaigns = self.get_active_campaigns(campaign_configs) + allowed_campaigns = [c for c in campaign_configs if c.type in allowed_types] + active_campaigns = self.get_active_campaigns(allowed_campaigns) for condition_name, campaign_group in groupby( sorted(active_campaigns, key=attrgetter("target")), key=attrgetter("target"), ): - campaigns = list(campaign_group) - if ( - campaigns - and campaigns[0].type in allowed_types - and (filter_all_conditions or str(condition_name) in conditions) - ): - yield condition_name, campaigns + campaigns = [c for c in allowed_campaigns if filter_all_conditions or str(condition_name) in conditions] + + yield condition_name, self.get_latest_campaign(campaigns) diff --git a/tests/unit/services/processors/test_campaign_evaluator.py b/tests/unit/services/processors/test_campaign_evaluator.py index a0b59a53a..1cdcaf737 100644 --- a/tests/unit/services/processors/test_campaign_evaluator.py +++ b/tests/unit/services/processors/test_campaign_evaluator.py @@ -33,12 +33,12 @@ def test_campaigns_grouped_by_condition_name_filters_correctly( # noqa: PLR0913 ): campaign = rule.CampaignConfigFactory.build(target=campaign_target, type=campaign_type) - result = campaign_evaluator.get_requested_grouped_campaigns([campaign], conditions_filter, category_filter) + result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], conditions_filter, category_filter) assert_that([(str(name), group[0].type) for name, group in result], is_(expected_result)) def test_campaigns_grouped_by_condition_name_with_no_campaigns(campaign_evaluator): - result = campaign_evaluator.get_requested_grouped_campaigns([], ["RSV"], "VACCINATIONS") + result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([], ["RSV"], "VACCINATIONS") assert_that(list(result), is_([])) @@ -47,7 +47,7 @@ def test_campaigns_grouped_by_condition_name_with_no_active_campaigns(campaign_e target="RSV", type="V", start_date=datetime.date(2025, 4, 20), end_date=datetime.date(2025, 4, 21) ) - result = campaign_evaluator.get_requested_grouped_campaigns([campaign], ["RSV"], "VACCINATIONS") + result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], ["RSV"], "VACCINATIONS") assert_that(list(result), is_([])) @@ -63,7 +63,7 @@ def test_campaigns_grouped_by_condition_name_with_various_categories( campaign_evaluator, category_filter, campaign_type, expected_count ): campaign = rule.CampaignConfigFactory.build(target="COVID", type=campaign_type) - result = list(campaign_evaluator.get_requested_grouped_campaigns([campaign], ["COVID"], category_filter)) + result = list(campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], ["COVID"], category_filter)) assert_that(len(result), is_(expected_count)) if expected_count > 0: assert_that(str(result[0][0]), is_("COVID")) @@ -71,7 +71,7 @@ def test_campaigns_grouped_by_condition_name_with_various_categories( def test_campaigns_grouped_by_condition_name_with_empty_conditions_filter(campaign_evaluator): campaign = rule.CampaignConfigFactory.build(target="RSV", type="V") - result = campaign_evaluator.get_requested_grouped_campaigns([campaign], [], "VACCINATIONS") + result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], [], "VACCINATIONS") assert_that(list(result), is_([])) @@ -84,7 +84,7 @@ def test_campaigns_grouped_by_condition_name_groups_multiple_campaigns_for_same_ ) all_campaigns = [campaign1, campaign2, campaign3, inactive_campaign] - result = list(campaign_evaluator.get_requested_grouped_campaigns(all_campaigns, ["COVID", "FLU"], "VACCINATIONS")) + result = list(campaign_evaluator.get_campaign_with_latest_active_iteration_per_target(all_campaigns, ["COVID", "FLU"], "VACCINATIONS")) assert_that(len(result), is_(2)) @@ -105,13 +105,13 @@ def test_campaign_grouping_is_affected_by_order_for_mixed_types(campaign_evaluat evaluator_s_first = campaign_evaluator result_s_first = list( - evaluator_s_first.get_requested_grouped_campaigns([campaign_s, campaign_v], ["RSV"], "VACCINATIONS") + evaluator_s_first.get_campaign_with_latest_active_iteration_per_target([campaign_s, campaign_v], ["RSV"], "VACCINATIONS") ) assert_that(result_s_first, is_([])) evaluator_v_first = campaign_evaluator result_v_first = list( - evaluator_v_first.get_requested_grouped_campaigns([campaign_v, campaign_s], ["RSV"], "VACCINATIONS") + evaluator_v_first.get_campaign_with_latest_active_iteration_per_target([campaign_v, campaign_s], ["RSV"], "VACCINATIONS") ) assert_that(len(result_v_first), is_(1)) assert_that(len(result_v_first[0][1]), is_(2)) From d62bca8ff3c3d2f37a83cc96971e544d5c238a7e Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Mon, 2 Mar 2026 17:05:31 +0000 Subject: [PATCH 05/23] ELI-615 | wip --- .../calculators/eligibility_calculator.py | 25 +++--------- .../services/processors/campaign_evaluator.py | 39 ++++++++++++------- .../processors/test_campaign_evaluator.py | 26 ++++++++++--- 3 files changed, 52 insertions(+), 38 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index ab37a0718..9f92bc9ad 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -81,7 +81,9 @@ def get_the_best_cohort_memberships( return best_status, best_cohorts - def get_eligibility_status(self, include_actions: str, conditions: list[str], requested_category: str) -> EligibilityStatus: + def get_eligibility_status( + self, include_actions: str, conditions: list[str], requested_category: str + ) -> EligibilityStatus: include_actions_flag = include_actions.upper() == "Y" condition_results: dict[ConditionName, IterationResult] = {} final_result = [] @@ -89,8 +91,8 @@ def get_eligibility_status(self, include_actions: str, conditions: list[str], re requested_grouped_campaigns = self.campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( self.campaign_configs, conditions, requested_category ) - for condition_name, campaign_group in requested_grouped_campaigns: - best_iteration_result = self.get_best_iteration_result(campaign_group) + for condition_name, campaign in requested_grouped_campaigns: + best_iteration_result = self.get_best_iteration_result(campaign) if best_iteration_result is None: continue @@ -123,23 +125,8 @@ def get_eligibility_status(self, include_actions: str, conditions: list[str], re # Consolidate all the results and return return eligibility_status.EligibilityStatus(conditions=final_result) - def get_best_iteration_result(self, campaign_group: list[CampaignConfig]) -> BestIterationResult | None: - iteration_results = self.get_iteration_results(campaign_group) - if not iteration_results: - return None - - (_best_iteration_name, best_iteration_result) = max( - iteration_results.items(), - key=lambda item: next(iter(item[1].cohort_results.values())).status.value - # Below handles the case where there are no cohort results - if item[1].cohort_results - else -1, - ) - - return best_iteration_result - - def get_iteration_results(self, campaign_group: list[CampaignConfig]) -> dict[IterationName, BestIterationResult]: + def get_iteration_results(self, campaign_group: CampaignConfig) -> dict[IterationName, BestIterationResult]: iteration_results: dict[IterationName, BestIterationResult] = {} for cc in campaign_group: diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 2b4f7140a..ded79dd12 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -15,22 +15,35 @@ class CampaignEvaluator: def get_active_campaigns(self, campaign_configs: Collection[CampaignConfig]) -> list[CampaignConfig]: return [cc for cc in campaign_configs if cc.campaign_live] - def get_latest_campaign(self, campaign_group: list[CampaignConfig]): - if not campaign_group: - return None + def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConfig]) -> CampaignConfig: + + """ + Returns the campaign with the latest active iteration date. - latest_date = max(c.start_date for c in campaign_group) + 1. Collect all campaigns with an active iteration. + 2. Sort by iteration date (descending). + 3. Extract the lead campaign, throwing an error if a tie for the latest date exists. + """ + + if not active_campaigns: + return None - latest = [c for c in campaign_group if c.start_date == latest_date] + valid_items = [ + (cc.current_iteration.iteration_date, cc) + for cc in active_campaigns if cc.current_iteration + ] - if len(latest) == 1: - return latest[0] + if not valid_items: + latest_date, latest_campaign = None, None + else: + max_date = max(item[0] for item in valid_items) + cc_with_max_iteration_date = [item for item in valid_items if item[0] == max_date] + if len(cc_with_max_iteration_date) > 1: + raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} campaigns found for date {max_date}") - if len(latest) > 1: - raise ValueError( - f"Multiple campaigns share the latest start_date: {latest_date}") # TODO handle it in FHIR format + latest_date, latest_campaign = cc_with_max_iteration_date[0] - return None + return latest_campaign def get_campaign_with_latest_active_iteration_per_target( self, campaign_configs: Collection[CampaignConfig], conditions: list[str], requested_category: str @@ -52,6 +65,6 @@ def get_campaign_with_latest_active_iteration_per_target( sorted(active_campaigns, key=attrgetter("target")), key=attrgetter("target"), ): - campaigns = [c for c in allowed_campaigns if filter_all_conditions or str(condition_name) in conditions] + filtered_campaigns = [c for c in allowed_campaigns if filter_all_conditions or str(condition_name) in conditions] - yield condition_name, self.get_latest_campaign(campaigns) + yield condition_name, self.get_campaign_with_latest_iteration(filtered_campaigns) diff --git a/tests/unit/services/processors/test_campaign_evaluator.py b/tests/unit/services/processors/test_campaign_evaluator.py index 1cdcaf737..5bdaa27cd 100644 --- a/tests/unit/services/processors/test_campaign_evaluator.py +++ b/tests/unit/services/processors/test_campaign_evaluator.py @@ -33,7 +33,9 @@ def test_campaigns_grouped_by_condition_name_filters_correctly( # noqa: PLR0913 ): campaign = rule.CampaignConfigFactory.build(target=campaign_target, type=campaign_type) - result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], conditions_filter, category_filter) + result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( + [campaign], conditions_filter, category_filter + ) assert_that([(str(name), group[0].type) for name, group in result], is_(expected_result)) @@ -47,7 +49,9 @@ def test_campaigns_grouped_by_condition_name_with_no_active_campaigns(campaign_e target="RSV", type="V", start_date=datetime.date(2025, 4, 20), end_date=datetime.date(2025, 4, 21) ) - result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], ["RSV"], "VACCINATIONS") + result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( + [campaign], ["RSV"], "VACCINATIONS" + ) assert_that(list(result), is_([])) @@ -63,7 +67,9 @@ def test_campaigns_grouped_by_condition_name_with_various_categories( campaign_evaluator, category_filter, campaign_type, expected_count ): campaign = rule.CampaignConfigFactory.build(target="COVID", type=campaign_type) - result = list(campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], ["COVID"], category_filter)) + result = list( + campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], ["COVID"], category_filter) + ) assert_that(len(result), is_(expected_count)) if expected_count > 0: assert_that(str(result[0][0]), is_("COVID")) @@ -84,7 +90,11 @@ def test_campaigns_grouped_by_condition_name_groups_multiple_campaigns_for_same_ ) all_campaigns = [campaign1, campaign2, campaign3, inactive_campaign] - result = list(campaign_evaluator.get_campaign_with_latest_active_iteration_per_target(all_campaigns, ["COVID", "FLU"], "VACCINATIONS")) + result = list( + campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( + all_campaigns, ["COVID", "FLU"], "VACCINATIONS" + ) + ) assert_that(len(result), is_(2)) @@ -105,13 +115,17 @@ def test_campaign_grouping_is_affected_by_order_for_mixed_types(campaign_evaluat evaluator_s_first = campaign_evaluator result_s_first = list( - evaluator_s_first.get_campaign_with_latest_active_iteration_per_target([campaign_s, campaign_v], ["RSV"], "VACCINATIONS") + evaluator_s_first.get_campaign_with_latest_active_iteration_per_target( + [campaign_s, campaign_v], ["RSV"], "VACCINATIONS" + ) ) assert_that(result_s_first, is_([])) evaluator_v_first = campaign_evaluator result_v_first = list( - evaluator_v_first.get_campaign_with_latest_active_iteration_per_target([campaign_v, campaign_s], ["RSV"], "VACCINATIONS") + evaluator_v_first.get_campaign_with_latest_active_iteration_per_target( + [campaign_v, campaign_s], ["RSV"], "VACCINATIONS" + ) ) assert_that(len(result_v_first), is_(1)) assert_that(len(result_v_first[0][1]), is_(2)) From 7a68a628c9f9a8444e83e1896066dbbfae30ff20 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 09:38:02 +0000 Subject: [PATCH 06/23] ELI-615 | wip --- .../services/processors/campaign_evaluator.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index ded79dd12..2e1e435ad 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -15,7 +15,7 @@ class CampaignEvaluator: def get_active_campaigns(self, campaign_configs: Collection[CampaignConfig]) -> list[CampaignConfig]: return [cc for cc in campaign_configs if cc.campaign_live] - def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConfig]) -> CampaignConfig: + def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConfig]) -> CampaignConfig | None: """ Returns the campaign with the latest active iteration date. @@ -34,14 +34,15 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf ] if not valid_items: - latest_date, latest_campaign = None, None + latest_campaign = None else: max_date = max(item[0] for item in valid_items) - cc_with_max_iteration_date = [item for item in valid_items if item[0] == max_date] + cc_with_max_iteration_date:list[CampaignConfig] = [item[1] for item in valid_items if item[0] == max_date] if len(cc_with_max_iteration_date) > 1: - raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} campaigns found for date {max_date}") + raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations for target {cc_with_max_iteration_date[0].iteration_date}" + f"found for date {max_date}") - latest_date, latest_campaign = cc_with_max_iteration_date[0] + latest_campaign = cc_with_max_iteration_date[0] return latest_campaign From f85348bbf317a276104eb3f0bd2df4940a77c1ec Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 26 Feb 2026 16:49:21 +0000 Subject: [PATCH 07/23] Bump werkzeug from 3.1.5 to 3.1.6 Bumps [werkzeug](https://github.com/pallets/werkzeug) from 3.1.5 to 3.1.6. - [Release notes](https://github.com/pallets/werkzeug/releases) - [Changelog](https://github.com/pallets/werkzeug/blob/main/CHANGES.rst) - [Commits](https://github.com/pallets/werkzeug/compare/3.1.5...3.1.6) --- updated-dependencies: - dependency-name: werkzeug dependency-version: 3.1.6 dependency-type: indirect ... Signed-off-by: dependabot[bot] --- poetry.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/poetry.lock b/poetry.lock index 1f2455bca..c59d3c799 100644 --- a/poetry.lock +++ b/poetry.lock @@ -3184,14 +3184,14 @@ files = [ [[package]] name = "werkzeug" -version = "3.1.5" +version = "3.1.6" description = "The comprehensive WSGI web application library." optional = false python-versions = ">=3.9" groups = ["main", "dev"] files = [ - {file = "werkzeug-3.1.5-py3-none-any.whl", hash = "sha256:5111e36e91086ece91f93268bb39b4a35c1e6f1feac762c9c822ded0a4e322dc"}, - {file = "werkzeug-3.1.5.tar.gz", hash = "sha256:6a548b0e88955dd07ccb25539d7d0cc97417ee9e179677d22c7041c8f078ce67"}, + {file = "werkzeug-3.1.6-py3-none-any.whl", hash = "sha256:7ddf3357bb9564e407607f988f683d72038551200c704012bb9a4c523d42f131"}, + {file = "werkzeug-3.1.6.tar.gz", hash = "sha256:210c6bede5a420a913956b4791a7f4d6843a43b6fcee4dfa08a65e93007d0d25"}, ] [package.dependencies] From a67ab1a1bee3a4fd8b5555ccaf644e6770d1489c Mon Sep 17 00:00:00 2001 From: oneeb-nhs <258801025+oneeb-nhs@users.noreply.github.com> Date: Mon, 2 Mar 2026 13:59:08 +0000 Subject: [PATCH 08/23] Updated not_member_of operator to NotMemberOf (#594) --- src/eligibility_signposting_api/model/campaign_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eligibility_signposting_api/model/campaign_config.py b/src/eligibility_signposting_api/model/campaign_config.py index 73199dc07..c2bdd6a8f 100644 --- a/src/eligibility_signposting_api/model/campaign_config.py +++ b/src/eligibility_signposting_api/model/campaign_config.py @@ -73,7 +73,7 @@ class RuleOperator(StrEnum): is_in = "in" not_in = "not_in" member_of = "MemberOf" - not_member_of = "NotaMemberOf" + not_member_of = "NotMemberOf" is_null = "is_null" is_not_null = "is_not_null" is_between = "between" From b8e4e34c9a4ff48aaa1ad5599cb38f0301b8c029 Mon Sep 17 00:00:00 2001 From: Robert Bailiff Date: Tue, 3 Mar 2026 10:26:20 +0000 Subject: [PATCH 09/23] Added vulture to workflows (#585) * Added vulture to workflows * Added new make commands and added to project * Added updated lockfile * Minimal config with no errors * Corrected vulture commands * Generating new lock file --- .github/actions/check-dead-code/action.yaml | 19 +++++++++++++++++++ .github/workflows/stage-1-commit.yaml | 10 ++++++++++ Makefile | 5 ++++- poetry.lock | 16 ++++++++++++++-- pyproject.toml | 7 +++++++ 5 files changed, 54 insertions(+), 3 deletions(-) create mode 100644 .github/actions/check-dead-code/action.yaml diff --git a/.github/actions/check-dead-code/action.yaml b/.github/actions/check-dead-code/action.yaml new file mode 100644 index 000000000..0e595052b --- /dev/null +++ b/.github/actions/check-dead-code/action.yaml @@ -0,0 +1,19 @@ +name: "Check Dead Code" +description: "Runs Vulture to detect unused Python code." + +runs: + using: "composite" + steps: + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.13" + + - name: Install dependencies + shell: bash + run: make dependencies install-python + + - name: Run Vulture + shell: bash + run: poetry run vulture + diff --git a/.github/workflows/stage-1-commit.yaml b/.github/workflows/stage-1-commit.yaml index 36a15737d..86c53ed5c 100644 --- a/.github/workflows/stage-1-commit.yaml +++ b/.github/workflows/stage-1-commit.yaml @@ -157,3 +157,13 @@ jobs: uses: actions/checkout@v6 - name: "Run OWASP Dependency Scan" uses: ./.github/actions/owasp-dependency-scan + check-dead-code: + name: "Check for dead code" + runs-on: ubuntu-latest + timeout-minutes: 2 + steps: + - name: "Checkout code" + uses: actions/checkout@v6 + - name: "Check for dead code" + uses: ./.github/actions/check-dead-code + diff --git a/Makefile b/Makefile index 4cd4d9fc1..342b5c14b 100644 --- a/Makefile +++ b/Makefile @@ -28,6 +28,9 @@ format: ## Format and fix code format_lint: format lint +vulture: + poetry run vulture + #Files to loop over in release _dist_include="pytest.ini poetry.lock poetry.toml pyproject.toml Makefile build/. tests" @@ -52,7 +55,7 @@ config:: # Configure development environment (main) @Configuration # TODO: Use only 'make' targets that are specific to this project, e.g. you may not need to install Node.js make _install-dependencies -precommit: test-unit build test-integration lint ## Pre-commit tasks +precommit: test-unit build test-integration lint vulture ## Pre-commit tasks python -m this # ============================================================================== diff --git a/poetry.lock b/poetry.lock index c59d3c799..bace7d18e 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 2.2.1 and should not be changed by hand. +# This file is automatically @generated by Poetry 2.1.4 and should not be changed by hand. [[package]] name = "aiohappyeyeballs" @@ -3170,6 +3170,18 @@ h2 = ["h2 (>=4,<5)"] socks = ["pysocks (>=1.5.6,!=1.5.7,<2.0)"] zstd = ["backports-zstd (>=1.0.0) ; python_version < \"3.14\""] +[[package]] +name = "vulture" +version = "2.14" +description = "Find dead code" +optional = false +python-versions = ">=3.8" +groups = ["dev"] +files = [ + {file = "vulture-2.14-py2.py3-none-any.whl", hash = "sha256:d9a90dba89607489548a49d557f8bac8112bd25d3cbc8aeef23e860811bd5ed9"}, + {file = "vulture-2.14.tar.gz", hash = "sha256:cb8277902a1138deeab796ec5bef7076a6e0248ca3607a3f3dee0b6d9e9b8415"}, +] + [[package]] name = "wcwidth" version = "0.2.13" @@ -3456,4 +3468,4 @@ propcache = ">=0.2.1" [metadata] lock-version = "2.1" python-versions = "^3.13" -content-hash = "c5064b43e402173391286c84cff772c1776fdf816a8fbd229cfdafa26da4b456" +content-hash = "4456e8d9141a4581c9fc2a1bda3c779fe194359c2d5a1588fe180563afb9b2b6" diff --git a/pyproject.toml b/pyproject.toml index 4fbaa3f8e..086a0ebce 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -63,6 +63,8 @@ python-dotenv = "^1.2.1" openapi-spec-validator = "^0.7.2" pip-licenses = "^5.5.0" cachetools = "^7.0.1" +vulture = "^2.14" + [tool.poetry-plugin-lambda-build] docker-image = "public.ecr.aws/sam/build-python3.13:1.139-x86_64" # See https://gallery.ecr.aws/search?searchTerm=%22python%22&architecture=x86-64&popularRegistries=amazon&verified=verified&operatingSystems=Linux @@ -114,3 +116,8 @@ exclude_lines = [ "if TYPE_CHECKING:", "raise NotImplementedError", ] + +[tool.vulture] +min_confidence = 80 +paths = ["src/", "tests/"] +ignore_names = ["secretsmanager_client", "consumer_*", "rule_processor_instance"] From 15895204d4915184857b09c4c61f7e58d005c08d Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 11:47:32 +0000 Subject: [PATCH 10/23] ELI-615 | modified iterations_result to iteration result --- .../calculators/eligibility_calculator.py | 47 ++++++++----------- .../services/processors/campaign_evaluator.py | 22 +++++---- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 9f92bc9ad..01a75229e 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -88,15 +88,15 @@ def get_eligibility_status( condition_results: dict[ConditionName, IterationResult] = {} final_result = [] - requested_grouped_campaigns = self.campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( + requested_cc_with_active_iteration = self.campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( self.campaign_configs, conditions, requested_category ) - for condition_name, campaign in requested_grouped_campaigns: - best_iteration_result = self.get_best_iteration_result(campaign) - - if best_iteration_result is None: + for condition_name, campaign in requested_cc_with_active_iteration: + if campaign is None: continue + best_iteration_result = self.get_iteration_result(campaign) + matched_action_detail = self.action_rule_handler.get_actions( self.person, best_iteration_result.active_iteration, @@ -126,31 +126,24 @@ def get_eligibility_status( return eligibility_status.EligibilityStatus(conditions=final_result) - def get_iteration_results(self, campaign_group: CampaignConfig) -> dict[IterationName, BestIterationResult]: - iteration_results: dict[IterationName, BestIterationResult] = {} + def get_iteration_result(self, campaign_with_active_iteration: CampaignConfig) -> BestIterationResult: - for cc in campaign_group: - try: - active_iteration = cc.current_iteration - except StopIteration: - logger.info("Skipping campaign ID %s as no active iteration was found.", cc.id) - continue - cohort_results: dict[CohortLabel, CohortGroupResult] = self.rule_processor.get_cohort_group_results( - self.person, active_iteration - ) + active_iteration = campaign_with_active_iteration.active_iteration + cohort_results: dict[CohortLabel, CohortGroupResult] = self.rule_processor.get_cohort_group_results( + self.person, active_iteration + ) - # Determine Result between cohorts - get the best - status, best_cohorts = self.get_the_best_cohort_memberships(cohort_results) - status_text = self.get_status_text(active_iteration.status_text, ConditionName(cc.target), status) + # Determine Result between cohorts - get the best + status, best_cohorts = self.get_the_best_cohort_memberships(cohort_results) + status_text = self.get_status_text(active_iteration.status_text, ConditionName(campaign_with_active_iteration.target), status) - iteration_results[active_iteration.name] = BestIterationResult( - IterationResult(status, status_text, best_cohorts, []), - active_iteration, - cc.id, - cc.version, - cohort_results, - ) - return iteration_results + return BestIterationResult( + IterationResult(status, status_text, best_cohorts, []), + active_iteration, + campaign_with_active_iteration.id, + campaign_with_active_iteration.version, + cohort_results, + ) @staticmethod def get_status_text( diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 2e1e435ad..613923481 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -1,3 +1,4 @@ +import logging from collections.abc import Collection, Iterator from itertools import groupby from operator import attrgetter @@ -7,6 +8,7 @@ from eligibility_signposting_api.model import eligibility_status from eligibility_signposting_api.model.campaign_config import CampaignConfig +logger = logging.getLogger(__name__) @service class CampaignEvaluator: @@ -25,13 +27,16 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf 3. Extract the lead campaign, throwing an error if a tie for the latest date exists. """ - if not active_campaigns: - return None + valid_items = [] - valid_items = [ - (cc.current_iteration.iteration_date, cc) - for cc in active_campaigns if cc.current_iteration - ] + for cc in active_campaigns: + if cc.current_iteration: + valid_items.append((cc.current_iteration.iteration_date, cc)) + else: + logger.info( + "Skipping campaign ID %s as no active iteration was found.", + cc.id, + ) if not valid_items: latest_campaign = None @@ -39,14 +44,15 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf max_date = max(item[0] for item in valid_items) cc_with_max_iteration_date:list[CampaignConfig] = [item[1] for item in valid_items if item[0] == max_date] if len(cc_with_max_iteration_date) > 1: - raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations for target {cc_with_max_iteration_date[0].iteration_date}" + raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations " + f"for target {cc_with_max_iteration_date[0].iteration_date}" f"found for date {max_date}") latest_campaign = cc_with_max_iteration_date[0] return latest_campaign - def get_campaign_with_latest_active_iteration_per_target( + def get_campaign_with_latest_active_iteration_per_target( self, campaign_configs: Collection[CampaignConfig], conditions: list[str], requested_category: str ) -> Iterator[tuple[eligibility_status.ConditionName, CampaignConfig]]: mapping = { From e97651d29937e24787137e20536cf1b02222cc78 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 12:19:35 +0000 Subject: [PATCH 11/23] ELI-615 | fix - naming issues | handle stop iter exception --- .../services/calculators/eligibility_calculator.py | 2 +- .../services/processors/campaign_evaluator.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 01a75229e..aea1afe84 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -128,7 +128,7 @@ def get_eligibility_status( def get_iteration_result(self, campaign_with_active_iteration: CampaignConfig) -> BestIterationResult: - active_iteration = campaign_with_active_iteration.active_iteration + active_iteration = campaign_with_active_iteration.current_iteration cohort_results: dict[CohortLabel, CohortGroupResult] = self.rule_processor.get_cohort_group_results( self.person, active_iteration ) diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 613923481..d2534a804 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -30,9 +30,9 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf valid_items = [] for cc in active_campaigns: - if cc.current_iteration: + try: valid_items.append((cc.current_iteration.iteration_date, cc)) - else: + except StopIteration: logger.info( "Skipping campaign ID %s as no active iteration was found.", cc.id, From 5aecbef366afbeed56ad1dcc733034b56ad52f71 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 14:46:02 +0000 Subject: [PATCH 12/23] ELI-615 | campaign_configs - fixture updated | test case fixed --- .../services/processors/campaign_evaluator.py | 2 +- tests/integration/conftest.py | 13 ++++++----- .../in_process/test_eligibility_endpoint.py | 22 ++++++++++++------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index d2534a804..7c5968a7d 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -45,7 +45,7 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf cc_with_max_iteration_date:list[CampaignConfig] = [item[1] for item in valid_items if item[0] == max_date] if len(cc_with_max_iteration_date) > 1: raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations " - f"for target {cc_with_max_iteration_date[0].iteration_date}" + f"for target {cc_with_max_iteration_date[0].current_iteration.iteration_date}" f"found for date {max_date}") latest_campaign = cc_with_max_iteration_date[0] diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 4293be080..be2ac01cf 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1188,12 +1188,14 @@ def campaign_configs(request, s3_client: BaseClient, rules_bucket: BucketName) - targets = [] campaign_id = [] - status = [] + iteration_status = [] + iteration_date = [] for t, _id, *rest in raw: targets.append(t) campaign_id.append(_id) - status.append(rest[0] if rest else None) + iteration_status.append(rest[0] if rest else None) + iteration_date.append(rest[1] if rest else None) for i in range(len(targets)): campaign: CampaignConfig = rule.CampaignConfigFactory.build( @@ -1221,8 +1223,9 @@ def campaign_configs(request, s3_client: BaseClient, rules_bucket: BucketName) - ], ) - if status[i] == "inactive": - campaign.iterations[0].iteration_date = datetime.datetime.now(tz=datetime.UTC) + datetime.timedelta(days=7) + # Update iteration date + if iteration_status[i]: + campaign.iterations[0].iteration_date = iteration_date[i] campaign_data = {"CampaignConfig": campaign.model_dump(by_alias=True)} key = f"{campaign.name}.json" @@ -1304,7 +1307,7 @@ def consumer_to_active_rsv_campaign_mapping( s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping_config.json") -@pytest.fixture(scope="class") +@pytest.fixture(scope="function") def consumer_to_active_campaign_having_and_rule_mapping( s3_client: BaseClient, consumer_mapping_bucket: BucketName, diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 2ff477bb8..466f0e575 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -30,6 +30,12 @@ def today(): return datetime.now(UTC).date() +def yesterday(): + return datetime.now(UTC).date()- timedelta(days=1) + +def tomorrow(): + return datetime.now(UTC).date()+ timedelta(days=1) + class TestBaseLine: def test_nhs_number_given( @@ -1196,14 +1202,14 @@ def test_valid_response_when_consumer_has_a_valid_campaign_config_mapping( # no [ ( [ - # Campaign configs in S3 - # Note: Configs are uploaded in order so the start date would be newer down the order. - ("RSV", "RSV_campaign_id_1"), - ("RSV", "RSV_campaign_id_2"), - ("RSV", "RSV_campaign_id_4"), - ("RSV", "RSV_campaign_id_3"), - ("RSV", "inactive_RSV_campaign_id_5", "inactive"), # inactive iteration - ("RSV", "RSV_campaign_id_6"), + # Creates campaign configs by [target, campaign id, iteration status, iteration date] + ("RSV", "RSV_campaign_id_1", "active", today()), + ("RSV", "RSV_campaign_id_2", "active",today()), + ("RSV", "RSV_campaign_id_3", "active", today()), + ("RSV", "RSV_campaign_id_4", "active", yesterday()), + # inactive iteration + ("RSV", "inactive_RSV_campaign_id_5", "inactive", tomorrow()), + ("RSV", "RSV_campaign_id_6", "active", today()), ], { # Consumer mappings in S3 From 29e566f07a087c306d0d5e2cce2d362855f6bfb6 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 15:11:33 +0000 Subject: [PATCH 13/23] ELI-615 | fix flaky tests do to fixture scope --- tests/integration/conftest.py | 2 +- tests/integration/in_process/test_eligibility_endpoint.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index be2ac01cf..8bdafd4e3 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -738,7 +738,7 @@ def inactive_iteration_config(s3_client: BaseClient, rules_bucket: BucketName) - s3_client.delete_object(Bucket=rules_bucket, Key=key) -@pytest.fixture(scope="class") +@pytest.fixture(scope="function") def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( target="RSV", diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 466f0e575..b5ba1868a 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1421,7 +1421,7 @@ def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campa ), ], ) - def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campaign_per_target_diff_start_date( # noqa : PLR0913 + def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaign_with_diff_iteration_date( # noqa : PLR0913 self, client: FlaskClient, persisted_person_pc_sw19: NHSNumber, From 5e2cf1c9bcb1420b04c9b5491971907819e1765b Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 15:39:14 +0000 Subject: [PATCH 14/23] ELI-615 | fix flaky tests - removed best status test --- tests/integration/conftest.py | 26 ++-- .../in_process/test_eligibility_endpoint.py | 121 ------------------ 2 files changed, 13 insertions(+), 134 deletions(-) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 8bdafd4e3..41098728a 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -586,7 +586,7 @@ def firehose_delivery_stream(firehose_client: BaseClient, audit_bucket: BucketNa return firehose_client.describe_delivery_stream(DeliveryStreamName=stream_name) -@pytest.fixture(scope="class") +@pytest.fixture def rsv_campaign_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( target="RSV", @@ -696,7 +696,7 @@ def campaign_config_with_rules_having_rule_mapper( s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") -@pytest.fixture(scope="class") +@pytest.fixture def inactive_iteration_config(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[list[CampaignConfig]]: campaigns, campaign_data_keys = [], [] @@ -738,7 +738,7 @@ def inactive_iteration_config(s3_client: BaseClient, rules_bucket: BucketName) - s3_client.delete_object(Bucket=rules_bucket, Key=key) -@pytest.fixture(scope="function") +@pytest.fixture def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( target="RSV", @@ -776,7 +776,7 @@ def campaign_config_with_and_rule(s3_client: BaseClient, rules_bucket: BucketNam s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") -@pytest.fixture(scope="class") +@pytest.fixture def campaign_config_with_tokens(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( target="RSV", @@ -843,7 +843,7 @@ def campaign_config_with_tokens(s3_client: BaseClient, rules_bucket: BucketName) s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") -@pytest.fixture(scope="class") +@pytest.fixture def campaign_config_with_invalid_tokens(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( target="RSV", @@ -1057,7 +1057,7 @@ def campaign_config_with_custom_target_attributes( s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") -@pytest.fixture(scope="class") +@pytest.fixture 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.""" campaigns, campaign_data_keys = [], [] @@ -1121,7 +1121,7 @@ def multiple_campaign_configs(s3_client: BaseClient, rules_bucket: BucketName) - s3_client.delete_object(Bucket=rules_bucket, Key=key) -@pytest.fixture(scope="class") +@pytest.fixture def campaign_config_with_virtual_cohort(s3_client: BaseClient, rules_bucket: BucketName) -> Generator[CampaignConfig]: campaign: CampaignConfig = rule.CampaignConfigFactory.build( target="COVID", @@ -1144,7 +1144,7 @@ def campaign_config_with_virtual_cohort(s3_client: BaseClient, rules_bucket: Buc s3_client.delete_object(Bucket=rules_bucket, Key=f"{campaign.name}.json") -@pytest.fixture(scope="class") +@pytest.fixture def campaign_config_with_missing_descriptions_missing_rule_text( s3_client: BaseClient, rules_bucket: BucketName ) -> Generator[CampaignConfig]: @@ -1265,7 +1265,7 @@ def create_and_put_consumer_mapping_in_s3( return consumer_mapping -@pytest.fixture(scope="class") +@pytest.fixture def consumer_to_active_campaign_having_invalid_tokens_mapping( s3_client: BaseClient, consumer_mapping_bucket: BucketName, @@ -1279,7 +1279,7 @@ def consumer_to_active_campaign_having_invalid_tokens_mapping( s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping_config.json") -@pytest.fixture(scope="class") +@pytest.fixture def consumer_to_active_campaign_having_tokens_mapping( s3_client: BaseClient, consumer_mapping_bucket: BucketName, @@ -1293,7 +1293,7 @@ def consumer_to_active_campaign_having_tokens_mapping( s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping_config.json") -@pytest.fixture(scope="class") +@pytest.fixture def consumer_to_active_rsv_campaign_mapping( s3_client: BaseClient, consumer_mapping_bucket: BucketName, @@ -1307,7 +1307,7 @@ def consumer_to_active_rsv_campaign_mapping( s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping_config.json") -@pytest.fixture(scope="function") +@pytest.fixture def consumer_to_active_campaign_having_and_rule_mapping( s3_client: BaseClient, consumer_mapping_bucket: BucketName, @@ -1456,7 +1456,7 @@ def consumer_to_campaign_having_inactive_iteration_mapping( s3_client.delete_object(Bucket=consumer_mapping_bucket, Key="consumer_mapping_config.json") -@pytest.fixture(scope="class") +@pytest.fixture def consumer_to_multiple_campaign_configs_mapping( multiple_campaign_configs: list[CampaignConfig], consumer_id: ConsumerId, diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index b5ba1868a..3c23c5a69 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1270,127 +1270,6 @@ def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_e else: assert_that(len(audit_data["response"]["condition"]), equal_to(0)) - def test_if_campaign_having_best_status_is_chosen_if_there_exists_multiple_campaign_per_target( # noqa : PLR0913 - self, - client: FlaskClient, - persisted_person_pc_sw19: NHSNumber, - s3_client: BaseClient, - consumer_mapping_bucket: BucketName, - rules_bucket: BucketName, - secretsmanager_client: BaseClient, # noqa: ARG002 - ): - # Given - consumer_id = "consumer-n3bs-jo4hn-ce4na" - headers = {"nhs-login-nhs-number": str(persisted_person_pc_sw19), UNIQUE_CONSUMER_HEADER: consumer_id} - - # Consumer Mapping Data - s3_client.put_object( - Bucket=consumer_mapping_bucket, - Key="consumer_mapping_config.json", - Body=json.dumps( - { - consumer_id: [ - {"CampaignConfigID": "RSV_campaign_id_not_actionable"}, - {"CampaignConfigID": "RSV_campaign_id_actionable"}, - ], - } - ), - ContentType="application/json", - ) - - # Campaign configs - campaign_1 = rule.CampaignConfigFactory.build( - id="RSV_campaign_id_not_actionable", - target="RSV", - type="V", - iterations=[ - rule.IterationFactory.build( - iteration_rules=[ - rule.PostcodeSuppressionRuleFactory.build(name="Exclude SW19", description=""), - ], - iteration_cohorts=[ - rule.IterationCohortFactory.build( - cohort_label="cohort1", - cohort_group="cohort_group1", - positive_description="positive_description", - ) - ], - status_text=None, - ) - ], - ) - - campaign_2 = rule.CampaignConfigFactory.build( - id="RSV_campaign_id_actionable", - target="RSV", - type="V", - iterations=[ - rule.IterationFactory.build( - iteration_rules=[ - rule.PostcodeSuppressionRuleFactory.build(name="Exclude M4", comparator=RuleComparator("M4")), - ], - iteration_cohorts=[ - rule.IterationCohortFactory.build( - cohort_label="cohort1", - cohort_group="cohort_group1", - positive_description="positive_description", - ) - ], - status_text=None, - ) - ], - ) - - for campaign in [campaign_1, campaign_2]: - s3_client.put_object( - Bucket=rules_bucket, - Key=f"{campaign.id}.json", - Body=json.dumps({"CampaignConfig": campaign.model_dump(by_alias=True)}), - ContentType="application/json", - ) - - # When - response = client.get(f"/patient-check/{persisted_person_pc_sw19}?includeActions=Y", headers=headers) - - # Then - assert_that( - response, - is_response() - .with_status_code(HTTPStatus.OK) - .and_text( - is_json_that( - has_entry( - "processedSuggestions", - equal_to( - [ - { - "condition": "RSV", - "status": "Actionable", - "eligibilityCohorts": [ - { - "cohortCode": "cohort_group1", - "cohortStatus": "Actionable", - "cohortText": "positive_description", - } - ], - "actions": [ - { - "actionCode": "action_code", - "actionType": "defaultcomms", - "description": "", - "urlLabel": "", - "urlLink": "", - } - ], - "suitabilityRules": [], - "statusText": "You should have the RSV vaccine", - } - ] - ), - ) - ) - ), - ) @pytest.mark.parametrize( ("campaign_1_start_date", "campaign_2_start_date", "postcode_for_comparator", "expected_campaign_id"), From 4679ca1c604bf6a3b3442d68ef2ca0de1f1fd590 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 15:56:56 +0000 Subject: [PATCH 15/23] ELI-615 | used raw campagin config for tests using iteration dates --- tests/integration/in_process/test_eligibility_endpoint.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 3c23c5a69..490f45950 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1334,13 +1334,14 @@ def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaig ) # Campaign configs - campaign_1 = rule.CampaignConfigFactory.build( + campaign_1 = rule.RawCampaignConfigFactory.build( id=campaign_1_start_date[0], target="RSV", start_date=campaign_1_start_date[1], type="V", iterations=[ rule.IterationFactory.build( + iteration_date=campaign_1_start_date[1], iteration_rules=[ rule.PostcodeSuppressionRuleFactory.build( name="Exclude M4", comparator=RuleComparator(postcode_for_comparator) @@ -1358,13 +1359,14 @@ def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaig ], ) - campaign_2 = rule.CampaignConfigFactory.build( + campaign_2 = rule.RawCampaignConfigFactory.build( id=campaign_2_start_date[0], target="RSV", type="V", start_date=campaign_2_start_date[1], iterations=[ rule.IterationFactory.build( + iteration_date=campaign_2_start_date[1], iteration_rules=[ rule.PostcodeSuppressionRuleFactory.build( name="Exclude M4", comparator=RuleComparator(postcode_for_comparator) From a1ec3f6616777f4e897c8f4cd4c403130dd56ac8 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 16:40:52 +0000 Subject: [PATCH 16/23] ELI-615 | fix - campaign group is used correctly --- .../services/processors/campaign_evaluator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 7c5968a7d..915b7056e 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -72,6 +72,6 @@ def get_campaign_with_latest_active_iteration_per_target( sorted(active_campaigns, key=attrgetter("target")), key=attrgetter("target"), ): - filtered_campaigns = [c for c in allowed_campaigns if filter_all_conditions or str(condition_name) in conditions] + filtered_campaigns = [c for c in campaign_group if filter_all_conditions or str(condition_name) in conditions] yield condition_name, self.get_campaign_with_latest_iteration(filtered_campaigns) From ae0d2c7651eac74df036257b4165fbcb32377637 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 16:59:43 +0000 Subject: [PATCH 17/23] ELI-615 | fix test_campaigns_grouped_by_condition_name_filters_correctly --- .../processors/test_campaign_evaluator.py | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/tests/unit/services/processors/test_campaign_evaluator.py b/tests/unit/services/processors/test_campaign_evaluator.py index 5bdaa27cd..8c56e21a1 100644 --- a/tests/unit/services/processors/test_campaign_evaluator.py +++ b/tests/unit/services/processors/test_campaign_evaluator.py @@ -16,16 +16,16 @@ def campaign_evaluator(): @pytest.mark.parametrize( ("campaign_target", "campaign_type", "conditions_filter", "category_filter", "expected_result"), [ - ("RSV", "V", ["RSV"], "VACCINATIONS", [("RSV", "V")]), - ("RSV", "V", ["COVID"], "VACCINATIONS", []), - ("RSV", "S", ["RSV"], "ALL", [("RSV", "S")]), - ("RSV", "S", ["ALL"], "ALL", [("RSV", "S")]), - ("RSV", "S", ["RSV"], "VACCINATIONS", []), - ("RSV", "V", ["RSV"], "ALL", [("RSV", "V")]), - ("FLU", "V", ["COVID", "RSV"], "ALL", []), - ("FLU", "S", ["ALL"], "ALL", [("FLU", "S")]), - ("COVID", "V", ["UNKNOWN"], "VACCINATIONS", []), - ("FLU", "V", ["COVID", "FLU"], "VACCINATIONS", [("FLU", "V")]), + ("RSV", "V", ["RSV"], "VACCINATIONS", ("RSV", "V")), + ("RSV", "V", ["COVID"], "VACCINATIONS", None), + ("RSV", "S", ["RSV"], "ALL", ("RSV", "S")), + ("RSV", "S", ["ALL"], "ALL", ("RSV", "S")), + ("RSV", "S", ["RSV"], "VACCINATIONS", None ), + ("RSV", "V", ["RSV"], "ALL", ("RSV", "V")), + ("FLU", "V", ["COVID", "RSV"], "ALL", None), + ("FLU", "S", ["ALL"], "ALL", ("FLU", "S")), + ("COVID", "V", ["UNKNOWN"], "VACCINATIONS", None), + ("FLU", "V", ["COVID", "FLU"], "VACCINATIONS", ("FLU", "V")), ], ) def test_campaigns_grouped_by_condition_name_filters_correctly( # noqa: PLR0913 @@ -36,7 +36,12 @@ def test_campaigns_grouped_by_condition_name_filters_correctly( # noqa: PLR0913 result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( [campaign], conditions_filter, category_filter ) - assert_that([(str(name), group[0].type) for name, group in result], is_(expected_result)) + + actual = next( + ((str(name), camp.type) for name, camp in result if camp is not None), + None + ) + assert actual == expected_result def test_campaigns_grouped_by_condition_name_with_no_campaigns(campaign_evaluator): From 39ad10773b5a4e422e2c58cfff5e71cba573c79c Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Tue, 3 Mar 2026 17:36:30 +0000 Subject: [PATCH 18/23] ELI-615 | fix tests --- .../processors/test_campaign_evaluator.py | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/tests/unit/services/processors/test_campaign_evaluator.py b/tests/unit/services/processors/test_campaign_evaluator.py index 8c56e21a1..066ed2bb6 100644 --- a/tests/unit/services/processors/test_campaign_evaluator.py +++ b/tests/unit/services/processors/test_campaign_evaluator.py @@ -83,12 +83,20 @@ def test_campaigns_grouped_by_condition_name_with_various_categories( def test_campaigns_grouped_by_condition_name_with_empty_conditions_filter(campaign_evaluator): campaign = rule.CampaignConfigFactory.build(target="RSV", type="V") result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], [], "VACCINATIONS") - assert_that(list(result), is_([])) + + actual = [(name, camp) for name, camp in result][0] + assert_that(actual, is_(("RSV", None))) def test_campaigns_grouped_by_condition_name_groups_multiple_campaigns_for_same_target(campaign_evaluator): - campaign1 = rule.CampaignConfigFactory.build(target="COVID", type="V", id="C1") - campaign2 = rule.CampaignConfigFactory.build(target="COVID", type="V", id="C2") + + # providing the start_date here, because CampaignConfigFactory used it for iteration_date + campaign1 = rule.CampaignConfigFactory.build(target="COVID", type="V", id="C1", start_date = datetime.datetime.now( + datetime.UTC).date() - datetime.timedelta(days=1), iterations=[ + rule.IterationFactory.build()]) + campaign2 = rule.CampaignConfigFactory.build(target="COVID", type="V", id="C2", start_date = datetime.datetime.now( + datetime.UTC).date(), iterations=[ + rule.IterationFactory.build()]) campaign3 = rule.CampaignConfigFactory.build(target="FLU", type="V", id="F1") inactive_campaign = rule.CampaignConfigFactory.build( target="COVID", type="V", id="C3", start_date=datetime.date(2025, 4, 20), end_date=datetime.date(2025, 4, 21) @@ -103,34 +111,34 @@ def test_campaigns_grouped_by_condition_name_groups_multiple_campaigns_for_same_ assert_that(len(result), is_(2)) - result_dict = {str(name): campaigns for name, campaigns in result} + result_dict = {str(name): campaign for name, campaign in result} + assert_that("COVID" in result_dict) assert_that("FLU" in result_dict) - assert_that(len(result_dict["COVID"]), is_(2)) - assert_that({c.id for c in result_dict["COVID"]}, is_({CampaignID("C1"), CampaignID("C2")})) + assert_that(result_dict["COVID"].id, is_(CampaignID("C2"))) + assert_that(result_dict["FLU"].id, is_(CampaignID("F1"))) - assert_that(len(result_dict["FLU"]), is_(1)) - assert_that(result_dict["FLU"][0].id, is_(CampaignID("F1"))) - -def test_campaign_grouping_is_affected_by_order_for_mixed_types(campaign_evaluator): +def test_campaign_grouping_is_not_affected_by_order_for_mixed_types(campaign_evaluator): campaign_v = rule.CampaignConfigFactory.build(target="RSV", type="V") campaign_s = rule.CampaignConfigFactory.build(target="RSV", type="S") - evaluator_s_first = campaign_evaluator + # Order: S then V result_s_first = list( - evaluator_s_first.get_campaign_with_latest_active_iteration_per_target( + campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( [campaign_s, campaign_v], ["RSV"], "VACCINATIONS" ) ) - assert_that(result_s_first, is_([])) + # Even if S is first, it is filtered out by 'allowed_types' + assert_that(len(result_s_first), is_(1)) + assert_that(result_s_first[0][1].type, is_("V")) - evaluator_v_first = campaign_evaluator + # Order: V then S result_v_first = list( - evaluator_v_first.get_campaign_with_latest_active_iteration_per_target( + campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( [campaign_v, campaign_s], ["RSV"], "VACCINATIONS" ) ) assert_that(len(result_v_first), is_(1)) - assert_that(len(result_v_first[0][1]), is_(2)) + assert_that(result_v_first[0][1].type, is_("V")) From da3657bee3f9a5557c08fe945836221b57940073 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 4 Mar 2026 10:18:48 +0000 Subject: [PATCH 19/23] ELI-615 | linting --- .../calculators/eligibility_calculator.py | 13 ++++---- .../services/processors/campaign_evaluator.py | 29 ++++++++++-------- .../in_process/test_eligibility_endpoint.py | 9 +++--- .../processors/test_campaign_evaluator.py | 30 +++++++++++-------- 4 files changed, 46 insertions(+), 35 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index aea1afe84..92b0ced7d 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -32,7 +32,6 @@ from eligibility_signposting_api.model.campaign_config import ( CampaignConfig, CohortLabel, - IterationName, ) from eligibility_signposting_api.model.person import Person @@ -88,8 +87,10 @@ def get_eligibility_status( condition_results: dict[ConditionName, IterationResult] = {} final_result = [] - requested_cc_with_active_iteration = self.campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( - self.campaign_configs, conditions, requested_category + requested_cc_with_active_iteration = ( + self.campaign_evaluator.get_campaign_with_latest_active_iteration_per_target( + self.campaign_configs, conditions, requested_category + ) ) for condition_name, campaign in requested_cc_with_active_iteration: if campaign is None: @@ -125,9 +126,7 @@ def get_eligibility_status( # Consolidate all the results and return return eligibility_status.EligibilityStatus(conditions=final_result) - def get_iteration_result(self, campaign_with_active_iteration: CampaignConfig) -> BestIterationResult: - active_iteration = campaign_with_active_iteration.current_iteration cohort_results: dict[CohortLabel, CohortGroupResult] = self.rule_processor.get_cohort_group_results( self.person, active_iteration @@ -135,7 +134,9 @@ def get_iteration_result(self, campaign_with_active_iteration: CampaignConfig) - # Determine Result between cohorts - get the best status, best_cohorts = self.get_the_best_cohort_memberships(cohort_results) - status_text = self.get_status_text(active_iteration.status_text, ConditionName(campaign_with_active_iteration.target), status) + status_text = self.get_status_text( + active_iteration.status_text, ConditionName(campaign_with_active_iteration.target), status + ) return BestIterationResult( IterationResult(status, status_text, best_cohorts, []), diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 915b7056e..119c601ef 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -10,6 +10,7 @@ logger = logging.getLogger(__name__) + @service class CampaignEvaluator: """Filters and groups campaign configurations.""" @@ -18,13 +19,12 @@ def get_active_campaigns(self, campaign_configs: Collection[CampaignConfig]) -> return [cc for cc in campaign_configs if cc.campaign_live] def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConfig]) -> CampaignConfig | None: - """ - Returns the campaign with the latest active iteration date. + Returns the campaign with the latest active iteration date. - 1. Collect all campaigns with an active iteration. - 2. Sort by iteration date (descending). - 3. Extract the lead campaign, throwing an error if a tie for the latest date exists. + 1. Collect all campaigns with an active iteration. + 2. Sort by iteration date (descending). + 3. Extract the lead campaign, throwing an error if a tie for the latest date exists. """ valid_items = [] @@ -42,19 +42,22 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf latest_campaign = None else: max_date = max(item[0] for item in valid_items) - cc_with_max_iteration_date:list[CampaignConfig] = [item[1] for item in valid_items if item[0] == max_date] + cc_with_max_iteration_date: list[CampaignConfig] = [item[1] for item in valid_items if item[0] == max_date] if len(cc_with_max_iteration_date) > 1: - raise ValueError(f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations " - f"for target {cc_with_max_iteration_date[0].current_iteration.iteration_date}" - f"found for date {max_date}") + err_msg = ( + f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations " + f"for target {cc_with_max_iteration_date[0].current_iteration.iteration_date}" + f"found for date {max_date}" + ) + raise ValueError(err_msg) latest_campaign = cc_with_max_iteration_date[0] return latest_campaign - def get_campaign_with_latest_active_iteration_per_target( + def get_campaign_with_latest_active_iteration_per_target( self, campaign_configs: Collection[CampaignConfig], conditions: list[str], requested_category: str - ) -> Iterator[tuple[eligibility_status.ConditionName, CampaignConfig]]: + ) -> Iterator[tuple[eligibility_status.ConditionName, CampaignConfig | None]]: mapping = { "ALL": {"V", "S"}, "VACCINATIONS": {"V"}, @@ -72,6 +75,8 @@ def get_campaign_with_latest_active_iteration_per_target( sorted(active_campaigns, key=attrgetter("target")), key=attrgetter("target"), ): - filtered_campaigns = [c for c in campaign_group if filter_all_conditions or str(condition_name) in conditions] + filtered_campaigns = [ + c for c in campaign_group if filter_all_conditions or str(condition_name) in conditions + ] yield condition_name, self.get_campaign_with_latest_iteration(filtered_campaigns) diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 490f45950..e40847bbe 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -30,11 +30,13 @@ def today(): return datetime.now(UTC).date() + def yesterday(): - return datetime.now(UTC).date()- timedelta(days=1) + return datetime.now(UTC).date() - timedelta(days=1) + def tomorrow(): - return datetime.now(UTC).date()+ timedelta(days=1) + return datetime.now(UTC).date() + timedelta(days=1) class TestBaseLine: @@ -1204,7 +1206,7 @@ def test_valid_response_when_consumer_has_a_valid_campaign_config_mapping( # no [ # Creates campaign configs by [target, campaign id, iteration status, iteration date] ("RSV", "RSV_campaign_id_1", "active", today()), - ("RSV", "RSV_campaign_id_2", "active",today()), + ("RSV", "RSV_campaign_id_2", "active", today()), ("RSV", "RSV_campaign_id_3", "active", today()), ("RSV", "RSV_campaign_id_4", "active", yesterday()), # inactive iteration @@ -1270,7 +1272,6 @@ def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_e else: assert_that(len(audit_data["response"]["condition"]), equal_to(0)) - @pytest.mark.parametrize( ("campaign_1_start_date", "campaign_2_start_date", "postcode_for_comparator", "expected_campaign_id"), [ diff --git a/tests/unit/services/processors/test_campaign_evaluator.py b/tests/unit/services/processors/test_campaign_evaluator.py index 066ed2bb6..db96d3622 100644 --- a/tests/unit/services/processors/test_campaign_evaluator.py +++ b/tests/unit/services/processors/test_campaign_evaluator.py @@ -20,7 +20,7 @@ def campaign_evaluator(): ("RSV", "V", ["COVID"], "VACCINATIONS", None), ("RSV", "S", ["RSV"], "ALL", ("RSV", "S")), ("RSV", "S", ["ALL"], "ALL", ("RSV", "S")), - ("RSV", "S", ["RSV"], "VACCINATIONS", None ), + ("RSV", "S", ["RSV"], "VACCINATIONS", None), ("RSV", "V", ["RSV"], "ALL", ("RSV", "V")), ("FLU", "V", ["COVID", "RSV"], "ALL", None), ("FLU", "S", ["ALL"], "ALL", ("FLU", "S")), @@ -37,10 +37,7 @@ def test_campaigns_grouped_by_condition_name_filters_correctly( # noqa: PLR0913 [campaign], conditions_filter, category_filter ) - actual = next( - ((str(name), camp.type) for name, camp in result if camp is not None), - None - ) + actual = next(((str(name), camp.type) for name, camp in result if camp is not None), None) assert actual == expected_result @@ -84,19 +81,26 @@ def test_campaigns_grouped_by_condition_name_with_empty_conditions_filter(campai campaign = rule.CampaignConfigFactory.build(target="RSV", type="V") result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], [], "VACCINATIONS") - actual = [(name, camp) for name, camp in result][0] + actual = next((name, camp) for name, camp in result) assert_that(actual, is_(("RSV", None))) def test_campaigns_grouped_by_condition_name_groups_multiple_campaigns_for_same_target(campaign_evaluator): - # providing the start_date here, because CampaignConfigFactory used it for iteration_date - campaign1 = rule.CampaignConfigFactory.build(target="COVID", type="V", id="C1", start_date = datetime.datetime.now( - datetime.UTC).date() - datetime.timedelta(days=1), iterations=[ - rule.IterationFactory.build()]) - campaign2 = rule.CampaignConfigFactory.build(target="COVID", type="V", id="C2", start_date = datetime.datetime.now( - datetime.UTC).date(), iterations=[ - rule.IterationFactory.build()]) + campaign1 = rule.CampaignConfigFactory.build( + target="COVID", + type="V", + id="C1", + start_date=datetime.datetime.now(datetime.UTC).date() - datetime.timedelta(days=1), + iterations=[rule.IterationFactory.build()], + ) + campaign2 = rule.CampaignConfigFactory.build( + target="COVID", + type="V", + id="C2", + start_date=datetime.datetime.now(datetime.UTC).date(), + iterations=[rule.IterationFactory.build()], + ) campaign3 = rule.CampaignConfigFactory.build(target="FLU", type="V", id="F1") inactive_campaign = rule.CampaignConfigFactory.build( target="COVID", type="V", id="C3", start_date=datetime.date(2025, 4, 20), end_date=datetime.date(2025, 4, 21) From 35d9638e91e4eb65481986ceffa77e1963f94095 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 4 Mar 2026 10:56:37 +0000 Subject: [PATCH 20/23] ELI-615 | renamed best_iteration_result to iteration_result_summary --- .../audit/audit_context.py | 14 ++++++------ .../model/eligibility_status.py | 2 +- .../calculators/eligibility_calculator.py | 22 ++++++++++--------- .../processors/action_rule_handler.py | 4 ++-- tests/unit/audit/test_audit_context.py | 14 ++++++------ 5 files changed, 29 insertions(+), 27 deletions(-) diff --git a/src/eligibility_signposting_api/audit/audit_context.py b/src/eligibility_signposting_api/audit/audit_context.py index 53358b4d6..66325669c 100644 --- a/src/eligibility_signposting_api/audit/audit_context.py +++ b/src/eligibility_signposting_api/audit/audit_context.py @@ -19,10 +19,10 @@ ) from eligibility_signposting_api.audit.audit_service import AuditService from eligibility_signposting_api.model.eligibility_status import ( - BestIterationResult, CohortGroupResult, ConditionName, IterationResult, + IterationResultSummary, MatchedActionDetail, Reason, RuleType, @@ -63,13 +63,13 @@ def add_request_details(request: Request) -> None: @staticmethod def append_audit_condition( condition_name: ConditionName, - best_iteration_result: BestIterationResult, + iteration_result_summary: IterationResultSummary, action_detail: MatchedActionDetail, ) -> None: audit_eligibility_cohorts, audit_eligibility_cohort_groups, audit_actions = [], [], [] - best_active_iteration = best_iteration_result.active_iteration - best_candidate = best_iteration_result.iteration_result - best_cohort_results = best_iteration_result.cohort_results + best_active_iteration = iteration_result_summary.active_iteration + best_candidate = iteration_result_summary.iteration_result + best_cohort_results = iteration_result_summary.cohort_results filter_audit_rules, suitability_audit_rules = [], [] if best_cohort_results: @@ -94,8 +94,8 @@ def append_audit_condition( audit_actions = AuditContext.create_audit_actions(action_detail.actions) audit_condition = AuditCondition( - campaign_id=best_iteration_result.campaign_id, - campaign_version=best_iteration_result.campaign_version, + campaign_id=iteration_result_summary.campaign_id, + campaign_version=iteration_result_summary.campaign_version, iteration_id=best_active_iteration.id if best_active_iteration else None, iteration_version=best_active_iteration.version if best_active_iteration else None, condition_name=condition_name, diff --git a/src/eligibility_signposting_api/model/eligibility_status.py b/src/eligibility_signposting_api/model/eligibility_status.py index aa19cd678..89b73828b 100644 --- a/src/eligibility_signposting_api/model/eligibility_status.py +++ b/src/eligibility_signposting_api/model/eligibility_status.py @@ -138,7 +138,7 @@ class IterationResult: @dataclass -class BestIterationResult: +class IterationResultSummary: iteration_result: IterationResult active_iteration: Iteration | None = None campaign_id: CampaignID | None = None diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 92b0ced7d..470c8a860 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -11,12 +11,12 @@ from eligibility_signposting_api.audit.audit_context import AuditContext from eligibility_signposting_api.model import campaign_config, eligibility_status from eligibility_signposting_api.model.eligibility_status import ( - BestIterationResult, CohortGroupResult, Condition, ConditionName, EligibilityStatus, IterationResult, + IterationResultSummary, Reason, Status, StatusText, @@ -94,21 +94,21 @@ def get_eligibility_status( ) for condition_name, campaign in requested_cc_with_active_iteration: if campaign is None: - continue + continue # skipping as no active iteration was found. - best_iteration_result = self.get_iteration_result(campaign) + iteration_result_summary = self.evaluate_iteration_result_summary(campaign) matched_action_detail = self.action_rule_handler.get_actions( self.person, - best_iteration_result.active_iteration, - best_iteration_result.iteration_result, + iteration_result_summary.active_iteration, + iteration_result_summary.iteration_result, include_actions_flag=include_actions_flag, ) - best_iteration_result = TokenProcessor.find_and_replace_tokens(self.person, best_iteration_result) + iteration_result_summary = TokenProcessor.find_and_replace_tokens(self.person, iteration_result_summary) matched_action_detail = TokenProcessor.find_and_replace_tokens(self.person, matched_action_detail) - condition_results[condition_name] = best_iteration_result.iteration_result + condition_results[condition_name] = iteration_result_summary.iteration_result condition_results[condition_name].actions = matched_action_detail.actions condition: Condition = self.build_condition( @@ -119,14 +119,16 @@ def get_eligibility_status( AuditContext.append_audit_condition( condition_name, - best_iteration_result, + iteration_result_summary, matched_action_detail, ) # Consolidate all the results and return return eligibility_status.EligibilityStatus(conditions=final_result) - def get_iteration_result(self, campaign_with_active_iteration: CampaignConfig) -> BestIterationResult: + def evaluate_iteration_result_summary( + self, campaign_with_active_iteration: CampaignConfig + ) -> IterationResultSummary: active_iteration = campaign_with_active_iteration.current_iteration cohort_results: dict[CohortLabel, CohortGroupResult] = self.rule_processor.get_cohort_group_results( self.person, active_iteration @@ -138,7 +140,7 @@ def get_iteration_result(self, campaign_with_active_iteration: CampaignConfig) - active_iteration.status_text, ConditionName(campaign_with_active_iteration.target), status ) - return BestIterationResult( + return IterationResultSummary( IterationResult(status, status_text, best_cohorts, []), active_iteration, campaign_with_active_iteration.id, diff --git a/src/eligibility_signposting_api/services/processors/action_rule_handler.py b/src/eligibility_signposting_api/services/processors/action_rule_handler.py index b30d4e0cc..44fd4f9ae 100644 --- a/src/eligibility_signposting_api/services/processors/action_rule_handler.py +++ b/src/eligibility_signposting_api/services/processors/action_rule_handler.py @@ -27,14 +27,14 @@ def get_actions( self, person: Person, active_iteration: Iteration | None, - best_iteration_result: IterationResult, + iteration_result: IterationResult, *, include_actions_flag: bool, ) -> MatchedActionDetail: action_detail = MatchedActionDetail() if active_iteration is not None and include_actions_flag: - rule_type = best_iteration_result.status.get_action_rule_type() + rule_type = iteration_result.status.get_action_rule_type() action_detail = self._handle(person, active_iteration, rule_type) return action_detail diff --git a/tests/unit/audit/test_audit_context.py b/tests/unit/audit/test_audit_context.py index 4e20f9bdc..637494228 100644 --- a/tests/unit/audit/test_audit_context.py +++ b/tests/unit/audit/test_audit_context.py @@ -15,11 +15,11 @@ ActionCode, ActionDescription, ActionType, - BestIterationResult, CohortGroupResult, ConditionName, InternalActionCode, IterationResult, + IterationResultSummary, MatchedActionDetail, Reason, RuleCode, @@ -147,7 +147,7 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g_for_actionable_ campaign_config.RuleName("RedirectRuleName1"), campaign_config.RulePriority(1), suggested_actions ) - best_iteration_results = BestIterationResult( + iteration_result_summary = IterationResultSummary( iteration_result, iteration, campaign_details[0], @@ -158,7 +158,7 @@ def test_append_audit_condition_adds_condition_to_audit_log_on_g_for_actionable_ with app.app_context(): g.audit_log = AuditEvent() - AuditContext.append_audit_condition(condition_name, best_iteration_results, matched_action_detail) + AuditContext.append_audit_condition(condition_name, iteration_result_summary, matched_action_detail) expected_audit_action = [ AuditAction( @@ -227,7 +227,7 @@ def test_should_append_audit_suppression_rules_for_actionable_status(app): ) campaign_details = (CampaignID("CampaignID1"), CampaignVersion(123)) - best_iteration_results = BestIterationResult( + iteration_result_summary = IterationResultSummary( iteration_result, iteration, campaign_details[0], @@ -238,7 +238,7 @@ def test_should_append_audit_suppression_rules_for_actionable_status(app): with app.app_context(): g.audit_log = AuditEvent() - AuditContext.append_audit_condition(condition_name, best_iteration_results, MatchedActionDetail()) + AuditContext.append_audit_condition(condition_name, iteration_result_summary, MatchedActionDetail()) assert g.audit_log.response.condition, condition_name cond = g.audit_log.response.condition[0] @@ -288,7 +288,7 @@ def test_should_append_audit_filter_rules_for_not_actionable_status(app): ) campaign_details = (CampaignID("CampaignID1"), CampaignVersion(123)) - best_iteration_results = BestIterationResult( + iteration_result_summary = IterationResultSummary( iteration_result, iteration, campaign_details[0], @@ -299,7 +299,7 @@ def test_should_append_audit_filter_rules_for_not_actionable_status(app): with app.app_context(): g.audit_log = AuditEvent() - AuditContext.append_audit_condition(condition_name, best_iteration_results, MatchedActionDetail()) + AuditContext.append_audit_condition(condition_name, iteration_result_summary, MatchedActionDetail()) assert g.audit_log.response.condition, condition_name cond = g.audit_log.response.condition[0] From 5d6d0927affaa344d13f4762e95c6b7139bd4541 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 4 Mar 2026 11:42:12 +0000 Subject: [PATCH 21/23] ELI-615 | add more test cases - it tests --- .../in_process/test_eligibility_endpoint.py | 46 +++++++++++++++++-- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index e40847bbe..600d2ba7b 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -17,7 +17,13 @@ has_key, ) -from eligibility_signposting_api.model.campaign_config import CampaignConfig, RuleComparator +from eligibility_signposting_api.model.campaign_config import ( + CampaignConfig, + RuleAttributeLevel, + RuleComparator, + RuleOperator, + RuleType, +) from eligibility_signposting_api.model.consumer_mapping import ConsumerId, ConsumerMapping from eligibility_signposting_api.model.eligibility_status import ( NHSNumber, @@ -1273,35 +1279,59 @@ def test_if_correct_campaign_is_chosen_for_the_consumer_when_multiple_campaign_e assert_that(len(audit_data["response"]["condition"]), equal_to(0)) @pytest.mark.parametrize( - ("campaign_1_start_date", "campaign_2_start_date", "postcode_for_comparator", "expected_campaign_id"), + ( + "campaign_1_start_date", + "campaign_2_start_date", + "postcode_for_comparator", + "cohort_for_comparator", + "expected_campaign_id", + ), [ ( ("RSV_campaign_id_1", today()), ("RSV_campaign_id_2", today() - timedelta(days=1)), - "SW19", # postcode for resulting in not-actionable + "SW19", # postcode for resulting in not-actionable (used by the suppression rule) + "cohort2", "RSV_campaign_id_1", ), ( ("RSV_campaign_id_1", today() - timedelta(days=1)), ("RSV_campaign_id_2", today()), "SW19", # postcode for resulting in not-actionable + "cohort2", "RSV_campaign_id_2", ), ( ("RSV_campaign_id_1", today()), ("RSV_campaign_id_2", today() - timedelta(days=1)), "M4", # postcode for resulting in actionable + "cohort2", "RSV_campaign_id_1", ), ( ("RSV_campaign_id_1", today() - timedelta(days=1)), ("RSV_campaign_id_2", today()), "M4", # postcode for resulting in actionable + "cohort2", + "RSV_campaign_id_2", + ), + ( + ("RSV_campaign_id_1", today()), + ("RSV_campaign_id_2", today() - timedelta(days=1)), + "M4", # cohort for resulting in not-eligible + "cohort1", + "RSV_campaign_id_1", + ), + ( + ("RSV_campaign_id_1", today() - timedelta(days=1)), + ("RSV_campaign_id_2", today()), + "M4", + "cohort1", # cohort for resulting in not-eligible (used by the filter rule) "RSV_campaign_id_2", ), ], ) - def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaign_with_diff_iteration_date( # noqa : PLR0913 + def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaign_with_diff_iteration_date( # noqa: PLR0913 self, client: FlaskClient, persisted_person_pc_sw19: NHSNumber, @@ -1313,6 +1343,7 @@ def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaig campaign_1_start_date: tuple[str, date], campaign_2_start_date: tuple[str, date], postcode_for_comparator: str, + cohort_for_comparator: str, expected_campaign_id: NHSNumber, ): # Given @@ -1344,6 +1375,13 @@ def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaig rule.IterationFactory.build( iteration_date=campaign_1_start_date[1], iteration_rules=[ + rule.IterationRuleFactory.build( + type=RuleType.filter, + name="Exclude if cohort matches", + attribute_level=RuleAttributeLevel.COHORT, + comparator=RuleComparator(cohort_for_comparator), + operator=RuleOperator.member_of, + ), rule.PostcodeSuppressionRuleFactory.build( name="Exclude M4", comparator=RuleComparator(postcode_for_comparator) ), From 7def478d27b10280de013ad614aa0eb8412a59a1 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 4 Mar 2026 11:57:39 +0000 Subject: [PATCH 22/23] ELI-615 | test commit - try git leaks ignore --- .gitleaksignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitleaksignore b/.gitleaksignore index cceb449a3..ff9cec0ef 100644 --- a/.gitleaksignore +++ b/.gitleaksignore @@ -1,3 +1,5 @@ # SEE: https://github.com/gitleaks/gitleaks/blob/master/README.md#gitleaksignore cd9c0efec38c5d63053dd865e5d4e207c0760d91:docs/guides/Perform_static_analysis.md:generic-api-key:37 + +bf0c77098978c450d8570b38fb480fbb8d6a0628:.github/instructions/*.instructions.md:stripe-access-token:140 From 8f3b03e0f351979eb2d5f0776063a2963b869368 Mon Sep 17 00:00:00 2001 From: karthikeyannhs <174426205+Karthikeyannhs@users.noreply.github.com> Date: Wed, 4 Mar 2026 15:44:32 +0000 Subject: [PATCH 23/23] ELI-615 | incorporated review comments --- .../calculators/eligibility_calculator.py | 3 - .../services/processors/campaign_evaluator.py | 13 ++- .../in_process/test_eligibility_endpoint.py | 97 +++++++++++++++++++ .../processors/test_campaign_evaluator.py | 7 +- 4 files changed, 109 insertions(+), 11 deletions(-) diff --git a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py index 470c8a860..9f83bd916 100644 --- a/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py +++ b/src/eligibility_signposting_api/services/calculators/eligibility_calculator.py @@ -93,9 +93,6 @@ def get_eligibility_status( ) ) for condition_name, campaign in requested_cc_with_active_iteration: - if campaign is None: - continue # skipping as no active iteration was found. - iteration_result_summary = self.evaluate_iteration_result_summary(campaign) matched_action_detail = self.action_rule_handler.get_actions( diff --git a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py index 119c601ef..9d1a9aad1 100644 --- a/src/eligibility_signposting_api/services/processors/campaign_evaluator.py +++ b/src/eligibility_signposting_api/services/processors/campaign_evaluator.py @@ -45,9 +45,10 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf cc_with_max_iteration_date: list[CampaignConfig] = [item[1] for item in valid_items if item[0] == max_date] if len(cc_with_max_iteration_date) > 1: err_msg = ( - f"Ambiguous result: {len(cc_with_max_iteration_date)} iterations " - f"for target {cc_with_max_iteration_date[0].current_iteration.iteration_date}" - f"found for date {max_date}" + f"Ambiguous result: '{len(cc_with_max_iteration_date)}' active iterations " + f"for target {cc_with_max_iteration_date[0].target} " + f"found for date '{max_date}' " + f"across campaign(s) {[cc.id for cc in cc_with_max_iteration_date]}" ) raise ValueError(err_msg) @@ -57,7 +58,7 @@ def get_campaign_with_latest_iteration(self, active_campaigns: list[CampaignConf def get_campaign_with_latest_active_iteration_per_target( self, campaign_configs: Collection[CampaignConfig], conditions: list[str], requested_category: str - ) -> Iterator[tuple[eligibility_status.ConditionName, CampaignConfig | None]]: + ) -> Iterator[tuple[eligibility_status.ConditionName, CampaignConfig]]: mapping = { "ALL": {"V", "S"}, "VACCINATIONS": {"V"}, @@ -79,4 +80,6 @@ def get_campaign_with_latest_active_iteration_per_target( c for c in campaign_group if filter_all_conditions or str(condition_name) in conditions ] - yield condition_name, self.get_campaign_with_latest_iteration(filtered_campaigns) + campaign = self.get_campaign_with_latest_iteration(filtered_campaigns) + if campaign is not None: + yield (condition_name, campaign) diff --git a/tests/integration/in_process/test_eligibility_endpoint.py b/tests/integration/in_process/test_eligibility_endpoint.py index 600d2ba7b..bc6fe2693 100644 --- a/tests/integration/in_process/test_eligibility_endpoint.py +++ b/tests/integration/in_process/test_eligibility_endpoint.py @@ -1445,3 +1445,100 @@ def test_if_cc_with_latest_active_iteration_is_chosen_if_exists_multiple_campaig assert_that(audit_data["response"]["condition"][0].get("campaignId"), equal_to(expected_campaign_id)) else: assert_that(len(audit_data["response"]["condition"]), equal_to(0)) + + def test_if_multiple_active_iterations_with_same_iteration_datetime_for_the_same_target_throws_internal_error( # noqa: PLR0913 + self, + client: FlaskClient, + persisted_person_pc_sw19: NHSNumber, + s3_client: BaseClient, + consumer_mapping_bucket: BucketName, + rules_bucket: BucketName, + secretsmanager_client: BaseClient, # noqa: ARG002 + caplog, + ): + # Given + consumer_id = "consumer-n3bs-jo4hn-ce4na" + headers = {"nhs-login-nhs-number": str(persisted_person_pc_sw19), UNIQUE_CONSUMER_HEADER: consumer_id} + + # Consumer Mapping Data + s3_client.put_object( + Bucket=consumer_mapping_bucket, + Key="consumer_mapping_config.json", + Body=json.dumps( + { + consumer_id: [ + {"CampaignConfigID": "RSV_campaign_id_1"}, + {"CampaignConfigID": "RSV_campaign_id_2"}, + ], + } + ), + ContentType="application/json", + ) + previous_day = yesterday() + # Campaign configs + campaign_1 = rule.RawCampaignConfigFactory.build( + id="RSV_campaign_id_1", + target="RSV", + start_date=previous_day, + type="V", + iterations=[rule.IterationFactory.build(iteration_date=previous_day)], + ) + + campaign_2 = rule.RawCampaignConfigFactory.build( + id="RSV_campaign_id_2", + target="RSV", + start_date=previous_day, + type="V", + iterations=[rule.IterationFactory.build(iteration_date=previous_day)], + ) + + for campaign in [campaign_1, campaign_2]: + s3_client.put_object( + Bucket=rules_bucket, + Key=f"{campaign.id}.json", + Body=json.dumps({"CampaignConfig": campaign.model_dump(by_alias=True)}), + ContentType="application/json", + ) + + # When + response = client.get(f"/patient-check/{persisted_person_pc_sw19}", headers=headers) + + assert_that( + response, + is_response() + .with_status_code(HTTPStatus.INTERNAL_SERVER_ERROR) + .with_headers(has_entries({"Content-Type": "application/fhir+json"})) + .and_text( + is_json_that( + has_entries( + resourceType="OperationOutcome", + issue=contains_exactly( + has_entries( + severity="error", + code="processing", + diagnostics="An unexpected error occurred.", + details={ + "coding": [ + { + "system": "https://fhir.nhs.uk/STU3/ValueSet/Spine-ErrorOrWarningCode-1", + "code": "INTERNAL_SERVER_ERROR", + "display": "An unexpected internal server error occurred.", + } + ] + }, + ) + ), + ) + ) + ), + ) + + err_msg = ( + "Ambiguous result: '2' active iterations " + "for target RSV " + f"found for date '{previous_day}' " + "across campaign(s) ['RSV_campaign_id_1', 'RSV_campaign_id_2']" + ) + assert any(err_msg in message for message in caplog.messages), ( + f"Expected log message not found. Logged messages: {caplog.messages}" + ) diff --git a/tests/unit/services/processors/test_campaign_evaluator.py b/tests/unit/services/processors/test_campaign_evaluator.py index db96d3622..4a0e1330f 100644 --- a/tests/unit/services/processors/test_campaign_evaluator.py +++ b/tests/unit/services/processors/test_campaign_evaluator.py @@ -79,10 +79,11 @@ def test_campaigns_grouped_by_condition_name_with_various_categories( def test_campaigns_grouped_by_condition_name_with_empty_conditions_filter(campaign_evaluator): campaign = rule.CampaignConfigFactory.build(target="RSV", type="V") - result = campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], [], "VACCINATIONS") + result = list( + campaign_evaluator.get_campaign_with_latest_active_iteration_per_target([campaign], [], "VACCINATIONS") + ) - actual = next((name, camp) for name, camp in result) - assert_that(actual, is_(("RSV", None))) + assert_that(result, is_([])) def test_campaigns_grouped_by_condition_name_groups_multiple_campaigns_for_same_target(campaign_evaluator):