CRM-20845: create alterMailingRecipients hook

[monishdeb]

• CRM-20845: create alterMailingRecipients hook

[monishdeb]

Submitted documentation as PR civicrm/civicrm-dev-docs#221

[JoeMurray]

Code looks nice.

[totten]

If you're using Mosaico/FlexMailer, does this new hook still have the expected effect?

[monishdeb]

@totten I think the new hook has an effect because via that hook we can build and select mail-recipients by manipulating filters. Later stored in mail_recipients table for delivery and delivered by Flexmailer as per the code

 $flexMailer = new \Civi\FlexMailer\FlexMailer(array(
   'mailing' => \CRM_Mailing_BAO_Mailing::findById($job->mailing_id),
   ...
 ));
 return $flexMailer->run(); 

[monishdeb]

ping @totten @eileenmcnaughton @lcdservices

[lcdservices]

I've tested this hook with multiple customizations and it's working as intended.

[monishdeb]

@lcdservices after the refactoring changes done to CRM_Mailing_BAO_Mailing::getRecipients($mailingID) which now accepts only mailing id parameter, I need to remove two parameters from alterMailingRecipients hook. Also, this same hook can be now used to modify filter criteria of SMS recipients. In addition, I have added a UT for this new hook. Please have a look.

[lcdservices]

@Monish I reviewed the changes and they look fine. We already had changed the hook to have only 3 params in a previous round of edits on this. Would be great if we can get this merged.

[monishdeb]

Thanks @lcdservices , also I have updated the doc PR civicrm/civicrm-dev-docs#221

@eileenmcnaughton @mlutfy @colemanw can you please review and merge this PR?

[colemanw]

I am reviewing this one today. @totten any thoughts about Monish's comment about Flexmailer compatibility?

[totten]

I have mixed feelings here -- mostly over what makes a good contract for this. First consideration: do SQL expressions represent a supportable/maintainable contract?

On one hand, you might say "no" because there's many possible symbols in SQL. I like how $params['filters'] gives a name to each condition; the list of names is short and concrete. If you want to remove a criterion, you can do that without understanding the particular SQL involved. But there's really not much you can do with that.

On the other hand, this hook doesn't really hide the SQL -- if you want to add a new condition or change a condition, then you'll have to understand the list of tables/aliases/fields in play. And if you're going to have that responsibility, then we should probably go all the way: pass a CRM_Utils_SQL_Select object to give access to the full list of params/joins/groupings/subqueries.

As an alternative, consider changing the list of SQL strings ($params['filters']) to a list of SQL objects -- i.e. totten@6995c3e (with totten@3ed9641). This allows the custom filter to benefit from escaping data, joining, etc.

<?php
// Ex: Remove a criterion by name
unset($criteria['is_deceased']);

// Ex: Add a filter based on subtype. Use some data provided by the site admin (with string-escaping).
$criteria['my_extra_criterion'] = CRM_Utils_SQL_Select::fragment()
  ->where('civicrm_contact.contact_sub_type IN (@subtypes)')
  ->param('subtypes', Civi::settings()->get('myextension_mailable_subtypes'));

// Ex: Add a filter based on Drupal role. Use some data provided by the admin (with numeric-validation).
$criteria['my_extra_criterion'] = CRM_Utils_SQL_Select::fragment()
  ->join('civicrm_uf_match', 'INNER JOIN civicrm_uf_match WHERE civicrm_uf_match.contact_id = civicrm_contact.id')
  ->join('users_roles', 'INNER JOIN users_roles ON civicrm_uf_match.uf_id = users_roles.uid AND rid IN (#allowedRoles)')
  ->param('allowedRoles', Civi::settings()->get('myextension_drupal_role_ids'));

Of course, you need to know/assume some things about SQL query. (Ex: You assume that the civicrm_contact record is named civicrm_contact and not contact_a or contact_b or recipient.) But that was also true when passing SQL strings.

[totten]

Continuing thoughts on contract, the second question: What are you supposed to do in post?

Is the idea that one would directly INSERT/DELETE records from civicrm_mailing_recipients?

[totten]

As a consumer, I guess I can see how that's kind'a nice -- i.e. you don't have think at all about how the main query-building works; you can just do your own thing.

OTOH, recall this: the CiviMail UI shows a preview of the #recipients, and it actually executes getRecipients() but performs a ROLLBACK. This was sort of a quick hack that people have periodically complained about, but it was easier than rewriting getRecipients(). Can we ever get rid of that hack?

As of today, the answer is, "Yes, if we spend a while refactoring."

If we change the contract so that hook_civicrm_alterMailingRecipients relies on direct manipulation of the civicrm_mailing_recipients table, then the answer is "No, we can't cleanup the weird ROLLBACK without breaking the contract."

[lcdservices]

@totten a few things, as we are already using this for NYSS in production and have several use cases:

