Bugzilla – Attachment 50912 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: Make membership_expiry cronjob more flexible
Bug-14834-Make-membershipexpiry-cronjob-more-flexi.patch (text/plain), 12.30 KB, created by
Kyle M Hall (khall)
on 2016-04-28 12:53:21 UTC
(
hide
)
Description:
Bug 14834: Make membership_expiry cronjob more flexible
Filename:
MIME Type:
Creator:
Kyle M Hall (khall)
Created:
2016-04-28 12:53:21 UTC
Size:
12.30 KB
patch
obsolete
>From dab09efc06c95fcdfdb3ce2158e8649d9ee7d612 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: Make membership_expiry cronjob more flexible > >This patch adds three parameters to the cron job: -before and -after, and >-branch. > >You can run the cronjob now in an adjusted frequency: say once a week with >before 6 or after 6 (not both together). If your pref is set to 14, running >before=6 will include expiries from 8 days to 14 days ahead. When you >use after=6, you would include 14 days to 20 days ahead, etc. > >You could also rerun the job of yesterday by setting before=1 and after=-1; >this could help in case of problem recovery. > >Obviously, the branch parameter can be used as a filter. > >NOTE: Why are these parameters passed only via the command line? >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. > >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. > >Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> >Work as described following test plan. >Test pass >No errors > >New parameters work with one (-) or two(--) dashes, no problem >with that but convention suggest that 'long' options use two-dashes. > >Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> >--- > C4/Members.pm | 43 +++++++++--- > misc/cronjobs/membership_expiry.pl | 26 ++++++- > .../Members/GetUpcomingMembershipExpires.t | 79 ++++++++++++---------- > 3 files changed, 101 insertions(+), 47 deletions(-) > >diff --git a/C4/Members.pm b/C4/Members.pm >index ed82f86..95c32f0 100644 >--- a/C4/Members.pm >+++ b/C4/Members.pm >@@ -1330,25 +1330,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 0cf52c8..46c723f 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; >@@ -142,7 +162,7 @@ if( !$expdays ) { > > 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; > >@@ -164,7 +184,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 1026b92..c9f8508 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 >@@ -19,17 +21,13 @@ > > use Modern::Perl; > >-use Test::More tests => 5; > use Test::MockModule; >-use t::lib::TestBuilder; >-use t::lib::Mocks qw( mock_preference ); >+use Test::More tests => 6; > >-use C4::Members; >+use C4::Members qw|GetUpcomingMembershipExpires|; > use Koha::Database; >- >-BEGIN { >- use_ok('C4::Members'); >-} >+use t::lib::TestBuilder; >+use t::lib::Mocks qw( mock_preference ); > > my $schema = Koha::Database->new->schema; > $schema->storage->txn_begin; >@@ -42,10 +40,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({ >@@ -59,13 +56,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', >@@ -74,7 +75,7 @@ $builder->build({ > surname => 'Martin', > cardnumber => '80808081', > categorycode => 'AD', >- branchcode => 'CR', >+ branchcode => $branchcode, > dateexpiry => '2015-06-30' > }, > }); >@@ -86,7 +87,7 @@ $builder->build({ > surname => 'Dupont', > cardnumber => '80808082', > categorycode => 'AD', >- branchcode => 'CR', >+ branchcode => $branchcode, > dateexpiry => '2015-06-29' > }, > }); >@@ -98,28 +99,38 @@ $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 > $schema->storage->txn_rollback; >- >-1; >-- >2.1.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