From 3024b560c44650e38475d3c6e83cd7eee07e5b0d Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 2 Jul 2024 12:59:01 +0000 Subject: [PATCH] Bug 29507: Tidy Signed-off-by: Martin Renvoize Signed-off-by: Katrin Fischer --- misc/cronjobs/automatic_renewals.pl | 343 ++++++++++++++-------------- 1 file changed, 175 insertions(+), 168 deletions(-) diff --git a/misc/cronjobs/automatic_renewals.pl b/misc/cronjobs/automatic_renewals.pl index bb0e053859..013fc12b60 100755 --- a/misc/cronjobs/automatic_renewals.pl +++ b/misc/cronjobs/automatic_renewals.pl @@ -87,14 +87,14 @@ use Koha::Checkouts; use Koha::Libraries; use Koha::Patrons; -my $command_line_options = join(" ",@ARGV); +my $command_line_options = join( " ", @ARGV ); my ( $help, $send_notices, $verbose, $confirm, $digest_per_branch ); GetOptions( - 'h|help' => \$help, - 's|send-notices' => \$send_notices, - 'v|verbose' => \$verbose, - 'c|confirm' => \$confirm, + 'h|help' => \$help, + 's|send-notices' => \$send_notices, + 'v|verbose' => \$verbose, + 'c|confirm' => \$confirm, 'b|digest-per-branch' => \$digest_per_branch, ) || pod2usage(1); @@ -110,6 +110,7 @@ to 'Never send emails' or 'Follow patron messaging preferences' END_WARN } else { + # If not following cron then: # - we should not send if set to never # - we will send any notice generated according to preferences if following those @@ -119,7 +120,7 @@ END_WARN # Since advance notice options are not visible in the web-interface # unless EnhancedMessagingPreferences is on, let the user know that # this script probably isn't going to do much -if ( ! C4::Context->preference('EnhancedMessagingPreferences') ) { +if ( !C4::Context->preference('EnhancedMessagingPreferences') ) { warn <<'END_WARN'; The "EnhancedMessagingPreferences" syspref is off. @@ -129,9 +130,9 @@ To change this, edit the "EnhancedMessagingPreferences" syspref. END_WARN } -cronlogaction({ info => $command_line_options }); +cronlogaction( { info => $command_line_options } ); -$verbose = 1 unless $verbose or $confirm; +$verbose = 1 unless $verbose or $confirm; print "Test run only\n" unless $confirm; print "getting auto renewals\n" if $verbose; @@ -140,14 +141,12 @@ my @auto_renews = Koha::Checkouts->search( auto_renew => 1, 'patron.autorenew_checkouts' => 1, }, - { - join => ['patron','item'] - } + { join => [ 'patron', 'item' ] } )->as_list; print "found " . scalar @auto_renews . " auto renewals\n" if $verbose; my $cron_options = C4::Context->config('auto_renew_cronjob'); -my $loops = $cron_options ? $cron_options->{parallel_loops_count} // 1 : 1; +my $loops = $cron_options ? $cron_options->{parallel_loops_count} // 1 : 1; # Split the list of issues into chunks to run in parallel my @chunks; @@ -164,20 +163,18 @@ if ( $loops > 1 ) { push( @{ $chunks[$i] }, $auto_renew ); } my $pm = Parallel::ForkManager->new($loops); - DATA_LOOP: +DATA_LOOP: foreach my $chunk (@chunks) { my $pid = $pm->start and next DATA_LOOP; _ProcessRenewals($chunk); $pm->finish; } $pm->wait_all_children; -} -else { +} else { _ProcessRenewals( \@auto_renews ); } -cronlogaction({ action => 'End', info => "COMPLETED" }); - +cronlogaction( { action => 'End', info => "COMPLETED" } ); =head1 METHODS @@ -190,157 +187,169 @@ cronlogaction({ action => 'End', info => "COMPLETED" }); sub _ProcessRenewals { my $auto_renew_issues = shift; -my $renew_digest = {}; -my %report; -my @item_renewal_ids; + my $renew_digest = {}; + my %report; + my @item_renewal_ids; foreach my $auto_renew (@$auto_renew_issues) { - print "examining item '" . $auto_renew->itemnumber . "' to auto renew\n" if $verbose; + print "examining item '" . $auto_renew->itemnumber . "' to auto renew\n" if $verbose; - my ( $borrower_preferences, $wants_messages, $wants_digest ) = ( undef, 0, 0 ); - if ( $send_notices_pref eq 'preferences' ) { - $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( - { - borrowernumber => $auto_renew->borrowernumber, - message_name => 'auto_renewals' - } - ); - $wants_messages = 1 - if $borrower_preferences - && $borrower_preferences->{transports}; - $wants_digest = 1 - if $wants_messages - && $borrower_preferences->{wants_digest}; - } else { # Preference is never or cron - $wants_messages = $send_notices; - $wants_digest = 0; - } - - my ( $success, $error, $updated ) = $auto_renew->attempt_auto_renew( { confirm => $confirm } ); - if ($success) { - 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'; - } - if ($confirm) { - push @item_renewal_ids, $auto_renew->itemnumber; - } - push @{ $report{ $auto_renew->borrowernumber } }, $auto_renew - if ($wants_messages) && !$wants_digest; - } elsif ( - - # FIXME Do we need to list every status? Why not simply else? - $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 '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; + my ( $borrower_preferences, $wants_messages, $wants_digest ) = ( undef, 0, 0 ); + if ( $send_notices_pref eq 'preferences' ) { + $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( + { + borrowernumber => $auto_renew->borrowernumber, + message_name => 'auto_renewals' + } + ); + $wants_messages = 1 + if $borrower_preferences + && $borrower_preferences->{transports}; + $wants_digest = 1 + if $wants_messages + && $borrower_preferences->{wants_digest}; + } else { # Preference is never or cron + $wants_messages = $send_notices; + $wants_digest = 0; } - if ($updated) { + + my ( $success, $error, $updated ) = $auto_renew->attempt_auto_renew( { confirm => $confirm } ); + if ($success) { + 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'; + } + if ($confirm) { + push @item_renewal_ids, $auto_renew->itemnumber; + } 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 ($wants_messages) && !$wants_digest; + } elsif ( + + # FIXME Do we need to list every status? Why not simply else? + $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 '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; + } + if ($updated) { + 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 ( $wants_digest ) { - # cache this one to process after we've run through all of the items. - if ($digest_per_branch) { - $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{success}++ if $success; - $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{error}++ unless $success || $error eq 'auto_too_soon'; - $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{results}->{defined $error ? $error : 'auto-renew'}++ ; - push @{$renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{issues}}, $auto_renew->itemnumber; - $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{updated} = 1 if $updated && (!$error || $error ne 'auto_too_soon'); - } else { - $renew_digest->{ $auto_renew->borrowernumber }->{success} ++ if $success; - $renew_digest->{ $auto_renew->borrowernumber }->{error}++ unless $success || $error eq 'auto_too_soon'; - $renew_digest->{ $auto_renew->borrowernumber }->{results}->{defined $error ? $error : 'auto-renew'}++ ; - $renew_digest->{ $auto_renew->borrowernumber }->{updated} = 1 if $updated && (!$error || $error ne 'auto_too_soon'); - push @{$renew_digest->{ $auto_renew->borrowernumber }->{issues}}, $auto_renew->itemnumber; + if ($wants_digest) { + + # cache this one to process after we've run through all of the items. + if ($digest_per_branch) { + $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{success}++ if $success; + $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{error}++ + unless $success || $error eq 'auto_too_soon'; + $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{results} + ->{ defined $error ? $error : 'auto-renew' }++; + push @{ $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{issues} }, + $auto_renew->itemnumber; + $renew_digest->{ $auto_renew->branchcode }->{ $auto_renew->borrowernumber }->{updated} = 1 + if $updated && ( !$error || $error ne 'auto_too_soon' ); + } else { + $renew_digest->{ $auto_renew->borrowernumber }->{success}++ if $success; + $renew_digest->{ $auto_renew->borrowernumber }->{error}++ unless $success || $error eq 'auto_too_soon'; + $renew_digest->{ $auto_renew->borrowernumber }->{results}->{ defined $error ? $error : 'auto-renew' }++; + $renew_digest->{ $auto_renew->borrowernumber }->{updated} = 1 + if $updated && ( !$error || $error ne 'auto_too_soon' ); + push @{ $renew_digest->{ $auto_renew->borrowernumber }->{issues} }, $auto_renew->itemnumber; + } } - } -} + } -if( @item_renewal_ids ){ - my $indexer = Koha::SearchEngine::Indexer->new({ index => $Koha::SearchEngine::BIBLIOS_INDEX }); - $indexer->index_records( \@item_renewal_ids, "specialUpdate", "biblioserver" ); -} + if (@item_renewal_ids) { + my $indexer = Koha::SearchEngine::Indexer->new( { index => $Koha::SearchEngine::BIBLIOS_INDEX } ); + $indexer->index_records( \@item_renewal_ids, "specialUpdate", "biblioserver" ); + } -if ( $send_notices && $confirm ) { - for my $borrowernumber ( keys %report ) { - my $patron = Koha::Patrons->find($borrowernumber); - my $borrower_preferences = - C4::Members::Messaging::GetMessagingPreferences( + if ( $send_notices && $confirm ) { + for my $borrowernumber ( keys %report ) { + my $patron = Koha::Patrons->find($borrowernumber); + my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { borrowernumber => $borrowernumber, message_name => 'auto_renewals' } ); - for my $issue ( @{ $report{$borrowernumber} } ) { - my $item = $issue->item; - # Force sending of email and only email if pref is set to "cron" - my @transports = $send_notices_pref eq 'preferences' ? keys %{ $borrower_preferences->{'transports'} } : 'email'; - foreach my $transport ( @transports ) { - my $letter = C4::Letters::GetPreparedLetter ( - module => 'circulation', - letter_code => 'AUTO_RENEWALS', - branchcode => $patron->branchcode, - tables => { - borrowers => $patron->borrowernumber, - issues => $issue->itemnumber, - items => $issue->itemnumber, - biblio => $item->biblionumber, - branches => $issue->branchcode, - }, - lang => $patron->lang, - message_transport_type => $transport, - ); - - if ($letter) { - my $library = $patron->library; - my $admin_email_address = $library->from_email_address; - - C4::Letters::EnqueueLetter( - { - letter => $letter, - borrowernumber => $borrowernumber, - from_address => $admin_email_address, - message_transport_type => $transport - } + for my $issue ( @{ $report{$borrowernumber} } ) { + my $item = $issue->item; + + # Force sending of email and only email if pref is set to "cron" + my @transports = + $send_notices_pref eq 'preferences' ? keys %{ $borrower_preferences->{'transports'} } : 'email'; + foreach my $transport (@transports) { + my $letter = C4::Letters::GetPreparedLetter( + module => 'circulation', + letter_code => 'AUTO_RENEWALS', + branchcode => $patron->branchcode, + tables => { + borrowers => $patron->borrowernumber, + issues => $issue->itemnumber, + items => $issue->itemnumber, + biblio => $item->biblionumber, + branches => $issue->branchcode, + }, + lang => $patron->lang, + message_transport_type => $transport, ); + + if ($letter) { + my $library = $patron->library; + my $admin_email_address = $library->from_email_address; + + C4::Letters::EnqueueLetter( + { + letter => $letter, + borrowernumber => $borrowernumber, + from_address => $admin_email_address, + message_transport_type => $transport + } + ); + } } } } - } - if ($digest_per_branch) { - while (my ($branchcode, $digests) = each %$renew_digest) { - send_digests({ - digests => $digests, - branchcode => $branchcode, - letter_code => 'AUTO_RENEWALS_DGST', - }); + if ($digest_per_branch) { + while ( my ( $branchcode, $digests ) = each %$renew_digest ) { + send_digests( + { + digests => $digests, + branchcode => $branchcode, + letter_code => 'AUTO_RENEWALS_DGST', + } + ); + } + } else { + send_digests( + { + digests => $renew_digest, + letter_code => 'AUTO_RENEWALS_DGST', + } + ); } - } else { - send_digests({ - digests => $renew_digest, - letter_code => 'AUTO_RENEWALS_DGST', - }); } -} } @@ -372,40 +381,39 @@ String that denote the letter code. sub send_digests { my $params = shift; - PATRON: while ( my ( $borrowernumber, $digest ) = each %{$params->{digests}} ) { +PATRON: while ( my ( $borrowernumber, $digest ) = each %{ $params->{digests} } ) { next unless defined $digest->{updated} && $digest->{updated} == 1; - my $borrower_preferences = - C4::Members::Messaging::GetMessagingPreferences( - { - borrowernumber => $borrowernumber, - message_name => 'auto_renewals' - } - ); + my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( + { + borrowernumber => $borrowernumber, + message_name => 'auto_renewals' + } + ); - next PATRON unless $borrower_preferences; # how could this happen? + next PATRON unless $borrower_preferences; # how could this happen? - my $patron = Koha::Patrons->find( $borrowernumber ); + my $patron = Koha::Patrons->find($borrowernumber); my $branchcode; if ( defined $params->{branchcode} ) { $branchcode = $params->{branchcode}; } else { $branchcode = $patron->branchcode; } - my $library = Koha::Libraries->find( $branchcode ); + my $library = Koha::Libraries->find($branchcode); my $from_address = $library->from_email_address; foreach my $transport ( keys %{ $borrower_preferences->{'transports'} } ) { - my $letter = C4::Letters::GetPreparedLetter ( - module => 'circulation', + my $letter = C4::Letters::GetPreparedLetter( + module => 'circulation', letter_code => $params->{letter_code}, - branchcode => $branchcode, - lang => $patron->lang, - substitute => { - error => $digest->{error}||0, - success => $digest->{success}||0, + branchcode => $branchcode, + lang => $patron->lang, + substitute => { + error => $digest->{error} || 0, + success => $digest->{success} || 0, results => $digest->{results}, }, - loops => { issues => \@{$digest->{issues}} }, - tables => { + loops => { issues => \@{ $digest->{issues} } }, + tables => { borrowers => $patron->borrowernumber, branches => $branchcode, }, @@ -421,10 +429,9 @@ sub send_digests { message_transport_type => $transport } ); - } - else { + } else { warn -"no letter of type '$params->{letter_code}' found for borrowernumber $borrowernumber. Please see sample_notices.sql"; + "no letter of type '$params->{letter_code}' found for borrowernumber $borrowernumber. Please see sample_notices.sql"; } } -- 2.39.5