Add recommendation module for LibGuides Profiles #2977
Conversation
|
Looks like a good start, @maccabeelevine; let me know when you're ready for a full review. And don't forget to put in a comment in searches.ini to describe the new module. :-) |
Thanks for the reminder @demiankatz, fixed that. This is ready for an initial review. See all my comments of course, there are things left to do. But I'd appreciate your input at this point on the general approach, and the light refactoring of OverdriveConnector, before I go further. |
There was a problem hiding this comment.
Thanks again, @maccabeelevine -- I can't test this, but I've given it a pretty thorough readthrough and left some comments/suggestions.
|
@demiankatz This is maybe actually ready now. |
There was a problem hiding this comment.
Definitely getting close to done, though a few (mostly nitpicky) comments.
Beyond that, the only other thing to consider is whether test coverage would be feasible. On the one hand, I suspect it would be a bit of a headache to collect all the necessary fixtures and put things together, so I'm sympathetic if you don't think it's worth the effort. On the other hand, this is something I can't personally test, so having some automated test coverage would give me greater confidence that it won't get broken by future refactoring.
config/vufind/LibGuidesAPI.ini
Outdated
|
|
||
| ; Calls to GET (...)/accounts endpoint | ||
| [GetAccounts] | ||
| ; Duration (seconds) to cache repsonse data. Default is 600. |
| * | ||
| * @return array An array containing the idToAccount and subjectToId maps | ||
| */ | ||
| protected function loadDataIfNeeded() |
There was a problem hiding this comment.
Nitpick, but I don't really like the method name loadDataIfNeeded -- that describes an internal implementation detail that the caller doesn't necessarily need to worry about. Maybe I'd call this one getLibGuidesData, since that's the one the user definitely wants to call, and then rename loadData to populateLibGuidesCache or something like that, to make it clear this is the primary entry point, and that is support for this.
There was a problem hiding this comment.
Thank you, I hated those two names and tried various options but couldn't come up with something reasonable. I actually had "maybeLoadData()" at one point. Your suggestions are much better.
| @@ -32,7 +32,7 @@ | |||
| /** | |||
| * Interface for classes using OauthServiceTrait. | |||
| * | |||
| * Classes which use this trait should also use LoggerAwareTrait. | |||
| * Classes which use this trait should also implement LoggerAwareInterface. | |||
There was a problem hiding this comment.
"this trait" --> "this interface"?
Yes I agree it's a good idea. Have to see which parts are effectively unit testable. |
There was a problem hiding this comment.
Thanks, @maccabeelevine, I think this is in good shape. I won't merge it yet in case you'd like to add tests here first, but if you prefer to merge this and do tests in a separate PR, that's fine too.
You might find it helpful to look at the json_log_file setting in the AbstractAPI ILS driver code (
) for a possible way to capture fixtures... and the FolioTest offers an example of how these can be used.
|
Thanks. I'm writing tests now and I can see they will be some work so I will do them separately. |
A LibGuides Profile page contains information about a LibGuides user. In practice it's for a librarian who created one or more subject guides. From https://ask.springshare.com/libapps/faq/638
This recommendation module displays the photo and basic information about a librarian who would be relevant based on the search query. To be specific:
I borrowed the connection logic (including OAuth token negotiation) from OverdriveConnector. I did refactor it into a new reusable trait, but I didn't touch the original.
TODO