From c318382c2e47d81f3f390be59ce23af362966e08 Mon Sep 17 00:00:00 2001 From: Ben Corlett Date: Thu, 26 Feb 2026 11:37:32 +0000 Subject: [PATCH 1/5] Change service and org name to be unique per environment --- app/functional_tests_fixtures/__init__.py | 7 +++++-- .../test_functional_test_fixtures.py | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index 6ff0f330ae..fcc58bbf07 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -153,8 +153,11 @@ def _create_db_objects( ) -> dict[str, str]: current_app.logger.info("Creating functional test fixtures for %s:", environment) + org_name_with_environment = f"{org_name} ({environment})" + service_name_with_environment = f"Functional Tests ({environment})" + current_app.logger.info("--> Ensure organisation exists") - org = _create_organiation(email_domain, org_name) + org = _create_organiation(email_domain, org_name_with_environment) current_app.logger.info("--> Ensure users exists") func_test_user = _create_user( @@ -178,7 +181,7 @@ def _create_db_objects( ) current_app.logger.info("--> Ensure service exists") - service = _create_service(org.id, service_admin_user) + service = _create_service(org.id, service_admin_user, service_name_with_environment) current_app.logger.info("--> Ensure users are added to service") dao_add_user_to_service(service, service_admin_user) diff --git a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py index 3385a96e92..1673f21dac 100644 --- a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py +++ b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py @@ -73,7 +73,7 @@ def test_create_db_objects_sets_db_up(notify_api, notify_service): assert "FUNCTIONAL_TESTS_SERVICE_EMAIL_PASSWORD" in variables[0] assert variables[0]["FUNCTIONAL_TESTS_SERVICE_NUMBER"] == "07700900501" assert "FUNCTIONAL_TESTS_SERVICE_ID" in variables[0] - assert variables[0]["FUNCTIONAL_TESTS_SERVICE_NAME"] == "Functional Tests" + assert variables[0]["FUNCTIONAL_TESTS_SERVICE_NAME"] == "Functional Tests (dev-env)" assert "FUNCTIONAL_TESTS_ORGANISATION_ID" in variables[0] assert variables[0]["FUNCTIONAL_TESTS_SERVICE_API_KEY"].startswith("functional_tests_service_live_key-") assert variables[0]["FUNCTIONAL_TESTS_SERVICE_API_TEST_KEY"].startswith("functional_tests_service_test_key-") From 86645b731e915007b0f66e601881c2c218281114 Mon Sep 17 00:00:00 2001 From: Ben Corlett Date: Thu, 26 Feb 2026 13:39:03 +0000 Subject: [PATCH 2/5] Keep the original org --- app/commands.py | 6 +++++- app/functional_tests_fixtures/__init__.py | 18 ++++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/commands.py b/app/commands.py index d8736b5496..bd1075354d 100644 --- a/app/commands.py +++ b/app/commands.py @@ -864,7 +864,11 @@ def functional_test_fixtures(): """ if current_app.config["REGISTER_FUNCTIONAL_TESTING_BLUEPRINT"]: - apply_fixtures() + try: + apply_fixtures() + except Exception: + current_app.logger.exception("Functional test fixtures failed") + raise else: print("Functional test fixtures are disabled. Set REGISTER_FUNCTIONAL_TESTING_BLUEPRINT to True in config.") raise SystemExit(1) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index fcc58bbf07..7d6fb00b7a 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -153,11 +153,10 @@ def _create_db_objects( ) -> dict[str, str]: current_app.logger.info("Creating functional test fixtures for %s:", environment) - org_name_with_environment = f"{org_name} ({environment})" service_name_with_environment = f"Functional Tests ({environment})" current_app.logger.info("--> Ensure organisation exists") - org = _create_organiation(email_domain, org_name_with_environment) + org = _create_organiation(email_domain, org_name) current_app.logger.info("--> Ensure users exists") func_test_user = _create_user( @@ -395,10 +394,21 @@ def _create_organiation(email_domain, org_name): if org is None: org = Organisation(name=org_name, active=True, crown=False, organisation_type="central") - dao_create_organisation(org) - dao_update_organisation(org.id, domains=[email_domain], can_approve_own_go_live_requests=True) + dao_update_organisation( + org.id, + name=org_name, + active=True, + crown=False, + organisation_type="central", + notes=None, + request_to_go_live_notes=None, + agreement_signed_on_behalf_of_name=None, + agreement_signed_on_behalf_of_email_address=None, + domains=[email_domain], + can_approve_own_go_live_requests=True, + ) return org From 01bf75f46ffd7f95386ba7002f540d45fc162cad Mon Sep 17 00:00:00 2001 From: Ben Corlett Date: Fri, 27 Feb 2026 07:47:49 +0000 Subject: [PATCH 3/5] Make functional fixtures reclaim inbound number idempotently --- app/functional_tests_fixtures/__init__.py | 33 +++++++++++ .../test_functional_test_fixtures.py | 57 ++++++++++++++++++- 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index 7d6fb00b7a..d96f64bfa0 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -6,6 +6,7 @@ from flask import current_app from sqlalchemy.exc import NoResultFound +from app import db from app.constants import ( EDIT_FOLDER_PERMISSIONS, EMAIL_AUTH, @@ -472,7 +473,37 @@ def _create_api_key(name, service_id, user_id, key_type="normal"): def _create_inbound_numbers(service_id, user_id, number="07700900500", provider="mmg"): inbound_number = dao_get_inbound_number_for_service(service_id=service_id) + if inbound_number is not None and inbound_number.number == number: + return inbound_number.id + + inbound_number_by_number = InboundNumber.query.filter_by(number=number).one_or_none() + if inbound_number_by_number is not None: + if inbound_number is not None and inbound_number.id != inbound_number_by_number.id: + inbound_number.service_id = None + db.session.add(inbound_number) + + previous_service_id = inbound_number_by_number.service_id + inbound_number_by_number.service_id = service_id + inbound_number_by_number.active = True + db.session.add(inbound_number_by_number) + db.session.commit() + + if previous_service_id and previous_service_id != service_id: + current_app.logger.info( + "Reassigned inbound number %s from service %s to fixture service %s", + number, + previous_service_id, + service_id, + ) + + return inbound_number_by_number.id + if inbound_number is not None: + inbound_number.number = number + inbound_number.provider = provider + inbound_number.active = True + db.session.add(inbound_number) + db.session.commit() return inbound_number.id inbound_number = InboundNumber() @@ -642,6 +673,8 @@ def _create_service_sms_senders(service_id, sms_sender, is_default, inbound_numb for service_sms_sender in service_sms_senders: if service_sms_sender.sms_sender == sms_sender: return service_sms_sender + if inbound_number_id and service_sms_sender.inbound_number_id == inbound_number_id: + return service_sms_sender return dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_number_id) diff --git a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py index 1673f21dac..3734c0aed1 100644 --- a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py +++ b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py @@ -7,8 +7,10 @@ from moto import mock_aws from app import db -from app.functional_tests_fixtures import _create_db_objects, _create_user, apply_fixtures +from app.functional_tests_fixtures import _create_db_objects, _create_service_sms_senders, _create_user, apply_fixtures +from app.models import InboundNumber from tests.conftest import set_config_values +from tests.app.db import create_inbound_number, create_service, create_service_sms_sender def test_create_db_objects_sets_db_up(notify_api, notify_service): @@ -112,6 +114,59 @@ def test_create_user_revalidates_email(): assert (datetime.utcnow() - test_user.email_access_validated_at).total_seconds() < 60 +def test_create_db_objects_reassigns_existing_inbound_number_from_another_service(notify_api, notify_service): + existing_service = create_service(service_name="Existing inbound owner") + existing_inbound = create_inbound_number(number="07700900500", service_id=existing_service.id) + + with set_config_values( + notify_api, + { + "MMG_INBOUND_SMS_USERNAME": ["test_mmg_username"], + "MMG_INBOUND_SMS_AUTH": ["test_mmg_password"], + "INTERNAL_CLIENT_API_KEYS": {"notify-functional-tests": ["functional-tests-secret-key"]}, + "ADMIN_BASE_URL": "http://localhost:6012", + "API_HOST_NAME": "http://localhost:6011", + }, + ): + variables = _create_db_objects( + "fake password", + "test_request_bin_token", + "dev-env", + "notify-tests-preview", + "digital.cabinet-office.gov.uk", + "govuk_notify", + "functional_tests_service_live_key", + "functional_tests_service_test_key", + str(notify_service.id), + "Functional Tests Org", + "07700900500", + ) + + reassigned_inbound = InboundNumber.query.filter_by(number="07700900500").one() + + assert str(reassigned_inbound.id) == str(existing_inbound.id) + assert str(reassigned_inbound.service_id) == str(variables["FUNCTIONAL_TESTS_SERVICE_ID"]) + + +def test_create_service_sms_senders_reuses_existing_sender_with_same_inbound_number_id(notify_service): + inbound = create_inbound_number(number="07700900888", service_id=notify_service.id) + existing_sender = create_service_sms_sender( + service=notify_service, + sms_sender="existing", + is_default=True, + inbound_number_id=inbound.id, + ) + + sender = _create_service_sms_senders( + notify_service.id, + "07700900888", + True, + inbound.id, + ) + + assert sender.id == existing_sender.id + + @mock_aws def test_function_test_fixtures_saves_to_disk_and_ssm(notify_api, os_environ, mocker): mocker.patch("app.functional_tests_fixtures._create_db_objects", return_value={"FOO": "BAR", "BAZ": "WAZ"}) From b412c9503035ebd4f8951bb37c90e0576bf33e42 Mon Sep 17 00:00:00 2001 From: Ben Corlett Date: Fri, 27 Feb 2026 08:06:35 +0000 Subject: [PATCH 4/5] Make the functional test fixtures reclaim the sms sender number --- app/functional_tests_fixtures/__init__.py | 28 +++++++++++++++++++ .../test_functional_test_fixtures.py | 24 +++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index d96f64bfa0..f75500d400 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -71,6 +71,7 @@ Service, ServiceCallbackApi, ServiceEmailReplyTo, + ServiceSmsSender, User, ) from app.schemas import api_key_schema, template_schema @@ -676,6 +677,33 @@ def _create_service_sms_senders(service_id, sms_sender, is_default, inbound_numb if inbound_number_id and service_sms_sender.inbound_number_id == inbound_number_id: return service_sms_sender + if inbound_number_id: + existing_inbound_sender = ServiceSmsSender.query.filter_by(inbound_number_id=inbound_number_id).one_or_none() + if existing_inbound_sender is not None: + if is_default: + for service_sms_sender in service_sms_senders: + if service_sms_sender.id != existing_inbound_sender.id and service_sms_sender.is_default: + service_sms_sender.is_default = False + db.session.add(service_sms_sender) + + previous_service_id = existing_inbound_sender.service_id + existing_inbound_sender.service_id = service_id + existing_inbound_sender.sms_sender = sms_sender + existing_inbound_sender.is_default = is_default + existing_inbound_sender.archived = False + db.session.add(existing_inbound_sender) + db.session.commit() + + if previous_service_id != service_id: + current_app.logger.info( + "Reassigned inbound sms sender for number %s from service %s to fixture service %s", + sms_sender, + previous_service_id, + service_id, + ) + + return existing_inbound_sender + return dao_add_sms_sender_for_service(service_id, sms_sender, is_default, inbound_number_id) diff --git a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py index 3734c0aed1..c2da2f39dd 100644 --- a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py +++ b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py @@ -9,8 +9,8 @@ from app import db from app.functional_tests_fixtures import _create_db_objects, _create_service_sms_senders, _create_user, apply_fixtures from app.models import InboundNumber -from tests.conftest import set_config_values from tests.app.db import create_inbound_number, create_service, create_service_sms_sender +from tests.conftest import set_config_values def test_create_db_objects_sets_db_up(notify_api, notify_service): @@ -167,6 +167,28 @@ def test_create_service_sms_senders_reuses_existing_sender_with_same_inbound_num assert sender.id == existing_sender.id +def test_create_service_sms_senders_reclaims_existing_sender_from_another_service(notify_service): + other_service = create_service(service_name="other sender owner") + inbound = create_inbound_number(number="07700900999", service_id=notify_service.id) + existing_sender = create_service_sms_sender( + service=other_service, + sms_sender="old", + is_default=True, + inbound_number_id=inbound.id, + ) + + sender = _create_service_sms_senders( + notify_service.id, + "07700900999", + True, + inbound.id, + ) + + assert sender.id == existing_sender.id + assert sender.service_id == notify_service.id + assert sender.sms_sender == "07700900999" + + @mock_aws def test_function_test_fixtures_saves_to_disk_and_ssm(notify_api, os_environ, mocker): mocker.patch("app.functional_tests_fixtures._create_db_objects", return_value={"FOO": "BAR", "BAZ": "WAZ"}) From 903cf030789b51606c7e3a8d93711ed18df2cf37 Mon Sep 17 00:00:00 2001 From: Ben Corlett Date: Fri, 27 Feb 2026 12:00:23 +0000 Subject: [PATCH 5/5] Use a new org name --- app/functional_tests_fixtures/__init__.py | 57 ++++++++++---- .../test_functional_test_fixtures.py | 75 ++++++++++++++++++- 2 files changed, 114 insertions(+), 18 deletions(-) diff --git a/app/functional_tests_fixtures/__init__.py b/app/functional_tests_fixtures/__init__.py index f75500d400..9d6436d5e1 100644 --- a/app/functional_tests_fixtures/__init__.py +++ b/app/functional_tests_fixtures/__init__.py @@ -4,6 +4,7 @@ import boto3 from flask import current_app +from itsdangerous.exc import BadSignature from sqlalchemy.exc import NoResultFound from app import db @@ -21,6 +22,7 @@ set_default_free_allowance_for_service, ) from app.dao.api_key_dao import ( + expire_api_key, get_model_api_keys, save_model_api_key, ) @@ -33,7 +35,6 @@ dao_create_organisation, dao_get_organisation_by_id, dao_get_organisations_by_partial_name, - dao_update_organisation, ) from app.dao.permissions_dao import permission_dao from app.dao.service_callback_api_dao import get_service_callback_api_by_callback_type, save_service_callback_api @@ -64,6 +65,7 @@ ) from app.dao.users_dao import get_user_by_email, save_model_user from app.models import ( + Domain, InboundNumber, InboundSms, Organisation, @@ -156,9 +158,10 @@ def _create_db_objects( current_app.logger.info("Creating functional test fixtures for %s:", environment) service_name_with_environment = f"Functional Tests ({environment})" + org_name_with_environment = f"{org_name} ({environment})" current_app.logger.info("--> Ensure organisation exists") - org = _create_organiation(email_domain, org_name) + org = _create_organiation(email_domain, org_name_with_environment) current_app.logger.info("--> Ensure users exists") func_test_user = _create_user( @@ -398,19 +401,29 @@ def _create_organiation(email_domain, org_name): org = Organisation(name=org_name, active=True, crown=False, organisation_type="central") dao_create_organisation(org) - dao_update_organisation( - org.id, - name=org_name, - active=True, - crown=False, - organisation_type="central", - notes=None, - request_to_go_live_notes=None, - agreement_signed_on_behalf_of_name=None, - agreement_signed_on_behalf_of_email_address=None, - domains=[email_domain], - can_approve_own_go_live_requests=True, - ) + org.name = org_name + org.active = True + org.crown = False + org.organisation_type = "central" + org.can_approve_own_go_live_requests = True + db.session.add(org) + + normalised_email_domain = email_domain.lower() + existing_domain = Domain.query.filter_by(domain=normalised_email_domain).one_or_none() + if existing_domain is None: + db.session.add(Domain(domain=normalised_email_domain, organisation_id=org.id)) + elif existing_domain.organisation_id != org.id: + previous_org_id = existing_domain.organisation_id + existing_domain.organisation_id = org.id + db.session.add(existing_domain) + current_app.logger.info( + "Reassigned domain %s from organisation %s to fixture organisation %s", + normalised_email_domain, + previous_org_id, + org.id, + ) + + db.session.commit() return org @@ -459,6 +472,20 @@ def _create_api_key(name, service_id, user_id, key_type="normal"): api_keys = get_model_api_keys(service_id=service_id) for key in api_keys: if key.name == name: + if key.expiry_date is not None: + continue + + try: + _ = key.secret + except BadSignature: + current_app.logger.warning( + "Fixture api key %s for service %s had invalid signature; expiring and recreating", + name, + service_id, + ) + expire_api_key(service_id=service_id, api_key_id=key.id) + continue + return key request = {"created_by": user_id, "key_type": key_type, "name": name} diff --git a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py index c2da2f39dd..d74c5446c0 100644 --- a/tests/app/functional_test_fixtures/test_functional_test_fixtures.py +++ b/tests/app/functional_test_fixtures/test_functional_test_fixtures.py @@ -7,9 +7,21 @@ from moto import mock_aws from app import db -from app.functional_tests_fixtures import _create_db_objects, _create_service_sms_senders, _create_user, apply_fixtures -from app.models import InboundNumber -from tests.app.db import create_inbound_number, create_service, create_service_sms_sender +from app.functional_tests_fixtures import ( + _create_api_key, + _create_db_objects, + _create_service_sms_senders, + _create_user, + apply_fixtures, +) +from app.models import ApiKey, Domain, InboundNumber, Organisation +from tests.app.db import ( + create_api_key, + create_inbound_number, + create_organisation, + create_service, + create_service_sms_sender, +) from tests.conftest import set_config_values @@ -77,6 +89,8 @@ def test_create_db_objects_sets_db_up(notify_api, notify_service): assert "FUNCTIONAL_TESTS_SERVICE_ID" in variables[0] assert variables[0]["FUNCTIONAL_TESTS_SERVICE_NAME"] == "Functional Tests (dev-env)" assert "FUNCTIONAL_TESTS_ORGANISATION_ID" in variables[0] + fixture_org = Organisation.query.get(variables[0]["FUNCTIONAL_TESTS_ORGANISATION_ID"]) + assert fixture_org.name == "Functional Tests Org (dev-env)" assert variables[0]["FUNCTIONAL_TESTS_SERVICE_API_KEY"].startswith("functional_tests_service_live_key-") assert variables[0]["FUNCTIONAL_TESTS_SERVICE_API_TEST_KEY"].startswith("functional_tests_service_test_key-") assert variables[0]["FUNCTIONAL_TESTS_API_AUTH_SECRET"] == "functional-tests-secret-key" @@ -148,6 +162,41 @@ def test_create_db_objects_reassigns_existing_inbound_number_from_another_servic assert str(reassigned_inbound.service_id) == str(variables["FUNCTIONAL_TESTS_SERVICE_ID"]) +def test_create_db_objects_reassigns_domain_to_environment_org(notify_api, notify_service): + previous_owner = create_organisation(name="Shared Org", domains=["digital.cabinet-office.gov.uk"]) + + with set_config_values( + notify_api, + { + "MMG_INBOUND_SMS_USERNAME": ["test_mmg_username"], + "MMG_INBOUND_SMS_AUTH": ["test_mmg_password"], + "INTERNAL_CLIENT_API_KEYS": {"notify-functional-tests": ["functional-tests-secret-key"]}, + "ADMIN_BASE_URL": "http://localhost:6012", + "API_HOST_NAME": "http://localhost:6011", + }, + ): + variables = _create_db_objects( + "fake password", + "test_request_bin_token", + "dev-env", + "notify-tests-preview", + "digital.cabinet-office.gov.uk", + "govuk_notify", + "functional_tests_service_live_key", + "functional_tests_service_test_key", + str(notify_service.id), + "Functional Tests Org", + "07700900500", + ) + + fixture_org = Organisation.query.get(variables["FUNCTIONAL_TESTS_ORGANISATION_ID"]) + domain = Domain.query.filter_by(domain="digital.cabinet-office.gov.uk").one() + + assert fixture_org.name == "Functional Tests Org (dev-env)" + assert str(domain.organisation_id) == str(fixture_org.id) + assert str(domain.organisation_id) != str(previous_owner.id) + + def test_create_service_sms_senders_reuses_existing_sender_with_same_inbound_number_id(notify_service): inbound = create_inbound_number(number="07700900888", service_id=notify_service.id) existing_sender = create_service_sms_sender( @@ -189,6 +238,26 @@ def test_create_service_sms_senders_reclaims_existing_sender_from_another_servic assert sender.sms_sender == "07700900999" +def test_create_api_key_recreates_when_existing_key_secret_is_invalid(notify_service, sample_user): + broken_key = create_api_key(notify_service, key_name="functional_tests_service_live_key") + broken_key._secret = "broken-signature-value" + db.session.add(broken_key) + db.session.commit() + + recreated_key = _create_api_key( + "functional_tests_service_live_key", + notify_service.id, + sample_user.id, + "normal", + ) + + refreshed_broken_key = ApiKey.query.get(broken_key.id) + assert refreshed_broken_key.expiry_date is not None + assert recreated_key.id != broken_key.id + assert recreated_key.expiry_date is None + assert recreated_key.secret is not None + + @mock_aws def test_function_test_fixtures_saves_to_disk_and_ssm(notify_api, os_environ, mocker): mocker.patch("app.functional_tests_fixtures._create_db_objects", return_value={"FOO": "BAR", "BAZ": "WAZ"})