From ed75df9ebc01c3f55d457e1f3e37482b36b65ba3 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 a range parameter (in days) to membership_expiry cronjob Content-Type: text/plain; charset=utf-8 The range parameter allows you to run the cronjob in an adjusted frequency (say once a week with range 3). You can also run it again and pass range 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 with range 7, the patron may receive two mails. While adding the range parameter, I decided to add a branch parameter too. The parameters are passed via a hashref. The cronjob also supports both new parameters: branch and range. 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 range 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 it and optionally override it?) 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 one or more patrons to now+16 (when the pref is still 14). [3] Run the cronjob without range. You should not see patrons from step 2. [4] Run the cronjob with range 2. You should see them now. [5] Run range 2 with a branchcode that does not exist. No patrons. --- C4/Members.pm | 41 +++++++++---- misc/cronjobs/membership_expiry.pl | 19 +++++- .../Members/GetUpcomingMembershipExpires.t | 62 ++++++++++++-------- 3 files changed, 83 insertions(+), 39 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 9e0c32e..1a9524b 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1489,25 +1489,46 @@ sub GetExpiryDate { =head2 GetUpcomingMembershipExpires - my $upcoming_mem_expires = GetUpcomingMembershipExpires(); + my $expires = + GetUpcomingMembershipExpires({ branch => $branch, range => $range }); + + $branch is an optional branch code. + $range is an optional number of days before and after the date that + is set by the preference MembershipExpiryDaysNotice. + If the pref is e.g. 14 and range is 3 days, you will get all expires + from 11 to 17 days. =cut sub GetUpcomingMembershipExpires { + my ( $params ) = @_; + my $range = $params->{range} || 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 - $range ); + my $date2 = dt_from_string->add( days => $days + $range ); + $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..f0322d4 100755 --- a/misc/cronjobs/membership_expiry.pl +++ b/misc/cronjobs/membership_expiry.pl @@ -61,6 +61,15 @@ 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<-range> + +Optional range parameter to extend the selection with a number of days around +the date set by the preference. + =back =head1 CONFIGURATION @@ -115,9 +124,11 @@ 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 $range = 0; +my $branch; GetOptions( 'help|?' => \$help, @@ -125,6 +136,8 @@ GetOptions( 'c' => \$confirm, 'n' => \$nomail, 'v' => \$verbose, + 'branch:s' => \$branch, + 'range:i' => \$range, ) or pod2usage(2); pod2usage( -verbose => 2 ) if $man; @@ -134,7 +147,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, range => $range }); warn 'found ' . scalar( @$upcoming_mem_expires ) . ' soon expiring members' if $verbose; @@ -156,7 +169,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..bd95357 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,16 @@ $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 +my $expires = scalar @{ GetUpcomingMembershipExpires() }; $builder->build({ source => 'Borrower', @@ -70,7 +70,7 @@ $builder->build({ surname => 'Martin', cardnumber => '80808081', categorycode => 'AD', - branchcode => 'CR', + branchcode => $branchcode, dateexpiry => '2015-06-30' }, }); @@ -82,7 +82,7 @@ $builder->build({ surname => 'Dupont', cardnumber => '80808082', categorycode => 'AD', - branchcode => 'CR', + branchcode => $branchcode, dateexpiry => '2015-06-29' }, }); @@ -94,24 +94,34 @@ $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.'); +# 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.' ); -is($upcoming_mem_expires->[0]{surname}, 'Martin', 'Get upcoming membership expires should return borrower "Martin".'); +# 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 GetUpcomingMembershipExpires() with MembershipExpiryDaysNotice == 0 -t::lib::Mocks::mock_preference('MembershipExpiryDaysNotice', 0); +# 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.' ); -$upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires(); -is(scalar(@$upcoming_mem_expires), 0, 'Get upcoming membership expires with 0 MembershipExpiryDaysNotice should return 0.'); +# 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 GetUpcomingMembershipExpires() with MembershipExpiryDaysNotice == undef -t::lib::Mocks::mock_preference('MembershipExpiryDaysNotice', undef); +# Test the range parameter +t::lib::Mocks::mock_preference( 'MembershipExpiryDaysNotice', 15 ); +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode, range => 1 }); +is( scalar(@$upcoming_mem_expires), 2, 'Expect two results for range==1'); +$upcoming_mem_expires = GetUpcomingMembershipExpires({ branch => $branchcode, range => 2 }); +is( scalar(@$upcoming_mem_expires), 3, 'Expect three results for range==2' ); -$upcoming_mem_expires = C4::Members::GetUpcomingMembershipExpires(); -is(scalar(@$upcoming_mem_expires), 0, 'Get upcoming membership expires without MembershipExpiryDaysNotice should return 0.'); +# End -- 1.7.10.4