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

CATL-1613: Add options to Mail Account settings to improve inbound mail processing #42

Open
wants to merge 1,197 commits into
base: master
Choose a base branch
from

Conversation

i-grou
Copy link

@i-grou i-grou commented Aug 17, 2020

Overview

According to new requirements we need to add an additional options to the Mail Account settings (see Mail Accounts page /civicrm/admin/mailSettings?action=add&reset=1) to change the email to activity processing behavior - when Email-to-Activity Processing value is selected for Used For? field we need to:

  1. Show an additional checkbox field - Skip emails which do not have a Case ID or Case token (defaults to unticked).
    If checked: emails which do not have a valid case ID or case token should be moved into the ignored mail folder after processing and no activity should be created.
  2. Show an additional checkbox field - Do not create new contacts when filing emails (defaults to unticked).
    If checked: when CiviCRM checks for a matching contact, if no matching contact is found it will not create one and the email is filed.
    So now administrator should be able to configure whether CiviCRM will create a new contact where a contact does not already exist within the system when filing inbound emails, so that he can ensure that new contacts are not created in the system when filing an email. The email should still be filed, even if no contacts are matched.

Email is considered to be related to cases if it's subject matches any of the following conditions:

  • Starts with [<any string> #<any string>], e.g: [case #2cd8467], [policy initiative #bdf3896].
  • Ends with Case ID <number>, e.g: Case ID 1298, case id 9.

Also:

  • Label of new field is printed to the right of the checkbox, not to the left, because left side is thin and long labels would look weird there.
  • Activity Status field label is also moved to the right for consistency. Thanks to this approach these fields would look more like Used For? dependent fields, and this would make them look more intuitive hence will improve UX.

Before

Without the new options above enabled CiviCRM will process emails as normal i.e:

  1. Will file emails without a case ID or case hash in the subject line against a contact record.
  2. Will always create new contacts if no matching contact found.

And Activity Status field label location is not changed.
image

After

With the new options above enabled CiviCRM:

  1. Will skip any emails that do not have either the case ID or case hash in the subject line and will instead move those emails to the ignored folder.
  2. Will not create new contacts if no matching contact found (from / to / cc / bcc), but email still will be filed.

Now form looks like this:
image

Technical Details

To make the Skip emails which do not have a Case ID or Case token checkbox work we:

  1. Added new field to the civicrm_mail_settings db table - is_non_case_email_skipped. Respective upgrader is added.
  2. Updated schema MailSettings.xml file to have new field.
  3. Updated form related files - Form/MailSettings.php and MailSettings.tpl - to print new field and save the value on form submit. As it has long help text it was desided to move it to MailSettings.hlp file and use civicrm ajax help popup feature.
  4. Updated EmailProcessor.php to handle new field during inbound email processing.
  5. The CRM_Utils_Mail_CaseMail class is added to store isCaseEmail() method, which is used to check if email is related to cases or not before email processing (creating new activity). The regexp patterns are copied from CRM_Activity_BAO_Activity->create() method for consistency, as it also performs this check.
  6. The caseEmailSubjectPatterns() hook is added to make it possible to change default email subject regexp patterns. It may be useful for example for CiviCase extension to add case type category names to patterns.
  7. Unit tests were added to test processing (with is_non_case_email_skipped enabled) of 2 emails: with case hash in subject and without case hash.
  8. Also as part of this PR formatting of MailSettings.tpl file is fixed.
  9. Also as part of this PR the deprecated _civicrm_api3_deprecated_activity_buildmailparams() method was moved to CRM_Utils_Mail_ParamsBuilder class that was just created and now we can use CRM_Utils_Mail_ParamsBuilder->buildActivityParams() instead. This was done to get rid of some toxic code.

To make the Do not create new contacts when filing emails checkobx work we:

  1. Added new field to the civicrm_mail_settings db table - is_contact_creation_disabled_if_no_match. Respective upgrader is added.
  2. Updated DAO/MailSettings.php and schema MailSettings.xml files to have new field.
  3. Updated form related files - Form/MailSettings.php and MailSettings.tpl - to print new field and save the value on form submit.
  4. Updated EmailProcessor.php to handle new field during inbound email processing. This also involved updating the Incoming.php to make contact creation optional.
  5. Unit tests were added to test processing with is_contact_creation_disabled_if_no_match enabled.

About the new CRM_Utils_Mail_CaseMail class

This class has a potential to become generic and widely used class, thats why:
• Email subject patterns are not just hardcoded, but are reusable, see getSubjectPatterns() method.
• The getSubjectPatterns() method is written keeping in mind CiviCase extension (thats why caseEmailSubjectPatterns() hook is added), which has case type categories, which names could be used in email subject, e.g: [case #hahoheh] Super email -> [policy initiative #hahoheh] Super email.

For now the most useful part of this class is isCaseEmail() method, but some other methods may be added later. For example:

  1. Generate case hash - there is a code duplication in civicrm to generate case hash, i.e:
    $hash = substr(sha1(CIVICRM_SITE_KEY . $form->_caseId), 0, 7).
  2. Generate case subject - there is a code duplication in civicrm to generate case related mail subject, i.e:
    $subject = "[case #$hash] $subject".

⚠ The email regexp patterns are copied from CRM_Activity_BAO_Activity->create() and now it makes sense to refactor that method to use CRM_Utils_Mail_CaseMail->getSubjectPatterns() to get patterns. But will skip it now as it's not in the scope of the task.

One more comment

I've noticed that civicrm stops processing of inbound email if it failed to connect to some mail account. This may cause some issues in case if there are several accounts created: if processing would be stopped on first account - then no other accounts would be processed as well.
I guess this should be added to the todo list to fix in the nearest future.

@i-grou i-grou force-pushed the CATL-1613-skip-non-case-email branch 8 times, most recently from de3440b to 5f754b5 Compare August 20, 2020 11:43
@i-grou i-grou force-pushed the CATL-1613-skip-non-case-email branch 2 times, most recently from e8f78a0 to 248c73d Compare September 10, 2020 08:59
eileenmcnaughton and others added 20 commits September 22, 2020 16:59
There is only 1 remaining place that calls this function & does not specifiy activityType. This fixes
that place to pass in activityType and stops attempting to calculate activityType
based on in-function guess work
[REF] Update composer compile plugin to latest version
[REF] Finally remove deprecated ids handling
[REF] Move daoName generation so we don't need to pass the variable name
…Entity function

This seems more legible than the metadata trick....
dev/core#2017 Remove unused function OpenID::isAllowedToLogin
Minor test data fix up  - ensure domain contact's email is primary
dev/core#2030 ensure that the Country selector is a Select 2 and ensu…
Maybe this will help us understand why the test is intermittant
dev/core#2040 - Multiple email activity cc recipients get scrunched together in recorded activity details field
[REF] Remove wrangling on activityType param
Switch membership BAO to use non-deprecated cached functions to get membershipType details
@i-grou i-grou force-pushed the CATL-1613-skip-non-case-email branch 3 times, most recently from 2b3c03d to fbb997c Compare October 6, 2020 10:02
pradpnayak and others added 19 commits October 6, 2020 11:48
dev/core#2093 - Fix red error box on new individual form and ltrim typos and doubling-up of class attribute
change civicrm_price_set.min_amount to float
dev/core#2079 [REF] clean up call to apiQuery
dev/core#2046 Fix blockDelete to delete while ensuring is_primary is valid
dev/core#2046 Migrate BAO_Address::create towards standardisation
…bility

[REF] Fix compatability with Drupal 9 installing of var_dumper
blockDelete is now just wrapper for del so this is really NFC
Search ext: rename to Search Kit, mark as beta
dev/core#2039 Call del directly - rather than now-deprecated blockDelete
dev/core#1790 - Contact Card - Email Links
[REF] Fix error when creating new AB test mailing because domain_id i…
[NFC] Update Checksum on CustomField DAO
@i-grou i-grou force-pushed the CATL-1613-skip-non-case-email branch 2 times, most recently from fbb997c to 234faf6 Compare October 7, 2020 07:50
@i-grou i-grou force-pushed the CATL-1613-skip-non-case-email branch from 234faf6 to f945f0c Compare October 7, 2020 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.