dev/core#2644 Add Scheduled Communications extension

[colemanw]

Overview

Allows Scheduled Reminders to be created from SearchKit with an optional extension.

Before

Only a handful of Scheduled Reminder entities available (Contact, Participant, Member, Activity), with narrowly-restricted criteria.

After

When enabled, the "Scheduled Communications" extension allows any SearchKit search with a date column and a contact id column can be used for scheduled reminders.

Comments

See https://lab.civicrm.org/dev/core/-/issues/2644

[civibot]

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

• ➡️ Demo site for this PR
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

[colemanw]

@eileenmcnaughton this is ready for review
retest this please

[colemanw]

retest this please

[eileenmcnaughton]

Flushing system caches
~/buildkit/build/build-0/web/sites/all/modules/civicrm/ext/search_kit ~/bui
ldkit
TAP version 13
PHPUnit 8.5.27 #StandWithUkraine

Warning: Undefined array key "Civi\Core\Container" in /home/homer/buildkit/
build/build-0/web/sites/all/modules/civicrm/Civi/Core/Container.php on line
702

Warning: Trying to access array offset on value of type null in /home/homer
/buildkit/build/build-0/web/sites/all/modules/civicrm/Civi/Core/Container.p
hp on line 702

Warning: Trying to access array offset on value of type null in /home/homer
/buildkit/build/build-0/web/sites/all/modules/civicrm/Civi/Core/Container.p
hp on line 702

Fatal error: Uncaught Error: Call to a member function getPath() on null in
/home/homer/buildkit/build/build-0/web/sites/all/modules/civicrm/Civi/Test
/EventChecker.php:94
Stack trace:
#0 /home/homer/buildkit/build/build-0/web/sites/all/modules/civicrm/Civi/Te
st/EventChecker.php(36): Civi\Test\EventChecker->findAll()
#1 /home/homer/buildkit/build/build-0/web/sites/all/modules/civicrm/Civi/Te
st/CiviTestListenerPHPUnit7.php(64): Civi\Test\EventChecker->start(Object(C
ivi\Search\ActionMappingTest))
#2 phar:///home/homer/buildkit/extern/phpunit8/phpunit8.phar/phpunit/Framew
ork/TestResult.php(362): Civi\Test\CiviTestListenerPHPUnit7->startTest(Obje
ct(Civi\Search\ActionMappingTest))
#3 phar:///home/homer/buildkit/extern/phpunit8/phpunit8.phar/phpunit/Framew
ork/TestSuite.php(453): PHPUnit\Framework\TestResult->startTest(Object(Civi
\Search\ActionMappingTest))
#4 phar:///home/homer/buildkit/extern/phpunit8/phpunit8.phar/phpunit/Framew
ork/TestSuite.php(475): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Fra
mework\TestResult))
#5 phar:///home/homer/buildkit/extern/phpunit8/phpunit8.phar/phpunit/TextUI
/TestRunner.php(462): PHPUnit\Framework\TestSuite->run(Object(PHPUnit\Frame
work\TestResult))
#6 phar:///home/homer/buildkit/extern/phpunit8/phpunit8.phar/phpunit/TextUI
/Command.php(129): PHPUnit\TextUI\TestRunner->doRun(Object(PHPUnit\Framewor
k\TestSuite), Array, Array, true)
#7 phar:///home/homer/buildkit/extern/phpunit8/phpunit8.phar/phpunit/TextUI
/Command.php(101): PHPUnit\TextUI\Command->run(Array, true)
#8 /home/homer/buildkit/extern/phpunit8/phpunit8.phar(1721): PHPUnit\TextUI
\Command::main()
#9 {main}
thrown in /home/homer/buildkit/build/build-0/web/sites/all/modules/civicr
m/Civi/Test/EventChecker.php on line 94

[colemanw]

retest this please

[colemanw]

@eileenmcnaughton I moved the test to a different directory and now it's passing 🤷🏻

[eileenmcnaughton]

Non-blocking but === would be better

