Fix missing membershiplog entries and add tests following #30493

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);
 
     // 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): 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($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;
+    }
+    // @todo maybe move this to an API4 call, or writeRecord()
+    CRM_Member_BAO_MembershipLog::add($membershipLog);
+    return $membershipLog;
+  }
+
 }

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']);
   }
 
   /**

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');