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

Ensure contacts without a name are updated when primary email changes #20403

Merged
merged 1 commit into from
May 27, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 42 additions & 0 deletions CRM/Contact/BAO/Contact.php
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,48 @@ public static function &create(&$params, $fixAddress = TRUE, $invokeHooks = TRUE
return $contact;
}

/**
* Check if a contact has a name.
*
* - Individuals need a first_name or last_name
* - Organizations need organization_name
* - Households need household_name
*
* @param array $contact
* @return bool
*/
public static function hasName(array $contact): bool {
$nameFields = [
'Individual' => ['first_name', 'last_name'],
'Organization' => ['organization_name'],
'Household' => ['household_name'],
];
// Casting to int filters out the string 'null'
$cid = (int) ($contact['id'] ?? NULL);
$contactType = $contact['contact_type'] ?? NULL;
if (!$contactType && $cid) {
$contactType = CRM_Core_DAO::getFieldValue(__CLASS__, $cid, 'contact_type');
Copy link
Contributor

Choose a reason for hiding this comment

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

So we can get this all down to one extra query if we retrieve the potential name fields at this point too. It's a bit messier php-wise but I'm very conscious that this is a high traffic function

Copy link
Member Author

Choose a reason for hiding this comment

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

Except that CRM_Core_DAO::getFieldValue uses caching so if we don't use it we don't get the potential benefit of the cache.
I guess I could write a new getFieldValues() (plural) function which re-uses the same cache...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I don't think we use getFieldValue enough to actually have it cached much? I feel like we have been mostly using the api (increasingly) to retrieve stuff like that. However, I guess it's arguable enough now that I won't hold this up on it

}
if (!$contactType || !isset($nameFields[$contactType])) {
throw new CRM_Core_Exception('No contact_type given to ' . __CLASS__ . '::' . __FUNCTION__);
}
foreach ($nameFields[$contactType] as $field) {
if (isset($contact[$field]) && is_string($contact[$field]) && $contact[$field] !== '') {
return TRUE;
}
}
// For existing contacts, look up name from database
if ($cid) {
foreach ($nameFields[$contactType] as $field) {
$value = $contact[$field] ?? CRM_Core_DAO::getFieldValue(__CLASS__, $cid, $field);
if (isset($value) && $value !== '') {
return TRUE;
}
}
}
return FALSE;
}

