From bb3ba83e2cb112af540b8b6c678a462107a956c4 Mon Sep 17 00:00:00 2001 From: Andrew Hunt Date: Mon, 29 Oct 2018 06:43:37 -0400 Subject: [PATCH] CRM-14098 Membership reminders: don't exclude people who inherit other memberships (#12510) * CRM-14098 Membership reminders: don't exclude people who inherit other memberships * Membership reminders: test had two reminders with same name, differing params * CRM-14098 Membership reminders: test for inherited members ---------------------------------------- * CRM-14098: Scheduled reminders silently skipped for contacts with a non-permissioned relationship associated with ANY inherited membership type https://issues.civicrm.org/jira/browse/CRM-14098 * Membership reminders: comments to document some shaky assumptions * CRM-14098 Membership reminders: fix relies on joins, causing DB error on repeat * CRM-14098 Membership reminders: missing alias on date field * CRM-14098 Reminders test: explicitly make emails primary --- CRM/Member/ActionMapping.php | 70 +++--- Civi/ActionSchedule/RecipientBuilder.php | 2 +- .../CRM/Core/BAO/ActionScheduleTest.php | 226 +++++++++++++++--- 3 files changed, 235 insertions(+), 63 deletions(-) diff --git a/CRM/Member/ActionMapping.php b/CRM/Member/ActionMapping.php index 069577e9da..25e64bf502 100644 --- a/CRM/Member/ActionMapping.php +++ b/CRM/Member/ActionMapping.php @@ -97,8 +97,16 @@ class CRM_Member_ActionMapping extends \Civi\ActionSchedule\Mapping { $query['casContactIdField'] = 'e.contact_id'; $query['casEntityIdField'] = 'e.id'; $query['casContactTableAlias'] = NULL; + + // Leaving this in case of legacy databases $query['casDateField'] = str_replace('membership_', 'e.', $schedule->start_action_date); + // Options currently are just 'join_date', 'start_date', and 'end_date': + // they need an alias + if (strpos($query['casDateField'], 'e.') !== 0) { + $query['casDateField'] = 'e.' . $query['casDateField']; + } + // FIXME: Numbers should be constants. if (in_array(2, $selectedStatuses)) { //auto-renew memberships @@ -113,11 +121,28 @@ class CRM_Member_ActionMapping extends \Civi\ActionSchedule\Mapping { ->param('memberTypeValues', $selectedValues); } else { + // FIXME: The membership type is never null, so nobody will ever get a + // reminder if no membership types are selected. Either this should be a + // validation on the reminder form or all types should get a reminder if + // no types are selected. $query->where("e.membership_type_id IS NULL"); } + // FIXME: This makes a lot of sense for renewal reminders, but a user + // scheduling another kind of reminder might not expect members to be + // excluded if they have status overrides. Ideally there would be some kind + // of setting per reminder. $query->where("( e.is_override IS NULL OR e.is_override = 0 )"); + + // FIXME: Similarly to overrides, excluding contacts who can't edit the + // primary member makes sense in the context of renewals (see CRM-11342) but + // would be a surprise for other use cases. $query->merge($this->prepareMembershipPermissionsFilter()); + + // FIXME: A lot of undocumented stuff happens with regard to + // `is_current_member`, and this is no exception. Ideally there would be an + // opportunity to pick statuses when setting up the scheduled reminder + // rather than making the assumptions here. $query->where("e.status_id IN (#memberStatus)") ->param('memberStatus', \CRM_Member_PseudoConstant::membershipStatus(NULL, "(is_current_member = 1 OR name = 'Expired')", 'id')); @@ -130,39 +155,22 @@ class CRM_Member_ActionMapping extends \Civi\ActionSchedule\Mapping { } /** - * @return array + * Filter out the memberships that are inherited from a contact that the + * recipient cannot edit. + * + * @return CRM_Utils_SQL_Select */ protected function prepareMembershipPermissionsFilter() { - $query = ' -SELECT cm.id AS owner_id, cm.contact_id AS owner_contact, m.id AS slave_id, m.contact_id AS slave_contact, cmt.relationship_type_id AS relation_type, rel.contact_id_a, rel.contact_id_b, rel.is_permission_a_b, rel.is_permission_b_a -FROM civicrm_membership m -LEFT JOIN civicrm_membership cm ON cm.id = m.owner_membership_id -LEFT JOIN civicrm_membership_type cmt ON cmt.id = m.membership_type_id -LEFT JOIN civicrm_relationship rel ON ( ( rel.contact_id_a = m.contact_id AND rel.contact_id_b = cm.contact_id AND rel.relationship_type_id = cmt.relationship_type_id ) - OR ( rel.contact_id_a = cm.contact_id AND rel.contact_id_b = m.contact_id AND rel.relationship_type_id = cmt.relationship_type_id ) ) -WHERE m.owner_membership_id IS NOT NULL AND - ( rel.is_permission_a_b = 0 OR rel.is_permission_b_a = 0) - -'; - $excludeIds = array(); - $dao = \CRM_Core_DAO::executeQuery($query, array()); - while ($dao->fetch()) { - if ($dao->slave_contact == $dao->contact_id_a && $dao->is_permission_a_b == 0) { - $excludeIds[] = $dao->slave_contact; - } - elseif ($dao->slave_contact == $dao->contact_id_b && $dao->is_permission_b_a == 0) { - $excludeIds[] = $dao->slave_contact; - } - } - - if (!empty($excludeIds)) { - return \CRM_Utils_SQL_Select::fragment() - ->where("!casContactIdField NOT IN (#excludeMemberIds)") - ->param(array( - '#excludeMemberIds' => $excludeIds, - )); - } - return NULL; + $joins = [ + 'cm' => 'LEFT JOIN civicrm_membership cm ON cm.id = e.owner_membership_id', + 'rela' => 'LEFT JOIN civicrm_relationship rela ON rela.contact_id_a = e.contact_id AND rela.contact_id_b = cm.contact_id AND rela.is_permission_a_b = #editPerm', + 'relb' => 'LEFT JOIN civicrm_relationship relb ON relb.contact_id_a = cm.contact_id AND relb.contact_id_b = e.contact_id AND relb.is_permission_b_a = #editPerm', + ]; + + return \CRM_Utils_SQL_Select::fragment() + ->join(NULL, $joins) + ->param('#editPerm', CRM_Contact_BAO_Relationship::EDIT) + ->where('!( e.owner_membership_id IS NOT NULL AND rela.id IS NULL and relb.id IS NULL )'); } } diff --git a/Civi/ActionSchedule/RecipientBuilder.php b/Civi/ActionSchedule/RecipientBuilder.php index ded78c660f..86c383ee64 100644 --- a/Civi/ActionSchedule/RecipientBuilder.php +++ b/Civi/ActionSchedule/RecipientBuilder.php @@ -314,7 +314,7 @@ class RecipientBuilder { $addlCheck = \CRM_Utils_SQL_Select::from($query['casAddlCheckFrom']) ->select('*') - ->merge($query, array('params', 'wheres'))// why only where? why not the joins? + ->merge($query, array('params', 'wheres', 'joins')) ->merge($this->prepareRepetitionEndFilter($query['casDateField'])) ->limit(1) ->strict() diff --git a/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php b/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php index 23be104e54..b653f8f07c 100644 --- a/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php +++ b/tests/phpunit/CRM/Core/BAO/ActionScheduleTest.php @@ -223,6 +223,35 @@ class CRM_Core_BAO_ActionScheduleTest extends CiviUnitTestCase { 'start_action_unit' => 'week', 'subject' => 'subject sched_membership_join_2week (joined {membership.join_date})', ); + $this->fixtures['sched_membership_start_1week'] = array( + 'name' => 'sched_membership_start_1week', + 'title' => 'sched_membership_start_1week', + 'absolute_date' => '', + 'body_html' => '

