Bug 8995 - Show OpenURL links in OPAC search results
Summary: Show OpenURL links in OPAC search results
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact:
URL:
Whiteboard:
Keywords: rel_17_11_candidate
Depends on:
Blocks:
 
Reported: 2012-10-31 13:48 UTC by Julian Maurice
Modified: 2017-10-13 17:05 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 8995: Show OpenURL links in OPAC search results (20.07 KB, patch)
2012-10-31 13:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (35.08 KB, patch)
2013-11-06 15:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (35.04 KB, patch)
2013-12-18 11:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Use uri_escape_utf8 instead of uri_escape (709 bytes, patch)
2013-12-18 11:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in bootstrap theme (5.23 KB, patch)
2013-12-18 11:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (35.07 KB, patch)
2014-01-09 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Use uri_escape_utf8 instead of uri_escape (709 bytes, patch)
2014-01-09 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in bootstrap theme (5.23 KB, patch)
2014-01-09 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (35.05 KB, patch)
2014-04-08 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Use uri_escape_utf8 instead of uri_escape (709 bytes, patch)
2014-04-08 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in bootstrap theme (5.06 KB, patch)
2014-04-08 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (35.21 KB, patch)
2014-06-30 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Use uri_escape_utf8 instead of uri_escape (707 bytes, patch)
2014-06-30 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in bootstrap theme (5.06 KB, patch)
2014-06-30 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (35.21 KB, patch)
2014-07-31 09:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Use uri_escape_utf8 instead of uri_escape (709 bytes, patch)
2014-07-31 09:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in bootstrap theme (5.06 KB, patch)
2014-07-31 09:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Add OpenURL link to OPAC bootstrap XSLT files (11.99 KB, patch)
2014-07-31 09:31 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Change meaning and name of syspref OPACHideOpenURLForItemTypes (5.93 KB, patch)
2014-07-31 09:32 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Consider OPACURLOpenInNewWindow for OpenURL links (6.45 KB, patch)
2014-07-31 10:07 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (37.91 KB, patch)
2015-02-09 12:22 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results (38.86 KB, patch)
2015-02-13 16:39 UTC, Paola Rossi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results (121.91 KB, patch)
2015-06-26 07:41 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results (121.90 KB, patch)
2015-11-09 10:59 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (31.87 KB, patch)
2016-02-25 15:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Update opac.css (80.82 KB, patch)
2016-02-25 15:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (31.90 KB, patch)
2016-09-02 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Update opac.css (80.75 KB, patch)
2016-09-02 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Remove useless code (2.02 KB, patch)
2016-09-02 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Fix URL generation when the URL in syspref has a query part (8.49 KB, patch)
2016-09-02 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Tell about the new sysprefs in COinSinOPACResults description (1.29 KB, patch)
2016-09-02 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Fix and simplify usage of syspref OpenURLImageLocation (7.92 KB, patch)
2016-09-02 14:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Add IGNORE to the INSERT statement (1.09 KB, patch)
2016-09-28 12:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (31.98 KB, patch)
2017-06-22 07:55 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Update opac.css (81.87 KB, patch)
2017-06-22 07:55 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Remove useless code (2.03 KB, patch)
2017-06-22 07:55 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Fix URL generation when the URL in syspref has a query part (8.53 KB, patch)
2017-06-22 07:55 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Tell about the new sysprefs in COinSinOPACResults description (1.29 KB, patch)
2017-06-22 07:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Fix and simplify usage of syspref OpenURLImageLocation (7.95 KB, patch)
2017-06-22 07:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Add IGNORE to the INSERT statement (1.10 KB, patch)
2017-06-22 07:56 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 8995: Show OpenURL links in OPAC search results (31.96 KB, patch)
2017-06-22 09:01 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 8995: Update opac.css (81.91 KB, patch)
2017-06-22 09:01 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 8995: Remove useless code (2.07 KB, patch)
2017-06-22 09:01 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 8995: Fix URL generation when the URL in syspref has a query part (8.54 KB, patch)
2017-06-22 09:01 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 8995: Tell about the new sysprefs in COinSinOPACResults description (1.34 KB, patch)
2017-06-22 09:02 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 8995: Fix and simplify usage of syspref OpenURLImageLocation (7.97 KB, patch)
2017-06-22 09:02 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 8995: Add IGNORE to the INSERT statement (1.14 KB, patch)
2017-06-22 09:02 UTC, Magnus Enger
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2012-10-31 13:48:48 UTC

    
Comment 1 Julian Maurice 2012-10-31 13:49:48 UTC Comment hidden (obsolete)
Comment 2 claire.hernandez@biblibre.com 2012-11-07 13:31:46 UTC
We are currently working on this patch, please do not consider this submission for the moment
Comment 3 Julian Maurice 2013-11-06 15:05:23 UTC Comment hidden (obsolete)
Comment 4 mathieu saby 2013-11-17 14:47:47 UTC
Sponsored by Rennes 2 university.
This patch also clean the date taken from 210 field, because openurl standard does not accept dates like "impr. 2010" or "cop 2009". It needs only 2010 or 2009.

