(dev/core#285) Fixed second membership repeating reminder

This changes the check for all reminders (not just memberships) to only
check against a sent reminder with the same "reference date". So if we
don't find a sent reminder (ActionLog) with the same reference date as
the one we are trying to send now, then we assume the reminder needs to
be sent again.

This fixes reminders for the cases where case where the original
object's date changes (like membership end dates, for example).

This does change the behavior for some things like events which wouldn't
have reminders sent again if the events date changed. Now if you change
the date of an event and a schedule reminder condition becomes true for
the new date, the reminder will go out even if it was already sent once
for the previous date. That does sound like more of a feature than a bug
to me, but that is a change from previous behavior.

CRM/Event/ActionMapping.php

@@ -158,7 +158,7 @@ public function createQuery($schedule, $phase, $defaultParams) {
     $query['casContactTableAlias'] = NULL;
     $query['casDateField'] = str_replace('event_', 'r.', $schedule->start_action_date);
     if (empty($query['casDateField']) && $schedule->absolute_date) {
-      $query['casDateField'] = $schedule->absolute_date;
+      $query['casDateField'] = "'{$schedule->absolute_date}'";
     }
 
     $query->join('r', 'INNER JOIN civicrm_event r ON e.event_id = r.id');

CRM/Member/ActionMapping.php

@@ -146,11 +146,6 @@ public function createQuery($schedule, $phase, $defaultParams) {
     $query->where("e.status_id IN (#memberStatus)")
       ->param('memberStatus', \CRM_Member_PseudoConstant::membershipStatus(NULL, "(is_current_member = 1 OR name = 'Expired')", 'id'));
 
-    // Why is this only for civicrm_membership?
-    if ($schedule->start_action_date && $schedule->is_repeat == FALSE) {
-      $query['casUseReferenceDate'] = TRUE;
-    }
-
     return $query;
   }
 

CRM/Upgrade/Incremental/sql/5.11.alpha1.mysql.tpl

@@ -1 +1,3 @@
 {* file to handle db changes in 5.11.alpha1 during upgrade *}
+ALTER TABLE civicrm_action_log CHANGE COLUMN reference_date reference_date date
+time;

Civi/ActionSchedule/RecipientBuilder.php

@@ -82,7 +82,6 @@
  * Some parameters are optional:
  *  - casContactTableAlias: string, SQL table alias
  *  - casAnniversaryMode: bool
- *  - casUseReferenceDate: bool
  *
  * Additionally, some parameters are automatically predefined:
  *  - casNow
@@ -169,67 +168,25 @@ public function build() {
   }
 
   /**
-   * Generate action_log's for new, first-time alerts to related contacts.
+   * Generate action_log's for new, first-time alerts to related contacts,
+   * and contacts who are again eligible to receive the alert e.g. membership
+   * renewal reminders.
    *
    * @throws \Exception
    */
   protected function buildRelFirstPass() {
     $query = $this->prepareQuery(self::PHASE_RELATION_FIRST);
 
     $startDateClauses = $this->prepareStartDateClauses();
-
-    // In some cases reference_date got outdated due to many reason e.g. In Membership renewal end_date got extended
-    // which means reference date mismatches with the end_date where end_date may be used as the start_action_date
-    // criteria  for some schedule reminder so in order to send new reminder we INSERT new reminder with new reference_date
-    // value via UNION operation
-    $referenceReminderIDs = array();
-    $referenceDate = NULL;
-    if (!empty($query['casUseReferenceDate'])) {
-      // First retrieve all the action log's ids which are outdated or in other words reference_date now don't match with entity date.
-      // And the retrieve the updated entity date which will later used below to update all other outdated action log records
-      $sql = $query->copy()
-        ->select('reminder.id as id')
-        ->select($query['casDateField'] . ' as reference_date')
-        ->merge($this->joinReminder('INNER JOIN', 'rel', $query))
-        ->where("reminder.id IS NOT NULL AND reminder.reference_date IS NOT NULL AND reminder.reference_date <> !casDateField")
-        ->where($startDateClauses)
-        ->orderBy("reminder.id desc")
-        ->strict()
-        ->toSQL();
-      $dao = \CRM_Core_DAO::executeQuery($sql);
-
-      while ($dao->fetch()) {
-        $referenceReminderIDs[] = $dao->id;
-        $referenceDate = $dao->reference_date;
-      }
-    }
-
-    if (empty($referenceReminderIDs)) {
-      $firstQuery = $query->copy()
-        ->merge($this->selectIntoActionLog(self::PHASE_RELATION_FIRST, $query))
-        ->merge($this->joinReminder('LEFT JOIN', 'rel', $query))
-        ->where("reminder.id IS NULL")
-        ->where($startDateClauses)
-        ->strict()
-        ->toSQL();
-      \CRM_Core_DAO::executeQuery($firstQuery);
-    }
-    else {
-      // INSERT new log to send reminder as desired entity date got updated
-      $referenceQuery = $query->copy()
-        ->merge($this->selectIntoActionLog(self::PHASE_RELATION_FIRST, $query))
-        ->merge($this->joinReminder('LEFT JOIN', 'rel', $query))
-        ->where("reminder.id = !reminderID")
-        ->where($startDateClauses)
-        ->param('reminderID', $referenceReminderIDs[0])
-        ->strict()
-        ->toSQL();
-      \CRM_Core_DAO::executeQuery($referenceQuery);
-
-      // Update all the previous outdated reference date valued, action_log rows to the latest changed entity date
-      $updateQuery = "UPDATE civicrm_action_log SET reference_date = '" . $referenceDate . "' WHERE id IN (" . implode(', ', $referenceReminderIDs) . ")";
-      \CRM_Core_DAO::executeQuery($updateQuery);
-    }
+    // Send reminder to all contacts who have never received this scheduled reminder
+    $firstInstanceQuery = $query->copy()
+      ->merge($this->selectIntoActionLog(self::PHASE_RELATION_FIRST, $query))
+      ->merge($this->joinReminder('LEFT JOIN', 'rel', $query))
+      ->where("reminder.id IS NULL")
+      ->where($startDateClauses)
+      ->strict()
+      ->toSQL();
+    \CRM_Core_DAO::executeQuery($firstInstanceQuery);
   }
 
   /**
@@ -276,31 +233,18 @@ protected function buildRelRepeatPass() {
     // @todo - this only handles events that get moved later. Potentially they might get moved earlier
     $repeatInsert = $query
       ->merge($this->joinReminder('INNER JOIN', 'rel', $query))
-      ->merge($this->selectActionLogFields(self::PHASE_RELATION_REPEAT, $query))
-      ->select("MAX(reminder.action_date_time) as latest_log_time")
+      ->merge($this->selectIntoActionLog(self::PHASE_RELATION_REPEAT, $query))
       ->merge($this->prepareRepetitionEndFilter($query['casDateField']))
       ->where($this->actionSchedule->start_action_date ? $startDateClauses[0] : array())
       ->groupBy("reminder.contact_id, reminder.entity_id, reminder.entity_table")
-      // @todo replace use of timestampdiff with a direct comparison as TIMESTAMPDIFF cannot use an index.
-      ->having("TIMESTAMPDIFF(HOUR, latest_log_time, CAST(!casNow AS datetime)) >= TIMESTAMPDIFF(HOUR, latest_log_time, DATE_ADD(latest_log_time, INTERVAL !casRepetitionInterval))")
+      ->having("TIMESTAMPDIFF(HOUR, MAX(reminder.action_date_time), CAST(!casNow AS datetime)) >= TIMESTAMPDIFF(HOUR, MAX(reminder.action_date_time), DATE_ADD(MAX(reminder.action_date_time), INTERVAL !casRepetitionInterval))")
       ->param(array(
         'casRepetitionInterval' => $this->parseRepetitionInterval(),
       ))
       ->strict()
       ->toSQL();
 
-    // For unknown reasons, we manually insert each row. Why not change
-    // selectActionLogFields() to selectIntoActionLog() above?
-
-    $arrValues = \CRM_Core_DAO::executeQuery($repeatInsert)->fetchAll();
-    if ($arrValues) {
-      \CRM_Core_DAO::executeQuery(
-        \CRM_Utils_SQL_Insert::into('civicrm_action_log')
-          ->columns(array('contact_id', 'entity_id', 'entity_table', 'action_schedule_id'))
-          ->rows($arrValues)
-          ->toSQL()
-      );
-    }
+    \CRM_Core_DAO::executeQuery($repeatInsert);
   }
 
   /**
@@ -323,32 +267,19 @@ protected function buildAddlRepeatPass() {
     $daoCheck = \CRM_Core_DAO::executeQuery($addlCheck);
     if ($daoCheck->fetch()) {
       $repeatInsertAddl = \CRM_Utils_SQL_Select::from('civicrm_contact c')
-        ->merge($this->selectActionLogFields(self::PHASE_ADDITION_REPEAT, $query))
+        ->merge($this->selectIntoActionLog(self::PHASE_ADDITION_REPEAT, $query))
         ->merge($this->joinReminder('INNER JOIN', 'addl', $query))
-        ->select("MAX(reminder.action_date_time) as latest_log_time")
         ->merge($this->prepareAddlFilter('c.id'), array('params'))
         ->where("c.is_deleted = 0 AND c.is_deceased = 0")
         ->groupBy("reminder.contact_id")
-        // @todo replace use of timestampdiff with a direct comparison as TIMESTAMPDIFF cannot use an index.
-        ->having("TIMESTAMPDIFF(HOUR, latest_log_time, CAST(!casNow AS datetime)) >= TIMESTAMPDIFF(HOUR, latest_log_time, DATE_ADD(latest_log_time, INTERVAL !casRepetitionInterval))")
+        ->having("TIMESTAMPDIFF(HOUR, MAX(reminder.action_date_time), CAST(!casNow AS datetime)) >= TIMESTAMPDIFF(HOUR, MAX(reminder.action_date_time), DATE_ADD(MAX(reminder.action_date_time), INTERVAL !casRepetitionInterval))")
         ->param(array(
           'casRepetitionInterval' => $this->parseRepetitionInterval(),
         ))
         ->strict()
         ->toSQL();
 
-      // For unknown reasons, we manually insert each row. Why not change
-      // selectActionLogFields() to selectIntoActionLog() above?
-
-      $addValues = \CRM_Core_DAO::executeQuery($repeatInsertAddl)->fetchAll();
-      if ($addValues) {
-        \CRM_Core_DAO::executeQuery(
-          \CRM_Utils_SQL_Insert::into('civicrm_action_log')
-            ->columns(array('contact_id', 'entity_id', 'entity_table', 'action_schedule_id'))
-            ->rows($addValues)
-            ->toSQL()
-        );
-      }
+      \CRM_Core_DAO::executeQuery($repeatInsertAddl);
     }
   }
 
@@ -565,21 +496,17 @@ protected function prepareAddlFilter($contactIdField) {
    * @throws \CRM_Core_Exception
    */
   protected function selectActionLogFields($phase, $query) {
+    $selectArray = array();
     switch ($phase) {
       case self::PHASE_RELATION_FIRST:
       case self::PHASE_RELATION_REPEAT:
         $fragment = \CRM_Utils_SQL_Select::fragment();
-        // CRM-15376: We are not tracking the reference date for 'repeated' schedule reminders.
-        if (!empty($query['casUseReferenceDate'])) {
-          $fragment->select($query['casDateField']);
-        }
-        $fragment->select(
-          array(
-            "!casContactIdField as contact_id",
-            "!casEntityIdField as entity_id",
-            "@casMappingEntity as entity_table",
-            "#casActionScheduleId as action_schedule_id",
-          )
+        $selectArray = array(
+          "!casContactIdField as contact_id",
+          "!casEntityIdField as entity_id",
+          "@casMappingEntity as entity_table",
+          "#casActionScheduleId as action_schedule_id",
+          "!casDateField as reference_date",
         );
         break;
 
@@ -591,19 +518,19 @@ protected function selectActionLogFields($phase, $query) {
           'casNow' => $this->now,
         );
         $fragment = \CRM_Utils_SQL_Select::fragment()->param($params);
-        $fragment->select(
-          array(
-            "c.id as contact_id",
-            "c.id as entity_id",
-            "'civicrm_contact' as entity_table",
-            "#casActionScheduleId as action_schedule_id",
-          )
+        $selectArray = array(
+          "c.id as contact_id",
+          "c.id as entity_id",
+          "'civicrm_contact' as entity_table",
+          "#casActionScheduleId as action_schedule_id",
+          "!casDateField as reference_date",
         );
         break;
 
       default:
         throw new \CRM_Core_Exception("Unrecognized phase: $phase");
     }
+    $fragment->select($selectArray);
     return $fragment;
   }
 
@@ -624,12 +551,8 @@ protected function selectIntoActionLog($phase, $query) {
       "entity_id",
       "entity_table",
       "action_schedule_id",
+      "reference_date",
     );
-    if ($phase === self::PHASE_RELATION_FIRST || $phase === self::PHASE_RELATION_REPEAT) {
-      if (!empty($query['casUseReferenceDate'])) {
-        array_unshift($actionLogColumns, 'reference_date');
-      }
-    }
 
     return $this->selectActionLogFields($phase, $query)
       ->insertInto('civicrm_action_log', $actionLogColumns);
@@ -667,7 +590,8 @@ protected function joinReminder($joinType, $for, $query) {
     $joinClause = "civicrm_action_log reminder ON reminder.contact_id = {$contactIdField} AND
 reminder.entity_id          = {$entityIdField} AND
 reminder.entity_table       = '{$entityName}' AND
-reminder.action_schedule_id = {$this->actionSchedule->id}";
+reminder.action_schedule_id = {$this->actionSchedule->id} AND
+reminder.reference_date = !casDateField";
 
     // Why do we only include anniversary clause for 'rel' queries?
     if ($for === 'rel' && !empty($query['casAnniversaryMode'])) {