Search a consortial VuFind instance, i.e. ReShare

[maccabeelevine]

This recommendation module searches a separate instance of VuFind via its public API and links to the record and results pages hosted within that instance. The intended use case is to search and link to a consortial catalog, such as ReShare, which uses its own VuFind instance to display consortium holdings and facilitate borrowing between institutions.

• Async support

[maccabeelevine]

@demiankatz I originally wrote this as a full backend with results page, record page, etc. Then I decided that the UX was wrong, that we don't want to display a results page or record page locally, because what actually matters about both of those pages is the "Request" button to initiate a consortium borrow, and that will only show up in the external UI. So I transitioned it to a recommendation module with external links to everything.

What I didn't consider is that now I don't have the easy async support that I'd have with an AbstractSearchObject against a normal backend. So I'd have to write that wrapper separately. Does that seem like the right approach?

I suppose there is also the chance someone would actually want to ingest a full external VuFind and display the results as local records. Although in that case, they could perhaps just use Solr2 instead.

[maccabeelevine]

Another option would be to build the full backend (and use an AbstractSearchObject subclass) but not to actually build out a complete record driver, template pages, etc. I have stubs built from my first attempt, but they are I'm sure incomplete. That seems like it would mislead someone in the future.

[demiankatz]

Thanks for getting this started, @maccabeelevine -- it's definitely a bit tricky to figure out the best way to position this.

I'm not opposed to having a full ExternalVuFind backend with record drivers, etc., even if they are just stubs. That would give people something to build upon, at least, and if it makes your implementation easier here, all the better. We could make the limitations clear in the documentation and configuration files.

That being said, if you prefer not to go that route and want to stick with a more narrowly-focused solution, that's okay with me too -- we can always expand and refactor in future. If we follow that route, a couple of specific thoughts:

1.) I haven't reviewed the code, but I wonder if there's a way to make a more generic deferred recommendation module that we could utilize here. Maybe a superclass of AbstractSearchObject could be created. But maybe it's all too case-by-case for that to make any sense.

2.) I wonder if we should devise a mechanism to have multiple ExternalVuFind recommendations from different sources. Right now, we have one hard-coded .ini file and one hard-coded template... and the template uses language strings which are defined by default to lean toward the resource sharing use case. I wonder if we'd be better off specifying the ini file name as part of the recommendation module line, and then contain some heading/intro text translation strings in the ini file. Obviously, we could still set all the defaults the same way, but then we're less restricted if we need to support more varied use cases in future.

Is that any help?

I'll give this all a deeper review once we're a little more settled on the high-level approach.

[maccabeelevine]

@demiankatz Thanks for the thoughts on backend-or-no-backend. I will ponder a bit more, I go back and forth.

2.) I wonder if we should devise a mechanism to have multiple ExternalVuFind recommendations from different sources. Right now, we have one hard-coded .ini file and one hard-coded template... and the template uses language strings which are defined by default to lean toward the resource sharing use case. I wonder if we'd be better off specifying the ini file name as part of the recommendation module line, and then contain some heading/intro text translation strings in the ini file. Obviously, we could still set all the defaults the same way, but then we're less restricted if we need to support more varied use cases in future.

This part I actually planned for, there is an [ini section] param in the searches.ini definition so we can have multiple external connections defined if needed, and I figured we would expand the config in that section for things like text overrides of the translation strings. I just didn't go that far yet not having a need.

[demiankatz]

This part I actually planned for, there is an [ini section] param in the searches.ini definition so we can have multiple external connections defined if needed, and I figured we would expand the config in that section for things like text overrides of the translation strings. I just didn't go that far yet not having a need.

Makes sense! I'd just suggest, even if you're not planning on adding more text options yet, to perhaps give the existing text strings more specific key names so we can differentiate them from other versions in the future as needed (i.e. if the text refers to resource sharing, the key probably should too, so we don't end up with a general "external VuFind" key that refers to something narrower than its name would imply, potentially resulting in muddled translations).

[maccabeelevine]

If I decide to leave this as a non-backend style recommendation, maybe I should rename it ConsortialVuFind or something similar so that the limitations make sense, as well as the default text strings?

[maccabeelevine]

I decided to stick with the limited scope recommendation module, since the UX is just plain different when we intend the results to only be displayed externally. I did rename the module ConsortialVuFind to make that purpose (and the default translation values) clearer.

1.) I haven't reviewed the code, but I wonder if there's a way to make a more generic deferred recommendation module that we could utilize here. Maybe a superclass of AbstractSearchObject could be created. But maybe it's all too case-by-case for that to make any sense.

I created the Deferred version, and as it turned out, all I needed to do was extend AbstractSearchObjectDeferred, even though the recommendation doesn't extend AbstractSearchObject. It does use the 'lookfor' param which is all that really matters. I considered refactoring the abstract class, but since I actually want all of the functionality intended for ASOs, there would have to be at least three levels of the abstraction, and it seemed to make things even more confusing and silly. I added a quick note instead explaining that the extension doesn't matter. I suppose we could simply rename the current AbstractSearchObjectDeferred class, but it's correct the rest of the time. So I think maybe leave things as is.

[demiankatz]

Thanks, @maccabeelevine, that makes sense! I agree that strictly speaking, AbstractSearchObjectDeferred might benefit from a rename... but that probably causes as much disruption as benefit, so I don't mind leaving it alone for now. We can always revisit in future if it really bugs us.

I see that you still have this marked as a draft. Is there anything more you want to complete before I give this a more thorough review?

[maccabeelevine]

I see that you still have this marked as a draft. Is there anything more you want to complete before I give this a more thorough review?

No, whoops, good now.

[dltj]

I don't feel qualified to review the code, but I appreciate the effort in constructing and submitting this code to VuFind.

[demiankatz]

Thanks, @maccabeelevine -- see below for a few suggestions. It also never hurts to add unit tests for new code like this, though I won't insist upon it. :-)

[demiankatz]

Thanks, @maccabeelevine -- gave this a closer review this morning and found a few very minor things. Once those are fixed, I'll do some hands-on testing and likely get it merged!

[demiankatz]

I did some hands-on testing and found one more problem...

[demiankatz]

Thanks, @maccabeelevine!