Simply buildMembershipTypeValues & access cached values

This simplifies this function & switches it to use a cached function to get the MembershipTypes. In general I would
argue that we should be caching metadata entities (membership types, statues, price fields, etc) and accessing
the cached versions & ideally the v4 api would be able to access cached versions filter-dependent.

In order to keep the test passing I worked a little on standardisation

diff --git a/CRM/Core/Config.php b/CRM/Core/Config.php
index a942d7ffccd3affbb3f5ede5513d376f4962628e..fdffa814a6a1a60361b759a11d80eb7e5f4f5194 100644 (file)
--- a/CRM/Core/Config.php
+++ b/CRM/Core/Config.php
@@ -242,7 +242,7 @@ class CRM_Core_Config extends CRM_Core_Config_MagicMerge {
       $domain = defined('CIVICRM_DOMAIN_ID') ? CIVICRM_DOMAIN_ID : 1;
     }
 
-    return $domain;
+    return (int) $domain;
   }
 
   /**

diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php
index 3fff6f955f935aeaa0fc959ac328a08cd654adc0..285d5be819fec5bb8b491a7c0966515d76d27eb7 100644 (file)
--- a/CRM/Member/BAO/Membership.php
+++ b/CRM/Member/BAO/Membership.php
@@ -1537,55 +1537,19 @@ WHERE  civicrm_membership.contact_id = civicrm_contact.id
    *   behaviour unchanged).
    *
    * @return array
+   *
+   * @throws \CiviCRM_API3_Exception
    */
-  public static function buildMembershipTypeValues(&$form, $membershipTypeID = [], $activeOnly = FALSE) {
-    $whereClause = " WHERE domain_id = " . CRM_Core_Config::domainID();
+  public static function buildMembershipTypeValues($form, $membershipTypeID = [], $activeOnly = FALSE) {
     $membershipTypeIDS = (array) $membershipTypeID;
+    $membershipTypeValues = CRM_Member_BAO_MembershipType::getPermissionedMembershipTypes();
 
-    if ($activeOnly) {
-      $whereClause .= " AND is_active = 1 ";
-    }
-    if (!empty($membershipTypeIDS)) {
-      $allIDs = implode(',', $membershipTypeIDS);
-      $whereClause .= " AND id IN ( $allIDs )";
-    }
-    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::ADD);
-
-    if ($financialTypes) {
-      $whereClause .= " AND financial_type_id IN (" . implode(',', array_keys($financialTypes)) . ")";
-    }
-    else {
-      $whereClause .= " AND financial_type_id IN (0)";
-    }
-
-    $query = "
-SELECT *
-FROM   civicrm_membership_type
-       $whereClause;
-";
-    $dao = CRM_Core_DAO::executeQuery($query);
-
-    $membershipTypeValues = [];
-    $membershipTypeFields = [
-      'id',
-      'minimum_fee',
-      'name',
-      'is_active',
-      'description',
-      'financial_type_id',
-      'auto_renew',
-      'member_of_contact_id',
-      'relationship_type_id',
-      'relationship_direction',
-      'max_related',
-      'duration_unit',
-      'duration_interval',
-    ];
-
-    while ($dao->fetch()) {
-      $membershipTypeValues[$dao->id] = [];
-      foreach ($membershipTypeFields as $mtField) {
-        $membershipTypeValues[$dao->id][$mtField] = $dao->$mtField;
+    // MembershipTypes are already filtered by domain, filter as appropriate by is_active & a passed in list of ids.
+    foreach ($membershipTypeValues as $id => $type) {
+      if (($activeOnly && empty($type['is_active']))
+        || (!empty($membershipTypeIDS) && !in_array($id, $membershipTypeIDS, FALSE))
+      ) {
+        unset($membershipTypeValues[$id]);
       }
     }
 
@@ -1594,11 +1558,10 @@ FROM   civicrm_membership_type
     if (is_numeric($membershipTypeID) &&
       $membershipTypeID > 0
     ) {
+      CRM_Core_Error::deprecatedFunctionWarning('Non arrays deprecated');
       return $membershipTypeValues[$membershipTypeID];
     }
-    else {
-      return $membershipTypeValues;
-    }
+    return $membershipTypeValues;
   }
 
   /**

diff --git a/CRM/Member/BAO/MembershipType.php b/CRM/Member/BAO/MembershipType.php
index 3ca7caf5d0b0a0f8ecf12806cccf2382c11961d9..f227519e7fec5ebb72886be819bc6c518eee93de 100644 (file)
--- a/CRM/Member/BAO/MembershipType.php
+++ b/CRM/Member/BAO/MembershipType.php
@@ -818,7 +818,22 @@ class CRM_Member_BAO_MembershipType extends CRM_Member_DAO_MembershipType {
    */
   public static function getAllMembershipTypes() {
     if (!Civi::cache('metadata')->has(__CLASS__ . __FUNCTION__)) {
-      Civi::cache('metadata')->set(__CLASS__ . __FUNCTION__, civicrm_api3('MembershipType', 'get', ['options' => ['limit' => 0, 'sort' => 'weight']])['values']);
+      $types = civicrm_api3('MembershipType', 'get', ['options' => ['limit' => 0, 'sort' => 'weight']])['values'];
+      $keys = ['description', 'relationship_type_id', 'relationship_direction', 'max_related'];
+      // In order to avoid down-stream e-notices we undo api v3 filtering of NULL values. This is covered
+      // in Unit tests & ideally we might switch to apiv4 but I would argue we should build caching
+      // of metadata entities like this directly into apiv4.
+      foreach ($types as $id => $type) {
+        foreach ($keys as $key) {
+          if (!isset($type[$key])) {
+            $types[$id][$key] = NULL;
+          }
+        }
+        if (isset($type['contribution_type_id'])) {
+          unset($types[$id]['contribution_type_id']);
+        }
+      }
+      Civi::cache('metadata')->set(__CLASS__ . __FUNCTION__, $types);
     }
     return Civi::cache('metadata')->get(__CLASS__ . __FUNCTION__);
   }
@@ -835,4 +850,21 @@ class CRM_Member_BAO_MembershipType extends CRM_Member_DAO_MembershipType {
     return self::getAllMembershipTypes()[$id];
   }
 
+  /**
+   * Get an array of all membership types the contact is permitted to access.
+   *
+   * @throws \CiviCRM_API3_Exception
+   */
+  public static function getPermissionedMembershipTypes() {
+    $types = self::getAllMembershipTypes();
+    $financialTypes = NULL;
+    $financialTypes = CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes, CRM_Core_Action::ADD);
+    foreach ($types as $id => $type) {
+      if (!isset($financialTypes[$type['financial_type_id']])) {
+        unset($types[$id]);
+      }
+    }
+    return $types;
+  }
+
 }