[eileenmcnaughton]

@colemanw I gave this a spin & was able to configure & test a Search-kit based scheduled reminders which went OK, and did resolve tokens.

Given the scope of this PR is mostly internal to this work & there is a lot of code I'm OK with it being merged as is & building on it - however, I think we need to ensure there are some safeguards in place in the SAME release this goes out in. The minimum version I can think of is

1. add new permission - administer search kit scheduled reminders
2. option to configure only available in search kit UI for users with this permission
3. saved searches with admin ui can't be modified by someone without the permission
4. saved search field frozen for users without the permission (it feels risky to me being able to change that once created anyway - I kinda lean to making people delete & recreate)
   

[JoeMurray]

Just checking that the new perm is validated for api calls as well as browser based actions.

[eileenmcnaughton]

@colemanw as my non-trusted user (has lots of administer but not the magic tier 1 or the new perm) I still see the search (which is fine) - however, I do not expect this user to be able to delete the communication - which they can

[eileenmcnaughton]

On the non-admin user delete the database still has the action_schedule entry (the second one)

[eileenmcnaughton]

However the screen fails to load with it deleted

[eileenmcnaughton]

Also, in my testing the orphan scheduled reminder did not go out, but it did block the non-saved search one too & this could be bad. I think this is a high risk area & it would be good to cover the specific scenario of an orphan sk communiction/reminder in a unit test

[eileenmcnaughton]

I think it might be worth adding a check on the expires_date field to the disabled / deleted search display test. There doesn't seem to be an is_active in civicrm_saved_search

[eileenmcnaughton]

Hmm this is tricksy - I'm getting an invalid field 'is_deceased'
UPDATE - ah because my base table is contribution & it is adding

$this->savedSearch['api_params']['where'][] = [$contactPrefix . 'is_deceased', '=', FALSE];
$this->savedSearch['api_params']['where'][] = [$contactPrefix . 'is_deleted', '=', FALSE];

My action schedule row
3,new_one,new one,,,23,id,1,day,after,receive_date,0,,,,,,,1,"","",,"

sk cash {contribution.financial_type_id:label}

",,yay,1,saved_search,,,,,,,Email,,,,,2023-08-30 05:08:48,2023-08-30 05:08:48,,

My search

[
  [
    "SavedSearch",
    "save",
    {
      "records": [
        {
          "name": "big_money",
          "label": "big money",
          "form_values": null,
          "mapping_id": null,
          "search_custom_id": null,
          "api_entity": "Contribution",
          "api_params": {
            "version": 4,
            "select": [
              "contact_id.sort_name",
              "total_amount",
              "financial_type_id:label",
              "contribution_status_id:label",
              "id",
              "receive_date"
            ],
            "orderBy": [],
            "where": [
              [
                "total_amount",
                ">",
                "1000"
              ]
            ],
            "groupBy": [],
            "join": [],
            "having": []
          },
          "expires_date": null,
          "description": null
        }
      ],
      "match": [
        "name"
      ]
    }
  ]
]

