From 5776abedb34984c7b591f8b75717e4917a904275 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Fri, 3 May 2024 14:08:59 -0300 Subject: [PATCH] Remove AUTOINITIALIZE env variable (#1824) Since importing app no longer automatically initializes anything, we are able to clean up this conditional code. --- src/palace/manager/api/app.py | 34 ++++++++++------------- tests/manager/api/test_app.py | 36 +++++++++++++++++++++++-- tests/manager/api/test_authenticator.py | 7 +---- 3 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/palace/manager/api/app.py b/src/palace/manager/api/app.py index b8aef5bfbe..705288b6c3 100644 --- a/src/palace/manager/api/app.py +++ b/src/palace/manager/api/app.py @@ -1,5 +1,4 @@ import logging -import os import flask_babel from flask import request @@ -17,7 +16,7 @@ PalaceXrayProfiler, ) from palace.manager.core.app_server import ErrorHandler -from palace.manager.service.container import Services, container_instance +from palace.manager.service.container import container_instance from palace.manager.sqlalchemy.flask_sqlalchemy_session import flask_scoped_session from palace.manager.sqlalchemy.model.key import Key, KeyType from palace.manager.sqlalchemy.session import SessionManager @@ -66,24 +65,19 @@ def initialize_admin(_db: Session | None = None): ).value -def initialize_circulation_manager(container: Services): - if os.environ.get("AUTOINITIALIZE") == "False": - # It's the responsibility of the importing code to set app.manager - # appropriately. - pass - else: - if getattr(app, "manager", None) is None: - try: - app.manager = CirculationManager(app._db) - except Exception: - logging.exception("Error instantiating circulation manager!") - raise - # Make sure that any changes to the database (as might happen - # on initial setup) are committed before continuing. - app.manager._db.commit() +def initialize_circulation_manager(): + if getattr(app, "manager", None) is None: + try: + app.manager = CirculationManager(app._db) + except Exception: + logging.exception("Error instantiating circulation manager!") + raise + # Make sure that any changes to the database (as might happen + # on initial setup) are committed before continuing. + app.manager._db.commit() - # setup the cache data object - CachedData.initialize(app._db) + # setup the cache data object + CachedData.initialize(app._db) def initialize_database(): @@ -113,6 +107,6 @@ def initialize_application() -> PalaceFlask: app.register_error_handler(Exception, error_handler.handle) # Initialize the circulation manager - initialize_circulation_manager(container) + initialize_circulation_manager() initialize_admin() return app diff --git a/tests/manager/api/test_app.py b/tests/manager/api/test_app.py index d7d79874f1..055cd11caa 100644 --- a/tests/manager/api/test_app.py +++ b/tests/manager/api/test_app.py @@ -1,6 +1,11 @@ -from unittest.mock import patch +from unittest.mock import MagicMock, patch -from palace.manager.api.app import initialize_application +import pytest + +from palace.manager.api.app import ( + initialize_application, + initialize_circulation_manager, +) from palace.manager.util.http import HTTP from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.services import ServicesFixture @@ -23,3 +28,30 @@ def test_initialize_application_http( # Make sure that the HTTP configuration was set mock_set_quick_failure_settings.assert_called_once() + + +def test_initialize_circulation_manager(caplog: pytest.LogCaptureFixture): + with ( + patch("palace.manager.api.app.app") as mock_app, + patch("palace.manager.api.app.CirculationManager") as mock_circulation_manager, + patch("palace.manager.api.app.CachedData") as mock_cached_data, + ): + # If app is already initialized, it should not be re-initialized + mock_app.manager = MagicMock() + initialize_circulation_manager() + mock_circulation_manager.assert_not_called() + mock_cached_data.initialize.assert_not_called() + + # If app is not initialized, it should be initialized + mock_app.manager = None + initialize_circulation_manager() + mock_circulation_manager.assert_called_once() + mock_cached_data.initialize.assert_called_once() + assert mock_app.manager == mock_circulation_manager.return_value + + # If an exception is raised, it should be logged + mock_app.manager = None + mock_circulation_manager.side_effect = Exception("Test exception") + with pytest.raises(Exception): + initialize_circulation_manager() + assert "Error instantiating circulation manager!" in caplog.text diff --git a/tests/manager/api/test_authenticator.py b/tests/manager/api/test_authenticator.py index 73298c5fc6..b621f69a01 100644 --- a/tests/manager/api/test_authenticator.py +++ b/tests/manager/api/test_authenticator.py @@ -5,7 +5,6 @@ import datetime import json -import os import re from collections.abc import Callable from decimal import Decimal @@ -1020,12 +1019,8 @@ def _geographic_areas(cls, library): # We're about to call url_for, so we must create an # application context. - os.environ["AUTOINITIALIZE"] = "False" from palace.manager.api.app import app - self.app = app - del os.environ["AUTOINITIALIZE"] - # Set up configuration settings for links. library_settings.terms_of_service = "http://terms.com" # type: ignore[assignment] library_settings.privacy_policy = "http://privacy.com" # type: ignore[assignment] @@ -1091,7 +1086,7 @@ def _geographic_areas(cls, library): finish=announcement_fixture.yesterday, ) - with self.app.test_request_context("/"): + with app.test_request_context("/"): url = authenticator.authentication_document_url() assert url.endswith("/%s/authentication_document" % library.short_name)