BrowZine: Allow disabling individual bestIntegratorLink types

[maccabeelevine]

BrowZine (libkey) responses support six types for bestIntegratorLink.

;[BestIntegratorLinks]
;fullTextFile = "PDF Full Text|browzine-pdf|https://assets.thirdiron.com/images/integrations/browzine-pdf-download-icon.svg"
;retractionNoticeUrl = "View Retraction Notice|browzine-retraction"
;expressionOfConcernNoticeUrl = "View Expression of Concern Notice|browzine-concern"
;contentLocation = "Get full text|browzine-view"
;documentDeliveryFulfillmentUrl = "Request full text|browzine-delivery"
;linkResolverOpenUrl = "full_text_options|browzine-link-resolver"

Commenting one out just causes that type of link to use the default text and icon instead. This PR allows for an individual linkType to be skipped altogether.

Use case: an institution may choose to display both the LibKey bestIntegratorLink as well as an OpenURL or other backup link. Then, if the best LibKey can do is provide the linkResolverOpenUrl link, it may be better to hide it altogether and just display the backup link.

[demiankatz]

Thanks, @maccabeelevine -- I see you have marked this as draft. Does something need to happen before this is ready to review/test?

[maccabeelevine]

Thanks, @maccabeelevine -- I see you have marked this as draft. Does something need to happen before this is ready to review/test?

@demiankatz Yeah I'm just thinking a little more about it. The IdentifierLinker spits out these links in three different places based on config

show_in_results = true      ; include in search results
show_in_record = false      ; include in core record metadata
show_in_holdings = false    ; include in holdings tab of record view

and I'm wary of creating another level of enabling/disabling when the two don't speak to each other. I.e. what if you want one type of bestIntegratorLink to appear in results but not in record view. Or based on another dimension altogether, like one backend but not another. So I'm thinking a better approach may be to mark each of these links with some class that shows its source, i.e. ".identifierlink-doi-browzine-bestIntegrator-linkResolverOpenUrl" and then just use right CSS selectors to show/hide. But I think it may depend how easy that is to do, and this (draft) solution is at least simple. So I think I will try the other first before deciding what to do with this.

[demiankatz]

Thanks for the clarification, @maccabeelevine! Please request a review from me whenever you're ready for a more careful inspection of the code.

I do agree that a CSS-based approach has some potential advantages... though we would want to think carefully about the approach so that different dimensions could be taken into account independently or in combination; i.e. maybe we need separate classes for backend and method, rather than one combined one... or maybe data elements would be a more appropriate approach.

The other question (which I can't remember the answer to without actually reviewing the code) is whether there's special behavior when no values come back from the linker -- i.e. if you rely on CSS hiding, will you get different visual/functional behavior in the scenario where all values are hidden vs. the scenario where there are no values in the first place? I assume we would want these two scenarios to be indistinguishable from the end user's perspective; I'm just not sure if there are any potential technical obstacles here. (Hopefully not -- just brainstorming!)

[maccabeelevine]

I do agree that a CSS-based approach has some potential advantages...

Trying this in #4800, either as a replacement or in addition to this PR.

[maccabeelevine]

#4800 has been merged, so the question remains whether this separate approach also has value. I think I'll let it sit for a while and see whether the CSS approach is good enough for my use cases.

[demiankatz]

Sounds good. As long as we leave it in draft mode, it won't get prematurely merged. :-)

[maccabeelevine]

This turns out to be a more useful solution than #4800 in some circumstances. Specifically, I have a use case where I display some other links based on whether IdentifierLinker is active / produces output. So that's a lot easier if I can just prevent IdentifierLinker from producing bad output in the first place, rather than looking for CSS hints.

[maccabeelevine]

The IdentifierLinker spits out these links in three different places based on config

show_in_results = true      ; include in search results
show_in_record = false      ; include in core record metadata
show_in_holdings = false    ; include in holdings tab of record view

and I'm wary of creating another level of enabling/disabling when the two don't speak to each other.

This is still a concern, except now (after #4800) we already have two ways of enabling/disabling identifier links, and this adds a third. :) But I'm not able to imagine a scenario where I would want one type if bestIntegratorLink to appear in one context (i.e. record page) but not another.

[demiankatz]

Thanks, @maccabeelevine -- see further discussion below!

[demiankatz]

Might a simpler (and slightly more robust) approach here be to eliminate the if statement and change the next line to something like:

$parts = explode('|', ((string)$configLine) . '||');

[maccabeelevine]

Maybe it's me but I find that change more confusing to parse, and it hides the purpose (i.e. handling an empty string). I admit though my solution is hacky. I rewrote it to maybe be cleaner.

[demiankatz]

I can't remember what this looked like before, but it looks fine to me now. The only question is a pre-existing one: the code currently assumes that $configLine will contain at least one | character, or else $parts[1] will throw an undefined index notice. Should we add a ?? fallback there to be on the safe side in case of misconfiguration, or should we ignore the value entirely if it has fewer than two values?

[maccabeelevine]

I went with option two, ignoring it completely, since that's what the .ini file implies should happen -- even though the template does technically support the null icon scenario.

[demiankatz]

I've been tracing through the code trying to figure out how empty link and label values will be handled, and I couldn't find any logic for dealing with them. Is this actually getting skipped/excluded or are we generating empty links in the markup? (I should know the answer to this since I wrote much of this code, but it's been long enough that I can't remember...)

[maccabeelevine]

I thought I had tested that, but it turns out nope! Was displaying some empty HTML. Fixed now with some more explicit handling.

[demiankatz]

Thanks, @maccabeelevine -- see below for a couple of new comments/questions.

[demiankatz]

I can't remember what this looked like before, but it looks fine to me now. The only question is a pre-existing one: the code currently assumes that $configLine will contain at least one | character, or else $parts[1] will throw an undefined index notice. Should we add a ?? fallback there to be on the safe side in case of misconfiguration, or should we ignore the value entirely if it has fewer than two values?

[demiankatz]

You allow the expected response to be null, but no test cases take advantage of this possibility. Is that intended/expected? Do we actually need the nullable type? If so, should we create a test case to exercise it? If not, should we revert that part of the change?

[maccabeelevine]

Thanks, I reverted the change. I think at one point I did expect null, but now it would be an empty array.