Bug 10832 - Multi transport types for overdue notices
Summary: Multi transport types for overdue notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 9016
Blocks: 11603 11696 12717
  Show dependency treegraph
 
Reported: 2013-09-06 09:09 UTC by Jonathan Druart
Modified: 2016-12-05 21:22 UTC (History)
7 users (show)

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


Attachments
Bug 10832: Multi transport types for overdue notices (12.34 KB, patch)
2013-09-06 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: Multi transport types for overdue notices (12.17 KB, patch)
2013-09-06 09:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: Multi transport types for overdue notices (12.16 KB, patch)
2013-09-06 09:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: Multi transport types for overdue notices (11.79 KB, patch)
2013-11-08 12:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: Multi transport types for overdue notices (11.85 KB, patch)
2014-01-03 12:44 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10832: FIX Some print notices may not be sent (1.53 KB, patch)
2014-02-13 13:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: FIX Some print notices may not be sent (1.59 KB, patch)
2014-02-13 14:24 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10832: Multi transport types for overdue notices (11.92 KB, patch)
2014-04-23 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: FIX Some print notices may not be sent (1.59 KB, patch)
2014-04-23 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10832: Multi transport types for overdue notices (12.01 KB, patch)
2014-04-23 15:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10832: FIX Some print notices may not be sent (1.69 KB, patch)
2014-04-23 15:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10832: Add the MTT in the warn message (1.14 KB, patch)
2014-04-23 15:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-06 09:09:34 UTC
Bug 9016 adds a UI for choosing transport types for overdues.
The cronjob script misc/cronjobs/overdue_notices.pl should deal with this configuration.
Comment 1 Jonathan Druart 2013-09-06 09:37:40 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-09-06 09:39:07 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-09-06 09:46:05 UTC Comment hidden (obsolete)
Comment 4 I'm just a bot 2013-09-29 05:52:55 UTC
Applying: Bug 9016: DB changes: new table overduerules_transport_types
Using index info to reconstruct a base tree...
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
Patch failed at 0001 Bug 9016: DB changes: new table overduerules_transport_types
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 5 I'm just a bot 2013-09-29 05:53:09 UTC
This bug depends on bug9016 which is in status Needs Signoff but the patches for it do not apply cleanly
Comment 6 Jonathan Druart 2013-09-30 07:48:00 UTC
(In reply to I'm just a bot from comment #5)
> This bug depends on bug9016 which is in status Needs Signoff but the patches
> for it do not apply cleanly

bug 9016 rebased.
Comment 7 I'm just a bot 2013-10-27 06:33:19 UTC
Patch applied cleanly, go forth and signoff
Comment 8 Olli-Antti Kivilahti 2013-11-08 10:14:53 UTC
-----------------------------------------------------------------------------
--  "git bz applying 10832" on top of "git bz apply all commits for 9016"  --
-----------------------------------------------------------------------------

Auto-merging misc/cronjobs/overdue_notices.pl
CONFLICT (content): Merge conflict in misc/cronjobs/overdue_notices.pl

Manually resolved the conflict.


--------------------------
--  overdue_notices.pl  --
--------------------------

overdue_notices.pl properly queues letters to koha.message_queue but the parse_letter() doesn't parse any letter placeholders <<branch.branchname>> atleast for email and sms transport type. Unsure if this is because of my merge. Placeholders work when triggering notifications from check-in/check-out for ex.
Only tested the main path, unsure how to trigger secondary execution paths.
Test plan works for the core test case.

Yet to test the specific case, but based on my code review it should work :)


Otherwise good work in such a complex application area.


-------------------------
--  Failed QA because  --
-------------------------

Merge conflict needs to be resolved.
Placeholders do not work for some reason.
Comment 9 Jonathan Druart 2013-11-08 12:52:34 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-11-08 12:53:46 UTC
(In reply to Olli-Antti Kivilahti from comment #8)

> overdue_notices.pl properly queues letters to koha.message_queue but the
> parse_letter() doesn't parse any letter placeholders <<branch.branchname>>
> atleast for email and sms transport type. Unsure if this is because of my
> merge. Placeholders work when triggering notifications from
> check-in/check-out for ex.

The placeholder is not branch.branchname but branches.branchname :)

