Skip to content

Commit

Permalink
Fixed more tests and mypy errors.
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathangreen committed Sep 13, 2023
1 parent 6fc943f commit 5199fef
Show file tree
Hide file tree
Showing 13 changed files with 168 additions and 248 deletions.
10 changes: 1 addition & 9 deletions api/admin/controller/analytics_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from api.google_analytics_provider import GoogleAnalyticsProvider
from api.s3_analytics_provider import S3AnalyticsProvider
from core.local_analytics_provider import LocalAnalyticsProvider
from core.model import ExternalIntegration, ExternalIntegrationLink
from core.model import ExternalIntegration

Check warning on line 9 in api/admin/controller/analytics_services.py

View check run for this annotation

Codecov / codecov/patch

api/admin/controller/analytics_services.py#L9

Added line #L9 was not covered by tests
from core.util import first_or_default
from core.util.problem_detail import ProblemDetail

Expand Down Expand Up @@ -95,14 +95,6 @@ def process_post(self):

service.name = name

external_integration_link = self._set_storage_external_integration_link(
service,
ExternalIntegrationLink.ANALYTICS,
S3UploaderConfiguration.ANALYTICS_BUCKET_KEY,
)
if isinstance(external_integration_link, ProblemDetail):
return external_integration_link

if is_new:
return Response(str(service.id), 201)
else:
Expand Down
41 changes: 1 addition & 40 deletions api/admin/controller/catalog_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,12 @@
from api.admin.problem_details import (
CANNOT_CHANGE_PROTOCOL,
INTEGRATION_NAME_ALREADY_IN_USE,
MISSING_INTEGRATION,
MISSING_SERVICE,
MULTIPLE_SERVICES_FOR_LIBRARY,
UNKNOWN_PROTOCOL,
)
from core.marc import MARCExporter
from core.model import ExternalIntegration, get_one, get_one_or_create
from core.model.configuration import ExternalIntegrationLink
from core.model import ExternalIntegration, get_one

Check warning on line 14 in api/admin/controller/catalog_services.py

View check run for this annotation

Codecov / codecov/patch

api/admin/controller/catalog_services.py#L14

Added line #L14 was not covered by tests
from core.util.problem_detail import ProblemDetail


Expand Down Expand Up @@ -85,10 +83,6 @@ def process_post(self):
if isinstance(result, ProblemDetail):
return result

external_integration_link = self._set_external_integration_link(service)
if isinstance(external_integration_link, ProblemDetail):
return external_integration_link

library_error = self.check_libraries(service)
if library_error:
self._db.rollback()
Expand All @@ -99,39 +93,6 @@ def process_post(self):
else:
return Response(str(service.id), 200)

def _set_external_integration_link(self, service):
"""Either set or delete the external integration link between the
service and the storage integration.
"""
mirror_integration_id = flask.request.form.get("mirror_integration_id")

# If no storage integration was selected, then delete the existing
# external integration link.
current_integration_link, ignore = get_one_or_create(
self._db,
ExternalIntegrationLink,
library_id=None,
external_integration_id=service.id,
purpose=ExternalIntegrationLink.MARC,
)

if mirror_integration_id == self.NO_MIRROR_INTEGRATION:
if current_integration_link:
self._db.delete(current_integration_link)
else:
storage_integration = get_one(
self._db, ExternalIntegration, id=mirror_integration_id
)
# Only get storage integrations that have a MARC file option set
if (
not storage_integration
or not storage_integration.setting(
S3UploaderConfiguration.MARC_BUCKET_KEY
).value
):
return MISSING_INTEGRATION
current_integration_link.other_integration_id = storage_integration.id

def validate_form_fields(self, protocol):
"""Verify that the protocol which the user has selected is in the list
of recognized protocol options."""
Expand Down
2 changes: 1 addition & 1 deletion api/controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ def load_settings(self):
interface.
"""
LogConfiguration.initialize(self._db)
self.analytics = Analytics(self._db, refresh=True, services=self.services)
self.analytics = Analytics(self._db, refresh=True)

with elapsed_time_logging(
log_method=self.log.debug,
Expand Down
2 changes: 1 addition & 1 deletion api/discovery/registration_script.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def do_run(
# Set up an application context so we have access to url_for.
from api.app import app

app.manager = manager or CirculationManager(self._db)
app.manager = manager or CirculationManager(self._db, self.services)
base_url = ConfigurationSetting.sitewide(
self._db, Configuration.BASE_URL_KEY
).value
Expand Down
6 changes: 4 additions & 2 deletions core/scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def parse_time(cls, time_string):
continue
raise ValueError("Could not parse time: %s" % time_string)

def __init__(self, _db=None, *args, **kwargs):
def __init__(self, _db=None, services: Optional[Services] = None, *args, **kwargs):
"""Basic constructor.
:_db: A database session to be used instead of
Expand All @@ -124,7 +124,9 @@ def __init__(self, _db=None, *args, **kwargs):
if _db:
self._session = _db

