[Koha-bugs] [Bug 8995] Show OpenURL links in OPAC search results

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8995

--- Comment #43 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 44639
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=44639
[SIGNED OFF] Bug 8995: Show OpenURL links in OPAC search results

Review of attachment 44639:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=8995&attachment=44639)
-----------------------------------------------------------------

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?

-- 
You are receiving this mail because:
You are watching all bug changes.