From 219be5476cbdf804d9dec81b08eea1e177d3aad2 Mon Sep 17 00:00:00 2001 From: Jonathan Green Date: Tue, 26 Nov 2024 16:31:23 -0400 Subject: [PATCH] Update database constraints --- ...11_update_database_nullable_constraints.py | 96 +++++++++++++++++++ .../controller/individual_admin_settings.py | 5 +- .../admin/controller/integration_settings.py | 9 -- .../admin/controller/patron_auth_services.py | 7 +- .../manager/api/admin/dashboard_stats.py | 2 +- .../password_admin_authentication_provider.py | 12 +-- .../api/authentication/access_token.py | 4 +- .../manager/api/authentication/basic_token.py | 4 +- src/palace/manager/api/authenticator.py | 20 ++-- src/palace/manager/api/circulation.py | 4 - src/palace/manager/api/enki.py | 2 - src/palace/manager/api/odl/api.py | 30 ++++-- .../manager/api/opds_for_distributors.py | 2 + .../generate_inventory_and_hold_reports.py | 3 - src/palace/manager/core/opds_import.py | 7 +- src/palace/manager/core/selftest.py | 16 +--- src/palace/manager/feed/acquisition.py | 21 ++-- src/palace/manager/feed/annotator/base.py | 4 +- .../manager/feed/annotator/circulation.py | 5 +- src/palace/manager/marc/annotator.py | 21 ++-- src/palace/manager/marc/exporter.py | 5 - src/palace/manager/search/external_search.py | 3 - src/palace/manager/sqlalchemy/model/admin.py | 16 ++-- .../manager/sqlalchemy/model/announcements.py | 15 +-- .../sqlalchemy/model/circulationevent.py | 2 +- .../sqlalchemy/model/classification.py | 12 ++- .../manager/sqlalchemy/model/collection.py | 6 +- .../manager/sqlalchemy/model/contributor.py | 10 +- .../manager/sqlalchemy/model/coverage.py | 25 ++--- .../manager/sqlalchemy/model/credential.py | 8 +- .../manager/sqlalchemy/model/customlist.py | 16 ++-- .../manager/sqlalchemy/model/datasource.py | 2 +- .../manager/sqlalchemy/model/devicetokens.py | 14 +-- .../model/discovery_service_registration.py | 8 +- .../manager/sqlalchemy/model/edition.py | 12 ++- .../manager/sqlalchemy/model/identifier.py | 24 +++-- .../manager/sqlalchemy/model/integration.py | 18 ++-- src/palace/manager/sqlalchemy/model/key.py | 12 ++- src/palace/manager/sqlalchemy/model/lane.py | 38 +++++--- .../manager/sqlalchemy/model/library.py | 16 ++-- .../manager/sqlalchemy/model/licensing.py | 43 +++++---- .../manager/sqlalchemy/model/marcfile.py | 19 ++-- .../manager/sqlalchemy/model/measurement.py | 6 +- src/palace/manager/sqlalchemy/model/patron.py | 52 +++++----- .../manager/sqlalchemy/model/resource.py | 24 ++--- src/palace/manager/sqlalchemy/model/saml.py | 16 ++-- .../manager/sqlalchemy/model/time_tracking.py | 38 ++++---- src/palace/manager/sqlalchemy/model/work.py | 15 ++- src/palace/manager/util/notifications.py | 10 +- tests/fixtures/database.py | 2 +- .../api/admin/controller/test_custom_lists.py | 2 + ..._password_admin_authentication_provider.py | 20 +--- .../api/controller/test_playtime_entries.py | 2 +- tests/manager/api/controller/test_work.py | 8 +- tests/manager/api/test_circulationapi.py | 49 +--------- tests/manager/celery/tasks/test_opds_odl.py | 5 +- tests/manager/core/test_coverage.py | 1 + tests/manager/feed/test_library_annotator.py | 22 ----- .../feed/test_loan_and_hold_annotator.py | 9 -- .../feed/test_opds_acquisition_feed.py | 6 +- .../manager/scripts/test_playtime_entries.py | 2 +- tests/manager/search/test_external_search.py | 5 - .../sqlalchemy/model/test_customlist.py | 1 + tests/manager/sqlalchemy/model/test_patron.py | 8 +- tests/manager/sqlalchemy/model/test_work.py | 34 +++---- 65 files changed, 469 insertions(+), 466 deletions(-) create mode 100644 alembic/versions/20241127_b1b08faa9811_update_database_nullable_constraints.py diff --git a/alembic/versions/20241127_b1b08faa9811_update_database_nullable_constraints.py b/alembic/versions/20241127_b1b08faa9811_update_database_nullable_constraints.py new file mode 100644 index 0000000000..72a3559386 --- /dev/null +++ b/alembic/versions/20241127_b1b08faa9811_update_database_nullable_constraints.py @@ -0,0 +1,96 @@ +"""Update database nullable constraints + +Revision ID: b1b08faa9811 +Revises: 272da5f400de +Create Date: 2024-11-27 17:05:00.839954+00:00 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "b1b08faa9811" +down_revision = "272da5f400de" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column( + "editions", "data_source_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "editions", "primary_identifier_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "equivalents", "input_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "equivalentscoveragerecords", + "equivalency_id", + existing_type=sa.INTEGER(), + nullable=False, + ) + op.alter_column("holds", "patron_id", existing_type=sa.INTEGER(), nullable=False) + op.alter_column( + "holds", "license_pool_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "licensepools", "data_source_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "licensepools", "identifier_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "licenses", "license_pool_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column("loans", "patron_id", existing_type=sa.INTEGER(), nullable=False) + op.alter_column( + "loans", "license_pool_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column( + "workgenres", "genre_id", existing_type=sa.INTEGER(), nullable=False + ) + op.alter_column("workgenres", "work_id", existing_type=sa.INTEGER(), nullable=False) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column("workgenres", "work_id", existing_type=sa.INTEGER(), nullable=True) + op.alter_column("workgenres", "genre_id", existing_type=sa.INTEGER(), nullable=True) + op.alter_column( + "loans", "license_pool_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column("loans", "patron_id", existing_type=sa.INTEGER(), nullable=True) + op.alter_column( + "licenses", "license_pool_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column( + "licensepools", "identifier_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column( + "licensepools", "data_source_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column( + "holds", "license_pool_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column("holds", "patron_id", existing_type=sa.INTEGER(), nullable=True) + op.alter_column( + "equivalentscoveragerecords", + "equivalency_id", + existing_type=sa.INTEGER(), + nullable=True, + ) + op.alter_column( + "equivalents", "input_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column( + "editions", "primary_identifier_id", existing_type=sa.INTEGER(), nullable=True + ) + op.alter_column( + "editions", "data_source_id", existing_type=sa.INTEGER(), nullable=True + ) + # ### end Alembic commands ### diff --git a/src/palace/manager/api/admin/controller/individual_admin_settings.py b/src/palace/manager/api/admin/controller/individual_admin_settings.py index 787e1dd026..5aa19ffb5d 100644 --- a/src/palace/manager/api/admin/controller/individual_admin_settings.py +++ b/src/palace/manager/api/admin/controller/individual_admin_settings.py @@ -418,7 +418,10 @@ def handle_password(self, password, admin: Admin, is_new, settingUp): # libraries then they should not be able to change the password for role in admin.roles: if not user.is_library_manager(role.library): - message = f"User is part of '{role.library.name}', you are not authorized to change their password." + library_name = ( + role.library.name if role.library else "unknown" + ) + message = f"User is part of '{library_name}', you are not authorized to change their password." break else: can_change_pw = True diff --git a/src/palace/manager/api/admin/controller/integration_settings.py b/src/palace/manager/api/admin/controller/integration_settings.py index 0ed2b28c74..6cd9e51e86 100644 --- a/src/palace/manager/api/admin/controller/integration_settings.py +++ b/src/palace/manager/api/admin/controller/integration_settings.py @@ -98,15 +98,6 @@ def configured_service_info( """This is the default implementation for getting details about a configured integration. It can be overridden by implementations that need to add additional information to the service info dict that gets returned to the admin UI.""" - - if service.goal is None: - # We should never get here, since we only query for services with a goal, and goal - # is a required field, but for mypy and safety, we check for it anyway. - self.log.warning( - f"IntegrationConfiguration {service.name}({service.id}) has no goal set. Skipping." - ) - return None - if service.protocol is None or service.protocol not in self.registry: self.log.warning( f"Unknown protocol: {service.protocol} for goal {self.registry.goal}" diff --git a/src/palace/manager/api/admin/controller/patron_auth_services.py b/src/palace/manager/api/admin/controller/patron_auth_services.py index f855a9199c..fc40ab47c6 100644 --- a/src/palace/manager/api/admin/controller/patron_auth_services.py +++ b/src/palace/manager/api/admin/controller/patron_auth_services.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, cast +from typing import Any import flask from flask import Response @@ -193,7 +193,4 @@ def get_library_configuration( if not (library_configurations := integration.library_configurations): return None # We sort by library id to ensure that the result is predictable. - # We cast the library id to `int`, since mypy doesn't understand the relationship. - return sorted( - library_configurations, key=lambda config: cast(int, config.library_id) - )[0] + return sorted(library_configurations, key=lambda config: config.library_id)[0] diff --git a/src/palace/manager/api/admin/dashboard_stats.py b/src/palace/manager/api/admin/dashboard_stats.py index 575b767faa..222191ebc6 100644 --- a/src/palace/manager/api/admin/dashboard_stats.py +++ b/src/palace/manager/api/admin/dashboard_stats.py @@ -122,7 +122,7 @@ def _authorized_collections( self, admin: Admin, authorized_libraries: list[Library], - ) -> tuple[list[_Collection], dict[str | None, list[_Collection]], bool]: + ) -> tuple[list[_Collection], dict[str, list[_Collection]], bool]: authorized_collections: set[_Collection] = set() authorized_collections_by_library = {} diff --git a/src/palace/manager/api/admin/password_admin_authentication_provider.py b/src/palace/manager/api/admin/password_admin_authentication_provider.py index f4e3d81c22..ca9f96b206 100644 --- a/src/palace/manager/api/admin/password_admin_authentication_provider.py +++ b/src/palace/manager/api/admin/password_admin_authentication_provider.py @@ -45,10 +45,7 @@ def __init__(self, send_email: SendEmailCallable): @staticmethod def get_secret_key(db: Session) -> str: - key = Key.get_key(db, KeyType.ADMIN_SECRET_KEY, raise_exception=True).value - # We know .value is a str because its a non-null column in the DB, so - # we use an ignore to tell mypy to trust us. - return key # type: ignore[return-value] + return Key.get_key(db, KeyType.ADMIN_SECRET_KEY, raise_exception=True).value def sign_in_template(self, redirect): password_sign_in_url = url_for("password_auth") @@ -108,13 +105,6 @@ def generate_reset_password_token(self, admin: Admin, _db: Session) -> str: def send_reset_password_email(self, admin: Admin, reset_password_url: str) -> None: subject = f"{AdminClientConfig.APP_NAME} - Reset password email" - if admin.email is None: - # This should never happen, but if it does, we should log it. - self.log.error( - "Admin has no email address, cannot send reset password email." - ) - return - receivers = [admin.email] mail_text = render_template_string( diff --git a/src/palace/manager/api/authentication/access_token.py b/src/palace/manager/api/authentication/access_token.py index 83a957d2ce..d6156c3590 100644 --- a/src/palace/manager/api/authentication/access_token.py +++ b/src/palace/manager/api/authentication/access_token.py @@ -3,7 +3,7 @@ import uuid from dataclasses import dataclass from datetime import timedelta -from typing import TYPE_CHECKING, cast +from typing import TYPE_CHECKING from jwcrypto import jwe, jwk @@ -80,7 +80,7 @@ def generate_token( plaintext=jwe.json_encode(dict(id=patron.id, pwd=password)), protected=dict( alg="dir", - kid=uuid_encode(cast(uuid.UUID, key.id)), + kid=uuid_encode(key.id), typ="JWE", enc="A128CBC-HS256", cty=cls.CTY, diff --git a/src/palace/manager/api/authentication/basic_token.py b/src/palace/manager/api/authentication/basic_token.py index 5106bbb018..61efa9e8cc 100644 --- a/src/palace/manager/api/authentication/basic_token.py +++ b/src/palace/manager/api/authentication/basic_token.py @@ -1,7 +1,7 @@ from __future__ import annotations from collections.abc import Generator -from typing import TYPE_CHECKING, cast +from typing import TYPE_CHECKING from flask import url_for from sqlalchemy.orm import Session @@ -48,7 +48,7 @@ def __init__( basic_provider: BasicAuthenticationProvider, ): self._db = _db - self.library_id = cast(int, library.id) + self.library_id = library.id # An access token provider is a companion authentication to the basic providers self.basic_provider = basic_provider diff --git a/src/palace/manager/api/authenticator.py b/src/palace/manager/api/authenticator.py index 6737a4d38d..ec7547ef2b 100644 --- a/src/palace/manager/api/authenticator.py +++ b/src/palace/manager/api/authenticator.py @@ -6,7 +6,6 @@ import sys from abc import ABC from collections.abc import Iterable -from typing import cast import flask import jwt @@ -139,11 +138,6 @@ def populate_authenticators( log_method=self.log.debug, message_prefix="populate_authenticators" ): for library in libraries: - if library.short_name is None: - self.log.error( - f"Library {library.name} ({library.id}) has no short name." - ) - continue self.library_authenticators[library.short_name] = ( LibraryAuthenticator.from_config(_db, library, analytics) ) @@ -262,9 +256,11 @@ def __init__( ) self.saml_providers_by_name = {} - self.bearer_token_signing_secret = bearer_token_signing_secret or cast( - str, - Key.get_key(_db, KeyType.BEARER_TOKEN_SIGNING, raise_exception=True).value, + self.bearer_token_signing_secret = ( + bearer_token_signing_secret + or Key.get_key( + _db, KeyType.BEARER_TOKEN_SIGNING, raise_exception=True + ).value ) self.initialization_exceptions: dict[ tuple[int | None, int | None], Exception @@ -307,8 +303,6 @@ def identifies_individuals(self) -> bool: @property def library(self) -> Library | None: - if self.library_id is None: - return None return Library.by_id(self._db, self.library_id) def register_provider( @@ -350,8 +344,8 @@ def register_provider( settings = impl_cls.settings_load(integration.parent) library_settings = impl_cls.library_settings_load(integration) provider = impl_cls( - self.library_id, # type: ignore[arg-type] - integration.parent_id, # type: ignore[arg-type] + self.library_id, + integration.parent_id, settings, library_settings, analytics, diff --git a/src/palace/manager/api/circulation.py b/src/palace/manager/api/circulation.py index bd1a8a07a5..11d020d493 100644 --- a/src/palace/manager/api/circulation.py +++ b/src/palace/manager/api/circulation.py @@ -603,8 +603,6 @@ def __init__(self, _db: Session, collection: Collection): @property def collection(self) -> Collection | None: - if self.collection_id is None: - return None return Collection.by_id(self._db, id=self.collection_id) @classmethod @@ -889,8 +887,6 @@ def __init__( @property def library(self) -> Library | None: - if self.library_id is None: - return None return Library.by_id(self._db, self.library_id) def api_for_license_pool( diff --git a/src/palace/manager/api/enki.py b/src/palace/manager/api/enki.py index 2c86861ac5..c87cf6534c 100644 --- a/src/palace/manager/api/enki.py +++ b/src/palace/manager/api/enki.py @@ -165,8 +165,6 @@ def _url(self, endpoint: str) -> str: def enki_library_id(self, library: Library) -> str | None: """Find the Enki library ID for the given library.""" - if library.id is None: - return None settings = self.library_settings(library.id) if settings is None: return None diff --git a/src/palace/manager/api/odl/api.py b/src/palace/manager/api/odl/api.py index 0b0e31b605..5556484f64 100644 --- a/src/palace/manager/api/odl/api.py +++ b/src/palace/manager/api/odl/api.py @@ -64,6 +64,7 @@ LicensePoolDeliveryMechanism, ) from palace.manager.sqlalchemy.model.patron import Hold, Loan, Patron +from palace.manager.sqlalchemy.model.resource import Resource from palace.manager.sqlalchemy.util import get_one from palace.manager.util import base64 from palace.manager.util.datetime_helpers import utc_now @@ -239,6 +240,11 @@ def _checkin(self, loan: Loan) -> None: # bookshelf forever. self.log.error(f"Loan {loan.id} has no external identifier.") return + if loan.license is None: + # We can't return a loan that doesn't have a license. This should never happen but if it does, + # we log an error and continue on, so it doesn't stay on the patrons bookshelf forever. + self.log.error(f"Loan {loan.id} has no license.") + return doc = self._request_loan_status("GET", loan.external_identifier) if not doc.active: @@ -496,29 +502,33 @@ def fulfill( return self._fulfill(loan, delivery_mechanism) @staticmethod - def _check_delivery_mechanism_available( + def _get_resource_for_delivery_mechanism( requested_delivery_mechanism: DeliveryMechanism, licensepool: LicensePool - ) -> None: - fulfillment = next( + ) -> Resource: + resource = next( ( - lpdm + lpdm.resource for lpdm in licensepool.delivery_mechanisms if lpdm.delivery_mechanism == requested_delivery_mechanism + and lpdm.resource is not None ), None, ) - if fulfillment is None: + if resource is None: raise FormatNotAvailable() + return resource def _unlimited_access_fulfill( self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism ) -> Fulfillment: licensepool = loan.license_pool - self._check_delivery_mechanism_available( + resource = self._get_resource_for_delivery_mechanism( delivery_mechanism.delivery_mechanism, licensepool ) - content_link = delivery_mechanism.resource.representation.public_url - content_type = delivery_mechanism.resource.representation.media_type + if resource.representation is None: + raise FormatNotAvailable() + content_link = resource.representation.public_url + content_type = resource.representation.media_type return RedirectFulfillment(content_link, content_type) def _license_fulfill( @@ -571,7 +581,7 @@ def _bearer_token_fulfill( self, loan: Loan, delivery_mechanism: LicensePoolDeliveryMechanism ) -> Fulfillment: licensepool = loan.license_pool - self._check_delivery_mechanism_available( + resource = self._get_resource_for_delivery_mechanism( delivery_mechanism.delivery_mechanism, licensepool ) @@ -592,7 +602,7 @@ def _bearer_token_fulfill( token_type="Bearer", access_token=self._session_token.token, expires_in=(int((self._session_token.expires - utc_now()).total_seconds())), - location=delivery_mechanism.resource.url, + location=resource.url, ) return DirectFulfillment( diff --git a/src/palace/manager/api/opds_for_distributors.py b/src/palace/manager/api/opds_for_distributors.py index a177ff3c2b..1d30b3abb5 100644 --- a/src/palace/manager/api/opds_for_distributors.py +++ b/src/palace/manager/api/opds_for_distributors.py @@ -306,6 +306,8 @@ def fulfill( # Find the acquisition link with the right media type. url = None for link in links: + if link.resource.representation is None: + continue media_type = link.resource.representation.media_type if ( link.rel == Hyperlink.GENERIC_OPDS_ACQUISITION diff --git a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py index 8ad0495519..d53731e861 100644 --- a/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py +++ b/src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py @@ -34,9 +34,6 @@ def eligible_integrations( """Subset a list of integrations to only those that are eligible for the inventory report.""" def is_eligible(integration: IntegrationConfiguration) -> bool: - if integration.protocol is None: - return False - settings_cls = registry[integration.protocol].settings_class() return issubclass(settings_cls, OPDSImporterSettings) diff --git a/src/palace/manager/core/opds_import.py b/src/palace/manager/core/opds_import.py index 88e70bf4d3..665456778a 100644 --- a/src/palace/manager/core/opds_import.py +++ b/src/palace/manager/core/opds_import.py @@ -300,7 +300,7 @@ def fulfill( delivery_mechanism: LicensePoolDeliveryMechanism, ) -> RedirectFulfillment: requested_mechanism = delivery_mechanism.delivery_mechanism - fulfillment = None + rep = None for lpdm in licensepool.delivery_mechanisms: if ( lpdm.resource is None @@ -313,15 +313,14 @@ def fulfill( if lpdm.delivery_mechanism == requested_mechanism: # We found it! This is how the patron wants # the book to be delivered. - fulfillment = lpdm + rep = lpdm.resource.representation break - if not fulfillment: + if not rep: # There is just no way to fulfill this loan the way the # patron wants. raise FormatNotAvailable() - rep = fulfillment.resource.representation content_link = rep.public_url media_type = rep.media_type diff --git a/src/palace/manager/core/selftest.py b/src/palace/manager/core/selftest.py index 69ce87f266..3796b20907 100644 --- a/src/palace/manager/core/selftest.py +++ b/src/palace/manager/core/selftest.py @@ -279,20 +279,8 @@ def store_self_test_results( @classmethod def load_self_test_results( - cls, integration: IntegrationConfiguration | None - ) -> dict[str, Any] | str | None: - if integration is None: - cls.logger().error( - "No IntegrationConfiguration was found. Self-test results could not be loaded." - ) - return None - - if not isinstance(integration.self_test_results, dict): - cls.logger().error( - "Self-test results were not stored as a dict. Self-test results could not be loaded." - ) - return None - + cls, integration: IntegrationConfiguration + ) -> dict[str, Any] | str: if integration.self_test_results == {}: # No self-test results have been stored yet. return "No results yet" diff --git a/src/palace/manager/feed/acquisition.py b/src/palace/manager/feed/acquisition.py index 9dae74cb77..aa85a6db22 100644 --- a/src/palace/manager/feed/acquisition.py +++ b/src/palace/manager/feed/acquisition.py @@ -564,14 +564,11 @@ def single_entry_loans_feed( # Sometimes the pool or work may be None # In those cases we have to protect against the exceptions - try: - work = license_pool.work or license_pool.presentation_edition.work - except AttributeError as ex: - cls.logger().error(f"Error retrieving a Work Object {ex}") - cls.logger().error( - f"Error Data: {license_pool} | {license_pool and license_pool.presentation_edition}" - ) - return NOT_FOUND_ON_REMOTE + work: Work | None = None + if license_pool.work: + work = license_pool.work + elif license_pool.presentation_edition: + work = license_pool.presentation_edition.work if not work: return NOT_FOUND_ON_REMOTE @@ -615,11 +612,17 @@ def single_entry( """Turn a work into an annotated work entry for an acquisition feed.""" identifier = None _work: Work + active_edition: Edition | None if isinstance(work, Edition): active_edition = work identifier = active_edition.primary_identifier active_license_pool = None - _work = active_edition.work # We always need a work for an entry + if active_edition.work is None: + # We always need a work for an entry + raise PalaceValueError( + f"Edition has no associated work: {active_edition!r}" + ) + _work = active_edition.work else: _work = work active_license_pool = annotator.active_licensepool_for(work) diff --git a/src/palace/manager/feed/annotator/base.py b/src/palace/manager/feed/annotator/base.py index 8eb0ea9f2e..4d6638b09e 100644 --- a/src/palace/manager/feed/annotator/base.py +++ b/src/palace/manager/feed/annotator/base.py @@ -293,11 +293,13 @@ def annotate_work_entry( samples = self.samples(edition) for sample in samples: + representation = sample.resource.representation + media_type = representation.media_type if representation else None other_links.append( Link( rel=Hyperlink.CLIENT_SAMPLE, href=sample.resource.url, - type=sample.resource.representation.media_type, + type=media_type, ) ) diff --git a/src/palace/manager/feed/annotator/circulation.py b/src/palace/manager/feed/annotator/circulation.py index 59b4123f05..c46a1a9027 100644 --- a/src/palace/manager/feed/annotator/circulation.py +++ b/src/palace/manager/feed/annotator/circulation.py @@ -626,9 +626,10 @@ def open_access_link( # LicensePoolDeliveryMechanism's Resource is the URL we should # send for download purposes. This will be the case unless we # previously mirrored that URL somewhere else. - href = lpdm.resource.url + resource = lpdm.resource + href = resource.url if resource else None - rep = lpdm.resource.representation + rep = resource.representation if resource else None if rep: if rep.media_type: kw["type"] = rep.media_type diff --git a/src/palace/manager/marc/annotator.py b/src/palace/manager/marc/annotator.py index ea1be3b8cd..0b7da03afc 100644 --- a/src/palace/manager/marc/annotator.py +++ b/src/palace/manager/marc/annotator.py @@ -57,12 +57,13 @@ def marc_record( # though they have different titles. We do not group editions of the same work in # different languages, so we can't use those yet. - cls.add_title(record, edition) - cls.add_contributors(record, edition) - cls.add_publisher(record, edition) - cls.add_physical_description(record, edition) + if edition is not None: + cls.add_title(record, edition) + cls.add_contributors(record, edition) + cls.add_publisher(record, edition) + cls.add_physical_description(record, edition) + cls.add_series(record, edition) cls.add_audience(record, work) - cls.add_series(record, edition) cls.add_system_details(record) cls.add_ebooks_subject(record) cls.add_distributor(record, license_pool) @@ -154,7 +155,11 @@ def leader(cls, revised: bool = False) -> str: @classmethod def add_control_fields( - cls, record: Record, identifier: Identifier, pool: LicensePool, edition: Edition + cls, + record: Record, + identifier: Identifier, + pool: LicensePool, + edition: Edition | None, ) -> None: # Unique identifier for this record. record.add_field(Field(tag="001", data=identifier.urn)) @@ -180,7 +185,7 @@ def add_control_fields( # Field 008 (fixed-length data elements): data = utc_now().strftime("%y%m%d") - publication_date = edition.issued or edition.published + publication_date = (edition.issued or edition.published) if edition else None if publication_date: date_type = "s" # single known date # Not using strftime because some years are pre-1900. @@ -195,7 +200,7 @@ def add_control_fields( data += "xxu" data += " " language = "eng" - if edition.language: + if edition and edition.language: language = LanguageCodes.string_to_alpha_3(edition.language) data += language data += " " diff --git a/src/palace/manager/marc/exporter.py b/src/palace/manager/marc/exporter.py index 3d6b798359..c948473fac 100644 --- a/src/palace/manager/marc/exporter.py +++ b/src/palace/manager/marc/exporter.py @@ -188,11 +188,6 @@ def enabled_libraries( library = library_integration.library library_id = library.id library_short_name = library.short_name - if library_id is None or library_short_name is None: - cls.logger().warning( - f"Library {library} is missing an ID or short name." - ) - continue last_updated_time = cls._last_updated(session, library, collection) update_frequency = cls.settings_load( library_integration.parent diff --git a/src/palace/manager/search/external_search.py b/src/palace/manager/search/external_search.py index 7c326350ff..13cc8f2cf4 100644 --- a/src/palace/manager/search/external_search.py +++ b/src/palace/manager/search/external_search.py @@ -214,9 +214,6 @@ def count_works(self, filter): def remove_work(self, work: Work | int) -> None: """Remove the search document for `work` from the search index.""" if isinstance(work, Work): - if work.id is None: - self.log.warning("Work has no ID, unable to remove. %r", work) - return work = work.id self._search_service.index_remove_document(doc_id=work) diff --git a/src/palace/manager/sqlalchemy/model/admin.py b/src/palace/manager/sqlalchemy/model/admin.py index 32aa4b4ec4..6080eee556 100644 --- a/src/palace/manager/sqlalchemy/model/admin.py +++ b/src/palace/manager/sqlalchemy/model/admin.py @@ -32,8 +32,8 @@ class Admin(Base, HasSessionCache): __tablename__ = "admins" - id = Column(Integer, primary_key=True) - email = Column(Unicode, unique=True, nullable=False) + id: Mapped[int] = Column(Integer, primary_key=True) + email: Mapped[str] = Column(Unicode, unique=True, nullable=False) # Admins can also log in with a local password. password_hashed = Column(Unicode, index=True) @@ -288,12 +288,16 @@ def __repr__(self): class AdminRole(Base, HasSessionCache): __tablename__ = "adminroles" - id = Column(Integer, primary_key=True) - admin_id = Column(Integer, ForeignKey("admins.id"), nullable=False, index=True) + id: Mapped[int] = Column(Integer, primary_key=True) + admin_id: Mapped[int] = Column( + Integer, ForeignKey("admins.id"), nullable=False, index=True + ) admin: Mapped[Admin] = relationship("Admin", back_populates="roles") library_id = Column(Integer, ForeignKey("libraries.id"), nullable=True, index=True) - library: Mapped[Library] = relationship("Library", back_populates="adminroles") - role = Column(Unicode, nullable=False, index=True) + library: Mapped[Library | None] = relationship( + "Library", back_populates="adminroles" + ) + role: Mapped[str] = Column(Unicode, nullable=False, index=True) __table_args__ = (UniqueConstraint("admin_id", "library_id", "role"),) diff --git a/src/palace/manager/sqlalchemy/model/announcements.py b/src/palace/manager/sqlalchemy/model/announcements.py index 9422bbf0ed..ab200cfe5b 100644 --- a/src/palace/manager/sqlalchemy/model/announcements.py +++ b/src/palace/manager/sqlalchemy/model/announcements.py @@ -25,10 +25,12 @@ class Announcement(Base): __tablename__ = "announcements" - id = Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - content = Column(Unicode, nullable=False) - start = Column(Date, nullable=False) - finish = Column(Date, nullable=False) + id: Mapped[uuid.UUID] = Column( + UUID(as_uuid=True), primary_key=True, default=uuid.uuid4 + ) + content: Mapped[str] = Column(Unicode, nullable=False) + start: Mapped[datetime.date] = Column(Date, nullable=False) + finish: Mapped[datetime.date] = Column(Date, nullable=False) # The Library associated with the announcement, announcements that should be shown to # all libraries will have a null library_id. @@ -38,8 +40,7 @@ class Announcement(Base): index=True, nullable=True, ) - - library: Mapped[Library] = relationship( + library: Mapped[Library | None] = relationship( "Library", back_populates="library_announcements" ) @@ -107,7 +108,7 @@ def sync( db.delete(existing_announcements[id]) for id in announcements_to_update: - existing_announcements[id].update(new[id]) # type: ignore[index] + existing_announcements[id].update(new[id]) for id in announcements_to_create: Announcement.from_data(db, new[id], library=library) diff --git a/src/palace/manager/sqlalchemy/model/circulationevent.py b/src/palace/manager/sqlalchemy/model/circulationevent.py index 763409dacc..91b9d5eed0 100644 --- a/src/palace/manager/sqlalchemy/model/circulationevent.py +++ b/src/palace/manager/sqlalchemy/model/circulationevent.py @@ -27,7 +27,7 @@ class CirculationEvent(Base): # Used to explicitly tag an event as happening at an unknown time. NO_DATE = object() - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # One LicensePool can have many circulation events. license_pool_id = Column(Integer, ForeignKey("licensepools.id"), index=True) diff --git a/src/palace/manager/sqlalchemy/model/classification.py b/src/palace/manager/sqlalchemy/model/classification.py index d43e3c63bb..e9d83b70b0 100644 --- a/src/palace/manager/sqlalchemy/model/classification.py +++ b/src/palace/manager/sqlalchemy/model/classification.py @@ -95,7 +95,7 @@ class Subject(Base): uri_lookup[v] = k __tablename__ = "subjects" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # Type should be one of the constants in this class. type = Column(Unicode, index=True) @@ -132,7 +132,7 @@ class Subject(Base): # Each Subject may claim affinity with one Genre. genre_id = Column(Integer, ForeignKey("genres.id"), index=True) - genre: Mapped[Genre] = relationship("Genre", back_populates="subjects") + genre: Mapped[Genre | None] = relationship("Genre", back_populates="subjects") # A locked Subject has been reviewed by a human and software will # not mess with it without permission. @@ -345,13 +345,15 @@ class Classification(Base): """The assignment of a Identifier to a Subject.""" __tablename__ = "classifications" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) identifier: Mapped[Identifier | None] = relationship( "Identifier", back_populates="classifications" ) subject_id = Column(Integer, ForeignKey("subjects.id"), index=True) - subject: Mapped[Subject] = relationship("Subject", back_populates="classifications") + subject: Mapped[Subject | None] = relationship( + "Subject", back_populates="classifications" + ) data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) data_source: Mapped[DataSource | None] = relationship( "DataSource", back_populates="classifications" @@ -482,7 +484,7 @@ class Genre(Base, HasSessionCache): """ __tablename__ = "genres" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) name = Column(Unicode, unique=True, index=True) # One Genre may have affinity with many Subjects. diff --git a/src/palace/manager/sqlalchemy/model/collection.py b/src/palace/manager/sqlalchemy/model/collection.py index ae0a350498..f9cf8d3844 100644 --- a/src/palace/manager/sqlalchemy/model/collection.py +++ b/src/palace/manager/sqlalchemy/model/collection.py @@ -49,7 +49,7 @@ class Collection(Base, HasSessionCache, RedisKeyMixin): """A Collection is a set of LicensePools obtained through some mechanism.""" __tablename__ = "collections" - id = Column(Integer, primary_key=True, nullable=False) + id: Mapped[int] = Column(Integer, primary_key=True, nullable=False) DATA_SOURCE_NAME_SETTING = "data_source" DATA_SOURCE_FOR_LICENSE_PROTOCOL = [ @@ -66,7 +66,7 @@ class Collection(Base, HasSessionCache, RedisKeyMixin): # designates the integration technique we will use to actually get # the metadata and licenses. Each Collection has a distinct # integration configuration. - integration_configuration_id = Column( + integration_configuration_id: Mapped[int] = Column( Integer, ForeignKey("integration_configurations.id"), unique=True, @@ -147,7 +147,7 @@ class Collection(Base, HasSessionCache, RedisKeyMixin): "CustomList", secondary="collections_customlists", back_populates="collections" ) - export_marc_records = Column(Boolean, default=False, nullable=False) + export_marc_records: Mapped[bool] = Column(Boolean, default=False, nullable=False) # Most data sources offer different catalogs to different # libraries. Data sources in this list offer the same catalog to diff --git a/src/palace/manager/sqlalchemy/model/contributor.py b/src/palace/manager/sqlalchemy/model/contributor.py index b125540939..d2592da82c 100644 --- a/src/palace/manager/sqlalchemy/model/contributor.py +++ b/src/palace/manager/sqlalchemy/model/contributor.py @@ -32,7 +32,7 @@ class Contributor(Base): """Someone (usually human) who contributes to books.""" __tablename__ = "contributors" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # Standard identifiers for this contributor. lc = Column(Unicode, index=True) @@ -483,12 +483,14 @@ class Contribution(Base): """A contribution made by a Contributor to a Edition.""" __tablename__ = "contributions" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) edition: Mapped[Edition] = relationship("Edition", back_populates="contributions") - edition_id = Column(Integer, ForeignKey("editions.id"), index=True, nullable=False) + edition_id: Mapped[int] = Column( + Integer, ForeignKey("editions.id"), index=True, nullable=False + ) - contributor_id = Column( + contributor_id: Mapped[int] = Column( Integer, ForeignKey("contributors.id"), index=True, nullable=False ) contributor: Mapped[Contributor] = relationship( diff --git a/src/palace/manager/sqlalchemy/model/coverage.py b/src/palace/manager/sqlalchemy/model/coverage.py index ddeeb2d1d6..4d4f911d9f 100644 --- a/src/palace/manager/sqlalchemy/model/coverage.py +++ b/src/palace/manager/sqlalchemy/model/coverage.py @@ -121,10 +121,10 @@ class Timestamp(Base): ) # Unique ID - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # Name of the service. - service = Column(String(255), index=True, nullable=False) + service: Mapped[str] = Column(String(255), index=True, nullable=False) # Type of the service -- monitor, coverage provider, or script. # If the service type does not fit into these categories, this field @@ -136,7 +136,7 @@ class Timestamp(Base): collection_id = Column( Integer, ForeignKey("collections.id"), index=True, nullable=True ) - collection: Mapped[Collection] = relationship( + collection: Mapped[Collection | None] = relationship( "Collection", back_populates="timestamps" ) @@ -324,16 +324,16 @@ class CoverageRecord(Base, BaseCoverageRecord): REPAIR_SORT_NAME_OPERATION = "repair-sort-name" METADATA_UPLOAD_OPERATION = "metadata-upload" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) - identifier: Mapped[Identifier] = relationship( + identifier: Mapped[Identifier | None] = relationship( "Identifier", back_populates="coverage_records" ) # If applicable, this is the ID of the data source that took the # Identifier as input. data_source_id = Column(Integer, ForeignKey("datasources.id")) - data_source: Mapped[DataSource] = relationship( + data_source: Mapped[DataSource | None] = relationship( "DataSource", back_populates="coverage_records" ) operation = Column(String(255), default=None) @@ -623,9 +623,9 @@ class WorkCoverageRecord(Base, BaseCoverageRecord): SUMMARY_OPERATION = "summary" QUALITY_OPERATION = "quality" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) work_id = Column(Integer, ForeignKey("works.id"), index=True) - work: Mapped[Work] = relationship("Work", back_populates="coverage_records") + work: Mapped[Work | None] = relationship("Work", back_populates="coverage_records") operation = Column(String(255), index=True, default=None) timestamp = Column(DateTime(timezone=True), index=True) @@ -775,10 +775,13 @@ class EquivalencyCoverageRecord(Base, BaseCoverageRecord): __tablename__ = "equivalentscoveragerecords" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) - equivalency_id = Column( - Integer, ForeignKey("equivalents.id", ondelete="CASCADE"), index=True + equivalency_id: Mapped[int] = Column( + Integer, + ForeignKey("equivalents.id", ondelete="CASCADE"), + index=True, + nullable=False, ) equivalency: Mapped[Equivalency] = relationship( "Equivalency", foreign_keys=equivalency_id diff --git a/src/palace/manager/sqlalchemy/model/credential.py b/src/palace/manager/sqlalchemy/model/credential.py index b33143b6f0..c6ceaf3b63 100644 --- a/src/palace/manager/sqlalchemy/model/credential.py +++ b/src/palace/manager/sqlalchemy/model/credential.py @@ -25,15 +25,15 @@ class Credential(Base): """A place to store credentials for external services.""" __tablename__ = "credentials" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) - data_source: Mapped[DataSource] = relationship( + data_source: Mapped[DataSource | None] = relationship( "DataSource", back_populates="credentials" ) patron_id = Column(Integer, ForeignKey("patrons.id"), index=True) - patron: Mapped[Patron] = relationship("Patron", back_populates="credentials") + patron: Mapped[Patron | None] = relationship("Patron", back_populates="credentials") collection_id = Column(Integer, ForeignKey("collections.id"), index=True) - collection: Mapped[Collection] = relationship( + collection: Mapped[Collection | None] = relationship( "Collection", back_populates="credentials" ) type = Column(String(255), index=True) diff --git a/src/palace/manager/sqlalchemy/model/customlist.py b/src/palace/manager/sqlalchemy/model/customlist.py index f6b5adb579..25a2f7e35f 100644 --- a/src/palace/manager/sqlalchemy/model/customlist.py +++ b/src/palace/manager/sqlalchemy/model/customlist.py @@ -47,10 +47,10 @@ class CustomList(Base): REPOPULATE = "repopulate" __tablename__ = "customlists" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) primary_language = Column(Unicode, index=True) data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) - data_source: Mapped[DataSource] = relationship( + data_source: Mapped[DataSource | None] = relationship( "DataSource", back_populates="custom_lists" ) foreign_identifier = Column(Unicode, index=True) @@ -60,11 +60,13 @@ class CustomList(Base): updated = Column(DateTime(timezone=True), index=True) responsible_party = Column(Unicode) library_id = Column(Integer, ForeignKey("libraries.id"), index=True, nullable=True) - library: Mapped[Library] = relationship("Library", back_populates="custom_lists") + library: Mapped[Library | None] = relationship( + "Library", back_populates="custom_lists" + ) # How many titles are in this list? This is calculated and # cached when the list contents change. - size = Column(Integer, nullable=False, default=0) + size: Mapped[int] = Column(Integer, nullable=False, default=0) entries: Mapped[list[CustomListEntry]] = relationship( "CustomListEntry", back_populates="customlist", uselist=True @@ -370,9 +372,9 @@ def update_size(self, db: Session): class CustomListEntry(Base): __tablename__ = "customlistentries" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) list_id = Column(Integer, ForeignKey("customlists.id"), index=True) - customlist: Mapped[CustomList] = relationship( + customlist: Mapped[CustomList | None] = relationship( "CustomList", back_populates="entries" ) @@ -386,7 +388,7 @@ class CustomListEntry(Base): "Work", back_populates="custom_list_entries" ) - featured = Column(Boolean, nullable=False, default=False) + featured: Mapped[bool] = Column(Boolean, nullable=False, default=False) annotation = Column(Unicode) # These two fields are for best-seller lists. Even after a book diff --git a/src/palace/manager/sqlalchemy/model/datasource.py b/src/palace/manager/sqlalchemy/model/datasource.py index 8f19bdf9aa..05b55d17f3 100644 --- a/src/palace/manager/sqlalchemy/model/datasource.py +++ b/src/palace/manager/sqlalchemy/model/datasource.py @@ -37,7 +37,7 @@ class DataSource(Base, HasSessionCache, DataSourceConstants): """A source for information about books, and possibly the books themselves.""" __tablename__ = "datasources" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) name = Column(String, unique=True, index=True) offers_licenses = Column(Boolean, default=False) primary_identifier_type = Column(String, index=True) diff --git a/src/palace/manager/sqlalchemy/model/devicetokens.py b/src/palace/manager/sqlalchemy/model/devicetokens.py index 2b69ff7598..06004093b2 100644 --- a/src/palace/manager/sqlalchemy/model/devicetokens.py +++ b/src/palace/manager/sqlalchemy/model/devicetokens.py @@ -25,8 +25,8 @@ class DeviceToken(Base): __tablename__ = "devicetokens" - id = Column("id", Integer, primary_key=True) - patron_id = Column( + id: Mapped[int] = Column("id", Integer, primary_key=True) + patron_id: Mapped[int] = Column( Integer, ForeignKey("patrons.id", ondelete="CASCADE", name="devicetokens_patron_fkey"), index=True, @@ -34,12 +34,14 @@ class DeviceToken(Base): ) patron: Mapped[Patron] = relationship("Patron", back_populates="device_tokens") - token_type_enum = Enum( - DeviceTokenTypes.FCM_ANDROID, DeviceTokenTypes.FCM_IOS, name="token_types" + token_type: Mapped[str] = Column( + Enum( + DeviceTokenTypes.FCM_ANDROID, DeviceTokenTypes.FCM_IOS, name="token_types" + ), + nullable=False, ) - token_type = Column(token_type_enum, nullable=False) - device_token = Column(Unicode, nullable=False, index=True) + device_token: Mapped[str] = Column(Unicode, nullable=False, index=True) __table_args__ = ( Index( diff --git a/src/palace/manager/sqlalchemy/model/discovery_service_registration.py b/src/palace/manager/sqlalchemy/model/discovery_service_registration.py index f88c03d8dd..609b960026 100644 --- a/src/palace/manager/sqlalchemy/model/discovery_service_registration.py +++ b/src/palace/manager/sqlalchemy/model/discovery_service_registration.py @@ -34,12 +34,12 @@ class DiscoveryServiceRegistration(Base): __tablename__ = "discovery_service_registrations" - status = Column( + status: Mapped[RegistrationStatus] = Column( AlchemyEnum(RegistrationStatus), default=RegistrationStatus.FAILURE, nullable=False, ) - stage = Column( + stage: Mapped[RegistrationStage] = Column( AlchemyEnum(RegistrationStage), default=RegistrationStage.TESTING, nullable=False, @@ -50,7 +50,7 @@ class DiscoveryServiceRegistration(Base): shared_secret = Column(Unicode) # The IntegrationConfiguration this registration is associated with. - integration_id = Column( + integration_id: Mapped[int] = Column( Integer, ForeignKey("integration_configurations.id", ondelete="CASCADE"), nullable=False, @@ -61,7 +61,7 @@ class DiscoveryServiceRegistration(Base): ) # The Library this registration is associated with. - library_id = Column( + library_id: Mapped[int] = Column( Integer, ForeignKey("libraries.id", ondelete="CASCADE"), nullable=False, diff --git a/src/palace/manager/sqlalchemy/model/edition.py b/src/palace/manager/sqlalchemy/model/edition.py index c071af001d..8d61f60d87 100644 --- a/src/palace/manager/sqlalchemy/model/edition.py +++ b/src/palace/manager/sqlalchemy/model/edition.py @@ -52,9 +52,11 @@ class Edition(Base, EditionConstants): """ __tablename__ = "editions" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) - data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) + data_source_id: Mapped[int] = Column( + Integer, ForeignKey("datasources.id"), index=True, nullable=False + ) data_source: Mapped[DataSource] = relationship( "DataSource", back_populates="editions" ) @@ -85,14 +87,16 @@ class Edition(Base, EditionConstants): # identifier--the one used by its data source to identify # it. Through the Equivalency class, it is associated with a # (probably huge) number of other identifiers. - primary_identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) + primary_identifier_id = Column( + Integer, ForeignKey("identifiers.id"), index=True, nullable=False + ) primary_identifier: Mapped[Identifier] = relationship( "Identifier", back_populates="primarily_identifies" ) # An Edition may be the presentation edition for a single Work. If it's not # a presentation edition for a work, work will be None. - work: Mapped[Work] = relationship( + work: Mapped[Work | None] = relationship( "Work", uselist=False, back_populates="presentation_edition" ) diff --git a/src/palace/manager/sqlalchemy/model/identifier.py b/src/palace/manager/sqlalchemy/model/identifier.py index 6dfb0e7fcf..885436b59c 100644 --- a/src/palace/manager/sqlalchemy/model/identifier.py +++ b/src/palace/manager/sqlalchemy/model/identifier.py @@ -237,7 +237,7 @@ class Identifier(Base, IdentifierConstants): """A way of uniquely referring to a particular edition.""" __tablename__ = "identifiers" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) type = Column(String(64), index=True) identifier = Column(String, index=True) @@ -674,12 +674,16 @@ def equivalent_to(self, data_source, identifier, strength): input=self, output=identifier, on_multiple="interchangeable", + create_method_kwargs={ + "strength": strength, + }, ) - eq.strength = strength if new: logging.info( "Identifier equivalency: %r==%r p=%.2f", self, identifier, strength ) + else: + eq.strength = strength return eq @classmethod @@ -1124,19 +1128,21 @@ class Equivalency(Base): # 'input' is the ID that was used as input to the datasource. # 'output' is the output - id = Column(Integer, primary_key=True) - input_id = Column(Integer, ForeignKey("identifiers.id"), index=True) + id: Mapped[int] = Column(Integer, primary_key=True) + input_id: Mapped[int] = Column( + Integer, ForeignKey("identifiers.id"), index=True, nullable=False + ) input: Mapped[Identifier] = relationship( "Identifier", foreign_keys=input_id, back_populates="equivalencies" ) - output_id = Column(Integer, ForeignKey("identifiers.id"), index=True) + output_id: Mapped[int] = Column(Integer, ForeignKey("identifiers.id"), index=True) output: Mapped[Identifier] = relationship( "Identifier", foreign_keys=output_id, back_populates="inbound_equivalencies" ) # Who says? data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) - data_source: Mapped[DataSource] = relationship( + data_source: Mapped[DataSource | None] = relationship( "DataSource", back_populates="id_equivalencies" ) @@ -1199,19 +1205,19 @@ class RecursiveEquivalencyCache(Base): __tablename__ = "recursiveequivalentscache" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # The "parent" or the start of the chain parent_identifier_id = Column( Integer, ForeignKey("identifiers.id", ondelete="CASCADE") ) - parent_identifier: Mapped[Identifier] = relationship( + parent_identifier: Mapped[Identifier | None] = relationship( "Identifier", foreign_keys=parent_identifier_id ) # The identifier chained to the parent identifier_id = Column(Integer, ForeignKey("identifiers.id", ondelete="CASCADE")) - identifier: Mapped[Identifier] = relationship( + identifier: Mapped[Identifier | None] = relationship( "Identifier", foreign_keys=identifier_id ) diff --git a/src/palace/manager/sqlalchemy/model/integration.py b/src/palace/manager/sqlalchemy/model/integration.py index 3a9ecf1d70..2742cc3aed 100644 --- a/src/palace/manager/sqlalchemy/model/integration.py +++ b/src/palace/manager/sqlalchemy/model/integration.py @@ -29,20 +29,20 @@ class IntegrationConfiguration(Base): """ __tablename__ = "integration_configurations" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # The protocol is used to load the correct implementation class for # this integration. It is looked up in the IntegrationRegistry. - protocol = Column(Unicode, nullable=False) + protocol: Mapped[str] = Column(Unicode, nullable=False) # The goal of the integration is used to differentiate between the # different types of integrations. For example, a goal of "authentication" # would be used for an authentication provider. - goal = Column(SQLAlchemyEnum(Goals), nullable=False, index=True) + goal: Mapped[Goals] = Column(SQLAlchemyEnum(Goals), nullable=False, index=True) # A unique name for this integration. This is primarily # used to identify integrations from command-line scripts. - name = Column(Unicode, nullable=False, unique=True) + name: Mapped[str] = Column(Unicode, nullable=False, unique=True) # The configuration settings for this integration. Stored as json. settings_dict: Mapped[dict[str, Any]] = Column( @@ -68,7 +68,9 @@ def context_update(self, new_context: dict[str, Any]) -> None: flag_modified(self, "context") # Self test results, stored as json. - self_test_results = Column(JSONB, nullable=False, default=dict) + self_test_results: Mapped[dict[str, Any]] = Column( + JSONB, nullable=False, default=dict + ) library_configurations: Mapped[list[IntegrationLibraryConfiguration]] = ( relationship( @@ -102,8 +104,6 @@ def for_library( db = Session.object_session(self) if isinstance(library, Library): - if library.id is None: - return None library_id = library.id else: library_id = library @@ -187,7 +187,7 @@ class IntegrationLibraryConfiguration(Base): # The IntegrationConfiguration this library configuration is # associated with. - parent_id = Column( + parent_id: Mapped[int] = Column( Integer, ForeignKey("integration_configurations.id", ondelete="CASCADE"), primary_key=True, @@ -198,7 +198,7 @@ class IntegrationLibraryConfiguration(Base): ) # The library this integration is associated with. - library_id = Column( + library_id: Mapped[int] = Column( Integer, ForeignKey("libraries.id", ondelete="CASCADE"), primary_key=True, diff --git a/src/palace/manager/sqlalchemy/model/key.py b/src/palace/manager/sqlalchemy/model/key.py index 2645b427e5..98c5f18888 100644 --- a/src/palace/manager/sqlalchemy/model/key.py +++ b/src/palace/manager/sqlalchemy/model/key.py @@ -8,7 +8,7 @@ from sqlalchemy import Enum as SaEnum from sqlalchemy import Unicode, delete, select from sqlalchemy.dialects.postgresql import UUID -from sqlalchemy.orm import Session +from sqlalchemy.orm import Mapped, Session from typing_extensions import Self from palace.manager.sqlalchemy.model.base import Base @@ -26,12 +26,14 @@ class KeyType(Enum): class Key(Base): __tablename__ = "keys" - id = Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - created = Column( + id: Mapped[uuid.UUID] = Column( + UUID(as_uuid=True), primary_key=True, default=uuid.uuid4 + ) + created: Mapped[datetime.datetime] = Column( DateTime(timezone=True), index=True, nullable=False, default=utc_now ) - value = Column(Unicode, nullable=False) - type = Column(SaEnum(KeyType), nullable=False, index=True) + value: Mapped[str] = Column(Unicode, nullable=False) + type: Mapped[KeyType] = Column(SaEnum(KeyType), nullable=False, index=True) def __repr__(self) -> str: return f"" diff --git a/src/palace/manager/sqlalchemy/model/lane.py b/src/palace/manager/sqlalchemy/model/lane.py index 73a87897e1..24ef9602bc 100644 --- a/src/palace/manager/sqlalchemy/model/lane.py +++ b/src/palace/manager/sqlalchemy/model/lane.py @@ -2598,22 +2598,26 @@ class LaneGenre(Base): """Relationship object between Lane and Genre.""" __tablename__ = "lanes_genres" - id = Column(Integer, primary_key=True) - lane_id = Column(Integer, ForeignKey("lanes.id"), index=True, nullable=False) + id: Mapped[int] = Column(Integer, primary_key=True) + lane_id: Mapped[int] = Column( + Integer, ForeignKey("lanes.id"), index=True, nullable=False + ) lane: Mapped[Lane] = relationship("Lane", back_populates="lane_genres") - genre_id = Column(Integer, ForeignKey("genres.id"), index=True, nullable=False) + genre_id: Mapped[int] = Column( + Integer, ForeignKey("genres.id"), index=True, nullable=False + ) genre: Mapped[Genre] = relationship(Genre, back_populates="lane_genres") # An inclusive relationship means that books classified under the # genre are included in the lane. An exclusive relationship means # that books classified under the genre are excluded, even if they # would otherwise be included. - inclusive = Column(Boolean, default=True, nullable=False) + inclusive: Mapped[bool] = Column(Boolean, default=True, nullable=False) # By default, this relationship applies not only to the genre # itself but to all of its subgenres. Setting recursive=false # means that only the genre itself is affected. - recursive = Column(Boolean, default=True, nullable=False) + recursive: Mapped[bool] = Column(Boolean, default=True, nullable=False) __table_args__ = (UniqueConstraint("lane_id", "genre_id"),) @@ -2640,8 +2644,10 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): MAX_CACHE_AGE = 20 * 60 __tablename__ = "lanes" - id = Column(Integer, primary_key=True) - library_id = Column(Integer, ForeignKey("libraries.id"), index=True, nullable=False) + id: Mapped[int] = Column(Integer, primary_key=True) + library_id: Mapped[int] = Column( + Integer, ForeignKey("libraries.id"), index=True, nullable=False + ) library: Mapped[Library] = relationship(Library, back_populates="lanes") parent_id = Column(Integer, ForeignKey("lanes.id"), index=True, nullable=True) @@ -2651,11 +2657,11 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): remote_side=[id], ) - priority = Column(Integer, index=True, nullable=False, default=0) + priority: Mapped[int] = Column(Integer, index=True, nullable=False, default=0) # How many titles are in this lane? This is periodically # calculated and cached. - size = Column(Integer, nullable=False, default=0) + size: Mapped[int] = Column(Integer, nullable=False, default=0) # How many titles are in this lane when viewed through a specific # entry point? This is periodically calculated and cached. @@ -2712,7 +2718,7 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): license_datasource_id = Column( Integer, ForeignKey("datasources.id"), index=True, nullable=True ) - license_datasource: Mapped[DataSource] = relationship( + license_datasource: Mapped[DataSource | None] = relationship( "DataSource", back_populates="license_lanes", foreign_keys=[license_datasource_id], @@ -2723,7 +2729,7 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): _list_datasource_id = Column( Integer, ForeignKey("datasources.id"), index=True, nullable=True ) - _list_datasource: Mapped[DataSource] = relationship( + _list_datasource: Mapped[DataSource | None] = relationship( "DataSource", back_populates="list_lanes", foreign_keys=[_list_datasource_id] ) @@ -2742,7 +2748,9 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): # If this is set to True, then a book will show up in a lane only # if it would _also_ show up in its parent lane. - inherit_parent_restrictions = Column(Boolean, default=True, nullable=False) + inherit_parent_restrictions: Mapped[bool] = Column( + Boolean, default=True, nullable=False + ) # Patrons whose external type is in this list will be sent to this # lane when they ask for the root lane. @@ -2757,11 +2765,13 @@ class Lane(Base, DatabaseBackedWorkList, HierarchyWorkList): # one would want to see a big list containing everything, and b) # the sublanes are exhaustive of the Lane's content, so there's # nothing new to be seen by going into that big list. - include_self_in_grouped_feed = Column(Boolean, default=True, nullable=False) + include_self_in_grouped_feed: Mapped[bool] = Column( + Boolean, default=True, nullable=False + ) # Only a visible lane will show up in the user interface. The # admin interface can see all the lanes, visible or not. - _visible = Column("visible", Boolean, default=True, nullable=False) + _visible: Mapped[bool] = Column("visible", Boolean, default=True, nullable=False) __table_args__ = (UniqueConstraint("parent_id", "display_name"),) diff --git a/src/palace/manager/sqlalchemy/model/library.py b/src/palace/manager/sqlalchemy/model/library.py index 286a89f85d..b8bf5f35e4 100644 --- a/src/palace/manager/sqlalchemy/model/library.py +++ b/src/palace/manager/sqlalchemy/model/library.py @@ -64,7 +64,7 @@ class Library(Base, HasSessionCache): __tablename__ = "libraries" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # The human-readable name of this library. Used in the library's # Authentication for OPDS document. @@ -72,7 +72,7 @@ class Library(Base, HasSessionCache): # A short name of this library, to use when identifying it in # scripts. e.g. "NYPL" for NYPL. - short_name = Column(Unicode, unique=True, nullable=False) + short_name: Mapped[str] = Column(Unicode, unique=True, nullable=False) # A UUID that uniquely identifies the library among all libraries # in the world. This is used to serve the library's Authentication @@ -150,12 +150,12 @@ class Library(Base, HasSessionCache): # The library's public / private RSA key-pair. # The public key is stored in PEM format. - public_key = Column(Unicode, nullable=False) + public_key: Mapped[str] = Column(Unicode, nullable=False) # The private key is stored in DER binary format. - private_key = Column(LargeBinary, nullable=False) + private_key: Mapped[bytes] = Column(LargeBinary, nullable=False) # The libraries logo image, stored as a base64 encoded string. - logo: Mapped[LibraryLogo] = relationship( + logo: Mapped[LibraryLogo | None] = relationship( "LibraryLogo", back_populates="library", cascade="all, delete-orphan", @@ -500,13 +500,15 @@ class LibraryLogo(Base): """ __tablename__ = "libraries_logos" - library_id = Column(Integer, ForeignKey("libraries.id"), primary_key=True) + library_id: Mapped[int] = Column( + Integer, ForeignKey("libraries.id"), primary_key=True + ) library: Mapped[Library] = relationship( "Library", back_populates="logo", uselist=False ) # The logo stored as a base-64 encoded png. - content = Column(LargeBinary, nullable=False) + content: Mapped[bytes] = Column(LargeBinary, nullable=False) @property def data_url(self) -> str: diff --git a/src/palace/manager/sqlalchemy/model/licensing.py b/src/palace/manager/sqlalchemy/model/licensing.py index f5569d7b73..d15a34d633 100644 --- a/src/palace/manager/sqlalchemy/model/licensing.py +++ b/src/palace/manager/sqlalchemy/model/licensing.py @@ -130,7 +130,7 @@ class License(Base, LicenseFunctions): """ __tablename__ = "licenses" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) identifier = Column(Unicode) checkout_url = Column(Unicode) @@ -149,7 +149,9 @@ class License(Base, LicenseFunctions): terms_concurrency = Column(Integer) # A License belongs to one LicensePool. - license_pool_id = Column(Integer, ForeignKey("licensepools.id"), index=True) + license_pool_id: Mapped[int] = Column( + Integer, ForeignKey("licensepools.id"), index=True, nullable=False + ) license_pool: Mapped[LicensePool] = relationship( "LicensePool", back_populates="licenses" ) @@ -215,27 +217,31 @@ class LicensePool(Base): UNLIMITED_ACCESS = -1 __tablename__ = "licensepools" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # A LicensePool may be associated with a Work. (If it's not, no one # can check it out.) work_id = Column(Integer, ForeignKey("works.id"), index=True) - work: Mapped[Work] = relationship("Work", back_populates="license_pools") + work: Mapped[Work | None] = relationship("Work", back_populates="license_pools") # Each LicensePool is associated with one DataSource and one # Identifier. - data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) + data_source_id: Mapped[int] = Column( + Integer, ForeignKey("datasources.id"), index=True, nullable=False + ) data_source: Mapped[DataSource] = relationship( "DataSource", back_populates="license_pools", lazy="joined" ) - identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) + identifier_id: Mapped[int] = Column( + Integer, ForeignKey("identifiers.id"), index=True, nullable=False + ) identifier: Mapped[Identifier] = relationship( "Identifier", back_populates="licensed_through", lazy="joined" ) # Each LicensePool belongs to one Collection. - collection_id = Column( + collection_id: Mapped[int] = Column( Integer, ForeignKey("collections.id"), index=True, nullable=False ) @@ -246,7 +252,7 @@ class LicensePool(Base): # Each LicensePool has an Edition which contains the metadata used # to describe this book. presentation_edition_id = Column(Integer, ForeignKey("editions.id"), index=True) - presentation_edition: Mapped[Edition] = relationship( + presentation_edition: Mapped[Edition | None] = relationship( "Edition", back_populates="is_presentation_for" ) @@ -298,7 +304,7 @@ class LicensePool(Base): licenses_available: int = Column(Integer, default=0, index=True) licenses_reserved: int = Column(Integer, default=0) patrons_in_hold_queue: int = Column(Integer, default=0) - should_track_playtime = Column(Boolean, default=False, nullable=False) + should_track_playtime: Mapped[bool] = Column(Boolean, default=False, nullable=False) # This lets us cache the work of figuring out the best open access # link for this LicensePool. @@ -1203,11 +1209,6 @@ def calculate_work( """ from palace.manager.sqlalchemy.model.work import Work - if not self.identifier: - # A LicensePool with no Identifier should never have a Work. - self.work = None - return None, False - if known_edition: presentation_edition = known_edition else: @@ -1503,23 +1504,23 @@ class LicensePoolDeliveryMechanism(Base): __tablename__ = "licensepooldeliveries" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) - data_source_id = Column( + data_source_id: Mapped[int] = Column( Integer, ForeignKey("datasources.id"), index=True, nullable=False ) data_source: Mapped[DataSource] = relationship( "DataSource", back_populates="delivery_mechanisms" ) - identifier_id = Column( + identifier_id: Mapped[int] = Column( Integer, ForeignKey("identifiers.id"), index=True, nullable=False ) identifier: Mapped[Identifier] = relationship( "Identifier", back_populates="delivery_mechanisms" ) - delivery_mechanism_id = Column( + delivery_mechanism_id: Mapped[int] = Column( Integer, ForeignKey("deliverymechanisms.id"), index=True, nullable=False ) delivery_mechanism: Mapped[DeliveryMechanism] = relationship( @@ -1528,7 +1529,7 @@ class LicensePoolDeliveryMechanism(Base): ) resource_id = Column(Integer, ForeignKey("resources.id"), nullable=True) - resource: Mapped[Resource] = relationship( + resource: Mapped[Resource | None] = relationship( "Resource", back_populates="licensepooldeliverymechanisms" ) @@ -1760,7 +1761,7 @@ class DeliveryMechanism(Base, HasSessionCache): } __tablename__ = "deliverymechanisms" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) content_type = Column(String) drm_scheme = Column(String) @@ -2036,7 +2037,7 @@ class RightsStatus(Base): } __tablename__ = "rightsstatus" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # A URI unique to the license. This may be a URL (e.g. Creative # Commons) diff --git a/src/palace/manager/sqlalchemy/model/marcfile.py b/src/palace/manager/sqlalchemy/model/marcfile.py index 8c3a555b82..28b43d7e81 100644 --- a/src/palace/manager/sqlalchemy/model/marcfile.py +++ b/src/palace/manager/sqlalchemy/model/marcfile.py @@ -1,5 +1,6 @@ from __future__ import annotations +import datetime import uuid from typing import TYPE_CHECKING @@ -18,7 +19,9 @@ class MarcFile(Base): """A record that a MARC file has been created and cached for a particular library and collection.""" __tablename__ = "marcfiles" - id = Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) + id: Mapped[uuid.UUID] = Column( + UUID(as_uuid=True), primary_key=True, default=uuid.uuid4 + ) # The library should never be null in normal operation, but if a library is deleted, we don't want to lose the # record of the MARC file, so we set the library to null. @@ -28,9 +31,7 @@ class MarcFile(Base): nullable=True, index=True, ) - library: Mapped[Library] = relationship( - "Library", - ) + library: Mapped[Library | None] = relationship("Library") # The collection should never be null in normal operation, but similar to the library, if a collection is deleted, # we don't want to lose the record of the MARC file, so we set the collection to null. @@ -40,15 +41,15 @@ class MarcFile(Base): nullable=True, index=True, ) - collection: Mapped[Collection] = relationship( - "Collection", - ) + collection: Mapped[Collection | None] = relationship("Collection") # The key in s3 used to store the file. - key = Column(Unicode, nullable=False) + key: Mapped[str] = Column(Unicode, nullable=False) # The creation date of the file. - created = Column(DateTime(timezone=True), nullable=False, index=True) + created: Mapped[datetime.datetime] = Column( + DateTime(timezone=True), nullable=False, index=True + ) # If the file is a delta, the date of the previous file. If the file is a full file, null. since = Column(DateTime(timezone=True), nullable=True) diff --git a/src/palace/manager/sqlalchemy/model/measurement.py b/src/palace/manager/sqlalchemy/model/measurement.py index 86b5085629..c287612a7d 100644 --- a/src/palace/manager/sqlalchemy/model/measurement.py +++ b/src/palace/manager/sqlalchemy/model/measurement.py @@ -710,7 +710,7 @@ class Measurement(Base): DataSourceConstants.LIBRARY_STAFF: [1, 5], } - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # A Measurement is always associated with some Identifier. identifier_id = Column(Integer, ForeignKey("identifiers.id"), index=True) @@ -719,7 +719,9 @@ class Measurement(Base): ) # A Measurement always comes from some DataSource. - data_source_id = Column(Integer, ForeignKey("datasources.id"), index=True) + data_source_id: Mapped[int] = Column( + Integer, ForeignKey("datasources.id"), index=True + ) data_source: Mapped[DataSource] = relationship( "DataSource", back_populates="measurements" ) diff --git a/src/palace/manager/sqlalchemy/model/patron.py b/src/palace/manager/sqlalchemy/model/patron.py index 242b7f65e6..78a4bbc25f 100644 --- a/src/palace/manager/sqlalchemy/model/patron.py +++ b/src/palace/manager/sqlalchemy/model/patron.py @@ -56,8 +56,6 @@ class LoanAndHoldMixin: def work(self) -> Work | None: """Try to find the corresponding work for this Loan/Hold.""" license_pool = self.license_pool - if not license_pool: - return None if license_pool.work: return license_pool.work if license_pool.presentation_edition and license_pool.presentation_edition.work: @@ -65,22 +63,21 @@ def work(self) -> Work | None: return None @property - def library(self) -> Library | None: - """Try to find the corresponding library for this Loan/Hold.""" - if self.patron: - return self.patron.library - # If this Loan/Hold belongs to an external patron, there may be no library. - return None + def library(self) -> Library: + """The corresponding library for this Loan/Hold.""" + return self.patron.library class Patron(Base, RedisKeyMixin): __tablename__ = "patrons" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # Each patron is the patron _of_ one particular library. An # individual human being may patronize multiple libraries, but # they will have a different patron account at each one. - library_id = Column(Integer, ForeignKey("libraries.id"), index=True, nullable=False) + library_id: Mapped[int] = Column( + Integer, ForeignKey("libraries.id"), index=True, nullable=False + ) library: Mapped[Library] = relationship("Library", back_populates="patrons") # The patron's permanent unique identifier in an external library @@ -113,7 +110,9 @@ class Patron(Base, RedisKeyMixin): # in a way that allows users to disassociate their patron info # with account activity at any time. When this UUID is reset it effectively # dissociates any patron activity history with this patron. - uuid = Column(UUID(as_uuid=True), nullable=False, default=uuid.uuid4) + uuid: Mapped[uuid.UUID] = Column( + UUID(as_uuid=True), nullable=False, default=uuid.uuid4 + ) # The last time this record was synced up with an external library # system such as an ILS. @@ -517,13 +516,17 @@ def ensure_tuple( class Loan(Base, LoanAndHoldMixin): __tablename__ = "loans" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) - patron_id = Column(Integer, ForeignKey("patrons.id"), index=True) + patron_id: Mapped[int] = Column( + Integer, ForeignKey("patrons.id"), index=True, nullable=False + ) patron: Mapped[Patron] = relationship("Patron", back_populates="loans") # A Loan is always associated with a LicensePool. - license_pool_id = Column(Integer, ForeignKey("licensepools.id"), index=True) + license_pool_id: Mapped[int] = Column( + Integer, ForeignKey("licensepools.id"), index=True, nullable=False + ) license_pool: Mapped[LicensePool] = relationship( "LicensePool", back_populates="loans" ) @@ -531,7 +534,7 @@ class Loan(Base, LoanAndHoldMixin): # It may also be associated with an individual License if the source # provides information about individual licenses. license_id = Column(Integer, ForeignKey("licenses.id"), index=True, nullable=True) - license: Mapped[License] = relationship("License", back_populates="loans") + license: Mapped[License | None] = relationship("License", back_populates="loans") fulfillment_id = Column(Integer, ForeignKey("licensepooldeliveries.id")) fulfillment: Mapped[LicensePoolDeliveryMechanism | None] = relationship( @@ -568,9 +571,16 @@ class Hold(Base, LoanAndHoldMixin): """A patron is in line to check out a book.""" __tablename__ = "holds" - id = Column(Integer, primary_key=True) - patron_id = Column(Integer, ForeignKey("patrons.id"), index=True) - license_pool_id = Column(Integer, ForeignKey("licensepools.id"), index=True) + id: Mapped[int] = Column(Integer, primary_key=True) + patron_id: Mapped[int] = Column( + Integer, ForeignKey("patrons.id"), index=True, nullable=False + ) + patron: Mapped[Patron] = relationship( + "Patron", back_populates="holds", lazy="joined" + ) + license_pool_id: Mapped[int] = Column( + Integer, ForeignKey("licensepools.id"), index=True, nullable=False + ) license_pool: Mapped[LicensePool] = relationship( "LicensePool", back_populates="holds" ) @@ -579,10 +589,6 @@ class Hold(Base, LoanAndHoldMixin): position = Column(Integer, index=True) patron_last_notified = Column(DateTime, nullable=True) - patron: Mapped[Patron] = relationship( - "Patron", back_populates="holds", lazy="joined" - ) - def __lt__(self, other: Any) -> bool: if not isinstance(other, Hold) or self.id is None or other.id is None: return NotImplemented @@ -717,7 +723,7 @@ class Annotation(Base): ] __tablename__ = "annotations" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) patron_id = Column(Integer, ForeignKey("patrons.id"), index=True) patron: Mapped[Patron] = relationship("Patron", back_populates="annotations") diff --git a/src/palace/manager/sqlalchemy/model/resource.py b/src/palace/manager/sqlalchemy/model/resource.py index 5bc4396040..8174681b01 100644 --- a/src/palace/manager/sqlalchemy/model/resource.py +++ b/src/palace/manager/sqlalchemy/model/resource.py @@ -67,7 +67,7 @@ class Resource(Base): # than a lousy cover we got from the Internet. MINIMUM_IMAGE_QUALITY = 0.25 - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # A URI that uniquely identifies this resource. Most of the time # this will be an HTTP URL, which is why we're calling it 'url', @@ -106,7 +106,7 @@ class Resource(Base): # An archived Representation of this Resource. representation_id = Column(Integer, ForeignKey("representations.id"), index=True) - representation: Mapped[Representation] = relationship( + representation: Mapped[Representation | None] = relationship( "Representation", back_populates="resource", uselist=False ) @@ -129,7 +129,7 @@ class Resource(Base): ) # A derivative resource may have one original. - derived_through: Mapped[ResourceTransformation] = relationship( + derived_through: Mapped[ResourceTransformation | None] = relationship( "ResourceTransformation", foreign_keys="ResourceTransformation.derivative_id", back_populates="derivative", @@ -385,7 +385,7 @@ class ResourceTransformation(Base): __tablename__ = "resourcetransformations" # The derivative resource. A resource can only be derived from one other resource. - derivative_id = Column( + derivative_id: Mapped[int] = Column( Integer, ForeignKey("resources.id"), index=True, primary_key=True ) derivative: Mapped[Resource] = relationship( @@ -394,7 +394,7 @@ class ResourceTransformation(Base): # The original resource that was transformed into the derivative. original_id = Column(Integer, ForeignKey("resources.id"), index=True) - original: Mapped[Resource] = relationship( + original: Mapped[Resource | None] = relationship( "Resource", back_populates="transformations", foreign_keys=[original_id] ) @@ -407,25 +407,25 @@ class Hyperlink(Base, LinkRelations): __tablename__ = "hyperlinks" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # A Hyperlink is always associated with some Identifier. - identifier_id = Column( + identifier_id: Mapped[int] = Column( Integer, ForeignKey("identifiers.id"), index=True, nullable=False ) identifier: Mapped[Identifier] = relationship("Identifier", back_populates="links") # The DataSource through which this link was discovered. - data_source_id = Column( + data_source_id: Mapped[int] = Column( Integer, ForeignKey("datasources.id"), index=True, nullable=False ) data_source: Mapped[DataSource] = relationship("DataSource", back_populates="links") # The link relation between the Identifier and the Resource. - rel = Column(Unicode, index=True, nullable=False) + rel: Mapped[str] = Column(Unicode, index=True, nullable=False) # The Resource on the other end of the link. - resource_id = Column( + resource_id: Mapped[int] = Column( Integer, ForeignKey("resources.id"), index=True, nullable=False ) resource: Mapped[Resource] = relationship("Resource", back_populates="links") @@ -477,7 +477,7 @@ class Representation(Base, MediaTypes): """ __tablename__ = "representations" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # URL from which the representation was fetched. url = Column(Unicode, index=True) @@ -485,7 +485,7 @@ class Representation(Base, MediaTypes): # The media type of the representation. media_type = Column(Unicode) - resource: Mapped[Resource] = relationship( + resource: Mapped[Resource | None] = relationship( "Resource", back_populates="representation", uselist=False ) diff --git a/src/palace/manager/sqlalchemy/model/saml.py b/src/palace/manager/sqlalchemy/model/saml.py index ba27410e75..214af282ee 100644 --- a/src/palace/manager/sqlalchemy/model/saml.py +++ b/src/palace/manager/sqlalchemy/model/saml.py @@ -11,9 +11,9 @@ class SAMLFederation(Base): __tablename__ = "samlfederations" - id = Column(Integer, primary_key=True) - type = Column(String(256), nullable=False, unique=True) - idp_metadata_service_url = Column(String(2048), nullable=False) + id: Mapped[int] = Column(Integer, primary_key=True) + type: Mapped[str] = Column(String(256), nullable=False, unique=True) + idp_metadata_service_url: Mapped[str] = Column(String(2048), nullable=False) last_updated_at = Column(DateTime(), nullable=True) certificate = Column(Text(), nullable=True) @@ -81,14 +81,14 @@ class SAMLFederatedIdentityProvider(Base): __tablename__ = "samlfederatedidps" - id = Column(Integer, primary_key=True) - entity_id = Column(String(256), nullable=False) - display_name = Column(String(256), nullable=False) + id: Mapped[int] = Column(Integer, primary_key=True) + entity_id: Mapped[str] = Column(String(256), nullable=False) + display_name: Mapped[str] = Column(String(256), nullable=False) - xml_metadata = Column(Text(), nullable=False) + xml_metadata: Mapped[str] = Column(Text(), nullable=False) federation_id = Column(Integer, ForeignKey("samlfederations.id"), index=True) - federation: Mapped[SAMLFederation] = relationship( + federation: Mapped[SAMLFederation | None] = relationship( "SAMLFederation", foreign_keys=federation_id, back_populates="identity_providers", diff --git a/src/palace/manager/sqlalchemy/model/time_tracking.py b/src/palace/manager/sqlalchemy/model/time_tracking.py index 4309bbcc81..77829673e6 100644 --- a/src/palace/manager/sqlalchemy/model/time_tracking.py +++ b/src/palace/manager/sqlalchemy/model/time_tracking.py @@ -31,7 +31,7 @@ class PlaytimeEntry(Base): __tablename__ = "playtime_entries" - id = Column(Integer, autoincrement=True, primary_key=True) + id: Mapped[int] = Column(Integer, autoincrement=True, primary_key=True) # Even if related objects are deleted, we keep our row. identifier_id = Column( @@ -39,39 +39,38 @@ class PlaytimeEntry(Base): ForeignKey("identifiers.id", onupdate="CASCADE", ondelete="SET NULL"), nullable=True, ) + identifier: Mapped[Identifier | None] = relationship("Identifier", uselist=False) collection_id = Column( Integer, ForeignKey("collections.id", onupdate="CASCADE", ondelete="SET NULL"), nullable=True, ) + collection: Mapped[Collection | None] = relationship("Collection", uselist=False) library_id = Column( Integer, ForeignKey("libraries.id", onupdate="CASCADE", ondelete="SET NULL"), nullable=True, ) + library: Mapped[Library | None] = relationship("Library", uselist=False) # Related objects can be deleted, so we keep string representation. - identifier_str = Column(String, nullable=False) - collection_name = Column(String, nullable=False) - library_name = Column(String, nullable=False) + identifier_str: Mapped[str] = Column(String, nullable=False) + collection_name: Mapped[str] = Column(String, nullable=False) + library_name: Mapped[str] = Column(String, nullable=False) timestamp: Mapped[datetime.datetime] = Column( DateTime(timezone=True), nullable=False ) - total_seconds_played = Column( + total_seconds_played: Mapped[int] = Column( Integer, CheckConstraint( "total_seconds_played <= 60", name="max_total_seconds_played_constraint" ), nullable=False, ) - tracking_id = Column(String(64), nullable=False) + tracking_id: Mapped[str] = Column(String(64), nullable=False) processed = Column(Boolean, default=False) - identifier: Mapped[Identifier] = relationship("Identifier", uselist=False) - collection: Mapped[Collection] = relationship("Collection", uselist=False) - library: Mapped[Library] = relationship("Library", uselist=False) - - loan_identifier = Column(String(40), nullable=False) + loan_identifier: Mapped[str] = Column(String(40), nullable=False) __table_args__ = ( UniqueConstraint( @@ -87,7 +86,7 @@ class PlaytimeEntry(Base): class PlaytimeSummary(Base): __tablename__ = "playtime_summaries" - id = Column(Integer, autoincrement=True, primary_key=True) + id: Mapped[int] = Column(Integer, autoincrement=True, primary_key=True) # Even if related objects are deleted, we keep our row. identifier_id = Column( @@ -96,22 +95,25 @@ class PlaytimeSummary(Base): nullable=True, index=True, ) + identifier: Mapped[Identifier | None] = relationship("Identifier", uselist=False) collection_id = Column( Integer, ForeignKey("collections.id", onupdate="CASCADE", ondelete="SET NULL"), nullable=True, index=True, ) + collection: Mapped[Collection | None] = relationship("Collection", uselist=False) library_id = Column( Integer, ForeignKey("libraries.id", onupdate="CASCADE", ondelete="SET NULL"), nullable=True, index=True, ) + library: Mapped[Library | None] = relationship("Library", uselist=False) # Related objects can be deleted, so we keep string representation. - identifier_str = Column(String, nullable=False) - collection_name = Column(String, nullable=False) - library_name = Column(String, nullable=False) + identifier_str: Mapped[str] = Column(String, nullable=False) + collection_name: Mapped[str] = Column(String, nullable=False) + library_name: Mapped[str] = Column(String, nullable=False) # This should be a per-minute datetime timestamp: Mapped[datetime.datetime] = Column( @@ -127,11 +129,7 @@ class PlaytimeSummary(Base): title = Column(String) isbn = Column(String) - loan_identifier = Column(String(40), nullable=False) - - identifier: Mapped[Identifier] = relationship("Identifier", uselist=False) - collection: Mapped[Collection] = relationship("Collection", uselist=False) - library: Mapped[Library] = relationship("Library", uselist=False) + loan_identifier: Mapped[str] = Column(String(40), nullable=False) __table_args__ = ( UniqueConstraint( diff --git a/src/palace/manager/sqlalchemy/model/work.py b/src/palace/manager/sqlalchemy/model/work.py index 9c0cc42637..3f20a77fbe 100644 --- a/src/palace/manager/sqlalchemy/model/work.py +++ b/src/palace/manager/sqlalchemy/model/work.py @@ -82,11 +82,15 @@ class WorkGenre(Base): """An assignment of a genre to a work.""" __tablename__ = "workgenres" - id = Column(Integer, primary_key=True) - genre_id = Column(Integer, ForeignKey("genres.id"), index=True) + id: Mapped[int] = Column(Integer, primary_key=True) + genre_id: Mapped[int] = Column( + Integer, ForeignKey("genres.id"), index=True, nullable=False + ) genre: Mapped[Genre] = relationship("Genre", back_populates="work_genres") - work_id = Column(Integer, ForeignKey("works.id"), index=True) + work_id: Mapped[int] = Column( + Integer, ForeignKey("works.id"), index=True, nullable=False + ) work: Mapped[Work] = relationship("Work", back_populates="work_genres") affinity = Column(Float, index=True, default=0) @@ -140,7 +144,7 @@ class Work(Base, LoggerMixin): } __tablename__ = "works" - id = Column(Integer, primary_key=True) + id: Mapped[int] = Column(Integer, primary_key=True) # One Work may have copies scattered across many LicensePools. license_pools: Mapped[list[LicensePool]] = relationship( @@ -175,7 +179,7 @@ class Work(Base, LoggerMixin): target_age = Column(INT4RANGE, index=True) fiction = Column(Boolean, index=True) - summary_id: Mapped[int] = Column( + summary_id: Mapped[int | None] = Column( Integer, ForeignKey("resources.id", use_alter=True, name="fk_works_summary_id"), index=True, @@ -1878,6 +1882,7 @@ def delete( def add_work_to_customlists_for_collection(pool_or_work: LicensePool | Work) -> None: + work: Work | None if isinstance(pool_or_work, Work): work = pool_or_work pools = work.license_pools diff --git a/src/palace/manager/util/notifications.py b/src/palace/manager/util/notifications.py index 7e731dc884..c78d6c75a6 100644 --- a/src/palace/manager/util/notifications.py +++ b/src/palace/manager/util/notifications.py @@ -112,11 +112,15 @@ def send_loan_expiry_message( - Which patron and make the loans api request with the right authentication""" url = self.base_url edition = loan.license_pool.presentation_edition + if edition is None: + self.log.error( + f"Failed to send loan expiry notification because the edition is missing for " + f"loan '{loan.id}', patron '{loan.patron.authorization_identifier}', lp '{loan.license_pool.id}'" + ) + return [] + identifier = loan.license_pool.identifier library = loan.library - # It shouldn't be possible to get here for a loan without a library, but for mypy - # and safety we will assert it anyway - assert library is not None library_short_name = library.short_name library_name = library.name title = f"Only {days_to_expiry} {'days' if days_to_expiry != 1 else 'day'} left on your loan!" diff --git a/tests/fixtures/database.py b/tests/fixtures/database.py index 13933062d5..d6b3037e91 100644 --- a/tests/fixtures/database.py +++ b/tests/fixtures/database.py @@ -725,7 +725,7 @@ def edition( unlimited_access=False, ): id = identifier_id or self.fresh_str() - source = DataSource.lookup(self.session, data_source_name) + source = DataSource.lookup(self.session, data_source_name, autocreate=True) wr = Edition.for_foreign_id(self.session, source, identifier_type, id)[0] if not title: title = self.fresh_str() diff --git a/tests/manager/api/admin/controller/test_custom_lists.py b/tests/manager/api/admin/controller/test_custom_lists.py index 37ea548421..59a7023b11 100644 --- a/tests/manager/api/admin/controller/test_custom_lists.py +++ b/tests/manager/api/admin/controller/test_custom_lists.py @@ -1113,6 +1113,7 @@ def test_auto_update_edit(self, admin_librarian_fixture: AdminLibrarianFixture): custom_list, _ = admin_librarian_fixture.ctrl.db.customlist( data_source_name=DataSource.LIBRARY_STAFF, num_entries=0 ) + custom_list.library = admin_librarian_fixture.ctrl.db.default_library() custom_list.add_entry(w1) custom_list.auto_update_enabled = True custom_list.auto_update_query = '{"query":"...."}' @@ -1120,6 +1121,7 @@ def test_auto_update_edit(self, admin_librarian_fixture: AdminLibrarianFixture): admin_librarian_fixture.ctrl.db.session.commit() assert isinstance(custom_list.name, str) + assert custom_list.library is not None response = admin_librarian_fixture.manager.admin_custom_lists_controller._create_or_update_list( custom_list.library, custom_list.name, diff --git a/tests/manager/api/admin/test_password_admin_authentication_provider.py b/tests/manager/api/admin/test_password_admin_authentication_provider.py index 2688afcab4..a9e4f6633f 100644 --- a/tests/manager/api/admin/test_password_admin_authentication_provider.py +++ b/tests/manager/api/admin/test_password_admin_authentication_provider.py @@ -1,7 +1,6 @@ -from unittest.mock import MagicMock, create_autospec +from unittest.mock import create_autospec import pytest -from pytest import LogCaptureFixture from palace.manager.api.admin.password_admin_authentication_provider import ( PasswordAdminAuthenticationProvider, @@ -130,20 +129,3 @@ def test_sign_in_case_insensitive( assert "ADMIN2@example.org" == admin_details.get("email") assert PasswordAdminAuthenticationProvider.NAME == admin_details.get("type") assert "foo" == redirect - - def test_send_reset_password_email( - self, - password_auth_provider: PasswordAdminAuthenticationProviderFixture, - caplog: LogCaptureFixture, - ): - password_auth = password_auth_provider.password_auth - mock_admin = MagicMock() - mock_admin.email = None - assert ( - password_auth.send_reset_password_email(mock_admin, "reset_password_url") - is None - ) - assert ( - "Admin has no email address, cannot send reset password email" - in caplog.text - ) diff --git a/tests/manager/api/controller/test_playtime_entries.py b/tests/manager/api/controller/test_playtime_entries.py index fb83419617..f94b662cf4 100644 --- a/tests/manager/api/controller/test_playtime_entries.py +++ b/tests/manager/api/controller/test_playtime_entries.py @@ -201,7 +201,7 @@ def test_track_playtime_duplicate_id_ok( library_id=library.id, identifier_str=identifier.urn, collection_name=collection.name, - library_name=library.name, + library_name=library.name or "", loan_identifier=loan_identifier, ) ) diff --git a/tests/manager/api/controller/test_work.py b/tests/manager/api/controller/test_work.py index 4a92038dec..5ca4b53c8c 100644 --- a/tests/manager/api/controller/test_work.py +++ b/tests/manager/api/controller/test_work.py @@ -29,7 +29,6 @@ from palace.manager.feed.types import WorkEntry from palace.manager.search.external_search import SortKeyPagination from palace.manager.sqlalchemy.model.datasource import DataSource -from palace.manager.sqlalchemy.model.edition import Edition from palace.manager.sqlalchemy.model.identifier import Identifier from palace.manager.sqlalchemy.model.lane import Facets, FeaturedFacets from palace.manager.sqlalchemy.model.licensing import LicensePool @@ -46,18 +45,13 @@ class WorkFixture(CirculationControllerFixture): - lp: LicensePool - identifier: Identifier - datasource: DataSource - edition: Edition - def __init__( self, db: DatabaseTransactionFixture, services_fixture: ServicesFixture ): super().__init__(db, services_fixture) [self.lp] = self.english_1.license_pools self.edition = self.lp.presentation_edition - self.datasource = self.lp.data_source.name # type: ignore + self.datasource = self.lp.data_source.name self.identifier = self.lp.identifier diff --git a/tests/manager/api/test_circulationapi.py b/tests/manager/api/test_circulationapi.py index 7c367bf1b1..27042a3990 100644 --- a/tests/manager/api/test_circulationapi.py +++ b/tests/manager/api/test_circulationapi.py @@ -38,7 +38,7 @@ from palace.manager.sqlalchemy.model.datasource import DataSource from palace.manager.sqlalchemy.model.identifier import Identifier from palace.manager.sqlalchemy.model.licensing import DeliveryMechanism, RightsStatus -from palace.manager.sqlalchemy.model.patron import Hold, Loan +from palace.manager.sqlalchemy.model.patron import Loan from palace.manager.sqlalchemy.model.resource import Hyperlink, Representation from palace.manager.util.datetime_helpers import utc_now from tests.fixtures.database import DatabaseTransactionFixture @@ -1100,53 +1100,6 @@ def patron_activity_circulation_api( class TestPatronActivityCirculationAPI: - def test_sync_patron_activity_ignores_local_loan_with_no_identifier( - self, - db: DatabaseTransactionFixture, - patron_activity_circulation_api: PatronActivityCirculationAPIFixture, - ): - loan, _ = patron_activity_circulation_api.pool.loan_to( - patron_activity_circulation_api.patron - ) - loan.start = patron_activity_circulation_api.yesterday - patron_activity_circulation_api.pool.identifier = None - - # Verify that we can sync without crashing. - patron_activity_circulation_api.sync_patron_activity() - - # The invalid loan was ignored and is still there. - loans = db.session.query(Loan).all() - assert [loan] == loans - - # Even worse - the loan has no license pool! - loan.license_pool = None - - # But we can still sync without crashing. - patron_activity_circulation_api.sync_patron_activity() - - def test_sync_patron_activity_ignores_local_hold_with_no_identifier( - self, - db: DatabaseTransactionFixture, - patron_activity_circulation_api: PatronActivityCirculationAPIFixture, - ): - hold, _ = patron_activity_circulation_api.pool.on_hold_to( - patron_activity_circulation_api.patron - ) - patron_activity_circulation_api.pool.identifier = None - - # Verify that we can sync without crashing. - patron_activity_circulation_api.sync_patron_activity() - - # The invalid hold was ignored and is still there. - holds = db.session.query(Hold).all() - assert [hold] == holds - - # Even worse - the hold has no license pool! - hold.license_pool = None - - # But we can still sync without crashing. - patron_activity_circulation_api.sync_patron_activity() - def test_sync_patron_activity_with_old_local_loan_and_no_remote_loan_deletes_local_loan( self, db: DatabaseTransactionFixture, diff --git a/tests/manager/celery/tasks/test_opds_odl.py b/tests/manager/celery/tasks/test_opds_odl.py index 500448bda3..213802ed1f 100644 --- a/tests/manager/celery/tasks/test_opds_odl.py +++ b/tests/manager/celery/tasks/test_opds_odl.py @@ -1,5 +1,4 @@ from datetime import datetime, timedelta -from typing import cast from unittest.mock import call, patch import pytest @@ -96,9 +95,7 @@ def holds( for idx in range(10) } - return cast(set[int], expired_holds), cast( - set[int], ready_non_expired_holds | not_ready_non_expired_holds - ) + return expired_holds, ready_non_expired_holds | not_ready_non_expired_holds def pool_with_licenses( self, collection: Collection, num_licenses: int = 2, available: bool = False diff --git a/tests/manager/core/test_coverage.py b/tests/manager/core/test_coverage.py index b132818423..47a4a3d175 100644 --- a/tests/manager/core/test_coverage.py +++ b/tests/manager/core/test_coverage.py @@ -1715,6 +1715,7 @@ def test_work(self, db: DatabaseTransactionFixture): # that just had to create a LicensePool using an # INTERNAL_PROCESSING DataSource rather than the DataSource # associated with the CoverageProvider. + db.session.delete(pool) identifier2 = db.identifier() identifier.licensed_through = [] collection2 = db.collection() diff --git a/tests/manager/feed/test_library_annotator.py b/tests/manager/feed/test_library_annotator.py index c0276960cf..ee73cc65ba 100644 --- a/tests/manager/feed/test_library_annotator.py +++ b/tests/manager/feed/test_library_annotator.py @@ -1089,28 +1089,6 @@ def test_loans_feed_includes_annotations_link( ] assert "/annotations" in annotations_link["href"] - def test_active_loan_feed_ignores_inconsistent_local_data( - self, annotator_fixture: LibraryAnnotatorFixture - ): - patron = annotator_fixture.db.patron() - - work1 = annotator_fixture.db.work(language="eng", with_license_pool=True) - loan, ignore = work1.license_pools[0].loan_to(patron) - work2 = annotator_fixture.db.work(language="eng", with_license_pool=True) - hold, ignore = work2.license_pools[0].on_hold_to(patron) - - # Uh-oh, our local loan data is bad. - loan.license_pool.identifier = None - - # Our local hold data is also bad. - hold.license_pool = None - - # We can still get a feed... - feed_obj = OPDSAcquisitionFeed.active_loans_for(None, patron).as_response() - - # ...but it's empty. - assert "" not in str(feed_obj) - def test_acquisition_feed_includes_license_information( self, annotator_fixture: LibraryAnnotatorFixture ): diff --git a/tests/manager/feed/test_loan_and_hold_annotator.py b/tests/manager/feed/test_loan_and_hold_annotator.py index 1ea3b987ec..cc07dfde75 100644 --- a/tests/manager/feed/test_loan_and_hold_annotator.py +++ b/tests/manager/feed/test_loan_and_hold_annotator.py @@ -9,7 +9,6 @@ from palace.manager.sqlalchemy.constants import EditionConstants, LinkRelations from palace.manager.sqlalchemy.model.lane import WorkList from palace.manager.sqlalchemy.model.licensing import LicensePool -from palace.manager.sqlalchemy.model.patron import Loan from palace.manager.sqlalchemy.util import get_one from tests.fixtures.database import DatabaseTransactionFixture from tests.fixtures.search import ExternalSearchFixtureFake @@ -113,14 +112,6 @@ def test_single_item_feed_without_work(self, db: DatabaseTransactionFixture): mock = MagicMock() # A loan without a pool annotator = LibraryLoanAndHoldAnnotator(mock, None, db.default_library()) - loan = Loan() - loan.patron = db.patron() - not_found_result = OPDSAcquisitionFeed.single_entry_loans_feed( - mock, - loan, - annotator, - ) - assert not_found_result == NOT_FOUND_ON_REMOTE work = db.work(with_license_pool=True) pool = get_one(db.session, LicensePool, work_id=work.id) diff --git a/tests/manager/feed/test_opds_acquisition_feed.py b/tests/manager/feed/test_opds_acquisition_feed.py index 1948a46ac7..6873866509 100644 --- a/tests/manager/feed/test_opds_acquisition_feed.py +++ b/tests/manager/feed/test_opds_acquisition_feed.py @@ -41,6 +41,8 @@ ) from palace.manager.sqlalchemy.model.licensing import DeliveryMechanism from palace.manager.sqlalchemy.model.resource import Representation +from palace.manager.sqlalchemy.model.work import Work +from palace.manager.sqlalchemy.util import create from palace.manager.util.datetime_helpers import utc_now from palace.manager.util.flask_util import OPDSEntryResponse, OPDSFeedResponse from palace.manager.util.opds_writer import OPDSFeed, OPDSMessage @@ -504,9 +506,7 @@ def test_error_when_work_has_no_identifier(self, db: DatabaseTransactionFixture) # We cannot create an OPDS entry for a Work that cannot be associated # with an Identifier. - work = db.work(title="Hello, World!", with_license_pool=True) - work.license_pools[0].identifier = None - work.presentation_edition.primary_identifier = None + work, _ = create(db.session, Work) with pytest.raises(PalaceValueError, match="Work has no associated identifier"): OPDSAcquisitionFeed.single_entry(work, Annotator()) diff --git a/tests/manager/scripts/test_playtime_entries.py b/tests/manager/scripts/test_playtime_entries.py index b137a641bd..c15886a359 100644 --- a/tests/manager/scripts/test_playtime_entries.py +++ b/tests/manager/scripts/test_playtime_entries.py @@ -47,7 +47,7 @@ def create_playtime_entries( total_seconds_played=entry.seconds_played, identifier_str=identifier.urn, collection_name=collection.name, - library_name=library.name, + library_name=library.name or "", loan_identifier=loan_identifier, ) db.session.add(inserted) diff --git a/tests/manager/search/test_external_search.py b/tests/manager/search/test_external_search.py index 38398828d8..efe61b0edd 100644 --- a/tests/manager/search/test_external_search.py +++ b/tests/manager/search/test_external_search.py @@ -147,11 +147,6 @@ def test_remove_work( client.indices.refresh() end_to_end_search_fixture.expect_results([], "Moby") - # If we try to remove a work with no id, we log a warning - work_no_id = Work() - index.remove_work(work_no_id) - assert "Work has no ID" in caplog.text - def test_add_document( self, end_to_end_search_fixture: EndToEndSearchFixture, diff --git a/tests/manager/sqlalchemy/model/test_customlist.py b/tests/manager/sqlalchemy/model/test_customlist.py index 36eda66d35..9c6f7d56cf 100644 --- a/tests/manager/sqlalchemy/model/test_customlist.py +++ b/tests/manager/sqlalchemy/model/test_customlist.py @@ -283,6 +283,7 @@ def test_remove_entry( assert 2 == custom_list.size # The editon's work has been scheduled for reindexing. + assert first.work is not None assert work_queue_indexing.is_queued(first.work, clear=True) # An entry is also removed if any of its equivalent editions diff --git a/tests/manager/sqlalchemy/model/test_patron.py b/tests/manager/sqlalchemy/model/test_patron.py index 00e21ae80c..449a3f10f9 100644 --- a/tests/manager/sqlalchemy/model/test_patron.py +++ b/tests/manager/sqlalchemy/model/test_patron.py @@ -312,9 +312,6 @@ def test_work(self, db: DatabaseTransactionFixture): loan, is_new = pool.loan_to(patron) assert work == loan.work - loan.license_pool = None - assert None == loan.work - # If pool.work is None but pool.edition.work is valid, we use that. loan.license_pool = pool pool.work = None @@ -334,9 +331,6 @@ def test_library(self, db: DatabaseTransactionFixture): loan, is_new = pool.loan_to(patron) assert db.default_library() == loan.library - loan.patron = None - assert None == loan.library - patron.library = db.library() loan.patron = patron assert patron.library == loan.library @@ -479,7 +473,7 @@ def test_cascade_delete(self, db: DatabaseTransactionFixture): assert len(db.session.query(Annotation).all()) == 1 # Give the patron a credential and check that it has been created - credential, is_new = create(db.session, Credential, patron=patron) + credential = db.credential(patron=patron) assert [credential] == patron.credentials assert len(db.session.query(Credential).all()) == 1 diff --git a/tests/manager/sqlalchemy/model/test_work.py b/tests/manager/sqlalchemy/model/test_work.py index d9717091c5..bd8f711b8f 100644 --- a/tests/manager/sqlalchemy/model/test_work.py +++ b/tests/manager/sqlalchemy/model/test_work.py @@ -31,6 +31,7 @@ work_library_suppressions, ) from palace.manager.sqlalchemy.util import ( + create, get_one_or_create, numericrange_to_tuple, tuple_to_numericrange, @@ -3090,28 +3091,21 @@ def test_merge_into_raises_exception_if_pwids_differ( excinfo.value ) - def test_licensepool_without_identifier_gets_no_work( - self, db: DatabaseTransactionFixture - ): - work = db.work(with_license_pool=True) - [lp] = work.license_pools - lp.identifier = None - - # Even if the LicensePool had a work before, it gets removed. - assert (None, False) == lp.calculate_work() - assert None == lp.work - def test_licensepool_without_presentation_edition_gets_no_work( self, db: DatabaseTransactionFixture ): - work = db.work(with_license_pool=True) - [lp] = work.license_pools - - # This LicensePool has no presentation edition and no way of - # getting one. - lp.presentation_edition = None - lp.identifier.primarily_identifies = [] + data_source = DataSource.lookup(db.session, DataSource.OVERDRIVE) + identifier = db.identifier() + work, _ = create(db.session, Work) + lp, _ = create( + db.session, + LicensePool, + identifier=identifier, + collection=db.default_collection(), + data_source=data_source, + work=work, + ) # Even if the LicensePool had a work before, it gets removed. - assert (None, False) == lp.calculate_work() - assert None == lp.work + assert lp.calculate_work() == (None, False) + assert lp.work is None