Deprecate creating partially paid contributions, other than by partially paying a contribution. #15855

eileenmcnaughton · 2019-11-15T04:18:17Z

Overview

Adds deprecation noise when an attempt is made to set a contribution to partially paid other than by adding a payment (using Payment.create). This is necessary not only because we have a preferred flow but because setting to PartiallyPaid doesn't actually create the financial_trxn that is required if done via Contribution.create flow

Before

Calling Contribution.create with status of 'Partially Paid' quietly does a bad job of creating the appropriate financial records

After

Calling Contribution.create with status of 'Partially Paid' noisily does a bad job of creating the appropriate financial records

Technical Details

As #15706 signposts - creating a contribution with a status of 'Partially Paid'
is not actually a valid thing to do. There are no created financial_trxns & the balance is wrong. In some places the code does
this in conjunction with some parameters. These don't really work either. We need to deprecate & eliminate this flow.

I'm expecting some tests to fail & need fixing before this passes

Comments

civibot · 2019-11-15T04:18:19Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

We want to deprecate this as it doesn't work properly. In this case there is no need to test it as it's not really valid & hopefully soon it would trigger a deprecation notice Test that will work when we can deprecate civicrm#15855

…lly paying a contribution. As civicrm#15706 signposts - creating a contribution with a status of 'Partially Paid' is not actually a valid thing to do. There are no created financial_trxns & the balance is wrong. In some places the code does this in conjunction with some parameters. These don't really work either. We need to deprecate & eliminate this flow. I'm expecting some tests to fail & need fixing before this passes

eileenmcnaughton · 2019-11-22T01:48:43Z

@seamuslee001 looks like this is mergeable now

JoeMurray · 2019-11-29T23:22:40Z

CRM/Contribute/BAO/Contribution.php

@@ -141,14 +140,19 @@ public static function add(&$params, $ids = []) {
        $params['creditnote_id'] = self::createCreditNoteId();
      }
    }
