Bug 36805 - Separate OPACSearchForTitleIn content from markup
Summary: Separate OPACSearchForTitleIn content from markup
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-07 17:25 UTC by Owen Leonard
Modified: 2024-08-22 02:15 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 36805: (proof of concept) Markup-free OPACSearchForTitleIn implementation (1.77 KB, patch)
2024-07-19 15:29 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2024-05-07 17:25:45 UTC
The OPACSearchForTitleIn system preference expects the user to be able to enter their list of links using the correct markup for them to be displayed in the "More searches" Bootstrap dropdown. As Bug 35844 shows, this content can get out of date with Bootstrap upgrades.

I think we might want to separate the link content from the markup. My suggestion is that we ask the user to enter each link on one line, with URL pipe-separated from link text, e.g.:

https://worldcat.org/search?q={TITLE}|Other Libraries (WorldCat)

We can parse that in the template:

[% FOREACH line IN OPACSearchForTitleIn.split('\n') %]
   [% part = line.split('\|') %]
   <a class="dropdown-item" href="[% part.first | html %]" target="_blank">[% part.last | html %]</a>
[% END %]

Does that sound reasonable?
Comment 1 Katrin Fischer 2024-05-07 18:19:40 UTC
It does... but then I know it will break some functionality that we have used OpacSearchForTitleIn for. It allows you to add other information at the moment using HTML/jQuery and the placeholders have proven useful to add the for example the Controlnumber in a parseable way. 

Maybe it would be nice to have another element that also allows to use the available placeholders/variables.

A migration path might also be tricky?
Comment 2 Owen Leonard 2024-07-19 15:29:31 UTC
Created attachment 169242 [details] [review]
Bug 36805: (proof of concept) Markup-free OPACSearchForTitleIn implementation

The updated default contents of OPACSearchForTitleIn would look like
this:

