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.

Sign up for GitHub

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

Fix Contribution.create to not attempt to set contacts on activity update #19202

Merged
merged 1 commit into from
Dec 22, 2020
Merged

Fix Contribution.create to not attempt to set contacts on activity update #19202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions CRM/Contribute/BAO/Contribution.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,18 +517,24 @@ public static function create(&$params) {
])->execute()->first();

$campaignParams = isset($params['campaign_id']) ? ['campaign_id' => ($params['campaign_id'] ?? NULL)] : [];
Activity::save(FALSE)->addRecord(array_merge([
$activityParams = array_merge([
'activity_type_id:name' => 'Contribution',
'source_record_id' => $contribution->id,
'source_contact_id' => CRM_Core_Session::getLoggedInContactID() ?: $contribution->contact_id,
'target_contact_id' => CRM_Core_Session::getLoggedInContactID() ? [$contribution->contact_id] : [],
'activity_date_time' => $contribution->receive_date,
'is_test' => (bool) $contribution->is_test,
'status_id:name' => $isCompleted ? 'Completed' : 'Scheduled',
'skipRecentView' => TRUE,
'subject' => CRM_Activity_BAO_Activity::getActivitySubject($contribution),
'id' => $existingActivity['id'] ?? NULL,
], $campaignParams))->execute();
], $campaignParams);
if (!$activityParams['id']) {
// Don't set target contacts on update as these will have been
// correctly created and we risk overwriting them with
// 'best guess' params.
$activityParams['source_contact_id'] = (int) ($params['source_contact_id'] ?? (CRM_Core_Session::getLoggedInContactID() ?: $contribution->contact_id));
$activityParams['target_contact_id'] = ($activityParams['source_contact_id'] === (int) $contribution->contact_id) ? [] : [$contribution->contact_id];
Copy link
Contributor

Choose a reason for hiding this comment

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

Summarizing the logic here: $params['source_contact_id'] seems to be a new thing that means "the on-behalf-of individual", so the first part is "if we came from on-behalf-of, use the individual contributor as the activity source contact, otherwise do what we did before". Then the second part seems to be "if we have just set the activity source to the same thing as the contribution contact, then blank out the existing activity target (because otherwise it will duplicate source), otherwise overwrite with the contribution contact", so it's saying that the contribution contact is the correct one to use for the activity target, regardless of what was on the activity before or whether it's on-behalf-of or other. That might be right, it's just trying to convince myself it's true in the generic case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I misread the part above. So this block is only for new activities. So then my question would be is changing a contribution contact by calling api contribution.create an officially supported operation, and if so, what should happen with the activity's contacts. Before it would update the activity to match, now the activity target is still the old contact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@demeritcowboy I don't think changing an activity via contribution create is supported.

The flow we need to work for repeattransactions is to be able to create a new contribution with the same activity contacts as the template activity. Since the contribution.create is taking responsibility for creating the activity I think it needs to do so with the correct activity contacts

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I don't have anything else then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @demeritcowboy I'll see how @MegaphoneJon goes with this patch over the next few days

}
Activity::save(FALSE)->addRecord($activityParams)->execute();
}

// do not add to recent items for import, CRM-4399
Expand Down
7 changes: 5 additions & 2 deletions tests/phpunit/CRM/Activity/Form/SearchTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ public function tearDown() {
}

/**
* Test submitted the search form.
* Test submitted the search form.
*
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
*/
public function testSearch() {
public function testSearch(): void {

$form = new CRM_Activity_Form_Search();
$_SERVER['REQUEST_METHOD'] = 'GET';
Expand Down
3 changes: 2 additions & 1 deletion tests/phpunit/api/v3/JobTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,8 +462,9 @@ public function testBatchMergeWorks($dataSet) {
* Note the group combinations & expected results:
*
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
*/
public function testBatchMergeWithAssets() {
public function testBatchMergeWithAssets(): void {
$contactID = $this->individualCreate();
$contact2ID = $this->individualCreate();
$this->contributionCreate(['contact_id' => $contactID]);
Expand Down