Bug 11209 - Multi transport types: same messages are sent for a patron
Summary: Multi transport types: same messages are sent for a patron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 11208
Blocks: 15417 11603
  Show dependency treegraph
 
Reported: 2013-11-06 14:24 UTC by Jonathan Druart
Modified: 2015-12-23 09:02 UTC (History)
6 users (show)

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


Attachments
Bug 11209: Don't sent the same message for the same user the same day (1.84 KB, patch)
2013-11-06 14:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11209: Don't sent the same message for the same user the same day (2.45 KB, patch)
2013-11-07 09:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11209: Check if a notice has been already sent today (1.32 KB, patch)
2013-12-03 11:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11209: Don't sent the same message for the same user the same day (2.50 KB, patch)
2014-05-01 03:39 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11209: Check if a notice has been already sent today (1.37 KB, patch)
2014-05-01 03:39 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11209: A message is duplicate if its content has been already sent (980 bytes, patch)
2014-05-02 14:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11209: Don't sent the same message for the same user the same day (2.60 KB, patch)
2014-05-05 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11209: Check if a notice has been already sent today (1.47 KB, patch)
2014-05-05 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11209: A message is duplicate if its content has been already sent (1.17 KB, patch)
2014-05-05 08:04 UTC, Marcel de Rooy
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-11-06 14:24:34 UTC

    
Comment 1 Jonathan Druart 2013-11-06 14:27:04 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-11-07 09:24:36 UTC Comment hidden (obsolete)
Comment 3 mathieu saby 2013-11-29 22:27:42 UTC
Before applying the patch:

I set 2 items due on the 3rd December for the same patron.
I ran $perl misc/cronjobs/advance_notices.pl -c
I checked the message_queue table : there are 2 messages, one for each item.

So, bug confirmed.

But the patch does not apply.
I got this message
Applying: Bug 11209: Don't sent the same message for the same user the same day
fatal: sha1 information is lacking or useless (C4/Letters.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
....

Mathieu
Comment 4 Jonathan Druart 2013-12-02 12:08:26 UTC
Did you check the dependency?
Comment 5 mathieu saby 2013-12-02 12:28:04 UTC
(In reply to Jonathan Druart from comment #4)
> Did you check the dependency?

Oh no, I did not see it!
Sorry
Comment 6 Jonathan Druart 2013-12-03 11:26:58 UTC Comment hidden (obsolete)
Comment 7 mathieu saby 2013-12-03 11:32:45 UTC
And it still depends on 11208?
Comment 8 Jonathan Druart 2013-12-03 11:48:06 UTC
yes, it is just a followup
Comment 9 Jonathan Druart 2013-12-03 11:50:04 UTC
In fact, it is not a code dependency, more a functionality dependency.
Comment 10 Mark Tompsett 2014-03-16 03:02:27 UTC
I'm curious about the time queued date vs. now(). What if there is a cross midnight situation? Can such a situation happen? I figure if some branches are 12 timezones away from other branchs, it may.
Comment 11 Katrin Fischer 2014-04-29 07:49:10 UTC
Can we get a sign off on this please?
Comment 12 Chris Cormack 2014-05-01 03:34:22 UTC
(In reply to M. Tompsett from comment #10)
> I'm curious about the time queued date vs. now(). What if there is a cross
> midnight situation? Can such a situation happen? I figure if some branches
> are 12 timezones away from other branchs, it may.

The timezone that matters is the timezone the database server is in, that is the one doing the NOW()
Comment 13 Chris Cormack 2014-05-01 03:39:08 UTC Comment hidden (obsolete)
Comment 14 Chris Cormack 2014-05-01 03:39:26 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2014-05-01 07:07:46 UTC
QA: Looking at this one now.
Comment 16 Marcel de Rooy 2014-05-01 07:38:18 UTC
Just an observation (no big deal):
If I have enabled email and sms for a patron, but I have no email address and the SMS driver should fail, I have two messages in the queue that fall back to PREDUE print. In this case you will have print duplicates. (Two runs now create four PREDUE prints.)
Comment 17 Marcel de Rooy 2014-05-01 07:45:28 UTC
Similarly, if I clear the SMS number, sms messages will be sent to the queue each time in status pending. They will fail only if process cronjob has been run.
Comment 18 Katrin Fischer 2014-05-01 07:49:58 UTC
It sounds to me like it would make sense not to try and create the message if the sms number is not sent or make sure to only have one printed mail for a fallback.
Comment 19 Marcel de Rooy 2014-05-01 08:03:32 UTC
I have some doubts on this patch:
It could block lots of sms messages. You write: The sms part for DUE and PREDUE should not contain dynamic parts, only a
standard message. But how do you prevent that (maybe apart from truncating the message to 160 chars)?
If a message for a hold or a due does not contain a reference to the title, is it still useful for a patron at all? I could imagine that you send one message for dues at cronjob time, but what about Hold filled? Wouldn't that be a candidate for sending more messages if applicable?
Comment 20 Jonathan Druart 2014-05-01 08:25:42 UTC
(In reply to M. de Rooy from comment #16)
> If I have enabled email and sms for a patron, but I have no email address
> and the SMS driver should fail, I have two messages in the queue that fall
> back to PREDUE print. In this case you will have print duplicates. (Two runs
> now create four PREDUE prints.)

This has already been raised by Olli, he opened a report, bug 11561.

(In reply to M. de Rooy from comment #17)
> Similarly, if I clear the SMS number, sms messages will be sent to the queue
> each time in status pending. They will fail only if process cronjob has been
> run.

Yes, a test could be added somewhere. But it should not append (yes you can via the UI, but it is a configuration issue).

(In reply to M. de Rooy from comment #19)
> I have some doubts on this patch:
> It could block lots of sms messages. You write: The sms part for DUE and
> PREDUE should not contain dynamic parts, only a
> standard message. But how do you prevent that (maybe apart from truncating
> the message to 160 chars)?
> If a message for a hold or a due does not contain a reference to the title,
> is it still useful for a patron at all? I could imagine that you send one
> message for dues at cronjob time, but what about Hold filled? Wouldn't that
> be a candidate for sending more messages if applicable?

Maybe could I add a condition in the sql query like " AND content = $message->{content}".
Like that, if the template contains dynamic parts, that won't match.
What do you think about that?
Comment 21 Jonathan Druart 2014-05-02 14:56:49 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2014-05-05 08:04:15 UTC
Created attachment 27937 [details] [review]
Bug 11209: Don't sent the same message for the same user the same day

For PREDUE messages, one message is sent to the message_queue table for
each items in advanced.
So the patron will receive the same message.

The sms part for DUE and PREDUE should not contain dynamic parts, only a
standard message.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2014-05-05 08:04:50 UTC
Created attachment 27938 [details] [review]
Bug 11209: Check if a notice has been already sent today

The previous patch checks if a notice has already been sent when the
current notices has been sent in queue. Which is wrong!
We have to check if a similar notice has been sent today.

This patch has been created after an observation on a production server:
If a user place on holds several items, he will receive 1 SMS per hold.
Here we only want 1 SMS for all holds.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2014-05-05 08:04:54 UTC
Created attachment 27939 [details] [review]
Bug 11209: A message is duplicate if its content has been already sent

If the template contains dynamic parts, the message won't be
considerated as duplicated.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Duplicate messages will be queued, but when sending the queued messages
duplicates are found and are marked as failed.
Comment 25 Galen Charlton 2014-05-05 15:17:51 UTC
Pushed to master.  Thanks, Jonathan!