From 880600455bd157486e6324bb8b581fad7bd0a2b8 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Fri, 13 Aug 2021 08:58:41 +1200 Subject: [PATCH] Add v4 order api, access it via order --- CRM/Member/BAO/Membership.php | 122 ++++++++++-------- Civi/Api4/Membership.php | 23 ++++ Civi/Api4/MembershipBlock.php | 23 ++++ Civi/Api4/MembershipStatus.php | 23 ++++ .../MembershipCreationSpecProvider.php | 51 ++++++++ api/v3/Order.php | 39 +++++- .../CRM/Core/Payment/PayPalIPNTest.php | 6 +- tests/phpunit/CiviTest/CiviUnitTestCase.php | 3 +- tests/phpunit/api/v3/MembershipTest.php | 6 +- tests/phpunit/api/v3/RelationshipTest.php | 32 ++--- .../api/v4/DataSets/ConformanceTest.json | 21 +++ .../phpunit/api/v4/Entity/ConformanceTest.php | 38 ++++-- 12 files changed, 292 insertions(+), 95 deletions(-) create mode 100644 Civi/Api4/Membership.php create mode 100644 Civi/Api4/MembershipBlock.php create mode 100644 Civi/Api4/MembershipStatus.php create mode 100644 Civi/Api4/Service/Spec/Provider/MembershipCreationSpecProvider.php diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php index edc63eda52..95bf1436fc 100644 --- a/CRM/Member/BAO/Membership.php +++ b/CRM/Member/BAO/Membership.php @@ -339,70 +339,80 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { } $params['membership_id'] = $membership->id; - // @todo further cleanup required to remove use of $ids['contribution'] from here - if (isset($ids['membership'])) { - $contributionID = CRM_Core_DAO::getFieldValue('CRM_Member_DAO_MembershipPayment', - $membership->id, - 'contribution_id', - 'membership_id' - ); - // @todo this is a temporary step to removing $ids['contribution'] completely - if (empty($params['contribution_id']) && !empty($contributionID)) { - $params['contribution_id'] = $contributionID; + // For api v4 we skip all of this stuff. There is an expectation that v4 users either use + // the order api, or handle any financial / related processing themselves. + // Note that the processing below is fairly intertwined with core usage and in some places + // problematic or to be removed. + // Note the choice of 'version' as a parameter is to make it + // unavailable through apiv3. + // once we are rid of direct calls to the BAO::create from core + // we will deprecate this stuff into the v3 api. + if (($params['version'] ?? 0) !== 4) { + // @todo further cleanup required to remove use of $ids['contribution'] from here + if (isset($ids['membership'])) { + $contributionID = CRM_Core_DAO::getFieldValue('CRM_Member_DAO_MembershipPayment', + $membership->id, + 'contribution_id', + 'membership_id' + ); + // @todo this is a temporary step to removing $ids['contribution'] completely + if (empty($params['contribution_id']) && !empty($contributionID)) { + $params['contribution_id'] = $contributionID; + } } - } - - // This code ensures a line item is created but it is recommended you pass in 'skipLineItem' or 'line_item' - if (empty($params['line_item']) && !empty($params['membership_type_id']) && empty($params['skipLineItem'])) { - CRM_Price_BAO_LineItem::getLineItemArray($params, NULL, 'membership', $params['membership_type_id']); - } - $params['skipLineItem'] = TRUE; - - // Record contribution for this membership and create a MembershipPayment - // @todo deprecate this. - if (!empty($params['contribution_status_id'])) { - $memInfo = array_merge($params, ['membership_id' => $membership->id]); - $params['contribution'] = self::recordMembershipContribution($memInfo); - } - // If the membership has no associated contribution then we ensure - // the line items are 'correct' here. This is a lazy legacy - // hack whereby they are deleted and recreated - if (empty($contributionID)) { - if (!empty($params['lineItems'])) { - $params['line_item'] = $params['lineItems']; + // This code ensures a line item is created but it is recommended you pass in 'skipLineItem' or 'line_item' + if (empty($params['line_item']) && !empty($params['membership_type_id']) && empty($params['skipLineItem'])) { + CRM_Price_BAO_LineItem::getLineItemArray($params, NULL, 'membership', $params['membership_type_id']); } - // do cleanup line items if membership edit the Membership type. - if (!empty($ids['membership'])) { - CRM_Price_BAO_LineItem::deleteLineItems($ids['membership'], 'civicrm_membership'); + $params['skipLineItem'] = TRUE; + + // Record contribution for this membership and create a MembershipPayment + // @todo deprecate this. + if (!empty($params['contribution_status_id'])) { + $memInfo = array_merge($params, ['membership_id' => $membership->id]); + $params['contribution'] = self::recordMembershipContribution($memInfo); } - // @todo - we should ONLY do the below if a contribution is created. Let's - // get some deprecation notices in here & see where it's hit & work to eliminate. - // This could happen if there is no contribution or we are in one of many - // weird and wonderful flows. This is scary code. Keep adding tests. - if (!empty($params['line_item']) && empty($params['contribution_id'])) { - foreach ($params['line_item'] as $priceSetId => $lineItems) { - foreach ($lineItems as $lineIndex => $lineItem) { - $lineMembershipType = $lineItem['membership_type_id'] ?? NULL; - if (!empty($params['contribution'])) { - $params['line_item'][$priceSetId][$lineIndex]['contribution_id'] = $params['contribution']->id; - } - if ($lineMembershipType && $lineMembershipType == ($params['membership_type_id'] ?? NULL)) { - $params['line_item'][$priceSetId][$lineIndex]['entity_id'] = $membership->id; - $params['line_item'][$priceSetId][$lineIndex]['entity_table'] = 'civicrm_membership'; - } - elseif (!$lineMembershipType && !empty($params['contribution'])) { - $params['line_item'][$priceSetId][$lineIndex]['entity_id'] = $params['contribution']->id; - $params['line_item'][$priceSetId][$lineIndex]['entity_table'] = 'civicrm_contribution'; + // If the membership has no associated contribution then we ensure + // the line items are 'correct' here. This is a lazy legacy + // hack whereby they are deleted and recreated + if (empty($contributionID)) { + if (!empty($params['lineItems'])) { + $params['line_item'] = $params['lineItems']; + } + // do cleanup line items if membership edit the Membership type. + if (!empty($ids['membership'])) { + CRM_Price_BAO_LineItem::deleteLineItems($ids['membership'], 'civicrm_membership'); + } + // @todo - we should ONLY do the below if a contribution is created. Let's + // get some deprecation notices in here & see where it's hit & work to eliminate. + // This could happen if there is no contribution or we are in one of many + // weird and wonderful flows. This is scary code. Keep adding tests. + if (!empty($params['line_item']) && empty($params['contribution_id'])) { + + foreach ($params['line_item'] as $priceSetId => $lineItems) { + foreach ($lineItems as $lineIndex => $lineItem) { + $lineMembershipType = $lineItem['membership_type_id'] ?? NULL; + if (!empty($params['contribution'])) { + $params['line_item'][$priceSetId][$lineIndex]['contribution_id'] = $params['contribution']->id; + } + if ($lineMembershipType && $lineMembershipType == ($params['membership_type_id'] ?? NULL)) { + $params['line_item'][$priceSetId][$lineIndex]['entity_id'] = $membership->id; + $params['line_item'][$priceSetId][$lineIndex]['entity_table'] = 'civicrm_membership'; + } + elseif (!$lineMembershipType && !empty($params['contribution'])) { + $params['line_item'][$priceSetId][$lineIndex]['entity_id'] = $params['contribution']->id; + $params['line_item'][$priceSetId][$lineIndex]['entity_table'] = 'civicrm_contribution'; + } } } + CRM_Price_BAO_LineItem::processPriceSet( + $membership->id, + $params['line_item'], + $params['contribution'] ?? NULL + ); } - CRM_Price_BAO_LineItem::processPriceSet( - $membership->id, - $params['line_item'], - $params['contribution'] ?? NULL - ); } } diff --git a/Civi/Api4/Membership.php b/Civi/Api4/Membership.php new file mode 100644 index 0000000000..3906edd802 --- /dev/null +++ b/Civi/Api4/Membership.php @@ -0,0 +1,23 @@ +getFieldByName('status_id')->setRequired(FALSE); + // This is a bit of dark-magic - the membership BAO code is particularly + // nasty. It has a lot of logic in it that does not belong there in + // terms of our current expectations. Removing it is difficult + // so the plan is that new api v4 membership.create users will either + // use the Order api flow when financial code should kick in. Otherwise + // the crud flow will bypass all the financial processing in membership.create. + // The use of the 'version' parameter to drive this is to be sure that + // we know the bypass will not affect the v3 api to the extent + // it cannot be reached by the v3 api at all (in time we can move some + // of the code we are deprecating into the v3 api, to die of natural deprecation). + $spec->addFieldSpec(new FieldSpec('version', 'Membership', 'Integer')); + $spec->getFieldByName('version')->setDefaultValue(4)->setRequired(TRUE); + } + + /** + * When does this apply. + * + * @param string $entity + * @param string $action + * + * @return bool + */ + public function applies($entity, $action): bool { + return $entity === 'Membership' && $action === 'create'; + } + +} diff --git a/api/v3/Order.php b/api/v3/Order.php index 93c4ddff56..e0529d385c 100644 --- a/api/v3/Order.php +++ b/api/v3/Order.php @@ -16,6 +16,8 @@ * @package CiviCRM_APIv3 */ +use Civi\Api4\Membership; + /** * Retrieve a set of Order. * @@ -140,15 +142,18 @@ function civicrm_api3_order_create(array $params): array { if ($entityParams['entity'] === 'membership') { if (empty($entityParams['id'])) { - $entityParams['status_id'] = 'Pending'; + $entityParams['status_id:name'] = 'Pending'; } if (!empty($params['contribution_recur_id'])) { $entityParams['contribution_recur_id'] = $params['contribution_recur_id']; } - $entityParams['skipLineItem'] = TRUE; - $entityResult = civicrm_api3('Membership', 'create', $entityParams); + // At this stage we need to get this passed through. + $entityParams['version'] = 4; + _order_create_wrangle_membership_params($entityParams); + + $membershipID = Membership::save($params['check_permissions'] ?? FALSE)->setRecords([$entityParams])->execute()->first()['id']; foreach ($entityParams['line_references'] as $lineIndex) { - $order->setLineItemValue('entity_id', $entityResult['id'], $lineIndex); + $order->setLineItemValue('entity_id', $membershipID, $lineIndex); } } } @@ -286,3 +291,29 @@ function _civicrm_api3_order_delete_spec(array &$params) { ]; $params['id']['api.aliases'] = ['contribution_id']; } + +/** + * Handle possibility of v3 style params. + * + * We used to call v3 Membership.create. Now we call v4. + * This converts membership input parameters. + * + * @param array $membershipParams + * + * @throws \API_Exception + */ +function _order_create_wrangle_membership_params(array &$membershipParams) { + $fields = Membership::getFields(FALSE)->execute()->indexBy('name'); + foreach ($fields as $fieldName => $field) { + $customFieldName = 'custom_' . ($field['custom_field_id'] ?? NULL); + if ($field['type'] === ['Custom'] && isset($membershipParams[$customFieldName])) { + $membershipParams[$field['custom_group'] . '.' . $field['custom_field']] = $membershipParams[$customFieldName]; + unset($membershipParams[$customFieldName]); + } + + if (!empty($membershipParams[$fieldName]) && $field['data_type'] === 'Integer' && !is_numeric($membershipParams[$fieldName])) { + $membershipParams[$field['name'] . ':name'] = $membershipParams[$fieldName]; + unset($membershipParams[$field['name']]); + } + } +} diff --git a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php index e54eef3fd5..03e9055456 100644 --- a/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php +++ b/tests/phpunit/CRM/Core/Payment/PayPalIPNTest.php @@ -156,12 +156,14 @@ class CRM_Core_Payment_PayPalIPNTest extends CiviUnitTestCase { } /** - * Test IPN response updates contribution_recur & contribution for first & second contribution. + * Test IPN response updates contribution_recur & contribution for first & + * second contribution. * * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception + * @throws \API_Exception */ - public function testIPNPaymentMembershipRecurSuccess() { + public function testIPNPaymentMembershipRecurSuccess(): void { $durationUnit = 'year'; $this->setupMembershipRecurringPaymentProcessorTransaction(['duration_unit' => $durationUnit, 'frequency_unit' => $durationUnit]); $this->callAPISuccessGetSingle('membership_payment', []); diff --git a/tests/phpunit/CiviTest/CiviUnitTestCase.php b/tests/phpunit/CiviTest/CiviUnitTestCase.php index a54aee4061..e4cd01f0d7 100644 --- a/tests/phpunit/CiviTest/CiviUnitTestCase.php +++ b/tests/phpunit/CiviTest/CiviUnitTestCase.php @@ -669,9 +669,8 @@ class CiviUnitTestCase extends PHPUnit\Framework\TestCase { * @param array $params * * @return int - * @throws \CRM_Core_Exception */ - public function contactMembershipCreate($params) { + public function contactMembershipCreate(array $params): int { $params = array_merge([ 'join_date' => '2007-01-21', 'start_date' => '2007-01-21', diff --git a/tests/phpunit/api/v3/MembershipTest.php b/tests/phpunit/api/v3/MembershipTest.php index 9a62fa14ad..83d7846f4c 100644 --- a/tests/phpunit/api/v3/MembershipTest.php +++ b/tests/phpunit/api/v3/MembershipTest.php @@ -965,9 +965,11 @@ class api_v3_MembershipTest extends CiviUnitTestCase { /** * Test civicrm_contact_memberships_create with membership id (edit * membership). - * success expected. + * + * @dataProvider versionThreeAndFour */ - public function testMembershipCreateWithId() { + public function testMembershipCreateWithId($version): void { + $this->_apiversion = $version; $membershipID = $this->contactMembershipCreate($this->_params); $params = [ 'id' => $membershipID, diff --git a/tests/phpunit/api/v3/RelationshipTest.php b/tests/phpunit/api/v3/RelationshipTest.php index 963a5bdd5d..9af49654d9 100644 --- a/tests/phpunit/api/v3/RelationshipTest.php +++ b/tests/phpunit/api/v3/RelationshipTest.php @@ -80,11 +80,7 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { * @throws \Exception */ public function tearDown(): void { - $this->contactDelete($this->_cId_a); - $this->contactDelete($this->_cId_a_2); - $this->contactDelete($this->_cId_b); - $this->contactDelete($this->_cId_b2); - $this->quickCleanup(['civicrm_relationship'], TRUE); + $this->quickCleanup(['civicrm_relationship', 'civicrm_membership'], TRUE); $this->relationshipTypeDelete($this->_relTypeID); parent::tearDown(); } @@ -94,7 +90,7 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { * @param int $version * @dataProvider versionThreeAndFour */ - public function testRelationshipCreateEmpty($version) { + public function testRelationshipCreateEmpty($version): void { $this->_apiversion = $version; $this->callAPIFailure('relationship', 'create', []); } @@ -1316,11 +1312,10 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { * @param int $version * * @dataProvider versionThreeAndFour - * - * @throws \CRM_Core_Exception */ - public function testCreateRelatedMembership($version) { + public function testCreateRelatedMembership(int $version): void { $this->_apiversion = $version; + $mainContactID = $this->organizationCreate(); $relatedMembershipType = $this->callAPISuccess('MembershipType', 'create', [ 'name' => 'Membership with Related', 'member_of_contact_id' => 1, @@ -1338,12 +1333,12 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { ]); $originalMembership = $this->callAPISuccess('Membership', 'create', [ 'membership_type_id' => $relatedMembershipType['id'], - 'contact_id' => $this->_cId_b, + 'contact_id' => $mainContactID, ]); $this->callAPISuccess('Relationship', 'create', [ 'relationship_type_id' => $this->_relTypeID, 'contact_id_a' => $this->_cId_a, - 'contact_id_b' => $this->_cId_b, + 'contact_id_b' => $mainContactID, ]); $contactAMembership = $this->callAPISuccessGetSingle('membership', ['contact_id' => $this->_cId_a]); $this->assertEquals($originalMembership['id'], $contactAMembership['owner_membership_id']); @@ -1352,14 +1347,19 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { $this->callAPISuccess('Relationship', 'create', [ 'relationship_type_id' => $this->_relTypeID, 'contact_id_a' => $this->_cId_a_2, - 'contact_id_b' => $this->_cId_b, + 'contact_id_b' => $mainContactID, 'start_date' => 'now + 1 week', ]); $this->callAPISuccessGetCount('membership', ['contact_id' => $this->_cId_a_2], 0); + // @todo - this is actually mimicing an accidental test set up from earlier. + // there is a legit bug whereby apiv4 contact.delete is + // somehow bypassing Membership cleanup. + // https://lab.civicrm.org/dev/core/-/issues/2757 + $this->_apiversion = 3; // Deleting the organization should cause the related membership to be deleted. - $this->callAPISuccess('contact', 'delete', ['id' => $this->_cId_b]); - $this->callAPISuccessGetCount('membership', ['contact_id' => $this->_cId_a], 0); + $this->callAPISuccess('Contact', 'delete', ['id' => $mainContactID]); + $this->callAPISuccessGetCount('Membership', ['contact_id' => $this->_cId_a], 0); } /** @@ -1382,7 +1382,7 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { $reln = new CRM_Contact_Form_Relationship(); $reln->_action = CRM_Core_Action::ADD; $reln->_contactId = $this->_cId_a; - list ($params, $relationshipIds) = $reln->submit($params); + [$params, $relationshipIds] = $reln->submit($params); $this->assertEquals( $this->_cId_b, $this->callAPISuccess('Contact', 'getvalue', [ @@ -1398,7 +1398,7 @@ class api_v3_RelationshipTest extends CiviUnitTestCase { ]; $reln->_action = CRM_Core_Action::UPDATE; $reln->_relationshipId = $relationshipIds[0]; - list ($params, $relationshipIds) = $reln->submit($params); + [$params, $relationshipIds] = $reln->submit($params); $this->assertEmpty($this->callAPISuccess('Contact', 'getvalue', [ 'id' => $this->_cId_a, 'return' => 'current_employer_id', diff --git a/tests/phpunit/api/v4/DataSets/ConformanceTest.json b/tests/phpunit/api/v4/DataSets/ConformanceTest.json index 3d7918ecba..bec93a78f6 100644 --- a/tests/phpunit/api/v4/DataSets/ConformanceTest.json +++ b/tests/phpunit/api/v4/DataSets/ConformanceTest.json @@ -90,5 +90,26 @@ "financial_type_id:name": "Donation", "min_contribution": "10.00" } + ], + "MembershipType": [ + { + "name": "General", + "period_type" : "fixed", + "financial_type_id:name": "Donation", + "duration_unit" : "month", + "member_of_contact_id" : "@ref test_contact_1.id" + } + ], + "ContributionPage": [ + { + "name": "General" + } + ], + "Membership": [ + { + "membership_type_id:name": "General", + "contact_id" : "@ref test_contact_1.id", + "status_id:name" : "Pending" + } ] } diff --git a/tests/phpunit/api/v4/Entity/ConformanceTest.php b/tests/phpunit/api/v4/Entity/ConformanceTest.php index 2bf2386f5c..7060fbb7ba 100644 --- a/tests/phpunit/api/v4/Entity/ConformanceTest.php +++ b/tests/phpunit/api/v4/Entity/ConformanceTest.php @@ -19,7 +19,12 @@ namespace api\v4\Entity; +use api\v4\Traits\CheckAccessTrait; +use api\v4\Traits\OptionCleanupTrait; +use api\v4\Traits\TableDropperTrait; use Civi\API\Exception\UnauthorizedException; +use Civi\Api4\CustomField; +use Civi\Api4\CustomGroup; use Civi\Api4\Entity; use api\v4\UnitTestCase; use Civi\Api4\Event\ValidateValuesEvent; @@ -33,9 +38,9 @@ use Civi\Test\HookInterface; */ class ConformanceTest extends UnitTestCase implements HookInterface { - use \api\v4\Traits\CheckAccessTrait; - use \api\v4\Traits\TableDropperTrait; - use \api\v4\Traits\OptionCleanupTrait { + use CheckAccessTrait; + use TableDropperTrait; + use OptionCleanupTrait { setUp as setUpOptionCleanup; } @@ -48,24 +53,31 @@ class ConformanceTest extends UnitTestCase implements HookInterface { * Set up baseline for testing */ public function setUp(): void { + $this->setUpOptionCleanup(); + $this->loadDataSet('CaseType'); + $this->loadDataSet('ConformanceTest'); + $this->creationParamProvider = \Civi::container()->get('test.param_provider'); + parent::setUp(); + $this->resetCheckAccess(); + } + + /** + * @throws \API_Exception + * @throws \Civi\API\Exception\UnauthorizedException + */ + public function tearDown(): void { + CustomField::delete()->addWhere('id', '>', 0)->execute(); + CustomGroup::delete()->addWhere('id', '>', 0)->execute(); $tablesToTruncate = [ 'civicrm_case_type', - 'civicrm_custom_group', - 'civicrm_custom_field', 'civicrm_group', 'civicrm_event', 'civicrm_participant', 'civicrm_batch', 'civicrm_product', ]; - $this->dropByPrefix('civicrm_value_myfavorite'); $this->cleanup(['tablesToTruncate' => $tablesToTruncate]); - $this->setUpOptionCleanup(); - $this->loadDataSet('CaseType'); - $this->loadDataSet('ConformanceTest'); - $this->creationParamProvider = \Civi::container()->get('test.param_provider'); - parent::setUp(); - $this->resetCheckAccess(); + parent::tearDown(); } /** @@ -133,7 +145,7 @@ class ConformanceTest extends UnitTestCase implements HookInterface { * * @throws \API_Exception */ - public function testConformance($entity): void { + public function testConformance(string $entity): void { $entityClass = CoreUtil::getApiClass($entity); $this->checkEntityInfo($entityClass); -- 2.25.1