From 5b18fa1fc26e5c20cd7695d607b03bb59adb91e6 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Wed, 11 Oct 2023 14:40:37 +0000 Subject: [PATCH] Bug 25393: (QA follow-up) Tidy Signed-off-by: Nick Clemens Signed-off-by: Tomas Cohen Arazi --- admin/smart-rules.pl | 80 ++++++------- .../data/mysql/atomicupdate/bug_25393.pl | 11 +- misc/cronjobs/automatic_renewals.pl | 55 ++++----- svc/checkouts | 108 +++++++++--------- t/db_dependent/Circulation.t | 27 +++-- 5 files changed, 145 insertions(+), 136 deletions(-) diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 68d28c321e..499d7e8e5a 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -266,8 +266,8 @@ elsif ($op eq 'add') { my $maxonsiteissueqty = strip_non_numeric( scalar $input->param('maxonsiteissueqty') ); my $renewalsallowed = $input->param('renewalsallowed'); my $unseen_renewals_allowed = defined $input->param('unseen_renewals_allowed') ? strip_non_numeric( scalar $input->param('unseen_renewals_allowed') ) : q{}; - my $renewalperiod = $input->param('renewalperiod'); - my $norenewalbefore = $input->param('norenewalbefore'); + my $renewalperiod = $input->param('renewalperiod'); + my $norenewalbefore = $input->param('norenewalbefore'); $norenewalbefore = q{} if $norenewalbefore =~ /^\s*$/; my $noautorenewalbefore = $input->param('noautorenewalbefore'); $noautorenewalbefore = q{} if $noautorenewalbefore =~ /^\s*$/; @@ -300,45 +300,45 @@ elsif ($op eq 'add') { my $recall_shelf_time = $input->param('recall_shelf_time'); my $rules = { - maxissueqty => $maxissueqty, - maxonsiteissueqty => $maxonsiteissueqty, - rentaldiscount => $rentaldiscount, - fine => $fine, - finedays => $finedays, - maxsuspensiondays => $maxsuspensiondays, - suspension_chargeperiod => $suspension_chargeperiod, - firstremind => $firstremind, - chargeperiod => $chargeperiod, - chargeperiod_charge_at => $chargeperiod_charge_at, - issuelength => $issuelength, - daysmode => $daysmode, - lengthunit => $lengthunit, - hardduedate => $hardduedate, - hardduedatecompare => $hardduedatecompare, - renewalsallowed => $renewalsallowed, - unseen_renewals_allowed => $unseen_renewals_allowed, - renewalperiod => $renewalperiod, - norenewalbefore => $norenewalbefore, - noautorenewalbefore => $noautorenewalbefore, - auto_renew => $auto_renew, - no_auto_renewal_after => $no_auto_renewal_after, + maxissueqty => $maxissueqty, + maxonsiteissueqty => $maxonsiteissueqty, + rentaldiscount => $rentaldiscount, + fine => $fine, + finedays => $finedays, + maxsuspensiondays => $maxsuspensiondays, + suspension_chargeperiod => $suspension_chargeperiod, + firstremind => $firstremind, + chargeperiod => $chargeperiod, + chargeperiod_charge_at => $chargeperiod_charge_at, + issuelength => $issuelength, + daysmode => $daysmode, + lengthunit => $lengthunit, + hardduedate => $hardduedate, + hardduedatecompare => $hardduedatecompare, + renewalsallowed => $renewalsallowed, + unseen_renewals_allowed => $unseen_renewals_allowed, + renewalperiod => $renewalperiod, + norenewalbefore => $norenewalbefore, + noautorenewalbefore => $noautorenewalbefore, + auto_renew => $auto_renew, + no_auto_renewal_after => $no_auto_renewal_after, no_auto_renewal_after_hard_limit => $no_auto_renewal_after_hard_limit, - reservesallowed => $reservesallowed, - holds_per_record => $holds_per_record, - holds_per_day => $holds_per_day, - onshelfholds => $onshelfholds, - opacitemholds => $opacitemholds, - overduefinescap => $overduefinescap, - cap_fine_to_replacement_price => $cap_fine_to_replacement_price, - article_requests => $article_requests, - note => $note, - decreaseloanholds => $decreaseloanholds, - recalls_allowed => $recalls_allowed, - recalls_per_record => $recalls_per_record, - on_shelf_recalls => $on_shelf_recalls, - recall_due_date_interval => $recall_due_date_interval, - recall_overdue_fine => $recall_overdue_fine, - recall_shelf_time => $recall_shelf_time, + reservesallowed => $reservesallowed, + holds_per_record => $holds_per_record, + holds_per_day => $holds_per_day, + onshelfholds => $onshelfholds, + opacitemholds => $opacitemholds, + overduefinescap => $overduefinescap, + cap_fine_to_replacement_price => $cap_fine_to_replacement_price, + article_requests => $article_requests, + note => $note, + decreaseloanholds => $decreaseloanholds, + recalls_allowed => $recalls_allowed, + recalls_per_record => $recalls_per_record, + on_shelf_recalls => $on_shelf_recalls, + recall_due_date_interval => $recall_due_date_interval, + recall_overdue_fine => $recall_overdue_fine, + recall_shelf_time => $recall_shelf_time, }; Koha::CirculationRules->set_rules( diff --git a/installer/data/mysql/atomicupdate/bug_25393.pl b/installer/data/mysql/atomicupdate/bug_25393.pl index 1b8fd1b5ff..d34c18f1d5 100755 --- a/installer/data/mysql/atomicupdate/bug_25393.pl +++ b/installer/data/mysql/atomicupdate/bug_25393.pl @@ -3,7 +3,7 @@ use Modern::Perl; return { bug_number => "25393", description => "Create separate 'no automatic renewal before' rule", - up => sub { + up => sub { my ($args) = @_; my ( $dbh, $out ) = @$args{qw(dbh out)}; @@ -12,7 +12,7 @@ return { { Slice => {} } ); - if(!scalar @{$rules}){ + if ( !scalar @{$rules} ) { my $existing_rules = $dbh->selectall_arrayref( q|SELECT * FROM circulation_rules WHERE rule_name = "norenewalbefore"|, { Slice => {} } @@ -31,10 +31,11 @@ return { $existing_rule->{rule_value} ); } - say $out "Bug 25939: New circulation rule 'noautorenewalbefore' has been added. Defaulting value to 'norenewalbefore'."; - }else{ + say $out + "Bug 25939: New circulation rule 'noautorenewalbefore' has been added. Defaulting value to 'norenewalbefore'."; + } else { say $out "Bug 25939: Circulation rule 'noautorenewalbefore' found. Skipping update."; } - }, + }, }; diff --git a/misc/cronjobs/automatic_renewals.pl b/misc/cronjobs/automatic_renewals.pl index 8115afee7c..16006d5989 100755 --- a/misc/cronjobs/automatic_renewals.pl +++ b/misc/cronjobs/automatic_renewals.pl @@ -178,46 +178,47 @@ while ( my $auto_renew = $auto_renews->next ) { $updated = 1; if ($verbose) { say sprintf "Issue id: %s for borrower: %s and item: %s %s be renewed.", - $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, $confirm ? 'will' : 'would'; + $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, + $confirm ? 'will' : 'would'; } - if ($confirm){ + if ($confirm) { my $date_due = AddRenewal( { - borrowernumber => $auto_renew->borrowernumber, - itemnumber => $auto_renew->itemnumber, - branch => $auto_renew->branchcode, - seen => 0, - automatic => 1, + borrowernumber => $auto_renew->borrowernumber, + itemnumber => $auto_renew->itemnumber, + branch => $auto_renew->branchcode, + seen => 0, + automatic => 1, } ); push @item_renewal_ids, $auto_renew->itemnumber; $auto_renew->auto_renew_error(undef)->store; } push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew - if ( $wants_messages ) && !$wants_digest; - } elsif ( - $error eq 'too_many' || - $error eq 'on_reserve' || - $error eq 'restriction' || - $error eq 'overdue' || - $error eq 'too_unseen' || - $error eq 'auto_account_expired' || - $error eq 'auto_too_late' || - $error eq 'auto_too_much_oweing' || - $error eq 'auto_too_soon' || - $error eq 'too_soon' || - $error eq 'item_denied_renewal' || - $error eq 'item_issued_to_other_patron' - ) { - if ( $verbose ) { + if ($wants_messages) && !$wants_digest; + } elsif ( $error eq 'too_many' + || $error eq 'on_reserve' + || $error eq 'restriction' + || $error eq 'overdue' + || $error eq 'too_unseen' + || $error eq 'auto_account_expired' + || $error eq 'auto_too_late' + || $error eq 'auto_too_much_oweing' + || $error eq 'auto_too_soon' + || $error eq 'too_soon' + || $error eq 'item_denied_renewal' + || $error eq 'item_issued_to_other_patron' ) + { + if ($verbose) { say sprintf "Issue id: %s for borrower: %s and item: %s %s not be renewed. (%s)", - $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, $confirm ? 'will' : 'would', $error; + $auto_renew->issue_id, $auto_renew->borrowernumber, $auto_renew->itemnumber, + $confirm ? 'will' : 'would', $error; } - $updated = 1 if (!$auto_renew->auto_renew_error || $error ne $auto_renew->auto_renew_error); - if ( $updated ) { + $updated = 1 if ( !$auto_renew->auto_renew_error || $error ne $auto_renew->auto_renew_error ); + if ($updated) { $auto_renew->auto_renew_error($error)->store if $confirm; push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew - if $error ne 'auto_too_soon' && ( $wants_messages && !$wants_digest ); # Do not notify if it's too soon + if $error ne 'auto_too_soon' && ( $wants_messages && !$wants_digest ); # Do not notify if it's too soon } } diff --git a/svc/checkouts b/svc/checkouts index e45582d85c..ae04eb9005 100755 --- a/svc/checkouts +++ b/svc/checkouts @@ -240,67 +240,71 @@ while ( my $c = $sth->fetchrow_hashref() ) { } my $checkout = { - DT_RowId => $c->{itemnumber} . '-' . $c->{borrowernumber}, - title => $c->{title}, - subtitle => \@subtitles, - medium => $c->{medium} // '', - part_number => $c->{part_number} // '', - part_name => $c->{part_name} // '', - author => $c->{author}, - barcode => $c->{barcode}, + DT_RowId => $c->{itemnumber} . '-' . $c->{borrowernumber}, + title => $c->{title}, + subtitle => \@subtitles, + medium => $c->{medium} // '', + part_number => $c->{part_number} // '', + part_name => $c->{part_name} // '', + author => $c->{author}, + barcode => $c->{barcode}, type_for_stat => $type_for_stat || q{}, itemtype_description => $itemtype || q{}, recordtype_description => $recordtype || q{}, - collection => $collection, - location => $location, - homebranch => $c->{homebranch}, - itemnotes => $c->{itemnotes}, - itemnotes_nonpublic => $c->{itemnotes_nonpublic}, - branchcode => $c->{branchcode}, - branchname => $c->{branchname}, - itemcallnumber => $c->{itemcallnumber} || q{}, - copynumber => $c->{copynumber} || q{}, - charge => $charge, - fine => $fine, - price => $c->{replacementprice} || q{}, - can_renew => $can_renew, - can_renew_error => $can_renew_error, - can_renew_date => $can_renew_date, - itemnumber => $c->{itemnumber}, - borrowernumber => $c->{borrowernumber}, - biblionumber => $c->{biblionumber}, - issuedate => $c->{issuedate}, - date_due => $c->{date_due}, - date_due_overdue => $c->{date_due_overdue} ? JSON::true : JSON::false, - timestamp => $c->{timestamp}, - auto_renew => $c->{auto_renew}, - onsite_checkout => $c->{onsite_checkout}, - enumchron => $c->{enumchron}, - renewals_count => $renewals_count, - renewals_allowed => $renewals_allowed || 0, - renewals_remaining => $renewals_remaining, - unseen_count => $unseen_count, - unseen_allowed => $unseen_allowed, - unseen_remaining => $unseen_remaining, - - return_claim_id => $c->{return_claim_id}, - return_claim_notes => $c->{return_claim_notes}, - return_claim_created_on => $c->{return_claim_created_on}, - return_claim_updated_on => $c->{return_claim_updated_on}, - return_claim_created_on_formatted => $c->{return_claim_created_on} ? output_pref({ dt => dt_from_string( $c->{return_claim_created_on} ) }) : undef, - return_claim_updated_on_formatted => $c->{return_claim_updated_on} ? output_pref({ dt => dt_from_string( $c->{return_claim_updated_on} ) }) : undef, - - lost => $lost, + collection => $collection, + location => $location, + homebranch => $c->{homebranch}, + itemnotes => $c->{itemnotes}, + itemnotes_nonpublic => $c->{itemnotes_nonpublic}, + branchcode => $c->{branchcode}, + branchname => $c->{branchname}, + itemcallnumber => $c->{itemcallnumber} || q{}, + copynumber => $c->{copynumber} || q{}, + charge => $charge, + fine => $fine, + price => $c->{replacementprice} || q{}, + can_renew => $can_renew, + can_renew_error => $can_renew_error, + can_renew_date => $can_renew_date, + itemnumber => $c->{itemnumber}, + borrowernumber => $c->{borrowernumber}, + biblionumber => $c->{biblionumber}, + issuedate => $c->{issuedate}, + date_due => $c->{date_due}, + date_due_overdue => $c->{date_due_overdue} ? JSON::true : JSON::false, + timestamp => $c->{timestamp}, + auto_renew => $c->{auto_renew}, + onsite_checkout => $c->{onsite_checkout}, + enumchron => $c->{enumchron}, + renewals_count => $renewals_count, + renewals_allowed => $renewals_allowed || 0, + renewals_remaining => $renewals_remaining, + unseen_count => $unseen_count, + unseen_allowed => $unseen_allowed, + unseen_remaining => $unseen_remaining, + + return_claim_id => $c->{return_claim_id}, + return_claim_notes => $c->{return_claim_notes}, + return_claim_created_on => $c->{return_claim_created_on}, + return_claim_updated_on => $c->{return_claim_updated_on}, + return_claim_created_on_formatted => $c->{return_claim_created_on} + ? output_pref( { dt => dt_from_string( $c->{return_claim_created_on} ) } ) + : undef, + return_claim_updated_on_formatted => $c->{return_claim_updated_on} + ? output_pref( { dt => dt_from_string( $c->{return_claim_updated_on} ) } ) + : undef, + + lost => $lost, claims_returned => $claims_returned, - damaged => $damaged, - materials => $materials, - borrower => { + damaged => $damaged, + materials => $materials, + borrower => { surname => $c->{surname}, firstname => $c->{firstname}, cardnumber => $c->{cardnumber}, }, issued_today => !$c->{not_issued_today}, - recalled => $recalled, + recalled => $recalled, }; if ( $c->{not_issued_today} ) { diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index 4c6138be58..fcd68e9bbe 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -278,16 +278,16 @@ Koha::CirculationRules->set_rules( branchcode => undef, itemtype => undef, rules => { - reservesallowed => 25, - issuelength => 14, - lengthunit => 'days', - renewalsallowed => 1, - renewalperiod => 7, + reservesallowed => 25, + issuelength => 14, + lengthunit => 'days', + renewalsallowed => 1, + renewalperiod => 7, norenewalbefore => undef, noautorenewalbefore => undef, - auto_renew => 0, - fine => .10, - chargeperiod => 1, + auto_renew => 0, + fine => .10, + chargeperiod => 1, } } ); @@ -845,10 +845,10 @@ subtest "CanBookBeRenewed tests" => sub { } ); - my $auto_renew_issue = AddIssue( $renewing_borrower_obj, $item_4->barcode, undef, undef, undef, undef, { auto_renew => 1 } ); + my $auto_renew_issue = + AddIssue( $renewing_borrower_obj, $item_4->barcode, undef, undef, undef, undef, { auto_renew => 1 } ); my $info; - ( $renewokay, $error, $info ) = - CanBookBeRenewed( $renewing_borrower_obj, $auto_renew_issue ); + ( $renewokay, $error, $info ) = CanBookBeRenewed( $renewing_borrower_obj, $auto_renew_issue ); is( $info->{soonest_renew_date}, undef, "soonest_renew_date is not returned because this issue can be renewed" ); is( $renewokay, 1, 'Bug 25393: Can do a manual renew, even if renewal is automatic and premature' ); is( @@ -939,7 +939,10 @@ subtest "CanBookBeRenewed tests" => sub { is( $error, 'too_soon', 'Bug 25393: Cannot renew, renewal is premature (returned code is too_soon)' ); - is( $info->{soonest_renew_date}, dt_from_string($auto_renew_issue->date_due)->subtract( days => 7 ), "Soonest renew date returned when error is 'auto_too_soon'"); + is( + $info->{soonest_renew_date}, dt_from_string( $auto_renew_issue->date_due )->subtract( days => 7 ), + "Soonest renew date returned when error is 'auto_too_soon'" + ); ( $renewokay_cron, $error_cron, $info_cron ) = CanBookBeRenewed( $renewing_borrower_obj, $auto_renew_issue, undef, 1 ); -- 2.39.5