Allow for configuration of activity contacts, type and campaign for email-to-activity #26905

larssandergreen · 2023-07-23T19:56:07Z

Overview

Email-to-activity processing is a powerful tool, but somewhat hampered by hardwired choices about which email addresses correspond to source, target and assignee contacts, which type of activity should be created, etc. This PR makes it possible to configure the activity source, target and assignee using the from, to, cc and bcc fields of the email according to the user's needs. It also makes it possible to configure the activity type, add a campaign, and to disable and enable a Mail Account.

Now, users can set up email to activity processing to process emails which were sent to the organization (the classic Inbound Email), emails that were sent by the organization to contacts (normally an Email activity), and emails that were sent by contacts to other contacts, copying the organization (probably a custom activity type). Users can use all three of these at the same time by setting up multiple mail accounts.

The popularity of the Inbound Email Activity extension shows there is demand for different configurations for email-to-activity, but this PR gives the user more control and allows for different configurations for different email accounts or different email folders.

Before

After

If the source is set to a field that can contain more than one email address, only the first email will be used as an activity can only have one source contact. Available activity types are Inbound Email, Email and all non-reserved types.

Comments

This works with the Inbound Email Activity extension (ping @JoeMurray), with no change for existing installs. Once this is merged, I can update the docs there to make it clearer how the extension works in the context of these settings.

civibot · 2023-07-23T19:56:09Z

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers

larssandergreen · 2023-07-23T19:59:36Z

CRM/Admin/Form/MailSettings.php

