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

Update ODL hold calculation logic (PP-1728) #2117

Merged
merged 9 commits into from
Oct 18, 2024

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Oct 16, 2024

Description

Completely re-work how we are calculating the hold queue for OPDS2 + ODL items.

All the work to calculate the holds is moved out to live in a Celery task. When a hold is invoked through the API the hold is always placed in the queue, and the queue is later recalculated by the celery task.

Motivation and Context

Previously the holds calculation was based on the number of loans that this CM had for an item. This is a problem is a loan gets lost, or if the collection is added to multiple CM. Now we use the number of licenses available, which should be safer and less error prone. However it may still be incorrect if the collection is on multiple CM, as these CM will be fighting for the same hold.

How Has This Been Tested?

  • Did some minor local testing
  • Unit tests

Checklist

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

@jonathangreen jonathangreen added the bug Something isn't working label Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 98.43750% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.72%. Comparing base (7d44fa4) to head (ea6f173).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/palace/manager/api/odl/api.py 93.33% 1 Missing and 1 partial ⚠️
src/palace/manager/celery/tasks/opds_odl.py 98.95% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2117   +/-   ##
=======================================
  Coverage   90.72%   90.72%           
=======================================
  Files         351      351           
  Lines       40878    40886    +8     
  Branches     8853     8871   +18     
=======================================
+ Hits        37085    37093    +8     
  Misses       2486     2486           
  Partials     1307     1307           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from chore/refactor-loan-hold-info to main October 16, 2024 20:21
@jonathangreen jonathangreen force-pushed the bugfix/odl-hold-calculation branch from a4928ee to 820376e Compare October 17, 2024 00:20
@jonathangreen jonathangreen marked this pull request as ready for review October 17, 2024 17:45
@jonathangreen jonathangreen requested a review from a team October 17, 2024 18:05
Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

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

Whoa: big PR. Looks good to the extent that I fully grocked it. Way to move another task out of the scripts and into celery.

@jonathangreen jonathangreen merged commit 801f8af into main Oct 18, 2024
20 checks passed
@jonathangreen jonathangreen deleted the bugfix/odl-hold-calculation branch October 18, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants