https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11361

Maksim Sen <maksim....@inlibro.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #60952|0                           |1
        is obsolete|                            |

--- Comment #9 from Maksim Sen <maksim....@inlibro.com> ---
Created attachment 72812
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=72812&action=edit
These are the points in the code review that I corrected in this patch.

::: C4/Auth.pm
@@ +523,4 @@
>              OPACURLOpenInNewWindow                => "" . 
> C4::Context->preference("OPACURLOpenInNewWindow"),
>              OPACUserCSS                           => "" . 
> C4::Context->preference("OPACUserCSS"),
>              OpacAuthorities                       => 
> C4::Context->preference("OpacAuthorities"),
> +            OpacZ3950                             => 
> C4::Context->preference("OpacZ3950"),

It's better to use the newer TT plugin method to check system preferences.
I think this is done below, so this can possibly be reoved.


::: installer/data/mysql/sysprefs.sql
@@ +569,5 @@
>  ('XSLTListsDisplay','default','','Enable XSLT stylesheet control over lists 
> pages display on intranet','Free'),
>  ('XSLTResultsDisplay','default','','Enable XSL stylesheet control over 
> results page display on intranet','Free'),
>  ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields 
> for Personal Name Authorities to fill biblio.author','free'),
> +('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will 
> replace authors in biblio.author','YesNo'),
> +('OPACZ3950','0','','Allow patrons to search for bibliographic records in 
> other libraries via Z39.50.','YesNo')

The list in sysprefs.sql should be sorted alphabetically to minimize conflicts.


::: installer/data/mysql/updatedatabase.pl
@@ +13959,4 @@
>      print "Upgrade to $DBversion done (Bug 16530 - Add a circ sidebar 
> navigation menu)\n";
>  }
>  
> +$DBversion = "XXX";

This should be an atomicupdate in a separate file now instead of an entry in
updatedatabase.


@@ +13960,5 @@
>  }
>  
> +$DBversion = "XXX";
> +if( CheckVersion( $DBversion ) ) {
> +    $dbh->do("ALTER TABLE z3950servers ADD COLUMN opac TINYINT(1);");

Please specify where the new column should be added using AFTER/BEFORE column.


@@ +13962,5 @@
> +$DBversion = "XXX";
> +if( CheckVersion( $DBversion ) ) {
> +    $dbh->do("ALTER TABLE z3950servers ADD COLUMN opac TINYINT(1);");
> +    $dbh->do("INSERT IGNORE INTO systempreferences 
> (variable,value,options,explanation,type) VALUES('OPACZ3950','0','','Allow 
> patrons to search for bibliographic records in other libraries via 
> Z39.50.','YesNo')");
> +    print "Add opac column to z3950servers table.\nAdd OPACZ3950 
> preference.\n";

I am not sure if the \n work nicely in the HTML output of the web installer,
maybe remove them.


::: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
@@ +273,4 @@
>                                      [% IF ( Koha.Preference( 
> 'UseCourseReserves' ) == 1 ) %]<li><a 
> href="/cgi-bin/koha/opac-course-reserves.pl">Course reserves</a></li>[% END %]
>                                      [% IF Koha.Preference( 'OpacBrowser' ) 
> == 1 %]<li><a href="/cgi-bin/koha/opac-browser.pl">Browse by 
> hierarchy</a></li>[% END %]
>                                      [% IF Koha.Preference( 'OpacAuthorities' 
> ) == 1 %]<li><a href="/cgi-bin/koha/opac-authorities-home.pl">Authority 
> search</a></li>[% END %]
> +                                    [% IF Koha.Preference( 'OpacZ3950' ) 
> %]<li><span id="z3950searchOpac"><a target="_blank" href="#" >Z39.50 
> Search</a></span></li>[% END %]

Not sure patrons will know what Z39.50 means, maybe we could find another label
- "search other catalogs" maybe?


@@ +64,5 @@
> +        <input type="hidden" name="op" id="op" value="do_search" />
> +        <input type="hidden" name="biblionumber" value="[% biblionumber %]" 
> />
> +        <input type="hidden" name="frameworkcode" value="[% frameworkcode 
> %]" />
> +        <div id="search_params" class="span6 form-horizontal">
> +            <h2>Search Parameters</h2>

Should be: Search parameters (only first word capitalized in Koha)


@@ +180,5 @@
> +    <div class="row-fluid">
> +        <div class="span4">
> +            <div class="input-prepend input-append">
> +                [% IF ( show_prevbutton ) %]
> +                    <button class="btn" type="button" name="changepage_prev" 
> onclick="$('#current_page').val([% current_page 
> %]-1);$('#page_form').submit();">Previous Page</button>

see above (Previous page)


@@ +184,5 @@
> +                    <button class="btn" type="button" name="changepage_prev" 
> onclick="$('#current_page').val([% current_page 
> %]-1);$('#page_form').submit();">Previous Page</button>
> +                [% END %]
> +                <span class="add-on">Page [% current_page %] / [% 
> total_pages %]</span>
> +                [% IF ( show_nextbutton ) %]
> +                    <button class="btn" type="button" name="changepage_next" 
> onclick="$('#current_page').val([% current_page 
> %]+1);$('#page_form').submit();">Next Page</button>

see above (Next page)


::: opac/opac-z3950-search.pl
@@ +18,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;

Should be: use Modern::Perl


::: opac/opac-z3950-search.pl
@@ +51,5 @@
> +my ( $template, $loggedinuser, $cookie ) = get_template_and_user({
> +        template_name   => "opac-z3950-search.tt",
> +        query           => $input,
> +        type            => "opac",
> +        authnotrequired => 1,

All pages should require auth if OpacPublic is set to Disable:
authnotrequired => ( C4::Context->preference("OpacPublic") ? 1 : 0 ),






These are the review points left to be corrected.


::: admin/z3950servers.pl
@@ +66,4 @@
>      $id = 0;
>  } elsif ( $op eq 'add_validated' ) {
>      my @fields=qw/host port db userid password rank syntax encoding timeout
> +        recordtype checked servername servertype sru_options sru_fields 
> attributes opac

attributes is added by another patch on bug 11297, so should be marked as a
dependency or removed here.


::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-z3950-search.tt
@@ +52,5 @@
> +    { label => "Keyword (any)", prop => 'srchany' },
> +    { label => "LC call number", prop => 'lccall' },
> +    { label => "Control no.", prop => 'controlnumber' },
> +    { label => "Dewey", prop => 'dewey' },
> +    { label => "Standard ID", prop => 'stdid' }

Not sure if this syntax is translatable - needs testing.


::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/z3950servers.tt
@@ +235,4 @@
>          You searched for [% searchfield %]
>      [% END %]
>      <table id="serverst">
> +        
> <thead><tr><th>Target</th><th>Hostname/Port</th><th>Database</th><th>Userid</th><th>Password</th><th>Preselected</th><th>Rank</th><th>Syntax</th><th>Encoding</th><th>Timeout</th><th>Record
>  type</th><th>Attributes</th><th>OPAC</th><th>Options</th>

Attributes are from bug 11297 - see above.


Sponsored-by: CCSR ( http://www.ccsr.qc.ca )

-- 
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
Koha-bugs@lists.koha-community.org
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/

Reply via email to