From 322b59f00ce2917cef8c3b1b56dd57540fb9ede2 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 9 Dec 2019 22:29:09 +1300 Subject: [PATCH] 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 --- CRM/Core/Config.php | 2 +- CRM/Member/BAO/Membership.php | 61 ++++--------------- CRM/Member/BAO/MembershipType.php | 34 ++++++++++- .../phpunit/CRM/Member/BAO/MembershipTest.php | 11 +++- 4 files changed, 55 insertions(+), 53 deletions(-) diff --git a/CRM/Core/Config.php b/CRM/Core/Config.php index a942d7ffcc..fdffa814a6 100644 --- 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 3fff6f955f..285d5be819 100644 --- 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 3ca7caf5d0..f227519e7f 100644 --- 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 6a6e155f4c..f722f117e0 100644 --- 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]); } -- 2.25.1