Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Call Payment.create from payment.cancel #13689

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Call Payment.create from payment.cancel #13689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3339,6 +3339,9 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues =
'net_amount' => CRM_Utils_Array::value('net_amount', $params, $totalAmount),
'currency' => $params['contribution']->currency,
'trxn_id' => $params['contribution']->trxn_id,
// @todo - this is getting the status id from the contribution - that is BAD - ie the contribution could be partially
// paid but each payment is completed. The work around is to pass in the status_id in the trxn_params but
// this should really default to completed (after discussion).
'status_id' => $statusId,
'payment_instrument_id' => CRM_Utils_Array::value('payment_instrument_id', $params, $params['contribution']->payment_instrument_id),
'check_number' => CRM_Utils_Array::value('check_number', $params),
Expand Down Expand Up @@ -3893,6 +3896,7 @@ public static function recordAdditionalPayment($contributionId, $trxnsData, $pay
$financialTrxn = CRM_Financial_BAO_Payment::recordPayment($contributionId, $trxnsData, $participantId);
}
elseif ($paymentType == 'refund') {
$trxnsData['total_amount'] = -$trxnsData['total_amount'];
$financialTrxn = CRM_Financial_BAO_Payment::recordRefundPayment($contributionId, $trxnsData, $updateStatus);
if ($participantId) {
// update participant status
Expand Down
7 changes: 5 additions & 2 deletions CRM/Financial/BAO/Payment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public static function create($params) {
// should be handled through Payment.create.
$isSkipRecordingPaymentHereForLegacyHandlingReasons = ($contributionStatus == 'Pending' && $isPaymentCompletesContribution);

if (!$isSkipRecordingPaymentHereForLegacyHandlingReasons) {
if (!$isSkipRecordingPaymentHereForLegacyHandlingReasons && $params['total_amount'] > 0) {
$trxn = CRM_Contribute_BAO_Contribution::recordPartialPayment($contribution, $params);

if (CRM_Utils_Array::value('line_item', $params) && !empty($trxn)) {
Expand Down Expand Up @@ -99,6 +99,9 @@ public static function create($params) {
CRM_Contribute_BAO_Contribution::assignProportionalLineItems($params, $trxn->id, $contribution['total_amount']);
}
}
elseif ($params['total_amount'] < 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb I've updated this PR with this call to record refunds from here - hopefully that means payment_cancel can call Payment.create now & we are closer to being rid of recordAdditionalPaymen

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton Yes, that would be fine.

Also, let me share my thoughts about what I was supposed to do to support refund using payment processor. I am thinking of having a new api payment.refund which will do refund using payment processor and eventually will call Payment.cancel API to record financial records - https://github.com/civicrm/civicrm-core/compare/master...JMAConsulting:financial-38?expand=1#diff-8967f6e57d6209aa277ded6512413608R510

Copy link
Member

@monishdeb monishdeb Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then for refund w/o PP (i.e. Manual PP), only call to payment.cancel will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb so I'm not sold on having one api do both the payment processor refund AND process the refund payment. I think we could sink into various edge cases so I lean towards having

PaymentProcessor.pay (already in Omnipay) and PaymentProcessor.refund (which would call doRefund on the processor class) and then the calling code can call either Payment.create or Payment.cancel - I'm not sure which makes more sense but they will eventually be more or less the same code anyway

Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I also think we can add PaymentProcessor.refund first & then as we start to use it we can consider if we have needs not met by the Payment apis & the PaymentProcessor.refund api rather than decide that up front - as the work for PaymentProcessor.refund will be required regardless).

My worry is that traditionally the financial functions have been overloaded with ... all sort of stuff - so if we can keep them simple & clean we might have less problems

$trxn = self::recordRefundPayment($params['contribution_id'], $params, FALSE);
}

if ($isPaymentCompletesContribution) {
civicrm_api3('Contribution', 'completetransaction', array('id' => $contribution['id']));
Expand Down Expand Up @@ -310,7 +313,7 @@ public static function recordRefundPayment($contributionId, $trxnData, $updateSt
$arAccountId = CRM_Contribute_PseudoConstant::getRelationalFinancialAccount($contributionDAO->financial_type_id, 'Accounts Receivable Account is');
$completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');

$trxnData['total_amount'] = $trxnData['net_amount'] = -$trxnData['total_amount'];
$trxnData['total_amount'] = $trxnData['net_amount'] = $trxnData['total_amount'];
$trxnData['from_financial_account_id'] = $arAccountId;
$trxnData['status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Refunded');
// record the entry
Expand Down
27 changes: 20 additions & 7 deletions api/v3/Payment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,20 @@ function civicrm_api3_payment_cancel(&$params) {
'financial_trxn_id' => $params['id'],
];
$entity = civicrm_api3('EntityFinancialTrxn', 'getsingle', $eftParams);
$contributionId = $entity['entity_id'];
$params['total_amount'] = $entity['amount'];
unset($params['id']);

$trxn = CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionId, $params, 'refund', NULL, FALSE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton I think this API should be called for cancelled contribution, isn't it? Because the refund handling here isn't quite right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb right - this PR can't be merged until we've resolved the refund handling - which we could add to this PR - I think just extracting makes sense - shall I add that so you can see what I mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure why not :)

$paymentParams = [
'total_amount' => -$entity['amount'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton I think we shouldn't negate here? Test failure complains about the same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb you are right - but I think we should be setting to negative 'up high' and handling that lower down so I have removed the expectation of a positive number in

https://github.com/civicrm/civicrm-core/pull/13689/files#diff-8967f6e57d6209aa277ded6512413608R316

and fixed the one other place that calls that to also pass in a negative number

https://github.com/civicrm/civicrm-core/pull/13689/files#diff-7a5b0e2d131dabc49178460fc63328c0R3882

There is still a test fail on status which I'm making sense of

'contribution_id' => $entity['entity_id'],
'trxn_date' => CRM_Utils_Array::value('trxn_date', $params, 'now'),
];

$values = [];
_civicrm_api3_object_to_array_unique_fields($trxn, $values[$trxn->id]);
return civicrm_api3_create_success($values, $params, 'Payment', 'cancel', $trxn);
foreach (['trxn_id', 'payment_instrument_id'] as $permittedParam) {
if (isset($params[$permittedParam])) {
$paymentParams[$permittedParam] = $params[$permittedParam];
}
}
$result = civicrm_api3('Payment', 'create', $paymentParams);
return civicrm_api3_create_success($result['values'], $params, 'Payment', 'cancel');
}

/**
Expand Down Expand Up @@ -169,6 +174,10 @@ function _civicrm_api3_payment_create_spec(&$params) {
'type' => CRM_Utils_Type::T_INT,
'api.aliases' => ['payment_id'],
],
'trxn_date' => [
'title' => 'Cancel Date',
'type' => CRM_Utils_Type::T_DATE + CRM_Utils_Type::T_TIME,
],
];
}

Expand Down Expand Up @@ -233,6 +242,10 @@ function _civicrm_api3_payment_cancel_spec(&$params) {
'type' => CRM_Utils_Type::T_INT,
'api.aliases' => ['payment_id'],
],
'trxn_date' => [
'title' => 'Cancel Date',
'type' => CRM_Utils_Type::T_DATE + CRM_Utils_Type::T_TIME,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, trxn_id is important for canceling a payment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb logically I agree with you - the current api is not really doing 'Payment.cancel' it's doing 'cancel one payment associated with the contribution' - which really makes me question whether the api is sensible & worth having. That aside, I think this PR doesn't CHANGE the situation in that I think trxn_id is not used currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh I think the only place Payment.cancel is currently called from is

function civicrm_api3_payment_create(&$params) {
  // Check if it is an update
  if (CRM_Utils_Array::value('id', $params)) {
    $amount = $params['total_amount'];
    civicrm_api3('Payment', 'cancel', $params);
    $params['total_amount'] = $amount;
  }

That is hurting my brain - I guess the idea is you can't alter a payment only reverse & recreate it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, my earlier point of not having that name as 'cancel' as we are not actually cancelling a existing payment but rather refunding the original payment. Reason why I was suggesting to use refund at it actually adds a new reverse transaction and doesn't affect the original payment. It keeps the clarity in bookkeeping report.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree PR doesn't change the workflow, I was suggesting to add more info. to this API so a dev. will be aware of what sort of parameter needed to refund a payment. Also Cancel Date is not appropriate it has to be termed as 'Refund Date' as we are actually refunding the payment. Like you earlier said Cancelling a contribution is something need to dealt at Form level ?

Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monishdeb So I'm actually suggesting it's even simpler - ie it doesn't call

 civicrm_api3('Payment', 'cancel', $result);

The calling code would call

civicrm_api3('PaymentProcessor', 'refund', $params);
 civicrm_api3('Payment', 'create', $result);

Whether we want an api that does those 2 lines depends a bit on how it looks as we get further down the track - but we could add Payment.ProcessRefund if we decide later on we need to

e.g here is the PaymentProcessor.pay api from Omnipay

/**
 * Action payment.
 *
 * @param array $params
 *
 * @return array
 *   API result array.
 * @throws CiviCRM_API3_Exception
 */
function civicrm_api3_payment_processor_pay($params) {
  $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']);
  $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', array('id' => $params['payment_processor_id'])));
  $result = $processor->doPayment($params);
  if (is_a($result, 'CRM_Core_Error')) {
    throw API_Exception('Payment failed');
  }
  return civicrm_api3_create_success(array($result), $params);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops missed the fact that Payment.create does call the Payment.cancel after ensuring its refund payment. I agree with you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's slightly terrifying - part of me is wondering if this PR will create a code loop that will crash the server.... But I guess it didn't before....

I feel like this is a BAD function but my goal in this PR is not to address that but just to get to the point where recordAdditionalPayment is only called from one place in the code....

Copy link
Member

@monishdeb monishdeb Mar 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes maybe I am seeing a bigger picture here to use Paymentprocessor to do partial/pending/refund payment all of which utilizes Payment.create API But that dream is after we simplify and strengthen the Payment.create BAO fn to handle all kind of additional payment and of course to get recordAdditionalPayment called from one place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right & maybe once we've one that I'll feel more comfortable about a more complex api - all I can see at the moment is this spaghetti of code trying to do 25 things at once & hence I'm resistant to more multipurpose functions while this is in front of me

],
];
}

Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/api/v3/PaymentTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ public function testRefundEmailReceipt($thousandSeparator) {
'from_financial_account_id' => 7,
'to_financial_account_id' => 6,
'total_amount' => -30,
'status_id' => 1,
'status_id' => CRM_Core_PseudoConstant::getKey('CRM_Core_BAO_FinancialTrxn', 'status_id', 'Refunded'),
'is_payment' => 1,
];
foreach ($expected as $key => $value) {
Expand Down Expand Up @@ -317,7 +317,7 @@ public function testCreatePaymentNoLineItems() {
*/
public function checkPaymentResult($payment, $expectedResult) {
foreach ($expectedResult[$payment['id']] as $key => $value) {
$this->assertEquals($payment['values'][$payment['id']][$key], $value);
$this->assertEquals($payment['values'][$payment['id']][$key], $value, 'mismatch on ' . $key);
}
}

Expand Down