From ac01e8367f2a7056c3a963ba5375e9ee330a6340 Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Tue, 7 Mar 2023 19:31:14 +1300
Subject: [PATCH] Fix retrieve function, add test for when trxn_id is present

---
 CRM/Core/Payment/PayPalProIPN.php             | 30 +++++++++----------
 .../CRM/Core/Payment/PayPalProIPNTest.php     | 18 ++++++-----
 2 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/CRM/Core/Payment/PayPalProIPN.php b/CRM/Core/Payment/PayPalProIPN.php
index 6f9fdeb59b..5b039d3b41 100644
--- a/CRM/Core/Payment/PayPalProIPN.php
+++ b/CRM/Core/Payment/PayPalProIPN.php
@@ -197,22 +197,20 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN {
    *   Data type.
    *   - String
    *   - Integer
-   * @param string $location
-   *   Deprecated.
    * @param bool $abort
    *   Abort if empty.
    *
    * @throws CRM_Core_Exception
    * @return mixed
    */
-  public function retrieve($name, $type, $location = 'POST', $abort = TRUE) {
+  public function retrieve($name, $type, $abort = TRUE) {
     $value = CRM_Utils_Type::validate(
       CRM_Utils_Array::value($name, $this->_inputParameters),
       $type,
       FALSE
     );
     if ($abort && $value === NULL) {
-      throw new CRM_Core_Exception("Could not find an entry for $name in $location");
+      throw new CRM_Core_Exception("Could not find an entry for $name");
     }
     return $value;
   }
@@ -465,15 +463,15 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN {
   public function getInput(&$input) {
     $billingID = CRM_Core_BAO_LocationType::getBilling();
 
-    $input['txnType'] = self::retrieve('txn_type', 'String', 'POST', FALSE);
-    $input['paymentStatus'] = self::retrieve('payment_status', 'String', 'POST', FALSE);
+    $input['txnType'] = $this->retrieve('txn_type', 'String', FALSE);
+    $input['paymentStatus'] = $this->retrieve('payment_status', 'String', FALSE);
 
-    $input['amount'] = self::retrieve('mc_gross', 'Money', 'POST', FALSE);
-    $input['reasonCode'] = self::retrieve('ReasonCode', 'String', 'POST', FALSE);
+    $input['amount'] = $this->retrieve('mc_gross', 'Money', FALSE);
+    $input['reasonCode'] = $this->retrieve('ReasonCode', 'String', FALSE);
 
     $lookup = [
-      "first_name" => 'first_name',
-      "last_name" => 'last_name',
+      'first_name' => 'first_name',
+      'last_name' => 'last_name',
       "street_address-{$billingID}" => 'address_street',
       "city-{$billingID}" => 'address_city',
       "state-{$billingID}" => 'address_state',
@@ -481,15 +479,15 @@ class CRM_Core_Payment_PayPalProIPN extends CRM_Core_Payment_BaseIPN {
       "country-{$billingID}" => 'address_country_code',
     ];
     foreach ($lookup as $name => $paypalName) {
-      $value = self::retrieve($paypalName, 'String', 'POST', FALSE);
+      $value = $this->retrieve($paypalName, 'String', FALSE);
       $input[$name] = $value ? $value : NULL;
     }
 
-    $input['is_test'] = self::retrieve('test_ipn', 'Integer', 'POST', FALSE);
-    $input['fee_amount'] = self::retrieve('mc_fee', 'Money', 'POST', FALSE);
-    $input['net_amount'] = self::retrieve('settle_amount', 'Money', 'POST', FALSE);
-    $input['trxn_id'] = self::retrieve('txn_id', 'String', 'POST', FALSE);
-    $input['payment_date'] = $input['receive_date'] = self::retrieve('payment_date', 'String', 'POST', FALSE);
+    $input['is_test'] = $this->retrieve('test_ipn', 'Integer', FALSE);
+    $input['fee_amount'] = $this->retrieve('mc_fee', 'Money', FALSE);
+    $input['net_amount'] = $this->retrieve('settle_amount', 'Money', FALSE);
+    $input['trxn_id'] = $this->retrieve('txn_id', 'String', FALSE);
+    $input['payment_date'] = $input['receive_date'] = $this->retrieve('payment_date', 'String', FALSE);
     $input['total_amount'] = $input['amount'];
   }
 
diff --git a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php
index af7ee37ced..cff0f9ee04 100644
--- a/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php
+++ b/tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php
@@ -23,12 +23,6 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase {
   protected $_contributionRecurID;
   protected $_contributionPageID;
   protected $_paymentProcessorID;
-  /**
-   * IDs of entities created to support the tests.
-   *
-   * @var array
-   */
-  protected $ids = [];
 
   /**
    * Set up function.
@@ -77,6 +71,16 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase {
     $this->assertEquals('8XA571746W2698126', $contribution['trxn_id']);
     // source gets set by processor
     $this->assertEquals('Online Contribution:', substr($contribution['source'], 0, 20));
+
+    // Re-try the IPN and confirm that a second contribution is not
+    // created (this relies on the trxn_id being the same).
+    $paypalIPN = new CRM_Core_Payment_PayPalProIPN($this->getPaypalProRecurTransaction());
+    $paypalIPN->main();
+    $contributions = Contribution::get()->addWhere('contribution_recur_id', '=', $this->_contributionRecurID)
+      ->addSelect('contribution_status_id:name', 'trxn_id', 'source')
+      ->execute();
+    $this->assertCount(1, $contributions);
+    // source gets set by processor
     $contributionRecur = $this->callAPISuccess('contribution_recur', 'getsingle', ['id' => $this->_contributionRecurID]);
     $this->assertEquals(5, $contributionRecur['contribution_status_id']);
     $paypalIPN = new CRM_Core_Payment_PayPalProIPN($this->getPaypalProRecurSubsequentTransaction());
@@ -182,7 +186,7 @@ class CRM_Core_Payment_PayPalProIPNTest extends CiviUnitTestCase {
    *
    * So, the point of this test is simply to ensure it fails in a known way
    */
-  public function testIPNPaymentExpressNoError() {
+  public function testIPNPaymentExpressNoError(): void {
     $this->setupRecurringPaymentProcessorTransaction();
     $paypalIPN = new CRM_Core_Payment_PayPalProIPN($this->getPaypalExpressTransactionIPN());
     $paypalIPN->main();
-- 
2.25.1