Bugzilla – Attachment 42646 Details for
Bug 14834
Make membership_expiry cronjob more flexible
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14834: Add a range parameter (in days) to membership_expiry cronjob
Bug-14834-Add-a-range-parameter-in-days-to-members.patch (text/plain), 11.60 KB, created by
Marcel de Rooy
on 2015-09-17 11:10:35 UTC
(
hide
)
Description:
Bug 14834: Add a range parameter (in days) to membership_expiry cronjob
Filename:
MIME Type:
Creator:
Marcel de Rooy
Created:
2015-09-17 11:10:35 UTC
Size:
11.60 KB
patch
obsolete
>From ed75df9ebc01c3f55d457e1f3e37482b36b65ba3 Mon Sep 17 00:00:00 2001 >From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> >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 <http://www.gnu.org/licenses>. > > 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
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 14834
:
42646
|
42650
|
46765
|
50458
|
50912