Moved away from container_isntance() towards injection

README.md

@@ -244,14 +244,18 @@ export SIMPLIFIED_FCM_CREDENTIALS_FILE="/opt/credentials/fcm_credentials.json"
 The FCM credentials can be downloaded once a Google Service account has been created.
 More details in the [FCM documentation](https://firebase.google.com/docs/admin/setup#set-up-project-and-service-account)
 
-##### Quicksight Dashboards
+#### Quicksight Dashboards
 
 For generating quicksight dashboard links the following environment variable is required
 `QUICKSIGHT_AUTHORIZED_ARNS` - A dictionary of the format `"<dashboard name>": ["arn:aws:quicksight:...",...]`
 where each quicksight dashboard gets treated with an arbitrary "name", and a list of "authorized arns".
 The first the "authorized arns" is always considered as the `InitialDashboardID` when creating an embed URL
 for the respective "dashboard name".
 
+#### Analytics
+
+To enabled S3 based analytics add `PALACE_S3_ANALYTICS_ENABLED=true` else do not add the environment variable at all.
+
 #### Email
 
 ### Email sending

api/admin/controller/view.py

@@ -10,8 +10,7 @@
 from api.admin.controller.base import AdminController
 from api.admin.templates import admin as admin_template
 from api.config import Configuration
-from core.local_analytics_provider import LocalAnalyticsProvider
-from core.model import ConfigurationSetting, ExternalIntegration, Library, get_one
+from core.model import ConfigurationSetting, Library
 from core.util.problem_detail import ProblemDetail
 
 
@@ -80,13 +79,8 @@ def __call__(self, collection, book, path=None):
             or Configuration.DEFAULT_TOS_TEXT
         )
 
-        local_analytics = get_one(
-            self._db,
-            ExternalIntegration,
-            protocol=LocalAnalyticsProvider.__module__,
-            goal=ExternalIntegration.ANALYTICS_GOAL,
-        )
-        show_circ_events_download = local_analytics != None
+        # We always have local_analytics
+        show_circ_events_download = True
 
         response = Response(
             flask.render_template_string(

api/controller.py

@@ -208,9 +208,16 @@ class CirculationManager:
     admin_view_controller: ViewController
     admin_quicksight_controller: QuickSightController
 
-    def __init__(self, _db, services: Services):
+    @inject
+    def __init__(
+        self,
+        _db,
+        services: Services,
+        analytics: Analytics = Provide[Services.analytics.analytics],
+    ):
         self._db = _db
         self.services = services
+        self.analytics = analytics
         self.site_configuration_last_update = (
             Configuration.site_configuration_last_update(self._db, timeout=0)
         )
@@ -250,10 +257,7 @@ def reload_settings_if_changed(self):
             self.site_configuration_last_update = last_update
 
     @log_elapsed_time(log_method=log.info, message_prefix="load_settings")
-    @inject
-    def load_settings(
-        self, analytics: Analytics = Provide[Services.analytics.analytics]
-    ):
+    def load_settings(self):
         """Load all necessary configuration settings and external
         integrations from the database.
 
@@ -262,8 +266,6 @@ def load_settings(
         configuration after changes are made in the administrative
         interface.
         """
-        self.analytics = analytics
-
         with elapsed_time_logging(
             log_method=self.log.debug,
             skip_start=True,

core/feed/annotator/circulation.py

@@ -9,6 +9,7 @@
 from collections import defaultdict
 from typing import Any, Dict, List, Optional, Tuple
 
+from dependency_injector.wiring import Provide, inject
 from flask import url_for
 from sqlalchemy.orm import Session
 
@@ -18,6 +19,7 @@
 from api.config import Configuration
 from api.lanes import DynamicLane
 from api.novelist import NoveListAPI
+from core.analytics import Analytics
 from core.classifier import Classifier
 from core.config import CannotLoadConfiguration
 from core.entrypoint import EverythingEntryPoint
@@ -50,7 +52,7 @@
 )
 from core.model.patron import Hold, Loan, Patron
 from core.model.work import Work
-from core.service.container import container_instance
+from core.service.container import Services
 from core.util.datetime_helpers import from_timestamp
 from core.util.opds_writer import OPDSFeed
 
@@ -179,13 +181,15 @@ def format_types(cls, delivery_mechanism: DeliveryMechanism) -> List[str]:
 class CirculationManagerAnnotator(Annotator):
     hidden_content_types: list[str]
 
+    @inject
     def __init__(
         self,
         lane: Optional[WorkList],
         active_loans_by_work: Optional[Dict[Work, Loan]] = None,
         active_holds_by_work: Optional[Dict[Work, Hold]] = None,
         active_fulfillments_by_work: Optional[Dict[Work, Any]] = None,
         hidden_content_types: Optional[List[str]] = None,
+        analytics: Analytics = Provide[Services.analytics.analytics],
     ) -> None:
         if lane:
             logger_name = "Circulation Manager Annotator for %s" % lane.display_name
@@ -198,6 +202,7 @@ def __init__(
         self.active_fulfillments_by_work = active_fulfillments_by_work or {}
         self.hidden_content_types = hidden_content_types or []
         self.facet_view = "feed"
+        self.analytics = analytics
 
     def is_work_entry_solo(self, work: Work) -> bool:
         """Return a boolean value indicating whether the work's OPDS catalog entry is served by itself,
@@ -927,7 +932,7 @@ def annotate_work_entry(
                 )
             )
 
-        if container_instance().analytics.analytics().is_configured():
+        if self.analytics.is_configured():
             entry.computed.other_links.append(
                 Link(
                     rel="http://librarysimplified.org/terms/rel/analytics/open-book",

core/service/container.py

@@ -47,6 +47,7 @@ def create_container() -> Services:
             "api.bibliotheca",
             "api.enki",
             "api.controller",
+            "core.feed.annotator.circulation",
         ]
     )
     return container

tests/api/test_bibliotheca.py

@@ -4,7 +4,16 @@
 import random
 from datetime import datetime, timedelta
 from io import BytesIO, StringIO
-from typing import TYPE_CHECKING, ClassVar, Optional, Protocol, Type, runtime_checkable
+from typing import (
+    TYPE_CHECKING,
+    ClassVar,
+    Literal,
+    Optional,
+    Protocol,
+    Type,
+    cast,
+    runtime_checkable,
+)
 from unittest import mock
 from unittest.mock import MagicMock, create_autospec
 
@@ -41,6 +50,7 @@
     RemoteInitiatedServerError,
 )
 from api.web_publication_manifest import FindawayManifest
+from core.analytics import Analytics
 from core.integration.goals import Goals
 from core.integration.registry import IntegrationRegistry
 from core.metadata_layer import ReplacementPolicy, TimestampData
@@ -1178,7 +1188,21 @@ def initialized_monitor(self, db: DatabaseTransactionFixture):
         ],
     )
     def test_optional_iso_date_valid_dates(
-        self, specified_default_start, expected_default_start, default_monitor
+        self,
+        specified_default_start: datetime
+        | Literal[
+            "2011",
+            "2011-10",
+            "2011-10-05",
+            "2011-10-05T15",
+            "2011-10-05T15:27",
+            "2011-10-05T15:27:33",
+            "2011-10-05 15:27:33",
+            "2011-10-05T15:27:33.123456",
+        ]
+        | None,
+        expected_default_start: datetime | None,
+        default_monitor: BibliothecaPurchaseMonitor,
     ):
         # ISO 8601 strings, `datetime`s, or None are valid.
         actual_default_start = default_monitor._optional_iso_date(
@@ -1190,8 +1214,8 @@ def test_optional_iso_date_valid_dates(
 
     def test_monitor_intrinsic_start_time(
         self,
-        default_monitor,
-        initialized_monitor,
+        default_monitor: BibliothecaPurchaseMonitor,
+        initialized_monitor: BibliothecaPurchaseMonitor,
         bibliotheca_fixture: BibliothecaAPITestFixture,
     ):
         db = bibliotheca_fixture.db
@@ -1234,9 +1258,10 @@ def test_monitor_intrinsic_start_time(
     )
     def test_specified_start_trumps_intrinsic_default_start(
         self,
-        specified_default_start,
-        override_timestamp,
-        expected_start,
+        specified_default_start: Literal["2011-10-05T15:27", "2011-10-05T15:27:33"]
+        | None,
+        override_timestamp: bool,
+        expected_start: datetime | None,
         bibliotheca_fixture: BibliothecaAPITestFixture,
     ):
         db = bibliotheca_fixture.db
@@ -1312,9 +1337,10 @@ def test_specified_start_trumps_intrinsic_default_start(
     )
     def test_specified_start_can_override_timestamp(
         self,
-        specified_default_start,
-        override_timestamp,
-        expected_start,
+        specified_default_start: Literal["2011-10-05T15:27", "2011-10-05T15:27:33"]
+        | None,
+        override_timestamp: bool,
+        expected_start: datetime | None,
         bibliotheca_fixture: BibliothecaAPITestFixture,
     ):
         monitor = BibliothecaPurchaseMonitor(
@@ -1349,11 +1375,15 @@ def test_specified_start_can_override_timestamp(
         assert progress.start == expected_actual_start_time
 
     @pytest.mark.parametrize("input", [("invalid"), ("2020/10"), (["2020-10-05"])])
-    def test_optional_iso_date_invalid_dates(self, input, default_monitor):
+    def test_optional_iso_date_invalid_dates(
+        self,
+        input: list[str] | Literal["invalid", "2020/10"],
+        default_monitor: BibliothecaPurchaseMonitor,
+    ):
         with pytest.raises(ValueError) as excinfo:
             default_monitor._optional_iso_date(input)
 
-    def test_catch_up_from(self, default_monitor):
+    def test_catch_up_from(self, default_monitor: BibliothecaPurchaseMonitor):
         # catch_up_from() slices up its given timespan, calls
         # purchases() to find purchases for each slice, processes each
         # purchase using process_record(), and sets a checkpoint for each
@@ -1425,7 +1455,7 @@ def test_catch_up_from(self, default_monitor):
             progress, start, full_slice[0], "MARC records processed: 1"
         )
 
-    def test__checkpoint(self, default_monitor):
+    def test__checkpoint(self, default_monitor: BibliothecaPurchaseMonitor):
         # The _checkpoint method allows the BibliothecaPurchaseMonitor
         # to preserve its progress in case of a crash.
 
@@ -1452,7 +1482,7 @@ def test__checkpoint(self, default_monitor):
         assert timestamp_obj.start == BibliothecaPurchaseMonitor.DEFAULT_START_TIME
         assert timestamp_obj.finish == finish
 
-    def test_purchases(self, default_monitor):
+    def test_purchases(self, default_monitor: BibliothecaPurchaseMonitor):
         # The purchases() method calls marc_request repeatedly, handling
         # pagination.
 
@@ -1475,15 +1505,18 @@ def test_purchases(self, default_monitor):
         assert ([1] * 50) + ([2] * 50) + ([3] * 49) == records
 
     def test_process_record(
-        self, default_monitor, caplog, bibliotheca_fixture: BibliothecaAPITestFixture
+        self,
+        default_monitor: BibliothecaPurchaseMonitor,
+        caplog: pytest.LogCaptureFixture,
+        bibliotheca_fixture: BibliothecaAPITestFixture,
     ):
         # process_record may create a LicensePool, trigger the
         # bibliographic coverage provider, and/or issue a "license
         # added" analytics event, based on the identifier found in a
         # MARC record.
         purchase_time = utc_now()
         analytics = MockAnalyticsProvider()
-        default_monitor.analytics = analytics
+        default_monitor.analytics = cast(Analytics, analytics)
         ensure_coverage = MagicMock()
         default_monitor.bibliographic_coverage_provider.ensure_coverage = (
             ensure_coverage
@@ -1548,7 +1581,9 @@ def test_process_record(
         assert analytics.count == 0
 
     def test_end_to_end(
-        self, default_monitor, bibliotheca_fixture: BibliothecaAPITestFixture
+        self,
+        default_monitor: BibliothecaPurchaseMonitor,
+        bibliotheca_fixture: BibliothecaAPITestFixture,
     ):
         # Limited end-to-end test of the BibliothecaPurchaseMonitor.
 
@@ -1564,10 +1599,10 @@ def test_end_to_end(
         # book, and one to the metadata endpoint for information about
         # that book.
         api = default_monitor.api
-        api.queue_response(
+        api.queue_response(  # type: ignore [attr-defined]
             200, content=bibliotheca_fixture.files.sample_data("marc_records_one.xml")
         )
-        api.queue_response(
+        api.queue_response(  # type: ignore [attr-defined]
             200,
             content=bibliotheca_fixture.files.sample_data("item_metadata_single.xml"),
         )
@@ -1592,7 +1627,7 @@ def test_end_to_end(
         # An analytics event was issued to commemorate the addition of
         # the book to the collection.
         # No more DISTRIBUTOR events
-        assert default_monitor.analytics.count == 0
+        assert default_monitor.analytics.count == 0  # type: ignore [attr-defined]
 
         # The timestamp has been updated; the next time the monitor
         # runs it will ask for purchases that haven't happened yet.
@@ -1741,7 +1776,7 @@ def test_handle_event(self, bibliotheca_fixture: BibliothecaAPITestFixture):
             db.session,
             bibliotheca_fixture.collection,
             api_class=api,
-            analytics=analytics,  # type: ignore [arg-type]
+            analytics=cast(Analytics, analytics),
         )
 
         now = utc_now()

tests/api/test_controller_analytics.py

@@ -32,15 +32,7 @@ def test_track_event(self, analytics_fixture: AnalyticsFixture):
             assert 400 == response.status_code
             assert INVALID_ANALYTICS_EVENT_TYPE.uri == response.uri
 
-        # If there is no active patron, or if the patron has no
-        # associated neighborhood, the CirculationEvent is created
-        # with no location.
         patron = db.patron()
-
-        # If the patron has an associated neighborhood, and the
-        # analytics controller is set up to use patron neighborhood as
-        # event location, then the CirculationEvent is created with
-        # that neighborhood as its location.
         patron.neighborhood = "Mars Grid 4810579"
         with analytics_fixture.request_context_with_library("/"):
             flask.request.patron = patron  # type: ignore
@@ -58,5 +50,7 @@ def test_track_event(self, analytics_fixture: AnalyticsFixture):
                 license_pool=analytics_fixture.lp,
             )
             assert circulation_event is not None
-            assert patron.neighborhood == circulation_event.location
+            assert (
+                circulation_event.location == None
+            )  # We no longer use the location source
             db.session.delete(circulation_event)

tests/api/test_controller_cm.py

@@ -35,8 +35,8 @@ def test_load_settings(
         # Certain fields of the CirculationManager have certain values
         # which are about to be reloaded.
         manager._external_search = object()
-        manager.auth = object()  # type: ignore [assignment]
-        manager.patron_web_domains = object()  # type: ignore [assignment]
+        manager.auth = object()
+        manager.patron_web_domains = object()
 
         # But some fields are _not_ about to be reloaded
         index_controller = manager.index_controller

tests/api/test_controller_loan.py

@@ -113,7 +113,9 @@ class MockLibraryAuthenticator:
 
         short_name = loan_fixture.db.default_library().short_name
         assert short_name is not None
-        loan_fixture.manager.auth.library_authenticators[short_name] = MockLibraryAuthenticator()  # type: ignore [assignment]
+        loan_fixture.manager.auth.library_authenticators[
+            short_name
+        ] = MockLibraryAuthenticator()
 
         def mock_can_fulfill_without_loan(patron, pool, lpdm):
             self.called_with = (patron, pool, lpdm)