Remove first contribution #17994

mattwire · 2020-07-29T13:45:56Z

Overview

This is a step towards simplifying completetransaction/repeattransaction calls. $objects['first_contribution'] is only used in the case of repeattransaction and is only used to pass the contribution ID and currency.

The currency was added via a relatively recent PR and could have just used the existing contribution object - but given the number of different objects/array being passed around it would have been difficult to choose which one to use!

Before

Additional object is passed around containing the "first_contribution" if called via Contribution.repeattransaction API.

After

Necessary parameters (contribution ID) are passed via input parameters. Currency is retrieved from $contribution object.

Technical Details

This is a further step towards simplifying this area of code.

Comments

@eileenmcnaughton You're probably the only one who can make sense of this :-) It sort of comes out of #17455 and should act as another step towards cleaning up the multiple template contribution lookups / calculation of original_contribution_id

civibot · 2020-07-29T13:45:59Z

(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).

eileenmcnaughton · 2020-07-29T22:00:21Z

Once this is merged #17777 there will be one less place calling that completeOrder function.

mattwire · 2020-07-31T17:51:21Z

@eileenmcnaughton This is now passing tests and is ready for review. Would you be able to take a look?

eileenmcnaughton · 2020-07-31T22:14:31Z

CRM/Contribute/BAO/Contribution.php

@@ -4464,8 +4465,8 @@ public static function completeOrder($input, &$ids, $objects, $transaction = NUL
    ));

    // CRM-20678 Ensure that the currency is correct in subseqent transcations.