/**
* Format the output of the create contact function
*
Expand Down
2 changes: 1 addition & 1 deletion CRM/Contact/BAO/Contact/Utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ public static function validChecksum($contactID, $inputCheck) {
*/
public static function createCurrentEmployerRelationship($contactID, $organization, $previousEmployerID = NULL, $newContact = FALSE) {
//if organization name is passed. CRM-15368,CRM-15547
if ($organization && !is_numeric($organization)) {
if (!CRM_Utils_System::isNull($organization) && !is_numeric($organization)) {
$dupeIDs = CRM_Contact_BAO_Contact::getDuplicateContacts(['organization_name' => $organization], 'Organization', 'Unsupervised', [], FALSE);

if (is_array($dupeIDs) && !empty($dupeIDs)) {
Expand Down
12 changes: 2 additions & 10 deletions CRM/Contact/Form/Inline/Email.php
Original file line number Diff line number Diff line change
Expand Up @@ -170,17 +170,9 @@ public function postProcess() {
}
CRM_Core_BAO_Block::create('email', $params);

// If contact has no name, set primary email as display name
// TODO: This should be handled in the BAO for the benefit of the api, etc.
// Changing email might change a contact's display_name so refresh name block content
if (!$this->contactHasName) {
foreach ($params['email'] as $email) {
if ($email['is_primary']) {
CRM_Core_DAO::setFieldValue('CRM_Contact_DAO_Contact', $this->_contactId, 'display_name', $email['email']);
Copy link
Contributor

Choose a reason for hiding this comment

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

ohh nasty - set from the form!

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

lol

CRM_Core_DAO::setFieldValue('CRM_Contact_DAO_Contact', $this->_contactId, 'sort_name', $email['email']);
$this->ajaxResponse['reloadBlocks'] = ['#crm-contactname-content'];
break;
}
}
$this->ajaxResponse['reloadBlocks'] = ['#crm-contactname-content'];
}

$this->log();
Expand Down
5 changes: 4 additions & 1 deletion CRM/Core/BAO/Block.php
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ public static function handlePrimary(&$params, $class) {
$entity = new $class();
$entity->id = $params['id'];
$entity->find(TRUE);
$contactId = $entity->contact_id;
$contactId = $params['contact_id'] = $entity->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.

OK - this line is only hit if $params['contact_id'] is not set

}
// If entity is not associated with contact, concept of is_primary not relevant
if (!$contactId) {
Expand Down Expand Up @@ -397,6 +397,9 @@ public static function handlePrimary(&$params, $class) {
// primary or return if is already is
$existingEntities->is_primary = 1;
$existingEntities->save();
if ($class === 'CRM_Core_BAO_Email') {
CRM_Core_BAO_Email::updateContactName($contactId, $existingEntities->email);
}
}
}

Expand Down
21 changes: 21 additions & 0 deletions CRM/Core/BAO/Email.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,12 @@ public static function create($params) {

$email->save();

$contactId = (int) ($email->contact_id ?? CRM_Core_DAO::getFieldValue(__CLASS__, $email->id, 'contact_id'));
if ($contactId && $email->is_primary) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is changed from primary to not primary no update will happen but I think that is ok because what is set in place of this will trigger a change

$address = $email->email ?? CRM_Core_DAO::getFieldValue(__CLASS__, $email->id, 'email');
self::updateContactName($contactId, $address);
}

if ($email->is_primary) {
// update the UF user email if that has changed
CRM_Core_BAO_UFMatch::updateUFName($email->contact_id);
Expand Down Expand Up @@ -361,4 +367,19 @@ public static function getEntityRefFilters() {
return $contactFields;
}

/**
*
*
* @param int $contactId
* @param string $primaryEmail
*/
public static function updateContactName($contactId, string $primaryEmail) {
if (is_string($primaryEmail) && $primaryEmail !== '' &&
!CRM_Contact_BAO_Contact::hasName(['id' => $contactId])
) {
CRM_Core_DAO::setFieldValue('CRM_Contact_DAO_Contact', $contactId, 'display_name', $primaryEmail);
CRM_Core_DAO::setFieldValue('CRM_Contact_DAO_Contact', $contactId, 'sort_name', $primaryEmail);
}
}

}
3 changes: 3 additions & 0 deletions Civi/Api4/Service/Spec/Provider/EmailCreationSpecProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ public function modifySpec(RequestSpec $spec) {
$spec->getFieldByName('email')->setRequired(TRUE);
$spec->getFieldByName('on_hold')->setRequired(FALSE);
$spec->getFieldByName('is_bulkmail')->setRequired(FALSE);

$defaultLocationType = \CRM_Core_BAO_LocationType::getDefault()->id ?? NULL;
$spec->getFieldByName('location_type_id')->setDefaultValue($defaultLocationType);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ext/afform/admin/Civi/AfformAdmin/AfformAdminMeta.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public static function getFields($entityName, $params = []) {
'includeCustom' => TRUE,
'loadOptions' => ['id', 'label'],
'action' => 'create',
'select' => ['name', 'label', 'input_type', 'input_attrs', 'required', 'options', 'help_pre', 'help_post', 'serialize', 'data_type', 'fk_entity'],
'select' => ['name', 'label', 'input_type', 'input_attrs', 'required', 'options', 'help_pre', 'help_post', 'serialize', 'data_type', 'fk_entity', 'readonly'],
'where' => [['input_type', 'IS NOT NULL']],
];
if (in_array($entityName, ['Individual', 'Household', 'Organization'])) {
Expand Down
4 changes: 3 additions & 1 deletion ext/afform/admin/ang/afGuiEditor/afGuiEntity.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@

function filterFields(fields) {
return _.transform(fields, function(fieldList, field) {
if (!search || _.contains(field.name, search) || _.contains(field.label.toLowerCase(), search)) {
if (!field.readonly &&
(!search || _.contains(field.name, search) || _.contains(field.label.toLowerCase(), search))
) {
fieldList.push(fieldDefaults(field));
}
}, []);
Expand Down
22 changes: 13 additions & 9 deletions ext/afform/core/Civi/Afform/Event/AfformSubmitEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,19 @@
use Civi\Api4\Action\Afform\Submit;

/**
* Class AfformSubmitEvent
* @package Civi\Afform\Event
* Handle submission of an "<af-form>" entity (or set of entities in the case of `<af-repeat>`).
*
* Handle submission of an "<af-form>" entity.
* @see Submit::processGenericEntity
* which is the default handler of this event, with priority 0.
*
* The default handler of this event is `Submit::processGenericEntity`
* If special processing for an entity type is desired, add a new listener with a higher priority
* than 0, and either manipulate the $records and allow the default listener to perform the save,
* or fully process the save and cancel event propagation to bypass `processGenericEntity`.
* than 0, and do one of two things:
*
* 1. Fully process the save, and cancel event propagation to bypass `processGenericEntity`.
* 2. Manipulate the $records and allow the default listener to perform the save.
* Setting $record['fields'] = NULL will cancel saving a record, e.g. if the record is not valid.
*
* @package Civi\Afform\Event
*/
class AfformSubmitEvent extends AfformBaseEvent {

Expand Down Expand Up @@ -54,14 +58,14 @@ class AfformSubmitEvent extends AfformBaseEvent {
* @param array $afform
* @param \Civi\Afform\FormDataModel $formDataModel
* @param \Civi\Api4\Action\Afform\Submit $apiRequest
* @param array $values
* @param array $records
* @param string $entityType
* @param string $entityName
* @param array $entityIds
*/
public function __construct(array $afform, FormDataModel $formDataModel, Submit $apiRequest, &$values, string $entityType, string $entityName, array &$entityIds) {
public function __construct(array $afform, FormDataModel $formDataModel, Submit $apiRequest, &$records, string $entityType, string $entityName, array &$entityIds) {
parent::__construct($afform, $formDataModel, $apiRequest);
$this->records =& $values;
$this->records =& $records;
$this->entityType = $entityType;
$this->entityName = $entityName;
$this->entityIds =& $entityIds;
Expand Down
35 changes: 35 additions & 0 deletions ext/afform/core/Civi/Api4/Action/Afform/Submit.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,38 @@ private function replaceReferences($entityName, $records) {
return $records;
}

/**
* Validate contact(s) meet the minimum requirements to be created (name and/or email).
*
* This requires a function because simple required fields validation won't work
* across multiple entities (contact + n email addresses).
*
* @param \Civi\Afform\Event\AfformSubmitEvent $event
* @throws \API_Exception
* @see afform_civicrm_config
*/
public static function preprocessContact(AfformSubmitEvent $event): void {
if ($event->getEntityType() !== 'Contact') {
return;
}
// When creating a contact, verify they have a name or email address
foreach ($event->records as $index => $contact) {
if (!empty($contact['fields']['id'])) {
continue;
}
if (empty($contact['fields']) || \CRM_Contact_BAO_Contact::hasName($contact['fields'])) {
continue;
}
foreach ($contact['joins']['Email'] ?? [] as $email) {
if (!empty($email['email'])) {
continue 2;
}
}
// Contact has no id, name, or email. Stop creation.
$event->records[$index]['fields'] = NULL;
}
}

/**
* @param \Civi\Afform\Event\AfformSubmitEvent $event
* @throws \API_Exception
Expand All @@ -85,6 +117,9 @@ private function replaceReferences($entityName, $records) {
public static function processGenericEntity(AfformSubmitEvent $event) {
$api4 = $event->getSecureApi4();
foreach ($event->records as $index => $record) {
if (empty($record['fields'])) {
continue;
}
try {
$saved = $api4($event->getEntityType(), 'save', ['records' => [$record['fields']]])->first();
$event->setEntityId($index, $saved['id']);
Expand Down
1 change: 1 addition & 0 deletions ext/afform/core/afform.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ function afform_civicrm_config(&$config) {

$dispatcher = Civi::dispatcher();
$dispatcher->addListener(Submit::EVENT_NAME, [Submit::class, 'processGenericEntity'], 0);
$dispatcher->addListener(Submit::EVENT_NAME, [Submit::class, 'preprocessContact'], 10);
$dispatcher->addListener('hook_civicrm_angularModules', ['\Civi\Afform\AngularDependencyMapper', 'autoReq'], -1000);
$dispatcher->addListener('hook_civicrm_alterAngular', ['\Civi\Afform\AfformMetadataInjector', 'preprocess']);
$dispatcher->addListener('hook_civicrm_check', ['\Civi\Afform\StatusChecks', 'hook_civicrm_check']);
Expand Down
96 changes: 89 additions & 7 deletions ext/afform/mock/tests/phpunit/api/v4/AfformUsageTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,9 @@ public static function setUpBeforeClass(): void {
<af-entity data="{contact_type: 'Organization'}" type="Contact" name="Organization1" label="Organization 1" actions="{create: true, update: true}" security="RBAC" />
<fieldset af-fieldset="Individual1">
<legend class="af-text">Individual 1</legend>
<div class="af-container">
<div class="af-container af-layout-inline">
<af-field name="first_name" />
<af-field name="middle_name" />
<af-field name="last_name" />
<div class="af-container"></div>
</div>
<afblock-name-individual></afblock-name-individual>
<div af-join="Email" min="1" af-repeat="Add">
<afjoin-email-default></afjoin-email-default>
</div>
<af-field name="employer_id" defn="{input_type: 'Select', input_attrs: {}}" />
</fieldset>
Expand All @@ -58,6 +54,9 @@ public static function setUpBeforeClass(): void {
<div class="af-container">
<af-field name="organization_name" />
</div>
<div af-join="Email">
<afjoin-email-default></afjoin-email-default>
</div>
</fieldset>
<button class="af-button btn-primary" crm-icon="fa-check" ng-click="afform.submit()">Submit</button>
</af-form>
Expand Down Expand Up @@ -230,6 +229,89 @@ public function testEmployerReference(): void {
$this->assertEquals($orgName, $contact['org.organization_name']);
}

public function testEmptyEmployerReference(): void {
$this->useValues([
'layout' => self::$layouts['employer'],
'permission' => CRM_Core_Permission::ALWAYS_ALLOW_PERMISSION,
]);

$firstName = uniqid(__FUNCTION__);
$values = [
'Individual1' => [
[
'fields' => [
'first_name' => $firstName,
'last_name' => 'non-employee',
// This should result in a NULL value because organization_name is left blank
'employer_id' => 'Organization1',
],
],
],
'Organization1' => [
[
'fields' => [
'organization_name' => '',
],
],
],
];
Civi\Api4\Afform::submit()
->setName($this->formName)
->setValues($values)
->execute();
$contact = \Civi\Api4\Contact::get()
->addWhere('first_name', '=', $firstName)
->addWhere('last_name', '=', 'non-employee')
->addSelect('employer_id')
->execute()->first();
$this->assertNull($contact['employer_id']);
}

public function testCreatingContactsWithOnlyEmail(): void {
$this->useValues([
'layout' => self::$layouts['employer'],
'permission' => CRM_Core_Permission::ALWAYS_ALLOW_PERMISSION,
]);

$individualEmail = uniqid('individual@') . '.test';
$orgEmail = uniqid('org@') . '.test';
$locationType = CRM_Core_BAO_LocationType::getDefault()->id;
$values = [
'Individual1' => [
[
'fields' => [
'employer_id' => 'Organization1',
],
'joins' => [
'Email' => [
['email' => $individualEmail, 'location_type_id' => $locationType],
],
],
],
],
'Organization1' => [
[
'fields' => [],
'joins' => [
'Email' => [
['email' => $orgEmail, 'location_type_id' => $locationType],
],
],
],
],
];
Civi\Api4\Afform::submit()
->setName($this->formName)
->setValues($values)
->execute();
$contact = \Civi\Api4\Contact::get()
->addWhere('display_name', '=', $individualEmail)
->addJoin('Contact AS org', 'LEFT', ['employer_id', '=', 'org.id'])
->addSelect('display_name', 'org.display_name')
->execute()->first();
$this->assertEquals($orgEmail, $contact['org.display_name']);
}

protected function useValues($values) {
$defaults = [
'title' => 'My form',
Expand Down
Loading