Fix inherited relationship being deleted when there is still a valid …

…relationship

This pulls out some of the good work & test written in civicrm#14410, but treating the parts as separate bugs
with separate cleanup requirements.

I put up cleanup in civicrm#15061

And this addresses the need for the relationship to not be deleted if a valid one still exists
- wrangled out into a separate function

CRM/Contact/BAO/Relationship.php

@@ -756,6 +756,7 @@ public static function del($id) {
    * @param bool $active
    *
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public static function disableEnableRelationship($id, $action, $params = [], $ids = [], $active = FALSE) {
     $relationship = self::clearCurrentEmployer($id, $action);
@@ -773,6 +774,8 @@ public static function disableEnableRelationship($id, $action, $params = [], $id
       // calling relatedMemberships to delete/add the memberships of
       // related contacts.
       if ($action & CRM_Core_Action::DISABLE) {
+        // @todo this could call a subset of the function that just relates to
+        // cleaning up no-longer-inherited relationships
         CRM_Contact_BAO_Relationship::relatedMemberships($contact_id_a,
           $params,
           $ids,
@@ -1512,6 +1515,7 @@ public static function getRelationType($targetContactType) {
    * @param bool $active
    *
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public static function relatedMemberships($contactId, &$params, $ids, $action = CRM_Core_Action::ADD, $active = TRUE) {
     // Check the end date and set the status of the relationship
@@ -1671,7 +1675,7 @@ public static function relatedMemberships($contactId, &$params, $ids, $action =
         $relTypeIds = [];
         if ($action & CRM_Core_Action::DELETE) {
           // @todo don't return relTypeId here - but it seems to be used later in a cryptic way (hint cryptic is not a complement).
-          list($relTypeId, $isDeletable) = self::isInheritedMembershipInvalidated($membershipValues, $values, $cid, $mainRelatedContactId);
+          list($relTypeId, $isDeletable) = self::isInheritedMembershipInvalidated($membershipValues, $values, $cid);
           if ($isDeletable) {
             CRM_Member_BAO_Membership::deleteRelatedMemberships($membershipValues['owner_membership_id'], $membershipValues['membership_contact_id']);
           }
@@ -2318,21 +2322,78 @@ private static function isRelationshipTypeCurrentEmployer(int $existingTypeID):
    * @param $membershipValues
    * @param array $values
    * @param int $cid
-   * @param int $mainRelatedContactId
    *
    * @return array
    * @throws \CiviCRM_API3_Exception
    */
-  private static function isInheritedMembershipInvalidated($membershipValues, array $values, $cid, $mainRelatedContactId): array {
+  private static function isInheritedMembershipInvalidated($membershipValues, array $values, $cid): array {
+    // @todo most of this can go - it's just the weird historical returning of $relTypeId that it does.
+    // now we have caching the parent fn can just call CRM_Member_BAO_MembershipType::getMembershipType
     $membershipType = CRM_Member_BAO_MembershipType::getMembershipType($membershipValues['membership_type_id']);
     $relTypeIds = $membershipType['relationship_type_id'];
-    $membshipInheritedFrom = $membershipValues['owner_membership_id'] ?? NULL;
-    if (!$membshipInheritedFrom || !in_array($values[$cid]['relationshipTypeId'], $relTypeIds)) {
+    $membershipInheritedFrom = $membershipValues['owner_membership_id'] ?? NULL;
+    if (!$membershipInheritedFrom || !in_array($values[$cid]['relationshipTypeId'], $relTypeIds)) {
       return [implode(',', $relTypeIds), FALSE];
     }
     //CRM-16300 check if owner membership exist for related membership
-    $isDeletable = !empty($values[$mainRelatedContactId]['memberships'][$membshipInheritedFrom]);
-    return [implode(',', $relTypeIds), $isDeletable];
+    return [implode(',', $relTypeIds), self::isContactHasValidRelationshipToInheritMembershipType((int) $cid, (int) $membershipValues['membership_type_id'], (int) $membershipValues['owner_membership_id'])];
+  }
+
+  /**
+   * Is there a valid relationship confering this membership type on this contact.
+   *
+   * @param int $contactID
+   * @param int $membershipTypeID
+   * @param int $parentMembershipID
+   *   Id of the membership being inherited.
+   *
+   * @return bool
+   *
+   * @throws \CiviCRM_API3_Exception
+   */
+  private static function isContactHasValidRelationshipToInheritMembershipType(int $contactID, int $membershipTypeID, int $parentMembershipID): bool {
+    $membershipType = CRM_Member_BAO_MembershipType::getMembershipType($membershipTypeID);
+    $existingRelationships = civicrm_api3('Relationship', 'get', [
+      'contact_id_a' => $contactID,
+      'contact_id_b' => $contactID,
+      'relationship_type_id' => ['IN' => $membershipType['relationship_type_id']],
+      'options' => ['or' => [['contact_id_a', 'contact_id_b']], 'limit' => 0],
+      'is_active' => 1,
+    ])['values'];
+
+    if (empty($existingRelationships)) {
+      return FALSE;
+    }
+
+    $membershipInheritedFromContactID = civicrm_api3('Membership', 'getvalue', ['return' => 'contact_id', 'id' => $parentMembershipID]);
+    // I don't think the api can correctly filter by start & end because of handling for NULL
+    // so we filter them out here.
+    foreach ($existingRelationships as $index => $existingRelationship) {
+      if (!empty($existingRelationship['start_date'])
+        && strtotime($existingRelationship['start_date']) > time()
+      ) {
+        unset($existingRelationships[$index]);
+        continue;
+      }
+      if (!empty($existingRelationship['end_date'])
+        && strtotime($existingRelationship['end_date']) < time()
+      ) {
+        unset($existingRelationships[$index]);
+        continue;
+      }
+      if ($membershipInheritedFromContactID !== (int) $membershipType['contact_id_a']
+        && $membershipInheritedFromContactID !== (int) $membershipType['contact_id_b']
+      ) {
+        // This is a weird scenario - they would still be entitled to have the membership
+        // just not from this relationship - and some max_related calcs etc would be required.
+        // For now ignore here & hope it's handled elsewhere - at least that's consistent with
+        // before this function was added.
+        unset($existingRelationships[$index]);
+        continue;
+      }
+
+    }
+    return empty($existingRelationships);
   }
 
 }

tests/phpunit/CRM/Contact/BAO/RelationshipTest.php

@@ -254,16 +254,18 @@ public function testSingleMembershipForTwoRelationships() {
     // Disable the relationship & check the membership is removed.
     $relationshipOne['is_active'] = 0;
     $this->callAPISuccess('Relationship', 'create', array_merge($relationshipOne, ['is_active' => 0]));
-    $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 0);
-    /*
-     * @todo this section not yet working due to bug in would-be-tested code.
+    $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 1);
+
     $relationshipTwo['is_active'] = 0;
     $this->callAPISuccess('Relationship', 'create', $relationshipTwo);
-    $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 1);
+    $this->callAPISuccessGetCount('Membership', ['contact_id' => $individualID], 0);
 
     $relationshipOne['is_active'] = 1;
     $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);