Bugzilla – Attachment 156840 Details for
Bug 25393
Create separate 'no automatic renewal before' rule
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 25393: (QA follow-up) Tidy
Bug-25393-QA-follow-up-Tidy.patch (text/plain), 20.16 KB, created by
Nick Clemens (kidclamp)
on 2023-10-11 14:56:48 UTC
(
hide
)
Description:
Bug 25393: (QA follow-up) Tidy
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2023-10-11 14:56:48 UTC
Size:
20.16 KB
patch
obsolete
>From c5f1f76b38ec3779324b790e575f9281800398ec Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Wed, 11 Oct 2023 14:40:37 +0000 >Subject: [PATCH] Bug 25393: (QA follow-up) Tidy > >Signed-off-by: Nick Clemens <nick@bywatersolutions.com> >--- > 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.30.2
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 25393
:
154533
|
154534
|
154535
|
154536
|
154537
|
154538
|
154539
|
154540
|
154541
|
154661
|
154662
|
154663
|
154664
|
154665
|
154666
|
154667
|
154668
|
154669
|
155307
|
155308
|
155309
|
155310
|
155311
|
155312
|
155313
|
155314
|
155315
|
156829
|
156830
|
156831
|
156832
|
156833
|
156834
|
156835
|
156836
|
156837
|
156838
|
156839
| 156840