From 62f3ad3266b5740d824451a6546b293c7ee1d163 Mon Sep 17 00:00:00 2001 From: Katrin Fischer Date: Mon, 9 Oct 2023 21:51:08 +0000 Subject: [PATCH] Bug 8367: (QA follow-up) Fix QA script * Add +x to atomic database update file * Perltidy * Add spans to rewritten tab label for translatability Signed-off-by: Katrin Fischer Signed-off-by: Kelly --- Koha/Hold.pm | 17 +- admin/smart-rules.pl | 162 +++++++++--------- ...g_8367-add_holds_pickup_period_circrule.pl | 12 +- .../prog/en/modules/circ/waitingreserves.tt | 2 +- t/db_dependent/Holds/WaitingReserves.t | 141 ++++++++------- 5 files changed, 178 insertions(+), 156 deletions(-) mode change 100644 => 100755 installer/data/mysql/atomicupdate/bug_8367-add_holds_pickup_period_circrule.pl diff --git a/Koha/Hold.pm b/Koha/Hold.pm index 9e2f23262c..0224d019b7 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -231,13 +231,16 @@ sub set_waiting { my $max_pickup_delay = C4::Context->preference("ReservesMaxPickUpDelay"); my $cancel_on_holidays = C4::Context->preference('ExpireReservesOnHolidays'); - my $rule = Koha::CirculationRules->get_effective_rule({ - categorycode => $self->borrower->categorycode, - itemtype => $self->item->effective_itemtype, - branchcode => $self->branchcode, - rule_name => 'holds_pickup_period', - }); - if ( defined($rule) and $rule->rule_value ne '' ){ + my $rule = Koha::CirculationRules->get_effective_rule( + { + categorycode => $self->borrower->categorycode, + itemtype => $self->item->effective_itemtype, + branchcode => $self->branchcode, + rule_name => 'holds_pickup_period', + } + ); + if ( defined($rule) and $rule->rule_value ne '' ) { + # circulation rule overrides ReservesMaxPickUpDelay $max_pickup_delay = $rule->rule_value; } diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl index 0d1b759d34..b1c7868184 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -74,8 +74,8 @@ if ($op eq 'delete') { Koha::CirculationRules->set_rules( { categorycode => $categorycode eq '*' ? undef : $categorycode, - branchcode => $branch eq '*' ? undef : $branch, - itemtype => $itemtype eq '*' ? undef : $itemtype, + branchcode => $branch eq '*' ? undef : $branch, + itemtype => $itemtype eq '*' ? undef : $itemtype, rules => { maxissueqty => undef, maxonsiteissueqty => undef, @@ -252,100 +252,102 @@ elsif ($op eq 'delete-branch-item') { } } # save the values entered -elsif ($op eq 'add') { - my $br = $branch; # branch - my $bor = $input->param('categorycode'); # borrower category - my $itemtype = $input->param('itemtype'); # item type - my $fine = $input->param('fine'); - my $finedays = $input->param('finedays'); - my $maxsuspensiondays = $input->param('maxsuspensiondays') || q{}; +elsif ( $op eq 'add' ) { + my $br = $branch; # branch + my $bor = $input->param('categorycode'); # borrower category + my $itemtype = $input->param('itemtype'); # item type + my $fine = $input->param('fine'); + my $finedays = $input->param('finedays'); + my $maxsuspensiondays = $input->param('maxsuspensiondays') || q{}; my $suspension_chargeperiod = $input->param('suspension_chargeperiod') || 1; - my $firstremind = $input->param('firstremind'); - my $chargeperiod = $input->param('chargeperiod'); - my $chargeperiod_charge_at = $input->param('chargeperiod_charge_at'); - my $maxissueqty = strip_non_numeric( scalar $input->param('maxissueqty') ); - 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 $firstremind = $input->param('firstremind'); + my $chargeperiod = $input->param('chargeperiod'); + my $chargeperiod_charge_at = $input->param('chargeperiod_charge_at'); + my $maxissueqty = strip_non_numeric( scalar $input->param('maxissueqty') ); + 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'); $norenewalbefore = q{} if $norenewalbefore =~ /^\s*$/; my $noautorenewalbefore = $input->param('noautorenewalbefore'); - $noautorenewalbefore = q{} if $noautorenewalbefore =~ /^\s*$/; - my $auto_renew = $input->param('auto_renew') eq 'yes' ? 1 : 0; + my $auto_renew = $input->param('auto_renew') eq 'yes' ? 1 : 0; my $no_auto_renewal_after = $input->param('no_auto_renewal_after'); $no_auto_renewal_after = q{} if $no_auto_renewal_after =~ /^\s*$/; my $no_auto_renewal_after_hard_limit = $input->param('no_auto_renewal_after_hard_limit') || q{}; - my $reservesallowed = strip_non_numeric( scalar $input->param('reservesallowed') ); - my $holds_per_record = strip_non_numeric( scalar $input->param('holds_per_record') ); - my $holds_per_day = strip_non_numeric( scalar $input->param('holds_per_day') ); - my $onshelfholds = $input->param('onshelfholds') || 0; - my $issuelength = $input->param('issuelength') || 0; - my $daysmode = $input->param('daysmode'); - my $lengthunit = $input->param('lengthunit'); - my $hardduedate = $input->param('hardduedate') || q{}; - my $hardduedatecompare = $input->param('hardduedatecompare'); - my $rentaldiscount = $input->param('rentaldiscount') || 0; - my $opacitemholds = $input->param('opacitemholds') || 0; - my $article_requests = $input->param('article_requests') || 'no'; - my $overduefinescap = $input->param('overduefinescap') + my $reservesallowed = strip_non_numeric( scalar $input->param('reservesallowed') ); + my $holds_per_record = strip_non_numeric( scalar $input->param('holds_per_record') ); + my $holds_per_day = strip_non_numeric( scalar $input->param('holds_per_day') ); + my $onshelfholds = $input->param('onshelfholds') || 0; + my $issuelength = $input->param('issuelength') || 0; + my $daysmode = $input->param('daysmode'); + my $lengthunit = $input->param('lengthunit'); + my $hardduedate = $input->param('hardduedate') || q{}; + my $hardduedatecompare = $input->param('hardduedatecompare'); + my $rentaldiscount = $input->param('rentaldiscount') || 0; + my $opacitemholds = $input->param('opacitemholds') || 0; + my $article_requests = $input->param('article_requests') || 'no'; + my $overduefinescap = $input->param('overduefinescap') && ( $input->param('overduefinescap') + 0 ) > 0 ? sprintf( "%.02f", $input->param('overduefinescap') ) : q{}; - my $cap_fine_to_replacement_price = ($input->param('cap_fine_to_replacement_price') || q{}) eq 'on'; - my $note = $input->param('note'); - my $decreaseloanholds = $input->param('decreaseloanholds') || q{}; - my $recalls_allowed = $input->param('recalls_allowed'); - my $recalls_per_record = $input->param('recalls_per_record'); - my $on_shelf_recalls = $input->param('on_shelf_recalls'); - my $recall_due_date_interval = $input->param('recall_due_date_interval'); - my $recall_overdue_fine = $input->param('recall_overdue_fine'); - my $recall_shelf_time = $input->param('recall_shelf_time'); - my $holds_pickup_period = strip_non_numeric( scalar $input->param('holds_pickup_period') ); + my $cap_fine_to_replacement_price = ( $input->param('cap_fine_to_replacement_price') || q{} ) eq 'on'; + my $note = $input->param('note'); + my $decreaseloanholds = $input->param('decreaseloanholds') || q{}; + my $recalls_allowed = $input->param('recalls_allowed'); + my $recalls_per_record = $input->param('recalls_per_record'); + my $on_shelf_recalls = $input->param('on_shelf_recalls'); + my $recall_due_date_interval = $input->param('recall_due_date_interval'); + my $recall_overdue_fine = $input->param('recall_overdue_fine'); + my $recall_shelf_time = $input->param('recall_shelf_time'); + my $holds_pickup_period = strip_non_numeric( scalar $input->param('holds_pickup_period') ); 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, - no_auto_renewal_after_hard_limit => $no_auto_renewal_after_hard_limit, - 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, - holds_pickup_period => $holds_pickup_period, + 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, + 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, + holds_pickup_period => $holds_pickup_period, }; Koha::CirculationRules->set_rules( { - categorycode => $bor eq '*' ? undef : $bor, + categorycode => $bor eq '*' ? undef : $bor, itemtype => $itemtype eq '*' ? undef : $itemtype, - branchcode => $br eq '*' ? undef : $br, + branchcode => $br eq '*' ? undef : $br, rules => $rules, } ); diff --git a/installer/data/mysql/atomicupdate/bug_8367-add_holds_pickup_period_circrule.pl b/installer/data/mysql/atomicupdate/bug_8367-add_holds_pickup_period_circrule.pl old mode 100644 new mode 100755 index efa20249b7..3e50bbf9a7 --- a/installer/data/mysql/atomicupdate/bug_8367-add_holds_pickup_period_circrule.pl +++ b/installer/data/mysql/atomicupdate/bug_8367-add_holds_pickup_period_circrule.pl @@ -1,17 +1,19 @@ use Modern::Perl; return { - bug_number => "8367", + bug_number => "8367", description => "Set hold pickup period circulation rule", - up => sub { + up => sub { my ($args) = @_; - my ($dbh, $out) = @$args{qw(dbh out)}; + my ( $dbh, $out ) = @$args{qw(dbh out)}; - $dbh->do(q{ + $dbh->do( + q{ INSERT IGNORE INTO circulation_rules ( branchcode, categorycode, itemtype, rule_name, rule_value ) SELECT u.* FROM (SELECT NULL as branchcode, NULL as categorycode, NULL as itemtype, 'holds_pickup_period' as rule_name, '' as rule_value) u WHERE NOT EXISTS ( SELECT rule_name FROM circulation_rules where rule_name = 'holds_pickup_period' ) - }); + } + ); say $out "Added default circulation rule for holds_pickup_period"; }, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt index ee259427da..97e72730bf 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/waitingreserves.tt @@ -99,7 +99,7 @@ Holds waiting: [% reservecount | html %] [% END %] [% WRAPPER tab_item tabname= "holdsover" %] - Holds waiting past their expiration date: [% overcount | html %] + Holds waiting past their expiration date: [% overcount | html %] [% END %] [% WRAPPER tab_item tabname= "holdscancelled" %] Holds with cancellation requests: [% cancel_reqs_count | html %] diff --git a/t/db_dependent/Holds/WaitingReserves.t b/t/db_dependent/Holds/WaitingReserves.t index 5f08ce73c3..b39ee11ce1 100755 --- a/t/db_dependent/Holds/WaitingReserves.t +++ b/t/db_dependent/Holds/WaitingReserves.t @@ -66,21 +66,23 @@ my $biblio4 = $builder->build_sample_biblio; my $biblio5 = $builder->build_sample_biblio; my $biblio6 = $builder->build_sample_biblio; -my $item1 = $builder->build_sample_item({biblionumber => $biblio->biblionumber}); -my $item2 = $builder->build_sample_item({biblionumber => $biblio2->biblionumber}); -my $item3 = $builder->build_sample_item({biblionumber => $biblio3->biblionumber}); -my $item4 = $builder->build_sample_item({biblionumber => $biblio4->biblionumber}); -my $item5 = $builder->build_sample_item({biblionumber => $biblio5->biblionumber}); -my $item6 = $builder->build_sample_item({biblionumber => $biblio6->biblionumber}); - -Koha::CirculationRules->set_rules({ - categorycode => undef, - itemtype => undef, - branchcode => undef, - rules => { - holds_pickup_period => undef, +my $item1 = $builder->build_sample_item( { biblionumber => $biblio->biblionumber } ); +my $item2 = $builder->build_sample_item( { biblionumber => $biblio2->biblionumber } ); +my $item3 = $builder->build_sample_item( { biblionumber => $biblio3->biblionumber } ); +my $item4 = $builder->build_sample_item( { biblionumber => $biblio4->biblionumber } ); +my $item5 = $builder->build_sample_item( { biblionumber => $biblio5->biblionumber } ); +my $item6 = $builder->build_sample_item( { biblionumber => $biblio6->biblionumber } ); + +Koha::CirculationRules->set_rules( + { + categorycode => undef, + itemtype => undef, + branchcode => undef, + rules => { + holds_pickup_period => undef, + } } -}); +); my $today = dt_from_string(); @@ -209,62 +211,75 @@ my $reserve4 = $builder->build({ t::lib::Mocks::mock_preference('ReservesMaxPickUpDelay', 10); ModReserveAffect( $item4->itemnumber, $patron2->{borrowernumber}, 0, $reserve4->{reserve_id}); -my $r4 = Koha::Holds->find($reserve4->{reserve_id}); -is($r4->expirationdate, $requested_expiredate->ymd, 'Requested expiration date should be kept' ); - -Koha::CirculationRules->set_rules({ - categorycode => $patron1->{categorycode}, - itemtype => undef, - branchcode => undef, - rules => { - holds_pickup_period => '3', +my $r4 = Koha::Holds->find( $reserve4->{reserve_id} ); +is( $r4->expirationdate, $requested_expiredate->ymd, 'Requested expiration date should be kept' ); + +Koha::CirculationRules->set_rules( + { + categorycode => $patron1->{categorycode}, + itemtype => undef, + branchcode => undef, + rules => { + holds_pickup_period => '3', + } } -}); -t::lib::Mocks::mock_preference('ReservesMaxPickUpDelay', 7); - -my $reserve5_reservedate = $today->clone; -my $reserve5_expirationdate = $reserve5_reservedate->add(days => 3); - -my $reserve5 = $builder->build({ - source => 'Reserve', - value => { - borrowernumber => $patron1->{borrowernumber}, - reservedate => $reserve5_reservedate->ymd, - expirationdate => undef, - biblionumber => $biblio5->biblionumber, - branchcode => 'LIB2', - priority => 1, - found => '', - patron_expiration_date => undef, - }, -}); +); +t::lib::Mocks::mock_preference( 'ReservesMaxPickUpDelay', 7 ); + +my $reserve5_reservedate = $today->clone; +my $reserve5_expirationdate = $reserve5_reservedate->add( days => 3 ); + +my $reserve5 = $builder->build( + { + source => 'Reserve', + value => { + borrowernumber => $patron1->{borrowernumber}, + reservedate => $reserve5_reservedate->ymd, + expirationdate => undef, + biblionumber => $biblio5->biblionumber, + branchcode => 'LIB2', + priority => 1, + found => '', + patron_expiration_date => undef, + }, + } +); -ModReserveAffect( $item5->itemnumber, $patron1->{borrowernumber}); -my $r5 = Koha::Holds->find($reserve5->{reserve_id}); +ModReserveAffect( $item5->itemnumber, $patron1->{borrowernumber} ); +my $r5 = Koha::Holds->find( $reserve5->{reserve_id} ); -is($r5->expirationdate, $reserve5_expirationdate->ymd, 'Expiration date should be set to today + 3 based on circulation rules' ); +is( + $r5->expirationdate, $reserve5_expirationdate->ymd, + 'Expiration date should be set to today + 3 based on circulation rules' +); my $reserve6_reservedate = $today->clone; -# add 3 days of pickup + 1 day of holiday -my $reserve6_expirationdate = $reserve6_reservedate->add(days => 5); -my $reserve6 = $builder->build({ - source => 'Reserve', - value => { - borrowernumber => $patron1->{borrowernumber}, - reservedate => $reserve6_reservedate->ymd, - expirationdate => undef, - biblionumber => $biblio6->biblionumber, - branchcode => 'LIB1', - priority => 1, - found => '', - patron_expiration_date => undef, - }, -}); +# add 3 days of pickup + 1 day of holiday +my $reserve6_expirationdate = $reserve6_reservedate->add( days => 5 ); + +my $reserve6 = $builder->build( + { + source => 'Reserve', + value => { + borrowernumber => $patron1->{borrowernumber}, + reservedate => $reserve6_reservedate->ymd, + expirationdate => undef, + biblionumber => $biblio6->biblionumber, + branchcode => 'LIB1', + priority => 1, + found => '', + patron_expiration_date => undef, + }, + } +); -ModReserveAffect( $item6->itemnumber, $patron1->{borrowernumber}); -my $r6 = Koha::Holds->find($reserve6->{reserve_id}); +ModReserveAffect( $item6->itemnumber, $patron1->{borrowernumber} ); +my $r6 = Koha::Holds->find( $reserve6->{reserve_id} ); -is($r6->expirationdate, $reserve6_expirationdate->ymd, 'Expiration date should be set to today + 4 based on circulation rules and including a holiday' ); +is( + $r6->expirationdate, $reserve6_expirationdate->ymd, + 'Expiration date should be set to today + 4 based on circulation rules and including a holiday' +); $schema->storage->txn_rollback; -- 2.30.2