> Merge conflict needs to be resolved.

Conflict resolved.
Comment 11 Olli-Antti Kivilahti 2014-01-03 12:44:42 UTC Comment hidden (obsolete)
Comment 12 Olli-Antti Kivilahti 2014-02-06 10:17:16 UTC
Just implementing our print message processing and found a nasty bug.
I am sending overdue notifications for each letter 1,2,3 via email,print,sms.
I have different template configured per transport_type.
If a patron doesn't have email configured, instead of getting three messages in the message_queue with their respective letter-templates used, I get only sms and print(using the email-template)

So the print is sent using the email template if email address is missing, instead of the print template. This is a big issue since the print templates often follow a very specific eLetter format and must use the proper transport type template.
Comment 13 Jonathan Druart 2014-02-06 10:56:35 UTC
I just submitted a patch on bug 11696 for this issue.
Comment 14 Olli-Antti Kivilahti 2014-02-13 12:41:05 UTC
Yet another bug in overdue_notices.pl.
For the overdue letter 2, if there are more than one borrower receiving a overdueletter2 from the same branch via $mtt = print.
Only the firstly handled borrower gets his overdue letter, 
other print letters for the same branch and borrowertype and overdueletternumber are skipped because $print_sent == 1.
overdue_notices.pl: 627 =>    $print_sent = 1 if $mtt eq 'print';
is the culprit, but
overdue_notices.pl: 438 => my $print_sent = 0; # We never sent a print notice
is instantiated too early, preventing iteration of extra borrowers.
Comment 15 Jonathan Druart 2014-02-13 13:58:19 UTC Comment hidden (obsolete)
Comment 16 Olli-Antti Kivilahti 2014-02-13 14:24:09 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2014-02-20 09:41:25 UTC
Olli-Antti, should this be signed off?
Comment 18 Olli-Antti Kivilahti 2014-02-20 13:00:21 UTC
oki
Comment 19 Marcel de Rooy 2014-04-23 12:23:26 UTC
Does not apply anymore on top of 9016. Please rebase.
Comment 20 Jonathan Druart 2014-04-23 13:00:35 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2014-04-23 13:00:40 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2014-04-23 13:01:32 UTC
Last patches fix conflict with bug 11598.
Comment 23 Marcel de Rooy 2014-04-23 14:39:27 UTC
QA: Looking at this one now.
Comment 24 Marcel de Rooy 2014-04-23 15:23:37 UTC
Created attachment 27496 [details] [review]
Bug 10832: Multi transport types for overdue notices

Test plan:
- define some complex overdue rules (tools/overduerules.pl).
For example:
First overdue from 2 to 5 days by sms and email with letter code L1
Second overdue from 5 to 15 days by email with letter code L2
Third overdue from 15 days by print with letter code L3
- defined a message for each transport type selected (tools/letters.pl).
- select 3 patrons (P1, P2, P3) and 3 barcodes (B1, B2, B3).
    * checkout B1 to P1 with a due date = NOW + 3 days
    * checkout B2 to P2 with a due date = NOW + 10 days
    * checkout B3 to P3 with a due date = NOW + 20 days
- into the mysql cli, note the value of unsent message:
  select count(*) from message_queue where status != "send";
- launch the cronjob:
  perl misc/cronjobs/overdue_notices.pl
- retry the previous sql query, you should have X + 4 unsent messages
  (depending of current checkouts in your DB!).
- view all unsent message:
  select borrowernumber, letter_code, message_transport_type, content
  from message_queue where status != "send";
  You should see:
    2 messages for P1, 1 for sms, 1 for email and the letter code L1
    1 message for P2, 1 for email and the letter code L2
    1 message for P3, 1 for print and the letter code L3

- Specific case: 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, a print notice is generated.
  A print notice is generated only 1 time per borrower and per level.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2014-04-23 15:23:45 UTC
Created attachment 27497 [details] [review]
Bug 10832: FIX Some print notices may not be sent

If overdues should be sent to some patron (on the same branch), only the
first one was notified.

This patch fixes this issue.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2014-04-23 15:27:44 UTC
No circulation PREDUE letter at /usr/share/koha/testclone/C4/Letters.pm line 455
(Similar remark as on report 10845. Listing the mtt would be helpful too.)

Passed QA
Comment 27 Jonathan Druart 2014-04-23 15:36:57 UTC
Created attachment 27498 [details] [review]
Bug 10832: Add the MTT in the warn message

If no template is defined for a letter and the needed MTT, we
should display the MTT.
Comment 28 Galen Charlton 2014-05-02 21:21:07 UTC
Pushed to master.  Thanks, Jonathan!
Comment 29 Katrin Fischer 2014-08-04 15:15:38 UTC
Hi, I have a (late) question about this development:

>- Specific case: 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, a print notice is generated.
>  A print notice is generated only 1 time per borrower and per level.

Before the MTT patches were pushed there as an email send to the library containing all notices for patrons without email address. Is this still possible now? Is some additional configuration required?

I have tested this and I can see the notices show up as 'print' in my message_queue. I think this fallback cannot be avoided, as I haven't set up any notices for print nor checked the print checkbox in notice triggers.
Comment 30 Jonathan Druart 2014-08-19 10:01:16 UTC
(In reply to Katrin Fischer from comment #29)
> Hi, I have a (late) question about this development:
> 
> >- Specific case: 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, a print notice is generated.
> >  A print notice is generated only 1 time per borrower and per level.
> 
> Before the MTT patches were pushed there as an email send to the library
> containing all notices for patrons without email address. Is this still
> possible now? Is some additional configuration required?
> 
> I have tested this and I can see the notices show up as 'print' in my
> message_queue. I think this fallback cannot be avoided, as I haven't set up
> any notices for print nor checked the print checkbox in notice triggers.

From the commit message:
- Specific case: 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, a print notice is generated.

I can provide a patch if everyone agrees on how we should manage this case.
Have a look at the wiki page (http://wiki.koha-community.org/wiki/Message_transport_types), we (Olli and me) explained the different behaviors.
Comment 31 Olli-Antti Kivilahti 2014-08-19 10:29:09 UTC
We could always catch failed sms'es with a report?
Then manually deal with those cases?

Or have an extra feature to handle failed message deliveries. Resend/Change MTT/Delete...
Comment 32 Jonathan Druart 2014-08-19 10:40:17 UTC
(In reply to Jonathan Druart from comment #30)

Whaou, this comment was quite stupid, I quoted what you already quoted...
Comment 33 Jonathan Druart 2014-08-19 10:46:05 UTC
(In reply to Katrin Fischer from comment #29)
> Before the MTT patches were pushed there as an email send to the library
> containing all notices for patrons without email address. Is this still
> possible now? Is some additional configuration required?
> 
> I have tested this and I can see the notices show up as 'print' in my
> message_queue. I think this fallback cannot be avoided, as I haven't set up
> any notices for print nor checked the print checkbox in notice triggers.

I think what you want is to call the gather_print_notices cronjob with the letter_code and the email params (introduced by bug 11603 and/or bug 11678).
Like that, you can sent the print notices to 1+ email.
Comment 34 delaye 2015-08-20 10:13:39 UTC
In 3.18

 -> if a user don't have an email defined and an email is
  required and print is not required, a print notice is generated with the content of email.

-> if a user don't have an email defined and an email is
  required and print is required, a print notice is generated with the content of print.

Print required ou not not required , we must have a print notice with content of print not content of email with a message_transport_type=print....
Comment 35 Jonathan Druart 2015-08-20 10:24:52 UTC
(In reply to delaye from comment #34)
> In 3.18
> 
>  -> if a user don't have an email defined and an email is
>   required and print is not required, a print notice is generated with the
> content of email.
> 
> -> if a user don't have an email defined and an email is
>   required and print is required, a print notice is generated with the
> content of print.
> 
> Print required ou not not required , we must have a print notice with
> content of print not content of email with a message_transport_type=print....

It looks the same as bug 14133.