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

Check expiration time in FOLIO token when using login-with-expiry #4186

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

meganschanz
Copy link
Contributor

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.

Copy link
Member

@demiankatz demiankatz left a 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.

@meganschanz
Copy link
Contributor Author

The reason I modified getOfflineMode was because it was called by more of the templates (like in the record page holdings tab, and from the user account pages). But the results page has the largest gain from this due to the number of ILS calls so I'm not opposed to your suggestion. Or we could return getOfflineMode back to the original scope it had and just add $this->ils()->getDriver() to the other templates as well.

@demiankatz
Copy link
Member

The reason I modified getOfflineMode was because it was called by more of the templates (like in the record page holdings tab, and from the user account pages). But the results page has the largest gain from this due to the number of ILS calls so I'm not opposed to your suggestion. Or we could return getOfflineMode back to the original scope it had and just add $this->ils()->getDriver() to the other templates as well.

Adding a $forceInitialization flag to getOfflineMode might also be an option (though I'm not sure if it's an option I like -- just brainstorming).

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. :-)

@meganschanz
Copy link
Contributor Author

The reason I modified getOfflineMode was because it was called by more of the templates (like in the record page holdings tab, and from the user account pages). But the results page has the largest gain from this due to the number of ILS calls so I'm not opposed to your suggestion. Or we could return getOfflineMode back to the original scope it had and just add $this->ils()->getDriver() to the other templates as well.

Adding a $forceInitialization flag to getOfflineMode might also be an option (though I'm not sure if it's an option I like -- just brainstorming).

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 0 reserves found was because the token was expiring and not renewing! I confirmed that in the logs of our staging environment, so I'm doing testing this morning to make sure this PR works for the command line tools too.

@meganschanz
Copy link
Contributor Author

Adding a $forceInitialization flag to getOfflineMode might also be an option (though I'm not sure if it's an option I like -- just brainstorming).
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 0 reserves found was because the token was expiring and not renewing! I confirmed that in the logs of our staging environment, so I'm doing testing this morning to make sure this PR works for the command line tools too.

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.

Copy link
Member

@demiankatz demiankatz left a 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.

module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a 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.

module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a 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. :-)

module/VuFind/src/VuFind/ILS/Driver/Folio.php Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
module/VuFind/src/VuFind/ILS/Driver/Folio.php Outdated Show resolved Hide resolved
@demiankatz
Copy link
Member

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.

@demiankatz demiankatz added this to the 11.0 milestone Jan 16, 2025
@meganschanz
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants