A new crontab based perl script to send membership expiry reminders. A system preference controls the number of days in advance of membership expiry that the notices will be sent on.
Created attachment 5375 [details] [review] Membership expiry notice
Updating Version : This ENH will be for Koha 3.8
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Hi Amit and Savitra, the way we handle system preferences has changed in 3.6. Only one change to a central systempreferences file is needed now. Can you please rework your patch and resubmit?
Created attachment 9751 [details] [review] Bug_6810 Send membership expiry reminder notices. A new crontab based perl script to send membership expiry reminders. A system preference controls the number of days in advance of membership expiry that the notices will be sent on. To Test: 1) Create a new Patron and set membership expiry date 14 days from the date of registration. 2) Check your systemprefence ( MemExpDayNotice to 14 days) 3) Manual testing Run ( perl membership_expiry.pl -h) It would give you various option: This script prepares for membership expiry reminders to be sent to patrons. It queues them in the message queue, which is processed by the process_message_queue.pl cronjob. See the comments in the script for directions on changing the script. This script has the following parameters : -c Confirm and remove this help & warning -n send No mail. Instead, all mail messages are printed on screen. Usefull for testing purposes. -v verbose Do you wish to continue? (y/n) 4) Choose option for ex: perl membership_expiry.pl -c 5) Go to your koha database and check message_queue table you see some results. 6) Run (perl process_message_queue.pl) it will send email to those patron whose membership after 14 days from today. 7) Cron testing: (10 1 * * * $KOHA_CRON_PATH/membership_expiry.pl -c) 8) Set your 15 * * * * $KOHA_CRON_PATH/process_message_queue.pl 9) After running membership_expiry.pl, (process_message_queue.pl will send emails to those patron whose membership after 14 days from today).
Can't be tested in a sandbox as it needs command line access.
It works as described. Few remarks: - A detailed description of the script operation and goals would help the documentation manager to do her job. - You should say that MEMEXP notification is used to prepare the email. - You should say that MemExpDaysNotice syspref specify the number of days before borrower registration expiration date the notification is sent. This is said in the syspref, but I'm not sure it's proper English. No being myself a native English speaker, I can't say if 'Send a membership expiry notice that a patron is about to expire after n days' is really the proper wording. - You should say that the script MUST be run once a day since the script send email for borrower whom registration expires exactly in x days. - I can imagine that libraries will want to send this notification once a week, or Monday and Thursday. I tend to think that a better approach is to work with interval of days, given to the script as arguments, so without syspref.
Created attachment 21870 [details] [review] Send membership expiry reminder notices. A new crontab based perl script to send membership expiry reminders. A system preference controls the number of days in advance of membership expiry that the notices will be sent on. To Test: 1) Create a new Patron and set membership expiry date 14 days from the date of registration. 2) Check your systemprefence ( MemExpDayNotice to 14 days default value) 3) Manual testing Run ( perl membership_expiry.pl -h) It would give you various option: This script prepares for membership expiry reminders to be sent to patrons. It queues them in the message queue, which is processed by the process_message_queue.pl cronjob. See the comments in the script for directions on changing the script. This script has the following parameters : -c Confirm and remove this help & warning -n send No mail. Instead, all mail messages are printed on screen. Usefull for testing purposes. -v verbose Do you wish to continue? (y/n) 4) Choose option for ex: perl membership_expiry.pl -c 5) Go to your koha database and check message_queue table you see some results. 6) Run (perl process_message_queue.pl) it will send email to those patron whose membership after 14 days from today. 7) Cron testing: (10 1 * * * $KOHA_CRON_PATH/membership_expiry.pl -c) 8) Set your 15 * * * * $KOHA_CRON_PATH/process_message_queue.pl 9) After running membership_expiry.pl, (process_message_queue.pl will send emails to those patron whose membership after 14 days from today).
Created attachment 21871 [details] Added new systempreference: Membership
Created attachment 21872 [details] Added new systempreference: MembershipExpiryNotice Added new systempreference: MembershipExpiryNotice
Applying: Bug 6810 Add new systempreference and sample notice (Membership Expiry notice) Using index info to reconstruct a base tree... M installer/data/mysql/sysprefs.sql M installer/data/mysql/updatedatabase.pl Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Patch failed at 0001 Bug 6810 Add new systempreference and sample notice (Membership Expiry notice) The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 22459 [details] [review] Added new systempreference: Membership Added new systempreference: Membership
The SysPref is present in the database (we looked for MembershipExpiryDaysNotice in PHPMyAdmin), but not in the Administration interface...
Looks OK at a glance, but the updatedatabase.pl is for Koha 3.13, so you'll want to update that.
Based at least on comment 13 I think this should be marked as "Failed QA."
(In reply to nicolas from comment #13) > The SysPref is present in the database (we looked for > MembershipExpiryDaysNotice in PHPMyAdmin), but not in the Administration > interface... Are you sure? Again, I haven't applied the actual patch, but it looks like it should be in the Patrons tab in the Global System Preferences.
*** Bug 11548 has been marked as a duplicate of this bug. ***
*** Bug 12496 has been marked as a duplicate of this bug. ***
(In reply to David Cook from comment #16) > (In reply to nicolas from comment #13) > > The SysPref is present in the database (we looked for > > MembershipExpiryDaysNotice in PHPMyAdmin), but not in the Administration > > interface... > > Are you sure? Again, I haven't applied the actual patch, but it looks like > it should be in the Patrons tab in the Global System Preferences. It is. Setting this back to "Needs signoff."
> 5) Go to your koha database and check message_queue table you see some > results. I confirm that the correct message has been added to the queue, but it doesn't contain an email address in the to_address column. Checking membership_expiry.pl it looks like it doesn't pass a "to_address" at all: C4::Letters::EnqueueLetter( { letter => $letter, borrowernumber => $recent->{'borrowernumber'}, from_address => $from_address, message_transport_type => 'email', } );
(In reply to Owen Leonard from comment #20) > > 5) Go to your koha database and check message_queue table you see some > > results. > > I confirm that the correct message has been added to the queue, but it > doesn't contain an email address in the to_address column. Checking > membership_expiry.pl it looks like it doesn't pass a "to_address" at all: > > C4::Letters::EnqueueLetter( { letter => $letter, > borrowernumber => $recent->{'borrowernumber'}, > from_address => $from_address, > message_transport_type => 'email', > } ); If I recall correctly, queued messages don't need a "to_address". When the message is being sent via process_message_queue.pl, it will lookup the email address using the borrowernumber and the "AutoEmailPrimaryAddress" system preference. Relevant subs are C4::Letters::SendQueuedMessages (in process_message_queue.pl), and C4::Members::GetNoticeEmailAddress (within the C4::Letters function calls).
If you process the message queue, you'll notice that the to_address in the database is updated after the email is sent. C4::Letters::_send_message_by_email() and C4::Letters::_update_message_to_address() take care of that.
Created attachment 32683 [details] [review] [SIGNED-OFF] Bug-6810: Send membership expiry reminder notices. A new crontab based perl script to send membership expiry reminders. A system preference controls the number of days in advance of membership expiry that the notices will be sent on. To Test: 1) Create a new Patron and set membership expiry date 14 days from the date of registration. 2) Check your systemprefence ( MemExpDayNotice to 14 days default value) 3) Manual testing Run ( perl membership_expiry.pl -h) It would give you various option: This script prepares for membership expiry reminders to be sent to patrons. It queues them in the message queue, which is processed by the process_message_queue.pl cronjob. See the comments in the script for directions on changing the script. This script has the following parameters : -c Confirm and remove this help & warning -n send No mail. Instead, all mail messages are printed on screen. Useful for testing purposes. -v verbose Do you wish to continue? (y/n) 4) Choose option for ex: perl membership_expiry.pl -c 5) Go to your koha database and check message_queue table you see some results. 6) Run (perl process_message_queue.pl) it will send email to those patron whose membership after 14 days from today. 7) Cron testing: (10 1 * * * $KOHA_CRON_PATH/membership_expiry.pl -c) 8) Set your 15 * * * * $KOHA_CRON_PATH/process_message_queue.pl 9) After running membership_expiry.pl, (process_message_queue.pl will send emails to those patron whose membership after 14 days from today). Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 32684 [details] [review] [SIGNED-OFF] Bug 6810 Add new systempreference and sample notice (Membership Expiry notice) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Tested both patches according to the test plan and found no problems.
Created attachment 32689 [details] [review] Bug 6810 [Revised] Add new systempreference and sample notice (Membership Expiry notice) This updated version of the database update patch revises the description of the system preference and the content of the notice to use the phrase "account expiration" or "card expiration," both of which are currently used in Koha templates.
Created attachment 32692 [details] [review] Bug 6810 [Revised] Add new systempreference and sample notice (Membership Expiry notice) This updated version of the database update patch revises the description of the system preference and the content of the notice to use the phrase "account expiration" or "card expiration," both of which are currently used in Koha templates. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Hi Amit, this is getting really close, but there are 2 things that should be fixed: - Can you please add the new sample notice to the non-English web-installers as well? The code is harcoded and that way it's easier for people to set up the new feature. Copying the English text is ok. - Can you please provide unit tests for the new routine GetUpcomingMembershipExpires? I'd like to see the cronjob being mentioned in the system preference, but that's not a blocker :)
Hi Katrin, Thanks for your input will recreate with test cases and sample notices.
*** Bug 6173 has been marked as a duplicate of this bug. ***
Created attachment 33954 [details] [review] Bug 6810 [Sample notices follow-up] Send membership expiry reminder notices This patch adds the new sample notice (in English) to the non-English web installers so that it is available to users of non-English templates. This patch also corrects the SQL of the sample notice updates to reflect changes in the structure of the notices table. To test, apply the patch and test installation of Koha with a blank database. The installation process should correctly add sample notices, including the new MEMEXP notice. Test again using a different set of translations to confirm that non-English sample notices are working as well.
> - Can you please add the new sample notice to the non-English web-installers > as well? I have added a patch for this because my library would really like to have this feature and I'd like to see progress on it. > - Can you please provide unit tests for the new routine > GetUpcomingMembershipExpires? Still waiting for this, so even if my new patch gets a signoff the bug should be set back to "Failed QA."
Hi Owen, I have written some test cases will share by this monday.
(In reply to Amit from comment #32) > Hi Owen, > > I have written some test cases will share by this monday. Up, have you these tests ?
The cron script would need a follow-up to for logging to the system log table, see Bug 13889
This patch set needs tests and a small follow up for the new cronjobs log. Setting status to failed QA. I think this would be a useful feature, please rescue it!
Hi Kartin, Will resend with test cases.
Patch does not apply.
Comment on attachment 32683 [details] [review] [SIGNED-OFF] Bug-6810: Send membership expiry reminder notices. Review of attachment 32683 [details] [review]: ----------------------------------------------------------------- ::: C4/Letters.pm @@ +611,5 @@ > + Add_Delta_Days( @dateexpiry,0) > + ), > + 'iso' > + )->output(); > + } There is no better way to format $values->{'dateexpiry'} from SQL to whatever the system preference format string is?! This is ugly. There has got to be a better way. ::: C4/Members.pm @@ +1561,5 @@ > + 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 = DATE_ADD(CURDATE(),INTERVAL $days DAY); Do the date math in Perl to avoid MySQLisms.
Comment on attachment 32692 [details] [review] Bug 6810 [Revised] Add new systempreference and sample notice (Membership Expiry notice) Review of attachment 32692 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/sysprefs.sql @@ +455,4 @@ > ('XSLTResultsDisplay','default','','Enable XSL stylesheet control over results page display on intranet','Free'), > ('yuipath','local','local|http://yui.yahooapis.com/2.5.1/build','Insert the path to YUI libraries, choose local if you use koha offline','Choice'), > ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'), > +('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo'); \ No newline at end of file This change is not necessary. ::: installer/data/mysql/updatedatabase.pl @@ +8845,4 @@ > SetVersion ($DBversion); > } > > +$DBversion = "3.17.00.XXX"; Might want to convert this updatedatabase.pl to an atomic update. http://wiki.koha-community.org/wiki/Database_updates Or at least change the number to 21 instead of 17. :)
Created attachment 40090 [details] [review] Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice)
Created attachment 40091 [details] [review] Bug 6810 - Add sample notices
Rebased the patches and replaced those which add sample notices and those which add syspref (using now atomic update). For the next step i will fix M. Tompsett QA comments and try to add test.
Alex++ :)
Created attachment 40100 [details] [review] [follow-up] Bug 6810 - Fix QA failures - Use KohaDates to convert dateexpiry - remove MYSQL specifics methods for date handling in GetUpcomingMembershipExpires - make the script membership_expiry.pl write in Koha system logs - add tests
Created attachment 40120 [details] [review] Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Created attachment 40121 [details] [review] Bug 6810 - Add sample notices Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Created attachment 40122 [details] [review] Bug 6810 - Fix QA failures - Use KohaDates to convert dateexpiry - remove MYSQL specifics methods for date handling in GetUpcomingMembershipExpires - make the script membership_expiry.pl write in Koha system logs - add tests Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Comment on attachment 40122 [details] [review] Bug 6810 - Fix QA failures Review of attachment 40122 [details] [review]: ----------------------------------------------------------------- The tests file is db dependent. ::: C4/Letters.pm @@ +35,4 @@ > use Encode; > use Carp; > use Koha::Email; > +use Koha::Template::Plugin::KohaDates; No, you should not use Koha::Template::Plugin outside templates. Use Koha::DateUtils. ::: t/Members.t @@ +40,5 @@ > + ); > + > +}); > + > +t::lib::Mocks::mock_preference('MembershipExpiryDaysNotice', 15); Please add tests with MembershipExpiryDaysNotice equals 0 or undef.
Created attachment 40179 [details] [review] [Followup-up] Bug 6810 - Fix QA failures: - use Koha::DateUtils instead of Koha::Template::Plugin::KohaDates, - Add test with syspref MembershipExpiryDaysNotice equals 0 and undef, - fix (new) test failure (when MembershipExpiryDaysNotice is undef).
Created attachment 40180 [details] [review] Bug 6810 - Fix QA failures: - use Koha::DateUtils instead of Koha::Template::Plugin::KohaDates, - Add test with syspref MembershipExpiryDaysNotice equals 0 and undef, - fix (new) test failure (when MembershipExpiryDaysNotice is undef). Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
A syspref again :( We could give the period as argument of script.
A command line option is a disavantage for package installations with multiple instances, but also for small libraries. If it's a pref, any librarian can change it, but if it's a command line option, only an IT person can. I am in favour of prefs in general.
(In reply to Katrin Fischer from comment #52) > A command line option is a disavantage for package installations with > multiple instances, but also for small libraries. If it's a pref, any > librarian can change it, but if it's a command line option, only an IT > person can. > I am in favour of prefs in general. If it's a small library, the librarian is the IT person. That being said, there is nothing stopping it from being both. That gives the geeky IT people the ability to not change any settings in the system, and it gives the library people the ability to actually do it themselves (and totally mess up the geeky IT people's work). We have had code in Koha where a command line was overridden by environment variables. We could easily use the same kind of logic and have system preferences override/augment command-line options.
I am not a fan of the override idea. I think either/or. I think otherwise it's just confusing things unnecessarily. This is a new feature, so we don't need to retain old behaviour. I also disagree about the IT person being the same thing as the librarian for small library - it might also be a volunteer who set up the system and is no longer available or it might just be a hosted solution. The range is wide.
I agree with Katrin. I don't think the assumption that the librarian is an/the IT person is necessarily correct for small libraries. And this setting is nothing that IT would decide, but a librarian, so it should be available to them, even though the amount of sysprefs is getting more and more overwhelming.
(In reply to Mirko Tietgen from comment #55) > I agree with Katrin. I don't think the assumption that the librarian is > an/the IT person is necessarily correct for small libraries. Depends largely on the geography concerned. For instance in most Indian libraries, the Librarian *is* the de-facto IT person. whatever they have the skills for its or not is a moot point.
(In reply to Katrin Fischer from comment #54) > I am not a fan of the override idea. I think either/or. I think otherwise > it's just confusing things unnecessarily. > This is a new feature, so we don't need to retain old behaviour. Okay. I was just throwing out a brainstorm idea. > I also disagree about the IT person being the same thing as > the librarian for small library - it might also be a volunteer > who set up the system and is no longer available -- thus left the IT responsibilities on the librarian. > or it might just be a hosted solution. *laugh* No, because it's a small library with no budget. This is why I said the librarian is the IT person. Doesn't mean they know how to do it, but they are responsible for it. Mirko wrote: > though the amount of sysprefs is getting more and more overwhelming. AMEN! Though, the advantage is they have a chance of being better documented than config file parameters. ;)
1/ + my $dateexpiry = DateTime->now()->add(days => $days)->ymd(); You should use Koha::DateUtils->dt_from_string; to take the tz into account. 2/ The letter.code field is a varchar(20), I am sure you can find something more meaningful than "MEMEXP". 3/ The file installer/data/mysql/sysprefs.sql should be kept ordered. Don't put your line at the end of the file. 4/ The cronjob: a. No need to warn, prefer say b. use Modern::Perl; c. a cronjob should not be interactive. d. use Pod2usage for the usage (see other scripts). 5/ The letter: Are you sure that only information from the borrowers table is useful? It would be good to have info from the branches table, as a minimum.
Created attachment 40641 [details] [review] Bug 6810 - Fix QA failures Bug 6810 - Fix QA failures - remove DateTime->now() - use Koha::DateUtils->dt_from_string; - use Pod2usage for the usage - use Modern::Perl - use branches table - Change letter code from MEMEXP to MEMBERSHIP_EXPIRY
Created attachment 40642 [details] [review] Bug 6810 - Fix QA failures MembershipExpiryDaysNotice system preferences arragned alphabetical order.
Created attachment 40643 [details] [review] Bug 6810 - Fix QA failures Change letter code from MEMEXP to MEMBERSHIP_EXPIRY add <<branches.branchname>> in letter
Created attachment 40644 [details] [review] Bug 6810 - Fix QA failures - remove DateTime->now() - use Koha::DateUtils->dt_from_string; - use Pod2usage for the usage - use Modern::Perl - use branches table - Change letter code from MEMEXP to MEMBERSHIP_EXPIRY
Created attachment 40645 [details] [review] Bug 6810 - Fix QA failures MembershipExpiryDaysNotice system preferences arragned alphabetical order.
Created attachment 40646 [details] [review] Bug 6810 - Fix QA failures Change letter code from MEMEXP to MEMBERSHIP_EXPIRY add branches table <<branches.branchname>> in letter
Created attachment 40649 [details] [review] Bug-6810-Add-sample-notices Add Sample notices
Created attachment 40665 [details] [review] [SIGNED-OFF] Bug-6810: Send membership expiry reminder notices. A new crontab based perl script to send membership expiry reminders. A system preference controls the number of days in advance of membership expiry that the notices will be sent on. To Test: 1) Create a new Patron and set membership expiry date 14 days from the date of registration. 2) Check your systemprefence ( MemExpDayNotice to 14 days default value) 3) Manual testing Run ( perl membership_expiry.pl -h) It would give you various option: This script prepares for membership expiry reminders to be sent to patrons. It queues them in the message queue, which is processed by the process_message_queue.pl cronjob. See the comments in the script for directions on changing the script. This script has the following parameters : -c Confirm and remove this help & warning -n send No mail. Instead, all mail messages are printed on screen. Useful for testing purposes. -v verbose Do you wish to continue? (y/n) 4) Choose option for ex: perl membership_expiry.pl -c 5) Go to your koha database and check message_queue table you see some results. 6) Run (perl process_message_queue.pl) it will send email to those patron whose membership after 14 days from today. 7) Cron testing: (10 1 * * * $KOHA_CRON_PATH/membership_expiry.pl -c) 8) Set your 15 * * * * $KOHA_CRON_PATH/process_message_queue.pl 9) After running membership_expiry.pl, (process_message_queue.pl will send emails to those patron whose membership after 14 days from today). Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 40666 [details] [review] Bug 6810: [SIGNED-OFF] Squashed Alex Arnaud's patches The following three patches were already signed off by Indranil and authored by Alex, so I squashed them for simplicity. -- Mark Tompsett Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) Bug 6810 - Fix QA failures - Use KohaDates to convert dateexpiry - remove MYSQL specifics methods for date handling in GetUpcomingMembershipExpires - make the script membership_expiry.pl write in Koha system logs - add tests Bug 6810 - Fix QA failures: - use Koha::DateUtils instead of Koha::Template::Plugin::KohaDates, - Add test with syspref MembershipExpiryDaysNotice equals 0 and undef, - fix (new) test failure (when MembershipExpiryDaysNotice is undef). Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
Created attachment 40667 [details] [review] Bug 6810: Squashed Amit Gupta's patches Amit added these fixes which were not signed off yet, so I squashed them for simplicity. -- Mark Tompsett Bug 6810 - Fix QA failures - remove DateTime->now() - use Koha::DateUtils->dt_from_string; - use Pod2usage for the usage - use Modern::Perl - use branches table - Change letter code from MEMEXP to MEMBERSHIP_EXPIRY Bug 6810 - Fix QA failures - MembershipExpiryDaysNotice system preferences arragned alphabetical order. Bug 6810 - Add sample notices
Comment on attachment 40667 [details] [review] Bug 6810: Squashed Amit Gupta's patches Review of attachment 40667 [details] [review]: ----------------------------------------------------------------- ::: C4/Members.pm @@ +42,4 @@ > use Koha::AuthUtils qw(hash_password); > use Koha::Database; > use Module::Load; > +use DateTime::Duration; Why make it more complex here... @@ +1491,4 @@ > > sub GetUpcomingMembershipExpires { > my $dbh = C4::Context->dbh; > + my $days = DateTime::Duration->new(days => C4::Context->preference("MembershipExpiryDaysNotice") || 0); and here... @@ +1491,5 @@ > > sub GetUpcomingMembershipExpires { > my $dbh = C4::Context->dbh; > + my $days = DateTime::Duration->new(days => C4::Context->preference("MembershipExpiryDaysNotice") || 0); > + my $dateexpiry = output_pref({ dt => (dt_from_string() + $days), dateformat => 'iso', dateonly => 1 }); -- Why not keep the ->add(days => $days): my $dateexpiry = output_pref({ dt => (dt_from_string()->add( days => $days)), dateformat => 'iso', dateonly => 1 }); -- dt_from_string passes back a DateTime object, just like DateTime->now(). -- Though, this gives me an idea to improve one of my patches. DOH! @@ -1489,5 @@ > > sub GetUpcomingMembershipExpires { > my $dbh = C4::Context->dbh; > - my $days = C4::Context->preference("MembershipExpiryDaysNotice") || 0; > - my $dateexpiry = DateTime->now()->add(days => $days)->ymd(); ... if the ->add(days => $days) part was working here? ::: installer/data/mysql/de-DE/mandatory/sample_notices.sql @@ +136,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Benachrichtigung bei Zugang', 'Bestelltes Medium ist eingetroffen', 'Liebe/r <<borrowers.firstname>> <<borrowers.surname>>,\n\nDie Bestellung <<aqorders.ordernumber>> (<<biblio.title>>) ist eingetroffen und wird bearbeitet.\n\nIhr Bibliotheksteam', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) Why did you exclude the message_transport_type? Plus, if you added is_html to the INSERT above this one (and added the appropriate default value of 0 in the VALUES list), you could have used commas, instead of a whole new INSERT. ::: installer/data/mysql/en/mandatory/sample_notices.sql @@ +155,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email') > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/es-ES/mandatory/sample_notices.sql @@ +150,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/fr-FR/1-Obligatoire/sample_notices.sql @@ +151,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/it-IT/necessari/notices.sql @@ +148,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/nb-NO/1-Obligatorisk/sample_notices.sql @@ +170,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/pl-PL/mandatory/sample_notices.sql @@ +148,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/ru-RU/mandatory/sample_notices.sql @@ +150,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: installer/data/mysql/uk-UA/mandatory/sample_notices.sql @@ +149,5 @@ > > INSERT INTO letter(module, code, branchcode, name, title, content, message_transport_type) > +VALUES ('acquisition', 'ACQ_NOTIF_ON_RECEIV', '', 'Notification on receiving', 'Order received', 'Dear <<borrowers.firstname>> <<borrowers.surname>>,\n\n The order <<aqorders.ordernumber>> (<<biblio.title>>) has been received.\n\nYour library.', 'email'); > + > +INSERT INTO letter(module, code, branchcode, name, title, is_html, content) As above. ::: misc/cronjobs/membership_expiry.pl @@ +30,5 @@ > +0 1 * * * membership_expiry.pl -c > + > +=head1 DESCRIPTION > + > +This script send memberships expires reminder notices to be sent to This script sends membership expiry reminder notices to patrons. @@ +31,5 @@ > + > +=head1 DESCRIPTION > + > +This script send memberships expires reminder notices to be sent to > +patrons. It queues them in the message queue, which is processed by It queues them in the message queue, which is processed by
Created attachment 40685 [details] [review] Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice)
Created attachment 40686 [details] [review] Bug 6810 - Fix QA failures
Created attachment 40687 [details] [review] Bug 6810 - Fix QA failures
Created attachment 40688 [details] [review] Bug 6810 - Fix QA failures - remove DateTime->now() - use Koha::DateUtils->dt_from_string; - use Pod2usage for the usage - use Modern::Perl - use branches table - Change letter code from MEMEXP to MEMBERSHIP_EXPIRY - review comments implemented
Created attachment 40689 [details] [review] Bug 6810 - Fix QA failures MembershipExpiryDaysNotice system preferences arragned alphabetical order
Created attachment 40690 [details] [review] Bug-6810-Add-sample-notices Review comments implemented
Created attachment 40700 [details] [review] Bug 6810: [SIGNED-OFF] Squashed Alex Arnaud's patches The following three patches already signed off by Indranil and authored by Alex, so I squashed them for simplicity.
Created attachment 40701 [details] [review] Bug 6810: Squashed Fixes QA failures Bug 6810 - Fix QA failures -- Squashed three patches for simplicity
FAIL C4/Members.pm OK critic FAIL forbidden patterns forbidden pattern: trailing space char (line 1493) OK pod OK spelling OK valid FAIL misc/cronjobs/membership_expiry.pl OK critic FAIL forbidden patterns forbidden pattern: trailing space char (line 172) forbidden pattern: trailing space char (line 226) forbidden pattern: trailing space char (line 121) forbidden pattern: trailing space char (line 36) forbidden pattern: trailing space char (line 156) forbidden pattern: trailing space char (line 176) forbidden pattern: trailing space char (line 115) FAIL pod *** ERROR: Apparent command =cut not preceded by blank line in file misc/cronjobs/membership_expiry.pl FAIL spelling Usefull ==> Useful OK valid Please run the qa script before submitting patches.
Created attachment 40766 [details] [review] Bug 6810: Squashed Fixes QA failures QA script result: Processing files before patches |========================>| 5 / 5 (100.00%) Processing files after patches |========================>| 5 / 5 (100.00%) OK C4/Members.pm OK critic OK forbidden patterns OK pod OK spelling OK valid OK misc/cronjobs/membership_expiry.pl OK critic OK forbidden patterns OK pod OK spelling 0 OK valid
Created attachment 41916 [details] [review] Bug 6810 - Fix QA failures - remove DateTime->now() - use Koha::DateUtils->dt_from_string; - use Pod2usage for the usage - use Modern::Perl - use branches table - Change letter code from MEMEXP to MEMBERSHIP_EXPIRY - review comments implemented - fix qa script comments Bug 6810 - Fix QA failures - MembershipExpiryDaysNotice system preferences arragned alphabetical order. Bug 6810 - Add sample notices - review comments implemented - default value of is_html field in letter table is 0 Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
QA: Looking at this one
Created attachment 42609 [details] [review] Bug-6810: Send membership expiry reminder notices. A new crontab based perl script to send membership expiry reminders. A system preference controls the number of days in advance of membership expiry that the notices will be sent on. To Test: 1) Create a new Patron and set membership expiry date 14 days from the date of registration. 2) Check your systemprefence ( MemExpDayNotice to 14 days default value) 3) Manual testing Run ( perl membership_expiry.pl -h) It would give you various option: This script prepares for membership expiry reminders to be sent to patrons. It queues them in the message queue, which is processed by the process_message_queue.pl cronjob. See the comments in the script for directions on changing the script. This script has the following parameters : -c Confirm and remove this help & warning -n send No mail. Instead, all mail messages are printed on screen. Useful for testing purposes. -v verbose Do you wish to continue? (y/n) 4) Choose option for ex: perl membership_expiry.pl -c 5) Go to your koha database and check message_queue table you see some results. 6) Run (perl process_message_queue.pl) it will send email to those patron whose membership after 14 days from today. 7) Cron testing: (10 1 * * * $KOHA_CRON_PATH/membership_expiry.pl -c) 8) Set your 15 * * * * $KOHA_CRON_PATH/process_message_queue.pl 9) After running membership_expiry.pl, (process_message_queue.pl will send emails to those patron whose membership after 14 days from today). Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 42610 [details] [review] Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Bug 6810 - Fix QA failures - Use KohaDates to convert dateexpiry - remove MYSQL specifics methods for date handling in GetUpcomingMembershipExpires - make the script membership_expiry.pl write in Koha system logs - add tests Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Bug 6810 - Fix QA failures: - use Koha::DateUtils instead of Koha::Template::Plugin::KohaDates, - Add test with syspref MembershipExpiryDaysNotice equals 0 and undef, - fix (new) test failure (when MembershipExpiryDaysNotice is undef). Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 42611 [details] [review] Bug 6810 - Fix QA failures - remove DateTime->now() - use Koha::DateUtils->dt_from_string; - use Pod2usage for the usage - use Modern::Perl - use branches table - Change letter code from MEMEXP to MEMBERSHIP_EXPIRY - review comments implemented - fix qa script comments Bug 6810 - Fix QA failures - MembershipExpiryDaysNotice system preferences arragned alphabetical order. Bug 6810 - Add sample notices - review comments implemented - default value of is_html field in letter table is 0 Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 42612 [details] [review] Bug 6810: [QA Follow-up] Move test file to db_dependent Note that this was already mentioned by Jonathan on comment48. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 42613 [details] [review] Bug 6810: [QA Follow-up] Some minor tidying up of cron script Moved some POD lines and removed some duplicated POD lines. Corrected some indentation. Did some minor spelling changes, rewording. Removed the C4::Dates module. No longer needed. Removed a label in front of the for statement. Removed the Do you wish to continue-code. Should not be in a cron job. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Nice feature, has been pending for quite some time. As you see, I added two minor follow-ups. One moved the test file and the other is merely cosmetic (moving spaces around and so :) Note that the notice text might still need some attention: === Dear Test02, Your library card will expire soon, on: 30/09/2015 00:00 Thank you, Librarian === A report that has been pushed (yes today), can eliminate the hours and minutes for you. See bug 13622; it adds the possibility of a filter dateonly. I would prefer to see the lines combined: Your card will expire on 30/09/2015. Librarian should probably be Your library (see other notices). No blocker, but maybe a quick qa follow-up or opening a new report can still improve this. (Final word for the RM :) Another point: With reference to comment7 it would be handy to have an option to include expires within a range of days. You could run it weekly or rerun it etc. I have opened a followup report under bug 14834. Passed QA
And for the record: We should still add this new cron job to the example file and somewhere commented in a cron file for the packages..
Created attachment 42700 [details] [review] Bug 6810: [QA Follow-up] Exit cronjob if pref not set As per suggestion of Robin on report 14840, it would be better to always run the cronjob and only do something when the pref is set. This patch adds a test in the cronjob and clears the former default of 14 days. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Removed the pref and ran the dbrev again: Fine. Run the cronjob with -c -v -n: Prints exit warning.
Patches pushed to master. Thanks Amit, Alex and Marcel!
Comment on attachment 42609 [details] [review] Bug-6810: Send membership expiry reminder notices. Review of attachment 42609 [details] [review]: ----------------------------------------------------------------- ::: C4/Letters.pm @@ +800,5 @@ > > + if ( $table eq 'borrowers' && $values->{'dateexpiry'} ){ > + my @dateexpiry = split /-/, $values->{'dateexpiry'}; > + > + $values->{'dateexpiry'} = C4::Dates->new( This was not really kind of Marc. He is trying to remove the C4::Dates usage.
(In reply to Jonathan Druart from comment #91) > Comment on attachment 42609 [details] [review] [review] > Bug-6810: Send membership expiry reminder notices. > > Review of attachment 42609 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: C4/Letters.pm > @@ +800,5 @@ > > > > + if ( $table eq 'borrowers' && $values->{'dateexpiry'} ){ > > + my @dateexpiry = split /-/, $values->{'dateexpiry'}; > > + > > + $values->{'dateexpiry'} = C4::Dates->new( > > This was not really kind of Marc. He is trying to remove the C4::Dates usage. Ooops, sorry, removed by next patch!!