Remove expired ODL items by vbessonov · Pull Request #25 · ThePalaceProject/circulation

vbessonov · 2021-08-16T19:58:25Z

Description

This PR includes the following changes:

It adds additional logic skipping expired items during initial import process
It adds ODLExpiredItemsReaper and ODL2ExpiredItemsReaper classes responsible for clearing expired items on a scheduled basis.

NOTE: This PR depends on ThePalaceProject/circulation-core#19.

Motivation and Context

How Has This Been Tested?

Checklist:

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

tdilauro · 2021-08-23T20:25:29Z

api/odl.py

+        response_content = response.content
+
+        if isinstance(response_content, bytes):
+            response_content = response_content.decode("utf-8")
+        elif not isinstance(response_content, str):
+            raise ValueError("Response content must be a string or byte-encoded value")
+
+        feed = feedparser.parse(response_content)


I think feedparser.parse can handle both bytes and str. If so, it may be possible to simplify this.

Yes, you're right. I think I had some issue back in Python 2.x. Python 3.x handles both cases correctly.

tdilauro

This is looking pretty good. A couple issues with docstring cut/paste and some minor import order issues for you to have another look at.

api/odl.py

api/odl2.py

bin/odl2_reaper

bin/odl_reaper

tdilauro · 2021-08-23T21:12:21Z

api/odl.py

+                    licenses_reserved=0,
+                    patrons_in_hold_queue=0,


Any issues here if there are still licenses owned and there were patrons in the hold queue?

I decided to leave those values as is. licenses_reserved is getting updated in ODLAPI.update_hold_queue each time patrons try to checkout or checkin.

As per removing licenses when there are active loans, TestODLExpiredItemsReaper.test_odl_reaper_removes_expired_licenses ensures that the reaper can hide licenses of a license pool with an active loan.

tests/test_odl.py

tests/test_odl2.py

tdilauro · 2021-08-24T12:14:28Z

tests/test_odl.py

+        # 1.2. Ensure that the license pool was successfully created but it does not have any available licenses.
+        assert len(imported_pools) == 1


Why do we want to ensure that the license pool is created in this circumstance?

Or is this more to help document the current behavior?

As far as I understand, LicensePool is required by CM to function correctly. I just wanted to emphasise that even when CM skips expired licenses, it's still business as usual and it still creates a LicensePool object.

In a situation like this it's best to either create Edition + LicensePool + Work (with the LicensePool accurately representing the fact that the book is not currently available) or to leave the whole thing out of the database. There are use cases for creating an Edition with no LicensePool or Work, but I think creating all three makes sense in this case.

I don't believe you can have an Edition and a Work with no LicensePool; the Work will get reaped.

tests/test_odl.py

tdilauro

Looks good!

leonardr

I've reviewed this PR and will be merging it into NYPL-Simplified/circulation, with credit, with few if any changes. I'll put up a link to the corresponding NYPL-Simplified/circulation PR once I create it.

leonardr · 2021-09-02T19:46:01Z

api/odl.py

        return HTTP._process_response(url, response, allowed_response_codes=allowed_response_codes)
+
+
+class ODLExpiredItemsReaper(IdentifierSweepMonitor):


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.

leonardr · 2021-09-02T20:01:20Z

tests/test_odl.py

        path = os.path.join(cls.resource_path, filename)
-        return open(path, "rb").read()
+        return open(path, "r").read()
+


We generally read test data as binary to eliminate the chance of bytestring/string confusion, since real-world files of this type would come in as bytestrings. For JSON files it's not as much of an issue, but you've got both JSON and XML files in this test suite. Was this causing a problem?

