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

Afform - Fix saving joined entities (email, address, phone, etc) #20264

Merged
merged 1 commit into from
May 13, 2021
Merged

Afform - Fix saving joined entities (email, address, phone, etc) #20264

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
4 changes: 2 additions & 2 deletions ext/afform/core/Civi/Api4/Action/Afform/AbstractProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@ protected static function getJoinWhereClause($mainEntityName, $joinEntityName, $
if (self::fieldExists($joinEntityName, 'entity_id')) {
$params[] = ['entity_id', '=', $mainEntityId];
if (self::fieldExists($joinEntityName, 'entity_table')) {
$params[] = ['entity_table', '=', 'civicrm_' . _civicrm_api_get_entity_name_from_camel($mainEntityName)];
$params[] = ['entity_table', '=', 'civicrm_' . \CRM_Core_DAO_AllCoreTables::convertEntityNameToLower($mainEntityName)];
}
}
else {
$mainEntityField = _civicrm_api_get_entity_name_from_camel($mainEntityName) . '_id';
$mainEntityField = \CRM_Core_DAO_AllCoreTables::convertEntityNameToLower($mainEntityName) . '_id';
$params[] = [$mainEntityField, '=', $mainEntityId];
}
return $params;
Expand Down
19 changes: 13 additions & 6 deletions ext/afform/core/Civi/Api4/Action/Afform/Submit.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public static function processContacts(AfformSubmitEvent $event) {
$api4 = $event->formDataModel->getSecureApi4($entityName);
foreach ($contacts as $contact) {
$saved = $api4('Contact', 'save', ['records' => [$contact['fields']]])->first();
self::saveJoins($api4, 'Contact', $saved['id'], $contact['joins'] ?? []);
self::saveJoins('Contact', $saved['id'], $contact['joins'] ?? []);
}
}
unset($event->entityValues['Contact']);
Expand All @@ -72,26 +72,33 @@ public static function processGenericEntity(AfformSubmitEvent $event) {
$api4 = $event->formDataModel->getSecureApi4($entityName);
foreach ($records as $record) {
$saved = $api4($entityType, 'save', ['records' => [$record['fields']]])->first();
self::saveJoins($api4, $entityType, $saved['id'], $record['joins'] ?? []);
self::saveJoins($entityType, $saved['id'], $record['joins'] ?? []);
}
}
unset($event->entityValues[$entityType]);
}
}

protected static function saveJoins($api4, $mainEntityName, $entityId, $joins) {
protected static function saveJoins($mainEntityName, $entityId, $joins) {
foreach ($joins as $joinEntityName => $join) {
$values = self::filterEmptyJoins($joinEntityName, $join);
// FIXME: Replace/delete should only be done to known contacts
// TODO: REPLACE works for creating or updating contacts, but different logic would be needed if
// the contact was being auto-updated via a dedupe rule; in that case we would not want to
// delete any existing records.
if ($values) {
$api4($joinEntityName, 'replace', [
civicrm_api4($joinEntityName, 'replace', [
// Disable permission checks because the main entity has already been vetted
'checkPermissions' => FALSE,
Copy link
Contributor

Choose a reason for hiding this comment

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

So this seems to be the guts of the pr - what limits it to only address etc entities and not others?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will only fire for af-join entitites which are like "sub-entities" of a Contact record (aside: in theory it could be possible to have a sub-entity of something else but so far it's just Contacts). And it only fires after the Contact has been saved, which would have thrown an exception if authentication failed. So at this point we already know that saving the Contact is allowed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw ok - but how do we ensure that when we extend the entities we don't accidentally expose more inappropriate sub entities - how are they defined? Presumably pro-actively?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also what if you are trying to update a joined case say from an activity but you don't have case permissions how would this work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes they have to be explicitly defined in code, e.g. afjoinAddressDefault.aff.json. And then they have to be explicitly added to the form by the form author.

Copy link
Member Author

@colemanw colemanw May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@seamuslee001 af-joins really are sub-entities. Other than email, phone, address, website, im, and multi-record custom fields, I don't expect there to be others to follow this pattern.

Activities and Cases are both full entities in their own right. We wouldn't use the af-join pattern with them. We'd probably use an EntityRef to link them, although we'd have to do something about bridging them through the ActivityContact table. But that's a separate issue to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw I think that deals with my concerns on a technical level but I'm still nervous on a docs level - ie the concept of sub-entities & implications should be documented. I think the urgency of having a home for afform documentation is increasing (@MikeyMJCO @totten @seamuslee001 )

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/913 to start to establish a docs location. It is very minimal but hopefully we could put something about how sub-entities are declared in there

'where' => self::getJoinWhereClause($mainEntityName, $joinEntityName, $entityId),
'records' => $values,
]);
}
// REPLACE doesn't work if there are no records, have to use DELETE
else {
try {
$api4($joinEntityName, 'delete', [
civicrm_api4($joinEntityName, 'delete', [
// Disable permission checks because the main entity has already been vetted
'checkPermissions' => FALSE,
'where' => self::getJoinWhereClause($mainEntityName, $joinEntityName, $entityId),
]);
}
Expand Down