I'm testing it.

M. Saby
Comment 5 mathieu saby 2013-11-17 15:07:26 UTC
Sorry, Software error:

Can't escape \x{0152}, try uri_escape_utf8() instead at /home/msaby/kohamaster/src/C4/Biblio.pm line 1452.

even with new syspref disabled. I suppose it is linked to the refactoring of COins.


2 questions : 
- now that we have a bootstrap opac defined as new default opac, does this patch needs a followup to work with bootstrap theme as well as with prog theme?

- as the changes made on building COins could potentially have effects even if the new syspref is not activated (cf my software error...), I think we need a little regression test plan, to be sure Coins are still build correctly for unimarc and marc21 (I've seen things about 773 field, but I don't know what is it for). I'm not sure of the best way to do that : I suppose some FF extensions like OpenURLReferrer could be usefull for that, but I've not tested it recently...

Mathieu
Comment 6 Julian Maurice 2013-12-18 11:01:44 UTC Comment hidden (obsolete)
Comment 7 Julian Maurice 2013-12-18 11:01:53 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2013-12-18 11:02:03 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2013-12-18 11:03:45 UTC
1st patch rebased on master
2nd patch fix the "Can't escape \x{0152}" error
3rd patch update bootstrap theme to display OpenURL links

All patches needs a signoff
Comment 10 Jonathan Druart 2014-01-09 14:08:18 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-01-09 14:08:26 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-01-09 14:08:33 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-01-09 14:08:48 UTC
rebased patches.
Comment 14 Holger Meißner 2014-03-31 08:34:24 UTC
Applying: Bug 8995: Show OpenURL links in OPAC search results
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
Auto-merging opac/opac-detail.pl
Auto-merging koha-tmpl/opac-tmpl/prog/en/xslt/UNIMARCslim2OPACResults.xsl
Auto-merging koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt
Auto-merging koha-tmpl/opac-tmpl/prog/en/css/opac.css
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8995: Show OpenURL links in OPAC search results
Comment 15 Jonathan Druart 2014-04-08 14:12:14 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2014-04-08 14:12:20 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-04-08 14:12:26 UTC Comment hidden (obsolete)
Comment 18 David Cook 2014-05-20 05:20:32 UTC
I like this idea, but I imagine it might make more sense to have a system preference called "OPACOpenURLForItemTypes" rather than "OPACHideOpenURLForItemTypes". 

That is, I imagine there are probably only going to be maybe one or two item types (i.e. eJournals and maybe eBooks) where you want to provide a OpenURL. So rather than listing all the ones you don't, it would be easier and less ongoing maintenance, to just list the ones that you do want to include.

--

It might also be an idea to change most of references to COinS to OpenURL in the code, since it really is OpenURL data being generated. The only time there really are COinS is when you stick the OpenURL into an HTML span, right?
Comment 19 Koha Team Lyon 3 2014-06-18 14:29:55 UTC
Some problems occurred applying patches from bug 8995:
<h1>Something went wrong !</h1>Applying: Bug 8995: Show OpenURL links in OPAC search results Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Biblio.pm
Auto-merging C4/XSLT.pm
CONFLICT (content): Merge conflict in C4/XSLT.pm Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging koha-tmpl/opac-tmpl/prog/en/css/opac.css
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt
Auto-merging koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt
Auto-merging koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACDetail.xsl
Auto-merging koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl
Auto-merging opac/opac-detail.pl
CONFLICT (content): Merge conflict in opac/opac-detail.pl Auto-merging opac/opac-search.pl Failed to merge in the changes.
Patch failed at 0001 Bug 8995: Show OpenURL links in OPAC search results When you have
Comment 20 Koha Team Lyon 3 2014-06-27 14:11:29 UTC
I'm really interested in this functionality. Are you still working on it ?
Comment 21 Jonathan Druart 2014-06-30 10:16:08 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-06-30 10:16:14 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-06-30 10:16:21 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2014-06-30 10:16:50 UTC
I just fixed the conflicts.
Comment 25 Koha Team Lyon 3 2014-07-04 13:55:05 UTC
I'm disappointed, because I made a lot of tests and everything is OK except I can't see the openurl link when I Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to "default".
It works very well when this sysprefs are empty (tested with two different open resolvers with electronic journals and ebooks.)

Two more remarks :
I agree with  David Cook (Comment 18)  : we usually use open url for 2 item types, it would be easier to list only those two and not all the others.

