Bug 10845 - Multi transport types for holds
Summary: Multi transport types for holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 9016
Blocks: 11561 11867 12251
  Show dependency treegraph
 
Reported: 2013-09-09 14:51 UTC by Jonathan Druart
Modified: 2015-12-03 22:03 UTC (History)
9 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 10845: Multi transport types for holds (7.75 KB, patch)
2013-09-09 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10845: Remove the notice code HOLD_PRINT (114.26 KB, patch)
2013-09-09 15:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10845: Multi transport types for holds (7.77 KB, patch)
2014-01-08 15:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10845: Remove the notice code HOLD_PRINT (114.26 KB, patch)
2014-01-08 15:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10845: Preventing duplicate print notices and enforcing a print notice if no notices can be generated (3.11 KB, patch)
2014-01-15 14:39 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10845: Multi transport types for holds (7.76 KB, patch)
2014-01-15 14:44 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10845: Remove the notice code HOLD_PRINT (114.22 KB, patch)
2014-01-15 14:45 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10845: Remove the notice code HOLD_PRINT (114.22 KB, patch)
2014-01-15 14:52 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10845: Remove the notice code HOLD_PRINT (114.30 KB, patch)
2014-01-15 15:02 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10845: Multi transport types for holds (7.84 KB, patch)
2014-01-15 15:03 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 10845: Remove the notice code HOLD_PRINT (114.70 KB, patch)
2014-04-23 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10845: Multi transport types for holds (8.14 KB, patch)
2014-04-23 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10845: Add the MTT in the die message (1.03 KB, patch)
2014-04-23 14:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10845: FIX message_transport_type parameter for Itiva cronjob (1.16 KB, patch)
2014-05-01 13:43 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-09 14:51:32 UTC
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.
Comment 1 Jonathan Druart 2013-09-09 15:15:50 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-09-09 15:15:54 UTC Comment hidden (obsolete)
Comment 3 I'm just a bot 2013-09-18 11:03:10 UTC
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".
Comment 4 Jonathan Druart 2013-09-18 12:38:36 UTC
(In reply to I'm just a bot from comment #3)
Depend on bug 9016
Comment 5 I'm just a bot 2013-09-29 06:09:11 UTC
This bug depends on 9016 which is in status Patch doesn't apply
Comment 6 I'm just a bot 2013-10-27 06:27:13 UTC
Patch applied cleanly, go forth and signoff
Comment 7 Olli-Antti Kivilahti 2014-01-03 13:58:41 UTC
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
Comment 8 Jonathan Druart 2014-01-06 15:28:33 UTC
Patches apply cleanly with patches from bug 9016 applied. I rebased them (conflicts in a UT file). Could you retry please?
Comment 9 Olli-Antti Kivilahti 2014-01-08 13:17:30 UTC
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.
Comment 10 Jonathan Druart 2014-01-08 15:21:52 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-01-08 15:22:15 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2014-01-08 15:22:46 UTC
It should be better now.
Comment 13 Olli-Antti Kivilahti 2014-01-09 08:20:38 UTC
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?
Comment 14 Olli-Antti Kivilahti 2014-01-09 08:44:08 UTC
BTW Could apply this patch now!
Comment 15 Olli-Antti Kivilahti 2014-01-09 08:56:35 UTC
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
Comment 16 Jonathan Druart 2014-01-09 16:29:16 UTC
(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?
Comment 17 Olli-Antti Kivilahti 2014-01-15 14:39:55 UTC Comment hidden (obsolete)
Comment 18 Olli-Antti Kivilahti 2014-01-15 14:44:46 UTC Comment hidden (obsolete)
Comment 19 Olli-Antti Kivilahti 2014-01-15 14:45:27 UTC Comment hidden (obsolete)
Comment 20 Olli-Antti Kivilahti 2014-01-15 14:52:03 UTC Comment hidden (obsolete)
Comment 21 Olli-Antti Kivilahti 2014-01-15 15:02:43 UTC Comment hidden (obsolete)
Comment 22 Olli-Antti Kivilahti 2014-01-15 15:03:09 UTC Comment hidden (obsolete)
Comment 23 Olli-Antti Kivilahti 2014-01-15 15:15:53 UTC
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.
Comment 24 Marcel de Rooy 2014-04-23 12:26:14 UTC
QA: Looking at this one now.
Comment 25 Marcel de Rooy 2014-04-23 14:24:25 UTC
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>
Comment 26 Marcel de Rooy 2014-04-23 14:24:30 UTC
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.
Comment 27 Marcel de Rooy 2014-04-23 14:27:15 UTC
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.
Comment 28 Jonathan Druart 2014-04-23 14:43:19 UTC
(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.
Comment 29 Jonathan Druart 2014-04-23 14:43:37 UTC
(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.
Comment 30 Marcel de Rooy 2014-04-23 14:54:50 UTC
(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.)
Comment 31 Jonathan Druart 2014-04-23 14:57:10 UTC
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.
Comment 32 Jonathan Druart 2014-04-23 14:59:04 UTC
(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!
Comment 33 Jonathan Druart 2014-05-01 13:43:59 UTC
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.
Comment 34 Jonathan Druart 2014-05-01 13:44:32 UTC
Last patch fixes a problem raised by Marcel in bug 11867 comment 9.
Comment 35 Galen Charlton 2014-05-02 21:20:30 UTC
Pushed to master.  Thanks, Jonathan!
Comment 36 Jonathan Druart 2014-05-14 07:15:59 UTC
These patches introduced errors in sql files, see bug 12251.
Comment 37 David Cook 2014-08-05 00:03:21 UTC
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.
Comment 38 Katrin Fischer 2014-08-05 05:14:28 UTC
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.
Comment 39 Olli-Antti Kivilahti 2014-08-07 07:08:24 UTC
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.
Comment 40 Olli-Antti Kivilahti 2014-08-07 07:09:19 UTC
I have seen queued emails just status=fail, rather than be turned into print notifications.
Comment 41 David Cook 2014-08-07 23:32:32 UTC
(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.
Comment 42 Katrin Fischer 2014-08-08 05:46:10 UTC
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.