dev/financial#77 ++ Make contribution_id mandatory for PaymentProcess…

…or.pay, pass incoieID

Replaces #15477 & also resolves https://lab.civicrm.org/dev/financial/issues/77

by requiring contribution_id for PaymentProcessor.pay as a way to ensure that it is only called after the order is created
per our preferred flow (people could still get past this but it feels like they would at least know they werre
hacking our process & take responsibility for any issues if it breaks or we suddenly start enforcing that
it is a valid contribution.

This also sets some of the recommended variables. Note that I had to use a few more lines to ensure we were always setting
contactID, contributionRecurID to an integer. I do think this stricter approach is better but it wound up
more verbose

api/v3/PaymentProcessor.php

@@ -124,11 +124,22 @@ function _civicrm_api3_payment_processor_getlist_defaults(&$request) {
  *   API result array.
  *
  * @throws \API_Exception
+ * @throws \CiviCRM_API3_Exception
  */
 function civicrm_api3_payment_processor_pay($params) {
+  /* @var CRM_Core_Payment $processor */
   $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']);
   $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', ['id' => $params['payment_processor_id']]));
   try {
+    $processor->setContributionID($params['contribution_id']);
+    $processor->setInvoiceID($params['invoice_id'] ?? '');
+    if (!empty($params['contact_id'])) {
+      $processor->setContactID((int) $params['contact_id']);
+    }
+    if (!empty($params['contribution_recur_id'])) {
+      $processor->setContributionRecurID((int) $params['contribution_recur_id']);
+    }
+
     $result = $processor->doPayment($params);
   }
   catch (\Civi\Payment\Exception\PaymentProcessorException $e) {
@@ -139,7 +150,7 @@ function civicrm_api3_payment_processor_pay($params) {
     }
     throw new API_Exception('Payment failed', $code, $errorData, $e);
   }
-  return civicrm_api3_create_success(array($result), $params);
+  return civicrm_api3_create_success([$result], $params);
 }
 
 /**
@@ -149,7 +160,7 @@ function civicrm_api3_payment_processor_pay($params) {
  */
 function _civicrm_api3_payment_processor_pay_spec(&$params) {
   $params['payment_processor_id'] = [
-    'api.required' => 1,
+    'api.required' => TRUE,
     'title' => ts('Payment processor'),
     'type' => CRM_Utils_Type::T_INT,
   ];
@@ -158,6 +169,23 @@ function _civicrm_api3_payment_processor_pay_spec(&$params) {
     'title' => ts('Amount to pay'),
     'type' => CRM_Utils_Type::T_MONEY,
   ];
+  $params['contribution_id'] = [
+    'api.required' => TRUE,
+    'title' => ts('Contribution ID'),
+    'type' => CRM_Utils_Type::T_INT,
+  ];
+  $params['contact_id'] = [
+    'title' => ts('Contact ID'),
+    'type' => CRM_Utils_Type::T_INT,
+  ];
+  $params['contribution_recur_id'] = [
+    'title' => ts('Contribution Recur ID'),
+    'type' => CRM_Utils_Type::T_INT,
+  ];
+  $params['invoice_id'] = [
+    'title' => ts('Invoice ID'),
+    'type' => CRM_Utils_Type::T_STRING,
+  ];
 }
 
 /**

tests/phpunit/api/v3/PaymentProcessorTest.php

@@ -35,6 +35,11 @@ class api_v3_PaymentProcessorTest extends CiviUnitTestCase {
   protected $_apiversion = 3;
   protected $_params;
 
+  /**
+   * Set up for class.
+   *
+   * @throws \CRM_Core_Exception
+   */
   public function setUp() {
     parent::setUp();
     $this->useTransaction(TRUE);
@@ -69,6 +74,8 @@ public function testPaymentProcessorCreateWithoutName() {
 
   /**
    * Create payment processor.
+   *
+   * @throws \Exception
    */
   public function testPaymentProcessorCreate() {
     $params = $this->_params;
@@ -88,6 +95,8 @@ public function testPaymentProcessorCreate() {
 
   /**
    * Update payment processor.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testPaymentProcessorUpdate() {
     $params = $this->_params;
@@ -131,6 +140,8 @@ public function testPaymentProcessorCreateExample() {
 
   /**
    * Check payment processor delete.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testPaymentProcessorDelete() {
     $result = $this->callAPISuccess('payment_processor', 'create', $this->_params);
@@ -143,6 +154,8 @@ public function testPaymentProcessorDelete() {
 
   /**
    * Check with valid params array.
+   *
+   * @throws \CRM_Core_Exception
    */
   public function testPaymentProcessorsGet() {
     $params = $this->_params;
@@ -158,4 +171,33 @@ public function testPaymentProcessorsGet() {
     $this->assertEquals('test@test.com', $results['values'][$results['id']]['user_name']);
   }
 
+  /**
+   * Test the processor passed to the hook can access the relevant variables.
+   *
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
+   */
+  public function testPaymentProcessorPay() {
+    $this->hookClass->setHook('civicrm_alterPaymentProcessorParams', [$this, 'hook_civicrm_alterPaymentProcessorParams']);
+    $processor = $this->dummyProcessorCreate();
+    $this->callAPISuccess('PaymentProcessor', 'pay', [
+      'payment_processor_id' => $processor->getID(),
+      'contribution_id' => 10,
+      'invoice_id' => 2,
+      'contribution_recur_id' => 3,
+      'amount' => 6,
+    ]);
+  }
+
+  /**
+   * Implements civicrm_alterPaymentProcessorParams hook.
+   *
+   * @param \CRM_Core_Payment_Dummy $paymentObject
+   */
+  public function hook_civicrm_alterPaymentProcessorParams($paymentObject) {
+    $this->assertEquals(10, $paymentObject->getContributionID());
+    $this->assertEquals(2, $paymentObject->getInvoiceID());
+    $this->assertEquals(3, $paymentObject->getContributionRecurID());
+  }
+
 }