Unwanted mail blast sent by Scheduled Reminders

changed the description

Related PR is https://github.com/civicrm/civicrm-core/pull/10255
FOR BUG1 I have 2 suggestions:

**SUGGESTION 1:**
Make function prepareStartDateClauses in Civi/ActionSchedule/RecipientBuilder.php more solid
A. FOR A REMINDER SCHEDULED BEFORE EVENT
MAKESURE: do not send anything AFTER the event!
before event start date → if eventstartdate > now = do not send
before event end date → if eventenddate > now = do not send
B. FOR A REMINDER SCHEDULED AFTER EVENT
MAKESURE: do not send anything BEFORE the event!
after event start date → if eventstartdate < now = do not send
after event end date → if eventenddate < now = do not send

SUGGESTION 2
Another suggestion would be to add a relative date filter when selecting 'event type'
Example parameters:

Event Type = 'annual reunion'
Period: 'last 12 months' or 'this fiscal year' (new parameter)
When: 12 weeks before event startdate In this way it is very clear how the reminder is configured and it prevents spam like in our case was sent

SUGGESTION 3
Introduce a new parameter:
CHECKBOX: send only on specified date; do not ‘catch up’
This will make it possible to configure several reminders before the event, but only sent the reminders from the date a person registered. EXAMPLE: Event is scheduled July 21st scheduled reminders 1: 6 months before event startdate (januari 21st) scheduled reminders 2: 4 months before event startdate (march 21st) scheduled reminders 3: 2 months before event startdate (may 21st) scheduled reminders 4: 1 months before event startdate (june 21st) Right now when someone would register in April, he would also receive reminder #1 (closed) & #299 (closed)

SUGGESTION 4
When editing a repeating reminder it is already possible to configure a value for 'until'. This 'until' parameter would also be good to have for 'after event end date' reminders. EXAMPLE:

send reminder 2 weeks after event end date
until 3 months after the event This will send the mail 2 weeks after the end date but will not 'catch up' any later than 3 months. This will prevent mail blasts especially for all older events with reminders after it.

I hope these suggestions help. I think there are a few serious bugs right now in the scheduled reminders that make it unreliable and harmful for both the reputation of our organization and for CiviCRM as a platform. I hope resources can be directed to this issue. I am more than happy to help test and spend my time to help towards a solution.

changed the description

Agree this stuff is nasty. Unfortunately it's also time consuming to dig into & needs tests.

added needs:concept approval triaged type:request labels

ping @jitendra @JoeMurray in case you have some bandwidth for this

Yes, nasty, important and complicated both in terms of getting agreement on requirements and then doing the coding.

I think it would be good to break this up and take some starter steps. Beginning by adding tests is a great start. However, I'm not sure I agree with the first suggestion, at least as it is worded. Yes, only a careful, well-informed administrator would realize that an email blast will go out if it is configured for a date long in the past before an event, due to them always being sent whenever a trigger event is in the past for a cron. This is a user experience bug that might be best solved by better explanation, or a warning popup, rather than introducing a new complication on how scheduled mailings get sent.

For example, I am not sure that it is true that in all cases when a scheduled reminder is setup to start sending before an event that it should not be sent if the first cron run to notice it is after the event start (or ended). I can imagine use cases and configurations where this is not true even if in the general case of the email being a reminder to attend one would not want it sent after the event is over. For example, it might be an invitation to become a member, or to signup for a different event. The cron run might be daily, etc.

Would it have helped to have a warning at the time of configuration that one or more mailings would go out immediately?

Perhaps an extension or a setting might help, but I'd prefer discussion first.

Bugs 2 and 3 should have tests added if they are not there. If @jitendra does one then @monish.deb could do the other.

I tried to replicate this on my setup and so far it seems to be working fine for me. From what I read above, If I create an event reminder to send "12 weeks before event start date" - prepareStartDateClause makes sure to only include recipients which satisfy the following conditions -

Array
(
  [0] => '!casNow' >= DATE_SUB(!casDateField, INTERVAL 12 week)
  [1] => DATE_SUB(!casNow, INTERVAL 1 DAY ) <= !casDateField
)

which means, For event having start date = 21 July (and let say of the reminder job is executed today) the job will only include those participant contacts which are registered to the event having the following conditions met -

current time should be greater than (21 July - 12 weeks). (Satisfied )
current time - 1 day (yesterday) should be less than 21 July. (Not Satisfied)

The second condition is not met and hence this event participants are not considered in the recipient list.

We also have a unit test checking similar behaviour which asserts the recipient retrieving email on different date intervals.

@magnolia61 Do you think there is a miss in my above testing?

Hi Jitendra, I see the condition prepareStartDateClause uses should be ok. The mails should not have been send. But they have.

Could it have something to do with the fact it was a disabled scheduled reminder that re-enabled?
Could it be there was some magic happening by which the recipients were not 'recalculated' against the current conditions?
I will try to replicate this.

I'm jumping in here @magnolia61 re-affirming this is a serious issue. But I see this use case as more simple than perhaps others do.

I think there's a valid case that there should be no emailing to past events prior to the timeframe set in the reminder itself. For example if it is Oct 8 and your newly-created reminder says "2 days after event" only an event on Oct 6 or future will get reminders - no exceptions.

Any other behavior IMHO is neither documented and nor intuitive.

If you are sending to people who have come to events in the past, that is what CiviMail is for, not Scheduled Reminders, which are for automating emails of events that are being newly created or don't yet exist.

In other words creating "do not send to events prior to day X" as a new and separate configuration is not necessary.

Thoughts?

@eileen and @JoeMurray I'm chiming in on this issue because it's an important one. We can't blast X past events a reminder without warning, and I don't think Jitendra's "second condition" solution may even be necessary given the use case of reminders. What can I do to help move forward a patch/solution to this issue?

Sorry, I haven't been following issue closely. I agree in general with the following:

I think there's a valid case that there should be no emailing to past events prior to the timeframe set in the reminder itself. For example if it is Oct 8 and your newly-created reminder says "2 days after event" only an event on Oct 6 or future will get reminders - no exceptions.

To implement that would likely require adding a created_date field to https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Core/ActionSchedule.xml. We tend to always display fields in browser that exist in db; I'd suggest showing the field on the Edit Scheduled Reminder field as frozen. Then the field could be used to prevent gotcha mail sends as described above.

That's an interesting idea, although I'm not sure a new date (created_date) field is absolutely necessary. civicrm_event.state_date compared to the reminder schedule (i.e. 2 days after, 3 weeks after, etc.) may be sufficient. Thoughts?

I guess in terms of progressing this - this makes sense for the paid issue queue since it hasn't been taken up pro bono - @colemanw any thoughts on that.

The cron job needs to know when the scheduled reminder was created in order to do what you suggest. If there is no way to check on the date of its creation, then it will do what it currently does, which is check if there are any dates prior to 2 days ago. The processing is done entirely independently of the creation, and doesn't know when the creation happened.

Yes please provide estimate. I will submit the 'paid issue' ticket immediately.

I see @JoeMurray, thanks, that is an excellent summary of the root of the problem.

would that be like my suggesttion #3 (closed) above but hardcoded?

Yes, but stripped down so it is less a user configurable feature that may be overlooked and more just a change in default behaviour to prevent bad stuff from happening. Your approach will likely be desired by those who like the ability to easily 'catch up' when they forgot to configure something.

I agree with a change to the default behavior. If a person wishes to play 'catch up' as @JoeMurray says, the best practice and tool would be to use CiviMail to contact participants of past events, rather than a scheduled reminder.

@Stoob: I totally and 100% agree with you for reminders AFTER an event. For reminders BEFORE an event I would not suggest changes. For reminders before an event, it is usually a few important mails with either a packlist, checking of registration info, for reminders before I would think reminders are plausible just as they work now, since they are reminders for a future event. It is for past events that current code reaps havoc.

My suggestion nr.3 was something like:
BEFORE THE EVENT: do not change current behaviour
AFTER THE EVENT: only send on the configured day (eg. exactly 2 days after the event, 'do not catch up')

@magnolia61 I agree that reminders created for before-the-event behavior does not need to change

@Stoob , this could be fixed by core team including adding unit tests and having PR review done including feedback for 5h. Let @josh know if you'll fund that, and Monish will implement at our 60% off rate to the core team, with QA by @colemanw .

@JoeMurray it doesn't appear this has ever been resolved. Is this offer still valid if I have a client interested in funding it?

@landbryo are you saying that because you have hit this issue recently? (I know there were some fixes to the code but I feel like they might have been more at the membership side)

@seamuslee

@eileen Yes, that is the case. As per this thread, the latest was that @Stoob was going to have the fix commissioned and his client lost interest.

@Stoob that's 5h @ the partner $125 rate, so $625. Let me know if that is acceptable and the CT and Monish and tackle it.

Thanks for trying @josh and @JoeMurray the client has been unresponsive for a week, after my initial email and as well as a reminder. I would not assume at this point they are not interested, but that is the most likely scenario. If that changes I will let you know. My guess is they will probably stop using this Civi feature entirely, and instead send event reminders semi-manually.

Oh well. Thx @Stoob

I think it's still useful @JoeMurray to place a warning in the tpl or help text (or at least documentation) about the behavior that causes newly created Scheduled Reminders to email all participants of all past events. It certainly was unexpected (and undesirable) behavior for both an apparent newcomer (magnolia61) and a veteran like myself. I volunteer to help. Suggestions?

Sure, phrased as help this could be good. I'd review the .hlp files or add a new one on the relevant form/field.

Hi @landbryo we could still do this for 5h @ USD$125 = USD$625 if you can provide the funds.

@JoeMurray I've received the okay from my client. What is the next step?

@JoeMurray checking in again on what the next steps are here.

Sorry I missed the comment last week.

Just confirming scope of work and wanting a nod from @eileen :

Add created_date field to https://github.com/civicrm/civicrm-core/blob/master/xml/schema/Core/ActionSchedule.xml.
Show the field on the Edit Scheduled Reminder field as frozen.
Change the cron job so that reminders are not sent in situations where the reminder should have been sent prior to the creation date of the scheduled reminder.
Add test that reminders are not sent and are sent as appropriate by cron job.
Add help text ('Reminder messages will not be sent for messages that should have already gone out before the Scheduled Reminder was created.') as per

@landbryo Can you email me joe (dot) murray (at) jmaconsulting (dot) biz and we can deal with setting up payment, etc.

This seems generally sensible to me - obviously - tests

@andrewhunt @JonGold @mfb any comments on the last comment which describes the scope

@Stoob ^

I've spent a long time in the guts of this code, and it's my position that using Scheduled Reminders for events is broken. The problems @magnolia61 is describing is typical of this "feature".

I think the scope makes sense, but for UX/simplicity, I think scheduled reminders shouldn't fire on ANY entity if the reminder was created after the entity - not just events. And there should be verbiage that indicates that scheduled reminders won't fire on previously created events/contributions/etc.

I don't agree about this. I think there might be specific situations--like events with end dates in the past--that could be excluded from new reminders. However, I can think of a few very reasonable and common situations that would be excluded if we apply the rule about previously-created entities:

Memberships would be the most common problem--you might even have memberships that were created when you migrated data to CiviCRM. You might want to add or revise the reminders for member renewal, but nearly all members would be excluded.
It's not uncommon to set up pre-event and post-event reminders even after some participants have registered.
I'm not sure whether you were expecting events or participants to count as the entity that can't predate the reminder, but you can't create an event-specific reminder without the event already existing. If the event is the thing older than the reminder that should cause reminders to be suppressed, this would make event-specific reminders impossible.
Reminders based on contact fields (e.g. "renew your CPR certification" or "happy birthday!") would be really haphazard if contacts couldn't be older than the reminders.

For as many nightmare scenarios you can think of with reminders based upon ancient donations or events, I can probably suggest reasonable situations where a reminder set up after the fact is a good thing. I much prefer the rule of "Reminder messages will not be sent for messages that should have already gone out before the Scheduled Reminder was created."

@andrewhunt I refined Jon's point so the restriction is based not on pre-existing entities, but on whether the email should already have gone out at time reminder was created. Does that subtle change address yourconcerns? Jon is good with it.

Yes, that's perfect. Coming in this late, it looked like he was wanting to go further than this, but I can see that it's an edit. Moreover, it's a good absolute rule to apply across the board rather than a entity- or situation-specific thing.

@JoeMurray could maybe whoever works on this look at https://github.com/civicrm/civicrm-core/pull/17641 first? I think it would be worth merging that first potentially since it ties in (& would probably help whoever get their head into the code)

Well, I'm not keen on adding a lot of work to a scope that isn't that big when it has languished for a year without funding. But...

Yeah, sure, we'll do the review. @monish.deb could you review the simple looking https://github.com/civicrm/civicrm-core/pull/17641 that is actually hovering over creaky code? Two very good coders are engaged there. I agree with idea that inactive events or templates should not have reminders sent. There might be some edge cases where a reminder is expected to go out after event but the event is marked inactive, but so be it.

@JonGold , I've expanded and refined the condition above stating when the reminder should NOT go out. Can you review if it now captures your view?

Yes, that accurately reflects my comments. Thanks Joe!

If the combined wisdom of people here is in agreement this is concept-approved from my pov

mentioned in issue #1956 (closed)

I have started to work on the agreed spec mentioned here and will add unit-tests to capture the behavior. Can we mark the ticket with 'Concept Approved'?

On the other hand, I have mentioned a minor change in https://github.com/civicrm/civicrm-core/pull/17641

added Concept approved needs:testing sig:bug labels and removed needs:concept approval label

@monish.deb thanks for reviewing that PR.

Just curious and would like to doublecheck if the design of the proposed solution will also address the scenario where reminder emails scheduled after the event date (e.g. two weeks after event x). And is a unit test on the limit by group or limit by role also part of the planned pr?

Any type of reminder that would have resulted in messages going out before the reminder was created should not be sent. So a reminder set to go to attendees 2 weeks after every event ends would not go out to attendees of an event that ended 3 week before the reminder was created.
I think there would be a benefit for lots of tests, but I've only committed us to 1 showing the messages are going out and 1 showing they are not going out. I'd prefer to do both on the same type of criteria, and normally I would expect for this fix to focus on relative time. Please feel free to submit additional tests, they'd be most welcome.

I have submitted the PR https://github.com/civicrm/civicrm-core/pull/18298 . Here's the details how the new condition will act upon schedule reminders configured for supported entities:

Introduced civicrm_action_schedule.created_date timestamp column, set default to current datetime.
Added upgrade code. I am not sure how to populate the created_date values for old reminders, on the upgrade as I am not sure when the reminders were configured in the system. But as per now, since created_date has a default set to current time, so after upgrade it will populate the created_date to the current time for existing reminders.
Use-cases:

Reminder for entity (say event) configured with a relative date send using "after":

1.1 If a reminder is configured to send emails to participants 1 week after the event end_date (= 25th Aug) and the reminder was configured today (created_date = 2nd September) which would be 1 week 1day after the event end_date - reminders won't be sent out to the participants.

1.2 If a reminder is configured to send emails to its participants  1 week after the event end_date (= 25th Aug) and the reminder was configured yesterday (created_date = 1st September) which would be exactly 1 week after the event end_date - reminders will be sent out to the participants.

A reminder for entity (say event) configured with a relative date send using "before":

1.1 If a reminder is configured to send emails to participants 1 week before the event start_date (= 25th Aug) and the reminder was configured on more then a week from start_date, say created_date = 15th Aug which would be 10days before the event start_date - reminders won't be sent out to the participants.

1.2 If a reminder is configured to send emails to its participants  1 week before the event start_date (= 25th Aug) and the reminder was configured more then a week ago from start_date, i.e. created_date = 12th Aug which 13days before the event start_date - reminders will be sent out to the participants.

Reminder for entity (say event) configured with an absolute date:

1.1 If a reminder is configured to send emails to its participants yesterday (created_date = 1st Sept), then it won't send out emails to participants.

1.2. If a reminder is configured to send emails to its participants today or any future date (created_date >= 2nd Sept), then it will deliver reminders to participants.

These use-cases hold true not only for the event but also for other supported entities, and for evidence, I have made necessary changes in the existing unit-tests with respective configurations, written for other entities

I have added unit-test to capture the 1st use-case here

Re 2. Let's set civicrm_action_schedule.created_date to be the date of upgrade, which will solve some issues without causing any.

Re: 1.2 If a reminder is configured to send emails to its participants 1 week before the event start_date (= 25th Aug) and the reminder was configured less then a week from start_date, i.e. created_date = 23rd Aug which would be 2days before the event start_date - reminders will be sent out to the participants. No this is incorrect. No reminders that would have been sent before a scheduled reminder is created will be sent out. If an event start date of Sep 25th has a scheduled reminder for 1 week before (ie Sep 18th) is created Sep 2, then it will be sent, as Sep 18th is after Sep 2.

For reminders with repetition, can you explain how it would work if there was a reminder repeated twice before an event? The same rules should apply to each repetition of the reminder. The objective is stop the sending of reminders that would have gone out prior to the creation of the reminder. For example, saying that a reminder should go out 2 days and 1 day in advance of every event with info about the event should not result in these going out to all events in the past.

@JoeMurray thanks for your feedback and I agree with your points. For 1.2, I have corrected the query to ensure that, for the reminder created before the scheduled date is valid, as the schedule date comes after the created_date and it should send out reminders to recipients.

For reminders with repetition, the reminders should always go out, if created before the scheduled date.

I have updated the PR along with the minor label changes mentioned in the patch, also updated my previous comment that highlights the use-cases.

It would be great if someone interested in this issue could do a review of https://github.com/civicrm/civicrm-core/pull/18298.

added current sprint label

mentioned in issue #2562 (closed)

mentioned in commit b103ff44

mentioned in commit f7ecb545

Just noting we merged something into 5.39 that adds optional effective start date & effective end date & is the earliest (for example) that the triggering receive date could be or the latest (for example) that the triggering register date could be.

This is the patch that adds the UI

https://github.com/civicrm/civicrm-core/pull/20432

There is a lot of confusing discussion in here & this is a variant on ONE of the things that was discussed. Nevertheless I'm going to close it against 5.39 as it provides at least a partial fix

I think the timezone issues will still challenge us as various things that probably should be stored as time stamps are - but I think this issue is too confusing at the moment & it's better to start a new issue for other things

closed

changed milestone to %5.39.0

Unwanted mail blast sent by Scheduled Reminders

Child items ...

Activity