Bug 9016 adds a UI for creating a message for each transport types for a letter. The patron messaging preferences allow to select one or more transport types to notify when an item is available for pick-up. In order to be consistent, this should be use the notice defined.
Created attachment 20917 [details] [review] Bug 10845: Multi transport types for holds The HOLD_PRINT and HOLD_PHONE notices become useless. This patch will modify existing notices in order to group them into the main notices 'HOLD'. Like that, on one screen, all these notices can be edited. Test plan: - apply the patch and execute the update database entry. - verify that yours previous HOLD_PHONE and HOLD_PRINT are displayed when editing the HOLD notice (under phone and print). - choose a patron and check sms, email, phone for "Hold filled" (on the patron messaging preferences). - place a hold. - check the item in and confirm the hold. - if the patron has an email *and* a sms number, 2 new messages are put into the sql message_queue table: 1 sms and 1 email. if the patron does not have 1 of them, there are 2 new messages: 1 sms/email and 1 print. if the user has neither of them, there is 1 new message: 1 print. - the generated messages should correspond with the notices defined, depending the message transport type.
Created attachment 20918 [details] [review] Bug 10845: Remove the notice code HOLD_PRINT The HOLD_PRINT notice code is the HOLD notice code with a message transport type equal to 'print'.
Applying: Bug 10845: Multi transport types for holds Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 10845: Multi transport types for holds 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".
(In reply to I'm just a bot from comment #3) Depend on bug 9016
This bug depends on 9016 which is in status Patch doesn't apply
Patch applied cleanly, go forth and signoff
Sorry for not getting back at these earlier, but here's one more for you! Applying: Bug 10845: Multi transport types for holds fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 10845: Multi transport types for holds
Patches apply cleanly with patches from bug 9016 applied. I rebased them (conflicts in a UT file). Could you retry please?
I am sorry, but I still get the same error. Applying: Bug 10845: Multi transport types for holds fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 10845: Multi transport types for holds This happens if I apply 10845 directly from 9016 or when I apply the whole feature set with 9016, 10832, 10833, 10845.
Created attachment 24012 [details] [review] Bug 10845: Multi transport types for holds The HOLD_PRINT and HOLD_PHONE notices become useless. This patch will modify existing notices in order to group them into the main notices 'HOLD'. Like that, on one screen, all these notices can be edited. Test plan: - apply the patch and execute the update database entry. - verify that yours previous HOLD_PHONE and HOLD_PRINT are displayed when editing the HOLD notice (under phone and print). - choose a patron and check sms, email, phone for "Hold filled" (on the patron messaging preferences). - place a hold. - check the item in and confirm the hold. - if the patron has an email *and* a sms number, 2 new messages are put into the sql message_queue table: 1 sms and 1 email. if the patron does not have 1 of them, there are 2 new messages: 1 sms/email and 1 print. if the user has neither of them, there is 1 new message: 1 print. - the generated messages should correspond with the notices defined, depending the message transport type.
Created attachment 24013 [details] [review] Bug 10845: Remove the notice code HOLD_PRINT The HOLD_PRINT notice code is the HOLD notice code with a message transport type equal to 'print'.
It should be better now.
Reading your test cases, and good job with the description! There is one concern! What if a patron defines to his own messaging preferences sms and email, but has no sms notification number defined nor email. This can easily be done accidentally. This might needlessly bloat our print notifications queue and cause lots of extra costs. Would it be possible to add a check that; if email or sms is succesfully sent, even if the other email or sms has failed, no print notice is generated. This is slightly problematic to implement but shouldn't be too difficult. Also a patron can request email and sms, buth have no such contact details. In this case the email is sent via print notice + the sms replaced with another print notice! In addition there should be a check in place to enforce that a patron cannot set his messaging preferences if sms or email is missing, but that is not in this bug's scope. Or is it the only way to send HOLD notifications via print? What if our patrons don't have sms or email?
BTW Could apply this patch now!
If I have had email and sms defined, but subsequently removed those contact details, I suspect this condition brings falsehood: if ( ($mtt eq 'email' and not $to_address) or ($mtt eq 'sms' and not $borrower->{smsalertnumber}) ) { $to_address and $borrower->{smsalertnumber} are defined as empty Strings and validate true in Perl. Thus I have been unable to trigger the double print notification bug, but instead got sms and email notifications with destination of '' == empty String
(In reply to Olli-Antti Kivilahti from comment #13) > Reading your test cases, and good job with the description! > > There is one concern! > What if a patron defines to his own messaging preferences sms and email, but > has no sms notification number defined nor email. > This can easily be done accidentally. > This might needlessly bloat our print notifications queue and cause lots of > extra costs. If no sms and email are defined, a print notice will be generated. Feel free to fix the preferences or not to sent the notification :) What do you propose ? > Would it be possible to add a check that; > if email or sms is succesfully sent, even if the other email or sms has > failed, no print notice is generated. This is slightly problematic to > implement but shouldn't be too difficult. The behavior I developed is the behavior needed by our customer. Maybe a syspref could be let the choice, but it is out of the scope of this patch. > Also a patron can request email and sms, buth have no such contact details. > In this case the email is sent via print notice + the sms replaced with > another print notice! No, normally only 1 print notice is generated. (In reply to Olli-Antti Kivilahti from comment #15) > If I have had email and sms defined, but subsequently removed those contact > details, I suspect this condition brings falsehood: > > if ( ($mtt eq 'email' and not $to_address) or ($mtt eq 'sms' and not > $borrower->{smsalertnumber}) ) { > > $to_address and $borrower->{smsalertnumber} are defined as empty Strings and > validate true in Perl. > > Thus I have been unable to trigger the double print notification bug, but > instead got sms and email notifications with destination of '' == empty > String 1 sms and 1 email have been generated with and empty to_address?
Created attachment 24398 [details] [review] Bug 10845: Preventing duplicate print notices and enforcing a print notice if no notices can be generated ------------------------- - REPLICATING THE ISSUE - ------------------------- 1. Set a Patrons "Hold filled"-messaging preference to SMS + Email 2. Remove the SMS number (sms notification number) and all email addresses. 3. Make a reservation for this Patron. 4. Check-in the reserved Item. 5. message_queue-table has two generated print notices for the Hold_filled event. One for both failed message transport types, email and sms. 1. Set a Patrons "Hold filled"-messaging preference to empty, remove all checks from boxes. 2. Make a reservation for this Patron 3. Check-in the reserved Item. 4. message_queue-table has no message for the Hold-filled event. This is problematic because a Patron should get somekind of a notification for a filled Hold. ----------------------------- - AFTER APPLYING THIS PATCH - ----------------------------- If all message transport types for "Hold filled" fail, a print notice is queued in the message_queue-table. Only one print message is queued even if many transports attempts fail.
Created attachment 24399 [details] [review] Bug 10845: Multi transport types for holds The HOLD_PRINT and HOLD_PHONE notices become useless. This patch will modify existing notices in order to group them into the main notices 'HOLD'. Like that, on one screen, all these notices can be edited. Test plan: - apply the patch and execute the update database entry. - verify that yours previous HOLD_PHONE and HOLD_PRINT are displayed when editing the HOLD notice (under phone and print). - choose a patron and check sms, email, phone for "Hold filled" (on the patron messaging preferences). - place a hold. - check the item in and confirm the hold. - if the patron has an email *and* a sms number, 2 new messages are put into the sql message_queue table: 1 sms and 1 email. if the patron does not have 1 of them, there are 2 new messages: 1 sms/email and 1 print. if the user has neither of them, there is 1 new message: 1 print. - the generated messages should correspond with the notices defined, depending the message transport type.
Created attachment 24400 [details] [review] Bug 10845: Remove the notice code HOLD_PRINT The HOLD_PRINT notice code is the HOLD notice code with a message transport type equal to 'print'.
Created attachment 24401 [details] [review] Bug 10845: Remove the notice code HOLD_PRINT The HOLD_PRINT notice code is the HOLD notice code with a message transport type equal to 'print'.
Created attachment 24405 [details] [review] [SIGNED-OFF] Bug 10845: Remove the notice code HOLD_PRINT The HOLD_PRINT notice code is the HOLD notice code with a message transport type equal to 'print'. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Created attachment 24406 [details] [review] [SIGNED-OFF] Bug 10845: Multi transport types for holds The HOLD_PRINT and HOLD_PHONE notices become useless. This patch will modify existing notices in order to group them into the main notices 'HOLD'. Like that, on one screen, all these notices can be edited. Test plan: - apply the patch and execute the update database entry. - verify that yours previous HOLD_PHONE and HOLD_PRINT are displayed when editing the HOLD notice (under phone and print). - choose a patron and check sms, email, phone for "Hold filled" (on the patron messaging preferences). - place a hold. - check the item in and confirm the hold. - if the patron has an email *and* a sms number, 2 new messages are put into the sql message_queue table: 1 sms and 1 email. if the patron does not have 1 of them, there are 2 new messages: 1 sms/email and 1 print. if the user has neither of them, there is 1 new message: 1 print. - the generated messages should correspond with the notices defined, depending the message transport type. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
There is a problem with this patch. On some occasions it is possible for a Patron to receive two print notices, if both email and sms notifications are enabled for "Hold filled" -event, but are not set in the Patron's details. Also if no message transport types are defined for "Hold filled", the patron gets no notification whatsoever about his hold status. Joubu promised to add a print checkbox to the Patron messaging preferences, thus alleviating this issue a bit, but making it possible for a patron to receive 3 print notices about this same event. One solution for this behaviour is in bug 11561.
QA: Looking at this one now.
Created attachment 27490 [details] [review] Bug 10845: Remove the notice code HOLD_PRINT The HOLD_PRINT notice code is the HOLD notice code with a message transport type equal to 'print'. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 27491 [details] [review] Bug 10845: Multi transport types for holds The HOLD_PRINT and HOLD_PHONE notices become useless. This patch will modify existing notices in order to group them into the main notices 'HOLD'. Like that, on one screen, all these notices can be edited. Test plan: - apply the patch and execute the update database entry. - verify that yours previous HOLD_PHONE and HOLD_PRINT are displayed when editing the HOLD notice (under phone and print). - choose a patron and check sms, email, phone for "Hold filled" (on the patron messaging preferences). - place a hold. - check the item in and confirm the hold. - if the patron has an email *and* a sms number, 2 new messages are put into the sql message_queue table: 1 sms and 1 email. if the patron does not have 1 of them, there are 2 new messages: 1 sms/email and 1 print. if the user has neither of them, there is 1 new message: 1 print. - the generated messages should correspond with the notices defined, depending the message transport type. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Just noting that if email and sms are disabled in the msg prefs, the user will not have a print message. And if the sms driver fails, the record status in message_queue is 'failed', but staff may not be aware of that.
Yet another idea for a follow-up: If you did not fill the text for sms, you will have an error: Could not find a letter called 'HOLD' in the 'reserves' module at C4/Reserves.pm line 1932. It would be helpful to include here the message transport type.
(In reply to M. de Rooy from comment #26) > Just noting that if email and sms are disabled in the msg prefs, the user > will not have a print message. I confirm it works for me. I filled the pref smsdriver, removed email et smsalertnumber for a user A. Checkout an item for user B and place an hold on the item for user A. I checked the item in and a HOLD print is generated in the message_queue.
(In reply to M. de Rooy from comment #27) > Yet another idea for a follow-up: > If you did not fill the text for sms, you will have an error: > Could not find a letter called 'HOLD' in the 'reserves' module at > C4/Reserves.pm line 1932. > > It would be helpful to include here the message transport type. I confirm it's look like a bug.
(In reply to Jonathan Druart from comment #28) > (In reply to M. de Rooy from comment #26) > > Just noting that if email and sms are disabled in the msg prefs, the user > > will not have a print message. > > I confirm it works for me. > I filled the pref smsdriver, removed email et smsalertnumber for a user A. > Checkout an item for user B and place an hold on the item for user A. > I checked the item in and a HOLD print is generated in the message_queue. If you disable email and sms in msg prefs and you have no sms number, you will have no print. If you leave sms enabled but without sms number, you do have a print notice. (Have just mentioned it for completeness on the signoff line.)
Created attachment 27493 [details] [review] Bug 10845: Add the MTT in the die message If no template is defined for the HOLD letter and the needed MTT, we should display the MTT.
(In reply to M. de Rooy from comment #30) > If you disable email and sms in msg prefs and you have no sms number, you > will have no print. If you leave sms enabled but without sms number, you do > have a print notice. (Have just mentioned it for completeness on the signoff > line.) Ok sorry, I misunderstood! Thank you for qaing these (and others!) patches!
Created attachment 27874 [details] [review] Bug 10845: FIX message_transport_type parameter for Itiva cronjob The message_transport_type param should passed to GetPreparedLetter, not part of the "tables" parameter.
Last patch fixes a problem raised by Marcel in bug 11867 comment 9.
Pushed to master. Thanks, Jonathan!
These patches introduced errors in sql files, see bug 12251.
Quick question... so "print" notices are put into the message_queue? How are they sent to borrowers? Are these print notices sent as individual to Koha branches/admin? Collected together and sent as one email? In 3.14.5, I noticed that HOLD_PRINT messages were just languishing forever in the message_queue, as there was no way for them to be sent to patrons. I think there's a cronjob that will store them as HTML files on the server, but that doesn't seem helpful either.
The HOLD_PRINT notices were always relying on cronjobs on the server to create files. I think there was never a feature to send them to the library. Most libraries I know don't send them out if the patron has no email address unlike overdues.
I run cronjob generate_print_notices.pl and store the print notices to a directory. Then web server is configured to show index for that directory, so our librarians can find the daily print messages.
I have seen queued emails just status=fail, rather than be turned into print notifications.
(In reply to Olli-Antti Kivilahti from comment #39) > I run cronjob generate_print_notices.pl and store the print notices to a > directory. Then web server is configured to show index for that directory, > so our librarians can find the daily print messages. Doesn't that seem like an inelegant solution though? How many librarians are actually going to think to look there? Do notices just accumulate overtime until someone arbitrarily decides to remove a backlog of print notices from the directory? Even if we do use a cronjob and a directory, it would be a good idea to at least have an interface for giving librarians access via Koha.
Hm, I think one problem is that we need to provide a way to access and download the files that requires authentication with your staff account.