From 4d34aefa5eca73d84813734c18eb6f2d9d7e2a70 Mon Sep 17 00:00:00 2001
From: eileen <eileen@fuzion.co.nz>
Date: Tue, 30 Jul 2013 21:29:35 +1200
Subject: [PATCH] CRM-12108 add code comments which explain why I couldn't make
 the test succeed without the try catch block - see CRM-12972 for user reports

---
 CRM/Contribute/BAO/Contribution.php |  3 +++
 CRM/Core/Payment/BaseIPN.php        | 15 +++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 6ef2d808ab..17187c89b6 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -2491,16 +2491,19 @@ WHERE  contribution_id = %1 ";
       $entityId = $params['contribution']->id;
       $entityTable = 'civicrm_contribution';
     }
+
     $entityID[] = $entityId;
     if (!empty($additionalParticipantId)) {
       $entityID += $additionalParticipantId;
     }
+    // prevContribution appears to mean - original contribution object- ie copy of contribution from before the update started that is being updated
     if (!CRM_Utils_Array::value('prevContribution', $params)) {
       $entityID = NULL;
     }
     else {
       $update = TRUE;
     }
+
     // build line item array if its not set in $params
     if (!CRM_Utils_Array::value('line_item', $params) || $additionalParticipantId) {
       CRM_Price_BAO_LineItem::getLineItemArray($params, $entityID, str_replace('civicrm_', '', $entityTable));
diff --git a/CRM/Core/Payment/BaseIPN.php b/CRM/Core/Payment/BaseIPN.php
index 0457bf7bf6..b7eb02b09a 100644
--- a/CRM/Core/Payment/BaseIPN.php
+++ b/CRM/Core/Payment/BaseIPN.php
@@ -533,7 +533,10 @@ LIMIT 1;";
         $paymentProcessorId = $objects['paymentProcessor']->id;
       }
     }
-
+    //it's hard to see how it could reach this point without a contributon id as it is saved in line 511 above
+    // which raised the question as to whether this check preceded line 511 & if so whether something could be broken
+    // From a lot of code reading /debugging I'm still not sure the intent WRT first & subsequent payments in this code
+    // it would be good if someone added some comments or refactored this
     if ($contribution->id) {
       $contributionStatuses = CRM_Contribute_PseudoConstant::contributionStatus(NULL, 'name');
       if (isset($input['prevContribution']) && !$input['prevContribution']->is_pay_later &&
@@ -550,7 +553,15 @@ LIMIT 1;";
         $input['participant_id'] = $contribution->_relatedObjects['participant']->id;
         $input['skipLineItem'] = 1;
       }
-
+      //@todo writing a unit test I was unable to create a scenario where this line did not fatal on second
+      // and subsequent payments. In this case the line items are created at $this->addrecurLineItems
+      // and since the contribution is saved prior to this line there is always a contribution-id,
+      // however there is never a prevContribution (which appears to mean original contribution not previous
+      // contribution - or preUpdateContributionObject most accurately)
+      // so, this is always called & only appears to succeed when prevContribution exists - which appears
+      // to mean "are we updating an exisitng pending contribution"
+      //I was able to make the unit test complete as fataling here doesn't prevent
+      // the contribution being created - but activities would not be created or emails sent
       CRM_Contribute_BAO_Contribution::recordFinancialAccounts($input, NULL);
     }
 
-- 
2.25.1