diff --git a/tests/phpunit/CRM/Member/BAO/MembershipTest.php b/tests/phpunit/CRM/Member/BAO/MembershipTest.php
index 6a6e155f4cb1e2a4135e166ae3e595274dbc04c1..f722f117e06dbe88a85604e02a314e9d7887528c 100644 (file)
--- a/tests/phpunit/CRM/Member/BAO/MembershipTest.php
+++ b/tests/phpunit/CRM/Member/BAO/MembershipTest.php
@@ -770,8 +770,11 @@ class CRM_Member_BAO_MembershipTest extends CiviUnitTestCase {
 
   /**
    * Test the buildMembershipTypeValues function.
+   *
+   * @throws \CiviCRM_API3_Exception
    */
   public function testBuildMembershipTypeValues() {
+    $this->restoreMembershipTypes();
     $form = new CRM_Core_Form();
     $values = CRM_Member_BAO_Membership::buildMembershipTypeValues($form);
     $this->assertEquals([
@@ -783,11 +786,15 @@ class CRM_Member_BAO_MembershipTest extends CiviUnitTestCase {
       'financial_type_id' => '2',
       'auto_renew' => '0',
       'member_of_contact_id' => $values[1]['member_of_contact_id'],
-      'relationship_type_id' => 7,
-      'relationship_direction' => 'b_a',
+      'relationship_type_id' => [7],
+      'relationship_direction' => ['b_a'],
       'max_related' => NULL,
       'duration_unit' => 'year',
       'duration_interval' => '2',
+      'domain_id' => '1',
+      'period_type' => 'rolling',
+      'visibility' => 'Public',
+      'weight' => '1',
     ], $values[1]);
   }