Sponsored-by: Halland County Library This enhancement will make it possible to turn on spellchecking that integrates with the LIBRIS API. It is a regional enhancement and it will be selectable by using a syspref. The enhancement will be implemented as "did you mean" that suggests keywords from the LIBRIS database depending on the search terms.
Hi Eivin, I am wondering - a 'did you mean' would be helpful in general. Could this maybe made so that you easily hook it with other sources? We already have some features called similarly that are available under Administration > Did you mean? - maybe it could also fit in there as another option.
Hi Katrin, yes I will hook it into the did you mean that is already there. I am not sure if it is easy to make a general service that can be reused with other spellchecking APIs but I will look into it.
Before I post the patch let me address the question about a general spellchecking/did you mean. There is already did you mean facilities to plug into and I also used the suggestionengine and made the spellchecker a plugin of that.
Created attachment 40816 [details] [review] Bug 14457: Integrate LIBRIS spellchecking This patch makes it possible to configure LIBRIS spellchecker as a "did you mean" feature. When searching for a word or phrase and misspelling the query will be sent to LIBRIS and if they have a suggestion it will be shown in the yellow did you mean box in the results page. The API is not very quick so this type of implementation was chosen to not disrupt the real-time feeling of the search. To test: 1. Apply the patch. 2. Go to http://api.libris.kb.se/bibspell/, enter the koha servers IP and click on the "Generera nyckel" button. 3. Under "Nyckel" you can copy the value that looks like this: E47B44829E265607274B677BC17B8D78, and enter it into the LibrisKey syspref (cgi-bin/koha/admin/preferences.pl?tab=searching). 4. In cgi-bin/koha/admin/didyoumean.pl check the box for using the LIBRIS API. It is only implemented for OPAC. 5. Perform some searches: tset - Did you mean should suggest: test jeg är på smester - suggestion: jag är på semester dantes inferna - suggestion: dantes inferno
I like this idea! Locally, Prosentient has already implemented a "Did you mean" spellcheck plugin using a pre-existing dictionary. Perhaps I should have him take a look at this bug as well... By the way, I'll provide a bit of feedback about some things I noticed at a glance.
Comment on attachment 40816 [details] [review] Bug 14457: Integrate LIBRIS spellchecking Review of attachment 40816 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-search.pl @@ +189,4 @@ > } > if ($cgi->cookie("search_path_code")) { > my $pathcode = $cgi->cookie("search_path_code"); > + given ($pathcode) We don't use the given/when construct in Koha... I also don't see why this change would be required here. It looks like it was included accidentally? @@ +578,4 @@ > # This sorts the facets into alphabetical order > if ($facets) { > foreach my $f (@$facets) { > + $f->{facets} = [ sort { uc($a->{facet_title_value}) cmp uc($b->{facet_title_value}) } @{ $f->{facets} } ]; This looks like it's outside the scope of this bug...
Hi David, I actually did most of the work several weeks ago but regarding the two issues I think that I just followed the example/plugin that was there. I will take a look at it and re-upload tonight.
The problem is that given-when is experimental and will spam the logs. It really needs to be rewritten to be an if-else-construct instead. And if there are more we should rewrite them as well. The facet change might just have slipped in by a rebase or something?
It was indeed the rebase. I have reviewed my original commit and those 2 changes was not part of it.
Created attachment 40827 [details] [review] Bug 14457: Integrate LIBRIS spellchecking This patch makes it possible to configure LIBRIS spellchecker as a "did you mean" feature. When searching for a word or phrase and misspelling the query will be sent to LIBRIS and if they have a suggestion it will be shown in the yellow did you mean box in the results page. The API is not very quick so this type of implementation was chosen to not disrupt the real-time feeling of the search. To test: 1. Apply the patch. 2. Go to http://api.libris.kb.se/bibspell/, enter the koha servers IP and click on the "Generera nyckel" button. 3. Under "Nyckel" you can copy the value that looks like this: E47B44829E265607274B677BC17B8D78, and enter it into the LibrisKey syspref (cgi-bin/koha/admin/preferences.pl?tab=searching). 4. In cgi-bin/koha/admin/didyoumean.pl check the box for using the LIBRIS API. It is only implemented for OPAC. 5. Perform some searches: tset - Did you mean should suggest: test jeg är på smester - suggestion: jag är på semester dantes inferna - suggestion: dantes inferno Restored opac-search.pl
The avaible patch doesn't modify code in opac-search.pl or opac-search.tt. So in fact doesn't operate. Probalby same line missing.
Hm does it need to? It uses the "did you mean" plugin structure. did you test?
I do a an error on reading the code. I reset 'Needs Signoff'.
Created attachment 45967 [details] [review] Bug 14457: Integrate LIBRIS spellchecking This patch makes it possible to configure LIBRIS spellchecker as a "did you mean" feature. When searching for a word or phrase and misspelling the query will be sent to LIBRIS and if they have a suggestion it will be shown in the yellow did you mean box in the results page. The API is not very quick so this type of implementation was chosen to not disrupt the real-time feeling of the search. To test: 1. Apply the patch. 2. Go to http://api.libris.kb.se/bibspell/, enter the koha servers IP and click on the "Generera nyckel" button. 3. Under "Nyckel" you can copy the value that looks like this: E47B44829E265607274B677BC17B8D78, and enter it into the LibrisKey syspref (cgi-bin/koha/admin/preferences.pl?tab=searching). 4. In cgi-bin/koha/admin/didyoumean.pl check the box for using the LIBRIS API. It is only implemented for OPAC. 5. Perform some searches: tset - Did you mean should suggest: test jeg är på smester - suggestion: jag är på semester dantes inferna - suggestion: dantes inferno Restored opac-search.pl Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Seems to work, of course I can't tell if it is giving me accurate suggestions :)
Comment on attachment 45967 [details] [review] Bug 14457: Integrate LIBRIS spellchecking Review of attachment 45967 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/searching.pref @@ +222,5 @@ > no: "don't search" > - on all variations of the ISBN. Note that this preference has no effect if UseQueryParser is on. > + API Keys: > + - > + - LIBRIS Spellcheking API key There is a typo here, but perhaps QA can fix it?
(In reply to David Cook from comment #15) > > + - LIBRIS Spellcheking API key > There is a typo here, but perhaps QA can fix it? Really nice typo at the right spot :)
QA Comment: Nice addition, Eivin. First I am stumbling over the hardcoded Swedish libris server in the code. Nothing against Sweden btw, but could we put that URL in a pref or config somewhere else? In this case you could add both prefs in one entry like some other prefs are organized (e.g. DefaultLongOverdueLostValue and DefaultLongOverdueDays ). Another small point is: Please add the new pref(s) to sysprefs.sql To test error checking, I changed the URL. Clicking on suggestions gave me: File does not exist: Can't connect to api.libris.kb.nl:80 (Bad hostname) LWP::Protocol::http::Socket: Bad hostname 'api.libris.kb.nl' Could you add some checks around line 34? What is the return value if get does not connect etc? Or when given an expired key? And in terms of security. You get $search from CGI param and directly feed it into LWP. Should we do some sanitizing here? When testing some searches, I always got only one alternative. Do you have examples where I get various results back? And the typo in spellcheking is hilarious. But please correct. Changing status to reflect need for some adjustments.
I am not sure about a preference here as it's a plugin for a specific API (LibrisSpellcheck.pm).
(In reply to Katrin Fischer from comment #18) > I am not sure about a preference here as it's a plugin for a specific API > (LibrisSpellcheck.pm). Please explain further. Plugin is read here in the context of SuggestionEngine. The word has become overloaded in Koha.
Looking at the code this seems a very specific feature, like Amazon-Covers, Syndectics, etc. Koha::SuggestionEngine::Plugin::LibrisSpellcheck; So if the URL is always the same and there is no other service this plugin works with - a configuration option doesn't seem necessary. ... and yes, we have multiples of plugins now (cataloguing, those, the plugin system...)
Hi, It seems like they have changed the url to http://api.libris.kb.se/bibspell/ so perhaps some way of changing it without committing code would be good. Please advise on the preferred solution. As Katrin already said this is a specific API for the Swedish libraries so it might not be useful for search in other languages. I also think it is very specific to what is in their catalog, it is not a generic spell-checker but a specialized one for existing library items. PS. Nice typo yes, thats what you get from coding on a laptop late at night :)
(In reply to Katrin Fischer from comment #20) > Looking at the code this seems a very specific feature, like Amazon-Covers, > Syndectics, etc. > > Koha::SuggestionEngine::Plugin::LibrisSpellcheck; The name does not tell that this is only interesting for Swedish libs. > So if the URL is always the same and there is no other service this plugin > works with - a configuration option doesn't seem necessary. The URL has changed already :) Note that e.g. Syndetics has 12 prefs or so in Koha. (In reply to Eivin Giske Skaaren from comment #21) > It seems like they have changed the url to http://api.libris.kb.se/bibspell/ > so perhaps some way of changing it without committing code would be good. > Please advise on the preferred solution. I do not really oppose a pref here. But you could use a local pref, or an "undocumented" koha-conf variable. Not ideal, since you would reference it from standard code.. The whole problem here is: How do we adjust Koha to include various language specific features (like this one)? Should they be part of the core code or not? Note that we recently had such a discussion about NorwegianPatronDB. There is no clear answer yet. For the time being, I would not oppose two prefs and toggling this feature via admin/didyoumean. Final word to the RM.
I think if a pref - better have a proper one so we know what's going on there :) The Did-you-mean-feature already has a very modulized structure, so I think adding to that should be ok for now until we can give the whole topic more thought. It won't hurt anyone and it doesn't touch core routines/templates apart from the small change in the include. Maybe we need to think about a good way to store configuration options for plugins?
Will rework to use prefs, with appropriate descriptions.
Created attachment 55948 [details] [review] Bug 14457 - Follow up Added another syspref for the url of the Libris API Added sysprefs to syspref.sql Please test the 2 patches together
When I click on "Check for suggestions" I got this error message : Can't locate object method "prefernce" via package "C4::Context" at /home/koha/src/Koha/SuggestionEngine/Plugin/LibrisSpellcheck.pm line 32. Seems like there is a mistake with the name of the method : "prefernce" instead of "preference" ?
I tried fixing the typo, but now I get: File does not exist: 400 URL must be absolute at /home/vagrant/kohaclone/Koha/SuggestionEngine/Plugin/LibrisSpellcheck.pm line 35. Maybe this is because I am testing this with kohadevbox and the example key (no server IP...)?
Will rework on it
(In reply to Katrin Fischer from comment #27) > I tried fixing the typo, but now I get: > File does not exist: 400 URL must be absolute > at /home/vagrant/kohaclone/Koha/SuggestionEngine/Plugin/LibrisSpellcheck.pm > line 35. > Maybe this is because I am testing this with kohadevbox and the example key > (no server IP...)? Hmm nope it will be because the atomic update didn't run. You need the url set in your sysprefs. You could do it manually, if you don't want to run the atomicupdate, just set that preference to http://api.libris.kb.se/bibspell/ (The preference is LibrisUrl)
Created attachment 56609 [details] [review] Bug 14457 - Follow up Added another syspref for the url of the Libris API Added sysprefs to syspref.sql Please test the 2 patches together
As you need to register the IP, I can't test on my kohadevbox. Maybe on a sandbox? QA tool doesn't complain about syspref order, but it was added to the end, maybe while you are on it move them up in the right alphabetic spot? In the system preference description it might be helfpul to add a quick description like "Swedish service for spellchecking" or something so you have a bit more of context looking at the pref and deciding if you want to turn it on. I know LIBRIS to be Swedish, but most people won't. QA test tools fail: FAIL Koha/SuggestionEngine/Plugin/LibrisSpellcheck.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'NAME' OK spelling OK valid FAIL installer/data/mysql/sysprefs.sql OK git manipulation FAIL semicolon simicolon found instead of comma at line 596 OK sysprefs_order FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/didyoumean.tt FAIL forbidden patterns forbidden pattern: tab char (line 17) forbidden pattern: tab char (line 16) OK git manipulation OK spelling OK tt_valid OK valid_template
Created attachment 86569 [details] [review] Bug 14457 - QA follow up Pod coverage for 'NAME'. Moved sysprefs in order and moved semicolon to last line. Replaced tab indentation with spaces. Added "Swedish service for spellchecking" to the syspref description.
Tried to fix the QA-stuff. Firs time so be nice. We've been using this patch in production for almost a year now so it'd be nice to have it in the community code.
(In reply to Björn Nylén from comment #33) > Tried to fix the QA-stuff. Firs time so be nice. We've been using this patch > in production for almost a year now so it'd be nice to have it in the > community code. Once you fixed things, you can switch the bug back to 'signed off' - this way it shows up in the queues for the QA team again. Sorry I missed the follow-up!
Created attachment 87374 [details] [review] Bug 14457: Integrate LIBRIS spellchecking This patch makes it possible to configure LIBRIS spellchecker as a "did you mean" feature. When searching for a word or phrase and misspelling the query will be sent to LIBRIS and if they have a suggestion it will be shown in the yellow did you mean box in the results page. The API is not very quick so this type of implementation was chosen to not disrupt the real-time feeling of the search. To test: 1. Apply the patch. 2. Go to http://api.libris.kb.se/bibspell/, enter the koha servers IP and click on the "Generera nyckel" button. 3. Under "Nyckel" you can copy the value that looks like this: E47B44829E265607274B677BC17B8D78, and enter it into the LibrisKey syspref (cgi-bin/koha/admin/preferences.pl?tab=searching). 4. In cgi-bin/koha/admin/didyoumean.pl check the box for using the LIBRIS API. It is only implemented for OPAC. 5. Perform some searches: tset - Did you mean should suggest: test jeg är på smester - suggestion: jag är på semester dantes inferna - suggestion: dantes inferno Restored opac-search.pl Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Seems to work, of course I can't tell if it is giving me accurate suggestions :) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 87375 [details] [review] Bug 14457: (follow-up) Add syspref for Libris API URL Added another syspref for the url of the Libris API Added sysprefs to syspref.sql Please test the 2 patches together Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 87376 [details] [review] Bug 14457: (QA follow-up) Fix QA script complaints and amend system preference text Pod coverage for 'NAME'. Moved sysprefs in order and moved semicolon to last line. Replaced tab indentation with spaces. Added "Swedish service for spellchecking" to the syspref description. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 87377 [details] [review] Bug 14457: (QA follow-up) Fix POD
I really like to see more use of the 'did you mean' structure!
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series.
Are there any terms for use for this service? Who is allowed to use it and under which conditions?