-
Notifications
You must be signed in to change notification settings - Fork 8
Daccess 630 tou link fix #2375
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
base: dev
Are you sure you want to change the base?
Daccess 630 tou link fix #2375
Conversation
…SS-630-tou-link-fix
…SS-630-tou-link-fix
…SS-630-tou-link-fix
…SS-630-tou-link-fix
Baroquem
left a comment
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.
Really nice refactoring job! Just needs a little more work on the token caching, I think.
|
|
||
| token = resp[:token] | ||
| ttl_secs = Integer(ENV.fetch('FOLIO_TOKEN_TTL_SECONDS', 45 * 60)) rescue 2700 | ||
| expires_at = Time.now.to_i + ttl_secs |
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.
There's a bit of a problem here. FOLIO actually provides two tokens upon successful authentication: an access token with a fairly short TTL (10 minutes is the default, though I'm not sure if that's what we have) and a refresh token that can be used to fetch a new pair of tokens without sending username and password again (details). So caching it for 45 minutes won't work. (This is the complexity I alluded to in the code comments from the first implementation -- the new expiring token mechanism.)
The CUL::FOLIO::Edge authenticate method provides the access token and the access token expiration time (as :token_exp in the response). It doesn't bother with the refresh token because we figured that all of our FOLIO API interactions in Blacklight and subcomponents are brief and discrete enough to not need an extended session. But that's a choice we could revisit if it seems advisable. For now, the expedient thing to do might be to use the :token_exp value to determine TTL.
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.
Thank you for the valuable feedback! 🙏 I'll dive deeper into this and see what I can come up with based on these details.
Jira Ticket: https://culibrary.atlassian.net/browse/DACCESS-630?atlOrigin=eyJpIjoiZjI1MjA4OWIyNGQwNDk0ZTliZThiNmZkNGEzZDY4ZjEiLCJwIjoiaiJ9
Changes
TouLookupService, including FOLIO and Solr calls, and updated cached token management.CatalogController#tou/new_touandDatabasesController#tou/new_touto use the service instead of inline Solr/FOLIO/ERM logic (and cleaning that helper logic out of the controller)./databases/new_tou/:title_id/:idflow and updated database TOU links to prefer it when atitleidis available.db_title,db_data,erm_db_result,default_rights_text,table_new_tou_result) to DRY up templates./databases/tou/:idand new/databases/new_tou/:title_id/:idpages.Example Comparison of old TOU and New TOU
New database TOU:
https://catalog-container-int.library.cornell.edu/databases/new_tou/2929539/4478166
Old Generic database TOU:
https://catalog.library.cornell.edu/databases/tou/4478166