20076 – Overdues email to library for patrons without email should be optional

Bug 20076 - Overdues email to library for patrons without email should be optional

Summary: Overdues email to library for patrons without email should be optional

Status:	CLOSED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Tools (show other bugs)
Version:	Main
Hardware:	All All

Importance:	P5 - low enhancement
Assignee:	Thibaud Guillot (thibaud_g)
QA Contact:	Testopia

URL:
Keywords:

Duplicates (1):	19888 (view as bug list)
Depends on:
Blocks:

Reported:	2018-01-23 14:48 UTC by Victor Grousset/tuxayo
Modified:	2022-12-12 21:24 UTC (History)
CC List:	9 users (show)

See Also:	19888 30054
Change sponsored?:	---
Patch complexity:	Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:	Currently, two print notices are generated when running overdue_notices.pl if a patron does not have an email address: - a print overdue notice for the patron, and - an email message to the library with all the print versions of the overdue notices. Depending on a library's work processes, they may want both or only the patron print overdue notice generated. This enhancement adds a new system preference, EmailOverduesNoEmail, that allows libraries to choose whether to send or not send overdue notices for patrons without an email address to library staff. The default is set to send, as this preserves the current behaviour.
Version(s) released in:	22.05.00,21.11.02
Circulation function:

Attachments
Bug 20076: Overdues: remove redundant message when patrons don't have email addresses (4.39 KB, patch) 2018-01-23 14:52 UTC, Victor Grousset/tuxayo	Details \| Diff \| Splinter Review
Add syspref to deal with overdues notification (7.31 KB, patch) 2021-12-10 15:19 UTC, Thibaud Guillot (thibaud_g)	Details \| Diff \| Splinter Review
Add syspref to remove overdues notification by email to staff if user has no email address (7.33 KB, patch) 2021-12-13 08:38 UTC, Thibaud Guillot (thibaud_g)	Details \| Diff \| Splinter Review
Add syspref to remove overdues notification by email to staff if user has no email address (7.30 KB, patch) 2021-12-14 16:24 UTC, David Nind	Details \| Diff \| Splinter Review
Bug 20076: Add syspref to remove overdues notification by email to staff if user has no email address (7.34 KB, patch) 2021-12-19 13:19 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 20076: (QA follow-up) Reformat database update to use newer format (1.42 KB, patch) 2021-12-19 13:19 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 20076: (QA follow-up) Rephrase system preference description sightly (3.30 KB, patch) 2021-12-19 13:19 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 20076: (QA follow-up) We need to default to 'Send' on update to keep existing behavior (1.21 KB, patch) 2021-12-19 13:19 UTC, Katrin Fischer	Details \| Diff \| Splinter Review
Bug 20076: (RM follow-up) Fix default value in installer/data/mysql/mandatory/sysprefs.sql (4.03 KB, patch) 2022-01-14 01:53 UTC, Fridolin Somers	Details \| Diff \| Splinter Review
Bug 20076: (RM follow-up) Fix system preference boolean logic (5.36 KB, patch) 2022-01-14 02:11 UTC, Fridolin Somers	Details \| Diff \| Splinter Review
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Victor Grousset/tuxayo 2018-01-23 14:48:21 UTC

When running overdue_notices.pl, one email message is sent to the library containing all the print versions of the related notices.
But since a past enhancement, one message of the type print also is created for each of such notices.
So now we have two print overdue notice generations.
Which is why the email message is not longer relevant as it's superseded by the individual and ready to print messages.

Comment 1 Victor Grousset/tuxayo 2018-01-23 14:52:06 UTC

Created attachment 70846 [details] [review]
Bug 20076: Overdues: remove redundant message when patrons don't have email addresses

When running overdue_notices.pl, one email message is sent to the
library containing all the print versions of the related notices.
But since a past enhancement, one message of the type print also is
created for each of such notices.
So now we have two print overdue notice generations.
Which is why the email message is not longer relevant as it's superseded
by the individual and ready to print messages.

Test plan
1. Check the size of the message queue
     With the following SQL query (using an SQL report if you want)
     SELECT COUNT(*) FROM message_queue;
