[PP-1650] Allows SirsiDynix Horizon Auth Provider users to reset Adob… #2026
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2026 +/- ##
=======================================
Coverage 90.76% 90.76%
=======================================
Files 342 342
Lines 40512 40514 +2
Branches 8770 8770
=======================================
+ Hits 36769 36773 +4
+ Misses 2486 2485 -1
+ Partials 1257 1256 -1 ☔ View full report in Codecov by Sentry. |
dbernstein
force-pushed
the
PP-1630
branch
2 times, most recently
from
cc8f246 to
2d99505
Compare
jonathangreen
approved these changes
Sep 6, 2024
| @@ -194,22 +193,30 @@ def remote_authenticate( | |||
|
|
|||
| def remote_patron_lookup( | |||
| self, patron_or_patrondata: Patron | PatronData | |||
| ) -> None | SirsiDynixPatronData: | |||
| ) -> None | SirsiDynixPatronData | PatronData: | |||
There was a problem hiding this comment.
Since SirsiDynixPatronData is a PatronData this can be simplified to None | PatronData
| if ( | ||
| not hasattr(patron_or_patrondata, "session_token") | ||
| or patron_or_patrondata.session_token is None | ||
| ): |
There was a problem hiding this comment.
You can eliminate all the # type: ignore comments below by updating this to be:
if (
not isinstance(patron_or_patrondata, SirsiDynixPatronData)
or patron_or_patrondata.session_token is None
):
patron_or_patrondata.complete = False
return patron_or_patrondata
* Remove commented code * Add a test to cover case when remote_patron_lookup is called with Patron parameter.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…e IDs.
Description
This update brings SirsiDynix Horizon Auth Provider in line with other Auth Providers which do not require users to authenticate before resetting their Adobe IDs.
When a CM admin tries to reset a patron's the Adobe ID, the CM makes an attempt to verify that the patron exists. For SIP2 and Millenium Providers, this kind of unauthenticated query is supported For Kansas, it is not supported so the KansasAuthenticationAPI just quietly passes the patron object back to the caller. However for SirsiDynix Horizon an authenticated session is required to pull patron information.
This update therefore allows a Sirsidynix Auth Provider caller to return an incomplete PatronData object rather than no result at all. In this way, CM admins will be able to reset adobe ids for sirsidynix-using library patrons without needing the patrons credentials.
I moved this PR out of draft mode because I haven't yet heard back from SirsiDynix and I'm not holding my breath. I doubt that their API supports what we ideally want to be able to do here: namely, to retrieve some minimal user info without having to authenticate. If it turns out that we can, I can always put in a new PR to take advantage of pulling patron info without authenticating.
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-1630
How Has This Been Tested?
Tests have been updated and coverage improved.
Checklist