#2019 - Bug fix MobileAppRegistry is a Singleton

Co-authored-by: David J. Kalbfleisch <[email protected]>

.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"

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)
 

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

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]:

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__,

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,

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

pyproject.toml

@@ -6,6 +6,7 @@ authors = [
     "VA Notify <[email protected]>",
 ]
 readme = "README.md"
+package-mode = false
 
 [tool.poetry.dependencies]
 python = "^3.10.13"

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

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'}

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'}