Bug 20478: Have the cronjob script advance_notices.pl send digest messages per branch.
authorAndreas Jonsson <andreas.jonsson@kreablo.se>
Mon, 22 Jan 2018 10:37:14 +0000 (10:37 +0000)
committerNick Clemens <nick@bywatersolutions.com>
Fri, 22 Mar 2019 19:46:31 +0000 (19:46 +0000)
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>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt
misc/cronjobs/advance_notices.pl
t/db_dependent/cronjobs/advance_notices_digest.t

index 2d2c4e0..d6f9e39 100644 (file)
         </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>
index f139d77..58d123c 100755 (executable)
@@ -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;
         }
 
index 8eb40fb..54b3758 100644 (file)
@@ -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;