( ! ) CRM_Core_Exception: Invalid field 'is_deceased' in /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/Civi/Api4/Query/Api4SelectQuery.php on line 372 Call Stack #TimeMemoryFunctionLocation 10.0046381952{main}( ).../index.php:0 20.10921501712menu_execute_active_handler( $path = ???, $deliver = ??? ).../index.php:21 30.10941502256civicrm_invoke( 'ajax', 'rest' ).../menu.inc:527 40.27473107184CRM_Core_Invoke::invoke( $args = [0 => 'civicrm', 1 => 'ajax', 2 => 'rest'] ).../civicrm.module:472 50.27473107184CRM_Core_Invoke::_invoke( $args = [0 => 'civicrm', 1 => 'ajax', 2 => 'rest'] ).../Invoke.php:36 60.28783156832CRM_Core_Invoke::runItem( $item = ['id' => '147', 'domain_id' => '1', 'path' => 'civicrm/ajax/rest', 'access_callback' => [0 => 'CRM_Core_Permission', 1 => 'checkMenu'], 'access_arguments' => [0 => [...], 1 => 'or'], 'page_callback' => [0 => 'CRM_Utils_REST', 1 => 'ajax'], 'breadcrumb' => [0 => [...]], 'is_active' => '1', 'is_public' => '0', 'is_exposed' => '1', 'is_ssl' => '0', 'weight' => '1', 'type' => '1', 'page_type' => '0', 'skipBreadcrumb' => '0', 'page_arguments' => FALSE] ).../Invoke.php:69 70.30553523056CRM_Utils_REST::ajax( ).../Invoke.php:285 80.30573523840CRM_Utils_REST::process( $args = [0 => 'civicrm', 1 => 'job', 2 => 'send_reminder'], $params = [] ).../REST.php:533 90.30573524256civicrm_api( $entity = 'job', $action = 'send_reminder', $params = ['check_permissions' => TRUE, 'version' => 3] ).../REST.php:288 100.31153552288Civi\API\Kernel->runSafe( $entity = 'job', $action = 'send_reminder', $params = ['check_permissions' => TRUE, 'version' => 3] ).../api.php:28 110.31183552752Civi\API\Kernel->runRequest( $apiRequest = ['id' => 1, 'version' => 3, 'params' => ['check_permissions' => TRUE, 'version' => 3], 'fields' => NULL, 'entity' => 'Job', 'action' => 'send_reminder'] ).../Kernel.php:79 120.32433678344Civi\API\Provider\MagicFunctionProvider->invoke( $apiRequest = ['id' => 1, 'version' => 3, 'params' => ['check_permissions' => TRUE, 'version' => 3], 'fields' => [], 'entity' => 'Job', 'action' => 'send_reminder', 'function' => 'civicrm_api3_job_send_reminder', 'is_generic' => FALSE] ).../Kernel.php:156 130.32433678344civicrm_api3_job_send_reminder( $params = ['check_permissions' => TRUE, 'version' => 3] ).../MagicFunctionProvider.php:89 140.32743726048CRM_Core_BAO_ActionSchedule::processQueue( $now = NULL, $params = ['check_permissions' => TRUE, 'version' => 3, 'rowCount' => 0] ).../Job.php:204 1526.04995321088CRM_Core_BAO_ActionSchedule::buildRecipientContacts( $mappingID = 'saved_search', $now = '20230830052037', $params = ['check_permissions' => TRUE, 'version' => 3, 'rowCount' => 0] ).../ActionSchedule.php:467 1627.99935346048Civi\ActionSchedule\RecipientBuilder->build( ).../ActionSchedule.php:450 1730.37505346048Civi\ActionSchedule\RecipientBuilder->buildRelFirstPass( ).../RecipientBuilder.php:139 1831.42425346048Civi\ActionSchedule\RecipientBuilder->prepareQuery( $phase = 'rel-first' ).../RecipientBuilder.php:162 1931.51625401704Civi\Search\ActionMapping->createQuery( $schedule = class CRM_Core_DAO_ActionSchedule { public $id = '3'; public $name = 'new_one'; public $title = 'new one'; public $recipient = NULL; public $limit_to = NULL; public $entity_value = '23'; public $entity_status = 'id'; public $start_action_offset = '1'; public $start_action_unit = 'day'; public $start_action_condition = 'after'; public $start_action_date = 'receive_date'; public $is_repeat = '0'; public $repetition_frequency_unit = NULL; public $repetition_frequency_interval = NULL; public $end_frequency_unit = NULL; public $end_frequency_interval = NULL; public $end_action = NULL; public $end_date = NULL; public $is_active = '1'; public $recipient_manual = ''; public $recipient_listing = ''; public $body_text = NULL; public $body_html = '<p>sk cash&nbsp;{contribution.financial_type_id:label}</p>\n'; public $sms_body_text = NULL; public $subject = 'yay'; public $record_activity = '1'; public $mapping_id = 'saved_search'; public $group_id = NULL; public $msg_template_id = NULL; public $sms_template_id = NULL; public $absolute_date = NULL; public $from_name = NULL; public $from_email = NULL; public $mode = 'Email'; public $sms_provider_id = NULL; public $used_for = NULL; public $filter_contact_language = NULL; public $communication_language = NULL; public $created_date = '2023-08-30 05:08:48'; public $modified_date = '2023-08-30 05:08:48'; public $effective_start_date = NULL; public $effective_end_date = NULL; protected $resultCopies = 0; protected $_options = []; public $_DB_DataObject_version = '1.11.3'; public $__table = 'civicrm_action_schedule'; public $N = 1; public $_database_dsn = ''; public $_database_dsn_md5 = '320a00603e17f988282908defa13fbd6'; public $_database = 'dmastercivicrm'; public $_query = FALSE; public $_DB_resultid = 64; public $_resultFields = FALSE; public $_link_loaded = FALSE; public $_join = ''; public $_lastError = FALSE }, $phase = 'rel-first', $defaultParams = ['casActionScheduleId' => '3', 'casMappingId' => 'saved_search', 'casMappingEntity' => 'civicrm_contribution', 'casNow' => '20230830052037'] ).../RecipientBuilder.php:283 2031.69875772712Civi\Api4\Query\Api4SelectQuery->getSql( ).../ActionMapping.php:164 2131.76406653912Civi\Api4\Query\Api4SelectQuery->buildWhereClause( ).../Api4Query.php:79 2231.76466654608Civi\Api4\Query\Api4SelectQuery->treeWalkClauses( $clause = [0 => 'is_deceased', 1 => '=', 2 => FALSE], $type = 'WHERE', $depth = ??? ).../Api4SelectQuery.php:225 2331.76466654608Civi\Api4\Query\Api4SelectQuery->composeClause( $clause = [0 => 'is_deceased', 1 => '=', 2 => FALSE], $type = 'WHERE', $depth = 0 ).../Api4Query.php:213 2431.76476654608Civi\Api4\Query\Api4SelectQuery->getExpression( $expr = 'is_deceased', $allowedTypes = [0 => 'SqlField', 1 => 'SqlFunction', 2 => 'SqlEquation'] ).../Api4Query.php:243 2531.76486655112Civi\Api4\Query\Api4SelectQuery->getField( $expr = 'is_deceased', $strict = TRUE ).../Api4Query.php:166