2. Run misc/cronjobs/overdue_notices.pl
3. Check the size of the message queue
     To ensure that no other overdues will create noise in this test plan.
     Or you can take them into account.
4. Choose a patron with no email address
5. Create an overdue (checkout an item and unfold "Checkout settings"
   and set a date in the past which is compatible with what you find in
   staff:/cgi-bin/koha/tools/overduerules.pl
6. Run misc/cronjobs/overdue_notices.pl
7. Check that you have two new messages in the queue
8. Inspect these two messages
   SELECT * FROM message_queue ORDER BY time_queued DESC LIMIT 2 \G
   1. One has the type "print" and the borrowernumber matching the patron.
   2. The other has
        subject: Overdue Notices
        borrowernumber: NULL
        message_transport_type: email
        and contains "These messages were not sent directly to the patrons."
        This is the one we don't want anymore.
        Because it's now obsolete due to the first message.
9. Apply this patch
10. Run misc/cronjobs/overdue_notices.pl
11. Check that only the print message is now generated and not the
      "Overdue Notices" one.

Comment 2 Victor Grousset/tuxayo 2018-01-23 15:21:11 UTC

This bugzilla and patch might be invalidated by:
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19888#c1

Comment 3 Katrin Fischer 2018-01-30 07:02:59 UTC

Isn't bug 19888 a duplicate?

*** This bug has been marked as a duplicate of bug 19888 ***

Comment 4 Katrin Fischer 2018-01-30 07:03:58 UTC

*** Bug 19888 has been marked as a duplicate of this bug. ***

Comment 5 Katrin Fischer 2018-01-30 07:04:52 UTC

Please create a system preference for this behaviour instead of just silently removing a feature some libraries rely on.

Comment 6 Victor Grousset/tuxayo 2018-02-01 11:09:25 UTC

(In reply to Katrin Fischer from comment #5)
> Please create a system preference for this behaviour instead of just
> silently removing a feature some libraries rely on.

That's the plan. We initially though that the first feature was completely superseded by the second one.

By the way, how to name the "first feature"? (email that groups the prints of user without email addresses)
Because I don't know how to name the future syspref.

Comment 7 Katrin Fischer 2018-02-01 11:54:14 UTC

EmailOverduesNoEmail?

Comment 8 Katrin Fischer 2018-02-01 11:54:23 UTC

Not feeling really creative :)

Comment 9 Arthur Suzuki 2020-01-08 10:54:49 UTC

Hi Katrin,
Bit of followup on my side : maybe easiest to make this parameter a commandline parameter than a syspref?
At least it'll be less of a hassle to maintain.

Comment 10 Katrin Fischer 2020-01-08 12:09:14 UTC

(In reply to Arthur Suzuki from comment #9)
> Hi Katrin,
> Bit of followup on my side : maybe easiest to make this parameter a
> commandline parameter than a syspref?
> At least it'll be less of a hassle to maintain.

I think a cronjob parameter is more hassle than a syspref in maintaining - I need my admins to change that, a pref I can switch ;)

Comment 11 Katrin Fischer 2020-01-08 12:20:53 UTC

(In reply to Katrin Fischer from comment #10)
> (In reply to Arthur Suzuki from comment #9)
> > Hi Katrin,
> > Bit of followup on my side : maybe easiest to make this parameter a
> > commandline parameter than a syspref?
> > At least it'll be less of a hassle to maintain.
> 
> I think a cronjob parameter is more hassle than a syspref in maintaining - I
> need my admins to change that, a pref I can switch ;)

Also: think package installations with instances. If some libraries want the message and others not, I have a lot more maintenance/lines in the job. Currently it works for us with one line for everyone. Preferences mean more flexibility.

Comment 12 Magnus Enger 2020-01-08 12:45:25 UTC

(In reply to Katrin Fischer from comment #11)
> Also: think package installations with instances. If some libraries want the
> message and others not, I have a lot more maintenance/lines in the job.
> Currently it works for us with one line for everyone. Preferences mean more
> flexibility.

I agree wirh Katrin here. Sysprefs give more flexibility, espcially in a multi-tenant setup. It also means libraries that do not have hands on access to their Koha server can change things without relying on their provider to do it for them.

Comment 13 Eric Swenson 2020-10-16 18:16:00 UTC

We recently switched over to community Koha and since we are a library that is hosted by a support provider I would like to also support Katrin's suggestion for this being a system preference that I could go in an toggle. We noticed the duplicate messages as well, so it was exciting to see a bug fix already in the works.

Comment 14 Katrin Fischer 2020-10-17 11:22:05 UTC

(In reply to Eric Swenson from comment #13)
> We recently switched over to community Koha and since we are a library that
> is hosted by a support provider I would like to also support Katrin's
> suggestion for this being a system preference that I could go in an toggle.
> We noticed the duplicate messages as well, so it was exciting to see a bug
> fix already in the works.

Hi Eric, thx for chiming in! Always happy to see libraries comment/interact on bugzilla and welcome to community :)

Comment 15 Thibaud Guillot (thibaud_g) 2021-12-10 15:19:03 UTC Comment hidden (obsolete)

Created attachment 128401 [details] [review]
Add syspref to deal with overdues notification

I took the same test plan as victor but I added the system preference to manage the case more easily, especially for users who do not have access to the koha server.

Test plan
1. Check the size of the message queue
     With the following SQL query (using an SQL report if you want)
     SELECT COUNT(*) FROM message_queue;
2. Run misc/cronjobs/overdue_notices.pl
3. Check the size of the message queue
     To ensure that no other overdues will create noise in this test plan.
     Or you can take them into account.
4. Choose a patron with no email address
5. Create an overdue (checkout an item and unfold "Checkout settings"
   and set a date in the past which is compatible with what you find in
   staff:/cgi-bin/koha/tools/overduerules.pl
6. Run misc/cronjobs/overdue_notices.pl
7. Check that you have two new messages in the queue
8. Inspect these two messages
   SELECT * FROM message_queue ORDER BY time_queued DESC LIMIT 2 \G
   1. One has the type "print" and the borrowernumber matching the patron.
   2. The other has
        subject: Overdue Notices
        borrowernumber: NULL
        message_transport_type: email
        and contains "These messages were not sent directly to the patrons."
        This is the one we don't want anymore.
        Because it's now obsolete due to the first message.
9. Apply this patch
10. Change syspref 'EmailOverduesNoEmail' to "Don't send"
11. Delete data from message_queue (if you have access) for a cleaner view
12. Run again misc/cronjobs/overdue_notices.pl
13. Check that only the print message is now generated and not the
      "Overdue Notices" one.

https://bugs.koha-community.org/show_bug.cgi?id=20074

Comment 16 Thibaud Guillot (thibaud_g) 2021-12-10 15:23:48 UTC Comment hidden (obsolete)

Sorry for the message at the bottom of the test plan, I miss my last attachement with git bz :/

Comment 17 David Nind 2021-12-11 07:10:46 UTC

Patch no longer applies 8-(...

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 20076: Overdues: remove redundant message when patrons don't have email addresses
Using index info to reconstruct a base tree...
M	misc/cronjobs/overdue_notices.pl
Falling back to patching base and 3-way merge...
Auto-merging misc/cronjobs/overdue_notices.pl
CONFLICT (content): Merge conflict in misc/cronjobs/overdue_notices.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 20076: Overdues: remove redundant message when patrons don't have email addresses

Testing notes (using koah-testing-docker):

* To get notices generated on Tools > Patrons and circulation > Overdue notice/status triggers I set the rules for all patron types and the default library to: delay = 1, Letter = Overdue notice, Email = ticked

Comment 18 Thibaud Guillot (thibaud_g) 2021-12-13 08:38:34 UTC

Created attachment 128427 [details] [review]
Add syspref to remove overdues notification by email to staff if user has no email address

I took the same test plan as victor but I added the system preference to manage the case more easily, especially for users who do not have access to the koha server.

Test plan
1. Check the size of the message queue
     With the following SQL query (using an SQL report if you want)
     SELECT COUNT(*) FROM message_queue;
2. Run misc/cronjobs/overdue_notices.pl
3. Check the size of the message queue
     To ensure that no other overdues will create noise in this test plan.
     Or you can take them into account.
4. Choose a patron with no email address
5. Create an overdue (checkout an item and unfold "Checkout settings"
   and set a date in the past which is compatible with what you find in
   staff:/cgi-bin/koha/tools/overduerules.pl
6. Run misc/cronjobs/overdue_notices.pl
7. Check that you have two new messages in the queue
8. Inspect these two messages
   SELECT * FROM message_queue ORDER BY time_queued DESC LIMIT 2 \G
   1. One has the type "print" and the borrowernumber matching the patron.
   2. The other has
        subject: Overdue Notices
        borrowernumber: NULL
        message_transport_type: email
        and contains "These messages were not sent directly to the patrons."
        This is the one we don't want anymore.
        Because it's now obsolete due to the first message.
9. Apply this patch
10. Run updatedabatase.pl
11. Change syspref 'EmailOverduesNoEmail' to "Don't send"
12. Delete data from message_queue (if you have access) for a cleaner view
13. Run again misc/cronjobs/overdue_notices.pl
14. Check that only the print message is now generated and not the
      "Overdue Notices" one.

Comment 19 Thibaud Guillot (thibaud_g) 2021-12-13 08:41:36 UTC

(In reply to David Nind from comment #17)
> Patch no longer applies 8-(...
> 
> Apply? [(y)es, (n)o, (i)nteractive] y
> Applying: Bug 20076: Overdues: remove redundant message when patrons don't
> have email addresses
> Using index info to reconstruct a base tree...
> M	misc/cronjobs/overdue_notices.pl
> Falling back to patching base and 3-way merge...
> Auto-merging misc/cronjobs/overdue_notices.pl
> CONFLICT (content): Merge conflict in misc/cronjobs/overdue_notices.pl
> error: Failed to merge in the changes.
> Patch failed at 0001 Bug 20076: Overdues: remove redundant message when
> patrons don't have email addresses
> 
> Testing notes (using koah-testing-docker):
> 
> * To get notices generated on Tools > Patrons and circulation > Overdue
> notice/status triggers I set the rules for all patron types and the default
> library to: delay = 1, Letter = Overdue notice, Email = ticked

Sorry David I miss up my attachment and some obsolete tag to apply correctly my patch (and also a important step in test plan : updatedabatase.. I'm losing my mind :') )

Now normally everything is going well

Comment 20 David Nind 2021-12-13 09:26:12 UTC

(In reply to Thibaud Guillot from comment #19)

> Sorry David I miss up my attachment and some obsolete tag to apply correctly
> my patch (and also a important step in test plan : updatedabatase.. I'm
> losing my mind :') )
> 
> Now normally everything is going well

No problem! We all have those days...

Comment 21 Victor Grousset/tuxayo 2021-12-13 21:25:05 UTC

> I miss up my attachment and some obsolete tag to apply correctly my patch

Don't worry, some tools take a bit long to get used to. And sometimes we just don't know an easier way.

For the record, here is how I attach two 2 on a ticket.
git-bz attach 20076 HEAD~2..HEAD 
If it's updating an existing patch. This will mark obsolete the old patches based on commit message matching. If the commit message changed, I manually mark the old patch via the UI. I don't know if git-bz is convenient for that.

Comment 22 Thibaud Guillot (thibaud_g) 2021-12-14 08:05:58 UTC

You are understanding, thank you :)

Comment 23 David Nind 2021-12-14 16:24:23 UTC

Created attachment 128515 [details] [review]
Add syspref to remove overdues notification by email to staff if user has no email address

I took the same test plan as victor but I added the system preference to manage the case more easily, especially for users who do not have access to the koha server.

Test plan
1. Check the size of the message queue
     With the following SQL query (using an SQL report if you want)
     SELECT COUNT(*) FROM message_queue;
2. Run misc/cronjobs/overdue_notices.pl
3. Check the size of the message queue
     To ensure that no other overdues will create noise in this test plan.
     Or you can take them into account.
4. Choose a patron with no email address
5. Create an overdue (checkout an item and unfold "Checkout settings"
   and set a date in the past which is compatible with what you find in
   staff:/cgi-bin/koha/tools/overduerules.pl
6. Run misc/cronjobs/overdue_notices.pl
7. Check that you have two new messages in the queue
8. Inspect these two messages
   SELECT * FROM message_queue ORDER BY time_queued DESC LIMIT 2 \G
   1. One has the type "print" and the borrowernumber matching the patron.
   2. The other has
        subject: Overdue Notices
        borrowernumber: NULL
        message_transport_type: email
        and contains "These messages were not sent directly to the patrons."
        This is the one we don't want anymore.
        Because it's now obsolete due to the first message.
9. Apply this patch
10. Run updatedabatase.pl
11. Change syspref 'EmailOverduesNoEmail' to "Don't send"
12. Delete data from message_queue (if you have access) for a cleaner view
13. Run again misc/cronjobs/overdue_notices.pl
14. Check that only the print message is now generated and not the
      "Overdue Notices" one.

https://bugs.koha-community.org/show_bug.cgi?id=20076

Signed-off-by: David Nind <david@davidnind.com>

Comment 24 Katrin Fischer 2021-12-19 13:10:11 UTC

Hi Thibault, 

some small things: 

1) Make sure that you start the subject line of your patch with Bug XXXX: I'll fix this in QA.

2) Database update: We have a new format for the database update, the patch still uses the old one. Please make sure to use the new format in future patches, see:
https://wiki.koha-community.org/wiki/Database_updates

3) I'll rephrase the description of the pref a bit. If you'd like some help for checking something in English, the native speakers on IRC are usually willing to help out quickly.

4) I noticed your patch has your name as "ThibaudGLT". We can map this to your full name if you want, but easiest is always to submit the patches with the name you want to appear on the Dashboard and the release notes already.

Comment 25 Katrin Fischer 2021-12-19 13:19:39 UTC

Created attachment 128712 [details] [review]
Bug 20076: Add syspref to remove overdues notification by email to staff if user has no email address

I took the same test plan as victor but I added the system preference to manage the case more easily, especially for users who do not have access to the koha server.

Test plan
1. Check the size of the message queue
     With the following SQL query (using an SQL report if you want)
     SELECT COUNT(*) FROM message_queue;
2. Run misc/cronjobs/overdue_notices.pl
3. Check the size of the message queue
     To ensure that no other overdues will create noise in this test plan.
     Or you can take them into account.
4. Choose a patron with no email address
5. Create an overdue (checkout an item and unfold "Checkout settings"
   and set a date in the past which is compatible with what you find in
   staff:/cgi-bin/koha/tools/overduerules.pl
6. Run misc/cronjobs/overdue_notices.pl
7. Check that you have two new messages in the queue
8. Inspect these two messages
   SELECT * FROM message_queue ORDER BY time_queued DESC LIMIT 2 \G
   1. One has the type "print" and the borrowernumber matching the patron.
   2. The other has
        subject: Overdue Notices
        borrowernumber: NULL
        message_transport_type: email
        and contains "These messages were not sent directly to the patrons."
        This is the one we don't want anymore.
        Because it's now obsolete due to the first message.
9. Apply this patch
10. Run updatedabatase.pl
11. Change syspref 'EmailOverduesNoEmail' to "Don't send"
12. Delete data from message_queue (if you have access) for a cleaner view
13. Run again misc/cronjobs/overdue_notices.pl
14. Check that only the print message is now generated and not the
      "Overdue Notices" one.

https://bugs.koha-community.org/show_bug.cgi?id=20076

Signed-off-by: David Nind <david@davidnind.com>

Comment 26 Katrin Fischer 2021-12-19 13:19:44 UTC

Created attachment 128713 [details] [review]
Bug 20076: (QA follow-up) Reformat database update to use newer format

Comment 27 Katrin Fischer 2021-12-19 13:19:48 UTC

Created attachment 128714 [details] [review]
Bug 20076: (QA follow-up) Rephrase system preference description sightly

Comment 28 Katrin Fischer 2021-12-19 13:19:53 UTC

Created attachment 128715 [details] [review]
Bug 20076: (QA follow-up) We need to default to 'Send' on update to keep existing behavior

Comment 29 Thibaud Guillot (thibaud_g) 2021-12-20 08:19:38 UTC

(In reply to Katrin Fischer from comment #24)
> Hi Thibault, 
> 
> some small things: 
> 
> 1) Make sure that you start the subject line of your patch with Bug XXXX:
> I'll fix this in QA.
> 
> 2) Database update: We have a new format for the database update, the patch
> still uses the old one. Please make sure to use the new format in future
> patches, see:
> https://wiki.koha-community.org/wiki/Database_updates
> 
> 3) I'll rephrase the description of the pref a bit. If you'd like some help
> for checking something in English, the native speakers on IRC are usually
> willing to help out quickly.
> 
> 4) I noticed your patch has your name as "ThibaudGLT". We can map this to
> your full name if you want, but easiest is always to submit the patches with
> the name you want to appear on the Dashboard and the release notes already.

Hi Katrin,

Thanks a lot for this complete review, sorry for the mistakes, but I understand now, I will keep this in mind for my future patchs.

Comment 30 Katrin Fischer 2021-12-20 08:23:22 UTC

> Hi Katrin,
> 
> Thanks a lot for this complete review, sorry for the mistakes, but I
> understand now, I will keep this in mind for my future patchs.

That was my only intention, hoping to see many more patches from you! :)

Comment 31 Fridolin Somers 2022-01-14 01:37:19 UTC

I've fixed a bit wiki page :
https://wiki.koha-community.org/wiki/Database_updates#How_to_write_an_atomicupdate_file

The perl file must contain "return" and have execute permissions.

Comment 32 Fridolin Somers 2022-01-14 01:53:00 UTC

Created attachment 129458 [details] [review]
Bug 20076: (RM follow-up) Fix default value in installer/data/mysql/mandatory/sysprefs.sql

Previous patch changed default value to 1 in atomicupdate, do the same
in installer/data/mysql/mandatory/sysprefs.sql

Also to be consistant, sets options = NULL instead of '' in atomicupdate

Also removed useless added empty line in /misc/cronjobs/overdue_notices.pl

Comment 33 Fridolin Somers 2022-01-14 02:11:57 UTC

Created attachment 129459 [details] [review]
Bug 20076: (RM follow-up) Fix system preference boolean logic

A Yes/No system preference must use 1 for Yes and 0 for No.
So "Send" for 1/Yes and "Don't send" for 0/No.
We add too much problems with double-negation boolean system preferences (such as dontmerge).

Previous patch changed default value to 1 in atomicupdate, do the same
in installer/data/mysql/mandatory/sysprefs.sql

Also to be consistant, sets options = NULL instead of '' in atomicupdate

Also removed useless added empty line in /misc/cronjobs/overdue_notices.pl

Comment 34 Fridolin Somers 2022-01-14 02:38:17 UTC

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑

Comment 35 Thibaud Guillot (thibaud_g) 2022-01-14 10:18:18 UTC

Thanks for the review Fridolin

Comment 36 Kyle M Hall (khall) 2022-01-14 14:28:47 UTC

Pushed to 21.11.x for 21.11.02

Comment 37 Fridolin Somers 2022-01-15 07:01:52 UTC

(In reply to Thibaud Guillot from comment #35)
> Thanks for the review Fridolin

Your welcome ;)

Please add a nice text in "Text to go in the release notes"

Comment 38 Andrew Fuerste-Henry 2022-01-25 17:42:59 UTC

This really feels like an enhancement rather than a bug fix. I'm hesitant to push a new syspref on a point release. Tentatively not backporting to 21.05, but please ask if needed.

Comment 39 David Nind 2022-01-29 18:08:04 UTC

I've attempted to update the text for the release notes - please amend if this doesn't make sense, is incorrect, or improves it.

I've tried to describe the current behaviour, what is wanted (a choice on whether to generate both notices - depending on a library's process), and the new system preference that enables this.

Comment 40 David Nind 2022-01-29 18:14:24 UTC

I've also changed the bug to an enhancement as per comment 38. It could really be either a bug fix or an enhancement, but adding a new system preference to enable a choice seems more of an enhancement to me.