Bugzilla – Attachment 86383 Details for
Bug 20478
Advance notices: send separate digest messages per library
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Bug-20478-Have-the-cronjob-script-advancenoticespl.patch (text/plain), 22.02 KB, created by
Martin Renvoize (ashimema)
on 2019-03-08 13:01:31 UTC
(
hide
)
Description:
Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2019-03-08 13:01:31 UTC
Size:
22.02 KB
patch
obsolete
>From be0ea79792f763ed6c3d3cb44db7d1b34b6b7bbc Mon Sep 17 00:00:00 2001 >From: Andreas Jonsson <andreas.jonsson@kreablo.se> >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 '<<count>> <<branches.branchname>>' > - 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 <andreas.jonsson@kreablo.se> >Signed-off-by: Magnus Enger <magnus@libriotech.no> >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. >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > .../prog/en/modules/tools/letter.tt | 2 - > misc/cronjobs/advance_notices.pl | 146 ++++++--- > .../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 28dd70d0e3..43da823339 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 @@ > </div> > [% END %] > >- [% IF code.search('DGST') %] <span class="overdue">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.</span> [% END %] >- > <input type="hidden" id="redirect" name="redirect" value="" /> > <input type="hidden" name="searchfield" value="[% searchfield | html %]" /> > </form> >diff --git a/misc/cronjobs/advance_notices.pl b/misc/cronjobs/advance_notices.pl >index f139d77fef..58d123caf1 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 8eb40fb91a..54b3758b23 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(<<DELETESQL); >+my $library1; >+my $library2; >+my $library3; >+my $borrower; >+ >+sub build_test_objects { >+ $dbh->{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(<<DELETESQL); > DELETE FROM letter > WHERE module='circulation' > AND code = 'PREDUEDGST' > AND message_transport_type='email' > AND branchcode='' > DELETESQL >-$dbh->do(<<DELETESQL); >+ >+ $dbh->do(<<DELETESQL); > DELETE FROM message_attributes WHERE message_name = 'Advance_Notice' > DELETESQL > >-my $message_attribute = $builder->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 => '<<count>> <<branches.branchname>>' >- } >-}); >-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 => '<<count>> <<branches.branchname>>' >+ } >+ }); >+ 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.20.1
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 20478
:
73308
|
73309
|
73310
|
73722
|
73723
|
73724
|
80867
|
80901
|
80902
|
80903
|
86381
|
86382
| 86383 |
86384