From 85c822c5c75cc48156f2899aeed3238d52bef811 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 10 Apr 2015 11:31:46 +0200 Subject: [PATCH] Bug 9809: Get rid of reserveconstraints First, to be honest, I am not completely sure this patch won't introduce regression. We need several pairs of eyes, and if possible from someone in place for a long time in the project :) Once again this looks vestige of Koha 2, and comments like "FIXME", "This does not make sense", "probably never reached" appears all around this code. Confirmed, reserveconstraints was in commit d0374d003716dfb40796caad6390a4d69bfb0376 Author: rangi Date: Tue Dec 19 23:45:51 2000 +0000 Initial revision This patch is not complete, the reserves.constrainttype should be removed too (to confirm?), and all call to *Reserves using it be updated. What I understood: This feature has never been finished, and has never worked. Developers try not touch the code not to break something. The only place something could be inserted in this table is in C4::Reserves::AddReserve, if the parameter "constraint" starts with an "o" or "e" (hum...). The only place (I found) where it is possible is in the quite muddled serials/routing-preview.pl script (if RoutingListAddReserves is set). This parameter is set to 'o' since the beginning of the history and it's not clear why. I suspect a simple wrong c/p. There are 2 places where something could be retrieved from this table: - C4::Reserves::GetReservesFromBiblionumber. I did not find any calls using something else than the number of reserves or data from the reserves table. Looking at the number of FIXME in this area, not sure it has already worked. - _Findgroupreserve: I let you have a look at the sql query but to me this cannot return anything if the reserveconstraints table is empty. Test plan: 0/ Don't apply this patch 1/ Try to add something in the reserveconstraints table ( :D ) --- C4/Reserves.pm | 95 +--------------------- .../bug_9809-drop_table_reserveconstraints.sql | 1 + installer/data/mysql/kohastructure.sql | 13 --- reserve/placerequest.pl | 11 --- serials/routing-preview.pl | 3 +- 5 files changed, 3 insertions(+), 120 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_9809-drop_table_reserveconstraints.sql diff --git a/C4/Reserves.pm b/C4/Reserves.pm index b82387f..942a68c 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -239,21 +239,6 @@ sub AddReserve { ); } } - - #} - ($const eq "o" || $const eq "e") or return; # FIXME: why not have a useful return value? - $query = qq{ - INSERT INTO reserveconstraints - (borrowernumber,biblionumber,reservedate,biblioitemnumber) - VALUES - (?,?,?,?) - }; - $sth = $dbh->prepare($query); # keep prepare outside the loop! - foreach (@$bibitems) { - $sth->execute($borrowernumber, $biblionumber, $resdate, $_); - } - - return $reserve_id; } =head2 GetReserve @@ -331,50 +316,7 @@ sub GetReservesFromBiblionumber { push( @params, $itemnumber ); } $query .= "ORDER BY priority"; - my $sth = $dbh->prepare($query); - $sth->execute( @params ); - my @results; - my $i = 0; - while ( my $data = $sth->fetchrow_hashref ) { - - # FIXME - What is this doing? How do constraints work? - if ($data->{constrainttype} eq 'o') { - $query = ' - SELECT biblioitemnumber - FROM reserveconstraints - WHERE biblionumber = ? - AND borrowernumber = ? - AND reservedate = ? - '; - my $csth = $dbh->prepare($query); - $csth->execute($data->{biblionumber}, $data->{borrowernumber}, $data->{reservedate}); - my @bibitemno; - while ( my $bibitemnos = $csth->fetchrow_array ) { - push( @bibitemno, $bibitemnos ); # FIXME: inefficient: use fetchall_arrayref - } - my $count = scalar @bibitemno; - - # if we have two or more different specific itemtypes - # reserved by same person on same day - my $bdata; - if ( $count > 1 ) { - $bdata = GetBiblioItemData( $bibitemno[$i] ); # FIXME: This doesn't make sense. - $i++; # $i can increase each pass, but the next @bibitemno might be smaller? - } - else { - # Look up the book we just found. - $bdata = GetBiblioItemData( $bibitemno[0] ); - } - # Add the results of this latest search to the current - # results. - # FIXME - An 'each' would probably be more efficient. - foreach my $key ( keys %$bdata ) { - $data->{$key} = $bdata->{$key}; - } - } - push @results, $data; - } - return \@results; + return $dbh->selectall_arrayref( $query, { Slice => {} }, @params ); } =head2 GetReservesFromItemnumber @@ -1970,41 +1912,6 @@ sub _Findgroupreserve { push( @results, $data ) unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; } - return @results if @results; - - my $query = qq{ - SELECT reserves.biblionumber AS biblionumber, - reserves.borrowernumber AS borrowernumber, - reserves.reservedate AS reservedate, - reserves.waitingdate AS waitingdate, - reserves.branchcode AS branchcode, - reserves.cancellationdate AS cancellationdate, - reserves.found AS found, - reserves.reservenotes AS reservenotes, - reserves.priority AS priority, - reserves.timestamp AS timestamp, - reserveconstraints.biblioitemnumber AS biblioitemnumber, - reserves.itemnumber AS itemnumber, - reserves.reserve_id AS reserve_id - FROM reserves - LEFT JOIN reserveconstraints ON reserves.biblionumber = reserveconstraints.biblionumber - WHERE reserves.biblionumber = ? - AND ( ( reserveconstraints.biblioitemnumber = ? - AND reserves.borrowernumber = reserveconstraints.borrowernumber - AND reserves.reservedate = reserveconstraints.reservedate ) - OR reserves.constrainttype='a' ) - AND (reserves.itemnumber IS NULL OR reserves.itemnumber = ?) - AND reserves.reservedate <= DATE_ADD(NOW(),INTERVAL ? DAY) - AND suspend = 0 - ORDER BY priority - }; - $sth = $dbh->prepare($query); - $sth->execute( $biblio, $bibitem, $itemnumber, $lookahead||0); - @results = (); - while ( my $data = $sth->fetchrow_hashref ) { - push( @results, $data ) - unless any{ $data->{borrowernumber} eq $_ } @$ignore_borrowers ; - } return @results; } diff --git a/installer/data/mysql/atomicupdate/bug_9809-drop_table_reserveconstraints.sql b/installer/data/mysql/atomicupdate/bug_9809-drop_table_reserveconstraints.sql new file mode 100644 index 0000000..e73c88d --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_9809-drop_table_reserveconstraints.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS reserveconstraints; diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 6ddba43..3c61b41 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1829,19 +1829,6 @@ CREATE TABLE reports_dictionary ( -- definitions (or snippets of SQL) stored for ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; -- --- Table structure for table `reserveconstraints` --- - -DROP TABLE IF EXISTS `reserveconstraints`; -CREATE TABLE `reserveconstraints` ( - `borrowernumber` int(11) NOT NULL default 0, - `reservedate` date default NULL, - `biblionumber` int(11) NOT NULL default 0, - `biblioitemnumber` int(11) default NULL, - `timestamp` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP -) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci; - --- -- Table structure for table `reserves` -- diff --git a/reserve/placerequest.pl b/reserve/placerequest.pl index 1499ea5..af66edb 100755 --- a/reserve/placerequest.pl +++ b/reserve/placerequest.pl @@ -38,11 +38,6 @@ my $input = CGI->new(); checkauth($input, 0, { reserveforothers => 'place_holds' }, 'intranet'); my @bibitems=$input->param('biblioitem'); -# FIXME I think reqbib does not exist anymore, it's used in line 82, to AddReserve of contraint type 'o' -# I bet it's a 2.x feature, reserving a given biblioitem, that is useless in Koha 3.0 -# we can remove this line, the AddReserve of constrainttype 'o', -# and probably remove the reserveconstraint table as well, I never could fill anything in this table. -my @reqbib=$input->param('reqbib'); my $biblionumber=$input->param('biblionumber'); my $borrowernumber=$input->param('borrowernumber'); my $notes=$input->param('notes'); @@ -115,12 +110,6 @@ if ($type eq 'str8' && $borrower){ if ($input->param('request') eq 'any'){ # place a request on 1st available AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem,$found); - } elsif ($reqbib[0] ne ''){ - # FIXME : elsif probably never reached, (see top of the script) - # place a request on a given item - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'o',\@reqbib,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); - } else { - AddReserve($branch,$borrower->{'borrowernumber'},$biblionumber,'a',\@realbi,$rank[0],$startdate,$expirationdate,$notes,$title,$checkitem, $found); } } } diff --git a/serials/routing-preview.pl b/serials/routing-preview.pl index db0984c..4e23ed9 100755 --- a/serials/routing-preview.pl +++ b/serials/routing-preview.pl @@ -79,7 +79,6 @@ if($ok){ $count--; } } - my $const = 'o'; my $notes; my $title = $subs->{'bibliotitle'}; for my $routing ( @routinglist ) { @@ -95,7 +94,7 @@ if($ok){ branchcode => $branch }); } else { - AddReserve($branch,$routing->{borrowernumber},$biblio,$const,\@bibitems,$routing->{ranking}, undef, undef, $notes,$title); + AddReserve($branch,$routing->{borrowernumber},$biblio,undef,\@bibitems,$routing->{ranking}, undef, undef, $notes,$title); } } } -- 2.1.0