-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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. |
There was a problem hiding this 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! 🚀
@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 |
There was a problem hiding this comment.
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.
@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") |
There was a problem hiding this comment.
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 calleddefault_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"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
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