From 96914c563eb5688351d29def9697a242a78f3552 Mon Sep 17 00:00:00 2001 From: Olli-Antti Kivilahti Date: Mon, 22 Feb 2016 12:33:07 +0200 Subject: [PATCH] Bug 15875 - Refactor DB accessors from "z39.50 admin setup / z3950servers.pl" to C4::Breeding z3950servers.pl is a horrible mess of hard SQL in the perl script/Controller-layer. Also a lack of proper internal API for mutating Z3950servers makes writing tests much harder. This patch refactors z3950servers.pl by moving SQL to C4::Breeding under proper accessors. --- C4/Breeding.pm | 165 +++++++++++++++++++++++++++++++++++++++++++++++++- admin/z3950servers.pl | 92 +++++++++++----------------- 2 files changed, 200 insertions(+), 57 deletions(-) diff --git a/C4/Breeding.pm b/C4/Breeding.pm index af7a4c2..9d3d0df 100644 --- a/C4/Breeding.pm +++ b/C4/Breeding.pm @@ -20,6 +20,7 @@ package C4::Breeding; use strict; use warnings; +use Data::Dumper; use C4::Biblio; use C4::Koha; @@ -28,6 +29,10 @@ use MARC::File::USMARC; use C4::ImportBatch; use C4::AuthoritiesMarc; #GuessAuthTypeCode, FindDuplicateAuthority +use Koha::Exception::DB; +use Koha::Exception::DuplicateObject; +use Koha::Exception::BadParameter; + use vars qw($VERSION @ISA @EXPORT @EXPORT_OK %EXPORT_TAGS); BEGIN { @@ -675,6 +680,164 @@ sub Z3950SearchAuth { ); } +=head getZ3950server + + my $servers = C4::Breeding::listZ3950servers({id => 443, name => "geigei"}); + +@RETURNS ARRAYRef of z3950servers-rows + +=cut + +sub listZ3950servers { + my ($params) = @_; + my $dbh=C4::Context->dbh; + my ($sth, $sql); + + $sql = "SELECT * FROM z3950servers "; + my @params; + my @andedColumns; + if (ref($params) eq 'HASH') { + while (my ($column, $value) = each(%$params)) { + push(@andedColumns, "$column = ?"); + push(@params, $value); + } + } + if (@params) { + $sql .= 'WHERE '.join(' AND ', @andedColumns); + } + $sql .= ' ORDER BY rank,name'; + + eval { + $sth = $dbh->prepare($sql); + $sth->execute( @params ); + }; + if ($@ || $sth->err) { + my @cc = caller(0); + my $paramsStr = Data::Dumper::Dumper($params); + Koha::Exception::DB->throw(error => $cc[3].'():>'.($@ || $sth->errstr)."\nWith search terms\n$paramsStr\n"); + } + return $sth->fetchall_arrayref({}); +} + +=head getZ3950server + + my $server = C4::Breeding::getZ3950server({id => 443, name => "geigei"}); + +=cut + +sub getZ3950server { + my ($params) = @_; + my $servers = listZ3950servers($params); + + if (not(ref($servers) eq 'ARRAY') || not(scalar(@$servers))) { + return undef; + } + elsif (scalar(@$servers) > 1) { + my @cc = caller(0); + my $paramsStr = Data::Dumper::Dumper($params); + Koha::Exception::DuplicateObject->throw(error => $cc[3]."():> Too many Z3950 servers found with search terms\n$paramsStr\n"); + } + return $servers->[0]; +} + +sub updateZ3950server { + my ($params) = @_; + my $dbh=C4::Context->dbh; + my ($sth, $sql); + + unless($params->{name}) { + my @cc = caller(0); + Koha::Exception::BadParameter->throw( error => $cc[3]."():> Missing attribute 'name'!" ); + } + + eval { + $sth=$dbh->prepare("update z3950servers set host=?, port=?, db=?, userid=?, password=?, name=?, checked=?, rank=?,syntax=?,encoding=?,timeout=?,recordtype=? where name=?"); + $sth->execute( + $params->{'host'}, + $params->{'port'}, + $params->{'db'}, + $params->{'userid'}, + $params->{'password'}, + $params->{'name'}, + $params->{'checked'} ? 1 : 0, + $params->{'rank'}, + $params->{'syntax'}, + $params->{'encoding'}, + $params->{'timeout'}, + $params->{'recordtype'}, + $params->{'name'}, + ); + }; + if ($@ || $sth->err) { + my @cc = caller(0); + Koha::Exception::DB->throw(error => $cc[3].'():>'.($@ || $sth->errstr)); + } +} + +sub addZ3950server { + my ($params) = @_; + my $dbh=C4::Context->dbh; + my ($sth, $sql); + + unless($params->{name}) { + my @cc = caller(0); + Koha::Exception::BadParameter->throw( error => $cc[3]."():> Missing attribute 'name'!" ); + } + + eval { + $sth=$dbh->prepare( + "INSERT INTO z3950servers " . + " (host,port,db,userid,password,name,checked,rank,syntax,encoding,timeout,recordtype) " . + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)" ); + $sth->execute( + $params->{'host'}, + $params->{'port'}, + $params->{'db'}, + $params->{'userid'}, + $params->{'password'}, + $params->{'name'}, + $params->{'checked'} ? 1 : 0, + $params->{'rank'}, + $params->{'syntax'}, + $params->{'encoding'}, + $params->{'timeout'}, + $params->{'recordtype'}, + ); + }; + if ($@ || $sth->err) { + my @cc = caller(0); + Koha::Exception::DB->throw(error => $cc[3].'():>'.($@ || $sth->errstr)); + } +} + +sub upsertZ3950server { + my ($params) = @_; + my $dbh=C4::Context->dbh; + my ($sth, $sql, $server); + + $server = getZ3950server( $params->{id} ? {id => $params->{id}} : {name => $params->{name}} ); + if ($server) { + C4::Breeding::updateZ3950server($params); + } + else { + C4::Breeding::addZ3950server($params); + } +} + +sub deleteZ3950server { + my ($id) = @_; + my $dbh=C4::Context->dbh; + my ($sth, $sql); + + eval { + $sth=$dbh->prepare("DELETE FROM z3950servers WHERE id=?"); + $sth->execute($id); + }; + if ($@ || $sth->err) { + my @cc = caller(0); + Koha::Exception::DB->throw(error => $cc[3].'():>'.($@ || $sth->errstr)); + } +} + 1; __END__ - diff --git a/admin/z3950servers.pl b/admin/z3950servers.pl index 1dbd118..2b7060a 100755 --- a/admin/z3950servers.pl +++ b/admin/z3950servers.pl @@ -25,6 +25,7 @@ use CGI; use C4::Context; use C4::Auth; use C4::Output; +use C4::Breeding; sub StringSearch { my ($searchstring,$type)=@_; @@ -90,14 +91,8 @@ if ( $op eq 'add_form' ) { #---- if primkey exists, it's a modify action, so read values to modify... my $data; - if ($searchfield) { - my $dbh = C4::Context->dbh; - my $sth = $dbh->prepare( -"select host,port,db,userid,password,name,id,checked,rank,syntax,encoding,timeout,recordtype from z3950servers where (name = ?) order by rank,name" - ); - $sth->execute($searchfield); - $data = $sth->fetchrow_hashref; - $sth->finish; + if (defined($searchfield)) { + $data = C4::Breeding::getZ3950server({name => $searchfield}); } $template->param( $_ => $data->{$_} ) for (qw( host port db userid password checked rank timeout encoding )); @@ -107,58 +102,25 @@ if ( $op eq 'add_form' ) { ################## ADD_VALIDATE ################################## # called by add_form, used to insert/modify data in DB } elsif ($op eq 'add_validate') { - my $dbh=C4::Context->dbh; - my $sth=$dbh->prepare("select * from z3950servers where name=?"); - $sth->execute($input->param('searchfield')); - my $checked = $input->param('checked') ? 1 : 0; - if ($sth->rows) { + my $params = castCGIToZ3950serverParams($input); + my $dbh=C4::Context->dbh; + my ($sth, $sql, $server); + + $server = C4::Breeding::getZ3950server( $params->{id} ? {id => $params->{id}} : {name => $params->{name}} ); + if ($server) { + C4::Breeding::updateZ3950server($params); $template->param(confirm_update => 1); - $sth=$dbh->prepare("update z3950servers set host=?, port=?, db=?, userid=?, password=?, name=?, checked=?, rank=?,syntax=?,encoding=?,timeout=?,recordtype=? where name=?"); - $sth->execute($input->param('host'), - $input->param('port'), - $input->param('db'), - $input->param('userid'), - $input->param('password'), - $input->param('searchfield'), - $checked, - $input->param('rank'), - $input->param('syntax'), - $input->param('encoding'), - $input->param('timeout'), - $input->param('recordtype'), - $input->param('searchfield'), - ); - } - else { + } + else { + C4::Breeding::addZ3950server($params); $template->param(confirm_add => 1); - $sth=$dbh->prepare( - "INSERT INTO z3950servers " . - "(host,port,db,userid,password,name,checked,rank,syntax,encoding,timeout,recordtype) " . - "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)" ); - $sth->execute( - $input->param('host'), $input->param('port'), - $input->param('db'), $input->param('userid'), - $input->param('password'), $input->param('searchfield'), - $checked, $input->param('rank'), - $input->param('syntax'), $input->param('encoding'), - $input->param('timeout'), $input->param('recordtype') - ); } - $sth->finish; - # END $OP eq ADD_VALIDATE ################## DELETE_CONFIRM ################################## # called by default form, used to confirm deletion of data in DB } elsif ($op eq 'delete_confirm') { $template->param( delete_confirm => 1 ); - my $dbh = C4::Context->dbh; - - my $sth2 = $dbh->prepare( -"select host,port,db,userid,password,name,id,checked,rank,syntax,encoding,timeout,recordtype from z3950servers where (name = ?) order by rank,name" - ); - $sth2->execute($searchfield); - my $data = $sth2->fetchrow_hashref; - $sth2->finish; + my $data = C4::Breeding::getZ3950server({name => $searchfield}); $template->param( host => $data->{'host'}, @@ -179,10 +141,8 @@ if ( $op eq 'add_form' ) { # called by delete_confirm, used to effectively confirm deletion of data in DB } elsif ($op eq 'delete_confirmed') { $template->param(delete_confirmed => 1); - my $dbh=C4::Context->dbh; - my $sth=$dbh->prepare("delete from z3950servers where name=?"); - $sth->execute($searchfield); - $sth->finish; + my $server = C4::Breeding::getZ3950server({name => $searchfield}); + C4::Breeding::deleteZ3950server($server->{id}); # END $OP eq DELETE_CONFIRMED ################## DEFAULT ################################## } else { # DEFAULT @@ -219,3 +179,23 @@ if ( $op eq 'add_form' ) { } } #---- END $OP eq DEFAULT output_html_with_http_headers $input, $cookie, $template->output; + +sub castCGIToZ3950serverParams { + my ($cgi) = @_; + + my $params = {}; + $params->{'host'} = $input->param('host'), + $params->{'port'} = $input->param('port'), + $params->{'db'} = $input->param('db'), + $params->{'userid'} = $input->param('userid'), + $params->{'password'} = $input->param('password'), + $params->{'name'} = $input->param('searchfield'), + $params->{'checked'} = $input->param('checked') ? 1 : 0, + $params->{'rank'} = $input->param('rank'), + $params->{'syntax'} = $input->param('syntax'), + $params->{'encoding'} = $input->param('encoding'), + $params->{'timeout'} = $input->param('timeout'), + $params->{'recordtype'} = $input->param('recordtype'), + $params->{'searchfield'} = $input->param('searchfield'), + return $params; +} \ No newline at end of file -- 1.9.1