From cc789d465dcaa6d23ce1f2b13bf2248db7e6c245 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 13 Nov 2014 14:54:30 +1300 Subject: [PATCH] CRM-15555 fix fatal error on postProcess due to confusion about pay later status --- CRM/Contribute/BAO/Contribution.php | 1 - CRM/Contribute/BAO/Contribution/Utils.php | 56 +++++++++++--------- CRM/Contribute/Form/Contribution/Confirm.php | 43 ++++++++++++--- CRM/Contribute/Form/Contribution/Main.php | 4 +- CRM/Core/Payment/Form.php | 5 +- CRM/Event/Form/Registration/Register.php | 29 +++++----- CRM/Member/BAO/Membership.php | 18 +++++-- 7 files changed, 103 insertions(+), 53 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index a6d58057fd..4df65693a0 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -102,7 +102,6 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution { } //per http://wiki.civicrm.org/confluence/display/CRM/Database+layer we are moving away from $ids array $contributionID = CRM_Utils_Array::value('contribution', $ids, CRM_Utils_Array::value('id', $params)); - $duplicates = array(); if (self::checkDuplicate($params, $duplicates, $contributionID)) { $error = CRM_Core_Error::singleton(); diff --git a/CRM/Contribute/BAO/Contribution/Utils.php b/CRM/Contribute/BAO/Contribution/Utils.php index 1ccebef1e9..8b3c1af9ca 100644 --- a/CRM/Contribute/BAO/Contribution/Utils.php +++ b/CRM/Contribute/BAO/Contribution/Utils.php @@ -46,8 +46,13 @@ class CRM_Contribute_BAO_Contribution_Utils { * @param int $contributionTypeId financial type id * @param int|string $component component id * - * @param null $fieldTypes + * @param array $fieldTypes presumably relates to custom field types - used when building data for sendMail * + * @param $isTest + * @param $isPayLater + * + * @throws CRM_Core_Exception + * @throws Exception * @return array associated array * * @static @@ -59,26 +64,13 @@ class CRM_Contribute_BAO_Contribution_Utils { $contactID, $contributionTypeId, $component = 'contribution', - $fieldTypes = NULL + $fieldTypes = NULL, + $isTest, + $isPayLater ) { CRM_Core_Payment_Form::mapParams($form->_bltID, $form->_params, $paymentParams, TRUE); - $isTest = ($form->_mode == 'test') ? 1 : 0; $lineItems = $form->_lineItem; - - //do overrides that should have been done in the calling function - - // @todo the calling function should pass the correct - // variable for $contributionTypeId as it's impossible to debug what is happening in each flow here - // but we need to ensure it is doing so before we remove this cruft - - if (isset($paymentParams['financial_type'])) { - $contributionTypeId = $paymentParams['financial_type']; - } - elseif (!empty($form->_values['pledge_id'])) { - $contributionTypeId = CRM_Core_DAO::getFieldValue('CRM_Pledge_DAO_Pledge', - $form->_values['pledge_id'], - 'financial_type_id' - ); - } + $isPaymentTransaction = self::isPaymentTransaction($form); $contributionType = new CRM_Financial_DAO_FinancialType(); $contributionType->id = $contributionTypeId; @@ -98,7 +90,8 @@ class CRM_Contribute_BAO_Contribution_Utils { $payment = NULL; $paymentObjError = ts('The system did not record payment details for this payment and so could not process the transaction. Please report this error to the site administrator.'); - if ($form->_values['is_monetary'] && $form->_amount > 0.0 && is_array($form->_paymentProcessor)) { + + if ($isPaymentTransaction) { $payment = CRM_Core_Payment::singleton($form->_mode, $form->_paymentProcessor, $form); } @@ -107,7 +100,7 @@ class CRM_Contribute_BAO_Contribution_Utils { $result = NULL; if ($form->_contributeMode == 'notify' || - $form->_params['is_pay_later'] + $isPayLater ) { // this is not going to come back, i.e. we fill in the other details // when we get a callback from the payment processor @@ -141,7 +134,7 @@ class CRM_Contribute_BAO_Contribution_Utils { $form->set('params', $form->_params); $form->postProcessPremium($premiumParams, $contribution); - if ($form->_values['is_monetary'] && $form->_amount >= 0.0) { + if ($isPaymentTransaction) { // add qfKey so we can send to paypal $form->_params['qfKey'] = $form->controller->_key; if ($component == 'membership') { @@ -149,12 +142,12 @@ class CRM_Contribute_BAO_Contribution_Utils { return $membershipResult; } else { - if (!$form->_params['is_pay_later']) { + if (!$isPayLater) { if (is_object($payment)) { // call postProcess hook before leaving $form->postProcessHook(); // this does not return - $result = &$payment->doTransferCheckout($form->_params, 'contribute'); + $result = $payment->doTransferCheckout($form->_params, 'contribute'); } else{ CRM_Core_Error::fatal($paymentObjError); @@ -211,7 +204,7 @@ class CRM_Contribute_BAO_Contribution_Utils { } } } - elseif ($form->_values['is_monetary'] && $form->_amount > 0.0) { + elseif ($isPaymentTransaction) { if (!empty($paymentParams['is_recur']) && $form->_contributeMode == 'direct' ) { @@ -362,6 +355,21 @@ class CRM_Contribute_BAO_Contribution_Utils { } } + /** + * Is a payment being made. + * Note that setting is_monetary on the form is somewhat legacy and the behaviour around this setting is confusing. It would be preferable + * to look for the amount only (assuming this cannot refer to payment in goats or other non-monetary currency + * @param $form + * + * @return bool + */ + static protected function isPaymentTransaction($form) { + if(!empty($form->_values['is_monetary']) && $form->_amount >= 0.0) { + return TRUE; + } + return FALSE; + + } /** * Function to get the contribution details by month * of the year diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php index 4f19a93753..b9d220aa78 100644 --- a/CRM/Contribute/Form/Contribution/Confirm.php +++ b/CRM/Contribute/Form/Contribution/Confirm.php @@ -667,7 +667,10 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr */ public function postProcess() { $contactID = $this->getContactID(); - + $isPayLater = $this->_params['is_pay_later']; + if ($this->_params['payment_processor'] == 0) { + $this->_params['is_pay_later'] = $isPayLater = TRUE; + } // add a description field at the very beginning $this->_params['description'] = ts('Online Contribution') . ': ' . (($this->_pcpInfo['title']) ? $this->_pcpInfo['title'] : $this->_values['title']); @@ -718,7 +721,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr //unset the billing parameters if it is pay later mode //to avoid creation of billing location - if ($params['is_pay_later'] && !$this->_isBillingAddressRequiredForPayLater) { + if ($isPayLater && !$this->_isBillingAddressRequiredForPayLater) { $billingFields = array( 'billing_first_name', 'billing_middle_name', @@ -994,7 +997,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr } } } - $this->processMembership($membershipParams, $contactID, $customFieldsFormatted, $fieldTypes, $premiumParams, $membershipLineItems); + $this->processMembership($membershipParams, $contactID, $customFieldsFormatted, $fieldTypes, $premiumParams, $membershipLineItems, $isPayLater); if (!$this->_amount > 0.0 || !$membershipParams['amount']) { // we need to explicitly create a CMS user in case of free memberships // since it is done under processConfirm for paid memberships @@ -1023,14 +1026,39 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr } $fieldTypes = array('Contact', 'Organization', 'Contribution'); } + $financialTypeID = $this->wrangleFinancialTypeID($contributionTypeId); CRM_Contribute_BAO_Contribution_Utils::processConfirm($this, $paymentParams, $premiumParams, $contactID, - $contributionTypeId, + $financialTypeID, 'contribution', - $fieldTypes + $fieldTypes, + ($this->_mode == 'test') ? 1 : 0, + $isPayLater + ); + } + } + + /** + * This wrangling of the financialType ID was happening in a shared function rather than in the form it relates to & hence has been moved to that form + * Pledges are not relevant to the membership code so that portion will not go onto the membership form. + * + * Comments from previous refactor indicate doubt as to what was going on + * @param $contributionTypeId + * + * @return null|string + */ + function wrangleFinancialTypeID($contributionTypeId) { + if (isset($paymentParams['financial_type'])) { + $contributionTypeId = $paymentParams['financial_type']; + } + elseif (!empty($this->_values['pledge_id'])) { + $contributionTypeId = CRM_Core_DAO::getFieldValue('CRM_Pledge_DAO_Pledge', + $this->_values['pledge_id'], + 'financial_type_id' ); } + return $contributionTypeId; } /** @@ -1756,8 +1784,9 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr * @param array $fieldTypes * @param array $premiumParams * @param array $membershipLineItems line items specifically relating to memberships + * @param $isPayLater */ - public function processMembership($membershipParams, $contactID, $customFieldsFormatted, $fieldTypes, $premiumParams, $membershipLineItems) { + public function processMembership($membershipParams, $contactID, $customFieldsFormatted, $fieldTypes, $premiumParams, $membershipLineItems, $isPayLater) { try { $membershipTypeIDs = (array) $membershipParams['selectMembership']; $membershipTypes = CRM_Member_BAO_Membership::buildMembershipTypeValues($this, $membershipTypeIDs); @@ -1783,7 +1812,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr CRM_Member_BAO_Membership::postProcessMembership($membershipParams, $contactID, $this, $premiumParams, $customFieldsFormatted, $fieldTypes, $membershipType, $membershipTypeIDs, $isPaidMembership, $this->_membershipId, $isProcessSeparateMembershipTransaction, $contributionTypeId, - $membershipLineItems + $membershipLineItems, $isPayLater ); $this->assign('membership_assign', TRUE); $this->set('membershipTypeID', $membershipParams['selectMembership']); diff --git a/CRM/Contribute/Form/Contribution/Main.php b/CRM/Contribute/Form/Contribution/Main.php index 000f3f905c..0f0aaa0d1e 100644 --- a/CRM/Contribute/Form/Contribution/Main.php +++ b/CRM/Contribute/Form/Contribution/Main.php @@ -57,7 +57,7 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu public $_useForMember; /** - * array of payment related fields to potentially display on this form (generally credit card or debit card fields). Th + * array of payment related fields to potentially display on this form (generally credit card or debit card fields). This is rendered via billingBlock.tpl * @var array */ public $_paymentFields = array(); @@ -1390,7 +1390,7 @@ class CRM_Contribute_Form_Contribution_Main extends CRM_Contribute_Form_Contribu /** * Handle Payment Processor switching * For contribution and event registration forms - * @param CRM_Contribute_Form_Contribution_Main $form + * @param CRM_Contribute_Form_Contribution_Main|CRM_Event_Form_Registration_Register $form * @param bool $noFees */ static function preProcessPaymentOptions(&$form, $noFees = FALSE) { diff --git a/CRM/Core/Payment/Form.php b/CRM/Core/Payment/Form.php index 51fb59e933..7d618e0ef9 100644 --- a/CRM/Core/Payment/Form.php +++ b/CRM/Core/Payment/Form.php @@ -67,7 +67,7 @@ class CRM_Core_Payment_Form { // @todo - replace this section with one similar to above per discussion - probably use a manual processor shell class to stand in for that capability //return without adding billing fields if billing_mode = 4 (@todo - more the ability to set that to the payment processor) // or payment processor is NULL (pay later) - if (($processor == NULL && !$forceBillingFieldsForPayLater) || $processor['billing_mode'] == 4) { + if (($processor == NULL && !$forceBillingFieldsForPayLater) || CRM_Utils_Array::value('billing_mode', $processor) == 4) { return; } //@todo setPaymentFields defines the billing fields - this should be moved to the processor class & renamed getBillingFields @@ -415,8 +415,7 @@ class CRM_Core_Payment_Form { */ protected static function addPaypalExpressCode(&$form) { if (empty($form->isBackOffice)) { - if ($form->_paymentProcessor['billing_mode'] & - CRM_Core_Payment::BILLING_MODE_BUTTON + if (CRM_Utils_Array::value('billing_mode', $form->_paymentProcessor) ==4 ) { $form->_expressButtonName = $form->getButtonName('upload', 'express'); $form->assign('expressButtonName', $form->_expressButtonName); diff --git a/CRM/Event/Form/Registration/Register.php b/CRM/Event/Form/Registration/Register.php index a77e73cc22..2d56816d13 100644 --- a/CRM/Event/Form/Registration/Register.php +++ b/CRM/Event/Form/Registration/Register.php @@ -62,13 +62,13 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { public $_quickConfig = NULL; /** - * Allow deveopera to use hook_civicrm_buildForm() + * Allow developer to use hook_civicrm_buildForm() * to override the registration dupe check * CRM-7604 */ public $_skipDupeRegistrationCheck = FALSE; - public $_ppType; + public $_paymentProcessorID; public $_snippet; /** @@ -76,6 +76,12 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { */ public $_noFees; + /** + * array of payment related fields to potentially display on this form (generally credit card or debit card fields). This is rendered via billingBlock.tpl + * @var array + */ + public $_paymentFields = array(); + /** * Function to set variables up before form is built * @@ -87,7 +93,7 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { //CRM-4320. //here we can't use parent $this->_allowWaitlist as user might - //walk back and we maight set this value in this postProcess. + //walk back and we might set this value in this postProcess. //(we set when spaces < group count and want to allow become part of waiting ) $eventFull = CRM_Event_BAO_Participant::eventFull($this->_eventId, FALSE, CRM_Utils_Array::value('has_waitlist', $this->_values['event'])); @@ -139,8 +145,9 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { * @return void */ function setDefaultValues() { - if ($this->_ppType && $this->_snippet && !($this->_paymentProcessor['billing_mode'] & CRM_Core_Payment::BILLING_MODE_FORM)) { - // see function comment block for explanation of this + if ($this->_paymentProcessorID && $this->_snippet && !($this->_paymentProcessor['billing_mode'] & CRM_Core_Payment::BILLING_MODE_FORM)) { + // see function comment block for explanation of this. Note that CRM-15555 will require this to look at the billing form fields not the + // billing_mode which return; } $this->_defaults = array(); @@ -168,7 +175,6 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { } if ($contactID) { - $options = array(); $fields = array(); if (!empty($this->_fields)) { @@ -327,13 +333,10 @@ class CRM_Event_Form_Registration_Register extends CRM_Event_Form_Registration { $this->set('profileAddressFields', $profileAddressFields); } - // Build payment processor form - if ($this->_ppType || $this->_isBillingAddressRequiredForPayLater) { - CRM_Core_Payment_ProcessorForm::buildQuickForm($this); - // Return if we are in an ajax callback - if ($this->_snippet) { - return; - } + CRM_Core_Payment_ProcessorForm::buildQuickForm($this); + // Return if we are in an ajax callback + if ($this->_snippet) { + return; } $contactID = $this->getContactID(); diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php index c6026d47e6..2f3709f4c1 100644 --- a/CRM/Member/BAO/Membership.php +++ b/CRM/Member/BAO/Membership.php @@ -1271,11 +1271,20 @@ AND civicrm_membership.is_test = %2"; */ public static function postProcessMembership($membershipParams, $contactID, &$form, $premiumParams, $customFieldsFormatted = NULL, $includeFieldTypes = NULL, $membershipDetails, $membershipTypeIDs, $isPaidMembership, $membershipID, - $isProcessSeparateMembershipTransaction, $defaultContributionTypeID, $membershipLineItems) { + $isProcessSeparateMembershipTransaction, $defaultContributionTypeID, $membershipLineItems, $isPayLater) { $result = $membershipContribution = NULL; $isTest = CRM_Utils_Array::value('is_test', $membershipParams, FALSE); $errors = $createdMemberships = array(); + //@todo move this into the calling function & pass in the correct financialTypeID + if (isset($paymentParams['financial_type'])) { + $financialTypeID = $paymentParams['financial_type']; + } + else { + $financialTypeID = $defaultContributionTypeID; + } + + if (CRM_Utils_Array::value('membership_source', $form->_params)) { $membershipParams['contribution_source'] = $form->_params['membership_source']; } @@ -1283,8 +1292,11 @@ AND civicrm_membership.is_test = %2"; if ($isPaidMembership) { $result = CRM_Contribute_BAO_Contribution_Utils::processConfirm($form, $membershipParams, $premiumParams, $contactID, - $defaultContributionTypeID, - 'membership' + $financialTypeID, + 'membership', + array(), + $isTest, + $isPayLater ); if (is_a($result[1], 'CRM_Core_Error')) { $errors[1] = CRM_Core_Error::getMessages($result[1]); -- 2.25.1