https://worldcat.org/search?q={TITLE}|Other Libraries (WorldCat)
https://scholar.google.com/scholar?q={TITLE}|Other Databases (Google Scholar)
https://www.bookfinder.com/search/?author={AUTHOR}&amp;title={TITLE}&amp;st=xl&amp;ac=qr|Online Stores (Bookfinder.com)
http://openlibrary.org/search/?author=({AUTHOR})&amp;title=({TITLE})|Open Library (openlibrary.org)
Comment 3 Owen Leonard 2024-07-19 15:32:36 UTC
(In reply to Katrin Fischer from comment #1)
> I know it will break some functionality that we have
> used OpacSearchForTitleIn for.

I've attached a proof of concept to test if you're interested. I'm not sure I understand what functionality it would break.

> A migration path might also be tricky?

Yes, and I haven't tried to figure it out. I'm not sure we can do anything automatically except maybe update the contents of the preference if it matches the default.
Comment 4 Katrin Fischer 2024-07-29 09:18:21 UTC
I don't want to block it because of our strange use case, but to explain: We have used it like a HTML customization basically, that is then moved into the right position using jQuery. As we use the CONTROLNUMBER and ISBN placeholders, it's not an easy move to a jQuery only solution.

It would be nice if we had a HTML custumization with the ability to use this placeholders available on the datail page.

Apart from "our problem" I think we need to think carefully how to communicate a possibly breaking change like this in a good way.
Comment 5 David Cook 2024-08-19 02:09:39 UTC
I noticed someone in bug 37573 using OPACSearchForTitleIn to inject <script> tags, which seems quite suboptimal to me.

Sure, if you have access to update OPACSearchForTitleIn you'd also have access to OpacUserJS, so it's not a security vulnerability at this stage, but... as Owen says... I think separating functionality here is a good idea.
Comment 6 David Cook 2024-08-19 02:14:29 UTC
(In reply to Katrin Fischer from comment #4)
> Apart from "our problem" I think we need to think carefully how to
> communicate a possibly breaking change like this in a good way.

At some stage, I'd love to just do 1 big breaking change where we say "all Javascript must be done via this one mechanism" and "all HTML inputs will now be scrubbed and limited to X, Y, Z elements/attributes".

It'll be unpleasant as an upgrade, but from a security perspective it would be wise.

Koha often errs on the side of convenience over security, and one day it may well come back to bite all of us.

--

I figure breaking changes are OK for libraries with support, because they can get it fixed rapidly. They're harder for libraries without support, but... that's one of the reasons you have support, so that there is someone there to support you through changes.

--

I think Owen's idea is a very good one. @Katrin, maybe for your use case we need to add a new HTML Customization that helps you out?
Comment 7 Katrin Fischer 2024-08-19 07:46:18 UTC
> 
> I think Owen's idea is a very good one. @Katrin, maybe for your use case we
> need to add a new HTML Customization that helps you out?

The main problem is that the other HTML customizations don't offer a way to add {CONTROLNUMBER}. We actually don't add JavaScript in there, just a hidden HTML element with that information in a parseable way.

I could solve that via XSLT knowing in advance, maybe that's the easier way out of this.

Having an area somewhere on the detail page for a custom HTML customization might still be a nice thing to have. I filed:
Bug 37668 - Add HTML customization to OPAC detail page
Comment 8 Owen Leonard 2024-08-19 11:15:30 UTC
What if we added data attributes to the dropdown wrapper with the available placeholder data?

<div id="moresearches_menu" data-author={} data-title={} etc. >
Comment 9 Katrin Fischer 2024-08-19 11:21:21 UTC
Hm, I think adding them might be nice, but if we change the way we do the OPACSearchForTitleIn, maybe another place would be more logical? I might misunderstand things!
Comment 10 David Cook 2024-08-20 01:33:04 UTC
(In reply to Katrin Fischer from comment #7)
> I could solve that via XSLT knowing in advance, maybe that's the easier way
> out of this.

What do you mean by this?

I've been doing XSLT magic lately so I might be able to help you out there...
Comment 11 Katrin Fischer 2024-08-20 07:45:42 UTC
(In reply to David Cook from comment #10)
> (In reply to Katrin Fischer from comment #7)
> > I could solve that via XSLT knowing in advance, maybe that's the easier way
> > out of this.
> 
> What do you mean by this?
> 
> I've been doing XSLT magic lately so I might be able to help you out there...

We use our own XSLT files for the detail page. My XSLT foo is OK for moving the invisible tag containing the 001 info into them for our specific use case. I am a bit weary of making those changes because it's used to integrate with a third party software, but it will just need some preparation/communication in the end.

So taking our use case out of consideration - how can we communicate to libraries to adjust their settings after an update? Can we store the current settings somehow so they see what needs to be updated? 

... Could we even attempt to parse the links and create the new structure?

Probably to hard with SQL, but with Perl it might work?
Comment 12 David Cook 2024-08-21 00:40:10 UTC
(In reply to Katrin Fischer from comment #11)
> So taking our use case out of consideration - how can we communicate to
> libraries to adjust their settings after an update? Can we store the current
> settings somehow so they see what needs to be updated? 
> 
> ... Could we even attempt to parse the links and create the new structure?
> 
> Probably to hard with SQL, but with Perl it might work?

Hmm that's a good point... I think it might be too hard. We might actually need to create a new system preference. We could remove the old one from the .pref file so that it just appears in Local Preferences, so that the data isn't lost...
Comment 13 Katrin Fischer 2024-08-21 12:36:00 UTC
I wonder how hard it would be to just parse the a tag. Getting a list of URLs from the href could be OK to do, description from the tags might be a little harder. But it could be worth while (saying as someone supporting over a 100 libraries... but admittedly can't provide a patch).
Comment 14 David Cook 2024-08-22 02:15:38 UTC
(In reply to Katrin Fischer from comment #13)
> I wonder how hard it would be to just parse the a tag. Getting a list of
> URLs from the href could be OK to do, description from the tags might be a
> little harder. But it could be worth while (saying as someone supporting
> over a 100 libraries... but admittedly can't provide a patch).

Assuming well-formed HTML, a person probably could do a regex to just grab the "a" elements out of the current data. But that person also isn't going to be me. Personally, my goal for this preference is to just prevent security problems.