body sched_membership_start_1week

', + 'body_text' => 'body sched_membership_start_1week', + 'end_action' => '', + 'end_date' => '', + 'end_frequency_interval' => '', + 'end_frequency_unit' => '', + 'entity_status' => '', + 'entity_value' => '', + 'group_id' => '', + 'is_active' => 1, + 'is_repeat' => '0', + 'mapping_id' => 4, + 'msg_template_id' => '', + 'recipient' => '', + 'recipient_listing' => '', + 'recipient_manual' => '', + 'record_activity' => 1, + 'repetition_frequency_interval' => '', + 'repetition_frequency_unit' => '', + 'start_action_condition' => 'after', + 'start_action_date' => 'membership_start_date', + 'start_action_offset' => '1', + 'start_action_unit' => 'week', + 'subject' => 'subject sched_membership_start_1week (joined {membership.start_date})', + ); $this->fixtures['sched_membership_end_2week'] = array( 'name' => 'sched_membership_end_2week', 'title' => 'sched_membership_end_2week', @@ -969,39 +998,57 @@ class CRM_Core_BAO_ActionScheduleTest extends CiviUnitTestCase { * an email should be sent. */ public function testMembershipDateMatch() { - foreach (['membership_join_date', 'membership_start_date'] as $date) { - $membership = $this->createTestObject('CRM_Member_DAO_Membership', array_merge($this->fixtures['rolling_membership'], array('status_id' => 1))); - $this->assertTrue(is_numeric($membership->id)); - $result = $this->callAPISuccess('Email', 'create', array( - 'contact_id' => $membership->contact_id, - 'email' => 'test-member@example.com', - 'location_type_id' => 1, - )); - $this->assertAPISuccess($result); + $membership = $this->createTestObject('CRM_Member_DAO_Membership', array_merge($this->fixtures['rolling_membership'], array('status_id' => 1))); + $this->assertTrue(is_numeric($membership->id)); + $result = $this->callAPISuccess('Email', 'create', array( + 'contact_id' => $membership->contact_id, + 'email' => 'test-member@example.com', + 'location_type_id' => 1, + 'is_primary' => 1, + )); - $this->callAPISuccess('contact', 'create', array_merge($this->fixtures['contact'], array('contact_id' => $membership->contact_id))); - $actionSchedule = $this->fixtures['sched_membership_join_2week']; - $actionSchedule['start_action_date'] = $date; - $actionSchedule['entity_value'] = $membership->membership_type_id; - $actionScheduleDao = CRM_Core_BAO_ActionSchedule::add($actionSchedule); - $this->assertTrue(is_numeric($actionScheduleDao->id)); + $this->callAPISuccess('contact', 'create', array_merge($this->fixtures['contact'], array('contact_id' => $membership->contact_id))); + $actionSchedule = $this->fixtures['sched_membership_join_2week']; + $actionSchedule['entity_value'] = $membership->membership_type_id; + $actionScheduleDao = CRM_Core_BAO_ActionSchedule::add($actionSchedule); + $this->assertTrue(is_numeric($actionScheduleDao->id)); - // start_date=2012-03-15 ; schedule is 2 weeks after start_date - $this->assertCronRuns(array( - array( - // Before the 2-week mark, no email. - 'time' => '2012-03-28 01:00:00', - 'recipients' => array(), - 'subjects' => array(), - ), - array( - // After the 2-week mark, send an email. - 'time' => '2012-03-29 01:00:00', - 'recipients' => array(array('test-member@example.com')), - 'subjects' => array('subject sched_membership_join_2week (joined March 15th, 2012)'), - ), - )); - } + // start_date=2012-03-15 ; schedule is 2 weeks after join_date + $this->assertCronRuns(array( + array( + // Before the 2-week mark, no email. + 'time' => '2012-03-28 01:00:00', + 'recipients' => array(), + 'subjects' => array(), + ), + array( + // After the 2-week mark, send an email. + 'time' => '2012-03-29 01:00:00', + 'recipients' => array(array('test-member@example.com')), + 'subjects' => array('subject sched_membership_join_2week (joined March 15th, 2012)'), + ), + )); + + $actionSchedule = $this->fixtures['sched_membership_start_1week']; + $actionSchedule['entity_value'] = $membership->membership_type_id; + $actionScheduleDao = CRM_Core_BAO_ActionSchedule::add($actionSchedule); + $this->assertTrue(is_numeric($actionScheduleDao->id)); + + // start_date=2012-03-15 ; schedule is 1 weeks after start_date + $this->assertCronRuns(array( + array( + // Before the 2-week mark, no email. + 'time' => '2012-03-21 01:00:00', + 'recipients' => array(), + 'subjects' => array(), + ), + array( + // After the 2-week mark, send an email. + 'time' => '2012-03-22 01:00:00', + 'recipients' => array(array('test-member@example.com')), + 'subjects' => array('subject sched_membership_start_1week (joined March 15th, 2012)'), + ), + )); } @@ -2005,6 +2052,123 @@ class CRM_Core_BAO_ActionScheduleTest extends CiviUnitTestCase { } } + /** + * Inherited members without permission to edit the main member contact should + * not get reminders. + * + * However, just because a contact inherits one membership doesn't mean + * reminders for other memberships should be suppressed. + * + * See CRM-14098 + */ + public function testInheritedMembershipPermissions() { + // Set up common parameters for memberships. + $membershipParams = $this->fixtures['rolling_membership']; + $membershipParams['status_id'] = 1; + + $membershipParams['membership_type_id']['relationship_type_id'] = 1; + $membershipParams['membership_type_id']['relationship_direction'] = 'b_a'; + $membershipType1 = $this->createTestObject('CRM_Member_DAO_MembershipType', $membershipParams['membership_type_id']); + + // We'll create a new membership type that can be held at the same time as + // the first one. + $membershipParams['membership_type_id']['relationship_type_id'] = 'NULL'; + $membershipParams['membership_type_id']['relationship_direction'] = 'NULL'; + $membershipType2 = $this->createTestObject('CRM_Member_DAO_MembershipType', $membershipParams['membership_type_id']); + + // Create the parent membership and contact + $membershipParams['membership_type_id'] = $membershipType1->id; + $mainMembership = $this->createTestObject('CRM_Member_DAO_Membership', $membershipParams); + + $contactParams = [ + 'contact_type' => 'Individual', + 'first_name' => 'Mom', + 'last_name' => 'Rel', + 'is_deceased' => 0, + ]; + $this->createTestObject('CRM_Contact_DAO_Contact', array_merge($contactParams, ['id' => $mainMembership->contact_id])); + + $emailParams = [ + 'contact_id' => $mainMembership->contact_id, + 'email' => 'test-member@example.com', + 'location_type_id' => 1, + 'is_primary' => 1, + ]; + $email = $this->createTestObject('CRM_Core_DAO_Email', $emailParams); + + // Set up contacts and emails for the two children + $contactParams['first_name'] = 'Favorite'; + $permChild = $this->createTestObject('CRM_Contact_DAO_Contact', $contactParams); + $emailParams['email'] = 'favorite@example.com'; + $emailParams['contact_id'] = $permChild->id; + $this->createTestObject('CRM_Core_DAO_Email', $emailParams); + + $contactParams['first_name'] = 'Black Sheep'; + $nonPermChild = $this->createTestObject('CRM_Contact_DAO_Contact', $contactParams); + $emailParams['email'] = 'black.sheep@example.com'; + $emailParams['contact_id'] = $nonPermChild->id; + $this->createTestObject('CRM_Core_DAO_Email', $emailParams); + + // Each child gets a relationship, one with permission to edit the parent. This + // will trigger inherited memberships for the first membership type + $relParams = [ + 'relationship_type_id' => 1, + 'contact_id_a' => $nonPermChild->id, + 'contact_id_b' => $mainMembership->contact_id, + 'is_active' => 1, + ]; + $this->callAPISuccess('relationship', 'create', $relParams); + + $relParams['contact_id_a'] = $permChild->id; + $relParams['is_permission_a_b'] = CRM_Contact_BAO_Relationship::EDIT; + $this->callAPISuccess('relationship', 'create', $relParams); + + // Mom and Black Sheep get their own memberships of the second type. + $membershipParams['membership_type_id'] = $membershipType2->id; + $membershipParams['owner_membership_id'] = 'NULL'; + $membershipParams['contact_id'] = $mainMembership->contact_id; + $this->createTestObject('CRM_Member_DAO_Membership', $membershipParams); + + $membershipParams['contact_id'] = $nonPermChild->id; + $this->createTestObject('CRM_Member_DAO_Membership', $membershipParams); + + // Test a reminder for the first membership type - that should exclude Black + // Sheep. + $actionSchedule = $this->fixtures['sched_membership_join_2week']; + $actionSchedule['entity_value'] = $membershipType1->id; + $actionScheduleDao = CRM_Core_BAO_ActionSchedule::add($actionSchedule); + $this->assertTrue(is_numeric($actionScheduleDao->id)); + + $this->assertCronRuns([ + [ + 'time' => '2012-03-29 01:00:00', + 'recipients' => [['test-member@example.com'], ['favorite@example.com']], + 'subjects' => [ + 'subject sched_membership_join_2week (joined March 15th, 2012)', + 'subject sched_membership_join_2week (joined March 15th, 2012)', + ], + ], + ]); + + // Test a reminder for the second membership type - that should include + // Black Sheep. + $actionSchedule = $this->fixtures['sched_membership_start_1week']; + $actionSchedule['entity_value'] = $membershipType2->id; + $actionScheduleDao = CRM_Core_BAO_ActionSchedule::add($actionSchedule); + $this->assertTrue(is_numeric($actionScheduleDao->id)); + + $this->assertCronRuns([ + [ + 'time' => '2012-03-22 01:00:00', + 'recipients' => [['test-member@example.com'], ['black.sheep@example.com']], + 'subjects' => [ + 'subject sched_membership_start_1week (joined March 15th, 2012)', + 'subject sched_membership_start_1week (joined March 15th, 2012)', + ], + ], + ]); + } + public function createModifiedDateTime($origDateTime, $modifyRule) { $newDateTime = clone($origDateTime); $newDateTime->modify($modifyRule); -- 2.25.1