From f59c3d8555d490697dd6200bc3303c953a9eea88 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikmedia.org>
Date: Mon, 20 May 2019 14:21:41 +1200
Subject: [PATCH] Cleanup input & output on paymentActivityCreate & improve
 test

This also switches over to the api, per todo, but without the potential cleanup that
enables as yet.
---
 CRM/Contribute/BAO/Contribution.php           | 44 ++++++++++++-------
 .../Contribute/Form/AdditionalPaymentTest.php |  6 ++-
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index d5f7a0b76e..6511e166ca 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -885,14 +885,24 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
   }
 
   /**
-   * @param $contributionId
-   * @param $participantId
-   * @param array $financialTrxn
+   * Record an activity when a payment is received.
+   *
+   * @todo this is intended to be moved to payment BAO class as a protected function
+   * on that class. Currently being cleaned up. The addActivityForPayment doesn't really
+   * merit it's own function as it makes the code less rather than more readable.
+   *
+   * @param int $contributionId
+   * @param int $participantId
+   * @param string $totalAmount
+   * @param string $currency
+   * @param string $trxnDate
    *
-   * @param $financialTrxn
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
-  protected static function recordPaymentActivity($contributionId, $participantId, $financialTrxn) {
-    $activityType = ($financialTrxn->total_amount < 0) ? 'Refund' : 'Payment';
+  protected static function recordPaymentActivity($contributionId, $participantId, $totalAmount, $currency, $trxnDate) {
+    $activityType = ($totalAmount < 0) ? 'Refund' : 'Payment';
+
     if ($participantId) {
       $inputParams['id'] = $participantId;
       $values = [];
@@ -907,8 +917,8 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution {
       $entityObj->find(TRUE);
       $title = ts('Contribution');
     }
-
-    self::addActivityForPayment($entityObj->contact_id, $financialTrxn, $activityType, $title, $contributionId);
+    // @todo per block above this is not a logical splitting off of functionality.
+    self::addActivityForPayment($entityObj->contact_id, $activityType, $title, $contributionId, $totalAmount, $currency, $trxnDate);
   }
 
   /**
@@ -3935,7 +3945,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
     }
 
     if (!empty($financialTrxn)) {
-      self::recordPaymentActivity($contributionId, $participantId, $financialTrxn);
+      self::recordPaymentActivity($contributionId, $participantId, $financialTrxn->total_amount, $financialTrxn->currency, $financialTrxn->trxn_date);
       return $financialTrxn;
     }
 
@@ -3943,17 +3953,20 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
 
   /**
    * @param int $targetCid
-   * @param $trxnObj
    * @param $activityType
    * @param string $title
    * @param int $contributionId
+   * @param string $totalAmount
+   * @param string $currency
+   * @param string $trxn_date
    *
-   * @throws CRM_Core_Exception
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
    */
-  public static function addActivityForPayment($targetCid, $trxnObj, $activityType, $title, $contributionId) {
-    $paymentAmount = CRM_Utils_Money::format($trxnObj->total_amount, $trxnObj->currency);
+  public static function addActivityForPayment($targetCid, $activityType, $title, $contributionId, $totalAmount, $currency, $trxn_date) {
+    $paymentAmount = CRM_Utils_Money::format($totalAmount, $currency);
     $subject = "{$paymentAmount} - Offline {$activityType} for {$title}";
-    $date = CRM_Utils_Date::isoToMysql($trxnObj->trxn_date);
+    $date = CRM_Utils_Date::isoToMysql($trxn_date);
     // source record id would be the contribution id
     $srcRecId = $contributionId;
 
@@ -3975,8 +3988,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac
       $activityParams['source_contact_id'] = $id;
       $activityParams['target_contact_id'][] = $targetCid;
     }
-    // @todo use api.
-    CRM_Activity_BAO_Activity::create($activityParams);
+    civicrm_api3('Activity', 'create', $activityParams);
   }
 
   /**
diff --git a/tests/phpunit/CRM/Contribute/Form/AdditionalPaymentTest.php b/tests/phpunit/CRM/Contribute/Form/AdditionalPaymentTest.php
index 7143f3b406..efedd30dfd 100644
--- a/tests/phpunit/CRM/Contribute/Form/AdditionalPaymentTest.php
+++ b/tests/phpunit/CRM/Contribute/Form/AdditionalPaymentTest.php
@@ -168,7 +168,7 @@ class CRM_Contribute_Form_AdditionalPaymentTest extends CiviUnitTestCase {
   /**
    * Test the submit function that completes the partially paid Contribution with multiple payments.
    */
-  public function testMultiplePaymentForPartialyPaidContribution() {
+  public function testMultiplePaymentForPartiallyPaidContribution() {
     $this->createContribution('Partially paid');
 
     // pay additional amount
@@ -184,10 +184,14 @@ class CRM_Contribute_Form_AdditionalPaymentTest extends CiviUnitTestCase {
       'activity_type_id' => 'Payment',
       'options' => ['sort' => 'id'],
       'sequential' => 1,
+      'return' => ['target_contact_id', 'assignee_contact_id', 'subject'],
     ])['values'];
     $this->assertEquals(2, count($activities));
     $this->assertEquals('$ 50.00 - Offline Payment for Contribution', $activities[0]['subject']);
     $this->assertEquals('$ 20.00 - Offline Payment for Contribution', $activities[1]['subject']);
+    $this->assertEquals(CRM_Core_Session::singleton()->getLoggedInContactID(), $activities[0]['source_contact_id']);
+    $this->assertEquals([$this->_individualId], $activities[0]['target_contact_id']);
+    $this->assertEquals([], $activities[0]['assignee_contact_id']);
   }
 
   /**
-- 
2.25.1