-
Notifications
You must be signed in to change notification settings - Fork 359
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
Check expiration time in FOLIO token when using login-with-expiry #4186
base: dev
Are you sure you want to change the base?
Conversation
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.
Thanks, @meganschanz! I haven't looked closely at the FOLIO parts yet, but thought it would be best to start by reviewing the framing changes you made. This all makes sense to me in principle; my question is just whether it's actually necessary to change the behavior of getOfflineMode, or if it would be easier to simply directly call $this->ils()->getDriver()
in the results template to ensure the driver is initialized without doing the extra offline mode stuff like the health check, which I don't think is necessary in this context. If the offline mode changes are necessary for some reason I'm overlooking, I'm certainly open to adjustments there -- but just checking to see if we can make this simpler and more self-contained.
The reason I modified |
Adding a Maybe for now, if you don't object, we could go with my simplified proposal since that gives us the biggest win with the least disruption... and then we could potentially have a separate follow-up PR to more universally apply the change. That way we can get this wrapped up more quickly, and also avoid mixing uncontroversial and potentially-more-controversial changes into the same commit. :-) |
Sounds good. I'll make a commit for that shortly. I'm testing course reserves importing with the new code -- I had an epiphany last night that the reason we occasionally were getting |
… having getOfflineMode do it
I just made that change. Also I had success with my testing of the reserves import! It renewed the token when it expired with the changes in this PR. Likely doesn't affect too many people (only those who have so many reserves the token expires before you can get them all), but a nice fix none the less. |
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.
Thanks again, @meganschanz! I've now taken a closer look at the FOLIO driver itself, and have a few more thoughts and questions.
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.
Another round of careful review of the FOLIO driver changes gave me a few more ideas (and revealed a couple of nitpicks I overlooked the first time). In any case, this is all looking very good, and most of these suggestions are optional/preference-based, but there's at least one that may be important.
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.
Thanks, @meganschanz, this keeps getting better! A few more minor polishing suggestions below, mostly about not losing comments that might be helpful in future. :-)
Thanks, @meganschanz, I think the FOLIO driver changes are now as polished as they can get! Before I approve and merge this, though, I think there are a couple more things to do: 1.) We can post this on the vufind-folio-integrations channel in Slack to solicit more testing/review. Would you like to do that? If not, I'm happy to -- just a question of whether it's more convenient for you to receive replies directly. 2.) I'd be interested in more opinions regarding the initialization of the ILS driver on the search results page, since this is clearly beneficial for FOLIO but might just introduce unnecessary overhead for other systems. Maybe we need a config setting to control this behavior; if so, we also need to discuss the most appropriate default value for the setting. We could discuss this on the next Community Call. I'll give this an 11.0 milestone to be sure it stays on the radar. |
Good idea! I've posted this in that channel asking for feedback on this PR. I've added it to my calendar to attend the next community call on February 4th to be around for the discussion. |
The call to the new authentication method in FOLIO was added a while back (
/authn/login-with-expiry
vs/authn/login
) but VuFind was not making use of the expiration time that was added with the new method. Using that, we can determine if the token is still valid without making an API call to check and save some time.While working on this, I realized that the token was not being written to the session cache at least on search result pages, where the most ILS calls happen, due to all the calls happening via AJAX -- which can't write to the session, only read from it. So I've made some updates to have it load the ILS driver when the search result page is loaded which will get the authentication token, if available for that driver, and save it to the session if it doesn't already exist. This slows down the initial page request slightly for the first search that session does, but speeds up all the subsequent AJAX calls which can now just use that token without having to request a new one for each call. I'm open to suggestions if there is a better way to accomplish this.
In my local testing, the initial page load slowed by about 100 ms (then returned to it's prior speed on subsequent searches by that session), and AJAX calls to
getItemStatuses
got about 1 second faster on average.