@@ -109,11 +127,15 @@ public function getDefaultEntity() {
  public function setDefaultValues() {
    $defaults = parent::setDefaultValues();

-    // Set activity status to "Completed" by default.
-    if ($this->_action != CRM_Core_Action::DELETE &&
-      (!$this->_id || !CRM_Core_DAO::getFieldValue('CRM_Core_BAO_MailSettings', $this->_id, 'activity_status'))


This only condition in which !CRM_Core_DAO::getFieldValue('CRM_Core_BAO_MailSettings', $this->_id, 'activity_status') would apply is if you were switching an existing bounce processing mail account to an email-to-activity account, which would happen rarely-to-never, so I don't we need to handle this separately, since the consequence is low (the activity status would be scheduled by default instead of completed).

Another consequence besides what you mentioned about status is you end up with the default of only one of the from and recipients being recorded, and in general while not a technical problem, seems undesirable to allow. I.e. none of the people it was sent to get recorded.

Also note that this situation then ends up being the default choices when creating an account for Oauth. So it's not as rare as thought.

And personally changing from bounce to activity is something I do somewhat often since not all sites have bounce processing, but activity processing is used a lot in cases.

Otherwise though the PR overall looks pretty good. I haven't looked at the admin ui part. Seems there's a separate discussion there.

I see what you mean, but not sure how to handle that. Could change the upgrade and the generated data to also set those defaults for bounce processing, which is fine because they don't do anything for bounce processing.
But that doesn't cover oauth. For that I think we'd need to add defaults in the oauth process or add defaults directly to the DB.
We could check if the activity status is not set and add defaults in that case, but that feels a bit odd and prone to create problems.
Thoughts?

I think the Admin UI part can go ahead as is. Hardly the only issue of that type in Admin UI.

larssandergreen · 2023-07-23T20:00:51Z

CRM/Admin/Form/MailSettings.php

-      (!$this->_id || !CRM_Core_DAO::getFieldValue('CRM_Core_BAO_MailSettings', $this->_id, 'activity_status'))
-    ) {
+    if (!$this->_id) {
+      $defaults['is_ssl'] = TRUE;


Added sensible defaults for new mail accounts here to check Use SSL and select email-to-activity instead of bounce processing (of which there can only be one anyways).

larssandergreen · 2023-07-23T20:04:12Z

CRM/Utils/Mail/Incoming.php

-
-    // we definitely need a contact id for the from address
-    // if we dont have one, skip this email
-    if ($requireContact && empty($params['from']['id'])) {


This check doesn't make sense in the new context as we don't know what the mapping from email fields to activity contacts is here. But it doesn't matter as this is handled in EmailProcessor at 210 anyways, since the condition can also come up in other ways (for instance, if $requireContact is not true, we still can't create an activity without a source contact).

larssandergreen · 2023-07-23T20:04:47Z

ext/civicrm_admin_ui/managed/SavedSearch_Manage_Mail_Accounts.mgd.php

@@ -2,6 +2,7 @@

 use CRM_CivicrmAdminUi_ExtensionUtil as E;

+// This SearchDisplay shows an editable-in-place field for Enabled? for all rows, including the bounce processing mail account, which cannot actually be disabled (you can change it to No, but it won't actually be disabled). So this is FIXME for when we can set rows to edit-in-place conditionally.


@colemanw Any thoughts on how to handle this? I think it has come up in another context as well, but can't find that at the moment.

Hmm, those edit-in-place links will check edit permissions before they render. So if the system had a rule like "you're not allowed to edit this" then it wouldn't be editable.
But lately we've been using the Enable/Disable action links instead of in-place-edit (or in-addition to, but I don't think both are strictly necessary). Those will have the same problem, so I guess we need to tweak the checkAccess callback for that entity to make sure it returns false for the ones that shouldn't be edited.

larssandergreen · 2023-07-23T20:05:45Z

tests/phpunit/CRM/Utils/Mail/EmailProcessorTest.php

@@ -242,6 +248,7 @@ public function setUpDoNotCreateContact() {
        'domain' => 'example.com',
        'is_default' => '0',
        'is_contact_creation_disabled_if_no_match' => TRUE,
+        'is_non_case_email_skipped' => FALSE,


This test wasn't actually testing anything before as it was skipping the non-case email.

CRM/Upgrade/Incremental/php/FiveSixtyFive.php

demeritcowboy · 2023-07-27T13:44:21Z

I plan to look at this.
FYI there's a merge conflict.

larssandergreen · 2023-07-27T13:49:40Z

Thanks @demeritcowboy. It just occurred to me a couple minutes ago that there is a bug in here. I'll fix that today and resolve the merge conflict.

CRM/Utils/Mail/Incoming.php

larssandergreen · 2023-07-29T19:56:59Z

Jenkins test this please

demeritcowboy · 2023-08-02T02:43:38Z

merge conflict from something yesterday, but not necessary for review

larssandergreen · 2023-08-05T19:39:28Z

@demeritcowboy I've moved the upgrade steps to FiveSixtySix as required.

demeritcowboy · 2023-08-05T19:54:48Z

Thanks. Yep this is still on my todo list.

larssandergreen · 2023-08-05T20:11:44Z

Thanks, appreciate that this one is a project to review.

xml/schema/Core/MailSettings.xml

templates/CRM/Admin/Page/MailSettings.hlp

CRM/Upgrade/Incremental/php/FiveSixtySix.php

demeritcowboy · 2023-08-07T04:14:29Z

CRM/Utils/Mail/EmailProcessor.php

-      throw new CRM_Core_Exception(ts('Could not find a valid Activity Type ID for Inbound Email'));
+    if ($usedfor == 0) {
+      // create an array of all of to, from, cc, bcc that are in use for this Mail Account, so we don't create contacts for emails we aren't adding to the activity
+      $emailFields = array_unique(array_merge(explode(",", $dao->activity_targets), explode(",", $dao->activity_assignees), explode(",", $dao->activity_source)));


If I tell it I don't want any assignees, then $dao->activity_assignees is the empty string, which exploded gives array(0 => ''), which then later tries to look for $mail->'' which gives a fatal error.

But also array_merge doesn't seem right here, because it's merging based on (semi-random) numeric keys, which doesn't actually "merge" because (a) it's not the values, and (b) php is either weird or awesome depending on your point of view and it won't merge numeric keys. But ok that doesn't matter because of the unique(). (But the first point is still true.)

Yes, intentionally doing the weird PHP thing here. Open to better ways though.

Maybe it would be best to use a SEPARATOR_BOOKEND or some other serialization for these fields?

I suppose you'd still have to explode since you'll always have a string to start with. Could add an array_filter()?

It doesn't look pretty, but it works with empty values now. Thanks for catching that.

demeritcowboy · 2023-08-12T19:17:20Z

CRM/Admin/Form/MailSettings.php

+    if (!$this->_id) {
+      $defaults['is_ssl'] = TRUE;
+      $defaults['is_default'] = 0;
+      $defaults['activity_type_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Inbound Email');
      $defaults['activity_status'] = 'Completed';
+      $defaults['activity_source'] = 'from';
+      $defaults['activity_targets'] = 'to,cc,bcc';
+      $defaults['activity_assignees'] = 'from';
+      $defaults['is_active'] = TRUE;
    }


Looking at how the parent form works, how about (plus the upgrade change for bounce acct)

Suggested change

if (!$this->_id) {

$defaults['is_ssl'] = TRUE;

$defaults['is_default'] = 0;

$defaults['activity_type_id'] = CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Inbound Email');

$defaults['activity_status'] = 'Completed';

$defaults['activity_source'] = 'from';

$defaults['activity_targets'] = 'to,cc,bcc';

$defaults['activity_assignees'] = 'from';

$defaults['is_active'] = TRUE;

}

$defaults['is_ssl'] = $defaults['is_ssl'] ?? TRUE;

$defaults['is_default'] = $defaults['is_default'] ?? 0;

$defaults['activity_type_id'] = $defaults['activity_type_id'] ?? CRM_Core_PseudoConstant::getKey('CRM_Activity_BAO_Activity', 'activity_type_id', 'Inbound Email');

$defaults['activity_status'] = $defaults['activity_status'] ?? 'Completed';

$defaults['activity_source'] = $defaults['activity_source'] ?? 'from';

$defaults['activity_targets'] = $defaults['activity_targets'] ?? 'to,cc,bcc';

$defaults['activity_assignees'] = $defaults['activity_assignees'] ?? 'from';

$defaults['is_active'] = $defaults['is_active'] ?? TRUE;

Seems to work for me.

Yes, this is perfect, have done this and updated the upgrade so that it sets the default values for bounce processing as well.

demeritcowboy · 2023-08-18T16:04:28Z

Oops this has upgrade merge conflicts again.
And what do you think of the setDefaults change above?

larssandergreen · 2023-08-19T00:36:34Z

Thanks @demeritcowboy, merge conflict resolved.

larssandergreen · 2023-08-19T01:01:11Z

Test fail is testGetOptionsBasic, unrelated.

demeritcowboy · 2023-08-19T01:02:29Z

yeah it's failing on every PR.

demeritcowboy · 2023-08-19T01:25:39Z

Yay!

larssandergreen · 2023-08-19T01:48:49Z

Thanks!

civibot bot added the master label Jul 23, 2023

larssandergreen commented Jul 23, 2023

View reviewed changes

larssandergreen added the has-test label Jul 23, 2023

larssandergreen force-pushed the More-inbound-email-processing-options branch from e831686 to 569995c Compare July 23, 2023 20:41

demeritcowboy reviewed Jul 24, 2023

View reviewed changes

CRM/Upgrade/Incremental/php/FiveSixtyFive.php Outdated Show resolved Hide resolved

larssandergreen force-pushed the More-inbound-email-processing-options branch from 569995c to c73c682 Compare July 24, 2023 16:28

larssandergreen marked this pull request as draft July 28, 2023 05:34

larssandergreen force-pushed the More-inbound-email-processing-options branch from c73c682 to 528e042 Compare July 29, 2023 19:53

larssandergreen commented Jul 29, 2023

View reviewed changes

CRM/Utils/Mail/Incoming.php Outdated Show resolved Hide resolved

larssandergreen force-pushed the More-inbound-email-processing-options branch from 528e042 to 4a1b45a Compare July 29, 2023 19:59

larssandergreen marked this pull request as ready for review July 29, 2023 20:48

larssandergreen force-pushed the More-inbound-email-processing-options branch 3 times, most recently from b26b769 to a446d6e Compare July 31, 2023 19:26

larssandergreen force-pushed the More-inbound-email-processing-options branch from a446d6e to d39befe Compare August 5, 2023 19:38

demeritcowboy reviewed Aug 6, 2023

View reviewed changes

xml/schema/Core/MailSettings.xml Outdated Show resolved Hide resolved

demeritcowboy reviewed Aug 6, 2023

View reviewed changes

templates/CRM/Admin/Page/MailSettings.hlp Outdated Show resolved Hide resolved

demeritcowboy reviewed Aug 6, 2023

View reviewed changes

CRM/Upgrade/Incremental/php/FiveSixtySix.php Show resolved Hide resolved

larssandergreen force-pushed the More-inbound-email-processing-options branch 2 times, most recently from c98813a to 0b0c20d Compare August 6, 2023 19:22

demeritcowboy reviewed Aug 7, 2023

View reviewed changes

larssandergreen force-pushed the More-inbound-email-processing-options branch from 0b0c20d to aacdb40 Compare August 7, 2023 17:35

demeritcowboy reviewed Aug 12, 2023

View reviewed changes

larssandergreen force-pushed the More-inbound-email-processing-options branch from aacdb40 to 6b9eb83 Compare August 19, 2023 00:29

Allow for configuration of activity details from email to activity

df97bb6

larssandergreen force-pushed the More-inbound-email-processing-options branch from 418b04c to df97bb6 Compare August 19, 2023 00:34

demeritcowboy merged commit a5b74bc into civicrm:master Aug 19, 2023

demeritcowboy mentioned this pull request Aug 19, 2023

[NFC] Add some email processor hooks tests #27097

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for configuration of activity contacts, type and campaign for email-to-activity #26905

Allow for configuration of activity contacts, type and campaign for email-to-activity #26905

larssandergreen commented Jul 23, 2023 •

edited

Loading

civibot bot commented Jul 23, 2023

larssandergreen Jul 23, 2023

demeritcowboy Aug 10, 2023

larssandergreen Aug 10, 2023

larssandergreen Jul 23, 2023

larssandergreen Jul 23, 2023

larssandergreen Jul 23, 2023

colemanw Aug 7, 2023

larssandergreen Jul 23, 2023

demeritcowboy commented Jul 27, 2023

larssandergreen commented Jul 27, 2023

larssandergreen commented Jul 29, 2023

demeritcowboy commented Aug 2, 2023

larssandergreen commented Aug 5, 2023

demeritcowboy commented Aug 5, 2023

larssandergreen commented Aug 5, 2023

demeritcowboy Aug 7, 2023 •

edited

Loading

larssandergreen Aug 7, 2023

demeritcowboy Aug 7, 2023

larssandergreen Aug 7, 2023

demeritcowboy Aug 12, 2023

larssandergreen Aug 19, 2023

demeritcowboy commented Aug 18, 2023

larssandergreen commented Aug 19, 2023

larssandergreen commented Aug 19, 2023

demeritcowboy commented Aug 19, 2023

demeritcowboy commented Aug 19, 2023

larssandergreen commented Aug 19, 2023

		@@ -2,6 +2,7 @@

		use CRM_CivicrmAdminUi_ExtensionUtil as E;

		// This SearchDisplay shows an editable-in-place field for Enabled? for all rows, including the bounce processing mail account, which cannot actually be disabled (you can change it to No, but it won't actually be disabled). So this is FIXME for when we can set rows to edit-in-place conditionally.

Allow for configuration of activity contacts, type and campaign for email-to-activity #26905

Allow for configuration of activity contacts, type and campaign for email-to-activity #26905

Conversation

larssandergreen commented Jul 23, 2023 • edited Loading

Overview

Before

After

Comments

civibot bot commented Jul 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Jul 27, 2023

larssandergreen commented Jul 27, 2023

larssandergreen commented Jul 29, 2023

demeritcowboy commented Aug 2, 2023

larssandergreen commented Aug 5, 2023

demeritcowboy commented Aug 5, 2023

larssandergreen commented Aug 5, 2023

demeritcowboy Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demeritcowboy commented Aug 18, 2023

larssandergreen commented Aug 19, 2023

larssandergreen commented Aug 19, 2023

demeritcowboy commented Aug 19, 2023

demeritcowboy commented Aug 19, 2023

larssandergreen commented Aug 19, 2023

larssandergreen commented Jul 23, 2023 •

edited

Loading

demeritcowboy Aug 7, 2023 •

edited

Loading