After looking at the core changes (specifically ThePalaceProject/circulation-core#16) I see that this works because the eventual consumer of this data has been changed to handle either bytes or strings. That's a good change but since real data from HTTP responses will come in as bytes I think the test was more realistic before.

leonardr · 2021-09-02T20:11:44Z

tests/test_odl.py

+        # 1.2. Ensure that the license pool was successfully created but it does not have any available licenses.
+        assert len(imported_pools) == 1


In a situation like this it's best to either create Edition + LicensePool + Work (with the LicensePool accurately representing the fact that the book is not currently available) or to leave the whole thing out of the database. There are use cases for creating an Edition with no LicensePool or Work, but I think creating all three makes sense in this case.

I don't believe you can have an Edition and a Work with no LicensePool; the Work will get reaped.

leonardr · 2021-09-03T13:46:13Z

tests/test_odl2.py

        return None

-    def sample_opds(self, filename):
+    def sample_opds(self, filename, file_type="r"):


file_type isn't used here but as I mentioned before I'd expect sample files to be read in as bytestrings to simulate actual data coming over the wire.

leonardr · 2021-09-07T17:56:59Z

api/odl.py

+
+            if licenses_owned != licensepool.licenses_owned or licenses_available != licensepool.licenses_available:
+                licenses_owned = max(licenses_owned, 0)
+                licenses_available = max(licenses_available, 0)


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.

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.

…types (#25) * Add ability to override the list of available for import identifiers

vbessonov requested review from jonathangreen and tdilauro August 16, 2021 19:58

Base automatically changed from python3 to main August 19, 2021 20:01

vbessonov force-pushed the feature/odl-dont-show-expired-items branch from d0cfcda to 06d508a Compare August 23, 2021 08:28

vbessonov mentioned this pull request Aug 23, 2021

Move common functionality from TestOPDS2Importer to OPDS2Test ThePalaceProject/circulation-core#19

Merged

2 tasks

vbessonov changed the title ~~[WIP] Remove expired ODL items~~ Remove expired ODL items Aug 23, 2021

tdilauro reviewed Aug 23, 2021

View reviewed changes

tdilauro reviewed Aug 24, 2021

View reviewed changes

vbessonov force-pushed the feature/odl-dont-show-expired-items branch from 63b3926 to 5ebfdd3 Compare August 24, 2021 16:09

Ignore expired ODL items

32100e7

vbessonov force-pushed the feature/odl-dont-show-expired-items branch from 5ebfdd3 to 32100e7 Compare August 24, 2021 16:40

tdilauro approved these changes Aug 24, 2021

View reviewed changes

vbessonov merged commit 2a7e758 into main Aug 25, 2021

vbessonov deleted the feature/odl-dont-show-expired-items branch August 25, 2021 14:50

jonathangreen mentioned this pull request Aug 25, 2021

Update crontab with new ODL + ODL2 scripts #32

Merged

leonardr reviewed Sep 3, 2021

View reviewed changes

leonardr mentioned this pull request Sep 3, 2021

Odl palace update NYPL-Simplified/circulation#1636

Draft

2 tasks

leonardr reviewed Sep 7, 2021

View reviewed changes

jonathangreen referenced this pull request in jonathangreen/circulation Sep 8, 2021

Add ability to override the list of available for import identifiers …

631651f

…types (#25) * Add ability to override the list of available for import identifiers

jonathangreen mentioned this pull request Sep 9, 2021

ODL with multiple license blocks #39

Merged

leonardr mentioned this pull request Oct 15, 2021

Update from Palace for ODL reaper improvements NYPL-Simplified/circulation#1648

Merged

2 tasks

		# 1.2. Ensure that the license pool was successfully created but it does not have any available licenses.
		assert len(imported_pools) == 1

		return HTTP._process_response(url, response, allowed_response_codes=allowed_response_codes)


		class ODLExpiredItemsReaper(IdentifierSweepMonitor):

Conversation

vbessonov commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

tdilauro Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tdilauro left a comment

Choose a reason for hiding this comment

Uh oh!

leonardr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vbessonov commented Aug 16, 2021 •

edited

Loading

tdilauro Aug 23, 2021 •

edited

Loading