From ff65d1d2c3fe2b2e7fa0b91629531ace2fb983c0 Mon Sep 17 00:00:00 2001 From: Mack Halliday Date: Thu, 31 Oct 2024 10:26:53 -0400 Subject: [PATCH] #2019 - Bug fix MobileAppRegistry is a Singleton Co-authored-by: David J. Kalbfleisch <1.21e9W@protonmail.com> --- .talismanrc | 6 ++- app/__init__.py | 7 ++++ app/mobile_app/__init__.py | 1 - app/mobile_app/mobile_app_registry.py | 24 ++++-------- app/status/healthcheck.py | 6 +-- app/v2/notifications/rest_push.py | 22 +++++------ ci/.local.env | 4 +- pyproject.toml | 1 + .../mobile_app/test_mobile_app_registry.py | 37 ++++++++++--------- .../test_push_broadcast_notifications.py | 21 +++-------- .../notifications/test_push_notifications.py | 17 +-------- 11 files changed, 59 insertions(+), 87 deletions(-) diff --git a/.talismanrc b/.talismanrc index 5db41eed01..cdd47b1a94 100644 --- a/.talismanrc +++ b/.talismanrc @@ -25,6 +25,8 @@ fileignoreconfig: checksum: 9e5161e8a0a13974d9b67d8a7e61d1b3fed9657a7e2dfeb6d82fd8ace64e2715 - filename: ci/docker-compose-test.yml checksum: e3efec2749e8c19e60f5bfc68eafabe24eba647530a482ceccfc4e0e62cff424 +- filename: ci/.local.env + checksum: 8caee8cf67974ad5195c835d3d266d81dbc4c635c547b7cc49704c0593e7833b - filename: lambda_functions/pinpoint_callback/pinpoint_callback_lambda.py checksum: 7bd4900e14b1fa789bbb2568b8a8d7a400e3c8350ba32fb44cc0b5b66a2df037 - filename: lambda_functions/ses_callback/ses_callback_lambda.py @@ -64,7 +66,7 @@ fileignoreconfig: - filename: app/commands.py checksum: 79d914948e4e28dabc9c074956a11a9ceec27e7ae57b8eb8fb483f4a6217f07d - filename: app/constants.py - checksum: 9262bddf98c91c1eb0bbad4aae89b436f898ae664050920a5755b6475d2821a9 + checksum: 44390d0a1258b184cf84dc9b6e97bd0768af84a9aa346ba963aa7735fc8bcb36 - filename: app/letters/utils.py checksum: 5e6071b9cab380f9f3ee172f8c731061241200f53453a9863f22bb5eaa05e6af - filename: app/service/utils.py @@ -78,5 +80,5 @@ fileignoreconfig: - filename: app/service/rest.py checksum: 7f8a30dd84b3ceb0d08bae949b5b127fd408ee2fd8097eb7d4b291ede61f8d0f - filename: tests/app/celery/test_process_delivery_status_result_tasks.py - checksum: c1c596d40f5bc8c8621204c6cfdd1c8adda0092a396fcd73b1d9f843f4951d7f + checksum: 62fa6216b62971d62c2e53f6b31aeeb659d7a1e404665362ee89cb3ec04793a6 version: "1.0" diff --git a/app/__init__.py b/app/__init__.py index 508f612bad..bebe4fc81b 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -37,6 +37,7 @@ from app.encryption import Encryption from app.attachments.store import AttachmentStore from app.db import db +from app.mobile_app.mobile_app_registry import MobileAppRegistry load_dotenv() @@ -50,6 +51,9 @@ mmg_client = MMGClient() aws_ses_client = AwsSesClient() +# This is initialized below, in create_app. +mobile_app_registry = None + from app.clients.email.govdelivery_client import GovdeliveryClient # noqa govdelivery_client = GovdeliveryClient() @@ -213,6 +217,9 @@ def create_app(application): jwt.init_app(application) + global mobile_app_registry + mobile_app_registry = MobileAppRegistry(application.logger) + register_blueprint(application) register_v2_blueprints(application) diff --git a/app/mobile_app/__init__.py b/app/mobile_app/__init__.py index ae238ea95d..cc6d35a9ea 100644 --- a/app/mobile_app/__init__.py +++ b/app/mobile_app/__init__.py @@ -1,3 +1,2 @@ from .mobile_app_types import MobileAppType, DEAFULT_MOBILE_APP_TYPE # noqa from .mobile_app import MobileApp # noqa -from .mobile_app_registry import MobileAppRegistry # noqa diff --git a/app/mobile_app/mobile_app_registry.py b/app/mobile_app/mobile_app_registry.py index 4dae636b69..ba0c6e29c5 100644 --- a/app/mobile_app/mobile_app_registry.py +++ b/app/mobile_app/mobile_app_registry.py @@ -1,35 +1,27 @@ from typing import List -from flask import current_app -from .mobile_app_types import MobileAppType + from .mobile_app import MobileApp +from .mobile_app_types import MobileAppType class MobileAppRegistry: - _instance = None - - def __new__( - cls, - *args, - **kwargs, - ): - if not cls._instance: - cls._instance = super().__new__(cls) - return cls._instance - - def __init__(self): + def __init__(self, logger): + self.logger = logger self._registry = {} + + logger.info('Initializing MobileAppRegistry') for type in MobileAppType: try: app = MobileApp(type) except ValueError: - current_app.logger.warning('Missing environment sid for type: %s and value: %s_SID', type, type.value) + logger.warning('Missing environment sid for type: %s and value: %s_SID', type, type.value) else: self._registry[type] = app def get_app( self, app_type: MobileAppType, - ) -> MobileApp: + ) -> MobileApp | None: return self._registry.get(app_type) def get_registered_apps(self) -> List[MobileAppType]: diff --git a/app/status/healthcheck.py b/app/status/healthcheck.py index 684e3e389e..aa5749062f 100644 --- a/app/status/healthcheck.py +++ b/app/status/healthcheck.py @@ -1,10 +1,9 @@ -from flask import jsonify, Blueprint, request +from flask import Blueprint, jsonify, request -from app import db, version, provider_service +from app import db, mobile_app_registry, version, provider_service from app.dao.services_dao import dao_count_live_services from app.dao.organisation_dao import dao_count_organsations_with_live_services from app.notifications.notification_type import NotificationType -from app.mobile_app import MobileAppRegistry from .redis_check import redis_check status = Blueprint('status', __name__) @@ -16,7 +15,6 @@ def show_status(): if request.args.get('simple', None): return jsonify(status='ok'), 200 else: - mobile_app_registry = MobileAppRegistry() return jsonify( status='ok', # This should be considered part of the public API git_commit=version.__git_commit__, diff --git a/app/v2/notifications/rest_push.py b/app/v2/notifications/rest_push.py index 709163f7ac..fb019a0f69 100644 --- a/app/v2/notifications/rest_push.py +++ b/app/v2/notifications/rest_push.py @@ -1,14 +1,15 @@ -from app import authenticated_service, vetext_client +from flask import jsonify, request + +from app import authenticated_service, vetext_client, mobile_app_registry from app.constants import PUSH_TYPE -from app.feature_flags import is_feature_enabled, FeatureFlag -from app.mobile_app import MobileAppRegistry, MobileAppType, DEAFULT_MOBILE_APP_TYPE +from app.feature_flags import FeatureFlag, is_feature_enabled +from app.mobile_app import DEAFULT_MOBILE_APP_TYPE, MobileAppType from app.schema_validation import validate from app.utils import get_public_notify_type_text from app.v2.errors import BadRequestError from app.v2.notifications import v2_notification_blueprint -from app.v2.notifications.notification_schemas import push_notification_request, push_notification_broadcast_request -from app.va.vetext import VETextRetryableException, VETextNonRetryableException, VETextBadRequestException -from flask import request, jsonify +from app.v2.notifications.notification_schemas import push_notification_broadcast_request, push_notification_request +from app.va.vetext import VETextBadRequestException, VETextNonRetryableException, VETextRetryableException @v2_notification_blueprint.route('/push', methods=['POST']) @@ -36,15 +37,12 @@ def push_notification_helper(schema: dict, is_broadcast: bool): ) req_json = validate(request.get_json(), schema) - registry = MobileAppRegistry() - if req_json.get('mobile_app'): - app_instance = registry.get_app(MobileAppType[req_json['mobile_app']]) + if 'mobile_app' in req_json: + app_instance = mobile_app_registry.get_app(MobileAppType[req_json['mobile_app']]) else: - app_instance = registry.get_app(DEAFULT_MOBILE_APP_TYPE) + app_instance = mobile_app_registry.get_app(DEAFULT_MOBILE_APP_TYPE) - if not app_instance: - return jsonify(result='error', message='Mobile app is not initialized'), 503 try: vetext_client.send_push_notification( app_instance.sid, diff --git a/ci/.local.env b/ci/.local.env index 5cb1acac12..cf151e4f36 100644 --- a/ci/.local.env +++ b/ci/.local.env @@ -30,14 +30,14 @@ SQLALCHEMY_DATABASE_URI_READ=postgresql://postgres:LocalPassword@db:5432/notific TRUNCATE_ARTIFACTS=False TWILIO_ACCOUNT_SID=TWILIO_TEST_ACCOUNT_SID_XXX TWILIO_AUTH_TOKEN=add_value -VA_FLAGSHIP_APP_SID=bar +VA_FLAGSHIP_APP_SID=VA_FLAGSHIP_APP_sid VA_ONSITE_URL=http://host.docker.internal:7005 VA_PROFILE_URL=http://host.docker.internal:7005 VANOTIFY_SSL_CERT=CERT-FILE VANOTIFY_SSL_CERT_PATH=certs/ssl_cert.pem VANOTIFY_SSL_KEY=KEY-FILE VANOTIFY_SSL_KEY_PATH=certs/ssl_key.pem -VETEXT_SID=foo +VETEXT_SID=VETEXT_sid VETEXT_URL=http://host.docker.internal:7008/api/vetext/pub # Feature flags diff --git a/pyproject.toml b/pyproject.toml index c99c72d6dd..95dddde537 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -6,6 +6,7 @@ authors = [ "VA Notify ", ] readme = "README.md" +package-mode = false [tool.poetry.dependencies] python = "^3.10.13" diff --git a/tests/app/mobile_app/test_mobile_app_registry.py b/tests/app/mobile_app/test_mobile_app_registry.py index d1758c325c..ce6c4e477f 100644 --- a/tests/app/mobile_app/test_mobile_app_registry.py +++ b/tests/app/mobile_app/test_mobile_app_registry.py @@ -1,20 +1,15 @@ -import pytest import os -from app.mobile_app import MobileAppType, MobileAppRegistry +import pytest +from unittest.mock import Mock +from app.mobile_app.mobile_app_registry import MobileAppRegistry +from app.mobile_app.mobile_app_types import MobileAppType -@pytest.fixture(autouse=True) -def mock_logger(mocker): - app_context_mock = mocker.patch('app.mobile_app.mobile_app_registry.current_app') - return app_context_mock.logger - -def test_registry_is_singleton( - client, -): - registry = MobileAppRegistry() - another_registry = MobileAppRegistry() - assert registry == another_registry +@pytest.fixture(autouse=True) +def mock_logger(): + logger = Mock() + return logger @pytest.mark.parametrize( @@ -30,11 +25,12 @@ def test_registry_initilizes_mobile_apps( mocker, apps, sids, + mock_logger, ): for app, sid in zip(apps, sids): mocker.patch.dict(os.environ, {f'{app.value}_SID': sid}) - registry = MobileAppRegistry() + registry = MobileAppRegistry(mock_logger) for app, sid in zip(apps, sids): assert registry.get_app(app).sid == sid @@ -54,11 +50,16 @@ def test_registry_initilizes_only_apps_with_sids_in_env( mocker, env, registered_app, + mock_logger, ): + """ + Note that the case where both apps have SIDs is tested above, in test_registry_initilizes_mobile_apps. + """ + mocker.patch.dict(os.environ, env) - registry = MobileAppRegistry() + registry = MobileAppRegistry(mock_logger) - expected_list = [registered_app] if registered_app else [] + expected_list = [registered_app] if (registered_app is not None) else [] assert registry.get_registered_apps() == expected_list @@ -69,7 +70,7 @@ def test_should_log_warning_for_uninitialized_apps_with_correct_count( ): for app in MobileAppType.values(): mocker.patch.dict(os.environ, {f'{app}_SID': ''}) - MobileAppRegistry() + MobileAppRegistry(mock_logger) assert mock_logger.warning.call_count == len(MobileAppType.values()) @@ -81,7 +82,7 @@ def test_should_correctly_log_warning_for_uninitialized_apps( app_type_str, ): mocker.patch.dict(os.environ, {f'{app_type_str}_SID': ''}) - MobileAppRegistry() + MobileAppRegistry(mock_logger) app_type = MobileAppType(app_type_str) mock_logger.warning.assert_called_once_with( 'Missing environment sid for type: %s and value: %s_SID', app_type, app_type.value diff --git a/tests/app/v2/notifications/test_push_broadcast_notifications.py b/tests/app/v2/notifications/test_push_broadcast_notifications.py index 13a9085ee7..b247367439 100644 --- a/tests/app/v2/notifications/test_push_broadcast_notifications.py +++ b/tests/app/v2/notifications/test_push_broadcast_notifications.py @@ -185,11 +185,15 @@ def test_makes_call_to_vetext_client( personalisation, app, ): + """ + App SIDs should be loaded from the environment. For testing, the is .local.env. + """ + service = sample_service(service_permissions=[PUSH_TYPE]) post_send_push_broadcast_notification(client, sample_api_key(service), payload) vetext_client.send_push_notification.assert_called_once_with( - f'some_sid_for_{app}', payload['template_id'], payload['topic_sid'], True, personalisation + f'{app}_sid', payload['template_id'], payload['topic_sid'], True, personalisation ) @pytest.mark.parametrize('exception', [VETextRetryableException, VETextNonRetryableException]) @@ -234,18 +238,3 @@ def test_maps_bad_request_exception( assert response.status_code == 400 resp_json = response.get_json() assert {'error': 'BadRequestError', 'message': exception.message} in resp_json['errors'] - - @pytest.mark.disable_autouse - def test_returns_503_if_mobile_app_not_initiliazed( - self, - client, - sample_api_key, - sample_service, - ): - service = sample_service(service_permissions=[PUSH_TYPE]) - - response = post_send_push_broadcast_notification(client, sample_api_key(service), PUSH_BROADCAST_REQUEST) - - assert response.status_code == 503 - resp_json = response.get_json() - assert resp_json == {'result': 'error', 'message': 'Mobile app is not initialized'} diff --git a/tests/app/v2/notifications/test_push_notifications.py b/tests/app/v2/notifications/test_push_notifications.py index 1b0ca4ce01..4207ad7824 100644 --- a/tests/app/v2/notifications/test_push_notifications.py +++ b/tests/app/v2/notifications/test_push_notifications.py @@ -218,7 +218,7 @@ def test_makes_call_to_vetext_client( post_send_notification(client, sample_api_key(service), PUSH_TYPE, payload) vetext_client.send_push_notification.assert_called_once_with( - f'some_sid_for_{app}', + f'{app}_sid', payload['template_id'], payload['recipient_identifier']['id_value'], False, @@ -267,18 +267,3 @@ def test_maps_bad_request_exception( assert response.status_code == 400 resp_json = response.get_json() assert {'error': 'BadRequestError', 'message': exception.message} in resp_json['errors'] - - @pytest.mark.disable_autouse - def test_returns_503_if_mobile_app_not_initiliazed( - self, - client, - sample_api_key, - sample_service, - ): - service = sample_service(service_permissions=[PUSH_TYPE]) - - response = post_send_notification(client, sample_api_key(service), PUSH_TYPE, PUSH_REQUEST) - - assert response.status_code == 503 - resp_json = response.get_json() - assert resp_json == {'result': 'error', 'message': 'Mobile app is not initialized'}