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

Adds collection subscriptions. (PP-1875) #2287

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

tdilauro
Copy link
Contributor

@tdilauro tdilauro commented Feb 18, 2025

Description

Adds implementation for collection subscriptions.

Motivation and Context

Allow collections to become active or inactive on a schedule. (This is an addition to the existing ability to add/delete collection in real-time via the Admin UI.)

[Jira PP-1875]

How Has This Been Tested?

Checklist

  • N/A - I have updated the documentation accordingly.
  • All new and existing tests passed.

Copy link

codecov bot commented Feb 19, 2025

Codecov Report

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

Project coverage is 91.12%. Comparing base (7a9a39c) to head (d9598b9).

Files with missing lines Patch % Lines
src/palace/manager/sqlalchemy/model/collection.py 92.85% 0 Missing and 2 partials ⚠️
src/palace/manager/api/lanes.py 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2287   +/-   ##
=======================================
  Coverage   91.12%   91.12%           
=======================================
  Files         362      362           
  Lines       41368    41410   +42     
  Branches     8865     8878   +13     
=======================================
+ Hits        37696    37735   +39     
  Misses       2406     2406           
- Partials     1266     1269    +3     

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

@tdilauro tdilauro marked this pull request as ready for review February 21, 2025 20:08
@tdilauro tdilauro requested a review from a team February 21, 2025 20:22
Copy link
Member

@jonathangreen jonathangreen left a comment

Choose a reason for hiding this comment

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

I made a couple minor comments on the tests to think about, but overall looks good to me! 🚀

Comment on lines +651 to +661
@staticmethod
def make_collection_inactive(collection: Collection):
"""Make a collection inactive using some settings that will make it so."""
collection.integration_configuration.settings_dict.update(
{
"subscription_activation_date": datetime.date(2000, 12, 31),
"subscription_expiration_date": datetime.date(1999, 1, 1),
}
)
flag_modified(collection.integration_configuration, "settings_dict")
assert not collection.is_active
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Might be nice to use the methods on the integration to make this settings change, since it does a little more validation.

Suggested change
@staticmethod
def make_collection_inactive(collection: Collection):
"""Make a collection inactive using some settings that will make it so."""
collection.integration_configuration.settings_dict.update(
{
"subscription_activation_date": datetime.date(2000, 12, 31),
"subscription_expiration_date": datetime.date(1999, 1, 1),
}
)
flag_modified(collection.integration_configuration, "settings_dict")
assert not collection.is_active
def make_collection_inactive(self, collection: Collection) -> None:
"""Make a collection inactive using some settings that will make it so."""
protocol_cls = self._goal_registry_mapping[Goals.LICENSE_GOAL][collection.protocol]
protocol_cls.settings_update(
collection.integration_configuration,
{
"subscription_activation_date": datetime.date(2000, 12, 31),
"subscription_expiration_date": datetime.date(1999, 1, 1),
},
merge=True
)
assert not collection.is_active

) -> None:
[c1] = db.default_library().associated_collections
default_library = db.library(short_name="default", name="Default Library")
Copy link
Member

Choose a reason for hiding this comment

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

Minor: This could be kind of confusing. I tend to assume the "default library" is what comes back from db.default_library(). Might be helpful to either:

  • Just use db.default_library()
  • use db.library() but rename the variable so its not called default_library

sync_task.apply_async.assert_any_call(
(library.id, patron.id, loan_fixture.valid_credentials["password"]),
(collection.id, patron.id, loan_fixture.valid_credentials["password"]),
Copy link
Member

Choose a reason for hiding this comment

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

😅

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