self._services = create_container()
if services is None:
services = create_container()
self._services = services

def run(self):
DataSource.well_known_sources(self._db)
Expand Down
3 changes: 3 additions & 0 deletions core/service/storage/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@

class Storage(DeclarativeContainer):

# See https://python-dependency-injector.ets-labs.org/wiring.html
# This lists modules that contain markers that will be used to wire up
# dependencies from this container automatically.
wiring_config = WiringConfiguration(
modules=[
"api.s3_analytics_provider",
Expand Down
2 changes: 1 addition & 1 deletion scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ def process_lane(self, lane, exporter=None):
return

# Find the storage service
storage_service = self.container.storage.public()
storage_service = self.services.storage.public()
if not storage_service:
self.log.info("No storage service was found.")
return
Expand Down
97 changes: 0 additions & 97 deletions tests/api/admin/controller/test_catalog_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from api.admin.problem_details import (
CANNOT_CHANGE_PROTOCOL,
INTEGRATION_NAME_ALREADY_IN_USE,
MISSING_INTEGRATION,
MISSING_SERVICE,
MULTIPLE_SERVICES_FOR_LIBRARY,
UNKNOWN_PROTOCOL,
Expand All @@ -21,7 +20,6 @@
create,
get_one,
)
from core.model.configuration import ExternalIntegrationLink
from tests.fixtures.api_admin import SettingsControllerFixture


Expand Down Expand Up @@ -158,63 +156,8 @@ def test_catalog_services_post_errors(
goal=ExternalIntegration.CATALOG_GOAL,
)

# Attempt to set an S3 mirror external integration but it does not exist!
with settings_ctrl_fixture.request_context_with_admin("/", method="POST"):
ME = MARCExporter
flask.request.form = ImmutableMultiDict(
[
("name", "exporter name"),
("id", str(service.id)),
("protocol", ME.NAME),
("mirror_integration_id", "1234"),
]
)
response = (
settings_ctrl_fixture.manager.admin_catalog_services_controller.process_catalog_services()
)
assert response.uri == MISSING_INTEGRATION.uri

s3, ignore = create(
settings_ctrl_fixture.ctrl.db.session,
ExternalIntegration,
protocol=ExternalIntegration.S3,
goal=ExternalIntegration.STORAGE_GOAL,
)

# Now an S3 integration exists, but it has no MARC bucket configured.
with settings_ctrl_fixture.request_context_with_admin("/", method="POST"):
ME = MARCExporter
flask.request.form = ImmutableMultiDict(
[
("name", "exporter name"),
("id", str(service.id)),
("protocol", ME.NAME),
("mirror_integration_id", str(s3.id)),
]
)
response = (
settings_ctrl_fixture.manager.admin_catalog_services_controller.process_catalog_services()
)
assert response.uri == MISSING_INTEGRATION.uri

settings_ctrl_fixture.admin.remove_role(AdminRole.SYSTEM_ADMIN)
settings_ctrl_fixture.ctrl.db.session.flush()
with settings_ctrl_fixture.request_context_with_admin("/", method="POST"):
flask.request.form = ImmutableMultiDict(
[
("name", "new name"),
("protocol", ME.NAME),
("mirror_integration_id", str(s3.id)),
]
)
pytest.raises(
AdminNotAuthorized,
settings_ctrl_fixture.manager.admin_catalog_services_controller.process_catalog_services,
)

# This should be the last test to check since rolling back database
# changes in the test can cause it to crash.
s3.setting(S3UploaderConfiguration.MARC_BUCKET_KEY).value = "marc-files"
service.libraries += [settings_ctrl_fixture.ctrl.db.default_library()]
settings_ctrl_fixture.admin.add_role(AdminRole.SYSTEM_ADMIN)

Expand All @@ -224,7 +167,6 @@ def test_catalog_services_post_errors(
[
("name", "new name"),
("protocol", ME.NAME),
("mirror_integration_id", str(s3.id)),
(
"libraries",
json.dumps(
Expand All @@ -249,20 +191,11 @@ def test_catalog_services_post_create(
):
ME = MARCExporter

s3, ignore = create(
settings_ctrl_fixture.ctrl.db.session,
ExternalIntegration,
protocol=ExternalIntegration.S3,
goal=ExternalIntegration.STORAGE_GOAL,
)
s3.setting(S3UploaderConfiguration.MARC_BUCKET_KEY).value = "marc-files"

with settings_ctrl_fixture.request_context_with_admin("/", method="POST"):
flask.request.form = ImmutableMultiDict(
[
("name", "exporter name"),
("protocol", ME.NAME),
("mirror_integration_id", str(s3.id)),
(
"libraries",
json.dumps(
Expand All @@ -288,24 +221,11 @@ def test_catalog_services_post_create(
goal=ExternalIntegration.CATALOG_GOAL,
)
assert isinstance(service, ExternalIntegration)
# There was one S3 integration and it was selected. The service has an
# External Integration Link to the storage integration that is created
# in a POST with purpose of ExternalIntegrationLink.MARC.
integration_link = get_one(
settings_ctrl_fixture.ctrl.db.session,
ExternalIntegrationLink,
external_integration_id=service.id,
purpose=ExternalIntegrationLink.MARC,
)
assert isinstance(integration_link, ExternalIntegrationLink)

assert service.id == int(response.get_data())
assert ME.NAME == service.protocol
assert "exporter name" == service.name
assert [settings_ctrl_fixture.ctrl.db.default_library()] == service.libraries
# We expect the Catalog external integration to have a link to the
# S3 storage external integration
assert s3.id == integration_link.other_integration_id
assert (
"false"
== ConfigurationSetting.for_library_and_externalintegration(
Expand All @@ -330,14 +250,6 @@ def test_catalog_services_post_edit(
):
ME = MARCExporter

s3, ignore = create(
settings_ctrl_fixture.ctrl.db.session,
ExternalIntegration,
protocol=ExternalIntegration.S3,
goal=ExternalIntegration.STORAGE_GOAL,
)
s3.setting(S3UploaderConfiguration.MARC_BUCKET_KEY).value = "marc-files"

service, ignore = create(
settings_ctrl_fixture.ctrl.db.session,
ExternalIntegration,
Expand All @@ -352,7 +264,6 @@ def test_catalog_services_post_edit(
("name", "exporter name"),
("id", str(service.id)),
("protocol", ME.NAME),
("mirror_integration_id", str(s3.id)),
(
"libraries",
json.dumps(
Expand All @@ -372,17 +283,9 @@ def test_catalog_services_post_edit(
)
assert response.status_code == 200

integration_link = get_one(
settings_ctrl_fixture.ctrl.db.session,
ExternalIntegrationLink,
external_integration_id=service.id,
purpose=ExternalIntegrationLink.MARC,
)
assert isinstance(integration_link, ExternalIntegrationLink)
assert service.id == int(response.get_data())
assert ME.NAME == service.protocol
assert "exporter name" == service.name
assert s3.id == integration_link.other_integration_id
assert [settings_ctrl_fixture.ctrl.db.default_library()] == service.libraries
assert (
"false"
Expand Down
10 changes: 10 additions & 0 deletions tests/api/mockapi/circulation.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
import logging
from abc import ABC
from collections import defaultdict
from typing import Optional
from unittest.mock import MagicMock

from sqlalchemy.orm import Session

from api.circulation import BaseCirculationAPI, CirculationAPI, HoldInfo, LoanInfo
from api.controller import CirculationManager
from core.external_search import MockExternalSearchIndex
from core.integration.settings import BaseSettings
from core.model import DataSource, Hold, Loan
from core.service.container import Services


class MockBaseCirculationAPI(BaseCirculationAPI, ABC):
Expand Down Expand Up @@ -165,6 +170,11 @@ def api_for_license_pool(self, licensepool):
class MockCirculationManager(CirculationManager):
d_circulation: MockCirculationAPI

def __init__(self, db: Session, services: Optional[Services] = None):
if services is None:
services = MagicMock(spec=Services)
super().__init__(db, services)

def setup_search(self):
"""Set up a search client."""
return MockExternalSearchIndex()
Expand Down
2 changes: 1 addition & 1 deletion tests/api/test_controller_cm.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ class BadSearch(CirculationManager):
def setup_search(self):
raise Exception("doomed!")

circulation = BadSearch(circulation_fixture.db.session)
circulation = BadSearch(circulation_fixture.db.session, MagicMock())

# We didn't get a search object.
assert None == circulation.external_search
Expand Down
2 changes: 1 addition & 1 deletion tests/api/test_controller_opdsfeed.py
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ class BadSearch(CirculationManager):
def setup_search(self):
raise Exception("doomed!")

circulation = BadSearch(circulation_fixture.db.session)
circulation = BadSearch(circulation_fixture.db.session, MagicMock())

# An attempt to call FeedController.search() will return a
# problem detail.
Expand Down
4 changes: 2 additions & 2 deletions tests/api/test_opds2.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import io
import json
from unittest.mock import patch
from unittest.mock import MagicMock, patch
from urllib.parse import parse_qs, quote, urlparse

import pytest
Expand Down Expand Up @@ -312,7 +312,7 @@ def test_opds2_with_authentication_tokens(
work = works[0]
identifier = work.presentation_edition.primary_identifier

manager = CirculationManager(controller_fixture.db.session)
manager = CirculationManager(controller_fixture.db.session, MagicMock())
patron = controller_fixture.db.patron()

# Borrow the book from the library
Expand Down
Loading

0 comments on commit 5199fef

Please sign in to comment.