It is possible to open the link in a new tab (I activate the syspref OPACOpenInNewWindow, but there is no difference)
Comment 26 Koha Team Lyon 3 2014-07-04 13:56:49 UTC
One detail : I have tested only with the bootstrap theme
Comment 27 Julian Maurice 2014-07-31 09:31:28 UTC Comment hidden (obsolete)
Comment 28 Julian Maurice 2014-07-31 09:31:39 UTC Comment hidden (obsolete)
Comment 29 Julian Maurice 2014-07-31 09:31:49 UTC Comment hidden (obsolete)
Comment 30 Julian Maurice 2014-07-31 09:31:55 UTC Comment hidden (obsolete)
Comment 31 Julian Maurice 2014-07-31 09:32:01 UTC Comment hidden (obsolete)
Comment 32 Julian Maurice 2014-07-31 09:43:01 UTC
(In reply to Koha Team Lyon 3 from comment #25)
> I'm disappointed, because I made a lot of tests and everything is OK except
> I can't see the openurl link when I Set sysprefs OPACXSLTDetailsDisplay and
> OPACXSLTResultsDisplay to "default".
> It works very well when this sysprefs are empty (tested with two different
> open resolvers with electronic journals and ebooks.)

Fixed by attachment 30388 [details] [review]

> Two more remarks :
> I agree with  David Cook (Comment 18)  : we usually use open url for 2 item
> types, it would be easier to list only those two and not all the others.

Fixed by attachment 30389 [details] [review]
Comment 33 Julian Maurice 2014-07-31 09:43:56 UTC
> It is possible to open the link in a new tab (I activate the syspref
> OPACOpenInNewWindow, but there is no difference)

I wasn't aware of OpacOpenInNewWindow syspref, I will take a look to see what I can do.
Comment 34 Julian Maurice 2014-07-31 10:07:58 UTC Comment hidden (obsolete)
Comment 35 Paola Rossi 2014-11-17 16:57:44 UTC
I've tried to apply the patches aginst master 3.17.00.055

Applying: Bug 8995: Show OpenURL links in OPAC search results
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-search.pl
CONFLICT (modify/delete): koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt deleted in HEAD and modified in Bug 8995: Show OpenURL links in OPAC search results. Version Bug 8995: Show OpenURL links in OPAC search results of koha-tmpl/opac-tmpl/prog/en/modules/opac-results.tt left in tree.
CONFLICT (modify/delete): koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt deleted in HEAD and modified in Bug 8995: Show OpenURL links in OPAC search results. Version Bug 8995: Show OpenURL links in OPAC search results of koha-tmpl/opac-tmpl/prog/en/modules/opac-detail.tt left in tree.
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/UNIMARCslim2OPACResults.xsl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/UNIMARCslim2OPACDetail.xsl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/opac-old.css
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/XSLT.pm
CONFLICT (content): Merge conflict in C4/XSLT.pm
Auto-merging C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8995: Show OpenURL links in OPAC search results

So I pass the patch to "Patch doesn't apply" status.
Comment 36 Julian Maurice 2015-02-09 12:22:59 UTC Comment hidden (obsolete)
Comment 37 Julian Maurice 2015-02-09 12:23:26 UTC
All patches rebased and squashed
Comment 38 Paola Rossi 2015-02-13 16:39:41 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2015-03-16 06:52:28 UTC
This patch conflicts in a since removed CSS file:
CONFLICT (modify/delete): koha-tmpl/opac-tmpl/bootstrap/css/opac-old.css

Looking at the patch I can see that the .less file for bootstrap has been modified, but there is no updated generated CSS file, but only the change to the outdated file. Can you check please?
Comment 40 Julian Maurice 2015-06-26 07:41:49 UTC Comment hidden (obsolete)
Comment 41 Katrin Fischer 2015-10-18 22:05:11 UTC
There are some conflicts - especially in the CSS files. Please fix!

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8995: Show OpenURL links in OPAC search results
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
M	installer/data/mysql/sysprefs.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
M	koha-tmpl/opac-tmpl/bootstrap/css/opac.css
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl
M	koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl
M	koha-tmpl/opac-tmpl/bootstrap/less/opac.less
M	opac/opac-detail.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/opac-detail.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/less/opac.less
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/less/opac.less
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACDetail.xsl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/css/opac.css
CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/css/opac.css
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8995: Show OpenURL links in OPAC search results
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/SIGNED-OFF-Bug-8995-Show-OpenURL-links-in-OPAC-sea-AHeoX5.patch
Comment 42 Julian Maurice 2015-11-09 10:59:23 UTC Comment hidden (obsolete)
Comment 43 Katrin Fischer 2015-11-30 23:10:30 UTC
Comment on attachment 44639 [details] [review]
[SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results

Review of attachment 44639 [details] [review]:
-----------------------------------------------------------------

Hi, 

I have started with a code review here:

1) Please remove the changes to opac-detail.tt and opac-results.tt. (see: http://wiki.koha-community.org/wiki/Coding_Guidelines#DEPR1:_non-XSLT_search_results_and_bib_display_templates_deprecated)

::: C4/XSLT.pm
@@ +215,5 @@
>          $sysxml .= "<syspref name=\"$syspref\">$sp</syspref>\n";
>      }
>      $sysxml .= "</sysprefs>\n";
> +
> +    $variables ||= {};

2) Please chose a better variable name reflecting that this is about the OpenUrl feature.

::: installer/data/mysql/sysprefs.sql
@@ +510,1 @@
>  ;

2.5) Please do the librarian in me a favor... and keep the alphabetic order in this file :)

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +324,4 @@
>                    both: "both top and footer"
>                    top: "top"
>                    footer: "only footer"
> +        -

3) Please create a new heading for the OpenURL preferences, so that they are listed in one section together.

@@ +324,5 @@
>                    both: "both top and footer"
>                    top: "top"
>                    footer: "only footer"
> +        -
> +            - 'Complete URL of OpenURL resolver (starting with <tt>http://</tt> or <tt>https://</tt>):'

4) Please remove the <tt> tags (see: https://www.w3.org/wiki/HTML/Elements/tt). I think we are using <code> in some other preferences.

@@ +349,5 @@
> +        -
> +            - 'List of item type codes (separated by spaces) for those you want to show the OpenURL link:'
> +            - pref: OPACOpenURLItemTypes
> +            - '<br />'
> +            - 'It uses biblioitems.itemtype field, so if you map a MARC field to it, and link this MARC field to a list of authorised values (for example CCODE), you can use these values for system preference value.'

5) Sorry, but I think this is bad advice. One shouldn't redefine the biblioitems.itemtype field like that - or you will cause yourself trouble in a lot of other places. For example some of the circulation code falls back to the itemtype defined on biblio level now.

What happens when pref is left empty? Can the OpenUrl be displayed for records with empty biblioitems.itemtype?
Comment 44 Marcel de Rooy 2016-01-24 16:13:05 UTC
Are there still plans to get this moving again?

Additional question: How is this related to the COinS html Koha already generates? Why do we exactly need this, if that span can be easily converted to an OpenURL with standard solutions (browser extensions etc.) ?

http://ocoins.info/latent.html
Comment 45 Julian Maurice 2016-02-25 15:18:30 UTC Comment hidden (obsolete)
Comment 46 Julian Maurice 2016-02-25 15:18:35 UTC Comment hidden (obsolete)
Comment 47 Julian Maurice 2016-02-25 15:30:03 UTC
(In reply to Katrin Fischer from comment #43)
> 1) Please remove the changes to opac-detail.tt and opac-results.tt. (see:
> http://wiki.koha-community.org/wiki/Coding_Guidelines#DEPR1:_non-
> XSLT_search_results_and_bib_display_templates_deprecated)

Changes removed

> 2) Please chose a better variable name reflecting that this is about the
> OpenUrl feature.

I am not sure of what you ask here, "COinS" is related to OpenURL, this name makes perfect sense to me.

> 2.5) Please do the librarian in me a favor... and keep the alphabetic order
> in this file :)

Done ;)

> 3) Please create a new heading for the OpenURL preferences, so that they are
> listed in one section together.

Done
 
> 4) Please remove the <tt> tags (see:
> https://www.w3.org/wiki/HTML/Elements/tt). I think we are using <code> in
> some other preferences.

Done

> 5) Sorry, but I think this is bad advice. One shouldn't redefine the
> biblioitems.itemtype field like that - or you will cause yourself trouble in
> a lot of other places. For example some of the circulation code falls back
> to the itemtype defined on biblio level now.

Advice removed

> What happens when pref is left empty? Can the OpenUrl be displayed for
> records with empty biblioitems.itemtype?

If the preference is empty, the feature is disabled. It can be changed if needed.


I took the opportunity to put opac.css changes in a separate patch as recommended by guidelines.
Comment 48 Katrin Fischer 2016-03-23 06:52:01 UTC
Hi Julian, coming back to this soon - could you point me to an example config mayb to see this in action?
Comment 49 Katrin Fischer 2016-03-28 19:36:39 UTC
Some notes:

1) a & in the URL breaks the detail page. How can we handle this? I tried testing the feature with a service that requires you to pass on a library ID and some other parameters in order to use the service.

2) We already have a system preference mentioning OpenURL: [Don't include|Include] COinSinOPACResults COinS / OpenURL / Z39.88 in OPAC search results. Warning: Enabling this feature will slow OPAC search response times. It has no effect on the OpenURL feature added by this patch. I'd suggest rephrasing it, but not sure about a suggestion. Maybe just an addition that the OpenURL feature is separate?

3) Further tests with the service I tried using showed that a lot of the attributes were invalid (after researching a bit I think we might provide 1.0 valid, while the service is a lower OpenURL version) - so I can't really use that for testing :(
I'd still much appreciate a 'real life example' to test this with.

4) Please take a look at comment#44

... still needs testing with UNIMARC.
Comment 50 Julian Maurice 2016-03-29 07:32:56 UTC
1) In the XSLT, a '?' is appended to the syspref, followed by other params, so it's a bug. Changing status to Failed QA

2) Telling about the other sysprefs here would be nice I think

3) I don't know any free service that could be used for testing. I'll try to find a way to make this testable

4) An OpenURL resolver can provide additional resources, and we can't expect all OPAC users to have installed a browser extension such as Zotero
Comment 51 Magnus Enger 2016-04-19 08:22:45 UTC
(In reply to Julian Maurice from comment #50)
> 1) In the XSLT, a '?' is appended to the syspref, followed by other params,
> so it's a bug. Changing status to Failed QA
> 
> 2) Telling about the other sysprefs here would be nice I think

