Bug 6810 - Send membership expiry reminder notices
Summary: Send membership expiry reminder notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P4 enhancement (vote)
Assignee: Amit Gupta
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 6173 11548 12496 (view as bug list)
Depends on:
Blocks: 14834 14840 15429
  Show dependency treegraph
 
Reported: 2011-08-30 05:14 UTC by Savitra Sirohi
Modified: 2017-06-14 22:10 UTC (History)
23 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this: 2013-10-28 00:00:00
Who signed the patch off:
Text to go in the release notes:


Attachments
Membership expiry notice (31.34 KB, patch)
2011-09-09 07:41 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug_6810 Send membership expiry reminder notices. (44.15 KB, patch)
2012-05-24 04:40 UTC, Amit Gupta
Details | Diff | Splinter Review
Send membership expiry reminder notices. (9.05 KB, patch)
2013-10-08 08:58 UTC, Amit Gupta
Details | Diff | Splinter Review
Added new systempreference: Membership (3.48 KB, application/mbox)
2013-10-08 08:59 UTC, Amit Gupta
Details
Added new systempreference: MembershipExpiryNotice (3.48 KB, application/mbox)
2013-10-08 09:00 UTC, Amit Gupta
Details
Added new systempreference: Membership (3.48 KB, patch)
2013-10-28 06:42 UTC, Amit Gupta
Details | Diff | Splinter Review
[SIGNED-OFF] Bug-6810: Send membership expiry reminder notices. (9.15 KB, patch)
2014-10-24 18:11 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 6810 Add new systempreference and sample notice (Membership Expiry notice) (3.78 KB, patch)
2014-10-24 18:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6810 [Revised] Add new systempreference and sample notice (Membership Expiry notice) (5.33 KB, patch)
2014-10-24 18:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6810 [Revised] Add new systempreference and sample notice (Membership Expiry notice) (5.39 KB, patch)
2014-10-24 19:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 6810 [Sample notices follow-up] Send membership expiry reminder notices (8.16 KB, patch)
2014-11-26 15:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) (2.89 KB, patch)
2015-06-11 13:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6810 - Add sample notices (13.11 KB, patch)
2015-06-11 13:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
[follow-up] Bug 6810 - Fix QA failures (6.16 KB, patch)
2015-06-12 10:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) (2.96 KB, patch)
2015-06-14 08:51 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 6810 - Add sample notices (13.18 KB, patch)
2015-06-14 08:53 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (6.21 KB, patch)
2015-06-14 08:54 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
[Followup-up] Bug 6810 - Fix QA failures: (3.09 KB, patch)
2015-06-16 12:52 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures: (3.14 KB, patch)
2015-06-16 13:16 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (3.15 KB, patch)
2015-06-26 06:33 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (2.19 KB, patch)
2015-06-26 06:34 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (13.53 KB, patch)
2015-06-26 06:35 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (7.23 KB, patch)
2015-06-26 06:40 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (2.19 KB, patch)
2015-06-26 06:41 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (13.53 KB, patch)
2015-06-26 06:42 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug-6810-Add-sample-notices (13.13 KB, patch)
2015-06-26 08:23 UTC, Amit Gupta
Details | Diff | Splinter Review
[SIGNED-OFF] Bug-6810: Send membership expiry reminder notices. (9.09 KB, patch)
2015-06-26 14:02 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6810: [SIGNED-OFF] Squashed Alex Arnaud's patches (9.99 KB, patch)
2015-06-26 14:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6810: Squashed Amit Gupta's patches (21.87 KB, patch)
2015-06-26 14:03 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) (2.91 KB, patch)
2015-06-26 17:42 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (6.22 KB, patch)
2015-06-26 17:43 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (3.15 KB, patch)
2015-06-26 17:44 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (6.79 KB, patch)
2015-06-26 17:45 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (2.19 KB, patch)
2015-06-26 17:45 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug-6810-Add-sample-notices (12.71 KB, patch)
2015-06-26 17:46 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810: [SIGNED-OFF] Squashed Alex Arnaud's patches (9.96 KB, patch)
2015-06-28 03:18 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810: Squashed Fixes QA failures (21.29 KB, patch)
2015-06-28 03:22 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810: Squashed Fixes QA failures (21.43 KB, patch)
2015-07-02 17:38 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (21.48 KB, patch)
2015-08-25 13:58 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug-6810: Send membership expiry reminder notices. (9.19 KB, patch)
2015-09-16 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6810 - Add new systempreference (MembershipExpiryDaysNotice) (10.05 KB, patch)
2015-09-16 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6810 - Fix QA failures (21.57 KB, patch)
2015-09-16 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6810: [QA Follow-up] Move test file to db_dependent (810 bytes, patch)
2015-09-16 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6810: [QA Follow-up] Some minor tidying up of cron script (8.38 KB, patch)
2015-09-16 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6810: [QA Follow-up] Exit cronjob if pref not set (3.58 KB, patch)
2015-09-18 11:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Savitra Sirohi 2011-08-30 05:14:18 UTC
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.
Comment 1 Amit Gupta 2011-09-09 07:41:56 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-10-24 11:38:16 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 3 Paul Poulain 2011-10-25 15:05:51 UTC
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)
Comment 4 Katrin Fischer 2011-12-20 20:55:18 UTC
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?
Comment 5 Amit Gupta 2012-05-24 04:40:51 UTC Comment hidden (obsolete)
Comment 6 Irma Birchall 2012-06-11 16:14:00 UTC
Can't be tested in a sandbox as it needs command line access.
Comment 7 Frédéric Demians 2012-07-14 16:43:44 UTC
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.
Comment 8 Amit Gupta 2013-10-08 08:58:45 UTC Comment hidden (obsolete)
Comment 9 Amit Gupta 2013-10-08 08:59:26 UTC Comment hidden (obsolete)
Comment 10 Amit Gupta 2013-10-08 09:00:17 UTC Comment hidden (obsolete)
Comment 11 I'm just a bot 2013-10-28 06:26:52 UTC
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".
Comment 12 Amit Gupta 2013-10-28 06:42:11 UTC Comment hidden (obsolete)
Comment 13 nicolas 2014-01-10 14:47:59 UTC
The SysPref is present in the database (we looked for MembershipExpiryDaysNotice in PHPMyAdmin), but not in the Administration interface...
Comment 14 David Cook 2014-03-26 06:53:23 UTC
Looks OK at a glance, but the updatedatabase.pl is for Koha 3.13, so you'll want to update that.
Comment 15 Owen Leonard 2014-03-28 19:41:44 UTC
Based at least on comment 13 I think this should be marked as "Failed QA."
Comment 16 David Cook 2014-03-30 22:49:30 UTC
(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.
Comment 17 Katrin Fischer 2014-06-29 12:55:11 UTC
*** Bug 11548 has been marked as a duplicate of this bug. ***
Comment 18 Katrin Fischer 2014-06-29 12:55:20 UTC
*** Bug 12496 has been marked as a duplicate of this bug. ***
Comment 19 Owen Leonard 2014-09-02 15:54:04 UTC
(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."
Comment 20 Owen Leonard 2014-09-02 16:08:34 UTC
>     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',
} );
Comment 21 David Cook 2014-09-02 23:30:10 UTC
(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).
Comment 22 David Cook 2014-09-02 23:32:36 UTC
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.
Comment 23 Owen Leonard 2014-10-24 18:11:52 UTC Comment hidden (obsolete)
Comment 24 Owen Leonard 2014-10-24 18:12:05 UTC Comment hidden (obsolete)
Comment 25 Owen Leonard 2014-10-24 18:58:04 UTC Comment hidden (obsolete)
Comment 26 Chris Cormack 2014-10-24 19:42:02 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2014-10-25 20:48:53 UTC
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 :)
Comment 28 Amit Gupta 2014-10-27 03:35:37 UTC
Hi Katrin,

