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

Code cleanup on greeting processing #24129

Merged
merged 1 commit into from
Aug 18, 2022
+35 −54
Merged

Code cleanup on greeting processing #24129

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
84 changes: 35 additions & 49 deletions CRM/Contact/BAO/Contact.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
+--------------------------------------------------------------------+
*/

use Civi\Token\TokenProcessor;

/**
*
* @package CRM
Expand Down Expand Up @@ -2725,64 +2727,48 @@ public static function updateGreetingsOnTokenFieldChange($updatedFields, $contac
* @param \CRM_Contact_DAO_Contact $contact
* Contact object after save.
*/
public static function processGreetings(&$contact) {

$emailGreetingString = self::getTemplateForGreeting('email_greeting', $contact);
$postalGreetingString = self::getTemplateForGreeting('postal_greeting', $contact);
$addresseeString = self::getTemplateForGreeting('addressee', $contact);
//@todo this function does a lot of unnecessary loading.
// ensureGreetingParamsAreSet now makes sure that the contact is
// loaded and using updateGreetingsOnTokenFieldChange
// allows us the possibility of only doing an update if required.

// The contact object has not always required the
// fields that are required to calculate greetings
// so we need to retrieve it again.
public static function processGreetings(CRM_Contact_DAO_Contact $contact): void {

$greetings = array_filter([
'email_greeting_display' => self::getTemplateForGreeting('email_greeting', $contact),
'postal_greeting_display' => self::getTemplateForGreeting('postal_greeting', $contact),
'addressee_display' => self::getTemplateForGreeting('addressee', $contact),
]);
// A DAO fetch here is more efficient than looking up
// values in the token processor - this may be substantially improved by
// https://github.com/civicrm/civicrm-core/pull/24294 and
// https://github.com/civicrm/civicrm-core/pull/24156 and could be re-tested
// in future but tests also 'expect' it to be populated.
if ($contact->_query !== FALSE) {
$contact->find(TRUE);
}

// store object values to an array
$contactDetails = [];
CRM_Core_DAO::storeValues($contact, $contactDetails);
$contactDetails = [[$contact->id => $contactDetails]];

$updateQueryString = [];

if ($emailGreetingString) {
CRM_Contact_BAO_Contact_Utils::processGreetingTemplate($emailGreetingString,
$contactDetails,
$contact->id,
'CRM_Contact_BAO_Contact'
);
$emailGreetingString = CRM_Core_DAO::escapeString(CRM_Utils_String::stripSpaces($emailGreetingString));
$updateQueryString[] = " email_greeting_display = '$emailGreetingString'";
}

if ($postalGreetingString) {
CRM_Contact_BAO_Contact_Utils::processGreetingTemplate($postalGreetingString,
$contactDetails,
$contact->id,
'CRM_Contact_BAO_Contact'
);
$postalGreetingString = CRM_Core_DAO::escapeString(CRM_Utils_String::stripSpaces($postalGreetingString));
$updateQueryString[] = " postal_greeting_display = '$postalGreetingString'";
// We can't use the DAO store method as it filters out NULL keys.
// Leaving NULL keys in is important as the token processor will
// do DB lookups to find the data if the keys are not set.
// We could just about skip this & just cast to an array - except create
// adds in `phone` and `email`
// in a weird & likely obsolete way....
$contactArray = array_intersect_key((array) $contact, $contact->fields());
$tokenProcessor = new TokenProcessor(\Civi::dispatcher(), [
'smarty' => TRUE,
'class' => __CLASS__,
'schema' => ['contactId'],
]);
$tokenProcessor->addRow(['contactId' => $contact->id, 'contact' => (array) $contactArray]);
foreach ($greetings as $greetingKey => $greetingString) {
$tokenProcessor->addMessage($greetingKey, $greetingString, 'text/plain');
}

if ($addresseeString) {
CRM_Contact_BAO_Contact_Utils::processGreetingTemplate($addresseeString,
$contactDetails,
$contact->id,
'CRM_Contact_BAO_Contact'
);
$addresseeString = CRM_Core_DAO::escapeString(CRM_Utils_String::stripSpaces($addresseeString));
$updateQueryString[] = " addressee_display = '$addresseeString'";
$tokenProcessor->evaluate();
$row = $tokenProcessor->getRow(0);
foreach ($greetings as $greetingKey => $greetingString) {
$parsedGreeting = CRM_Core_DAO::escapeString(CRM_Utils_String::stripSpaces($row->render($greetingKey)));
$updateQueryString[] = " $greetingKey = '$parsedGreeting'";
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton the one thing I wonder bout here is should we do a test for checking if $parsedGreeting != current passed greeting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm yeah - could do - I think it is no change at the moment - ie the check is not there already - so I'd probably look at it as a follow up

}

if (!empty($updateQueryString)) {
$updateQueryString = implode(',', $updateQueryString);
$queryString = "UPDATE civicrm_contact SET $updateQueryString WHERE id = {$contact->id}";
CRM_Core_DAO::executeQuery($queryString);
CRM_Core_DAO::executeQuery("UPDATE civicrm_contact SET $updateQueryString WHERE id = {$contact->id}");
}
}

Expand Down
5 changes: 0 additions & 5 deletions tests/phpunit/api/v3/ContactTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4843,7 +4843,6 @@ public function testCreateCommunicationStylePassed(): void {
* @param int $version
*
* @dataProvider versionThreeAndFour
* @throws \CRM_Core_Exception
*/
public function testContactGreetingsCreate(int $version): void {
$this->_apiversion = $version;
Expand Down Expand Up @@ -4873,8 +4872,6 @@ public function testContactGreetingsCreate(int $version): void {
* @param int $version
*
* @dataProvider versionThreeAndFour
*
* @throws \CRM_Core_Exception
*/
public function testContactGreetingsCreateWithCustomField(int $version): void {
$this->_apiversion = $version;
Expand Down Expand Up @@ -4917,8 +4914,6 @@ public function testContactGreetingsCreateWithCustomField(int $version): void {
*
* @param int $version
*
* @throws \CRM_Core_Exception
* @throws \CiviCRM_API3_Exception
* @dataProvider versionThreeAndFour
*/
public function testGreetingParseSmarty(int $version): void {
Expand Down