Modify backend Record Payment forms to use payment create API #13647

monishdeb · 2019-02-20T09:16:29Z

Overview

Modify backend Record Payment forms to use payment create API and move API code to BAO layer.

Before

Payment create api exists but is not used by backend record payment forms.

After

Payment create API is used to records payments for pending or partially paid contributions.

Comments

This is a sub-PR of #13330

ping @monishdeb @eileenmcnaughton

civibot · 2019-02-20T09:16:31Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

eileenmcnaughton · 2019-02-20T11:15:29Z

api/v3/Payment.php

+      'type' => CRM_Utils_Type::T_STRING,
+      'description' => ts("Type of payment - 'owed' or 'refund'."),
+    ),
+    'is_email_receipt' => array(


@monishdeb so the thing I've been saying is I think instead of adding these 2 parameters we should add a call above to Payment.sendconfirmation

the reason being I'm not convinced that is_email_receipt should sent the payment notification - I think it should likely be a bool for whether completetransaction type emails go out - but we can punt that decision by using the payment.sendconfirmation for now

Oops I missed it, removing it

@monishdeb once you merge this #13610 we can switch over to Payment.sendconfirmation

eileenmcnaughton · 2019-02-20T11:16:03Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -609,10 +591,12 @@ public function testSubmit($params, $creditCardMode = NULL, $entityType = 'contr
      if (!empty($paymentInfo['refund_due'])) {
        $this->_refund = $paymentInfo['refund_due'];
        $this->_paymentType = 'refund';
+        $this->paymentAmount = $paymentInfo['refund_due'];


I don't think we need this once we start calling sendconfirmation per below

eileenmcnaughton · 2019-02-20T11:16:24Z

CRM/Contribute/Form/AdditionalPayment.php

+    $payment = civicrm_api3('Payment', 'create', array_merge([
+      'contribution_id' => $this->_contributionId,
+      'total_amount' => $this->_params['total_amount'],
+      'payment_type' => $this->_paymentType,


I don't think we need to pass this in per below

eileenmcnaughton · 2019-02-20T11:16:50Z

CRM/Contribute/Form/AdditionalPayment.php

      $this->_paymentType = 'refund';
    }
    elseif (!empty($paymentInfo['amount_owed'])) {
-      $paymentAmt = $this->_owed = $paymentInfo['amount_owed'];
+      $this->paymentAmount = $this->_owed = $paymentInfo['amount_owed'];


I don't think we need this - per below

eileenmcnaughton · 2019-02-20T11:16:51Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -110,11 +110,11 @@ public function preProcess() {
    $this->_amtTotal = $paymentDetails['total'];

    if (!empty($paymentInfo['refund_due'])) {
-      $paymentAmt = $this->_refund = $paymentInfo['refund_due'];


I don't think we need this - per below

eileenmcnaughton · 2019-02-20T11:16:55Z

CRM/Contribute/Form/AdditionalPayment.php

@@ -130,7 +130,7 @@ public function preProcess() {
    $this->assign('contributionMode', $this->_mode);
    $this->assign('contactId', $this->_contactID);
    $this->assign('paymentType', $this->_paymentType);
-    $this->assign('paymentAmt', abs($paymentAmt));
+    $this->assign('paymentAmt', abs($this->paymentAmount));


I don't think we need this - per below

eileenmcnaughton · 2019-02-20T11:19:18Z

CRM/Contribute/Form/AdditionalPayment.php

+      'total_amount' => $this->_params['total_amount'],
+      'payment_type' => $this->_paymentType,
+      'participant_id' => $participantId,
+      'mode' => $this->_mode,


I don't think payment.create uses mode or participant_id

eileenmcnaughton · 2019-02-20T11:20:35Z

api/v3/Payment.php

@@ -169,6 +169,16 @@ function _civicrm_api3_payment_create_spec(&$params) {
      'type' => CRM_Utils_Type::T_INT,
      'api.aliases' => array('payment_id'),
    ),
+    'payment_type' => array(


this param is only for the receipts too - not needed

monishdeb · 2019-02-20T11:32:27Z

@eileenmcnaughton can you please check now?

eileenmcnaughton · 2019-02-20T11:34:38Z

tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php

@@ -218,17 +218,28 @@ public function testTransactionInfo() {
    $submittedValues = array(
      'total_amount' => 20,
      'payment_instrument_id' => 3,
+      'payment_type' => 'owed',


let's only add contribution_id here as the other payment_type etc aren't needed

eileenmcnaughton · 2019-02-20T11:35:05Z

tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php


    //Record a refund of the remaining amount.
    $submittedValues['total_amount'] = 50;
-    CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionID, $submittedValues, 'refund', $result['participant']['id']);
-    $paymentInfo = CRM_Contribute_BAO_Contribution::getPaymentInfo($result['participant']['id'], 'event', TRUE);
+    $submittedValues['payment_type'] = 'refund';


ditto here - no payment_type

monishdeb · 2019-02-20T11:35:48Z

tests/phpunit/CRM/Event/BAO/AdditionalPaymentTest.php

+      'payment_type' => 'owed',
+      'participant_id' => $participant['id'],
+      'contribution_id' => $contributionID,
+      'is_email_receipt' => TRUE,


ok let me remove these 3 except contribution_id

eileenmcnaughton · 2019-02-20T11:35:57Z

@monishdeb OK - let's see how the test goes - i think merging #13610 is going to help us here though

eileenmcnaughton · 2019-02-21T05:51:03Z

@monishdeb do I feel like we were expecting some fails - the first one should be resolved if we can get the payment.sendconfirmation pr/s merged

eileenmcnaughton · 2019-02-22T19:22:45Z

test this please

eileenmcnaughton · 2019-02-22T19:26:23Z

I think the other PR will have fixed one fail. Once tests are passing my thoughts here is we should do a mini-audit of differences between recordAdditionalPayment & Payment.create - & perhaps a few UI tests (although test cover is pretty good) and we should probably cleanup the form a little & remove unneeded stuff & them not make any more payment related changes until the next rc is cut

eileenmcnaughton · 2019-02-24T04:57:15Z

CRM/Contribute/Form/AdditionalPayment.php

-    $params = array('id' => $this->_contributionId);
-    $contribution = CRM_Contribute_BAO_Contribution::retrieve($params, $defaults, $params);
-    CRM_Contribute_BAO_Contribution::addPayments(array($contribution), $contributionStatusId);
+    $payment = civicrm_api3('Payment', 'create', array_merge([


So we now need to sync this with below (which expects a var called $result) - this would do it

index e168ff4adf..0143de2658 100644 --- a/CRM/Contribute/Form/AdditionalPayment.php +++ b/CRM/Contribute/Form/AdditionalPayment.php @@ -362,8 +362,8 @@ class CRM_Contribute_Form_AdditionalPayment extends CRM_Contribute_Form_Abstract $statusMsg = ts('The payment record has been processed.'); // send email - if (!empty($result) && !empty($this->_params['is_email_receipt'])) { - $sendResult = civicrm_api3('Payment', 'sendconfirmation', ['id' => $result->id])['values'][$result->id]; + if (!empty($this->_params['is_email_receipt'])) { + $sendResult = civicrm_api3('Payment', 'sendconfirmation', ['id' => $payment['id']])['values'][$payment['id]']; if ($sendResult['is_sent']) { $statusMsg .= ' ' . ts('A receipt has been emailed to the contributor.'); }

eileenmcnaughton · 2019-02-24T05:02:19Z

OK - I added a comment about the top test fail.

However, after fixing that it now sends out TWO emails - the payment notification AND a contribution notification - this was what I was talking about before - ie. what should be sent when payment.create api is called.

Perhaps the param we want here on Payment.create is 'is_send_confirmation_on_completed_contribution' = > 0

that would be passed through to the Contribution.completetransaction api.

We could perhaps find something shorter but is_email_receipt is ambiguous & abbreviations can be hard to remember.

monishdeb · 2019-02-25T03:23:22Z

@eileenmcnaughton I have encountered multiple issues with the original issue fix:

The original PR which extends a UT to assert the refund financial trxns on fee-change was not working.
Payment.create API doesn't handle refund payment via backoffice form unlike partial payment for which we got FinancialTrxn::getPartialPaymentTrxn();
Via 'Record refund' backoffice form if we submit the refund amount, it creates extraneous records in FinancialTrxn table.

So I am thinking of extracting the refund code for recordAdditionalPayment() into separate fn say FinancialTrxn::getPartialPaymentTrxn() and handle the refund payment there? And at last it will call completetransaction API to complete the tied contribution?

eileenmcnaughton · 2019-02-25T03:30:23Z

@monishdeb I think that what the form is doing is what we want the api to support - the api is immature :-(

I think we can update it to support the other scenarios - the goal here is to get to a point where we have one reliable thing that works & which we can use from various places.

I'm just trying to get my head around the functions involved....

eileenmcnaughton · 2019-02-25T03:35:02Z

@monishdeb so RecordAdditionalPayment is only called twice outside of the unit tests - that's good

eileenmcnaughton · 2019-02-25T04:02:39Z

@monishdeb what about tackling it this way - #13689 - ie. resolve getting the cancel right via the api before we touch the form

eileenmcnaughton · 2019-02-25T04:14:35Z

Hmm - maybe this is what you are suggesting....

eileenmcnaughton · 2019-02-25T04:22:45Z

#13690

eileenmcnaughton · 2019-03-17T23:54:11Z

Our current blocker on this is #13689 - let's close this one down until we've finished merging recordAdditionalPayment with Payment.create since it can't be reviewed until we have finished that (& this part is the easy part :-)

civibot bot added the master label Feb 20, 2019

monishdeb mentioned this pull request Feb 20, 2019

[WIP] Modify backend Record Payment forms to use payment create API. #13330

Closed

eileenmcnaughton reviewed Feb 20, 2019

View reviewed changes

monishdeb force-pushed the partial-2 branch 4 times, most recently from 51df384 to 2f3264b Compare February 20, 2019 11:31

eileenmcnaughton reviewed Feb 20, 2019

View reviewed changes

monishdeb commented Feb 20, 2019

View reviewed changes

monishdeb force-pushed the partial-2 branch 5 times, most recently from 6508fd1 to b8c418f Compare February 21, 2019 03:09

Modify backend Record Payment forms to use payment create API

b43cb00

monishdeb force-pushed the partial-2 branch from b8c418f to b43cb00 Compare February 21, 2019 06:50

eileenmcnaughton reviewed Feb 24, 2019

View reviewed changes

eileenmcnaughton self-assigned this Feb 24, 2019

eileenmcnaughton mentioned this pull request Feb 25, 2019

Clean up Payment.create function #13690

Merged

magnolia61 mentioned this pull request Mar 6, 2019

dev/financial#20 - Extend pay now functionality for partially paid contribut… #12319

Closed

eileenmcnaughton closed this Mar 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify backend Record Payment forms to use payment create API #13647

Modify backend Record Payment forms to use payment create API #13647

monishdeb commented Feb 20, 2019

civibot bot commented Feb 20, 2019

eileenmcnaughton Feb 20, 2019

monishdeb Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

monishdeb commented Feb 20, 2019

eileenmcnaughton Feb 20, 2019

eileenmcnaughton Feb 20, 2019

monishdeb Feb 20, 2019

eileenmcnaughton commented Feb 20, 2019

eileenmcnaughton commented Feb 21, 2019

eileenmcnaughton commented Feb 22, 2019

eileenmcnaughton commented Feb 22, 2019

eileenmcnaughton Feb 24, 2019

eileenmcnaughton commented Feb 24, 2019

monishdeb commented Feb 25, 2019 •

edited

Loading

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Mar 17, 2019

Modify backend Record Payment forms to use payment create API #13647

Modify backend Record Payment forms to use payment create API #13647

Conversation

monishdeb commented Feb 20, 2019

Overview

Before

After

Comments

civibot bot commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 20, 2019

eileenmcnaughton commented Feb 21, 2019

eileenmcnaughton commented Feb 22, 2019

eileenmcnaughton commented Feb 22, 2019

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 24, 2019

monishdeb commented Feb 25, 2019 • edited Loading

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Feb 25, 2019

eileenmcnaughton commented Mar 17, 2019

monishdeb commented Feb 25, 2019 •

edited

Loading