Skip to content

Update from Palace for ODL reaper improvements#1648

Merged
nballenger merged 3 commits intodevelopfrom
bugfix/SIMPLY-3868
Feb 2, 2022
Merged

Update from Palace for ODL reaper improvements#1648
nballenger merged 3 commits intodevelopfrom
bugfix/SIMPLY-3868

Conversation

@leonardr
Copy link
Contributor

@leonardr leonardr commented Oct 15, 2021

Description

This branch copies in code from the Palace project -- specifically these two PRs 1 2 -- to address https://jira.nypl.org/browse/SIMPLY-3868.

There are two main goals:

  1. Add a reaper for ODL licenses, so that books disappear from the collection once all loans have been used up.
  2. Change the ODL importer so that books aren't imported in the first place if there aren't actually any licenses.

Motivation and Context

In my review of the original branch I noticed that the algorithm for determining whether an ODL license had expired only handled the most simple use cases, so Tim and Viacheslav from Lyrasis added some more complex test cases and make them work.

To make this particular PR I just copied the api/odl.py and tests/test_odl.py files over from the second Palace PR. We haven't done any development on those files for a long time, so there should have been no divergence. The Palace repo has a lot of new code in odl2.py and test_odl2.py, but that's for the JSON flavor of ODL based on OPDS 2, which is a new flavor of ODL that we don't currently support.

How Has This Been Tested?

Checklist:

  • I have updated the documentation accordingly.
  • All new and existing tests passed.


# 0 is a starting point,
# we're going through all the valid licenses in the pool and count up available checkouts.
for license_pool_license in licensepool.licenses:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main addition to the second PR. Previously we were assuming that a LicensePool had only a single license.

metadata_client = MockMetadataClient()

warrior_time_limited = dict(checkouts=dict(available=1))
warrior_time_limited = dict(checkouts=dict(left=52, available=1))
Copy link
Contributor Author

@leonardr leonardr Oct 15, 2021

Choose a reason for hiding this comment

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

This sets up a distinction that wasn't being handled properly before, between 'left' or 'licenses_owned' (the number of loans we can issue for this book before we run out) and 'available' or 'licenses_available' (the number of simultaneous loans that can be out on this book before we start having to put people in the holds queue).

assert ([RightsStatus.IN_COPYRIGHT, RightsStatus.IN_COPYRIGHT] ==
[lpdm.rights_status.uri for lpdm in lpdms])
assert 2 == midnight_pool.licenses_owned
assert 72 == midnight_pool.licenses_owned # 20 + 52 remaining checkouts in corresponding License Info Documents
Copy link
Contributor Author

@leonardr leonardr Oct 15, 2021

Choose a reason for hiding this comment

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

And this shows what ought to happen when a LicensePool has two different licenses.

template = env.get_template(self.ODL_TEMPLATE_FILENAME)
feed = template.render(licenses=licenses)

return feed
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uses a Jinja2 template to generate an ODL feed for whatever licenses you want to test. This way you can create the License objects for your use case and generate the feed, without having to pregenerate a separate feed for each use case.

This isn't how we've done this kind of testing up to this point, but it does make it easier to test a large number of slight variant files.

"remaining_checkouts_is_zero",
TestLicense(total_checkouts=10, concurrent_checkouts=5),
TestLicenseInfo(remaining_checkouts=0, available_concurrent_checkouts=0)
)
Copy link
Contributor Author

@leonardr leonardr Oct 15, 2021

Choose a reason for hiding this comment

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

So at this point we can just list all the ways a license might be ignorable, and verify the importer ignores them.

@leonardr leonardr requested a review from nballenger October 15, 2021 19:28
@leonardr
Copy link
Contributor Author

Running a real test of this is pretty difficult for me since we don't have a real ODL collection that we make use of over time. However we do have a small test collection, and running an import from that collection I got a log message that showed the new parse_license code in action:

License # urn:uuid:5371d8e8-3e45-11ea-a2da-a26cdbaff453 expired since the remaining number of checkouts is 0

I then ran the reaper, and it processed each book but didn't change anything because the one book that should have been reaped (the one with no remaining checkouts) wasn't imported in the first place.

@nballenger nballenger merged commit b015127 into develop Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants