From 3741f351db785a5ed1f1459962b7e40696713996 Mon Sep 17 00:00:00 2001 From: Jon Goldberg Date: Thu, 18 Jun 2020 14:27:17 -0400 Subject: [PATCH] core#1590: Don't send reminders to add'l recipients on deleted/inactive/template events support reminders with event types/templates and multiple event/type/template IDs fix --- CRM/Activity/ActionMapping.php | 8 +++++ CRM/Contact/ActionMapping.php | 8 +++++ CRM/Contribute/ActionMapping/ByPage.php | 8 +++++ CRM/Contribute/ActionMapping/ByType.php | 8 +++++ CRM/Event/ActionMapping.php | 46 ++++++++++++++++++++++++ CRM/Member/ActionMapping.php | 8 +++++ Civi/ActionSchedule/Mapping.php | 6 ++++ Civi/ActionSchedule/MappingInterface.php | 6 ++++ Civi/ActionSchedule/RecipientBuilder.php | 25 ++----------- tests/phpunit/api/v3/JobTest.php | 35 ++++++++++++++++++ 10 files changed, 135 insertions(+), 23 deletions(-) diff --git a/CRM/Activity/ActionMapping.php b/CRM/Activity/ActionMapping.php index 05abcf9e24..9f73592644 100644 --- a/CRM/Activity/ActionMapping.php +++ b/CRM/Activity/ActionMapping.php @@ -118,4 +118,12 @@ class CRM_Activity_ActionMapping extends \Civi\ActionSchedule\Mapping { return $query; } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + public function sendToAdditional($entityId): bool { + return TRUE; + } + } diff --git a/CRM/Contact/ActionMapping.php b/CRM/Contact/ActionMapping.php index f4aee154e8..ea96b5da78 100644 --- a/CRM/Contact/ActionMapping.php +++ b/CRM/Contact/ActionMapping.php @@ -139,4 +139,12 @@ class CRM_Contact_ActionMapping extends \Civi\ActionSchedule\Mapping { return $query; } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + public function sendToAdditional($entityId): bool { + return TRUE; + } + } diff --git a/CRM/Contribute/ActionMapping/ByPage.php b/CRM/Contribute/ActionMapping/ByPage.php index 8c09c81cc2..6dcab6e2a7 100644 --- a/CRM/Contribute/ActionMapping/ByPage.php +++ b/CRM/Contribute/ActionMapping/ByPage.php @@ -216,4 +216,12 @@ class CRM_Contribute_ActionMapping_ByPage implements \Civi\ActionSchedule\Mappin return FALSE; } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + public function sendToAdditional($entityId): bool { + return TRUE; + } + } diff --git a/CRM/Contribute/ActionMapping/ByType.php b/CRM/Contribute/ActionMapping/ByType.php index ea259901a5..88d60e49c7 100644 --- a/CRM/Contribute/ActionMapping/ByType.php +++ b/CRM/Contribute/ActionMapping/ByType.php @@ -235,4 +235,12 @@ class CRM_Contribute_ActionMapping_ByType implements \Civi\ActionSchedule\Mappin return FALSE; } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + public function sendToAdditional($entityId): bool { + return TRUE; + } + } diff --git a/CRM/Event/ActionMapping.php b/CRM/Event/ActionMapping.php index 77fa8d77ed..9c8aaae7a5 100644 --- a/CRM/Event/ActionMapping.php +++ b/CRM/Event/ActionMapping.php @@ -159,6 +159,7 @@ class CRM_Event_ActionMapping extends \Civi\ActionSchedule\Mapping { } // build where clause + // FIXME: This handles scheduled reminder of type "Event Name" and "Event Type", gives incorrect result on "Event Template". if (!empty($selectedValues)) { $valueField = ($this->id == \CRM_Event_ActionMapping::EVENT_TYPE_MAPPING_ID) ? 'event_type_id' : 'id'; $query->where("r.{$valueField} IN (@selectedValues)") @@ -186,4 +187,49 @@ class CRM_Event_ActionMapping extends \Civi\ActionSchedule\Mapping { return $query; } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + * + * @param string $entityId Either an event ID/event type ID, or a set of event IDs/types separated + * by the separation character. + */ + public function sendToAdditional($entityId): bool { + $selectedValues = (array) \CRM_Utils_Array::explodePadded($entityId); + switch ($this->id) { + case self::EVENT_TYPE_MAPPING_ID: + $valueTable = 'e'; + $valueField = 'event_type_id'; + $templateReminder = FALSE; + break; + + case self::EVENT_NAME_MAPPING_ID: + $valueTable = 'e'; + $valueField = 'id'; + $templateReminder = FALSE; + break; + + case self::EVENT_TPL_MAPPING_ID: + $valueTable = 't'; + $valueField = 'id'; + $templateReminder = TRUE; + break; + } + // Don't send to additional recipients if this event is deleted or a template. + $query = new \CRM_Utils_SQL_Select('civicrm_event e'); + $query + ->select('e.id') + ->where("e.is_template = 0") + ->where("e.is_active = 1"); + if ($templateReminder) { + $query->join('r', 'INNER JOIN civicrm_event t ON e.template_title = t.template_title AND t.is_template = 1'); + } + $sql = $query + ->where("{$valueTable}.{$valueField} IN (@selectedValues)") + ->param('selectedValues', $selectedValues) + ->toSQL(); + $dao = \CRM_Core_DAO::executeQuery($sql); + return (bool) $dao->N; + } + } diff --git a/CRM/Member/ActionMapping.php b/CRM/Member/ActionMapping.php index 56db7eef41..e42add8682 100644 --- a/CRM/Member/ActionMapping.php +++ b/CRM/Member/ActionMapping.php @@ -168,4 +168,12 @@ class CRM_Member_ActionMapping extends \Civi\ActionSchedule\Mapping { } } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + public function sendToAdditional($entityId): bool { + return TRUE; + } + } diff --git a/Civi/ActionSchedule/Mapping.php b/Civi/ActionSchedule/Mapping.php index f6c1450d56..fc0e667b4d 100644 --- a/Civi/ActionSchedule/Mapping.php +++ b/Civi/ActionSchedule/Mapping.php @@ -337,4 +337,10 @@ abstract class Mapping implements MappingInterface { return FALSE; } + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + abstract public function sendToAdditional($entityId): bool; + } diff --git a/Civi/ActionSchedule/MappingInterface.php b/Civi/ActionSchedule/MappingInterface.php index 3dcd04fea1..f551fc11c1 100644 --- a/Civi/ActionSchedule/MappingInterface.php +++ b/Civi/ActionSchedule/MappingInterface.php @@ -139,4 +139,10 @@ interface MappingInterface { */ public function resetOnTriggerDateChange($schedule); + /** + * Determine whether a schedule based on this mapping should + * send to additional contacts. + */ + public function sendToAdditional($entityId): bool; + } diff --git a/Civi/ActionSchedule/RecipientBuilder.php b/Civi/ActionSchedule/RecipientBuilder.php index c8a32dd97e..c52f64adbb 100644 --- a/Civi/ActionSchedule/RecipientBuilder.php +++ b/Civi/ActionSchedule/RecipientBuilder.php @@ -138,7 +138,7 @@ class RecipientBuilder { public function build() { $this->buildRelFirstPass(); - if ($this->prepareAddlFilter('c.id') && $this->notTemplate()) { + if ($this->prepareAddlFilter('c.id') && $this->mapping->sendToAdditional($this->actionSchedule->entity_value)) { $this->buildAddlFirstPass(); } @@ -146,7 +146,7 @@ class RecipientBuilder { $this->buildRelRepeatPass(); } - if ($this->actionSchedule->is_repeat && $this->prepareAddlFilter('c.id')) { + if ($this->actionSchedule->is_repeat && $this->prepareAddlFilter('c.id') && $this->mapping->sendToAdditional($this->actionSchedule->entity_value)) { $this->buildAddlRepeatPass(); } } @@ -603,25 +603,4 @@ reminder.action_schedule_id = {$this->actionSchedule->id}"; return $this->mapping->resetOnTriggerDateChange($this->actionSchedule); } - /** - * Confirm this object isn't attached to a template. - * Returns TRUE if this action schedule isn't attached to a template. - * Templates are (currently) unique to events, so we only evaluate those. - * - * @return bool; - */ - private function notTemplate() { - if ($this->mapping->getEntity() === 'civicrm_participant') { - $entityId = $this->actionSchedule->entity_value; - $query = new \CRM_Utils_SQL_Select('civicrm_event e'); - $sql = $query - ->select('is_template') - ->where("e.id = {$entityId}") - ->toSQL(); - $dao = \CRM_Core_DAO::executeQuery($sql); - return !(bool) $dao->fetchValue(); - } - return TRUE; - } - } diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php index 444714204a..08a0188f6e 100644 --- a/tests/phpunit/api/v3/JobTest.php +++ b/tests/phpunit/api/v3/JobTest.php @@ -330,6 +330,41 @@ class api_v3_JobTest extends CiviUnitTestCase { $this->assertEquals(0, $successfulCronCount); } + /** + * Deleted events should not send reminders to additional contacts. + * + * @throws \CRM_Core_Exception + */ + public function testDeletedEventRemindAddlContacts() { + $contactId = $this->individualCreate(); + $groupId = $this->groupCreate(['name' => 'Additional Contacts', 'title' => 'Additional Contacts']); + $this->callAPISuccess('GroupContact', 'create', [ + 'contact_id' => $contactId, + 'group_id' => $groupId, + ]); + $event = $this->eventCreate(['title' => 'delete this event']); + $eventId = $event['id']; + + $this->callAPISuccess('action_schedule', 'create', [ + 'title' => 'Do not send me', + 'subject' => 'I am a reminder attached to a (soon to be) deleted event.', + 'entity_value' => $eventId, + 'mapping_id' => CRM_Event_ActionMapping::EVENT_NAME_MAPPING_ID, + 'start_action_date' => 'start_date', + 'start_action_offset' => 1, + 'start_action_condition' => 'before', + 'start_action_unit' => 'day', + 'group_id' => $groupId, + 'limit_to' => FALSE, + 'mode' => 'Email', + ]); + $this->callAPISuccess('event', 'delete', ['id' => $eventId]); + + $this->callAPISuccess('job', 'send_reminder', []); + $successfulCronCount = CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_action_log'); + $this->assertEquals(0, $successfulCronCount); + } + /** * Test scheduled reminders respect limit to (since above identified addition_to handling issue). * -- 2.25.1