[eileenmcnaughton]

@colemanw perhaps we should also require view all contacts since we fetch the contact with $this->savedSearch['api_params']['checkPermissions'] = FALSE; & that means no ACLs are used

I think without that the Greens / multisites could get in trouble on this. I'd rather be too strict at first & loosen later. There is also a 'view all custom data' I think that the greens do not give all admin users from memory (@seamuslee001 ?)

[seamuslee001]

@eileenmcnaughton I think that might be right but I would have thought that the is_deleted error should have been swallowed somewhere along the way

[eileenmcnaughton]

@seamuslee001 yeah the is_deleted is kinda separate - in that I looked in the code for that reason & then saw the checkPermission FALSE & wondered about the implications of one person configuring & assuming that what they see (their ACL permissions) would be in play but instead someone elses is

[colemanw]

@eileenmcnaughton @JoeMurray I think before going further we should consider the pros and cons of restricting this new permission to only SearchKit communications. The unpermissioned query issue you've identified is endemic to the existing scheduled communications system which this new feature inherits. So for example if you create a (non-SearchKit) scheduled communication for Contacts it will bypass your personal ACLs and go out to all contacts meeting your criteria.
I understand the argument for a new feature hidden behind a new permission, but wonder if the extra work involved justifies it. It would be less dev time to require the same permissions for all scheduled communications, and it would plug the holes that are already there in addition to the new ones.

[eileenmcnaughton]

@seamuslee001 @johntwyman @andrew-cormick-dockery can you weigh in at all?

The issue is

