[REF] Follow up clean up - remove contribution_mode

[eileenmcnaughton]

Overview

Removes legacy parameter

Before

'contribution_mode' set to reflect line item 'type' but no longer used for membership

After

poof

Technical Details

Contribution mode was only set to be used to set isRelatedID
here https://github.com/civicrm/civicrm-core/pull/20653/files#diff-4c9d0b1abe07057a4eea2b47bc627eecb95face8ed8d86c1c005312a52cca811L3420

but we removed that....

Comments

@monishdeb can you check this?

[civibot]

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

[christianwach]

@eileenmcnaughton Woah, cool! I was just about to open an issue about this. Glad to see it's being stripped out.

Am I right in thinking that CRM_Contribute_BAO_Contribution::recordFinancialAccounts() only operated in "single contribution_mode mode" leading to incorrect Financial Types being assigned when Payment.create was called on Orders where there are Line Items of "mixed types", e.g. 1 Donation + 1 Participant, 1 Membership + 1 Participant etc. Here's the result for 1 Donation + 1 Participant... the initial 1200 (Debit) Financial Types are fine, but after Payment.create the Event Fee becomes a Donation:

Additionally (just FYI) in the 1 Membership + 1 Participant scenario, the Membership "mode" took priority and so the Participant record was never updated to "Registered".

[christianwach]

@eileenmcnaughton Follow-up to the above FYI:

The behaviour without removing contribution_mode depended on the order of the Line Items in the Contribution. The screenshot above was for 1 Participant + 1 Donation in that order. The result for 1 Donation + 1 Participant in that order is shown in this screenshot:

Hope that helps disentangle this.

[eileenmcnaughton]

@christianwach so my understanding of the reason for 'mode' was that line items were not being passed correctly to recordFinancialAccounts originally so it was using an assumption-based-approach to determine what they should be

Can I summarise your comments by saying 'this PR improves things but there is still a bug afterwards?

[eileenmcnaughton]

I should note - I haven't figured out what is going on with mode when used in the participant context yet - I had concluded it was not a good thing in the membership context & was going to dig a bit more & see if it does anything useful once this part was sorted

[christianwach]

Can I summarise your comments by saying 'this PR improves things but there is still a bug afterwards?

@eileenmcnaughton I'm not sure TBH... I haven't been able to test with the removal of contribution_mode because it seems to be done in a constellation of PRs, not all of which I can identify. My testing has been with 5.39.beta1 which solved the tax recalculation. I'm just leaving my diagnoses of remaining issues that seem to be due, at least in part, to contribution_mode only handling one situation (e.g. participant) when there may be many (e.g. participant, membership and contribution) present in a Payment.create for a given Order.create.

I'll start testing with 5.40.x as PRs get merged.

[eileenmcnaughton]

I'm trying to simplify the Order api usage a bit #20681 so I can fix a few more tests to use it - then I can start throwing in a few more deprecation notices in places like this https://github.com/civicrm/civicrm-core/pull/20678/files#diff-a16d4d7449cf5f3a0616d1d282a32f27ab6d3f7d2726d076c02ad1d4d655af41R395
& in the places that seem dodgey in recordFinancialAccounts

[monishdeb]

@eileenmcnaughton why don't we also get rid of this assignment

- $params['contribution_mode'] = 'participant';

And later in Contribution::create

-  if (CRM_Utils_Array::value('contribution_mode', $params) == 'participant') {
+ if (!empty($params['participant_id'])) {
      $entityId = $params['participant_id'];
      $entityTable = 'civicrm_participant';
      $additionalParticipantId = CRM_Event_BAO_Participant::getAdditionalParticipantIds($entityId);
    }
    elseif (!empty($params['membership_id'])) {
      //so far $params['membership_id'] should only be set coming in from membershipBAO::create so the situation where multiple memberships
      // are created off one contribution should be handled elsewhere
      $entityId = $params['membership_id'];
      $entityTable = 'civicrm_membership';
    }
    else {
      $entityId = $params['contribution']->id;
      $entityTable = 'civicrm_contribution';
    }

[eileenmcnaughton]

@monishdeb I'm happy to - I didn't remove it because I haven't worked through anything on the participant side yet & am only focussing on membership so far - I was going to switch my focus once the current PRs are merged

[eileenmcnaughton]

Acutally that is not quite true - I had started on the participant side over here #20650 & I wanted to get that additional test cover before going further on it

[monishdeb]

Tested earlier in local, till then no changes have been made. Reviewed the patch .. all looks good. Merging now