1. the advantage of the filters array is that it's simple and targets the key elements that could be manipulated, removed, or added to. true -- it doesn't give you access to the full SQL. but the build recipients function calls a number of queries, all of which are rather complex. this hook let's us impact the common variables across all of those queries very easily.
2. pre/post is really important. for example, for NYSS all we do in the pre step is unset the on_hold filter. we then do a ton of post processing in the post step (including reimplementing on_hold; we don't want those records excluded as we need to manipulate them and do on_hold removal as a final step)
3. the hook doesn't rely on direct manipulation of the civicrm_mailing_recipients table, but we are able to directly manipulate that table as a result of the hook.
4. there's other places where we allow direct SQL manipulation as part of the hook contract. e.g. alterReportVar. I understand why it's worth asking the question as to whether that's what we want to do -- but I think there are places where it's legitimate and useful.
5. @monishdeb has worked on another PR (@monishdeb, can you track it down?) that refactors the buildRecipients function considerably, as well as makes modifications to the angular interface, so that we no longer do rollbacks. we are already using that in NYSS production. would be great if you could review and we get those merged.

for reference, you can see our implementation of this hook here:
https://github.com/nysenate/Bluebird-CRM/blob/dev/civicrm/custom/ext/gov.nysenate.mail/mail.php#L184

as you can see, we have six different steps we walk through when constructing the recipient list after the core function has handled the base construction.

[monishdeb]

@totten w.r.t to the 5th point made by @lcdservices

@monishdeb has worked on another PR (@monishdeb, can you track it down?) that refactors the buildRecipients function considerably, as well as makes modifications to the angular interface, so that we no longer do rollbacks. we are already using that in NYSS production. would be great if you could review and we get those merged.

This is the PR https://github.com/civicrm/civicrm-core/pull/11142/files which contain the refactoring changes made earlier, where rollbacks are avoided and now buildRecipients fn only depends on $mailingID to populate recipients. But there is a limitation as we cannot use a single query that can be used to fetch recipients from regular and/or smart groups and previous mailing. But then there was a scope to manipulate the queries as these queries have one thing in common - filters & order by clause. This new hook allows us to modify these filters and order by so we fetch recipients of our choice e.g.

1. Fetch multiple emails NOT just primary email of a contact
2. Consider deceased contact too as recipients
   .. so on

OTOH, recall this: the CiviMail UI shows a preview of the #recipients, and it actually executes getRecipients() but performs a ROLLBACK. This was sort of a quick hack that people have periodically complained about, but it was easier than rewriting getRecipients(). Can we ever get rid of that hack?

Nope it doesn't rollback now, see here

[totten]

OTOH, recall this: the CiviMail UI shows a preview of the #recipients, and it actually executes getRecipients() but performs a ROLLBACK. This was sort of a quick hack that people have periodically complained about, but it was easier than rewriting getRecipients(). Can we ever get rid of that hack?

Nope it doesn't rollback now, see here

OK, good point. The process now actually commits the full list while editing the mailing - so "Previewing" the recipient just means "show the currently save list of recipients".

the hook doesn't rely on direct manipulation of the civicrm_mailing_recipients table, but we are able to directly manipulate that table as a result of the hook.

Six/half-dozen -- based on the example and based on the design of the post variant, the hook can't be used for its primary purpose unless you manipulate civicrm_mailing_recipients table. And I see some elegance in that. It should be communicated, though, as part of the docs/examples.

[monishdeb]

Thanks @totten for suggesting your patch which I have now included in my PR and also updated the unit-test to work with the new changes. Updated the doc PR civicrm/civicrm-dev-docs#221 and I think the example will be enough, telling about how to use SQL fragments to override/change the filters.

@lcdservices in this new change we will now use SQL fragments which is flexible enough to consider subqueries too. I have added an example in UT which filters contact that is deceased and has a tag - "Tagged". Please have a look.

[monishdeb]

Jenkins test this please

[eileenmcnaughton]

@totten your suggestions appear to have been acted on & this is requiring your re-review

[eileenmcnaughton]

@totten per above - I think this is waiting on you after your feedback was responded to

[eileenmcnaughton]

@totten ^^

[eileenmcnaughton]

@monishdeb you might rebase this - although that shouldn't prevent @totten from confirming status

[monishdeb]

@eileenmcnaughton rebased it and the extended test passed.

[eileenmcnaughton]

OK - pending @totten

[eileenmcnaughton]

@totten can you please clarify why you still have reservations about this - it's obviously in your too hard basket - I'm happy to close it if it's really not going anywhere (given it's been almost a year)

[eileenmcnaughton]

I discussed this with @seamuslee001 & we agreed not to hold this up further on efforts to get @totten to respond. In summary the PR has been fairly thoroughly tested by @lcdservices, unit tests have been added & code feedback responded to - merging

[monishdeb]

Thanks @eileenmcnaughton

[lcdservices]

thanks @eileenmcnaughton