I would love to see this in 16.05. Any chance these two issues can be adressed in time?
Comment 52 Julian Maurice 2016-09-02 14:01:27 UTC
Created attachment 55131 [details] [review]
Bug 8995: Show OpenURL links in OPAC search results

This patch use GetCOinS sub to provide an OpenURL link in OPAC search
results. It uses 4 new system preferences:
  - OpenURLinOPACResults: enable or disable this feature
  - OpenURLResolverURL: url of the openURL resolver
  - OpenURLText: text of the link
  - OpenURLImageLocation: image of the link

Link is displayed as an image if OpenURLImageLocation is defined, and as
text otherwise.
It works both with and without XSLT enabled.

Changes made to GetCOinSBiblio:

For 'journal':
- Title should be in rft.jtitle instead of rft.title
- rft.date, rft.aulast, rft.aufirst, rft.au, rft.pub and rft.pages have
  no meaning for a subscription, so they are simply removed from URL

This patch refactors GetCOinSBiblio, so the construction of URL is done
only at the end. This way we do not have ugly
  $var .= "&amp;$value"
in the function body.

Also use URI::Escape instead of custom regexps.

This development consider the value of syspref OPACURLOpenInNewWindow
when building the link.

Test plan:
1/ Enable syspref OPACShowOpenURL and put your OpenURL resolver URL in
OpenURLResolverURL syspref (if you don't have one, just fill it with
some fake URL, you'll have to check if OpenURL links are correct)
2/ If you want, set the other sysprefs OpenURLImageLocation and
OpenURLText
3/ Fill syspref OPACOpenURLItemTypes with some (not all) of your
item types.
4/ Empty sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay
5/ Go to OPAC and launch a search.
6/ Check you have in the results (near the title) the OpenURL link (only
for itemtypes that are in OPACOpenURLItemTypes).
Toggle OPACURLOpenInNewWindow syspref and check that the behaviour of
the link is correct.
7/ Go to the detail page of one of those and check you have the OpenURL
link too. (Above tags)
Toggle OPACURLOpenInNewWindow syspref and check that the behaviour of
the link is correct.
8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to
"default" and repeat steps 5 to 7

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Comment 53 Julian Maurice 2016-09-02 14:01:33 UTC
Created attachment 55132 [details] [review]
Bug 8995: Update opac.css
Comment 54 Julian Maurice 2016-09-02 14:01:38 UTC
Created attachment 55133 [details] [review]
Bug 8995: Remove useless code

OpenURL link display is done in XSLT only, so this code is now useless
Comment 55 Julian Maurice 2016-09-02 14:01:42 UTC
Created attachment 55134 [details] [review]
Bug 8995: Fix URL generation when the URL in syspref has a query part
Comment 56 Julian Maurice 2016-09-02 14:01:47 UTC
Created attachment 55135 [details] [review]
Bug 8995: Tell about the new sysprefs in COinSinOPACResults description
Comment 57 Julian Maurice 2016-09-02 14:01:51 UTC
Created attachment 55136 [details] [review]
Bug 8995: Fix and simplify usage of syspref OpenURLImageLocation

There were still references to the OPAC 'prog' theme in XSL files
Now, when using a relative URL, we have to give the full path
Comment 58 Julian Maurice 2016-09-02 14:03:35 UTC
First 2 patches rebased on master
4 new patches added to fix some issues
Comment 59 Jonathan Druart 2016-09-28 12:17:40 UTC
How can we test this feature correctly?
Comment 60 Jonathan Druart 2016-09-28 12:20:34 UTC
Created attachment 55882 [details] [review]
Bug 8995: Add IGNORE to the INSERT statement
Comment 61 Magnus Enger 2016-09-29 19:39:43 UTC
(In reply to Jonathan Druart from comment #59)
> How can we test this feature correctly?

The trouble is finding a link resolver that can be used. One of my customers is very interested in this patch, so I have set up a gitified, temporary instance for them with the patches on this bug. They have configured it to use their existing SFX lin resolver. Here is an example:
http://stami.demo.bibkat.no/cgi-bin/koha/opac-detail.pl?biblionumber=9810

They have tested it and found it satisfactory. Their testing was before the 5 followup patches, but the instance is currently running with all the patches on this bug.
Comment 62 M. Tompsett 2017-03-03 19:50:51 UTC
If one fixes the opac.less (easy to fix), and recompiles the opac.css (easy to do in kohadevbox -- mine isn't booted), this patch set could be rescued.
Comment 63 Magnus Enger 2017-06-21 09:11:06 UTC
Any chance of another rebase? I'll sign off quickley this time.
Comment 64 Magnus Enger 2017-06-21 12:36:09 UTC
If anyone needs an OpenURL linkresolver to test these patches against, I have a customer that has one that can be used. Please contact me for details.
Comment 65 Julian Maurice 2017-06-22 07:55:41 UTC
Created attachment 64513 [details] [review]
Bug 8995: Show OpenURL links in OPAC search results

This patch use GetCOinS sub to provide an OpenURL link in OPAC search
results. It uses 4 new system preferences:
  - OpenURLinOPACResults: enable or disable this feature
  - OpenURLResolverURL: url of the openURL resolver
  - OpenURLText: text of the link
  - OpenURLImageLocation: image of the link

Link is displayed as an image if OpenURLImageLocation is defined, and as
text otherwise.
It works both with and without XSLT enabled.

Changes made to GetCOinSBiblio:

For 'journal':
- Title should be in rft.jtitle instead of rft.title
- rft.date, rft.aulast, rft.aufirst, rft.au, rft.pub and rft.pages have
  no meaning for a subscription, so they are simply removed from URL

This patch refactors GetCOinSBiblio, so the construction of URL is done
only at the end. This way we do not have ugly
  $var .= "&amp;$value"
in the function body.

Also use URI::Escape instead of custom regexps.

This development consider the value of syspref OPACURLOpenInNewWindow
when building the link.

Test plan:
1/ Enable syspref OPACShowOpenURL and put your OpenURL resolver URL in
OpenURLResolverURL syspref (if you don't have one, just fill it with
some fake URL, you'll have to check if OpenURL links are correct)
2/ If you want, set the other sysprefs OpenURLImageLocation and
OpenURLText
3/ Fill syspref OPACOpenURLItemTypes with some (not all) of your
item types.
4/ Empty sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay
5/ Go to OPAC and launch a search.
6/ Check you have in the results (near the title) the OpenURL link (only
for itemtypes that are in OPACOpenURLItemTypes).
Toggle OPACURLOpenInNewWindow syspref and check that the behaviour of
the link is correct.
7/ Go to the detail page of one of those and check you have the OpenURL
link too. (Above tags)
Toggle OPACURLOpenInNewWindow syspref and check that the behaviour of
the link is correct.
8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to
"default" and repeat steps 5 to 7

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Comment 66 Julian Maurice 2017-06-22 07:55:48 UTC
Created attachment 64514 [details] [review]
Bug 8995: Update opac.css
Comment 67 Julian Maurice 2017-06-22 07:55:53 UTC
Created attachment 64515 [details] [review]
Bug 8995: Remove useless code

OpenURL link display is done in XSLT only, so this code is now useless
Comment 68 Julian Maurice 2017-06-22 07:55:59 UTC
Created attachment 64516 [details] [review]
Bug 8995: Fix URL generation when the URL in syspref has a query part
Comment 69 Julian Maurice 2017-06-22 07:56:04 UTC
Created attachment 64517 [details] [review]
Bug 8995: Tell about the new sysprefs in COinSinOPACResults description
Comment 70 Julian Maurice 2017-06-22 07:56:10 UTC
Created attachment 64518 [details] [review]
Bug 8995: Fix and simplify usage of syspref OpenURLImageLocation

There were still references to the OPAC 'prog' theme in XSL files
Now, when using a relative URL, we have to give the full path
Comment 71 Julian Maurice 2017-06-22 07:56:15 UTC
Created attachment 64519 [details] [review]
Bug 8995: Add IGNORE to the INSERT statement
Comment 72 Julian Maurice 2017-06-22 07:56:42 UTC
Patches rebased
Comment 73 Magnus Enger 2017-06-22 09:01:12 UTC
Created attachment 64532 [details] [review]
Bug 8995: Show OpenURL links in OPAC search results

This patch use GetCOinS sub to provide an OpenURL link in OPAC search
results. It uses 4 new system preferences:
  - OpenURLinOPACResults: enable or disable this feature
  - OpenURLResolverURL: url of the openURL resolver
  - OpenURLText: text of the link
  - OpenURLImageLocation: image of the link

Link is displayed as an image if OpenURLImageLocation is defined, and as
text otherwise.
It works both with and without XSLT enabled.

Changes made to GetCOinSBiblio:

For 'journal':
- Title should be in rft.jtitle instead of rft.title
- rft.date, rft.aulast, rft.aufirst, rft.au, rft.pub and rft.pages have
  no meaning for a subscription, so they are simply removed from URL

This patch refactors GetCOinSBiblio, so the construction of URL is done
only at the end. This way we do not have ugly
  $var .= "&amp;$value"
in the function body.

Also use URI::Escape instead of custom regexps.

This development consider the value of syspref OPACURLOpenInNewWindow
when building the link.

Test plan:
1/ Enable syspref OPACShowOpenURL and put your OpenURL resolver URL in
OpenURLResolverURL syspref (if you don't have one, just fill it with
some fake URL, you'll have to check if OpenURL links are correct)
2/ If you want, set the other sysprefs OpenURLImageLocation and
OpenURLText
3/ Fill syspref OPACOpenURLItemTypes with some (not all) of your
item types.
4/ Empty sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay
5/ Go to OPAC and launch a search.
6/ Check you have in the results (near the title) the OpenURL link (only
for itemtypes that are in OPACOpenURLItemTypes).
Toggle OPACURLOpenInNewWindow syspref and check that the behaviour of
the link is correct.
7/ Go to the detail page of one of those and check you have the OpenURL
link too. (Above tags)
Toggle OPACURLOpenInNewWindow syspref and check that the behaviour of
the link is correct.
8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to
"default" and repeat steps 5 to 7

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 74 Magnus Enger 2017-06-22 09:01:23 UTC
Created attachment 64533 [details] [review]
Bug 8995: Update opac.css

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 75 Magnus Enger 2017-06-22 09:01:31 UTC
Created attachment 64534 [details] [review]
Bug 8995: Remove useless code

OpenURL link display is done in XSLT only, so this code is now useless

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 76 Magnus Enger 2017-06-22 09:01:56 UTC
Created attachment 64535 [details] [review]
Bug 8995: Fix URL generation when the URL in syspref has a query part

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 77 Magnus Enger 2017-06-22 09:02:05 UTC
Created attachment 64536 [details] [review]
Bug 8995: Tell about the new sysprefs in COinSinOPACResults description

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 78 Magnus Enger 2017-06-22 09:02:26 UTC
Created attachment 64537 [details] [review]
Bug 8995: Fix and simplify usage of syspref OpenURLImageLocation

There were still references to the OPAC 'prog' theme in XSL files
Now, when using a relative URL, we have to give the full path

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 79 Magnus Enger 2017-06-22 09:02:35 UTC
Created attachment 64538 [details] [review]
Bug 8995: Add IGNORE to the INSERT statement

Signed-off-by: Magnus Enger <magnus@libriotech.no>
Comment 80 Magnus Enger 2017-06-22 09:03:39 UTC
One of my customers have tested this, and are very happy with the result. This should be a very nice addition for many academic libraries.
Comment 81 Katrin Fischer 2017-06-22 19:34:16 UTC
Hi Magnuse, as you have a good test case for it - could I get you to write something for the manual once this goes in?
Comment 82 Magnus Enger 2017-06-23 06:20:23 UTC
(In reply to Katrin Fischer from comment #81)
> Hi Magnuse, as you have a good test case for it - could I get you to write
> something for the manual once this goes in?

Sure! Can you remind me about it?
Comment 83 Katrin Fischer 2017-06-23 06:21:48 UTC
I will! ;)