1. we have a new feature that makes it possible to specify the critieria for a scheduled reminder in a search kit (ie gets away from the existing, weird, hard-coded field choices)
2. we have identified that at the point of sending permissions are not applied - ie a person who has ACL restrictions could have inadventently configured a scheduled reminder that will go to people they do not have permissions to see
   3)@colemanw has pointed out this is the status quo
3. we have already decided to require a new permission for search-kit scheduled communications with the thinking we would require it for existing scheduled communication configuration later (giving a bit more testing time before most people start to see the new feature).

I had suggested that users need 'view all contacts' to configure scheduled reminders because any ACLS that they have are overridden when calculating them (new style or old). If we did that I think we would to both legacy and new style

I think @colemanw is suggesting that rather than require that we accept that anyone with 'Schedule Communications' permission can configure them to go to people they don't otherwise have access to - and require this permission for all scheduled reminders. This would probably look like a pre upgrade message saying that people will need to review their permissions and add this new permission to anyone who they want to be able to schedule reminders with some warnings about how it interacts with ACLS. This would probably be pretty clear on the upgrade screen but not that visible to people who might be assigning permisssions in 6 months time

In general people having administer civicrm and not view all contacts is a less common config - but one which is used in multisite & it seems like ideally we would tighten the permissions in this scenario with 1 or both of the above.

Note the current 'permission rollout' also has the effect of 'hiding' the new feature in the initial release - which I kinda liked as it would mean those who want to play can but most site admins would not be aware of it.

[andrew-cormick-dockery]

For us, scheduled reminders are used for two functions: membership and event reminders. We use multisite to constrain people's viewing contacts to Australia's states. But since membership is also state-based, and of course events are generally even more local than that, if scheduled reminders are completely unconstrained in that manner, that doesn't really bother us. I guess the fact that I'm aware of this now might help us avoid problems in the future.

If scheduled reminders are going to be expanded to use Search Kit, from our perspective even if it's not ACL constrained it's not going to bother us too much. This is because we've restricted the use of Search Kit to administrators due to the serious performance issues which could come about through a less restrictive regime.

But it sounds like you're intending to restrict that function to "view all contacts" in any case, which would be ideal. Only administrators have that function on our system.

(For what it's worth, if there's any part of scheduled reminders I'd like to see worked on, it's cleaning up the Manage Schedule Reminder screen! Oh, and if you're working on multiple reminders at a time in multiple tabs, the last one to save doesn't overwrite all the ones you have open!)

[colemanw]

(For what it's worth, if there's any part of scheduled reminders I'd like to see worked on, it's cleaning up the Manage Schedule Reminder screen! Oh, and if you're working on multiple reminders at a time in multiple tabs, the last one to save doesn't overwrite all the ones you have open!)

For what it's worth, those issues have all recently been fixed!

[francescbassas]

As far as I understand, one of the purposes of this issue is to be able to send the results of a SearchKit by email using Scheduled Reminders like CiviReport does.

Since you are discussing ACL, I add another vision that I think would be interesting to contemplate and with which it would not be necessary to think about ACL. Instead of sending the results by email, you can send a customizable message by email with a link to the corresponding SearchKit or Afform of the style: "There are 21 contributions pending to be thanked, please review the report in the following link."

What do you think? Could this functionality be considered in this issue? Or should it be addressed in another one?

[colemanw]

As far as I understand, one of the purposes of this issue is to be able to send the results of a SearchKit by email using Scheduled Reminders like CiviReport does.

@francescbassas sorry no this PR does not do that. This PR allows you to use SearchKit to pick the recipients of the scheduled communication. It does not send search kit reports by email.

[eileenmcnaughton]

OK - moving the new feature stuff to an extension means we can merge it knowing only people who choose to play with it will have the feature - allowing us a bit more time to resolve the permissions question. All the core stuff has good test cover

[colemanw]

@eileenmcnaughton I pushed up a bit of error checking to ensure it would delete the reminder when deleting the saved-search, and to make sure it doesn't crash if you disable the extension.