Created attachment 22752 [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.
Created attachment 22762 [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.
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
Did you check the dependency?
(In reply to Jonathan Druart from comment #4) > Did you check the dependency? Oh no, I did not see it! Sorry
Created attachment 23255 [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.
And it still depends on 11208?
yes, it is just a followup
In fact, it is not a code dependency, more a functionality dependency.
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.
Can we get a sign off on this please?
(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()
Created attachment 27827 [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>
Created attachment 27828 [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>
QA: Looking at this one now.
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.)
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.
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.
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?
(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?
Created attachment 27919 [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.
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>
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>
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.
Pushed to master. Thanks, Jonathan!