Created attachment 13141 [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 are made for UNIMARC and MARC21 xslt. Note: this patch also moves the Babeltheque code outside an if statement "if (C4::Context->preference('COinSinOPACResults'))" (was misplaced because Babeltheque don't need COinS)
We are currently working on this patch, please do not consider this submission for the moment
Created attachment 22753 [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 .= "&$value" in the function body. Also use URI::Escape instead of custom regexps. 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 OPACHideOpenURLForItemTypes 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 not in OPACHideOpenURLForItemTypes) 7/ Go to the detail page of one of those and check you have the OpenURL link too. (Above tags) 8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to "default" 9/ Repeat steps 5 to 7
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
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
Created attachment 23625 [details] [review] Bug 8995: Show OpenURL links in OPAC search results Rebased on master
Created attachment 23626 [details] [review] Bug 8995: Use uri_escape_utf8 instead of uri_escape
Created attachment 23627 [details] [review] Bug 8995: Show OpenURL links in bootstrap theme
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
Created attachment 24087 [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 .= "&$value" in the function body. Also use URI::Escape instead of custom regexps. 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 OPACHideOpenURLForItemTypes 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 not in OPACHideOpenURLForItemTypes) 7/ Go to the detail page of one of those and check you have the OpenURL link too. (Above tags) 8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to "default" 9/ Repeat steps 5 to 7
Created attachment 24088 [details] [review] Bug 8995: Use uri_escape_utf8 instead of uri_escape
Created attachment 24089 [details] [review] Bug 8995: Show OpenURL links in bootstrap theme
rebased patches.
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
Created attachment 26899 [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 .= "&$value" in the function body. Also use URI::Escape instead of custom regexps. 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 OPACHideOpenURLForItemTypes 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 not in OPACHideOpenURLForItemTypes) 7/ Go to the detail page of one of those and check you have the OpenURL link too. (Above tags) 8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to "default" 9/ Repeat steps 5 to 7
Created attachment 26900 [details] [review] Bug 8995: Use uri_escape_utf8 instead of uri_escape
Created attachment 26901 [details] [review] Bug 8995: Show OpenURL links in bootstrap theme
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?
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
I'm really interested in this functionality. Are you still working on it ?
Created attachment 29357 [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 .= "&$value" in the function body. Also use URI::Escape instead of custom regexps. 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 OPACHideOpenURLForItemTypes 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 not in OPACHideOpenURLForItemTypes) 7/ Go to the detail page of one of those and check you have the OpenURL link too. (Above tags) 8/ Set sysprefs OPACXSLTDetailsDisplay and OPACXSLTResultsDisplay to "default" 9/ Repeat steps 5 to 7
Created attachment 29358 [details] [review] Bug 8995: Use uri_escape_utf8 instead of uri_escape
Created attachment 29359 [details] [review] Bug 8995: Show OpenURL links in bootstrap theme
I just fixed the conflicts.
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)
One detail : I have tested only with the bootstrap theme
Created attachment 30385 [details] [review] Bug 8995: Show OpenURL links in OPAC search results Rebased on master
Created attachment 30386 [details] [review] Bug 8995: Use uri_escape_utf8 instead of uri_escape Rebased on master
Created attachment 30387 [details] [review] Bug 8995: Show OpenURL links in bootstrap theme Rebased on master
Created attachment 30388 [details] [review] Bug 8995: Add OpenURL link to OPAC bootstrap XSLT files
Created attachment 30389 [details] [review] Bug 8995: Change meaning and name of syspref OPACHideOpenURLForItemTypes OPACHideOpenURLForItemTypes is now named OPACOpenURLItemTypes and do the exact opposite as before: it shows OpenURL link only if the biblio itemtype is in the syspref.
(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]
> 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.
Created attachment 30390 [details] [review] Bug 8995: Consider OPACURLOpenInNewWindow for OpenURL links
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.
Created attachment 35747 [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 .= "&$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
All patches rebased and squashed
Created attachment 35916 [details] [review] [SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results The new sysprefs are 5: NB - OPACShowOpenURL: enable or disable this feature - OpenURLResolverURL: url of the openURL resolver - OpenURLText: text of the link - OpenURLImageLocation: image of the link NB - OPACOpenURLItemTypes: List of item type codes (separated by spaces) for those you want to show the OpenURL link Link is displayed as an image if OpenURLImageLocation is defined. Link is displayed as an text if OpenURLText is defined. If neither OpenURLImageLocation nor OpenURLText are defined, link is displayed as the text "OpenURL". If either OpenURLImageLocation or OpenURLText are defined, link is displayed as an image whose title-text is the OpenURLText text. I've applied the patch against master 3.19.00.011 The patch contains "3.17.00" and needs to be rebased trivially. Soon after upgrading, and having set the sysprefs to enable OpenURL, I re-indexed zebra. Everything is OK as required. So I pass the patch to "Signed OFF" status.
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?
Created attachment 40647 [details] [review] [SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results Rebased on master + updated generated CSS file
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
Created attachment 44639 [details] [review] [SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results Rebased on master updatedatabase entry moved into atomicupdate/
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?
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
Created attachment 48371 [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 .= "&$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>
Created attachment 48372 [details] [review] Bug 8995: Update opac.css
(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.
Hi Julian, coming back to this soon - could you point me to an example config mayb to see this in action?
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.
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
(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?
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 .= "&$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>
Created attachment 55132 [details] [review] Bug 8995: Update opac.css
Created attachment 55133 [details] [review] Bug 8995: Remove useless code OpenURL link display is done in XSLT only, so this code is now useless
Created attachment 55134 [details] [review] Bug 8995: Fix URL generation when the URL in syspref has a query part
Created attachment 55135 [details] [review] Bug 8995: Tell about the new sysprefs in COinSinOPACResults description
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
First 2 patches rebased on master 4 new patches added to fix some issues
How can we test this feature correctly?
Created attachment 55882 [details] [review] Bug 8995: Add IGNORE to the INSERT statement
(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.
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.
Any chance of another rebase? I'll sign off quickley this time.
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.
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 .= "&$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>
Created attachment 64514 [details] [review] Bug 8995: Update opac.css
Created attachment 64515 [details] [review] Bug 8995: Remove useless code OpenURL link display is done in XSLT only, so this code is now useless
Created attachment 64516 [details] [review] Bug 8995: Fix URL generation when the URL in syspref has a query part
Created attachment 64517 [details] [review] Bug 8995: Tell about the new sysprefs in COinSinOPACResults description
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
Created attachment 64519 [details] [review] Bug 8995: Add IGNORE to the INSERT statement
Patches rebased
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 .= "&$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>
Created attachment 64533 [details] [review] Bug 8995: Update opac.css Signed-off-by: Magnus Enger <magnus@libriotech.no>
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>
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>
Created attachment 64536 [details] [review] Bug 8995: Tell about the new sysprefs in COinSinOPACResults description Signed-off-by: Magnus Enger <magnus@libriotech.no>
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>
Created attachment 64538 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no>
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.
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?
(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?
I will! ;)
Patch doesn't apply, please, rebase. Also, we will need pod and test for GetOpenURLResolverURL in C4::Biblio
Created attachment 86483 [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 .= "&$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>
Created attachment 86484 [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>
Created attachment 86485 [details] [review] Bug 8995: Fix URL generation when the URL in syspref has a query part Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86486 [details] [review] Bug 8995: Tell about the new sysprefs in COinSinOPACResults description Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86487 [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>
Created attachment 86488 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86489 [details] [review] Bug 8995: (follow-up) Adapt to current codebase
Created attachment 86490 [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 .= "&$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>
Created attachment 86491 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86492 [details] [review] Bug 8995: (follow-up) Adapt to current codebase
I rebased and squashed the patches a bit. To pass QA there are at least missing tests for GetOpenURLResolverURL.
Created attachment 86541 [details] [review] Bug 8995: (follow-up) Added test
Created attachment 86542 [details] [review] Bug 8995: (follow-up) Add POD for GetOpenURLResolverURL
Created attachment 86557 [details] [review] Bug 8995: (follow-up) Move GetOpenURLResolverURL to Koha::Biblio->get_openurl
Created attachment 86558 [details] [review] Bug 8995: (follow-up) Move GetCOinSBiblio to Koha::Biblio->get_coins Test plan: 1) Ensure the COinS span tag is still included on this pages. You need to look into html source and search for span tag with class 'Z3988', which has COinS string in title. Staff client: catalogue -> ISBDdetail catalogue -> MARCdetail catalogue -> detail virtualshelves -> shelves OPAC (you should have COinSinOPACResults system preference enabled): opac detail opac search opac shelves 2) Run tests: prove t/Biblio.t t/db_dependent/Biblio.t t/db_dependent/Koha/Biblio.t
Created attachment 86559 [details] [review] Bug 8995: (follow-up) Add one more test fot get_openurl Test plan: prove t/db_dependent/Koha/Biblio.t
Created attachment 86601 [details] [review] Bug 8995: (follow-up) Tak system preference item-level_itypes into account
Created attachment 86602 [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 .= "&$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>
Created attachment 86603 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86604 [details] [review] Bug 8995: (follow-up) Adapt to current codebase Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86605 [details] [review] Bug 8995: (follow-up) Added test Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86606 [details] [review] Bug 8995: (follow-up) Add POD for GetOpenURLResolverURL Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86607 [details] [review] Bug 8995: (follow-up) Move GetOpenURLResolverURL to Koha::Biblio->get_openurl Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86608 [details] [review] Bug 8995: (follow-up) Move GetCOinSBiblio to Koha::Biblio->get_coins Test plan: 1) Ensure the COinS span tag is still included on this pages. You need to look into html source and search for span tag with class 'Z3988', which has COinS string in title. Staff client: catalogue -> ISBDdetail catalogue -> MARCdetail catalogue -> detail virtualshelves -> shelves OPAC (you should have COinSinOPACResults system preference enabled): opac detail opac search opac shelves 2) Run tests: prove t/Biblio.t t/db_dependent/Biblio.t t/db_dependent/Koha/Biblio.t Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86609 [details] [review] Bug 8995: (follow-up) Add one more test fot get_openurl Test plan: prove t/db_dependent/Koha/Biblio.t Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 86610 [details] [review] Bug 8995: (follow-up) Tak system preference item-level_itypes into account Signed-off-by: Magnus Enger <magnus@libriotech.no> Tested with all 9 current patches. Works as advertised, including OPACURLOpenInNewWindow. If a record has no items, no OpenURL link is displayed. All the suggested tests pass. I did not test with XSLT turned off.
Created attachment 88665 [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 .= "&$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>
Created attachment 88666 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 88667 [details] [review] Bug 8995: (follow-up) Adapt to current codebase Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 88668 [details] [review] Bug 8995: (follow-up) Added test Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 88669 [details] [review] Bug 8995: (follow-up) Add POD for GetOpenURLResolverURL Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 88670 [details] [review] Bug 8995: (follow-up) Move GetOpenURLResolverURL to Koha::Biblio->get_openurl Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 88671 [details] [review] Bug 8995: (follow-up) Move GetCOinSBiblio to Koha::Biblio->get_coins Signed-off-by: Magnus Enger <magnus@libriotech.no> Test plan: 1) Ensure the COinS span tag is still included on this pages. You need to look into html source and search for span tag with class 'Z3988', which has COinS string in title. Staff client: catalogue -> ISBDdetail catalogue -> MARCdetail catalogue -> detail virtualshelves -> shelves OPAC (you should have COinSinOPACResults system preference enabled): opac detail opac search opac shelves 2) Run tests: prove t/Biblio.t t/db_dependent/Biblio.t t/db_dependent/Koha/Biblio.t
Created attachment 88672 [details] [review] Bug 8995: (follow-up) Add one more test fot get_openurl Test plan: prove t/db_dependent/Koha/Biblio.t Signed-off-by: Magnus Enger <magnus@libriotech.no>
Created attachment 88673 [details] [review] Bug 8995: (follow-up) Tak system preference item-level_itypes into account Signed-off-by: Magnus Enger <magnus@libriotech.no> Tested with all 9 current patches. Works as advertised, including OPACURLOpenInNewWindow. If a record has no items, no OpenURL link is displayed. All the suggested tests pass. I did not test with XSLT turned off.
Just rebased on master
Created attachment 88735 [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 .= "&$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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88736 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88737 [details] [review] Bug 8995: (follow-up) Adapt to current codebase Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88738 [details] [review] Bug 8995: (follow-up) Added test Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88739 [details] [review] Bug 8995: (follow-up) Add POD for GetOpenURLResolverURL Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88740 [details] [review] Bug 8995: (follow-up) Move GetOpenURLResolverURL to Koha::Biblio->get_openurl Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88741 [details] [review] Bug 8995: (follow-up) Move GetCOinSBiblio to Koha::Biblio->get_coins Signed-off-by: Magnus Enger <magnus@libriotech.no> Test plan: 1) Ensure the COinS span tag is still included on this pages. You need to look into html source and search for span tag with class 'Z3988', which has COinS string in title. Staff client: catalogue -> ISBDdetail catalogue -> MARCdetail catalogue -> detail virtualshelves -> shelves OPAC (you should have COinSinOPACResults system preference enabled): opac detail opac search opac shelves 2) Run tests: prove t/Biblio.t t/db_dependent/Biblio.t t/db_dependent/Koha/Biblio.t Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88742 [details] [review] Bug 8995: (follow-up) Add one more test fot get_openurl Test plan: prove t/db_dependent/Koha/Biblio.t Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88743 [details] [review] Bug 8995: (follow-up) Take system preference item-level_itypes into account Signed-off-by: Magnus Enger <magnus@libriotech.no> Tested with all 9 current patches. Works as advertised, including OPACURLOpenInNewWindow. If a record has no items, no OpenURL link is displayed. All the suggested tests pass. I did not test with XSLT turned off. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 88744 [details] [review] Bug 8995: (QA follow-up) Convert db update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Good work team, well shepherded to completion. This works well for me and I'm unable to find any regressions.. QA scripts all pass (with the exception of the git manipulation issue where I converted the .sql file to a .perl one as per the current RM's preference). Passing QA.. any chance of something for the release notes?
I like this one a lot - can we get a squashed/clean version of these patches just for readability sake?
Created attachment 89012 [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 .= "&$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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 89013 [details] [review] Bug 8995: Add IGNORE to the INSERT statement Signed-off-by: Magnus Enger <magnus@libriotech.no> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 89014 [details] [review] Bug 8995: (follow-up) Add tests, move open_url/coins routines to Koha namespace Test plan: 1) Ensure the COinS span tag is still included on this pages. You need to look into html source and search for span tag with class 'Z3988', which has COinS string in title. Staff client: catalogue -> ISBDdetail catalogue -> MARCdetail catalogue -> detail virtualshelves -> shelves OPAC (you should have COinSinOPACResults system preference enabled): opac detail opac search opac shelves 2) Run tests: prove t/Biblio.t t/db_dependent/Biblio.t t/db_dependent/Koha/Biblio.t Signed-off-by: Magnus Enger <magnus@libriotech.no> Tested with all 9 current patches. Works as advertised, including OPACURLOpenInNewWindow. If a record has no items, no OpenURL link is displayed. All the suggested tests pass. I did not test with XSLT turned off. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 89015 [details] [review] Bug 8995: (QA follow-up) Convert db update to perl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Nick Clemens from comment #132) > I like this one a lot - can we get a squashed/clean version of these patches > just for readability sake? Is this rebase OK for you Nick?
(In reply to Josef Moravec from comment #137) > (In reply to Nick Clemens from comment #132) > > I like this one a lot - can we get a squashed/clean version of these patches > > just for readability sake? > > Is this rebase OK for you Nick? Indeed, thanks Josef!. I was worried that changes don't work without XSLT, but if I understand earlier comments they were remove as that view is deprecated.
Awesome work all! Pushed to master for 19.05
Maybe we should update to new feature?
Created attachment 89120 [details] [review] Bug 8995: (QA follow-up) Mock required sysprefs in the tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
New feature will not be backported to 18.11.x series.