From 6345c936fcb87bc3fac0f92dc6e7d13caddaae32 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 6 Nov 2019 04:32:08 +1300 Subject: [PATCH] dev/core#1361 Fix bug where 2 memberships created for same relationship Fixes a bug (likely recent regression) identified by @agileware-pengyi where duplicate inherited relationships for the same membership were being created I've been pretty much sweating blood trying to understand the code, & have now resorted to adding a class to generate a saner data set --- CRM/Contact/BAO/Relationship.php | 59 ++----- CRM/Member/Utils/RelationshipProcessor.php | 156 ++++++++++++++++++ .../CRM/Contact/BAO/RelationshipTest.php | 4 +- 3 files changed, 167 insertions(+), 52 deletions(-) create mode 100644 CRM/Member/Utils/RelationshipProcessor.php diff --git a/CRM/Contact/BAO/Relationship.php b/CRM/Contact/BAO/Relationship.php index 8b60404760..6f3ee6fdbc 100644 --- a/CRM/Contact/BAO/Relationship.php +++ b/CRM/Contact/BAO/Relationship.php @@ -1595,7 +1595,7 @@ LEFT JOIN civicrm_country ON (civicrm_address.country_id = civicrm_country.id) $contact = $contactId; } elseif ($action & CRM_Core_Action::UPDATE) { - $contact = $ids['contact']; + $contact = (int) $ids['contact']; $targetContact = [$ids['contactTarget'] => 1]; } @@ -1634,57 +1634,17 @@ LEFT JOIN civicrm_country ON (civicrm_address.country_id = civicrm_country.id) } } - // CRM-15829 UPDATES - // If we're looking for active memberships we must consider pending (id: 5) ones too. - // Hence we can't just call CRM_Member_BAO_Membership::getValues below with the active flag, is it would completely miss pending relatioships. - // As suggested by @davecivicrm, the pending status id is fetched using the CRM_Member_PseudoConstant::membershipStatus() class and method, since these ids differ from system to system. - $pendingStatusId = array_search('Pending', CRM_Member_PseudoConstant::membershipStatus()); - - $query = 'SELECT * FROM `civicrm_membership_status`'; - if ($active) { - $query .= ' WHERE `is_current_member` = 1 OR `id` = %1 '; - } - - $dao = CRM_Core_DAO::executeQuery($query, [1 => [$pendingStatusId, 'Integer']]); - - while ($dao->fetch()) { - $membershipStatusRecordIds[$dao->id] = $dao->id; - } $deceasedStatusId = array_search('Deceased', CRM_Member_PseudoConstant::membershipStatus()); - // Now get the active memberships for all the contacts. - // If contact have any valid membership(s), then add it to - // 'values' array. - foreach ($values as $cid => $subValues) { - $memberships = civicrm_api3('Membership', 'get', [ - 'contact_id' => $cid, - 'status_id' => ['IN' => $membershipStatusRecordIds], - ])['values']; - - foreach ($memberships as $key => $membership) { - //get ownerMembershipIds for related Membership - //this is to handle memberships being deleted and recreated - if (!empty($membership['owner_membership_id'])) { - $ownerMemIds[$cid] = $membership['owner_membership_id']; - } - } - $values[$cid]['memberships'] = $memberships; - } - - // done with 'values' array. - // Finally add / edit / delete memberships for the related contacts - + $relationshipProcessor = new CRM_Member_Utils_RelationshipProcessor(array_keys($values), $active); foreach ($values as $cid => $details) { - if (!array_key_exists('memberships', $details)) { - continue; - } - $relatedContacts = array_keys(CRM_Utils_Array::value('relatedContacts', $details, [])); $mainRelatedContactId = reset($relatedContacts); - foreach ($details['memberships'] as $membershipId => $membershipValues) { + foreach ($relationshipProcessor->getRelationshipMembershipsForContact((int) $cid) as $membershipId => $membershipValues) { $membershipInherittedFromContactID = NULL; if (!empty($membershipValues['owner_membership_id'])) { + // @todo - $membership already has this now. // Use get not getsingle so that we get e-notice noise but not a fatal is the membership has already been deleted. $inheritedFromMembership = civicrm_api3('Membership', 'get', ['id' => $membershipValues['owner_membership_id'], 'sequential' => 1])['values'][0]; $membershipInherittedFromContactID = (int) $inheritedFromMembership['contact_id']; @@ -1713,6 +1673,7 @@ LEFT JOIN civicrm_country ON (civicrm_address.country_id = civicrm_country.id) // add / edit the memberships for related // contacts. + // @todo - all these lines get 'relTypeDirs' - but it's already a key in the $membership array. // Get the Membership Type Details. $membershipType = CRM_Member_BAO_MembershipType::getMembershipType($membershipValues['membership_type_id']); // Check if contact's relationship type exists in membership type @@ -1745,11 +1706,11 @@ LEFT JOIN civicrm_country ON (civicrm_address.country_id = civicrm_country.id) $membershipValues['skipStatusCal'] = TRUE; } - if ($action & CRM_Core_Action::UPDATE) { + if (in_array($action, [CRM_Core_Action::UPDATE, CRM_Core_Action::ADD, CRM_Core_Action::ENABLE])) { //if updated relationship is already related to contact don't delete existing inherited membership - if (in_array($relTypeId, $relTypeIds - ) && !empty($values[$relatedContactId]['memberships']) && !empty($ownerMemIds - ) && in_array($membershipValues['owner_membership_id'], $ownerMemIds[$relatedContactId])) { + if (in_array((int) $relatedContactId, $membershipValues['inheriting_contact_ids'], TRUE) + || $relatedContactId === $membershipValues['owner_contact_id'] + ) { continue; } @@ -1758,7 +1719,7 @@ LEFT JOIN civicrm_country ON (civicrm_address.country_id = civicrm_country.id) CRM_Member_BAO_Membership::deleteRelatedMemberships($membershipId, $relatedContactId); } //skip status calculation for pay later memberships. - if (!empty($membershipValues['status_id']) && $membershipValues['status_id'] == $pendingStatusId) { + if ('Pending' === CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $membershipValues['status_id'])) { $membershipValues['skipStatusCal'] = TRUE; } // As long as the membership itself was not created by inheritance from the same contact diff --git a/CRM/Member/Utils/RelationshipProcessor.php b/CRM/Member/Utils/RelationshipProcessor.php new file mode 100644 index 0000000000..3e8ea01e82 --- /dev/null +++ b/CRM/Member/Utils/RelationshipProcessor.php @@ -0,0 +1,156 @@ +contactIDs = $contactIDs; + $this->active = $active; + $this->setMemberships(); + } + + /** + * Get memberships for contact of potentially inheritable types. + * + * @param int $contactID + * + * @return array + */ + public function getRelationshipMembershipsForContact(int $contactID):array { + $memberships = []; + foreach ($this->memberships as $id => $membership) { + if ((int) $membership['contact_id'] === $contactID) { + $memberships[$id] = $membership; + } + } + return $memberships; + } + + /** + * Set the relevant memberships on the class. + * + * We are looking at relationships that are potentially inheritable + * so we can filter out membership types with NULL relationship_type_id + * + * @throws \CiviCRM_API3_Exception + */ + protected function setMemberships() { + $return = array_keys(civicrm_api3('Membership', 'getfields', [])['values']); + $return[] = 'owner_membership_id.contact_id'; + $return[] = 'membership_type_id.relationship_type_id'; + $return[] = 'membership_type_id.relationship_direction'; + $memberships = civicrm_api3('Membership', 'get', [ + 'contact_id' => ['IN' => $this->contactIDs], + 'status_id' => ['IN' => $this->getInheritableMembershipStatusIDs()], + 'membership_type_id.relationship_type_id' => ['IS NOT NULL' => TRUE], + 'return' => $return, + 'options' => ['limit' => 0], + ])['values']; + foreach ($memberships as $id => $membership) { + if (!isset($membership['inheriting_membership_ids'])) { + $memberships[$id]['inheriting_membership_ids'] = []; + $memberships[$id]['inheriting_contact_ids'] = []; + } + if (!empty($membership['owner_membership_id']) && isset($memberships[$membership['owner_membership_id']])) { + $memberships[$membership['owner_membership_id']]['inheriting_membership_ids'][] = (int) $membership['id']; + $memberships[$membership['owner_membership_id']]['inheriting_contact_ids'][] = (int) $membership['contact_id']; + $membership['owner_membership_id.contact_id'] = (int) $membership['owner_membership_id.contact_id']; + } + // Just for the sake of having an easier parameter to access. + $memberships[$id]['owner_contact_id'] = $membership['owner_membership_id.contact_id'] ?? NULL; + + // Ensure it is an array & use an easier parameter name. + $memberships[$id]['relationship_type_ids'] = (array) $membership['membership_type_id.relationship_type_id']; + $memberships[$id]['relationship_type_directions'] = (array) $membership['membership_type_id.relationship_direction']; + + foreach ($memberships[$id]['relationship_type_ids'] as $index => $relationshipType) { + $memberships[$id]['relationship_type_keys'][] = $relationshipType . '_' . $memberships[$id]['relationship_type_directions'][$index]; + } + } + $this->memberships = $memberships; + } + + /** + * Get membership statuses that could be inherited. + * + * @return array + */ + protected function getInheritableMembershipStatusIDs() { + // @todo - clean this up - was legacy code that got moved. + $membershipStatusRecordIds = []; + // CRM-15829 UPDATES + // If we're looking for active memberships we must consider pending (id: 5) ones too. + // Hence we can't just call CRM_Member_BAO_Membership::getValues below with the active flag, is it would completely miss pending relatioships. + // As suggested by @davecivicrm, the pending status id is fetched using the CRM_Member_PseudoConstant::membershipStatus() class and method, since these ids differ from system to system. + $pendingStatusId = array_search('Pending', CRM_Member_PseudoConstant::membershipStatus()); + + $query = 'SELECT * FROM `civicrm_membership_status`'; + if ($this->active) { + $query .= ' WHERE `is_current_member` = 1 OR `id` = %1 '; + } + + $dao = CRM_Core_DAO::executeQuery($query, [1 => [$pendingStatusId, 'Integer']]); + + while ($dao->fetch()) { + $membershipStatusRecordIds[$dao->id] = $dao->id; + } + return $membershipStatusRecordIds; + } + +} diff --git a/tests/phpunit/CRM/Contact/BAO/RelationshipTest.php b/tests/phpunit/CRM/Contact/BAO/RelationshipTest.php index 3bbd6ac1cb..4171372004 100644 --- a/tests/phpunit/CRM/Contact/BAO/RelationshipTest.php +++ b/tests/phpunit/CRM/Contact/BAO/RelationshipTest.php @@ -276,8 +276,6 @@ class CRM_Contact_BAO_RelationshipTest extends CiviUnitTestCase { $this->callAPISuccess('Relationship', 'create', $relationshipOne); $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 1); - /* - * @todo this section not yet working due to bug in would-be-tested code. $relationshipTwo['is_active'] = 1; $this->callAPISuccess('Relationship', 'create', $relationshipTwo); $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 1); @@ -285,7 +283,7 @@ class CRM_Contact_BAO_RelationshipTest extends CiviUnitTestCase { $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 1); $this->callAPISuccess('Relationship', 'delete', ['id' => $relationshipOne['id']]); $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 0); - */ + } /** -- 2.25.1