From eadf3a60bf287c25993847afc805805158635182 Mon Sep 17 00:00:00 2001 From: Andreas Jonsson Date: Mon, 22 Jan 2018 10:37:14 +0000 Subject: [PATCH] Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Desired behavior of the script advance_notices.pl is that the sender address on the notice message is that of the branch of the issues in question. Thus, the solution is to generate digest messages per branch. To test: 1) Inspect unit test in t/db_dependent/cronjobs/advance_notices_digest.t and note that: - There are three libraries - There is a borrower - The borrower is registered at library1 - The borrower has message preference wants_digest set to 1 - The borrower has message preference days_in_advance set to 1 - The content of the letter PREDUEDGST is '<> <>' - There are three items - There is one issue per item - There is one issues at library2 - There are two issues at library3 - The date_due of the issues are set to tomorrow - For the default case (no -digest-per-message) - It is asserted that there is one message in the message queue after running the script - It is asserted that there are three items in the message. - It is asserted that the branchname is that of the borrower's home library. - For the case where -digest-per-message is enabled - It is asserted that there are two messages in the message queue after running the script - It is asserted that the item count of the message corresponding to library2 is 1 - It is asserted that the item count of the message corresponding to library3 is 2 - It is asserted that the branchnames are correct. 2) Run unit test: prove t/db_dependent/cronjobs/advance_notices_digest.t Sponsored-By: Bibliotek Mellansjö, which is a cooperation between Sponsored-By: Gullspångs kommunbibliotek Sponsored-By: Hjo stadsbibliotek Sponsored-By: Karlsborgs bibliotek Sponsored-By: Mariestads stadsbibliotek Sponsored-By: Skövde stadsbibliotek Sponsored-By: Tibro bibliotek Sponsored-By: Tidaholms stadsbibliotek Sponsored-By: Töreboda kommunbibliotek Signed-off-by: Andreas Jonsson Signed-off-by: Magnus Enger Adding the --digest-per-branch switch turns the digest into one digest per library. I think it makes perfect sense to keep the default behaviour and hide this new functionality behind a command line switch. --- .../intranet-tmpl/prog/en/modules/tools/letter.tt | 2 - misc/cronjobs/advance_notices.pl | 146 +++++++--- t/db_dependent/cronjobs/advance_notices_digest.t | 310 +++++++++++++-------- 3 files changed, 302 insertions(+), 156 deletions(-) diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt index 36df8f6..60d8190 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt @@ -394,8 +394,6 @@ [% END %] - [% IF code.search('DGST') %] Warning, this is a template for a Digest, as such, any references to branch data ( e.g. branches.branchname ) will refer to the borrower's home branch. [% END %] - diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl index f139d77..58d123c 100755 --- a/misc/cronjobs/advance_notices.pl +++ b/misc/cronjobs/advance_notices.pl @@ -109,6 +109,21 @@ defaults to date_due,title,author,barcode Other possible values come from fields in the biblios, items and issues tables. +=item B<--digest-per-branch> + +Flag to indicate that generation of message digests should be +performed separately for each branch. + +A patron could potentially have loans at several different branches +There is no natural branch to set as the sender on the aggregated +message in this situation so the default behavior is to use the +borrowers home branch. This could surprise to the borrower when +message sender is a library where he or she has not borrowed anything. + +Enabling this flag ensures that the issuing library is the sender of +the digested message. It has no effect unless the borrower has +choosen 'Digests only' on the advance messages. + =back =head1 DESCRIPTION @@ -174,6 +189,7 @@ my $nomail; # -n: No mai my $mindays = 0; # -m: Maximum number of days in advance to send notices my $maxdays = 30; # -e: the End of the time period my $verbose = 0; # -v: verbose +my $digest_per_branch = 0; # -digest-per-branch: Prepare and send digests per branch my $itemscontent = join(',',qw( date_due title author barcode )); my $help = 0; @@ -186,6 +202,7 @@ GetOptions( 'n' => \$nomail, 'm:i' => \$maxdays, 'v' => \$verbose, + 'digest-per-branch' => \$digest_per_branch, 'itemscontent=s' => \$itemscontent, )or pod2usage(2); pod2usage(1) if $help; @@ -218,8 +235,8 @@ warn 'found ' . scalar( @$upcoming_dues ) . ' issues' if $verbose; # hash of borrowernumber to number of items upcoming # for patrons wishing digests only. -my $upcoming_digest; -my $due_digest; +my $upcoming_digest = {}; +my $due_digest = {}; my $dbh = C4::Context->dbh(); my $sth = $dbh->prepare(<<'END_SQL'); @@ -250,8 +267,13 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { if ( $borrower_preferences->{'wants_digest'} ) { # cache this one to process after we've run through all of the items. - $due_digest->{ $upcoming->{borrowernumber} }->{email} = $from_address; - $due_digest->{ $upcoming->{borrowernumber} }->{count}++; + if ($digest_per_branch) { + $due_digest->{ $upcoming->{branchcode} }->{ $upcoming->{borrowernumber} }->{email} = $from_address; + $due_digest->{ $upcoming->{branchcode} }->{ $upcoming->{borrowernumber} }->{count}++; + } else { + $due_digest->{ $upcoming->{borrowernumber} }->{email} = $from_address; + $due_digest->{ $upcoming->{borrowernumber} }->{count}++; + } } else { my $item = Koha::Items->find( $upcoming->{itemnumber} ); my $letter_type = 'DUE'; @@ -283,8 +305,13 @@ UPCOMINGITEM: foreach my $upcoming ( @$upcoming_dues ) { if ( $borrower_preferences->{'wants_digest'} ) { # cache this one to process after we've run through all of the items. - $upcoming_digest->{ $upcoming->{borrowernumber} }->{email} = $from_address; - $upcoming_digest->{ $upcoming->{borrowernumber} }->{count}++; + if ($digest_per_branch) { + $upcoming_digest->{ $upcoming->{branchcode} }->{ $upcoming->{borrowernumber} }->{email} = $from_address; + $upcoming_digest->{ $upcoming->{branchcode} }->{ $upcoming->{borrowernumber} }->{count}++; + } else { + $upcoming_digest->{ $upcoming->{borrowernumber} }->{email} = $from_address; + $upcoming_digest->{ $upcoming->{borrowernumber} }->{count}++; + } } else { my $item = Koha::Items->find( $upcoming->{itemnumber} ); my $letter_type = 'PREDUE'; @@ -342,32 +369,67 @@ SELECT biblio.*, items.*, issues.* AND (TO_DAYS(date_due)-TO_DAYS(NOW()) = ?) END_SQL -send_digests({ - sth => $sth_digest, - digests => $upcoming_digest, - letter_code => 'PREDUEDGST', - get_item_info => sub { - my $params = shift; - $params->{sth}->execute($params->{borrowernumber}, - $params->{borrower_preferences}->{'days_in_advance'}); - return sub { - $params->{sth}->fetchrow_hashref; - }; +if ($digest_per_branch) { + while (my ($branchcode, $digests) = each %$upcoming_digest) { + send_digests({ + sth => $sth_digest, + digests => $digests, + letter_code => 'PREDUEDGST', + branchcode => $branchcode, + get_item_info => sub { + my $params = shift; + $params->{sth}->execute($params->{borrowernumber}, + $params->{borrower_preferences}->{'days_in_advance'}); + return sub { + $params->{sth}->fetchrow_hashref; + }; + } + }); } -}); - -send_digests({ - sth => $sth_digest, - digest => $due_digest, - letter_code => 'DUEGST', - get_item_info => sub { - my $params = shift; - $params->{sth}->execute($params->{borrowernumber}, 0); - return sub { - $params->{sth}->fetchrow_hashref; - }; + + while (my ($branchcode, $digests) = each %$due_digest) { + send_digests({ + sth => $sth_digest, + digest => $due_digest, + letter_code => 'DUEGST', + branchcode => $branchcode, + get_item_info => sub { + my $params = shift; + $params->{sth}->execute($params->{borrowernumber}, 0); + return sub { + $params->{sth}->fetchrow_hashref; + }; + } + }); } -}); +} else { + send_digests({ + sth => $sth_digest, + digests => $upcoming_digest, + letter_code => 'PREDUEDGST', + get_item_info => sub { + my $params = shift; + $params->{sth}->execute($params->{borrowernumber}, + $params->{borrower_preferences}->{'days_in_advance'}); + return sub { + $params->{sth}->fetchrow_hashref; + }; + } + }); + + send_digests({ + sth => $sth_digest, + digest => $due_digest, + letter_code => 'DUEGST', + get_item_info => sub { + my $params = shift; + $params->{sth}->execute($params->{borrowernumber}, 0); + return sub { + $params->{sth}->fetchrow_hashref; + }; + } + }); +} =head1 METHODS @@ -471,6 +533,18 @@ sub send_digests { my $count = $digest->{count}; my $from_address = $digest->{email}; + my %branch_info; + my $branchcode; + + if (defined($params->{branchcode})) { + %branch_info = (); + $branchcode = $params->{branchcode}; + } else { + ## Get branch info for borrowers home library. + %branch_info = get_branch_info( $borrowernumber ); + $branchcode = $branch_info{'branches.branchcode'}; + } + my $borrower_preferences = C4::Members::Messaging::GetMessagingPreferences( { @@ -491,9 +565,6 @@ sub send_digests { $titles .= C4::Letters::get_item_content( { item => $item_info, item_content_fields => \@item_content_fields } ); } - ## Get branch info for borrowers home library. - my %branch_info = get_branch_info( $borrowernumber ); - foreach my $transport ( keys %{ $borrower_preferences->{'transports'} } ) { my $letter = parse_letter( { @@ -502,13 +573,12 @@ sub send_digests { substitute => { count => $count, 'items.content' => $titles, - %branch_info, + %branch_info }, - branchcode => $branch_info{"branches.branchcode"}, - message_transport_type => $transport, + branchcode => $branchcode, + message_transport_type => $transport } - ) - or warn "no letter of type '$params->{letter_type}' found for borrowernumber $borrowernumber. Please see sample_notices.sql"; + ) || warn "no letter of type '$params->{letter_type}' found for borrowernumber $borrowernumber. Please see sample_notices.sql"; push @letters, $letter if $letter; } diff --git a/t/db_dependent/cronjobs/advance_notices_digest.t b/t/db_dependent/cronjobs/advance_notices_digest.t index 8eb40fb..54b3758 100644 --- a/t/db_dependent/cronjobs/advance_notices_digest.t +++ b/t/db_dependent/cronjobs/advance_notices_digest.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 3; +use Test::More tests => 8; use t::lib::TestBuilder; use DateTime; use File::Spec; @@ -30,155 +30,233 @@ my $scriptDir = dirname(File::Spec->rel2abs( __FILE__ )); my $dbh = C4::Context->dbh; -# Set only to avoid exception. -$ENV{"OVERRIDE_SYSPREF_dateformat"} = 'metric'; - -$dbh->{AutoCommit} = 0; -$dbh->{RaiseError} = 1; - -my $builder = t::lib::TestBuilder->new; - -my $library1 = $builder->build({ - source => 'Branch', -}); -my $library2 = $builder->build({ - source => 'Branch', -}); -my $library3 = $builder->build({ - source => 'Branch', -}); -my $borrower = $builder->build({ - source => 'Borrower', - value => { - branchcode => $library1->{branchcode}, - } -}); -$dbh->do(<{AutoCommit} = 0; + $dbh->{RaiseError} = 1; + + # Set only to avoid exception. + $ENV{"OVERRIDE_SYSPREF_dateformat"} = 'metric'; + + my $builder = t::lib::TestBuilder->new; + + $library1 = $builder->build({ + source => 'Branch', + }); + $library2 = $builder->build({ + source => 'Branch', + }); + $library3 = $builder->build({ + source => 'Branch', + }); + $borrower = $builder->build({ + source => 'Borrower', + value => { + branchcode => $library1->{branchcode}, + } + }); + $dbh->do(<do(<do(<build({ - source => 'MessageAttribute', - value => { - message_name => 'Advance_Notice' - } -}); - -my $letter = $builder->build({ - source => 'Letter', - value => { - module => 'circulation', - code => 'PREDUEDGST', - branchcode => '', - message_transport_type => 'email', - lang => 'default', - is_html => 0, - content => '<> <>' - } -}); -my $borrower_message_preference = $builder->build({ - source => 'BorrowerMessagePreference', - value => { - borrowernumber => $borrower->{borrowernumber}, - wants_digest => 1, - days_in_advance => 1, - message_attribute_id => $message_attribute->{message_attribute_id} - } -}); + my $message_attribute = $builder->build({ + source => 'MessageAttribute', + value => { + message_name => 'Advance_Notice' + } + }); -my $borrower_message_transport_preference = $builder->build({ - source => 'BorrowerMessageTransportPreference', - value => { - borrower_message_preference_id => $borrower_message_preference->{borrower_message_preference_id}, - message_transport_type => 'email' - } -}); - -my $biblio = $builder->build({ - source => 'Biblio', -}); -my $biblioitem = $builder->build({ - source => 'Biblioitem', - value => { - biblionumber => $biblio->{biblionumber} - } -}); -my $item1 = $builder->build({ - source => 'Item' -}); -my $item2 = $builder->build({ - source => 'Item' -}); -my $now = DateTime->now(); -my $tomorrow = $now->add(days => 1)->strftime('%F'); - -my $issue1 = $builder->build({ - source => 'Issue', - value => { - date_due => $tomorrow, - itemnumber => $item1->{itemnumber}, - branchcode => $library1->{branchcode}, - borrowernumber => $borrower->{borrowernumber}, - returndate => undef - } -}); - -my $issue2 = $builder->build({ - source => 'Issue', - value => { - date_due => $tomorrow, - itemnumber => $item2->{itemnumber}, - branchcode => $library2->{branchcode}, - branchcode => $library3->{branchcode}, - borrowernumber => $borrower->{borrowernumber}, - returndate => undef - } -}); + my $letter = $builder->build({ + source => 'Letter', + value => { + module => 'circulation', + code => 'PREDUEDGST', + branchcode => '', + message_transport_type => 'email', + lang => 'default', + is_html => 0, + content => '<> <>' + } + }); + my $borrower_message_preference = $builder->build({ + source => 'BorrowerMessagePreference', + value => { + borrowernumber => $borrower->{borrowernumber}, + wants_digest => 1, + days_in_advance => 1, + message_attribute_id => $message_attribute->{message_attribute_id} + } + }); + + my $borrower_message_transport_preference = $builder->build({ + source => 'BorrowerMessageTransportPreference', + value => { + borrower_message_preference_id => $borrower_message_preference->{borrower_message_preference_id}, + message_transport_type => 'email' + } + }); -C4::Context->set_preference('EnhancedMessagingPreferences', 1); + my $biblio = $builder->build({ + source => 'Biblio', + }); + my $biblioitem = $builder->build({ + source => 'Biblioitem', + value => { + biblionumber => $biblio->{biblionumber} + } + }); + my $item1 = $builder->build({ + source => 'Item' + }); + my $item2 = $builder->build({ + source => 'Item' + }); + my $item3 = $builder->build({ + source => 'Item' + }); + my $now = DateTime->now(); + my $tomorrow = $now->add(days => 1)->strftime('%F'); -my $script = ''; + my $issue1 = $builder->build({ + source => 'Issue', + value => { + date_due => $tomorrow, + itemnumber => $item1->{itemnumber}, + branchcode => $library2->{branchcode}, + borrowernumber => $borrower->{borrowernumber}, + returndate => undef + } + }); + + my $issue2 = $builder->build({ + source => 'Issue', + value => { + date_due => $tomorrow, + itemnumber => $item2->{itemnumber}, + branchcode => $library3->{branchcode}, + borrowernumber => $borrower->{borrowernumber}, + returndate => undef + } + }); + my $issue3 = $builder->build({ + source => 'Issue', + value => { + date_due => $tomorrow, + itemnumber => $item3->{itemnumber}, + branchcode => $library3->{branchcode}, + borrowernumber => $borrower->{borrowernumber}, + returndate => undef + } + }); + + C4::Context->set_preference('EnhancedMessagingPreferences', 1); +} + +sub run_script { + my $script = shift; + local @ARGV = @_; + + ## no critic + + # We simulate script execution by evaluating the script code in the context + # of this unit test. + + eval $script; #Violates 'ProhibitStringyEval' + + ## use critic + + die $@ if $@; +} + +my $scriptContent = ''; my $scriptFile = "$scriptDir/../../../misc/cronjobs/advance_notices.pl"; open my $scriptfh, "<", $scriptFile or die "Failed to open $scriptFile: $!"; while (<$scriptfh>) { - $script .= $_; + $scriptContent .= $_; } close $scriptfh; -@ARGV = ('advanced_notices.pl', '-c'); - -## no critic - -# We simulate script execution by evaluating the script code in the context -# of this unit test. +my $sthmq = $dbh->prepare('SELECT * FROM message_queue WHERE borrowernumber = ?'); -eval $script; #Violates 'ProhibitStringyEval' +# +# Test default behavior +# -## use critic +build_test_objects(); -die $@ if $@; +run_script($scriptContent, 'advanced_notices.pl', '-c'); -my $sthmq = $dbh->prepare('SELECT * FROM message_queue WHERE borrowernumber = ?'); $sthmq->execute($borrower->{borrowernumber}); my $messages = $sthmq->fetchall_hashref('message_id'); is(scalar(keys %$messages), 1, 'There is one message in the queue'); + for my $message (keys %$messages) { $messages->{$message}->{content} =~ /(\d+) (.*)/; my $count = $1; my $branchname = $2; - is ($count, '2', 'Issue count is 2'); + is ($count, '3', 'Issue count is 3'); is ($branchname, $library1->{branchname}, 'Branchname is that of borrowers home branch.'); } $dbh->rollback; + +# +# Test -digest-per-branch +# + +build_test_objects(); + +run_script($scriptContent, 'advanced_notices.pl', '-c', '-digest-per-branch'); + +$sthmq->execute($borrower->{borrowernumber}); + +$messages = $sthmq->fetchall_hashref('message_id'); + +is(scalar(keys %$messages), 2, 'There are two messages in the queue'); + +my %expected = ( + $library2->{branchname} => { + count => 1, + }, + $library3->{branchname} => { + count => 2, + } + ); + +my %expected_branchnames = ( + $library2->{branchname} => 1, + $library3->{branchname} => 1 +); + +my $i = 0; +for my $message (keys %$messages) { + $messages->{$message}->{content} =~ /(\d+) (.*)/; + my $count = $1; + my $branchname = $2; + + ok ($expected_branchnames{$branchname}, 'Branchname is that of expected issuing branch.'); + + $expected_branchnames{$branchname} = 0; + + is ($count, $expected{$branchname}->{count}, 'Issue count is ' . $expected{$branchname}->{count}); + + $i++; +} + +$dbh->rollback; -- 2.1.4