Thanks for your input will recreate with test cases and sample notices.
Comment 29 Katrin Fischer 2014-11-02 10:53:47 UTC
*** Bug 6173 has been marked as a duplicate of this bug. ***
Comment 30 Owen Leonard 2014-11-26 15:19:06 UTC Comment hidden (obsolete)
Comment 31 Owen Leonard 2014-11-26 15:20:27 UTC
> - 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."
Comment 32 Amit Gupta 2014-11-27 00:42:12 UTC
Hi Owen,

I have written some test cases will share by this monday.
Comment 33 Fridolin SOMERS 2015-02-02 15:42:37 UTC
(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 ?
Comment 34 Marc Véron 2015-05-24 06:45:24 UTC
The cron script would need a follow-up to for logging to the system log table, see Bug 13889
Comment 35 Katrin Fischer 2015-05-24 14:15:10 UTC
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!
Comment 36 Amit Gupta 2015-05-24 14:18:27 UTC
Hi Kartin,

Will resend with test cases.
Comment 37 M. Tompsett 2015-06-04 01:43:17 UTC
Patch does not apply.
Comment 38 M. Tompsett 2015-06-04 03:04:20 UTC
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 39 M. Tompsett 2015-06-04 03:07:08 UTC
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. :)
Comment 40 Alex Arnaud 2015-06-11 13:01:15 UTC Comment hidden (obsolete)
Comment 41 Alex Arnaud 2015-06-11 13:01:38 UTC Comment hidden (obsolete)
Comment 42 Alex Arnaud 2015-06-11 13:05:50 UTC
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.
Comment 43 Katrin Fischer 2015-06-11 13:58:19 UTC
Alex++ :)
Comment 44 Alex Arnaud 2015-06-12 10:11:16 UTC Comment hidden (obsolete)
Comment 45 Indranil Das Gupta 2015-06-14 08:51:31 UTC Comment hidden (obsolete)
Comment 46 Indranil Das Gupta 2015-06-14 08:53:05 UTC Comment hidden (obsolete)
Comment 47 Indranil Das Gupta 2015-06-14 08:54:42 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2015-06-16 09:08:12 UTC
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.
Comment 49 Alex Arnaud 2015-06-16 12:52:50 UTC Comment hidden (obsolete)
Comment 50 Indranil Das Gupta 2015-06-16 13:16:13 UTC Comment hidden (obsolete)
Comment 51 Fridolin SOMERS 2015-06-19 09:18:49 UTC
A syspref again :(
We could give the period as argument of script.
Comment 52 Katrin Fischer 2015-06-19 09:31:31 UTC
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.
Comment 53 M. Tompsett 2015-06-19 12:26:39 UTC
(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.
Comment 54 Katrin Fischer 2015-06-19 12:43:12 UTC
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.
Comment 55 Mirko Tietgen 2015-06-19 13:01:39 UTC
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.
Comment 56 Indranil Das Gupta 2015-06-19 13:12:31 UTC
(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.
Comment 57 M. Tompsett 2015-06-19 13:14:18 UTC
(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. ;)
Comment 58 Jonathan Druart 2015-06-22 12:12:38 UTC
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.
Comment 59 Amit Gupta 2015-06-26 06:33:34 UTC Comment hidden (obsolete)
Comment 60 Amit Gupta 2015-06-26 06:34:24 UTC Comment hidden (obsolete)
Comment 61 Amit Gupta 2015-06-26 06:35:16 UTC Comment hidden (obsolete)
Comment 62 Amit Gupta 2015-06-26 06:40:25 UTC Comment hidden (obsolete)
Comment 63 Amit Gupta 2015-06-26 06:41:42 UTC Comment hidden (obsolete)
Comment 64 Amit Gupta 2015-06-26 06:42:26 UTC Comment hidden (obsolete)
Comment 65 Amit Gupta 2015-06-26 08:23:01 UTC Comment hidden (obsolete)
Comment 66 M. Tompsett 2015-06-26 14:02:59 UTC Comment hidden (obsolete)
Comment 67 M. Tompsett 2015-06-26 14:03:16 UTC Comment hidden (obsolete)
Comment 68 M. Tompsett 2015-06-26 14:03:31 UTC Comment hidden (obsolete)
Comment 69 M. Tompsett 2015-06-26 14:50:49 UTC
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
Comment 70 Amit Gupta 2015-06-26 17:42:54 UTC Comment hidden (obsolete)
Comment 71 Amit Gupta 2015-06-26 17:43:52 UTC Comment hidden (obsolete)
Comment 72 Amit Gupta 2015-06-26 17:44:25 UTC Comment hidden (obsolete)
Comment 73 Amit Gupta 2015-06-26 17:45:07 UTC Comment hidden (obsolete)
Comment 74 Amit Gupta 2015-06-26 17:45:50 UTC Comment hidden (obsolete)
Comment 75 Amit Gupta 2015-06-26 17:46:31 UTC Comment hidden (obsolete)
Comment 76 Amit Gupta 2015-06-28 03:18:53 UTC Comment hidden (obsolete)
Comment 77 Amit Gupta 2015-06-28 03:22:24 UTC Comment hidden (obsolete)
Comment 78 Jonathan Druart 2015-07-02 10:06:46 UTC
 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.
Comment 79 Amit Gupta 2015-07-02 17:38:36 UTC Comment hidden (obsolete)
Comment 80 Alex Arnaud 2015-08-25 13:58:37 UTC Comment hidden (obsolete)
Comment 81 Marcel de Rooy 2015-09-16 14:20:33 UTC
QA: Looking at this one
Comment 82 Marcel de Rooy 2015-09-16 14:56:14 UTC
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>
Comment 83 Marcel de Rooy 2015-09-16 14:56:20 UTC
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>
Comment 84 Marcel de Rooy 2015-09-16 14:56:26 UTC
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>
Comment 85 Marcel de Rooy 2015-09-16 14:56:31 UTC
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>
Comment 86 Marcel de Rooy 2015-09-16 14:56:37 UTC
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>
Comment 87 Marcel de Rooy 2015-09-16 15:27:53 UTC
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
Comment 88 Marcel de Rooy 2015-09-16 15:39:22 UTC
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..
Comment 89 Marcel de Rooy 2015-09-18 11:32:37 UTC
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.
Comment 90 Tomás Cohen Arazi 2015-10-06 16:19:48 UTC
Patches pushed to master.

Thanks Amit, Alex and Marcel!
Comment 91 Jonathan Druart 2015-10-07 07:57:25 UTC
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.
Comment 92 Jonathan Druart 2015-10-07 07:58:27 UTC
(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!!