Summary: | Overdues email to library for patrons without email should be optional | ||
---|---|---|---|
Product: | Koha | Reporter: | Victor Grousset/tuxayo <victor> |
Component: | Tools | Assignee: | Thibaud Guillot <thibaud.guillot> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | andrewfh, david, fridolin.somers, katrin.fischer, kyle, magnus, sally.healey, sophie.meynieux, tech, thibaud.guillot |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19888 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=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
|
Attachments: |
Bug 20076: Overdues: remove redundant message when patrons don't have email addresses
Add syspref to deal with overdues notification Add syspref to remove overdues notification by email to staff if user has no email address Add syspref to remove overdues notification by email to staff if user has no email address Bug 20076: Add syspref to remove overdues notification by email to staff if user has no email address Bug 20076: (QA follow-up) Reformat database update to use newer format Bug 20076: (QA follow-up) Rephrase system preference description sightly Bug 20076: (QA follow-up) We need to default to 'Send' on update to keep existing behavior Bug 20076: (RM follow-up) Fix default value in installer/data/mysql/mandatory/sysprefs.sql Bug 20076: (RM follow-up) Fix system preference boolean logic |
Description
Victor Grousset/tuxayo
2018-01-23 14:48:21 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. This bugzilla and patch might be invalidated by: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19888#c1 *** Bug 19888 has been marked as a duplicate of this bug. *** Please create a system preference for this behaviour instead of just silently removing a feature some libraries rely on. (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. EmailOverduesNoEmail? Not feeling really creative :) 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. (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 ;) (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. (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. 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. (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 :) 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 Sorry for the message at the bottom of the test plan, I miss my last attachement with git bz :/ 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 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. (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 (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... > 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.
You are understanding, thank you :) 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> 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. 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> Created attachment 128713 [details] [review] Bug 20076: (QA follow-up) Reformat database update to use newer format Created attachment 128714 [details] [review] Bug 20076: (QA follow-up) Rephrase system preference description sightly Created attachment 128715 [details] [review] Bug 20076: (QA follow-up) We need to default to 'Send' on update to keep existing behavior (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.
> 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! :)
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. 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 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 Pushed to master for 22.05, thanks to everybody involved 🦄 Thanks for the review Fridolin Pushed to 21.11.x for 21.11.02 (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" 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. 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. 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. |