diff --git a/.ds.baseline b/.ds.baseline index 9077a065b..9f962cba3 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -305,7 +305,7 @@ "filename": "tests/app/service/test_rest.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 1285, + "line_number": 1282, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2025-06-02T13:22:36Z" + "generated_at": "2025-06-09T16:07:54Z" } diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 96836714e..22bbaeba2 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -62,15 +62,9 @@ def get_notification_by_id(notification_id): notification.body = template.content_with_placeholders_filled_in if hasattr(template, "subject"): try: - notification.__dict__["subject"] = template.subject - except AttributeError as e: - current_app.logger.warning( - f"Could not set subject on notification: {e}" - ) - except Exception as e: - current_app.logger.error( - f"Unexpected error setting notification subject: {e}" - ) + notification.subject = template.subject + except Exception: + pass schema = PublicNotificationResponseSchema() schema.context = {"notification_instance": notification} diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7c040f079..3a706ec7a 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -738,8 +738,6 @@ def test_update_service(client, notify_db_session, sample_service): headers=[("Content-Type", "application/json"), auth_header], ) result = resp.json - print(resp.status_code) - print(resp.json) assert resp.status_code == 200 assert result["data"]["name"] == "updated service name" @@ -1281,7 +1279,7 @@ def test_add_existing_user_to_another_service_with_all_permissions( == user_already_in_service.email_address ) - fake_password = "password" # pragma: allowlist secret + fake_password = "password" # add new user to service user_to_add = User( name="Invited User", @@ -1356,7 +1354,7 @@ def test_add_existing_user_to_another_service_with_send_permissions( user_to_add = User( name="Invited User", email_address="invited@digital.fake.gov", - password="password", # pragma: allowlist secret + password="password", mobile_number="+14254147755", ) save_model_user(user_to_add, validated_email_access=True) @@ -1406,7 +1404,7 @@ def test_add_existing_user_to_another_service_with_manage_permissions( user_to_add = User( name="Invited User", email_address="invited@digital.fake.gov", - password="password", # pragma: allowlist secret + password="password", mobile_number="+14254147755", ) save_model_user(user_to_add, validated_email_access=True) @@ -1457,7 +1455,7 @@ def test_add_existing_user_to_another_service_with_folder_permissions( user_to_add = User( name="Invited User", email_address="invited@digital.fake.gov", - password="password", # pragma: allowlist secret + password="password", mobile_number="+14254147755", ) save_model_user(user_to_add, validated_email_access=True) @@ -1498,7 +1496,7 @@ def test_add_existing_user_to_another_service_with_manage_api_keys( user_to_add = User( name="Invited User", email_address="invited@digital.fake.gov", - password="password", # pragma: allowlist secret: keyword + password="password", mobile_number="+14254147755", ) save_model_user(user_to_add, validated_email_access=True) @@ -1538,7 +1536,7 @@ def test_add_existing_user_to_non_existing_service_returns404( user_to_add = User( name="Invited User", email_address="invited@digital.fake.gov", - password="password", # pragma: allowlist secret: keyword + password="password", mobile_number="+14254147755", ) save_model_user(user_to_add, validated_email_access=True) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 89c66cfc2..5d313c4a6 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -3,12 +3,14 @@ from datetime import date, datetime import pytest from freezegun import freeze_time -from app.enums import ServicePermissionType +from app.enums import ServicePermissionType, TemplateType from app.utils import ( get_midnight_in_utc, get_public_notify_type_text, + get_template_instance, midnight_n_days_ago, ) +from notifications_utils.template import HTMLEmailTemplate, SMSMessageTemplate @pytest.mark.parametrize( @@ -52,3 +54,87 @@ def test_get_public_notify_type_text(): assert ( get_public_notify_type_text(ServicePermissionType.UPLOAD_DOCUMENT) == "document" ) + +@pytest.mark.parametrize( + "template_type, expected_class", + [ + (TemplateType.SMS, SMSMessageTemplate), + (TemplateType.EMAIL, HTMLEmailTemplate), + ], +) +def test_get_template_instance_with_none_values(template_type, expected_class): + """Test that get_template_instance handles None values safely for both template types""" + template = { + "template_type": template_type, + "id": "test-id", + "content": "Test content", + "subject": "Test subject" if template_type == TemplateType.EMAIL else None, + } + + result = get_template_instance(template, values=None) + assert isinstance(result, expected_class) + + result_default = get_template_instance(template) + assert isinstance(result_default, expected_class) + + +@pytest.mark.parametrize( + "template_type, expected_class", + [ + (TemplateType.SMS, SMSMessageTemplate), + (TemplateType.EMAIL, HTMLEmailTemplate), + ], +) +def test_get_template_instance_with_actual_values(template_type, expected_class): + """Test that get_template_instance works normally with actual values""" + template = { + "template_type": template_type, + "id": "test-id", + "content": "Hello ((name))", + "subject": "Test subject" if template_type == TemplateType.EMAIL else None, + } + + values = {"name": "World"} + + result = get_template_instance(template, values=values) + assert isinstance(result, expected_class) + + +def test_get_template_instance_invalid_template_type(): + """Test that get_template_instance raises KeyError for invalid template types""" + template = { + "template_type": "INVALID_TYPE", + "id": "test-id", + "content": "Test content", + } + + with pytest.raises(KeyError): + get_template_instance(template, values=None) + + +@pytest.mark.parametrize( + "template_type, values", + [ + (TemplateType.SMS, None), + (TemplateType.SMS, {}), + (TemplateType.SMS, {"name": "test"}), + (TemplateType.EMAIL, None), + (TemplateType.EMAIL, {}), + (TemplateType.EMAIL, {"name": "test"}), + ], +) +def test_get_template_instance_comprehensive(template_type, values): + """Comprehensive test covering all combinations of template types and value scenarios""" + template = { + "template_type": template_type, + "id": "test-id", + "content": "Test content ((name))" if values and "name" in values else "Test content", + "subject": "Test subject" if template_type == TemplateType.EMAIL else None, + } + + result = get_template_instance(template, values=values) + + if template_type == TemplateType.SMS: + assert isinstance(result, SMSMessageTemplate) + else: + assert isinstance(result, HTMLEmailTemplate)