From 4eba8926824dac77b94659cd1a25ea4c57b5fd53 Mon Sep 17 00:00:00 2001 From: eileen Date: Wed, 2 Sep 2020 18:43:08 +1200 Subject: [PATCH] [REF] Add test for existing Participant batch update cancel and fix to not call BaseIPN->cancelled I dug into what is 'achieved' by calling BaseIPN->cancelled here and everything except the bit where the contribution is updated to cancelled is actually bypassed so a simple api call suffices. I also discovered the cancellation of the contribution is highly conditional and arguably illogical. I will separately log a gitlab to discuss whether it still makes sense, but I wouldn't want that discussion to derail this no-change cleanup --- CRM/Core/Payment/BaseIPN.php | 17 ++++++------ CRM/Event/Form/Task/Batch.php | 9 ++----- .../phpunit/CRM/Event/Form/Task/BatchTest.php | 26 ++++++++++++++++--- .../CRMTraits/Financial/OrderTrait.php | 12 +++++++++ 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/CRM/Core/Payment/BaseIPN.php b/CRM/Core/Payment/BaseIPN.php index e4312a5fd7..5947187809 100644 --- a/CRM/Core/Payment/BaseIPN.php +++ b/CRM/Core/Payment/BaseIPN.php @@ -324,19 +324,18 @@ class CRM_Core_Payment_BaseIPN { CRM_Contribute_BAO_ContributionRecur::copyCustomValues($objects['contributionRecur']->id, $contribution->id); } - if (empty($input['IAmAHorribleNastyBeyondExcusableHackInTheCRMEventFORMTaskClassThatNeedsToBERemoved'])) { - if (!empty($memberships)) { - foreach ($memberships as $membership) { - if ($membership) { - $this->cancelMembership($membership, $membership->status_id); - } + if (!empty($memberships)) { + foreach ($memberships as $membership) { + if ($membership) { + $this->cancelMembership($membership, $membership->status_id); } } + } - if ($participant) { - $this->cancelParticipant($participant->id); - } + if ($participant) { + $this->cancelParticipant($participant->id); } + if ($transaction) { $transaction->commit(); } diff --git a/CRM/Event/Form/Task/Batch.php b/CRM/Event/Form/Task/Batch.php index 16d8752bde..31f690d363 100644 --- a/CRM/Event/Form/Task/Batch.php +++ b/CRM/Event/Form/Task/Batch.php @@ -285,7 +285,8 @@ class CRM_Event_Form_Task_Batch extends CRM_Event_Form_Task { $contributionStatusId = array_search('Completed', $contributionStatuses); } if (array_key_exists($statusId, $negativeStatuses)) { - $contributionStatusId = array_search('Cancelled', $contributionStatuses); + civicrm_api3('Contribution', 'create', ['id' => $contributionId, 'contribution_status_id' => 'Cancelled']); + return; } if (!$contributionStatusId || !$participantId || !$contributionId) { @@ -374,12 +375,6 @@ class CRM_Event_Form_Task_Batch extends CRM_Event_Form_Task { 'flip' => 1, ]); $input['IAmAHorribleNastyBeyondExcusableHackInTheCRMEventFORMTaskClassThatNeedsToBERemoved'] = $params['IAmAHorribleNastyBeyondExcusableHackInTheCRMEventFORMTaskClassThatNeedsToBERemoved'] ?? NULL; - if ($statusId == $contributionStatuses['Cancelled']) { - $transaction = new CRM_Core_Transaction(); - $baseIPN->cancelled($objects, $transaction, $input); - $transaction->commit(); - return; - } if ($statusId == $contributionStatuses['Failed']) { $transaction = new CRM_Core_Transaction(); $baseIPN->failed($objects, $transaction, $input); diff --git a/tests/phpunit/CRM/Event/Form/Task/BatchTest.php b/tests/phpunit/CRM/Event/Form/Task/BatchTest.php index 15f24a48bf..d9e590cfbd 100644 --- a/tests/phpunit/CRM/Event/Form/Task/BatchTest.php +++ b/tests/phpunit/CRM/Event/Form/Task/BatchTest.php @@ -7,19 +7,19 @@ * @group headless */ class CRM_Event_Form_Task_BatchTest extends CiviUnitTestCase { + use CRMTraits_Financial_OrderTrait; /** * Test the the submit function on the event participant submit function. - * - * @todo extract submit functions on other Batch update classes, use dataprovider to test on all. */ public function testSubmit() { - $group = $this->CustomGroupCreate(['extends' => 'Participant', 'title' => 'Participant']); + $group = $this->customGroupCreate(['extends' => 'Participant', 'title' => 'Participant']); $field = $this->customFieldCreate(['custom_group_id' => $group['id'], 'html_type' => 'CheckBox', 'option_values' => ['two' => 'A couple', 'three' => 'A few', 'four' => 'Too Many']]); $participantID = $this->participantCreate(); $participant = $this->callAPISuccessGetSingle('Participant', ['id' => $participantID]); $this->assertEquals(2, $participant['participant_status_id']); + /* @var CRM_Event_Form_Task_Batch $form */ $form = $this->getFormObject('CRM_Event_Form_Task_Batch'); $form->submit(['field' => [$participantID => ['participant_status_id' => 1, 'custom_' . $field['id'] => ['two' => 1, 'four' => 1]]]]); @@ -27,4 +27,24 @@ class CRM_Event_Form_Task_BatchTest extends CiviUnitTestCase { $this->assertEquals(1, $participant['participant_status_id']); } + /** + * Test the the submit function on the event participant submit function. + * + * Test is to establish existing behaviour prior to code cleanup. It turns out the existing + * code ONLY cancels the contribution as well as the participant record if is_pay_later is true + * AND the source is 'Online Event Registration'. + */ + public function testSubmitCancel() { + $this->createEventOrder(['source' => 'Online Event Registration', 'is_pay_later' => 1]); + $participantCancelledStatusID = CRM_Core_PseudoConstant::getKey('CRM_Event_BAO_Participant', 'status_id', 'Cancelled'); + + /* @var CRM_Event_Form_Task_Batch $form */ + $form = $this->getFormObject('CRM_Event_Form_Task_Batch'); + $form->submit(['field' => [$this->ids['Participant'][0] => ['participant_status' => $participantCancelledStatusID]]]); + + $participant = $this->callAPISuccessGetSingle('Participant', ['id' => $this->ids['Participant'][0]]); + $this->assertEquals($participantCancelledStatusID, $participant['participant_status_id']); + $this->callAPISuccessGetSingle('Contribution', ['id' => $this->ids['Contribution'][0], 'contribution_status_id' => 'Cancelled']); + } + } diff --git a/tests/phpunit/CRMTraits/Financial/OrderTrait.php b/tests/phpunit/CRMTraits/Financial/OrderTrait.php index 5bd1292a56..c927e71857 100644 --- a/tests/phpunit/CRMTraits/Financial/OrderTrait.php +++ b/tests/phpunit/CRMTraits/Financial/OrderTrait.php @@ -162,6 +162,18 @@ trait CRMTraits_Financial_OrderTrait { $this->ids['Contribution'][0] = $orderID; } + /** + * Create an order for an event. + * + * @param array $orderParams + * + * @throws \CRM_Core_Exception + */ + protected function createEventOrder($orderParams = []) { + $this->ids['Contribution'][0] = $this->callAPISuccess('Order', 'create', array_merge($this->getParticipantOrderParams(), $orderParams))['id']; + $this->ids['Participant'][0] = $this->callAPISuccessGetValue('ParticipantPayment', ['return' => 'participant_id', 'contribution_id' => $this->ids['Contribution'][0]]); + } + /** * Create an extraneous contribution to throw off any 'number one bugs'. * -- 2.25.1