From 65e88edbd2361ab06f97bae25a793f8612286df7 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 17 Sep 2015 10:25:50 +0200 Subject: [PATCH] Bug 14834: Add range parameters to membership_expiry cronjob Content-Type: text/plain; charset=utf-8 The before/after parameter allows you to run the cronjob in an adjusted frequency (say once a week with before 7 or after 7). You can also run it again and pass before==1 to include the expires that should have been mailed yesterday, etc. Or use it to remind the patron more than once: if you choose for pref 14 and run weekly with before 7, the patron may receive two mails. While adding those two parameters, I decided to add a branch parameter too. The parameters are passed via a hashref. I decided to pass them on the command line and not via a preference. Why? Well, obviously the branch parameter is not suitable for a pref. The before/after parameter allows you to handle expiry mails different from the normal scheme or could be used in some sort of recovery. In those cases it will be more practical to use a command line parameter than editing a pref. (If needed, we could still add them and optionally override..) NOTE: The unit test has been adjusted for the above reasons, but I also added some lines to let existing expires not interfere with the added borrowers by an additional count and using the branchcode parameter. Test plan: [1] Run the adjusted unit test GetUpcomingMembershipExpires.t [2] Set the expiry date for patron A to now+16 (with pref 14). Set the expiry date for patron B to now+11. [3] Run the cronjob without range. You should not see A and B. [4] Run the cronjob with before 3. You should see patron B. [5] Run the cronjob with before 3 and after 2. You should see A and B. [6] Repeat step 5 with a branchcode that does not exist. No patrons. --- C4/Members.pm | 43 ++++++++--- misc/cronjobs/membership_expiry.pl | 26 ++++++- .../Members/GetUpcomingMembershipExpires.t | 76 ++++++++++++-------- 3 files changed, 101 insertions(+), 44 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 9e0c32e..52ad21c 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1489,25 +1489,48 @@ sub GetExpiryDate { =head2 GetUpcomingMembershipExpires - my $upcoming_mem_expires = GetUpcomingMembershipExpires(); + my $expires = GetUpcomingMembershipExpires({ + branch => $branch, before => $before, after => $after, + }); + + $branch is an optional branch code. + $before/$after is an optional number of days before/after the date that + is set by the preference MembershipExpiryDaysNotice. + If the pref would be 14, before 2 and after 3, you will get all expires + from 12 to 17 days. =cut sub GetUpcomingMembershipExpires { + my ( $params ) = @_; + my $before = $params->{before} || 0; + my $after = $params->{after} || 0; + my $branch = $params->{branch}; + my $dbh = C4::Context->dbh; my $days = C4::Context->preference("MembershipExpiryDaysNotice") || 0; - my $dateexpiry = output_pref({ dt => (dt_from_string()->add( days => $days)), dateformat => 'iso', dateonly => 1 }); + my $date1 = dt_from_string->add( days => $days - $before ); + my $date2 = dt_from_string->add( days => $days + $after ); + $date1= output_pref({ dt => $date1, dateformat => 'iso', dateonly => 1 }); + $date2= output_pref({ dt => $date2, dateformat => 'iso', dateonly => 1 }); - my $query = " + my $query = q| SELECT borrowers.*, categories.description, branches.branchname, branches.branchemail FROM borrowers - LEFT JOIN branches on borrowers.branchcode = branches.branchcode - LEFT JOIN categories on borrowers.categorycode = categories.categorycode - WHERE dateexpiry = ?; - "; - my $sth = $dbh->prepare($query); - $sth->execute($dateexpiry); - my $results = $sth->fetchall_arrayref({}); + LEFT JOIN branches USING (branchcode) + LEFT JOIN categories USING (categorycode) + |; + if( $branch ) { + $query.= 'WHERE branchcode=? AND dateexpiry BETWEEN ? AND ?'; + } else { + $query.= 'WHERE dateexpiry BETWEEN ? AND ?'; + } + + my $sth = $dbh->prepare( $query ); + my @pars = $branch? ( $branch ): (); + push @pars, $date1, $date2; + $sth->execute( @pars ); + my $results = $sth->fetchall_arrayref( {} ); return $results; } diff --git a/misc/cronjobs/membership_expiry.pl b/misc/cronjobs/membership_expiry.pl index 4a0f98b..342d35b 100755 --- a/misc/cronjobs/membership_expiry.pl +++ b/misc/cronjobs/membership_expiry.pl @@ -61,6 +61,20 @@ the patrons are printed to standard out. Confirm flag: Add this option. The script will only print a usage statement otherwise. +=item B<-branch> + +Optional branchcode to restrict the cronjob to that branch. + +=item B<-before> + +Optional parameter to extend the selection with a number of days BEFORE +the date set by the preference. + +=item B<-after> + +Optional parameter to extend the selection with a number of days AFTER +the date set by the preference. + =back =head1 CONFIGURATION @@ -115,9 +129,12 @@ use C4::Log; # These are defaults for command line options. my $confirm; # -c: Confirm that the user has read and configured this script. my $nomail; # -n: No mail. Will not send any emails. -my $verbose= 0; # -v: verbose +my $verbose = 0; # -v: verbose my $help = 0; my $man = 0; +my $before = 0; +my $after = 0; +my $branch; GetOptions( 'help|?' => \$help, @@ -125,6 +142,9 @@ GetOptions( 'c' => \$confirm, 'n' => \$nomail, 'v' => \$verbose, + 'branch:s' => \$branch, + 'before:i' => \$before, + 'after:i' => \$after, ) or pod2usage(2); pod2usage( -verbose => 2 ) if $man; @@ -134,7 +154,7 @@ cronlogaction(); my $admin_adress = C4::Context->preference('KohaAdminEmailAddress'); warn 'getting upcoming membership expires' if $verbose; -my $upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires(); +my $upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires({ branch => $branch, before => $before, after => $after }); warn 'found ' . scalar( @$upcoming_mem_expires ) . ' soon expiring members' if $verbose; @@ -156,7 +176,7 @@ foreach my $recent ( @$upcoming_mem_expires ) { }); if ($letter) { if ($nomail) { - print $letter->{'content'}; + print $letter->{'content'}."\n"; } else { C4::Letters::EnqueueLetter({ letter => $letter, diff --git a/t/db_dependent/Members/GetUpcomingMembershipExpires.t b/t/db_dependent/Members/GetUpcomingMembershipExpires.t index 241858f..f8e5e8d 100644 --- a/t/db_dependent/Members/GetUpcomingMembershipExpires.t +++ b/t/db_dependent/Members/GetUpcomingMembershipExpires.t @@ -1,5 +1,7 @@ #!/usr/bin/perl +# Tests for C4::Members::GetUpcomingMembershipExpires + # This file is part of Koha. # # Copyright 2015 Biblibre @@ -18,17 +20,13 @@ # along with Koha; if not, see . use Modern::Perl; -use C4::Members; -use Test::MockModule; -use t::lib::TestBuilder; -use t::lib::Mocks qw( mock_preference ); -use Test::More tests => 5; use Test::MockModule; +use Test::More tests => 6; -BEGIN { - use_ok('C4::Members'); -} +use C4::Members qw|GetUpcomingMembershipExpires|; +use t::lib::TestBuilder; +use t::lib::Mocks qw( mock_preference ); my $date_time = new Test::MockModule('DateTime'); $date_time->mock( @@ -38,10 +36,9 @@ $date_time->mock( month => 6, day => 15, ); - }); -t::lib::Mocks::mock_preference('MembershipExpiryDaysNotice', 15); +t::lib::Mocks::mock_preference( 'MembershipExpiryDaysNotice', 15 ); my $builder = t::lib::TestBuilder->new(); $builder->build({ @@ -55,13 +52,17 @@ $builder->build({ }, }); -$builder->build({ +my $branch = $builder->build({ source => 'Branch', value => { - branchcode => 'CR', branchname => 'My branch', }, }); +my $branchcode = $branch->{branchcode}; +# before we add borrowers to this branch, add the expires we have now +# note that this pertains to the current mocked setting of the pref +# for this reason we add the new branchcode to most of the tests +my $expires = scalar @{ GetUpcomingMembershipExpires() }; $builder->build({ source => 'Borrower', @@ -70,7 +71,7 @@ $builder->build({ surname => 'Martin', cardnumber => '80808081', categorycode => 'AD', - branchcode => 'CR', + branchcode => $branchcode, dateexpiry => '2015-06-30' }, }); @@ -82,7 +83,7 @@ $builder->build({ surname => 'Dupont', cardnumber => '80808082', categorycode => 'AD', - branchcode => 'CR', + branchcode => $branchcode, dateexpiry => '2015-06-29' }, }); @@ -94,24 +95,37 @@ $builder->build({ surname => 'Dupond', cardnumber => '80808083', categorycode => 'AD', - branchcode => 'CR', + branchcode => $branchcode, dateexpiry => '2015-07-02' }, }); -my $upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires(); -is(scalar(@$upcoming_mem_expires), 1, 'Get upcoming membership expires should return 1 borrower.'); - -is($upcoming_mem_expires->[0]{surname}, 'Martin', 'Get upcoming membership expires should return borrower "Martin".'); - -# Test GetUpcomingMembershipExpires() with MembershipExpiryDaysNotice == 0 -t::lib::Mocks::mock_preference('MembershipExpiryDaysNotice', 0); - -$upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires(); -is(scalar(@$upcoming_mem_expires), 0, 'Get upcoming membership expires with 0 MembershipExpiryDaysNotice should return 0.'); - -# Test GetUpcomingMembershipExpires() with MembershipExpiryDaysNotice == undef -t::lib::Mocks::mock_preference('MembershipExpiryDaysNotice', undef); - -$upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires(); -is(scalar(@$upcoming_mem_expires), 0, 'Get upcoming membership expires without MembershipExpiryDaysNotice should return 0.'); +# Test without extra parameters +my $upcoming_mem_expires = GetUpcomingMembershipExpires(); +is( scalar(@$upcoming_mem_expires), $expires + 1, 'Get upcoming membership expires should return one new borrower.' ); + +# Test with branch +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode }); +is( @$upcoming_mem_expires==1 && $upcoming_mem_expires->[0]{surname} eq 'Martin',1 , 'Get upcoming membership expires should return borrower "Martin".' ); + +# Test MembershipExpiryDaysNotice == 0 +t::lib::Mocks::mock_preference( 'MembershipExpiryDaysNotice', 0 ); +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode }); +is( scalar(@$upcoming_mem_expires), 0, 'Get upcoming membership expires with MembershipExpiryDaysNotice==0 should not return new records.' ); + +# Test MembershipExpiryDaysNotice == undef +t::lib::Mocks::mock_preference( 'MembershipExpiryDaysNotice', undef ); +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode }); +is( scalar(@$upcoming_mem_expires), 0, 'Get upcoming membership expires without MembershipExpiryDaysNotice should not return new records.' ); + +# Test the before parameter +t::lib::Mocks::mock_preference( 'MembershipExpiryDaysNotice', 15 ); +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode, before => 1 }); +# Expect 29/6 and 30/6 +is( scalar(@$upcoming_mem_expires), 2, 'Expect two results for before==1'); +# Test after parameter also +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode, before => 1, after => 2 }); +# Expect 29/6, 30/6 and 2/7 +is( scalar(@$upcoming_mem_expires), 3, 'Expect three results when adding after' ); + +# End -- 1.7.10.4