Fix missing membershiplog entries and add tests following #30493

civicrm · Dec 24, 2024 · 525c486 · 525c486
1 parent 72cdaf2
commit 525c486
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 27 deletions.
diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php
@@ -88,30 +88,8 @@ public static function add(&$params) {
     //it is set during renewal of membership.
     $logStartDate = $params['log_start_date'] ?? NULL;
     $logStartDate = ($logStartDate) ? CRM_Utils_Date::isoToMysql($logStartDate) : CRM_Utils_Date::isoToMysql($membership->start_date);
-    $values = self::getStatusANDTypeValues($membership->id);
-
-    $membershipLog = [
-      'membership_id' => $membership->id,
-      'status_id' => $membership->status_id,
-      'start_date' => $logStartDate,
-      'end_date' => CRM_Utils_Date::isoToMysql($membership->end_date),
-      'modified_date' => CRM_Utils_Time::date('Ymd'),
-      'membership_type_id' => $values[$membership->id]['membership_type_id'],
-      'max_related' => $membership->max_related,
-    ];
-
-    if (!empty($params['modified_id'])) {
-      $membershipLog['modified_id'] = $params['modified_id'];
-    }
-    // If we have an authenticated session, set modified_id to that user's contact_id, else set to membership.contact_id
-    elseif (CRM_Core_Session::getLoggedInContactID()) {
-      $membershipLog['modified_id'] = CRM_Core_Session::getLoggedInContactID();
-    }
-    else {
-      $membershipLog['modified_id'] = $membership->contact_id;
-    }
-
-    CRM_Member_BAO_MembershipLog::add($membershipLog);
+    $membershipTypeID = (int) self::getStatusANDTypeValues($membership->id)[$membership->id]['membership_type_id'];
+    $membershipLog = self::createMembershipLog($membership, $logStartDate, $membershipTypeID, $params['modified_id'] ?? NULL);
 
     // reset the group contact cache since smart groups might be affected due to this
     CRM_Contact_BAO_GroupContactCache::opportunisticCacheFlush();
@@ -2027,6 +2005,7 @@ public static function updateAllMembershipStatus($params = []) {
     // This query retrieves ALL memberships of active types.
     // Note: id, is_test, campaign_id expected by CRM_Activity_BAO_Activity::addActivity()
     //   called by createChangeMembershipStatusActivity().
+    // max_related expected by createMembershipLog().
     $baseQuery = "
 SELECT     civicrm_membership.id                    as membership_id,
            civicrm_membership.id                    as id,
@@ -2040,6 +2019,7 @@ public static function updateAllMembershipStatus($params = []) {
            civicrm_membership.start_date            as start_date,
            civicrm_membership.end_date              as end_date,
            civicrm_membership.source                as source,
+           civicrm_membership.max_related           as max_related,
            civicrm_contact.id                       as contact_id,
            civicrm_membership.owner_membership_id   as owner_membership_id,
            civicrm_membership.contribution_recur_id as recur_id
@@ -2087,6 +2067,8 @@ public static function updateAllMembershipStatus($params = []) {
         $allStatusLabels = CRM_Member_BAO_Membership::buildOptions('status_id', 'get');
         $changedByContactID = CRM_Core_Session::getLoggedInContactID() ?? $dao2->contact_id;
         self::createChangeMembershipStatusActivity($dao2, $allStatusLabels[$dao2->status_id], $allStatusLabels[$newStatusId], $changedByContactID);
+        $dao2->status_id = $newStatusId;
+        self::createMembershipLog($dao2);
         $updateCount++;
       }
     }
@@ -2571,4 +2553,46 @@ private static function createChangeMembershipStatusActivity($membership, string
     );
   }
 
+  /**
+   *  Create the MembershipLog record.
+   *  This was embedded deep in the ::add() function.
+   *  Extracted here to it's own function so we have a single place to create it.
+   *
+   * @param CRM_Core_DAO $membership
+   * @param string $logStartDate
+   * @param ?int $membershipTypeID
+   *
+   * @return array
+   *
+   * @internal Signature may change
+   */
+  private static function createMembershipLog($membership, string $logStartDate = '', ?int $membershipTypeID = NULL, ?int $modifiedContactID = NULL): array {
+    if (empty($logStartDate)) {
+      $logStartDate = CRM_Utils_Date::isoToMysql($membership->start_date);
+    }
+    $membershipLog = [
+      'membership_id' => $membership->id,
+      'status_id' => $membership->status_id,
+      'start_date' => $logStartDate,
+      'end_date' => CRM_Utils_Date::isoToMysql($membership->end_date),
+      'modified_date' => CRM_Utils_Time::date('Ymd'),
+      'membership_type_id' => $membershipTypeID ?? $membership->membership_type_id,
+      'max_related' => $membership->max_related,
+    ];
+
+    if (!empty($modifiedContactID)) {
+      $membershipLog['modified_id'] = $modifiedContactID;
+    }
+    // If we have an authenticated session, set modified_id to that user's contact_id, else set to membership.contact_id
+    elseif (CRM_Core_Session::getLoggedInContactID()) {
+      $membershipLog['modified_id'] = CRM_Core_Session::getLoggedInContactID();
+    }
+    else {
+      $membershipLog['modified_id'] = $membership->contact_id;
+    }
+    // @todo maybe move this to an API4 call, or writeRecord()
+    CRM_Member_BAO_MembershipLog::add($membershipLog);
+    return $membershipLog;
+  }
+
 }
