CRM-20577 - When creating an activity per-contact when sending letters, store the version with rendered tokens #10348
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -335,9 +335,6 @@ public static function combineContributions($existing, $contribution, $separator | |||
| * @param int $groupByID | |||
| */ | |||
| public static function assignCombinedContributionValues($contact, $contributions, $groupBy, $groupByID) { | |||
| if (!defined('CIVICRM_MAIL_SMARTY') || !CIVICRM_MAIL_SMARTY) { | |||
| return; | |||
| } | |||
| CRM_Core_Smarty::singleton()->assign('contact_aggregate', $contact['contact_aggregate']); | |||
There was a problem hiding this comment.
This early return actually adds no value. No performance benefit and it makes it harder for extensions & unit tests to interact
|
Jenkins re test this please |
eileenmcnaughton
changed the title
| // This seems silly, but the old behavior was to first check `_cid` | ||
| // and then use the provided `$contactIds`. Probably not even necessary, | ||
| // but difficult to audit. | ||
| $contactIds = $form->_cid ? array($form->_cid) : $form->_contactIds; |
There was a problem hiding this comment.
I've moved this weirdness out of the shared function to each form so we can work to remove it - impossible to ever get rid of it, per comments, if hidden in a shared function
eileenmcnaughton
force-pushed
the
token
branch
5 times, most recently
from
b177614
to
d590739
Compare
In addition to the unit test there is a minor change to not do the early return before assigning variables if smarty is not set. This doesn't really have any performance advantage, as they are already calculated, blocks the unit test and also blocks using these variables from outside extensions
(At least, this covers contributions at this stage). I have also tried to move towards clearer inputs & outpus on createActivities, with the weirdness around contactIds being moved up to the forms themselves and not being passed in. Would prefer not to see form passed in either but, next time. Unit tests cover emails & activity create (for the first time)
|
Tested, working fine. The changes made in |
|
Thanks @monishdeb |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In addition to the unit test there is a minor change to not do the early return before assigning variables
if smarty is not set. This doesn't really have any performance advantage, as they are already calculated,
blocks the unit test and also blocks using these variables from outside extensions