dev/core#1590: Don't send reminders on deleted events

[MegaphoneJon]

Overview

If you have a scheduled reminder for an event that's deleted, it will still send (repeatedly!) reminders to anyone listed under "Additional Recipients".

Details for replication and a technical discussion are in core#1590.

Technical Details

This is an improvement on the approach used in #16311. When I wrote that patch, I thought the only exception to "send to additional recipients" was if the event was a template. I can now see that each ActionMapping class needs its own criteria to determine if additional recipients should be reminded. E.g. contacts need to check for is_deleted, contributions/memberships need to actually check if the record was deleted, events must do the latter AND ensure it's not a template.

So instead of simply modifying the logic of the notTemplate() method, I've created a new method Civi/ActionSchedule/MappingInterface::sendToAdditional(), and moved the notTemplate() logic into CRM_Event_ActionMapping .

Comments

This passes the test in #16311, and adds another test for deleted events.

I've set all the other sendToAdditional() methods to return TRUE to preserve current behavior on all other scheduled reminder types. I don't doubt there are some undesired behaviors in there, but that's beyond the scope of this PR (though this PR should make those fixes trivial).

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

I started by reading 16311 and then this PR. I like how this one has a (REF) to delegate the check from the RecipientBuilder to each ActionMapping.

In either revision, there's a couple things which feel odd/fishy. Just thinking out loud.

1. If a civicrm_event is generally not actionable because of is_template=1, and if you can make the short-circuit decision here (which I'm not 100% certain of; but that seems to be implied), then shouldn't it short-circuit all four passes(buildRelFirstPass(), buildAddlFirstPass(), buildRelRepeatPass(), buildAddlRepeatPass())? It seems the value ofis_template would be equally germane to all of them?
2. In the underlying createQuery(), it has a filter on is_template and is_active. My expectation was that the filter would protect all four passes. (createQuery() is supposed to be a common building-block by way of RecipientBuilder::build{Rel,Addl}{First,Repeat}}Pass() => RecipientBuilder::prepareQuery() => ActionMapping::createQuery()). But evidently not...
3. I suppose there's a question about whether reminders should be sent for an inactive event. I would generally think not, and the createQuery() says no (r.is_active = 1)... so if we have to reproduce these guards, perhaps sendToAdditional() should also reproduce the is_active guard?

Anyway, these comments aren't meant as hard blocks. As a (REF) and a bugfix, the new code seems better than the old code. I'm just commenting because something about the context feels fishy.

[MegaphoneJon]

The heart of the issue here is that createQuery() isn't called when sending to additional recipients. Which is why these passes are necessary to begin with - createQuery()'s docblock reads: Generate a query to locate recipients who match the given schedule.
Additional recipients are by definition arbitrary and outside the normal query parameters.

I hope that addresses your points 1 and 2! I agree re: point 3, and I'll fix that when I clean up these failing tests.

[totten]

The heart of the issue here is that createQuery() isn't called when sending to additional recipients.

Yeah, I had that intuition -- that maybe createQuery() isn't called because AddlFirst/AddlRepeat can use a static contact-list. But then I tried to check the intuition:

1. At least in skimming the PHP, there is a code-path from buildAddl{First,Repeat}Pass() to prepareQuery() to createQuery() to $query->where('r.is_template = 0');. As near I can see, the path doesn't have any obstructions/short-circuits. It could just be my bad reading though. I guess a debugger would reveal the truth. Or maybe there's something in the structure of the query (like an OR or IN or UNION) which makes the filter inoperative.

2. AddlFirst/AddlRepeat do need some information from the ActionMapping - i.e. to identify the subject-matter for the reminder (the entity and the date; eg civicrm_event, 123, start_date, 2020-06-02). I suppose that some mapping-types (EVENT_TPL_MAPPING_ID, EVENT_NAME_MAPPING_ID) do convey part of the subject-matter (entity ID) in their $schedule->entity_value record. But $schedule->entity_value is not necessarily entity IDs. It could equally be an array of even-type-ids or activity-type-ids (dependent on the mapping type...).

(IMHO, entity_value is a misnomer... it's more like "the primary filter supplied by the user" or primary_query_value.)

Tangentially... there may be a crossed wire in using $schedule->entity_value to set sendToAdditional(int $entityId). Since $schedule->entity_value can have different things packed into it, it takes some work to unpack. Notice the extra work that createQuery() does here and here.

[monishdeb]

I agree with @totten . How about simply passing the entityValue (which may be event ID(s) or event_type ID(s) based on EVENT_TPL_MAPPING_ID) and extending the SQL here a bit

diff --git a/CRM/Event/ActionMapping.php b/CRM/Event/ActionMapping.php
index 77fa8d77ed..9a91b69e07 100644
--- a/CRM/Event/ActionMapping.php
+++ b/CRM/Event/ActionMapping.php
@@ -186,4 +186,24 @@ class CRM_Event_ActionMapping extends \Civi\ActionSchedule\Mapping {
+  /**
+   * Determine whether a schedule based on this mapping should
+   * send to additional contacts.
+   * @param int|array $entityValue 
+   */
+  public function sendToAdditional($entityValue): bool {
+    $selectedValues = (array) \CRM_Utils_Array::explodePadded($entityValue);
+    $valueField = ($this->id == \CRM_Event_ActionMapping::EVENT_TYPE_MAPPING_ID) ? 'event_type_id' : 'id';
+    // Don't send to additional recipients if this event is deleted or a template.
+    $query = new \CRM_Utils_SQL_Select('civicrm_event e');
+    $sql = $query
+      ->select('e.id')
+      ->where("e.{$valueField} IN (@selectedValues)")
+      ->where("is_template = 0")
+      ->where("is_active = 1")
+      ->param('selectedValues', $selectedValues)
+      ->toSQL();
+    $dao = \CRM_Core_DAO::executeQuery($sql);
+    return (bool) $dao->N;
+  }
+
}

^^ @MegaphoneJon

[MegaphoneJon]

I went to implement this, then realized that the code Tim mentions doesn't account for "event template" scheduled reminders - just "event type" and "event name" scheduled reminders. I implemented similar code, but it's a bit larger to support all three cases.

[JoeMurray]

@monishdeb can you review again the updated PR?

[MegaphoneJon]

Hi Tim - you're right, createQuery() IS called. However, if you look at Civi\ActionScheduler\RecipientBuilder::buildAddlFirstPass(), you'll see that it's just so the $params can be merged into a query that's built within that function. Notably, this function doesn't join to any tables except civicrm_contact and civicrm_action_log. So the WHERE statement from createQuery() is ignored.

I'll look into the $schedule->entity_value issue.

[eileenmcnaughton]

test this please

[MegaphoneJon]

I have a lot of feelings about this PR.

On the one hand, it's better in than out. OTOH, "additional recipients" scheduled reminder functionality is so utterly broken that I'm inclined to think that it should be disabled altogether, or at least limited to certain entities.

Because the scheduled reminder is based on civicrm_contact rather than the membership/contribution/etc., we have a few issues:

• Tokens don't work. Using tokens with Scheduled Reminders isn't always necessary, but it's pretty common.
• Participant-based scheduled reminders are completely broken. There's no way to link the contact to the event dates, so the reminder simply goes out the next time "Send Scheduled Reminders" runs.

This is, I believe, my third PR targeting "send to additional contacts" bugs, but the truth is that even with this merged, it's still quite broken.

[BettyDolfing]

Test this please

[monishdeb]

Jenkins test this please

[eileenmcnaughton]

test this please

[MegaphoneJon]

test this please

[eileenmcnaughton]

test this please

[seamuslee001]

@MegaphoneJon Error in the test run (had to look at the console log for it tho)

PHP Fatal error:  Declaration of CRM_Event_ActionMapping::sendToAdditional($entityId): bool must be compatible with Civi\ActionSchedule\Mapping::sendToAdditional(int $entityId): bool in /home/jenkins/bknix-dfl/build/core-17641-7i3tf/web/sites/all/modules/civicrm/CRM/Event/ActionMapping.php on line 235

[monishdeb]

@MegaphoneJon I think it would be appropriate to rename the parameter to $entityValue from $entityID and update the param type to mixed - string|int

[MegaphoneJon]

test this please

[eileenmcnaughton]

PHP Fatal error: Declaration of CRM_Event_ActionMapping::sendToAdditional($entityId): bool must be compatible with Civi\ActionSchedule\Mapping::sendToAdditional(int $entityId): bool in /home/jenkins/bknix-dfl/build/core-17641-7utem/web/sites/all/modules/civicrm/CRM/Event/ActionMapping.php on line 235
PHP Stack trace:

[eileenmcnaughton]

you either need to add int to the parent or remove here

[eileenmcnaughton]

test this please

[eileenmcnaughton]

(fail was unrelated but let's get the green tick)

[monishdeb]

• (r-explain) PASS
  • (r-user):PASS
  • (r-doc) PASS
  • (r-run): PASS
  • (r-tech): PASS
  • (r-code) PASS
  • (r-maint) PASS
  • (r-test) PASS