diff --git a/tests/phpunit/CRM/Member/BAO/MembershipTest.php b/tests/phpunit/CRM/Member/BAO/MembershipTest.php
@@ -9,6 +9,9 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\Api4\MembershipLog;
+use Civi\Api4\MembershipStatus;
+
 /**
  * Class CRM_Member_BAO_MembershipTest
  * @group headless
@@ -600,6 +603,19 @@ public function testUpdateAllMembershipStatusConvertExpiredOverriddenStatusToNor
 
     $this->assertEquals($createdMembershipID, $membershipAfterProcess['id']);
     $this->assertArrayNotHasKey('status_override_end_date', $membershipAfterProcess);
+
+    // Check that MembershipLog was created and is correct
+    $latestMembershipLog = MembershipLog::get(FALSE)
+      ->addWhere('membership_id', '=', $createdMembershipID)
+      ->addOrderBy('id', 'DESC')
+      ->execute()
+      ->first();
+    $newMembershipStatus = MembershipStatus::get(FALSE)
+      ->addSelect('id')
+      ->addWhere('name', '=', 'New')
+      ->execute()
+      ->first();
+    $this->assertEquals($newMembershipStatus['id'], $latestMembershipLog['status_id']);
   }
 
   /**
@@ -630,6 +646,19 @@ public function testUpdateAllMembershipStatusHandleOverriddenWithEndOverrideDate
 
     $this->assertEquals($createdMembershipID, $membershipAfterProcess['id']);
     $this->assertArrayNotHasKey('status_override_end_date', $membershipAfterProcess);
+
+    // Check that MembershipLog was created and is correct
+    $latestMembershipLog = MembershipLog::get(FALSE)
+      ->addWhere('membership_id', '=', $createdMembershipID)
+      ->addOrderBy('id', 'DESC')
+      ->execute()
+      ->first();
+    $newMembershipStatus = MembershipStatus::get(FALSE)
+      ->addSelect('id')
+      ->addWhere('name', '=', 'New')
+      ->execute()
+      ->first();
+    $this->assertEquals($newMembershipStatus['id'], $latestMembershipLog['status_id']);
   }
 
   /**
@@ -659,6 +688,14 @@ public function testUpdateAllMembershipStatusDoesNotConvertOverriddenMembershipW
 
     $this->assertEquals($createdMembershipID, $membershipAfterProcess['id']);
     $this->assertEquals(1, $membershipAfterProcess['is_override']);
+
+    // Check that MembershipLog was created and is correct
+    $latestMembershipLog = MembershipLog::get(FALSE)
+      ->addWhere('membership_id', '=', $createdMembershipID)
+      ->addOrderBy('id', 'DESC')
+      ->execute()
+      ->first();
+    $this->assertEquals($this->_membershipStatusID, $latestMembershipLog['status_id']);
   }
 
   /**

diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php
@@ -708,6 +708,8 @@ public function membershipStatusCreate($name = 'test member status'): int {
     $params['end_event'] = 'end_date';
     $params['is_current_member'] = 1;
     $params['is_active'] = 1;
+    // Make sure weight is after existing statuses (could be cleverer and get max(weight) first).
+    $params['weight'] = 100;
 
     $result = $this->callAPISuccess('MembershipStatus', 'Create', $params);
     CRM_Member_PseudoConstant::flush('membershipStatus');

diff --git a/tests/phpunit/api/v3/MembershipTest.php b/tests/phpunit/api/v3/MembershipTest.php
@@ -222,15 +222,15 @@ public function testMultipleMembershipsContribution(): void {
     $memberships = $this->callAPISuccess('Membership', 'get', ['sort' => 'contact_id', 'sequential' => 1])['values'];
 
     $this->assertEquals($contactId1, $memberships[0]['contact_id']);
-    $this->assertEquals('test status', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[0]['status_id']));
+    $this->assertEquals('New', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[0]['status_id']));
 
     // check for Membership 2
     $this->assertEquals($contactId2, $memberships[1]['contact_id']);
-    $this->assertEquals('test status', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[1]['status_id']));
+    $this->assertEquals('New', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[1]['status_id']));
 
     // check for Membership 3
     $this->assertEquals($contactId3, $memberships[2]['contact_id']);
-    $this->assertEquals('test status', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[2]['status_id']));
+    $this->assertEquals('New', CRM_Core_PseudoConstant::getName('CRM_Member_BAO_Membership', 'status_id', $memberships[2]['status_id']));
   }
 
   /**