-    if (empty($params['contribution_status_id'])) {
+    $contributionStatusID = $params['contribution_status_id'] ?? NULL;


Forgive my ignorance, but I don't see what conditions would lead the '?? NULL' to change the value of $contributionStatusID. Whatever is in $params['contribution_status_id'] will be assigned, whether it is NULL or not, regardless of the Null coalescing operator, no?

it gets populated down on line 155

So it will be set to NULL if for some reason $params['contribution_status_id'] is not set and seems sensible here

JoeMurray · 2019-11-29T23:26:05Z

CRM/Contribute/BAO/Contribution.php

-    if (empty($params['contribution_status_id'])) {
+    $contributionStatusID = $params['contribution_status_id'] ?? NULL;
+    if (CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', (int) $contributionStatusID) === 'Partially paid' && empty($params['is_post_payment_create'])) {
+      CRM_Core_Error::deprecatedFunctionWarning('Setting status to partially paid other than by using Payment.create is deprecated and unreliable');


If a contribution with completed status is edited so that the total amount increases, then the status is supposed to change to partially paid. So in that use case I think this error message is inappropriate.

@JoeMurray we have quite a lot of testing on editing values and this line is never hit or the test would fail.

@JoeMurray this seems safe to me as the changes that happen here https://github.com/civicrm/civicrm-core/pull/16218/files#diff-0474c991325eed0cec87e8240916ef79R507 which cause the total amount of a contribution linked to an event do not trigger this

JoeMurray · 2019-11-29T23:45:36Z

CRM/Contribute/BAO/Contribution.php

@@ -183,9 +187,10 @@ public static function add(&$params, $ids = []) {
    if ($contributionID && $setPrevContribution) {
      $params['prevContribution'] = self::getOriginalContribution($contributionID);
    }
+    $previousContributionStatus = ($contributionID && !empty($params['prevContribution'])) ? CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', (int) $params['prevContribution']->contribution_status_id) : NULL;


I don't really follow if there is a functional change here or not. From a code readability perspective, I like the creation of the $previousContributionStatus variable even if it is only used once. But it's a bit more complicated how you are doing it. Is the reason for only checking the previous contribution status if there is a contribution and a previous contribution mainly to improve performance? Or is there a possible error that's being avoided? Sorry, I'm trying to help with a review but I'm struggling a bit.

It's primarily about making the IF lower down readable in this case.

seamuslee001 · 2020-01-06T23:53:03Z

@eileenmcnaughton @JoeMurray I have reviewed this and concluded it is a sensible move. In regards to your concern Joe, I added in some more assertions into the changed fee selection tests #16218 which demonstrates when an event fee selection is changed such that the total amount increases then the contribution is correctly set to Partially Paid

seamuslee001 · 2020-01-06T23:55:29Z

CRM/Contribute/BAO/Contribution.php

+    if (CRM_Core_PseudoConstant::getName('CRM_Contribute_BAO_Contribution', 'contribution_status_id', (int) $contributionStatusID) === 'Partially paid' && empty($params['is_post_payment_create'])) {
+      CRM_Core_Error::deprecatedFunctionWarning('Setting status to partially paid other than by using Payment.create is deprecated and unreliable');
+    }
+    if (!$contributionStatusID) {


@JoeMurray So if $params['contribution_status_id'] was set to NULL contribution_status_id key was not included then we should get the contribution status Id here

seamuslee001 · 2020-01-06T23:56:48Z

Jenkins re test this please

seamuslee001 · 2020-01-06T23:58:40Z

Adding merge on pass as it reads fine to me and we have reasonable test coverage in this area

civibot bot added the master label Nov 15, 2019

eileenmcnaughton force-pushed the test_dep branch 2 times, most recently from 859618c to 7e4aefe Compare November 15, 2019 20:54

This was referenced Nov 16, 2019

[NFC] [Test-only] Remove partially paid from statuses tested for RenewContribution #15866

Merged

[NFC] fix test to correctly set up partially paid contribution #15867

Merged

eileenmcnaughton force-pushed the test_dep branch 2 times, most recently from 796e805 to 076dcc3 Compare November 21, 2019 07:59

eileenmcnaughton force-pushed the test_dep branch from 076dcc3 to b048ed1 Compare November 21, 2019 09:49

JoeMurray reviewed Nov 29, 2019

View reviewed changes

seamuslee001 reviewed Jan 6, 2020

View reviewed changes

seamuslee001 added the merge on pass label Jan 6, 2020

seamuslee001 merged commit ae0f40d into civicrm:master Jan 7, 2020

seamuslee001 deleted the test_dep branch January 7, 2020 01:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate creating partially paid contributions, other than by partially paying a contribution. #15855

Deprecate creating partially paid contributions, other than by partially paying a contribution. #15855

eileenmcnaughton commented Nov 15, 2019

civibot bot commented Nov 15, 2019

eileenmcnaughton commented Nov 22, 2019

JoeMurray Nov 29, 2019

eileenmcnaughton Nov 30, 2019

seamuslee001 Jan 6, 2020

JoeMurray Nov 29, 2019

eileenmcnaughton Nov 30, 2019

seamuslee001 Jan 6, 2020

JoeMurray Nov 29, 2019

eileenmcnaughton Nov 30, 2019

seamuslee001 commented Jan 6, 2020

seamuslee001 Jan 6, 2020

seamuslee001 commented Jan 6, 2020

seamuslee001 commented Jan 6, 2020

Deprecate creating partially paid contributions, other than by partially paying a contribution. #15855

Deprecate creating partially paid contributions, other than by partially paying a contribution. #15855

Conversation

eileenmcnaughton commented Nov 15, 2019

Overview

Before

After

Technical Details

Comments

civibot bot commented Nov 15, 2019

eileenmcnaughton commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Jan 6, 2020

Choose a reason for hiding this comment

seamuslee001 commented Jan 6, 2020

seamuslee001 commented Jan 6, 2020