Make contribution_id mandatory for PaymentProcessor.Pay API

[mattwire]

Overview

Make contribution_id mandatory for API PaymentProcessor.pay. Per https://lab.civicrm.org/dev/financial/issues/53 we should always have a contribution before calling doPayment - that means we must pass in a contribution_id.

Before

contribution_id not required by API PaymentProcessor.pay.

After

contribution_id required by API PaymentProcessor.pay.

Technical Details

See https://lab.civicrm.org/dev/financial/issues/53.

Comments

@JoeMurray @eileenmcnaughton I'm trying to clear off a few "simple" fixes and backport them to the 5.19 RC so we've got a set of more useable APIs from 5.19 on. These should all be "safe" changes.

[civibot]

(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]

Looks like I looked at these back to front :-)

I'm just trying to think about whether there are some cases where the payment needs to be started on the first page (before the contribution id exists) or whether we can always use a separate preApprove action there (which might need to be in core as an api)

[mattwire]

I'm just trying to think about whether there are some cases where the payment needs to be started on the first page (before the contribution id exists) or whether we can always use a separate preApprove action there (which might need to be in core as an api)

@eileenmcnaughton I was tasked with documenting this API so thought I'd better actually try using it :-)

As this API is basically a thin wrapper for doPayment it should not be handling anything other than that. We probably want another API PaymentProcessor.doPreApprove but that is not blocking on this PR.

We've agreed in https://lab.civicrm.org/dev/financial/issues/53 that we must always have a contribution before processing payment so by definition we must have a contribution ID when calling PaymentProcessor.pay

[eileenmcnaughton]

Ok - I took a look at the 2 places I know of using this api (which is probably the only 2).

Omnipay looks like this

      $payment = civicrm_api3('PaymentProcessor', 'pay', array(
        'amount' => $originalContribution['total_amount'],
        'currency' => $originalContribution['currency'],
        'payment_processor_id' => $paymentProcessorID,
        'contributionID' => $pending['id'],
        'contactID' => $originalContribution['contact_id'],
        'description' => ts('Repeat payment, original was ' . $originalContribution['id']),
        'token' => civicrm_api3('PaymentToken', 'getvalue', [
          'id' => $recurringPayment['payment_token_id'],
          'return' => 'token',
        ]),
        'payment_action' => 'purchase',
      ));

contributionID has some 'credibility' as an input for doPayment & I think it makes sense to add it as an alias so either / or are supported.

@ejegg the only other place is in SmashPig. To comply with this change the flow would need to change from

        $payment = $this->makePayment($paymentParams);
        $this->recordPayment(
          $payment, $recurringPayment, $previousContribution
        );

to creating the pending payment first & updating it after. This is the agreed flow & I think it makes sense for us to change SmashPig when we next update to comply.

[mattwire]

contributionID has some 'credibility' as an input for doPayment & I think it makes sense to add it as an alias so either / or are supported.

@eileenmcnaughton But this is a "new" API. Historically we've ended in a huge mess where you might need to retrieve the contribution ID from various places. Can we just standardise at the API interface and support ONLY one parameter - contribution_id. If there is already code using PaymentProcessor.Pay API they can update (you've probably identified the only two users).

[eileenmcnaughton]

@mattwire yes - so the issue is the parameters are passed through to doPayment from various places currently & the list above kinda reflects those.

It would be good to normalise those but I think we'd need to add getters & setters on CRM_Core_Payment like

diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php
index 69b97a342f..7f7ea78b0f 100644
--- a/CRM/Core/Payment.php
+++ b/CRM/Core/Payment.php
@@ -149,6 +149,27 @@ abstract class CRM_Core_Payment {
    */
   protected $backOffice = FALSE;
 
+  /**
+   * Contribution that is being paid.
+   *
+   * @var int
+   */
+  protected $contributionID;
+
+  /**
+   * @return int
+   */
+  public function getContributionID(): int {
+    return $this->contributionID;
+  }
+
+  /**
+   * @param int $contributionID
+   */
:...skipping...
diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php
index 69b97a342f..7f7ea78b0f 100644
--- a/CRM/Core/Payment.php
+++ b/CRM/Core/Payment.php
@@ -149,6 +149,27 @@ abstract class CRM_Core_Payment {
    */
   protected $backOffice = FALSE;
 
+  /**
+   * Contribution that is being paid.
+   *
+   * @var int
+   */
+  protected $contributionID;
+
+  /**
+   * @return int
+   */
+  public function getContributionID(): int {
+    return $this->contributionID;
+  }
+
+  /**
+   * @param int $contributionID
+   */
+  public function setContributionID(int $contributionID) {
+    $this->contributionID = $contributionID;
+  }
+
   /**
    * @return bool
    */
@@ -1219,6 +1240,15 @@ abstract class CRM_Core_Payment {
     return $params;
   }
 
+  /**
+   * Set class variables from params.
+   *
+   * @param array $params
+   */
+  protected function setVariables($params) {
+    $this->setContributionID($params['contribution_id'] ?? $params['contributionID']);
+  }
+
   /**
    * Process payment - this function wraps around both doTransferCheckout and doDirectPayment.
    * Any processor that still implements the deprecated doTransferCheckout() or doDirectPayment() should be updated to use doPayment().
Eileens-MacBook-Pro:civicrm eileenmcnaughton$ git diff
diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php
index 69b97a342f..0f13fed404 100644
--- a/CRM/Core/Payment.php
+++ b/CRM/Core/Payment.php
@@ -149,6 +149,27 @@ abstract class CRM_Core_Payment {
    */
   protected $backOffice = FALSE;
 
+  /**
+   * Contribution that is being paid.
+   *
+   * @var int
+   */
+  protected $contributionID;
+
+  /**
+   * @return int
+   */
+  public function getContributionID(): int {
+    return $this->contributionID;
+  }
+
+  /**
+   * @param int $contributionID
+   */
+  public function setContributionID(int $contributionID) {
+    $this->contributionID = $contributionID;
+  }
+
   /**
    * @return bool
    */
@@ -1219,6 +1240,15 @@ abstract class CRM_Core_Payment {
     return $params;
   }
 
+  /**
+   * Set class variables from params.
+   *
+   * @param array $params
+   */
+  protected function setVariables($params) {
+    $this->setContributionID($params['contribution_id'] ?? $params['contributionID']);
+  }
+
   /**
    * Process payment - this function wraps around both doTransferCheckout and doDirectPayment.
    * Any processor that still implements the deprecated doTransferCheckout() or doDirectPayment() should be updated to use doPayment().
@@ -1245,6 +1275,7 @@ abstract class CRM_Core_Payment {
    */
   public function doPayment(&$params, $component = 'contribute') {
     $this->_component = $component;
+    $this->setVariables($params);
     $statuses = CRM_Contribute_BAO_Contribution::buildOptions('contribution_status_id', 'validate');
 
     // If we have a $0 amount, skip call to processor and set payment_status to Completed.
(END)

[mattwire]

@eileenmcnaughton Right, I misunderstood your comment. The API should only accept contribution_id obviously, but we need to pass contributionID through to doPayment. I already have a getter in my shared payment extension: https://lab.civicrm.org/extensions/mjwshared/blob/master/CRM/Core/Payment/MJWTrait.php#L134 It's one of the only ones that only actually checks one parameter!

[mattwire]

@eileenmcnaughton I've pushed a change that sets contributionID

[eileenmcnaughton]

@mattwire I'd rather see us put some getters & setters in the CRM_Core_Payment than wrangle it in the api but I'd merge this now if it had a test

[mattwire]

@eileenmcnaughton Do we have any existing tests in core for PaymentProcessor.Pay API? I can't find anything.

[eileenmcnaughton]

@mattwire maybe not! What we can use to check what is happening within it is the alterPaymentProcessorParams hook - to check doDirectPayment receives contributionID

I think next we should define a bunch of getters & setters for the params we expect to be available to CRM_Core_Payment - we have done amount in the past which standardised that

[eileenmcnaughton]

@mattwire I turned the comment above into a PR #15509

[ejegg]

@eileenmcnaughton our recurring processor has an option to either create the donations directly or send them to a queue. We're running it with the queue setting as the queue consumer does some pre-processing to fill in a bunch of our custom fields.

This change would mean we'd need to either let the queue consumer update a transaction from Pending to Completed while adding the custom fields (current code would drop the queue message as a duplicate txn) or, if we got rid of the 'send to queue' option, move that pre-processing to a Civi save hook.

Ours is a pretty odd use case, so it doesn't feel right to block a core change if it would be more correct for the vast majority of users.

[mattwire]

What we can use to check what is happening within it is the alterPaymentProcessorParams hook - to check doPayment receives contributionID

@sarvesh211999 Can you write a unit test for this? An example can be found in api/v3/ContributionPageTest.php: $this->hookClass->setHook('civicrm_alterPaymentProcessorParams', [$this, 'hook_civicrm_alterPaymentProcessorParams']); The new test should be in api/v3/PaymentProcessorTest.php

[eileenmcnaughton]

@mattwire do you want to add a call to $processor->setContributionID() - it will duplicate passing it in but I think that is how we transition.

[eileenmcnaughton]

I did the test in #15639 so closing this