dev/core#2450 - Update source/target contacts on contribution updates #19820
Conversation
|
Can one of the admins verify this patch? |
|
(Standard links)
|
|
@civicrm-builder add to whitelist. |
There was a problem hiding this comment.
I just reviewed the code, but haven't done an r-run.
I'll first say that this is incredible quality for someone who's never submitted a PR to Civi core before - or frankly, for someone who has. Very readable, uses modern Civi conventions, and a well-written test.
I spent some time worrying about the comment directly above this patch that says, "Don't update the activity target values when the contribution is updated" but I can't see a scenario where this patch doesn't create the correct values.
Part of me wonders if this update should happen in the movecontrib extension - but tbh I think that functionality belongs in core anyway.
CRM/Contribute/BAO/Contribution.php
Outdated
| } | ||
| else { | ||
| // If a target contact exists, update it | ||
| $activityParams['target_contact_id'] = $contribution->contact_id; |
There was a problem hiding this comment.
On line 541 above, $activityParams['target_contact_id'] accepts an array; here, it's passed a scalar. Are both correct, or should this be an array?
There was a problem hiding this comment.
Thanks for your feedback!
Yes, I think this should be an array.
CRM/Contribute/BAO/Contribution.php
Outdated
| @@ -539,6 +540,22 @@ public static function create(&$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]; | |||
| } | |||
| else { | |||
| // Check if target contact exists | |||
There was a problem hiding this comment.
The comments on lines 537-539 should be removed if we're now setting target contacts on update.
|
Many thanks @mflandorfer, my colleague is setting up to do a manual test now. The manual test plus my code review should be enough to get this approved for merge. |
|
I just finished a manual test. Everything worked great. And, I didn't find anything odd or broken as a result of applying this patch. |
| ['contact_id' => $contactId_1] | ||
| ))['values'][0]; | ||
|
|
||
| $activity = $this->callAPISuccess('Activity', 'get', [ |
There was a problem hiding this comment.
You don't need to change this but one small shortcut is
$activity = $this->callAPISuccessGetSingle('Activity', ['source_record_id' => $contribution['id']])
of course you have to set a limit if there could be legimately more than one...
CRM/Contribute/BAO/Contribution.php
Outdated
| $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]; | ||
| } | ||
| else { | ||
| // Check if target contact exists | ||
| $targetContactQuery = ActivityContact::get(FALSE)->setLimit(1)->setWhere([ |
There was a problem hiding this comment.
So this is obviously a high-volume function & we need to be mindful of queries. In particular unnecessary updates can be locking queries - so I would do a bit more work here to check whether we are actually making a change of the contribution->contact is already the target - probably by opening up ['record_type_id:name', '=', 'Activity Targets'], to
['record_type_id:name', 'IN', ['Activity Targets', 'Activity Sources']],
and doing extra checks in the php layer. If it starts to get a bit long then extract it
|
I agree with @MegaphoneJon - this looks really good. I think it's worth doing a bit more work to avoid doing updates if there is no change though as this is a high-volume function |
mflandorfer
force-pushed
the
GP-15884-activity-contact-not-updated
branch
from
c81b1fd
to
493a4d6
Compare
Test Result (3 failures / +3)CRM_Contribute_BAO_ContributionTest.testUpdateActivityContactOnContributionContactChangeCRM_Contribute_Form_Contribution_ConfirmTest.testPayNowPaymentapi_v3_SyntaxConformanceTest.testCreateSingleValueAlter with data set #82 |
|
This looks good - are you able to squash the commits before we merge? |
mflandorfer
force-pushed
the
GP-15884-activity-contact-not-updated
branch
from
fe82d06
to
44679f8
Compare
|
@mflandorfer I just checked in on this again & it is now in conflict :-( Can you ping me once it's fixed so I hopefully don't miss it again |
|
@eileenmcnaughton I just resolved the merge conflict |
|
@mflandorfer jenkins is still not loving it - can you do a git pull --rebase with a force push in the hope it cheers up? |
dev/core#2450 - Incorporate requested changes dev/core#2450 - Make activity target contact parameter an array dev/core#2450 - Delete now obsolete comment about target contact updates dev/core#2450 - Compare activity contact IDs on contribution update to avoid unnecessary updates dev/core#2450 - Simplify API calls in unit test dev/core#2450 - Remove limit from activity contact query
mflandorfer
force-pushed
the
GP-15884-activity-contact-not-updated
branch
from
5cc1674
to
6b5e660
Compare
|
@eileenmcnaughton OK, looks good now |
|
@mflandorfer yay - let's lock it in! |
Overview
https://lab.civicrm.org/dev/core/-/issues/2450
Update source/target activity contacts whenever a contribution is updated.
Before
Since PR #19202 CiviCRM would skip updates to source/target activity contact IDs when a contribution is updated, to prevent overwriting them with less accurate contact IDs. However, if a contribution is moved to another contact, the associated activity would still be stuck with the old contact.
After
When a contribution is updated and ...
contact_id. The source contact remains untouched.contact_id. This would be the case, if the contribution was originally created via the API.Technical Details
Comments