From: eileen <emcnaughton@wikimedia.org>
Date: Mon, 24 Aug 2020 03:22:19 +0000 (+1200)
Subject: Fix for ongoing issues with static upsetting the apple cart
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=696737b5b83f5986edf19b7208e43cde253f2bdb;p=civicrm-core.git

Fix for ongoing issues with static upsetting the apple cart

This is an alternate to https://github.com/civicrm/civicrm-core/pull/18218

I managed to replicate the bug in https://issues.civicrm.org/jira/browse/CRM-4213 that the static
was added to cover. This takes an alternate approach of loading the memberships & checking them
rather than a static. I think it might mean more queries but they shouldn't be expensive queries & the
code is just too hard-going to skip straight from hacked but still intermittantly buggy to also having no additional
queries
---

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index a74d99bc57..9b68859709 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -5261,11 +5261,6 @@ LIMIT 1;";
       //so make status override false.
       $membershipParams['is_override'] = FALSE;
       $membershipParams['status_override_end_date'] = 'null';
-
-      //CRM-17723 - reset static $relatedContactIds array()
-      // @todo move it to Civi Statics.
-      $var = TRUE;
-      CRM_Member_BAO_Membership::createRelatedMemberships($var, $var, TRUE);
       civicrm_api3('Membership', 'create', $membershipParams);
     }
   }
diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php
index 1030eaae1c..3ccda33cf0 100644
--- a/CRM/Member/BAO/Membership.php
+++ b/CRM/Member/BAO/Membership.php
@@ -1316,25 +1316,10 @@ WHERE  civicrm_membership.contact_id = civicrm_contact.id
    * @param CRM_Core_DAO $dao
    *   Membership object.
    *
-   * @param bool $reset
-   *
-   * @return array|null
-   *   Membership details, if created.
-   *
    * @throws \CRM_Core_Exception
    * @throws \CiviCRM_API3_Exception
    */
