Bug 14457

Summary: Integrate LIBRIS spellchecking
Product: Koha Reporter: Eivin Giske Skaaren <eivin>
Component: SearchingAssignee: Eivin Giske Skaaren <eivin>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: bjorn.nylen, chris, dcook, genty, katrin.fischer, m.de.rooy, martin.renvoize, nick, ztajoli
Version: master   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00
Attachments: Bug 14457: Integrate LIBRIS spellchecking
Bug 14457: Integrate LIBRIS spellchecking
Bug 14457: Integrate LIBRIS spellchecking
Bug 14457 - Follow up
Bug 14457 - Follow up
Bug 14457 - QA follow up
Bug 14457: Integrate LIBRIS spellchecking
Bug 14457: (follow-up) Add syspref for Libris API URL
Bug 14457: (QA follow-up) Fix QA script complaints and amend system preference text
Bug 14457: (QA follow-up) Fix POD

Description Eivin Giske Skaaren 2015-06-24 19:15:15 UTC
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.
Comment 1 Katrin Fischer 2015-06-24 20:04:17 UTC
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.
Comment 2 Eivin Giske Skaaren 2015-06-25 11:45:24 UTC
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.
Comment 3 Eivin Giske Skaaren 2015-07-06 21:43:15 UTC
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.
Comment 4 Eivin Giske Skaaren 2015-07-06 22:29:52 UTC Comment hidden (obsolete)
Comment 5 David Cook 2015-07-07 00:50:35 UTC
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 6 David Cook 2015-07-07 00:52:10 UTC
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...
Comment 7 Eivin Giske Skaaren 2015-07-07 07:35:06 UTC
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.
Comment 8 Katrin Fischer 2015-07-07 12:00:43 UTC
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?
Comment 9 Eivin Giske Skaaren 2015-07-07 16:18:13 UTC
It was indeed the rebase. I have reviewed my original commit and those 2 changes was not part of it.
Comment 10 Eivin Giske Skaaren 2015-07-07 16:23:41 UTC Comment hidden (obsolete)
Comment 11 Zeno Tajoli 2015-09-03 08:35:45 UTC
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.
Comment 12 Katrin Fischer 2015-09-03 08:44:04 UTC
Hm does it need to? It uses the "did you mean" plugin structure. did you test?
Comment 13 Zeno Tajoli 2015-09-03 09:15:21 UTC
I do a an error on reading the code.
I reset 'Needs Signoff'.
Comment 14 Chris Cormack 2015-12-23 23:11:22 UTC
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 15 David Cook 2015-12-29 22:24:38 UTC
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?
Comment 16 Marcel de Rooy 2016-02-19 09:48:05 UTC
(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 :)
Comment 17 Marcel de Rooy 2016-02-19 10:24:11 UTC
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.
Comment 18 Katrin Fischer 2016-02-19 11:44:59 UTC
I am not sure about a preference here as it's a plugin for a specific API (LibrisSpellcheck.pm).
Comment 19 Marcel de Rooy 2016-02-19 12:56:49 UTC
(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.
Comment 20 Katrin Fischer 2016-02-19 12:59:50 UTC
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...)
Comment 21 Eivin Giske Skaaren 2016-02-19 14:59:12 UTC
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 :)
Comment 22 Marcel de Rooy 2016-02-22 10:52:40 UTC
(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.
Comment 23 Katrin Fischer 2016-02-22 10:56:49 UTC
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?
Comment 24 Chris Cormack 2016-09-29 20:53:11 UTC
Will rework to use prefs, with appropriate descriptions.
Comment 25 Chris Cormack 2016-09-29 21:49:19 UTC
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
Comment 26 lesteretsel 2016-10-13 14:19:46 UTC
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" ?
Comment 27 Katrin Fischer 2016-10-16 11:28:34 UTC
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...)?
Comment 28 Chris Cormack 2016-10-16 20:10:43 UTC
Will rework on it
Comment 29 Chris Cormack 2016-10-17 19:21:55 UTC
(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)
Comment 30 Chris Cormack 2016-10-17 19:22:23 UTC
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
Comment 31 Katrin Fischer 2017-10-15 10:24:39 UTC
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
Comment 32 Björn Nylén 2019-03-13 11:38:42 UTC
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.
Comment 33 Björn Nylén 2019-03-13 11:43:05 UTC
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.
Comment 34 Katrin Fischer 2019-04-03 21:03:45 UTC
(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!
Comment 35 Katrin Fischer 2019-04-03 21:22:42 UTC
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>
Comment 36 Katrin Fischer 2019-04-03 21:22:48 UTC
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>
Comment 37 Katrin Fischer 2019-04-03 21:22:52 UTC
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>
Comment 38 Katrin Fischer 2019-04-03 21:22:57 UTC
Created attachment 87377 [details] [review]
Bug 14457: (QA follow-up) Fix POD
Comment 39 Katrin Fischer 2019-04-03 21:23:55 UTC
I really like to see more use of the 'did you mean' structure!
Comment 40 Nick Clemens 2019-04-16 13:22:42 UTC
Awesome work all!

Pushed to master for 19.05
Comment 41 Martin Renvoize 2019-04-26 11:50:00 UTC
Enhancement will not be backported to 18.11.x series.
Comment 42 Katrin Fischer 2022-03-16 09:53:47 UTC
Are there any terms for use for this service?
Who is allowed to use it and under which conditions?