Bug 14133 - Print notices generated in special case do not use print template
Summary: Print notices generated in special case do not use print template
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15967
  Show dependency treegraph
 
Reported: 2015-05-04 11:40 UTC by Sophie MEYNIEUX
Modified: 2016-12-05 21:25 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14133: Print notices should be generated with the print template (3.06 KB, patch)
2015-08-21 10:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14133: Print notices should be generated with the print template (3.07 KB, patch)
2015-12-30 14:46 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 14133: Print notices should be generated with the print template (3.14 KB, patch)
2016-01-01 02:56 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 14133: Print notices should be generated with the print template (3.19 KB, patch)
2016-01-08 18:31 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Sophie MEYNIEUX 2015-05-04 11:40:44 UTC
In http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10832, there are specific cases where print notices are generated (If a user don't have a smsalertnumber and a sms is required or if a user don't have an email defined and an email is required).
But in this case, the template used is not print one, but sms or email.
The pb is that in that case, there is no way to identify the borrower that should receive this message (especially for sms where the message is very brief)
Comment 1 Jonathan Druart 2015-08-20 10:25:02 UTC
Sophie, what do you suggest?
Comment 2 Sophie MEYNIEUX 2015-08-21 09:14:29 UTC
That print messages use print template, even when print is a fallback
Comment 3 Jonathan Druart 2015-08-21 10:06:19 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2015-10-27 15:41:28 UTC
> IMPORTANT NOTE: This test plan does not take into account the notices
> generated for the staff ("These messages were not sent directly to the
> patrons."). However the behavior will also change, the print template
> will be used in all cases. Is it what we want?

The question is: What for the -n parameter is used? Who used it? I personally never uses it.
Comment 5 Katrin Fischer 2015-11-24 10:25:51 UTC
Hm, I think -n might make most send for testing, not for production use. So if you want to see which patrons would be notified and how the notices would look like.

91 
92 =item B<-n>
93 
94 Do not send any email. Overdue notices that would have been sent to
95 the patrons or to the admin are printed to standard out. CSV data (if
96 the -csv flag is set) is written to standard out or to any csv
97 filename given.

Using the print template in the email send to staff for patrons without email addresses etc. makes sense for me.
Comment 6 Sophie MEYNIEUX 2015-12-30 14:46:59 UTC Comment hidden (obsolete)
Comment 7 Sophie MEYNIEUX 2015-12-30 14:47:46 UTC
Patch rebased
Comment 8 Chris Cormack 2016-01-01 02:56:22 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2016-01-08 18:31:16 UTC
Created attachment 46452 [details] [review]
Bug 14133: Print notices should be generated with the print template

If the overdue notice to send is an email or a SMS and the patron does
not provided such information, a print notice is generated.
But it will be generated with the email or sms template, which is not
useful, especially in the SMS case.

The template to use should be the print one.

Test plan:
0/ Does not apply this patch and Correctly define the overdue rules to generate
an overdue notice to the message_queue table.
Check email + SMS
1/ Define an email address and a SMS number for a patron
2/ Generate the overdue notices (misc/cronjobs/overdue_notices.pl [-t])
3/ 2 notices should have been generated:
    mtt=email with the email template used
    mtt=sms   with the sms template used
4/ Truncate the message_queue table
5/ Remove the email address and repeat 2
6/ Only 1 notice should have been generated:
    mtt=print with the email template used
7/ Truncate the message_queue table
8/ Remove the sms alert number and repeat 2
9/ 2 notices should have been generated:
    mtt=print with the email template used (or sms I suppose)

10/ Apply this patch and repeat previous steps.
The print notices should be generated with the print template.

IMPORTANT NOTE: This test plan does not take into account the notices
generated for the staff ("These messages were not sent directly to the
patrons."). However the behavior will also change, the print template
will be used in all cases. Is it what we want?

Signed-off-by: Chris <chris@bigballofwax.co.nz>

Seems fine by me

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

The IMPORTANT NOTE change makes sense to me.
Comment 10 Brendan Gallagher 2016-01-27 02:19:29 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks
Comment 11 Julian Maurice 2016-01-29 13:43:56 UTC
Patch pushed to 3.22.x, will be in 3.22.3
Comment 12 Frédéric Demians 2016-02-12 06:39:02 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.
Comment 13 Nick Clemens 2016-03-03 00:01:40 UTC
I think there is a problem here.

Unless I missed something the lines below will now never be triggered, so the notice will not be pushed to output_chunks, so it will never be sent to the library in the case of a patron with no emails or sms


727                         if ( ($mtt eq 'email' and not scalar @emails_to_use) or ($mtt eq 'sms' and not $data->{smsalertnumber}) ) {
728                             push @output_chunks,
Comment 14 Jonathan Druart 2016-03-03 08:52:17 UTC
(In reply to Nick Clemens from comment #13)
> I think there is a problem here.
> 
> Unless I missed something the lines below will now never be triggered, so
> the notice will not be pushed to output_chunks, so it will never be sent to
> the library in the case of a patron with no emails or sms
> 
> 
> 727                         if ( ($mtt eq 'email' and not scalar
> @emails_to_use) or ($mtt eq 'sms' and not $data->{smsalertnumber}) ) {
> 728                             push @output_chunks,

Haaaaa!!!
Good catch Nick!
This script drives me crazy.
Comment 15 Jonathan Druart 2016-03-03 08:57:51 UTC
See bug 15967.
Comment 16 Katrin Fischer 2016-04-04 10:10:15 UTC
Note: Jonathan pointed out that this changed behaviour, but I am worried the true consequence got lost. It looks like if you currently have no overdue print template defined, now there will be NO NOTICE generated to the library at all if the user has no email address, while before this worked using the email template.

I'd suggest having a fallback again... if there is a print template, use it. If not, use the email one. Otherwise this behaviour change could create long unnoticed problems for libraries not receiving the emails as they are used to.

The other idea would be automatically populating the print notice templates - but it seems more complicated. Should we do this only for overdue notices (looking at the triggers?) or for other notices as well?
Comment 17 Nick Clemens 2016-04-04 13:32:04 UTC
(In reply to Katrin Fischer from comment #16)
> I'd suggest having a fallback again... if there is a print template, use it.
> If not, use the email one. 

Maybe we could fallback to just sending the library a notice saying "No email for patron XXX - define print notice if you would like to receive printable notices instead of this message"?  Or something along those lines?
Comment 18 Katrin Fischer 2016-04-04 13:34:17 UTC
I think your idea is a good third option. Jonathan has proposed a new patch on bug 15967 with the fallback solution. Maybe we should move discussion there.