-  public static function createRelatedMemberships($params, $dao, $reset = FALSE) {
-    // CRM-4213 check for loops, using static variable to record contacts already processed.
-    if (!isset(\Civi::$statics[__CLASS__]['related_contacts'])) {
-      \Civi::$statics[__CLASS__]['related_contacts'] = [];
-    }
-    if ($reset) {
-      // CRM-17723.
-      unset(\Civi::$statics[__CLASS__]['related_contacts']);
-      return FALSE;
-    }
-    $relatedContactIds = &\Civi::$statics[__CLASS__]['related_contacts'];
+  public static function createRelatedMemberships($params, $dao) {
 
     $membership = new CRM_Member_DAO_Membership();
     $membership->id = $dao->id;
@@ -1371,9 +1356,9 @@ WHERE  civicrm_membership.contact_id = civicrm_contact.id
     );
 
     // CRM-4213, CRM-19735 check for loops, using static variable to record contacts already processed.
-    // Remove repeated related contacts, which already inherited membership of this type.
-    $relatedContactIds[$membership->contact_id][$membership->membership_type_id] = TRUE;
+    // Remove repeated related contacts, which already inherited membership of this type$relatedContactIds[$membership->contact_id][$membership->membership_type_id] = TRUE;
     foreach ($allRelatedContacts as $cid => $status) {
+      // relatedContactIDs is always empty now - will remove next roud because of whitespace readability.
       if (empty($relatedContactIds[$cid]) || empty($relatedContactIds[$cid][$membership->membership_type_id])) {
         $relatedContactIds[$cid][$membership->membership_type_id] = TRUE;
 
@@ -1470,7 +1455,9 @@ WHERE  civicrm_membership.contact_id = civicrm_contact.id
         $ids = [];
         if (($params['status_id'] == $deceasedStatusId) || ($params['status_id'] == $expiredStatusId)) {
           // related membership is not active so does not count towards maximum
-          CRM_Member_BAO_Membership::create($params);
+          if (!self::hasExistingInheritedMembership($params)) {
+            CRM_Member_BAO_Membership::create($params);
+          }
         }
         else {
           // related membership already exists, so this is just an update
@@ -1495,7 +1482,9 @@ WHERE  civicrm_membership.contact_id = civicrm_contact.id
         if ($numRelatedAvailable <= 0) {
           break;
         }
-        CRM_Member_BAO_Membership::create($params);
+        if (!self::hasExistingInheritedMembership($params)) {
+          CRM_Member_BAO_Membership::create($params);
+        }
         $numRelatedAvailable--;
       }
     }
@@ -2092,6 +2081,58 @@ INNER JOIN  civicrm_contact contact ON ( contact.id = membership.contact_id AND
     return $count;
   }
 
+  /**
+   * Does the existing membership match the required membership.
+   *
+   * Check before updating that the params are not a match - this is part of avoiding
+   * a loop if we have already updated.
+   *
+   * https://issues.civicrm.org/jira/browse/CRM-4213
+   * @param array $params
+   *
+   * @param array $membership
+   *
+   * @return bool
+   */
+  protected static function matchesRequiredMembership($params, $membership) {
+    foreach (['start_date', 'end_date'] as $date) {
+      if (strtotime($params[$date]) !== strtotime($membership[$date])) {
+        return FALSE;
+      }
+      if ((int) $params['status_id'] !== (int) $membership['status_id']) {
+        return FALSE;
+      }
+      if ((int) $params['membership_type_id'] !== (int) $membership['membership_type_id']) {
+        return FALSE;
+      }
+    }
+    return TRUE;
+  }
+
+  /**
+   * Params of new membership.
+   *
+   * @param array $params
+   *
+   * @return bool
+   * @throws \CiviCRM_API3_Exception
+   */
+  protected static function hasExistingInheritedMembership($params) {
+    foreach (civicrm_api3('Membership', 'get', ['contact_id' => $params['contact_id']])['values'] as $membership) {
+      if (!empty($membership['owner_membership_id'])
+        && $membership['membership_type_id'] === $params['membership_type_id']
+        && (int) $params['owner_membership_id'] !== (int) $membership['owner_membership_id']
+      ) {
+        // Inheriting it from another contact, don't update here.
+        return TRUE;
+      }
+      if (self::matchesRequiredMembership($params, $membership)) {
+        return TRUE;
+      }
+    }
+    return FALSE;
+  }
+
   /**
    * Process price set and line items.
    *
diff --git a/tests/phpunit/CRM/Member/Form/MembershipTest.php b/tests/phpunit/CRM/Member/Form/MembershipTest.php
index ef0d0e99bd..79730e5b92 100644
--- a/tests/phpunit/CRM/Member/Form/MembershipTest.php
+++ b/tests/phpunit/CRM/Member/Form/MembershipTest.php
@@ -1023,6 +1023,7 @@ Expires: ',
    * Uses some data from tests/phpunit/CRM/Member/Form/dataset/data.xml .
    *
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public function testTwoInheritedMembershipsViaPriceSetInBackend() {
     // Create an organization and give it a "Member of" relationship to $this->_individualId.
@@ -1045,7 +1046,7 @@ Expires: ',
     $primaryMembershipIds = [];
     foreach ($orgMembershipResult['values'] as $membership) {
       $primaryMembershipIds[] = $membership['id'];
-      $this->assertTrue(empty($membership['owner_membership_id']), "Membership on the organization has owner_membership_id so is inherited.");
+      $this->assertTrue(empty($membership['owner_membership_id']), 'Membership on the organization has owner_membership_id so is inherited.');
     }
 
     // CRM-20955: check that correct inherited memberships were created for the individual,
@@ -1100,6 +1101,7 @@ Expires: ',
    * checking that the line items have correct amounts.
    *
    * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
   public function testTwoMembershipsViaPriceSetInBackendWithDiscount() {
     // Register buildAmount hook to apply discount.
diff --git a/tests/phpunit/api/v3/JobTest.php b/tests/phpunit/api/v3/JobTest.php
index f042fa9585..6e30be943c 100644
--- a/tests/phpunit/api/v3/JobTest.php
+++ b/tests/phpunit/api/v3/JobTest.php
@@ -1990,7 +1990,6 @@ class api_v3_JobTest extends CiviUnitTestCase {
    * and left alone when it shouldn't.
    *
    * @throws \CRM_Core_Exception
-   * @throws \CiviCRM_API3_Exception
    */
   public function testProcessMembershipUpdateStatus() {
     $this->ids['MembershipType'] = $this->membershipTypeCreate();
@@ -2107,10 +2106,6 @@ class api_v3_JobTest extends CiviUnitTestCase {
     $this->assertEquals($organizationMembershipID, $expiredInheritedRelationship['owner_membership_id']);
     $this->assertMembershipStatus('Grace', (int) $expiredInheritedRelationship['status_id']);
 
-    // Reset static $relatedContactIds array in createRelatedMemberships(),
-    // to avoid bug where inherited membership gets deleted.
-    $var = TRUE;
-    CRM_Member_BAO_Membership::createRelatedMemberships($var, $var, TRUE);
     // Check that after running process_membership job, statuses are correct.
     $this->callAPISuccess('Job', 'process_membership', []);
 
diff --git a/tests/phpunit/api/v3/MembershipTest.php b/tests/phpunit/api/v3/MembershipTest.php
index 7e03eee9a3..8f6171d49c 100644
--- a/tests/phpunit/api/v3/MembershipTest.php
+++ b/tests/phpunit/api/v3/MembershipTest.php
@@ -9,6 +9,10 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\Api4\MembershipType;
+use Civi\Api4\Relationship;
+use Civi\Api4\RelationshipType;
+
 /**
  *  Test APIv3 civicrm_membership functions
  *
@@ -495,7 +499,6 @@ class api_v3_MembershipTest extends CiviUnitTestCase {
    * and max_related property for Membership_Type and Membership entities
    *
    * @throws \CRM_Core_Exception
-   * @throws \CiviCRM_API3_Exception
    */
   public function testCreateWithRelationship() {
     // Create membership type: inherited through employment, max_related = 2
@@ -667,6 +670,75 @@ class api_v3_MembershipTest extends CiviUnitTestCase {
     $this->contactDelete($membershipOrgId);
   }
 
+  /**
+   * Test that loops are not created when adding spouse relationships.
+   *
+   * This add a test for https://issues.civicrm.org/jira/browse/CRM-4213 in the hope of removing
+   * the buggy fix for that without a resurgence.
+   *
+   * @throws \API_Exception
+   * @throws \CRM_Core_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
+   */
+  public function testCreateWithSpouseRelationship() {
+    $relationshipTypeID = RelationshipType::get()->addSelect('id')->addWhere('name_a_b', '=', 'Spouse of')->execute()->first()['id'];
+    MembershipType::update()->setValues([
+      'relationship_direction' => ['b_a', 'a_b'],
+      'relationship_type_id' => [$relationshipTypeID, $relationshipTypeID],
+    ])
+      ->addWhere('name', '=', 'General')
+      ->execute()->first()['id'];
+
+    $spouse1ID = $this->individualCreate(['first_name' => 'him']);
+    $spouse2ID = $this->individualCreate(['first_name' => 'her']);
+    $spouse3ID = $this->individualCreate(['first_name' => 'they']);
+    $spouse4ID = $this->individualCreate(['first_name' => 'them']);
+    Relationship::create()->setValues([
+      'contact_id_a' => $spouse1ID,
+      'contact_id_b' => $spouse2ID,
+      'relationship_type_id' => $relationshipTypeID,
+    ])->execute();
+
+    $this->contactMembershipCreate([
+      'contact_id' => $spouse1ID,
+      'start_date' => date('Y-m-d'),
+      'end_date' => '+1 year',
+    ]);
+
+    $this->callAPISuccessGetSingle('Membership', [
+      'contact_id' => $spouse2ID,
+      'membership_type_id' => 'General',
+    ]);
+
+    $this->callAPISuccessGetSingle('Membership', [
+      'contact_id' => $spouse1ID,
+      'membership_type_id' => 'General',
+    ]);
+    // Add another Spouse
+    Relationship::create()->setValues([
+      'contact_id_a' => $spouse3ID,
+      'contact_id_b' => $spouse1ID,
+      'relationship_type_id' => $relationshipTypeID,
+    ])->execute();
+    $this->callAPISuccessGetSingle('Membership', [
+      'contact_id' => $spouse3ID,
+      'membership_type_id' => 'General',
+    ]);
+    $this->callAPISuccessGetCount('Membership', [], 3);
+    Relationship::create()->setValues([
+      'contact_id_a' => $spouse1ID,
+      'contact_id_b' => $spouse4ID,
+      'relationship_type_id' => $relationshipTypeID,
+    ])->execute();
+
+    $this->callAPISuccessGetSingle('Membership', [
+      'contact_id' => $spouse4ID,
+      'membership_type_id' => 'General',
+    ]);
+
+    $this->callAPISuccessGetCount('Membership', [], 4);
+  }
+
   /**
    * We are checking for no e-notices + only id & end_date returned
    *