-    if (empty($contributionParams['currency']) && isset($objects['first_contribution']->currency)) {
-      $contributionParams['currency'] = $objects['first_contribution']->currency;
+    if (empty($contributionParams['currency']) && isset($contribution->currency)) {


We need a test covering this -and it definitely needs moving to within repeattransaction function as it only applies for repeats. It would be good to tackle & resolve this first

$contribution object is $objects['contribution'] which is a copy of $objects['first_contribution'] with 3 parameters removed (id, receive_date and invoice_id). So currency at this point is the same value.
Agree with moving, not sure it needs it's own test because I'm happy from code analysis that it's the same value. Would prefer to move after this PR

I'm pretty sure these lines can go now - we should just add currency to that test - that might be quick to do - looking

OK I confirmed they can go #18055

eileenmcnaughton · 2020-07-31T22:16:27Z

api/v3/Contribution.php

@@ -579,6 +579,8 @@ function _civicrm_api3_contribution_completetransaction_spec(&$params) {
 function civicrm_api3_contribution_repeattransaction($params) {
  $input = $ids = [];
  civicrm_api3_verify_one_mandatory($params, NULL, ['contribution_recur_id', 'original_contribution_id']);
+  // @todo Clean this up - we should pass through either original_contribution_id or contribution_recur_id


Once we get rid of the other place that calls completeOrder none of this makes sense inn the api class & it all looks a lot different once you consolidate it in the other function

Agree with that as the goal - but we have over 100 lines of code to work through before we can get there with various bugs (such as the template contribution issue) to work through. I don't see a way to get there without simplifying some of this stuff first.

eileenmcnaughton · 2020-07-31T22:20:45Z

CRM/Contribute/BAO/Contribution.php

@@ -4417,7 +4417,8 @@ public static function completeOrder($input, &$ids, $objects, $transaction = NUL
      $transaction = new CRM_Core_Transaction();
    }
    $contribution = $objects['contribution'];
-    $primaryContributionID = $contribution->id ?? $objects['first_contribution']->id;
+
+    $primaryContributionID = $contribution->id ?? $input['original_contribution_id'];


This is only actually relevant for the membership update later on

Not quite - it's also used earlier for the isSingleLineItem check. Again we can simplify further later - it's now clearer what that primaryContributionID actually represents.

Right - neither of those uses of it actually make sense the more I think about it. So it makes more sense to work to eliminate the value

So removing this line is still valid

eileenmcnaughton · 2020-07-31T22:30:20Z

Hmm - I guess I can dig again - I might reach the same conclusion if I sink a few hours into it -I will need to think about what tests are needed

eileenmcnaughton · 2020-08-01T06:19:33Z

In actual fact the only usage for $primaryContributionId is

      $membershipParams['num_terms'] = $contribution->getNumTermsByContributionAndMembershipType(
          $membershipParams['membership_type_id'],
          $primaryContributionID
        );

which uses the line items to determine the number of terms

so I guess where we need tests is around that - in the case of repeattransaction we already have the line items loaded so I think we can remove that dependence all together

mattwire · 2020-08-01T10:08:23Z

@eileenmcnaughton This does have quite a bit of test coverage (I hit some errors during development). I struggle to get my head around what some of this code is actually trying to do and there are so many objects being passed around with most of the parameters unused. I identified first_contribution as a "low-hanging fruit" that both allows us to remove a parameter from ipn_process_transaction and make it really clear when original_contribution_id is being passed.

eileenmcnaughton · 2020-08-01T19:47:52Z

@mattwire - yeah the code is nasty for sure.

eileenmcnaughton · 2020-08-01T21:03:01Z

@mattwire here is the PR to remove one of the 2 places primaryContributionID is used - #18032 - I checked &it there was no specific cover for the 2 line items scenario so I added the test for that.

It looks like it won't be too hard to remove the other place. It's pretty clear we should be using our saved contribution rather than the 'primaryContributionID' - however the key use case for that whole subsystem is calculating num_terms so I want to be sure num_terms calculation specifically has test cover.

eileenmcnaughton · 2020-08-04T00:08:06Z

CRM/Core/Payment/BaseIPN.php

@@ -135,12 +135,6 @@ public function validateData($input, &$ids, &$objects, $required = TRUE, $paymen
    if (!$this->loadObjects($input, $ids, $objects, $required, $paymentProcessorID)) {
      return FALSE;
    }
-    //the process is that the loadObjects is kind of hacked by loading the objects for the original contribution and then somewhat inconsistently using them for the
-    //current contribution. Here we ensure that the original contribution is available to the complete transaction function
-    //we don't want to fix this in the payment processor classes because we would have to fix all of them - so better to fix somewhere central


definitely can go once primaryContribution goes

eileenmcnaughton · 2020-08-04T00:08:48Z

CRM/Core/Payment/BaseIPN.php

@@ -483,6 +477,14 @@ private function cancelMembership($membership, $membershipStatusID, $onlyCancelP
   * @throws \CiviCRM_API3_Exception
   */
  public function completeTransaction(&$input, &$ids, &$objects, $transaction = NULL) {
+    // @todo This replaces usage of $objects['first_contribution']->id - is it required here?


Can we just skip this now - since the reason for it predates removing the usage of first_contribtion

eileenmcnaughton · 2020-08-04T00:08:59Z

CRM/Event/Form/Task/Batch.php

@@ -379,6 +379,10 @@ public static function updateContributionStatus($params) {
    if (!$baseIPN->validateData($input, $ids, $objects, FALSE)) {
      throw new CRM_Core_Exception('validation error');
    }
+    // @todo This replaces usage of $objects['first_contribution']->id - is it required here?


Shouldn't be needed now

eileenmcnaughton · 2020-08-04T00:09:58Z

api/v3/Contribution.php

@@ -626,8 +628,9 @@ function civicrm_api3_contribution_repeattransaction($params) {
      'membership_id',
    ];
    $input = array_intersect_key($params, array_fill_keys($passThroughParams, NULL));
+    $input['original_contribution_id'] = $originalContributionID ?? NULL;


Can we leave this change out & just do the removal in this PR ?

eileenmcnaughton · 2020-08-04T00:10:16Z

api/v3/Contribution.php

@@ -646,19 +649,14 @@ function civicrm_api3_contribution_repeattransaction($params) {
 *
 * @param array $ids
 *
- * @param CRM_Contribute_BAO_Contribution $firstContribution


All this part below still makes sense

eileenmcnaughton · 2020-08-04T00:14:36Z

@mattwire this needs a rebase but I took a look at the parts that still seem relevant - basically what it says on the label 'remove first_contribution' - & commented.

One thing I note is that original_contribution really means 'any contribution so that we can identify the recurring id and select the template contribution' -rather than the id of the contribution to use as a template. I think for now we can think about that from a code comments point of view with a back-burner goal to properly support template_contribution_id once we have separated the complete_order & repeattransaction code paths

eileenmcnaughton · 2020-08-04T00:30:34Z

api/v3/Contribution.php

@@ -579,6 +579,8 @@ function _civicrm_api3_contribution_completetransaction_spec(&$params) {
 function civicrm_api3_contribution_repeattransaction($params) {
  $input = $ids = [];
  civicrm_api3_verify_one_mandatory($params, NULL, ['contribution_recur_id', 'original_contribution_id']);
+  // @todo Clean this up - we should pass through either original_contribution_id or contribution_recur_id
+  //   With a contribution_recur_id the template contribution will be calculated automatically.


In fact we actually calculate the template contribution based on getTemplateContribution either way ....

eileenmcnaughton · 2020-08-04T21:51:39Z

This is no longer ready for review - so I've removed the tag. I'm not a fan of the tag as I find it's rarely true and they should all be ready for review so I personally leave PRs with that tag for other people - so you might not want to re-add it on this PR.

mattwire · 2020-08-14T14:51:54Z

@eileenmcnaughton I've rebased this. Won't have time to go through again until after the security release when I'll probably need to re-test some of the fixes around this area for that version. Feel free to cherry-pick anything useful in the meantime.

eileenmcnaughton · 2020-08-14T21:57:15Z

@mattwire I took a quick look but I think it would be easier to close this & do a new PR that just removes the first_contribution key since a lot of the stuff in here is no longer relevant. I didn't do that clean up earlier because this PR was open

mattwire · 2020-08-14T23:54:36Z

@eileenmcnaughton Yes, makes sense I think given the cleanup we've now done.

civibot bot added the master label Jul 29, 2020

mattwire force-pushed the remove_first_contribution branch 2 times, most recently from 57eeee9 to cc834dd Compare July 29, 2020 15:23

mattwire force-pushed the remove_first_contribution branch from cc834dd to 0e0dd42 Compare July 30, 2020 09:16

mattwire marked this pull request as ready for review July 30, 2020 09:23

mattwire added the Refactor label Jul 30, 2020

mattwire force-pushed the remove_first_contribution branch 2 times, most recently from fbb5323 to d1bc95b Compare July 30, 2020 13:03

mattwire mentioned this pull request Jul 31, 2020

Get the template contribution instead of the first contribution when using repeattransaction #17455

Closed

mattwire added the ready for review label Jul 31, 2020

eileenmcnaughton added ready for review and removed ready for review labels Jul 31, 2020

eileenmcnaughton reviewed Jul 31, 2020

View reviewed changes

eileenmcnaughton reviewed Aug 4, 2020

View reviewed changes

eileenmcnaughton removed the ready for review label Aug 4, 2020

eileenmcnaughton added sig:code maintenance readability testability needs-to-not-make-tests-fail needs-rebase and removed Refactor needs-to-not-make-tests-fail labels Aug 4, 2020

Remove use of objects['first_contribution'] from repeattransaction

cac26cb

mattwire force-pushed the remove_first_contribution branch from d1bc95b to cac26cb Compare August 14, 2020 14:49

mattwire added needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state and removed needs-rebase labels Aug 14, 2020

mattwire closed this Aug 14, 2020

mattwire mentioned this pull request Aug 19, 2020

Remove first_contribution key from repeattransaction #18197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove first contribution #17994

Remove first contribution #17994

mattwire commented Jul 29, 2020 •

edited

Loading

civibot bot commented Jul 29, 2020

eileenmcnaughton commented Jul 29, 2020

mattwire commented Jul 31, 2020

eileenmcnaughton Jul 31, 2020

mattwire Aug 1, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton Jul 31, 2020

mattwire Aug 1, 2020

eileenmcnaughton Jul 31, 2020

mattwire Aug 1, 2020

eileenmcnaughton Aug 1, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton commented Jul 31, 2020 •

edited

Loading

eileenmcnaughton commented Aug 1, 2020 •

edited

Loading

mattwire commented Aug 1, 2020

eileenmcnaughton commented Aug 1, 2020

eileenmcnaughton commented Aug 1, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton Aug 4, 2020 •

edited

Loading

eileenmcnaughton Aug 4, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton commented Aug 4, 2020

eileenmcnaughton Aug 4, 2020

eileenmcnaughton commented Aug 4, 2020

mattwire commented Aug 14, 2020

eileenmcnaughton commented Aug 14, 2020

mattwire commented Aug 14, 2020

Remove first contribution #17994

Remove first contribution #17994

Conversation

mattwire commented Jul 29, 2020 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Jul 29, 2020

eileenmcnaughton commented Jul 29, 2020

mattwire commented Jul 31, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Jul 31, 2020 • edited Loading

eileenmcnaughton commented Aug 1, 2020 • edited Loading

mattwire commented Aug 1, 2020

eileenmcnaughton commented Aug 1, 2020

eileenmcnaughton commented Aug 1, 2020

Choose a reason for hiding this comment

eileenmcnaughton Aug 4, 2020 • edited Loading

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

eileenmcnaughton commented Aug 4, 2020

Choose a reason for hiding this comment

eileenmcnaughton commented Aug 4, 2020

mattwire commented Aug 14, 2020

eileenmcnaughton commented Aug 14, 2020

mattwire commented Aug 14, 2020

mattwire commented Jul 29, 2020 •

edited

Loading

eileenmcnaughton commented Jul 31, 2020 •

edited

Loading

eileenmcnaughton commented Aug 1, 2020 •

edited

Loading

eileenmcnaughton Aug 4, 2020 •

edited

Loading