From e862d7ce606677516d2e3f9c97d7bc25f0d0dee5 Mon Sep 17 00:00:00 2001 From: Colin Campbell Date: Tue, 5 Jan 2010 10:13:16 +0000 Subject: [PATCH] Fix some code issues in circulation/returns Fix obvious warning generators use of string comparison on numeric values use of capture variables without testing comparison reuse of variable names in same lexical scope Tidy some layout issues remove commented out code remove unused variables remove tabs from mixed space tab layouts rewrite a couple of expressions where code flow obscured --- circ/circulation.pl | 193 +++++++++++++++++++------------------------- circ/returns.pl | 91 +++++++++++---------- 2 files changed, 132 insertions(+), 152 deletions(-) diff --git a/circ/circulation.pl b/circ/circulation.pl index 19df3bfc96..5ab38828c5 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -94,7 +94,6 @@ for (@failedrenews) { $renew_failed{$_} = 1; } my $findborrower = $query->param('findborrower'); $findborrower =~ s|,| |g; -#$findborrower =~ s|'| |g; my $borrowernumber = $query->param('borrowernumber'); $branch = C4::Context->userenv->{'branch'}; @@ -102,7 +101,7 @@ $printer = C4::Context->userenv->{'branchprinter'}; # If AutoLocation is not activated, we show the Circulation Parameters to chage settings of librarian -if (C4::Context->preference("AutoLocation") ne 1) { # FIXME: string comparison to number +if (C4::Context->preference("AutoLocation") != 1) { $template->param(ManualLocation => 1); } @@ -133,15 +132,6 @@ if ( $barcode ) { } } -#set up cookie..... -# my $branchcookie; -# my $printercookie; -# if ($query->param('setcookies')) { -# $branchcookie = $query->cookie(-name=>'branch', -value=>"$branch", -expires=>'+1y'); -# $printercookie = $query->cookie(-name=>'printer', -value=>"$printer", -expires=>'+1y'); -# } -# - my ($datedue,$invalidduedate,$globalduedate); if(C4::Context->preference('globalDueDate') && (C4::Context->preference('globalDueDate') =~ C4::Dates->regexp('syspref'))){ @@ -150,19 +140,19 @@ if(C4::Context->preference('globalDueDate') && (C4::Context->preference('globalD my $duedatespec_allow = C4::Context->preference('SpecifyDueDate'); if($duedatespec_allow){ if ($duedatespec) { - if ($duedatespec =~ C4::Dates->regexp('syspref')) { - my $tempdate = C4::Dates->new($duedatespec); - if ($tempdate and $tempdate->output('iso') gt C4::Dates->new()->output('iso')) { - # i.e., it has to be later than today/now - $datedue = $tempdate; - } else { - $invalidduedate = 1; - $template->param(IMPOSSIBLE=>1, INVALID_DATE=>$duedatespec); - } - } else { - $invalidduedate = 1; - $template->param(IMPOSSIBLE=>1, INVALID_DATE=>$duedatespec); - } + if ($duedatespec =~ C4::Dates->regexp('syspref')) { + my $tempdate = C4::Dates->new($duedatespec); + if ($tempdate and $tempdate->output('iso') gt C4::Dates->new()->output('iso')) { + # i.e., it has to be later than today/now + $datedue = $tempdate; + } else { + $invalidduedate = 1; + $template->param(IMPOSSIBLE=>1, INVALID_DATE=>$duedatespec); + } + } else { + $invalidduedate = 1; + $template->param(IMPOSSIBLE=>1, INVALID_DATE=>$duedatespec); + } } else { # pass global due date to tmpl if specifyduedate is true # and we have no barcode (loading circ page but not checking out) @@ -240,9 +230,12 @@ if ($borrowernumber) { my ($warning_year, $warning_month, $warning_day) = split /-/, $borrower->{'dateexpiry'}; my ( $enrol_year, $enrol_month, $enrol_day) = split /-/, $borrower->{'dateenrolled'}; # Renew day is calculated by adding the enrolment period to today - my ( $renew_year, $renew_month, $renew_day) = - Add_Delta_YM( $enrol_year, $enrol_month, $enrol_day, - 0 , $borrower->{'enrolmentperiod'}) if ($enrol_year*$enrol_month*$enrol_day>0); + my ( $renew_year, $renew_month, $renew_day); + if ($enrol_year*$enrol_month*$enrol_day>0) { + ( $renew_year, $renew_month, $renew_day) = + Add_Delta_YM( $enrol_year, $enrol_month, $enrol_day, + 0 , $borrower->{'enrolmentperiod'}); + } # if the expiry date is before today ie they have expired if ( $warning_year*$warning_month*$warning_day==0 || Date_to_Days($today_year, $today_month, $today_day ) @@ -280,44 +273,44 @@ if ($borrowernumber) { # # if ($barcode) { - # always check for blockers on issuing - my ( $error, $question ) = + # always check for blockers on issuing + my ( $error, $question ) = CanBookBeIssued( $borrower, $barcode, $datedue , $inprocess ); - my $blocker = $invalidduedate ? 1 : 0; + my $blocker = $invalidduedate ? 1 : 0; - delete $question->{'DEBT'} if ($debt_confirmed); - foreach my $impossible ( keys %$error ) { - $template->param( - $impossible => $$error{$impossible}, - IMPOSSIBLE => 1 - ); - $blocker = 1; - } + delete $question->{'DEBT'} if ($debt_confirmed); + foreach my $impossible ( keys %$error ) { + $template->param( + $impossible => $$error{$impossible}, + IMPOSSIBLE => 1 + ); + $blocker = 1; + } if( !$blocker ){ my $confirm_required = 0; - unless($issueconfirmed){ + unless($issueconfirmed){ # Get the item title for more information my $getmessageiteminfo = GetBiblioFromItemNumber(undef,$barcode); - $template->param( itemhomebranch => $getmessageiteminfo->{'homebranch'} ); - - # pass needsconfirmation to template if issuing is possible and user hasn't yet confirmed. - foreach my $needsconfirmation ( keys %$question ) { - $template->param( - $needsconfirmation => $$question{$needsconfirmation}, - getTitleMessageIteminfo => $getmessageiteminfo->{'title'}, - NEEDSCONFIRMATION => 1 - ); - $confirm_required = 1; - } - } + $template->param( itemhomebranch => $getmessageiteminfo->{'homebranch'} ); + + # pass needsconfirmation to template if issuing is possible and user hasn't yet confirmed. + foreach my $needsconfirmation ( keys %$question ) { + $template->param( + $needsconfirmation => $$question{$needsconfirmation}, + getTitleMessageIteminfo => $getmessageiteminfo->{'title'}, + NEEDSCONFIRMATION => 1 + ); + $confirm_required = 1; + } + } unless($confirm_required) { AddIssue( $borrower, $barcode, $datedue, $cancelreserve ); - $inprocess = 1; + $inprocess = 1; if($globalduedate && ! $stickyduedate && $duedatespec_allow ){ $duedatespec = $globalduedate->output(); $stickyduedate = 1; } - } + } } # FIXME If the issue is confirmed, we launch another time GetMemberIssuesAndFines, now display the issue count after issue @@ -333,8 +326,6 @@ if ($borrowernumber) { ################################################################################## # BUILD HTML # show all reserves of this borrower, and the position of the reservation .... -my $borrowercategory; -my $category_type; if ($borrowernumber) { # new op dev @@ -402,7 +393,7 @@ if ($borrowernumber) { push( @reservloop, \%getreserv ); # if we have a reserve waiting, initiate waitingreserveloop - if ($getreserv{waiting} eq 1) { + if ($getreserv{waiting} == 1) { push (@WaitingReserveLoop, \%getWaitingReserveInfo) } @@ -445,14 +436,14 @@ if ($borrower) { ); $it->{"renew_error_${can_renew_error}"} = 1 if defined $can_renew_error; my ( $restype, $reserves ) = CheckReserves( $it->{'itemnumber'} ); - $it->{'can_renew'} = $can_renew; - $it->{'can_confirm'} = !$can_renew && !$restype; - $it->{'renew_error'} = $restype; - $it->{'checkoutdate'} = C4::Dates->new($it->{'issuedate'},'iso')->output('syspref'); - - $totalprice += $it->{'replacementprice'}; - $it->{'itemtype'} = $itemtypeinfo->{'description'}; - $it->{'itemtype_image'} = $itemtypeinfo->{'imageurl'}; + $it->{'can_renew'} = $can_renew; + $it->{'can_confirm'} = !$can_renew && !$restype; + $it->{'renew_error'} = $restype; + $it->{'checkoutdate'} = C4::Dates->new($it->{'issuedate'},'iso')->output('syspref'); + + $totalprice += $it->{'replacementprice'}; + $it->{'itemtype'} = $itemtypeinfo->{'description'}; + $it->{'itemtype_image'} = $itemtypeinfo->{'imageurl'}; $it->{'dd'} = format_date($it->{'date_due'}); $it->{'issuedate'} = format_date($it->{'issuedate'}); $it->{'od'} = ( $it->{'date_due'} lt $todaysdate ) ? 1 : 0 ; @@ -487,13 +478,12 @@ if ($borrower) { my $dbh = C4::Context->dbh; # how many of each is allowed? -my $issueqty_sth = $dbh->prepare( " -SELECT itemtypes.description AS description,issuingrules.itemtype,maxissueqty -FROM issuingrules - LEFT JOIN itemtypes ON (itemtypes.itemtype=issuingrules.itemtype) - WHERE categorycode=? -" ); -$issueqty_sth->execute("*"); # This is a literal asterisk, not a wildcard. +my $issueqty_sth = $dbh->prepare( + 'SELECT itemtypes.description AS description,issuingrules.itemtype,maxissueqty ' . + 'FROM issuingrules LEFT JOIN itemtypes ON (itemtypes.itemtype=issuingrules.itemtype) ' . + 'WHERE categorycode=?' +); +$issueqty_sth->execute(q{*}); # This is a literal asterisk, not a wildcard. while ( my $data = $issueqty_sth->fetchrow_hashref() ) { @@ -504,12 +494,11 @@ while ( my $data = $issueqty_sth->fetchrow_hashref() ) { $issued_itemtypes_count->{ $data->{'description'} } ); # can't have a negative number of remaining - if ( $data->{'left'} < 0 ) { $data->{'left'} = "0" } - $data->{'flag'} = 1 unless ( $data->{'maxissueqty'} > $data->{'count'} ); - unless ( ( $data->{'maxissueqty'} < 1 ) - || ( $data->{'itemtype'} eq "*" ) - || ( $data->{'itemtype'} eq "CIRC" ) ) - { + if ( $data->{'left'} < 0 ) { $data->{'left'} = '0' } + if ( $data->{maxissueqty} <= $data->{count} ) { + $data->{flag} = 1; + } + if ( $data->{maxissueqty} > 0 && $data->{itemtype} !~m/^(\*|CIRC)$/ ) { push @issued_itemtypes_count_loop, $data; } } @@ -599,16 +588,7 @@ foreach my $flag ( sort keys %$flags ) { ); my $items = $flags->{$flag}->{'itemlist'}; -# useless ??? -# { -# my @itemswaiting; -# foreach my $item (@$items) { -# my ($iteminformation) = -# getiteminformation( $item->{'itemnumber'}, 0 ); -# push @itemswaiting, $iteminformation; -# } -# } - if ( ! $query->param('module') or $query->param('module') ne 'returns' ) { + if ( ! $query->param('module') || $query->param('module') ne 'returns' ) { $template->param( nonreturns => 'true' ); } } @@ -665,18 +645,18 @@ my $address = $borrower->{'streetnumber'}.' '.$roadttype_hashref->{$borrower->{' $template->param( issued_itemtypes_count_loop => \@issued_itemtypes_count_loop, - lib_messages_loop => $lib_messages_loop, - bor_messages_loop => $bor_messages_loop, - all_messages_del => C4::Context->preference('AllowAllMessageDeletion'), - findborrower => $findborrower, - borrower => $borrower, - borrowernumber => $borrowernumber, - branch => $branch, - branchname => GetBranchName($borrower->{'branchcode'}), - printer => $printer, - printername => $printer, - firstname => $borrower->{'firstname'}, - surname => $borrower->{'surname'}, + lib_messages_loop => $lib_messages_loop, + bor_messages_loop => $bor_messages_loop, + all_messages_del => C4::Context->preference('AllowAllMessageDeletion'), + findborrower => $findborrower, + borrower => $borrower, + borrowernumber => $borrowernumber, + branch => $branch, + branchname => GetBranchName($borrower->{'branchcode'}), + printer => $printer, + printername => $printer, + firstname => $borrower->{'firstname'}, + surname => $borrower->{'surname'}, dateexpiry => format_date($newexpiry), expiry => format_date($borrower->{'dateexpiry'}), categorycode => $borrower->{'categorycode'}, @@ -687,8 +667,8 @@ $template->param( emailpro => $borrower->{'emailpro'}, borrowernotes => $borrower->{'borrowernotes'}, city => $borrower->{'city'}, - zipcode => $borrower->{'zipcode'}, - country => $borrower->{'country'}, + zipcode => $borrower->{'zipcode'}, + country => $borrower->{'country'}, phone => $borrower->{'phone'} || $borrower->{'mobile'}, cardnumber => $borrower->{'cardnumber'}, amountold => $amountold, @@ -697,14 +677,14 @@ $template->param( duedatespec => $duedatespec, message => $message, CGIselectborrower => $CGIselectborrower, - totalprice => sprintf("%.2f", $totalprice), - totaldue => sprintf("%.2f", $total), + totalprice => sprintf('%.2f', $totalprice), + totaldue => sprintf('%.2f', $total), todayissues => \@todaysissues, previssues => \@previousissues, inprocess => $inprocess, memberofinstution => $member_of_institution, CGIorganisations => $CGIorganisations, - is_child => ($borrower->{'category_type'} eq 'C'), + is_child => ($borrower->{'category_type'} eq 'C'), circview => 1, ); @@ -713,16 +693,11 @@ if ($stickyduedate) { $session->param( 'stickyduedate', $duedatespec ); } -#if ($branchcookie) { -#$cookie=[$cookie, $branchcookie, $printercookie]; -#} - my ($picture, $dberror) = GetPatronImage($borrower->{'cardnumber'}); $template->param( picture => 1 ) if $picture; # get authorised values with type of BOR_NOTES my @canned_notes; -my $dbh = C4::Context->dbh; my $sth = $dbh->prepare('SELECT * FROM authorised_values WHERE category = "BOR_NOTES"'); $sth->execute(); while ( my $row = $sth->fetchrow_hashref() ) { diff --git a/circ/returns.pl b/circ/returns.pl index 40eeed153f..be8da4f1ee 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -47,13 +47,13 @@ use C4::Koha; # FIXME : is it still useful ? my $query = new CGI; if (!C4::Context->userenv){ - my $sessionID = $query->cookie("CGISESSID"); - my $session = get_session($sessionID); - if ($session->param('branch') eq 'NO_LIBRARY_SET'){ - # no branch set we can't return - print $query->redirect("/cgi-bin/koha/circ/selectbranchprinter.pl"); - exit; - } + my $sessionID = $query->cookie("CGISESSID"); + my $session = get_session($sessionID); + if ($session->param('branch') eq 'NO_LIBRARY_SET'){ + # no branch set we can't return + print $query->redirect("/cgi-bin/koha/circ/selectbranchprinter.pl"); + exit; + } } #getting the template @@ -72,7 +72,6 @@ my ( $template, $librarian, $cookie ) = get_template_and_user( my $branches = GetBranches(); my $printers = GetPrinters(); -#my $branch = C4::Context->userenv?C4::Context->userenv->{'branch'}:""; my $printer = C4::Context->userenv ? C4::Context->userenv->{'branchprinter'} : ""; my $overduecharges = (C4::Context->preference('finesMode') && C4::Context->preference('finesMode') ne 'off'); @@ -87,10 +86,18 @@ my %riduedate; my %riborrowernumber; my @inputloop; foreach ( $query->param ) { - (next) unless (/ri-(\d*)/); + my $counter; + if (/ri-(\d*)/) { + $counter = $1; + if ($counter > 20) { + next; + } + } + else { + next; + } + my %input; - my $counter = $1; - (next) if ( $counter > 20 ); my $barcode = $query->param("ri-$counter"); my $duedate = $query->param("dd-$counter"); my $borrowernumber = $query->param("bn-$counter"); @@ -140,7 +147,7 @@ if ( $query->param('resbarcode') ) { if ( $messages->{'transfert'} ) { $template->param( itemtitle => $iteminfo->{'title'}, - itembiblionumber => $iteminfo->{'biblionumber'}, + itembiblionumber => $iteminfo->{'biblionumber'}, iteminfo => $iteminfo->{'author'}, tobranchname => GetBranchName($messages->{'transfert'}), name => $name, @@ -163,15 +170,15 @@ my $exemptfine = $query->param('exemptfine'); my $dropboxmode = $query->param('dropboxmode'); my $dotransfer = $query->param('dotransfer'); my $calendar = C4::Calendar->new( branchcode => $userenv_branch ); - #dropbox: get last open day (today - 1) +#dropbox: get last open day (today - 1) my $today = C4::Dates->new(); my $today_iso = $today->output('iso'); my $dropboxdate = $calendar->addDate($today, -1); if ($dotransfer){ - # An item has been returned to a branch other than the homebranch, and the librarian has chosen to initiate a transfer - my $transferitem = $query->param('transferitem'); - my $tobranch = $query->param('tobranch'); - ModItemTransfer($transferitem, $userenv_branch, $tobranch); +# An item has been returned to a branch other than the homebranch, and the librarian has chosen to initiate a transfer + my $transferitem = $query->param('transferitem'); + my $tobranch = $query->param('tobranch'); + ModItemTransfer($transferitem, $userenv_branch, $tobranch); } # actually return book and prepare item table..... @@ -263,23 +270,23 @@ if ( $messages->{'WasTransfered'} ) { } if ( $messages->{'NeedsTransfer'} ){ - $template->param( - found => 1, - needstransfer => 1, - itemnumber => $itemnumber, - ); + $template->param( + found => 1, + needstransfer => 1, + itemnumber => $itemnumber, + ); } if ( $messages->{'Wrongbranch'} ){ - $template->param( - wrongbranch => 1, - ); + $template->param( + wrongbranch => 1, + ); } # case of wrong transfert, if the document wasn't transfered to the right library (according to branchtransfer (tobranch) BDD) if ( $messages->{'WrongTransfer'} and not $messages->{'WasTransfered'}) { - $template->param( + $template->param( WrongTransfer => 1, TransferWaitingAt => $messages->{'WrongTransfer'}, WrongTransferItem => $messages->{'WrongTransferItem'}, @@ -347,7 +354,7 @@ if ( $messages->{'ResFound'}) { debarred => $borr->{'debarred'}, gonenoaddress => $borr->{'gonenoaddress'}, barcode => $barcode, - destbranch => $reserve->{'branchcode'}, + destbranch => $reserve->{'branchcode'}, borrowernumber => $reserve->{'borrowernumber'}, itemnumber => $reserve->{'itemnumber'}, reservenotes => $reserve->{'reservenotes'}, @@ -401,7 +408,7 @@ foreach my $code ( keys %$messages ) { } elsif ( $code eq 'Wrongbranch' ) { } - + else { die "Unknown error code $code"; # note we need all the (empty) elsif's above, or we die. # This forces the issue of staying in sync w/ Circulation.pm @@ -487,32 +494,30 @@ my @riloop; foreach ( sort { $a <=> $b } keys %returneditems ) { my %ri; if ( $count++ < $returned_counter ) { - my $barcode = $returneditems{$_}; + my $bar_code = $returneditems{$_}; my $duedate = $riduedate{$_}; - my $overduetext; - my $borrowerinfo; if ($duedate) { my @tempdate = split( /-/, $duedate ); $ri{year} = $tempdate[0]; $ri{month} = $tempdate[1]; $ri{day} = $tempdate[2]; $ri{duedate} = format_date($duedate); - my ($borrower) = GetMemberDetails( $riborrowernumber{$_}, 0 ); + my ($b) = GetMemberDetails( $riborrowernumber{$_}, 0 ); $ri{return_overdue} = 1 if ($duedate lt $today->output('iso')); - $ri{borrowernumber} = $borrower->{'borrowernumber'}; - $ri{borcnum} = $borrower->{'cardnumber'}; - $ri{borfirstname} = $borrower->{'firstname'}; - $ri{borsurname} = $borrower->{'surname'}; - $ri{bortitle} = $borrower->{'title'}; - $ri{bornote} = $borrower->{'borrowernotes'}; - $ri{borcategorycode}= $borrower->{'categorycode'}; + $ri{borrowernumber} = $b->{'borrowernumber'}; + $ri{borcnum} = $b->{'cardnumber'}; + $ri{borfirstname} = $b->{'firstname'}; + $ri{borsurname} = $b->{'surname'}; + $ri{bortitle} = $b->{'title'}; + $ri{bornote} = $b->{'borrowernotes'}; + $ri{borcategorycode}= $b->{'categorycode'}; } else { $ri{borrowernumber} = $riborrowernumber{$_}; } # my %ri; - my $biblio = GetBiblioFromItemNumber(GetItemnumberFromBarcode($barcode)); + my $biblio = GetBiblioFromItemNumber(GetItemnumberFromBarcode($bar_code)); # fix up item type for display $biblio->{'itemtype'} = C4::Context->preference('item-level_itypes') ? $biblio->{'itype'} : $biblio->{'itemtype'}; $ri{itembiblionumber} = $biblio->{'biblionumber'}; @@ -522,12 +527,12 @@ foreach ( sort { $a <=> $b } keys %returneditems ) { $ri{itemnote} = $biblio->{'itemnotes'}; $ri{ccode} = $biblio->{'ccode'}; $ri{itemnumber} = $biblio->{'itemnumber'}; - $ri{barcode} = $barcode; + $ri{barcode} = $bar_code; } else { last; } - push( @riloop, \%ri ); + push @riloop, \%ri; } $template->param( @@ -539,7 +544,7 @@ $template->param( errmsgloop => \@errmsgloop, exemptfine => $exemptfine, dropboxmode => $dropboxmode, - dropboxdate => $dropboxdate->output(), + dropboxdate => $dropboxdate->output(), overduecharges => $overduecharges, ); -- 2.39.5