[REF] Paypal ipn - cleanup references to completion

This extracts a function to check if the contribution is completed.

I also rationalised the validation - it was using a combo of recur and first to
validate but on thinking it through I realised all it was saying was
'if we are finalising a pending contribution the amount must match'

I think that's fine even for recur with a change in amount - that seems
to me to be something that happens down the track but we still expect
the very first one to come in with the value it originally
had - if that is NOT true then we probably should just remove the check

CRM/Core/Payment/PayPalProIPN.php

@@ -363,14 +363,11 @@ public function recur($input, $recur, $contribution, $first) {
   public function single($input, $contribution, $recur = FALSE, $first = FALSE) {
 
     // make sure the invoice is valid and matches what we have in the contribution record
-    if ((!$recur) || ($recur && $first)) {
+    if (!$this->isContributionCompleted()) {
       if ($this->getContributionObject()->invoice_id !== $input['invoice']) {
         throw new CRM_Core_Exception('PayPalProIPN: Invoice values dont match between database and IPN request.');
       }
-    }
-
-    if (!$recur) {
-      if ($this->getContributionObject()->total_amount != $input['amount']) {
+      if (!$this->getContributionRecurID() && $this->getContributionObject()->total_amount != $input['amount']) {
         throw new CRM_Core_Exception('PayPalProIPN: Amount values dont match between database and IPN request.');
       }
     }
@@ -380,8 +377,8 @@ public function single($input, $contribution, $recur = FALSE, $first = FALSE) {
       Contribution::update(FALSE)->setValues([
         'cancel_date' => 'now',
         'contribution_status_id:name' => 'Failed',
-      ])->addWhere('id', '=', $contribution->id)->execute();
-      Civi::log()->debug("Setting contribution status to Failed");
+      ])->addWhere('id', '=', $this->getContributionID())->execute();
+      Civi::log()->debug('Setting contribution status to Failed');
       return;
     }
     if ($status === 'Pending') {
@@ -396,14 +393,12 @@ public function single($input, $contribution, $recur = FALSE, $first = FALSE) {
       Civi::log()->debug("Setting contribution status to Cancelled");
       return;
     }
-    elseif ($status !== 'Completed') {
+    if ($status !== 'Completed') {
       Civi::log()->debug('Returning since contribution status is not handled');
       return;
     }
 
-    // check if contribution is already completed, if so we ignore this ipn
-    $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');
-    if ($contribution->contribution_status_id == $completedStatusId) {
+    if ($this->isContributionCompleted()) {
       Civi::log()->debug('PayPalProIPN: Returning since contribution has already been handled.');
       echo 'Success: Contribution has already been handled<p>';
       return;
@@ -527,11 +522,7 @@ public function main() {
       if ($this->getContributionRecurID()) {
         $contributionRecur = $this->getContributionRecurObject();
         // check if first contribution is completed, else complete first contribution
-        $first = TRUE;
-        $completedStatusId = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');
-        if ($contribution->contribution_status_id == $completedStatusId) {
-          $first = FALSE;
-        }
+        $first = !$this->isContributionCompleted();
         $this->recur($input, $contributionRecur, $contribution, $first);
         return;
       }
@@ -731,4 +722,15 @@ protected function getContactID(): int {
     return $this->getValue('c', TRUE);
   }
 
+  /**
+   * Is the original contribution completed.
+   *
+   * @return bool
+   * @throws \CRM_Core_Exception
+   */
+  private function isContributionCompleted(): bool {
+    $status = CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', $this->getContributionObject()->contribution_status_id);
+    return $status === 'Completed';
+  }
+
 }

tests/phpunit/CRM/Core/Payment/PayPalProIPNTest.php

@@ -124,14 +124,18 @@ public function testIPNPaymentMembershipRecurSuccess() {
   }
 
   /**
-   * CRM-13743 test IPN edge case where the first transaction fails and the second succeeds.
+   * CRM-13743 test IPN edge case where the first transaction fails and the
+   * second succeeds.
    *
-   * We are checking that the created contribution has the same date as IPN says it should
-   * Note that only one contribution will be created (no evidence of the failed contribution is left)
-   * It seems likely that may change in future & this test will start failing (I point this out in the hope it
-   * will help future debuggers)
+   * We are checking that the created contribution has the same date as IPN
+   * says it should Note that only one contribution will be created (no
+   * evidence of the failed contribution is left) It seems likely that may
+   * change in future & this test will start failing (I point this out in the
+   * hope it will help future debuggers)
+   *
+   * @throws \CRM_Core_Exception
    */
-  public function testIPNPaymentCRM13743() {
+  public function testIPNPaymentCRM13743(): void {
     $this->setupRecurringPaymentProcessorTransaction();
     $firstPaymentParams = $this->getPaypalProRecurTransaction();
     $firstPaymentParams['txn_type'] = 'recurring_payment_failed';