diff --git a/CRM/Event/ActionMapping.php b/CRM/Event/ActionMapping.php index 629e17438d18..915ec195f06b 100644 --- a/CRM/Event/ActionMapping.php +++ b/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'); diff --git a/CRM/Member/ActionMapping.php b/CRM/Member/ActionMapping.php index a33e55887136..a091901d355f 100644 --- a/CRM/Member/ActionMapping.php +++ b/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; } diff --git a/CRM/Upgrade/Incremental/sql/5.11.alpha1.mysql.tpl b/CRM/Upgrade/Incremental/sql/5.11.alpha1.mysql.tpl index 26170aa4235a..2c86b6a9636b 100644 --- a/CRM/Upgrade/Incremental/sql/5.11.alpha1.mysql.tpl +++ b/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; diff --git a/Civi/ActionSchedule/RecipientBuilder.php b/Civi/ActionSchedule/RecipientBuilder.php index b72bbdada5ff..eaa7b4df90a2 100644 --- a/Civi/ActionSchedule/RecipientBuilder.php +++ b/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,7 +168,9 @@ 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 */ @@ -177,59 +178,15 @@ 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'])) { diff --git a/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php b/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php index fead50b0ddd8..9dfc9eb30f0c 100644 --- a/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php +++ b/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php @@ -46,6 +46,15 @@ public function setUp() { $this->mut = new CiviMailUtils($this, TRUE); + $this->fixtures['rolling_membership_type'] = array( + 'period_type' => 'rolling', + 'duration_unit' => 'month', + 'duration_interval' => '3', + 'is_active' => 1, + 'domain_id' => 1, + 'financial_type_id' => 2, + ); + $this->fixtures['rolling_membership'] = array( 'membership_type_id' => array( 'period_type' => 'rolling', @@ -98,6 +107,14 @@ public function setUp() { 'first_name' => 'Churmondleia', 'last_name' => 'Ōtākou', ); + $this->fixtures['contact_2'] = array( + 'is_deceased' => 0, + 'contact_type' => 'Individual', + 'email' => 'test-contact-2@example.com', + 'gender_id' => 'Male', + 'first_name' => 'Fabble', + 'last_name' => 'Fi', + ); $this->fixtures['contact_birthdate'] = array( 'is_deceased' => 0, 'contact_type' => 'Individual', @@ -194,6 +211,36 @@ public function setUp() { 'start_action_unit' => '', 'subject' => '1-Day (repeating) (about {activity.activity_type})', ); + $this->fixtures['sched_eventname_1day_on_abs_date'] = array( + 'name' => 'sched_eventname_1day_on_abs_date', + 'title' => 'sched_eventname_1day_on_abs_date', + 'limit_to' => 1, + 'absolute_date' => CRM_Utils_Date::processDate('20120614100000'), + 'body_html' => '

sched_eventname_1day_on_abs_date

', + 'body_text' => 'sched_eventname_1day_on_abs_date', + 'entity_status' => '1', + 'entity_value' => '2', + 'group_id' => NULL, + 'is_active' => '1', + 'is_repeat' => '0', + 'mapping_id' => '3', + 'msg_template_id' => NULL, + 'recipient' => '2', + 'recipient_listing' => NULL, + 'recipient_manual' => NULL, + 'record_activity' => NULL, + 'repetition_frequency_interval' => NULL, + 'repetition_frequency_unit' => NULL, + 'end_action' => NULL, + 'end_date' => NULL, + 'end_frequency_interval' => NULL, + 'end_frequency_unit' => NULL, + 'start_action_condition' => NULL, + 'start_action_date' => NULL, + 'start_action_offset' => NULL, + 'start_action_unit' => NULL, + 'subject' => 'sched_eventname_1day_on_abs_date', + ); $this->fixtures['sched_membership_join_2week'] = array( 'name' => 'sched_membership_join_2week', 'title' => 'sched_membership_join_2week', @@ -987,6 +1034,40 @@ public function testActivityDateTimeMatchRepeatableScheduleOnAbsDate() { )); } + public function testEventNameWithAbsoluteDateAndNothingElse() { + $participant = $this->createTestObject('CRM_Event_DAO_Participant', array_merge($this->fixtures['participant'], array('status_id' => 1))); + $this->callAPISuccess('Email', 'create', array( + 'contact_id' => $participant->contact_id, + 'email' => 'test-event@example.com', + )); + $this->callAPISuccess('contact', 'create', array_merge($this->fixtures['contact'], array('contact_id' => $participant->contact_id))); + + $actionSchedule = $this->fixtures['sched_eventname_1day_on_abs_date']; + $actionSchedule['entity_value'] = $participant->event_id; + $this->callAPISuccess('action_schedule', 'create', $actionSchedule); + + $this->assertCronRuns(array( + array( + // Before the 24-hour mark, no email + 'time' => '2012-06-13 04:00:00', + 'recipients' => array(), + 'subjects' => array(), + ), + array( + // On absolute date set on 2012-06-14 + 'time' => '2012-06-14 00:00:00', + 'recipients' => array(array('test-event@example.com')), + 'subjects' => array('sched_eventname_1day_on_abs_date'), + ), + array( + // Run cron 4 hours later; first message already sent + 'time' => '2012-06-14 04:00:00', + 'recipients' => array(), + 'subjects' => array(), + ), + )); + } + /** * For contacts/activities which don't match the schedule filter, * an email should *not* be sent. @@ -1191,15 +1272,43 @@ public function testMembershipEndDateRepeat() { // end_date=2012-06-15 ; schedule is 2 weeks before end_date $this->assertCronRuns(array( array( - // After the 2-week mark, send an email. + // After the 1-month mark, no email + 'time' => '2012-07-15 01:00:00', + 'recipients' => array(), + ), + array( + // After the 2-month mark, send an email. 'time' => '2012-08-15 01:00:00', 'recipients' => array(array('test-member@example.com')), ), array( - // After the 2-week mark, send an email. + // 4 weeks after first email send first repeat 'time' => '2012-09-12 01:00:00', 'recipients' => array(array('test-member@example.com')), ), + array( + // 1 week after first repeat send nothing + // There was a bug where the first repeat went out and then + // it would keep going out every cron run. This is to check that's + // not happening. + 'time' => '2012-09-19 01:00:00', + 'recipients' => array(), + ), + array( + // 4 weeks after first repeat send second repeat + 'time' => '2012-10-10 01:00:00', + 'recipients' => array(array('test-member@example.com')), + ), + array( + // 4 months after membership end, send nothing + 'time' => '2012-10-15 01:00:00', + 'recipients' => array(), + ), + array( + // 5 months after membership end, send nothing + 'time' => '2012-11-15 01:00:00', + 'recipients' => array(), + ), )); } @@ -1268,8 +1377,6 @@ public function testMembershipEndDateMatch() { array( // Before the 2-week mark, no email. 'time' => '2012-05-31 01:00:00', - // 'time' => '2012-06-01 01:00:00', - // FIXME: Is this the right boundary? 'recipients' => array(), ), array( @@ -1277,6 +1384,11 @@ public function testMembershipEndDateMatch() { 'time' => '2012-06-01 01:00:00', 'recipients' => array(array('test-member@example.com')), ), + array( + // After the email is sent, another one is not sent + 'time' => '2012-06-01 02:00:00', + 'recipients' => array(), + ), )); // Now suppose user has renewed for rolling membership after 3 months, so upcoming assertion is written @@ -1300,10 +1412,121 @@ public function testMembershipEndDateMatch() { 'time' => '2012-08-31 01:00:00', 'recipients' => array(), ), - //array( // After the 2-week mark, send an email - //'time' => '2012-09-01 01:00:00', - //'recipients' => array(array('member2@example.com')), - //), + array( + // After the 2-week mark, send an email + 'time' => '2012-09-01 01:00:00', + 'recipients' => array(array('member2@example.com')), + ), + array( + // After the email is sent, another one is not sent + 'time' => '2012-09-01 02:00:00', + 'recipients' => array(), + ), + )); + + $membership->end_date = '2012-12-15'; + $membership->save(); + // end_date=2012-12-15 ; schedule is 2 weeks before end_date + $this->assertCronRuns(array( + array( + // Before the 2-week mark, no email + 'time' => '2012-11-30 01:00:00', + 'recipients' => array(), + ), + array( + // After the 2-week mark, send an email + 'time' => '2012-12-01 01:00:00', + 'recipients' => array(array('member2@example.com')), + ), + array( + // After the email is sent, another one is not sent + 'time' => '2012-12-01 02:00:00', + 'recipients' => array(), + ), + )); + + } + + public function createMembershipAndContact($contactFixture, $membershipTypeId) { + $result = $this->callAPISuccess('contact', 'create', $contactFixture); + $contact = $result['values'][$result['id']]; + $params = array( + 'status_id' => 2, + 'contact_id' => $contact['id'], + 'membership_type_id' => $membershipTypeId, + 'owner_membership_id' => 'NULL', + ); + $params = array_merge($this->fixtures['rolling_membership'], $params); + $membership = $this->createTestObject('CRM_Member_DAO_Membership', $params); + $this->assertTrue(is_numeric($membership->id)); + return $membership; + } + + /** + * This test is very similar to testMembershipEndDateMatch, but it adds + * another contact because there was a bug in + * RecipientBuilder::buildRelFirstPass where it was only sending the + * reminder for the first contact returned in a query for renewed + * memberships. Other contacts wouldn't get the mail. + */ + public function testMultipleMembershipEndDateMatch() { + $membershipTypeId = $this->membershipTypeCreate($this->fixtures['rolling_membership']['membership_type_id']); + $membershipOne = $this->createMembershipAndContact($this->fixtures['contact'], $membershipTypeId); + $membershipTwo = $this->createMembershipAndContact($this->fixtures['contact_2'], $membershipTypeId); + $actionSchedule = $this->fixtures['sched_membership_end_2week']; + $actionSchedule['entity_value'] = $membershipTypeId; + $actionScheduleDao = CRM_Core_BAO_ActionSchedule::add($actionSchedule); + $this->assertTrue(is_numeric($actionScheduleDao->id)); + + // end_date=2012-06-15 ; schedule is 2 weeks before end_date + $this->assertCronRuns(array( + array( + // Before the 2-week mark, no email. + 'time' => '2012-05-31 01:00:00', + 'recipients' => array(), + ), + array( + // After the 2-week mark, send emails. + 'time' => '2012-06-01 01:00:00', + 'recipients' => array( + array('test-member@example.com'), + array('test-contact-2@example.com'), + ), + ), + array( + // After the email is sent, another one is not sent + 'time' => '2012-06-01 02:00:00', + 'recipients' => array(), + ), + )); + + // Now suppose user has renewed for rolling membership after 3 months, so upcoming assertion is written + // to ensure that new reminder is sent 2 week before the new end_date i.e. '2012-09-15' + $membershipOne->end_date = '2012-09-15'; + $membershipOne->save(); + $membershipTwo->end_date = '2012-09-15'; + $membershipTwo->save(); + + // end_date=2012-09-15 ; schedule is 2 weeks before end_date + $this->assertCronRuns(array( + array( + // Before the 2-week mark, no email + 'time' => '2012-08-31 01:00:00', + 'recipients' => array(), + ), + array( + // After the 2-week mark, send an email + 'time' => '2012-09-01 01:00:00', + 'recipients' => array( + array('test-member@example.com'), + array('test-contact-2@example.com'), + ), + ), + array( + // After the email is sent, another one is not sent + 'time' => '2012-06-01 02:00:00', + 'recipients' => array(), + ), )); } @@ -1334,12 +1557,10 @@ public function testMembershipEndDateNoMatch() { array( // Before the 2-week mark, no email. 'time' => '2012-05-31 01:00:00', - // 'time' => '2012-06-01 01:00:00', - // FIXME: Is this the right boundary? 'recipients' => array(), ), array( - // After the 2-week mark, send an email. + // After the 2-week mark, no email 'time' => '2013-05-01 01:00:00', 'recipients' => array(), ), @@ -1533,7 +1754,7 @@ public function testMembership_referenceDate() { //check if reference date is set to membership's join date //as per the action_start_date chosen for current schedule reminder - $this->assertEquals('2012-03-15', + $this->assertEquals('2012-03-15 00:00:00', CRM_Core_DAO::getFieldValue('CRM_Core_DAO_ActionLog', $membership->contact_id, 'reference_date', 'contact_id') ); diff --git a/xml/schema/Core/ActionLog.xml b/xml/schema/Core/ActionLog.xml index 9bfc8cbcd480..51d9746ab547 100644 --- a/xml/schema/Core/ActionLog.xml +++ b/xml/schema/Core/ActionLog.xml @@ -99,9 +99,10 @@ reference_date Reference Date - date + datetime NULL Stores the date from the entity which triggered this reminder action (e.g. membership.end_date for most membership renewal reminders) 4.6 + 4.7