Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove expired ODL items #25

Merged
merged 1 commit into from
Aug 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 107 additions & 51 deletions api/odl.py
Original file line number Diff line number Diff line change
@@ -1,83 +1,73 @@
import datetime
import dateutil
import json
import uuid
from flask_babel import lazy_gettext as _
import urllib.parse
from collections import defaultdict
import flask
from flask import Response
import feedparser
from lxml import etree
from .problem_details import NO_LICENSES
from io import StringIO
import re
from uritemplate import URITemplate

import dateutil
import feedparser
import flask
from flask import url_for
from flask_babel import lazy_gettext as _
from lxml import etree
from sqlalchemy.sql.expression import or_
from uritemplate import URITemplate

from core.opds_import import (
OPDSXMLParser,
OPDSImporter,
OPDSImportMonitor,
)
from core.monitor import (
CollectionMonitor,
TimelineMonitor,
from core import util
from core.analytics import Analytics
from core.metadata_layer import (
CirculationData,
FormatData,
LicenseData,
TimestampData,
)
from core.model import (
Collection,
ConfigurationSetting,
Credential,
DataSource,
DeliveryMechanism,
Edition,
ExternalIntegration,
Hold,
Hyperlink,
Identifier,
IntegrationClient,
LicensePool,
Loan,
MediaTypes,
RightsStatus,
Session,
create,
get_one,
get_one_or_create,
Representation)
from core.monitor import (
CollectionMonitor,
IdentifierSweepMonitor)
from core.opds_import import (
OPDSXMLParser,
OPDSImporter,
OPDSImportMonitor,
)
from core.metadata_layer import (
CirculationData,
FormatData,
IdentifierData,
LicenseData,
TimestampData,
)
from .circulation import (
BaseCirculationAPI,
LoanInfo,
FulfillmentInfo,
HoldInfo,
from core.testing import (
DatabaseTest,
MockRequestsResponse,
)
from core.analytics import Analytics
from core.util.datetime_helpers import (
utc_now,
strptime_utc,
)
from core.util.http import (
HTTP,
BadResponseException,
RemoteIntegrationException,
)
from core.util.string_helpers import base64
from flask import url_for
from core.testing import (
DatabaseTest,
MockRequestsResponse,
from .circulation import (
BaseCirculationAPI,
LoanInfo,
FulfillmentInfo,
HoldInfo,
)
from .circulation_exceptions import *
from .shared_collection import BaseSharedCollectionAPI


class ODLAPI(BaseCirculationAPI, BaseSharedCollectionAPI):
"""ODL (Open Distribution to Libraries) is a specification that allows
libraries to manage their own loans and holds. It offers a deeper level
Expand Down Expand Up @@ -596,7 +586,7 @@ def update_hold_queue(self, licensepool):
Loan.end>utc_now()
)
).count()
remaining_licenses = licensepool.licenses_owned - loans_count
remaining_licenses = max(licensepool.licenses_owned - loans_count, 0)

holds = _db.query(Hold).filter(
Hold.license_pool_id==licensepool.id
Expand Down Expand Up @@ -782,6 +772,7 @@ class ODLXMLParser(OPDSXMLParser):
NAMESPACES = dict(OPDSXMLParser.NAMESPACES,
odl="http://opds-spec.org/odl")


class ODLImporter(OPDSImporter):
"""Import information and formats from an ODL feed.

Expand Down Expand Up @@ -887,8 +878,13 @@ def _detail_for_elementtree_entry(cls, parser, entry_tag, feed_url=None, do_get=
if terms:
concurrent_checkouts = subtag(terms[0], "odl:concurrent_checkouts")
expires = subtag(terms[0], "odl:expires")

if expires:
expires = dateutil.parser.parse(expires)
expires = util.datetime_helpers.to_utc(dateutil.parser.parse(expires))
now = util.datetime_helpers.utc_now()

if expires <= now:
continue

licenses_owned += int(concurrent_checkouts or 0)
licenses_available += int(available_checkouts or 0)
Expand All @@ -914,6 +910,7 @@ def _detail_for_elementtree_entry(cls, parser, entry_tag, feed_url=None, do_get=
data['circulation']['licenses_available'] = licenses_available
return data


class ODLImportMonitor(OPDSImportMonitor):
"""Import information from an ODL feed."""
PROTOCOL = ODLImporter.NAME
Expand Down Expand Up @@ -959,11 +956,12 @@ def run_once(self, progress):
progress = TimestampData(achievements=message)
return progress


class MockODLAPI(ODLAPI):
"""Mock API for tests that overrides _get and _url_for and tracks requests."""

@classmethod
def mock_collection(self, _db):
def mock_collection(cls, _db, protocol=ODLAPI.NAME):
"""Create a mock ODL collection to use in tests."""
library = DatabaseTest.make_default_library(_db)
collection, ignore = get_one_or_create(
Expand All @@ -973,7 +971,7 @@ def mock_collection(self, _db):
)
)
integration = collection.create_external_integration(
protocol=ODLAPI.NAME
protocol=protocol
)
integration.username = 'a'
integration.password = 'b'
Expand Down Expand Up @@ -1040,6 +1038,25 @@ def __init__(self, _db, collection):

self.base_url = collection.external_account_id

@staticmethod
def _parse_feed_from_response(response):
"""Parse ODL (Atom) feed from the HTTP response.

:param response: HTTP response
:type response: requests.Response

:return: Parsed ODL (Atom) feed
:rtype: dict
"""
response_content = response.content

if not isinstance(response_content, (str, bytes)):
raise ValueError("Response content must be a string or byte-encoded value")

feed = feedparser.parse(response_content)

return feed

def internal_format(self, delivery_mechanism):
"""Each consolidated copy is only available in one format, so we don't need
a mapping to internal formats.
Expand Down Expand Up @@ -1091,7 +1108,8 @@ def checkout(self, patron, pin, licensepool, internal_format):
hold_info_response = self._get(hold.external_identifier)
except RemoteIntegrationException as e:
raise CannotLoan()
feed = feedparser.parse(str(hold_info_response.content))

feed = self._parse_feed_from_response(hold_info_response)
entries = feed.get("entries")
if len(entries) < 1:
raise CannotLoan()
Expand All @@ -1117,7 +1135,8 @@ def checkout(self, patron, pin, licensepool, internal_format):
elif response.status_code == 404:
if hasattr(response, 'json') and response.json().get('type', '') == NO_LICENSES.uri:
raise NoLicenses()
feed = feedparser.parse(str(response.content))

feed = self._parse_feed_from_response(response)
entries = feed.get("entries")
if len(entries) < 1:
raise CannotLoan()
Expand Down Expand Up @@ -1181,7 +1200,8 @@ def checkin(self, patron, pin, licensepool):
raise CannotReturn()
if response.status_code == 404:
raise NotCheckedOut()
feed = feedparser.parse(str(response.content))

feed = self._parse_feed_from_response(response)
entries = feed.get("entries")
if len(entries) < 1:
raise CannotReturn()
Expand Down Expand Up @@ -1286,7 +1306,8 @@ def release_hold(self, patron, pin, licensepool):
raise CannotReleaseHold()
if response.status_code == 404:
raise NotOnHold()
feed = feedparser.parse(str(response.content))

feed = self._parse_feed_from_response(response)
entries = feed.get("entries")
if len(entries) < 1:
raise CannotReleaseHold()
Expand Down Expand Up @@ -1325,7 +1346,7 @@ def patron_activity(self, patron, pin):
if response.status_code == 404:
# 404 is returned when the loan has been deleted. Leave this loan out of the result.
continue
feed = feedparser.parse(str(response.content))
feed = self._parse_feed_from_response(response)
entries = feed.get("entries")
if len(entries) < 1:
raise CirculationException()
Expand Down Expand Up @@ -1354,7 +1375,7 @@ def patron_activity(self, patron, pin):
if response.status_code == 404:
# 404 is returned when the hold has been deleted. Leave this hold out of the result.
continue
feed = feedparser.parse(str(response.content))
feed = self._parse_feed_from_response(response)
entries = feed.get("entries")
if len(entries) < 1:
raise CirculationException()
Expand Down Expand Up @@ -1518,3 +1539,38 @@ def _get(self, url, patron=None, headers=None, allowed_response_codes=None):
self.request_args.append((patron, headers, allowed_response_codes))
response = self.responses.pop()
return HTTP._process_response(url, response, allowed_response_codes=allowed_response_codes)


class ODLExpiredItemsReaper(IdentifierSweepMonitor):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reviewing this PR for merging into the NYPL-Simplified/circulation repo. Hopefully I won't find any serious issues and my comments will be purely advisory.

I see what this is doing but the performance could be improved quite a bit. You can override SweepMonitor.item_query to put additional constraints on the query. The is_expired implementation can be represented as a set of SQLAlchemy constraints. So you should be able to make this Monitor query only the Identifiers that have a LicensePool that has expired but does not have licenses_owned and licenses_available set appropriately.

"""Responsible for removing expired ODL licenses."""

SERVICE_NAME = "ODL Expired Items Reaper"
PROTOCOL = ODLAPI.NAME

def __init__(self, _db, collection):
super(ODLExpiredItemsReaper, self).__init__(_db, collection)

def process_item(self, identifier):
for licensepool in identifier.licensed_through:
licenses_owned = licensepool.licenses_owned
licenses_available = licensepool.licenses_available

for license in licensepool.licenses:
if license.is_expired:
licenses_owned -= 1
licenses_available -= 1

if licenses_owned != licensepool.licenses_owned or licenses_available != licensepool.licenses_available:
licenses_owned = max(licenses_owned, 0)
licenses_available = max(licenses_available, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about this logic. What happens if a LicensePool has multiple licenses, and one of those licenses has expired but the others have not? I think this is going to clear out an entire LicensePool if only one of the licenses has expired. Instead I'd expect the LicensePool's totals to be recalculated based on the licenses that haven't expired.

Unlike the other stuff I've mentioned on this PR, this seems serious enough that I'd like to hear from @vbessonov and/or @tdilauro before proceeding with NYPL-Simplified/circulation#1636.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong about this code clearing out an entire LicensePool (I was misreading -= 1 as = -1). However after talking about this we agreed that this isn't accurately doing the math necessary to add up License values to get LicensePool values.

We don't actually know what the math is -- other vendors take care of it for us -- but we have a pretty good idea.


circulation_data = CirculationData(
data_source=licensepool.data_source,
primary_identifier=identifier,
licenses_owned=licenses_owned,
licenses_available=licenses_available,
licenses_reserved=licensepool.licenses_reserved,
patrons_in_hold_queue=licensepool.patrons_in_hold_queue,
)

circulation_data.apply(self._db, self.collection)
16 changes: 15 additions & 1 deletion api/odl2.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
from flask_babel import lazy_gettext as _
from webpub_manifest_parser.opds2.registry import OPDS2LinkRelationsRegistry

from api.odl import ODLAPI
from api.odl import ODLAPI, ODLExpiredItemsReaper
from core import util
from core.metadata_layer import FormatData, LicenseData
from core.model import DeliveryMechanism, Edition, MediaTypes, RightsStatus
from core.model.configuration import (
Expand Down Expand Up @@ -239,6 +240,13 @@ def _extract_publication_metadata(self, feed, publication, data_source_name):
expires = license.metadata.terms.expires
concurrent_checkouts = license.metadata.terms.concurrency

if expires:
expires = util.datetime_helpers.to_utc(expires)
now = util.datetime_helpers.utc_now()

if expires <= now:
continue

licenses_owned += int(concurrent_checkouts or 0)
licenses_available += int(available_checkouts or 0)

Expand Down Expand Up @@ -270,3 +278,9 @@ class ODL2ImportMonitor(OPDS2ImportMonitor):

PROTOCOL = ODL2Importer.NAME
SERVICE_NAME = "ODL 2.x Import Monitor"


class ODL2ExpiredItemsReaper(ODLExpiredItemsReaper):
"""Responsible for removing expired ODL licenses."""
SERVICE_NAME = "ODL 2 Expired Items Reaper"
PROTOCOL = ODL2Importer.NAME
10 changes: 10 additions & 0 deletions bin/odl2_reaper
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env python
"""Remove all expired licenses from ODL 2.x collections."""
import os
import sys
bin_dir = os.path.split(__file__)[0]
package_dir = os.path.join(bin_dir, "..")
sys.path.append(os.path.abspath(package_dir))
from core.scripts import RunCollectionMonitorScript
from api.odl2 import ODL2ExpiredItemsReaper
RunCollectionMonitorScript(ODL2ExpiredItemsReaper).run()
10 changes: 10 additions & 0 deletions bin/odl_reaper
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#!/usr/bin/env python
"""Remove all expired licenses from ODL 1.x collections."""
import os
import sys
bin_dir = os.path.split(__file__)[0]
package_dir = os.path.join(bin_dir, "..")
sys.path.append(os.path.abspath(package_dir))
from core.scripts import RunCollectionMonitorScript
from api.odl import ODLExpiredItemsReaper
RunCollectionMonitorScript(ODLExpiredItemsReaper).run()
2 changes: 1 addition & 1 deletion core
Loading