Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardise more params in completeOrder #18952

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Standardise more params in completeOrder

Before

Handling to load the following from the previous contribution is specific to Authorize.net for

  • amount_level
  • address_id
  • campaign_id

After

Handling moved to completeOrder. In the case of campaign_id it is only used if not set in input params or contribution recur record

Technical Details

I also removed $objects from

 $isFirstOrLastRecurringPayment = $this->recur($input, [
        'related_contact' => $ids['related_contact'] ?? NULL,
        'participant' => !empty($objects['participant']) ? $objects['participant']->id : NULL,
        'participant' => NULL,
        'contributionRecur' => $contributionRecur->id,
      ], $contributionRecur, $objects['contribution'], $first);
      ], $contributionRecur, $contribution, $first);

In the case of participant we actually don't do A.net IPNs for events as they are only processed on recurring for A.net and we don't have expectations for participants here - this is just copy & paste. In the case of $objects['contribution'], this was only set to $contribution a few lines earlier

$objects['contribution'] = &$contribution;

Comments

@civibot
Copy link

civibot bot commented Nov 10, 2020

(Standard links)

// CRM-17718 the campaign id on the contribution recur record should get precedence.
$contributionParams['campaign_id'] = $recurringContribution['campaign_id'];
}
if (!isset($contributionParams['campaign_id']) && isset($templateContribution['campaign_id'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eileenmcnaughton this seems to be new in this PR? So we are saying that if the Recur object has a campaign we use that otherwise if there is a campaign in the template use that. I think that makes sense but might be worthwhile code documenting that this is a deliberate change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seamuslee001 so the recur part is just moved - I guess it didn't have to be but it seemed clearer. The fall back to templateContribution was not done except for A.net before... unless they passed in campaign_id as an input param

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that the recur part is just moved its just this template override part that felt new

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK - I've added a comment about falling back on the template value

This moves the loading of the following params from the first contribution from Authorize.net to
completeOrder
- amount_level
- address_id

In addition campaign_id is moved, however, this is only in it is not an
input param or derived from the recur record
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants