(WIP) CRM-18538 check schedule reminder email privacy settings

[lcdservices]

• CRM-18538: scheduled reminder emails do not respect do not email settings

[monishdeb]

Or can we extend getPrimaryEmail() to allow second parameter say CRM_Contact_BAO_Contact::getPrimaryEmail($toContactID, $filter = array()); for reusability and in this case
$filter = array(is_opt_out => 1, is_deceased => 1, on_hold => 1); to add criteria !!

[JoeMurray]

It's almost always better for performance to amalgamate two db queries, like here where there is one for checkPrivacy one and another to retrieve the primary email. Wouldn't it be okay to do something like this instead:

$result = civicrm_api3('Contact', 'get', array(
'sequential' => 1,
'return' => array("email"),
'do_not_email' => 0,
'is_opt_out' => 0,
'is_deceased' => 0,
'on_hold' => 0,
'id' => $toContactID,
'email' => array('IS NOT NULL' => 1),
));

[monishdeb]

Jenkin, test this please

[JoeMurray]

Hi Brian, as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR? I'm also hoping you'll be able to help on the Accrual accounting stuff.

[JoeMurray]

@kenwest would you be interested in reviewing this PR?

[kenwest]

Sure. Can I assume Monish's comment is not in scope?

[JoeMurray]

Yes @kenwest I think @monishdeb's comment is out of scope for now.

Could we grep the code base to identify places that would be impacted by the change in functionality to provide a sense of the risk? If there are some, one fallback would be a simpler version of Monish's proposal, just an True/False arg rather than an array.

@monishdeb could you explain the rationale for your suggestion more? I think it may be useful to be able to retrieve emails in certain contexts in codebase, perhaps trying to sync with other systems based on email. But I think it is gold-plating to develop filtering based on various criteria without a good use case requiring it.

@lcdservices what is your sense on this?

[lcdservices]

I'm not opposed to the filtering. But agree that there's no pressing need to add the option unless we know we need to be able to call this method with and without the filter.

[JoeMurray]

This will need user documentation. @eileenmcnaughton what's your sense on this one? It's changing long-standing practice, if I understand correctly.

[monishdeb]

My suggestion was to support filter by improving the existing CRM_Contact_BAO_Contact::getPrimaryEmail($toContactID, $filter = array()); allowing a second parameter $filter via which we can provide filter(s) of our own choice. Like here

protected static function sendReminderEmail($tokenRow, $schedule, $toContactID) {
 +        $filter = array(is_opt_out => 1, is_deceased => 1, on_hold => 1);
 +        $toEmail = CRM_Contact_BAO_Contact::getPrimaryEmail($toContactID, $filter);
...

Because why calling a separate query where we can already use the query inside getPrimaryEmail() definition

But overall the patch is working fine and I agree user documentation is needed if merged

[eileenmcnaughton]

(NB the main argument for Monish's approach in my mind is that we discussed queries at the sprint & it was agreed sql in code should be discouraged - if for no other reason than because it's too easy to let it become insecure)

[jitendrapurohit]

@eileenmcnaughton Can we close this as it hasn't progressed from a long time? Also, one can always re-open this when anyone feels to work on it.

[eileenmcnaughton]

OK - let's close it & get it re-opened when work commences again on it - there is a change to existing functionality & my feeling, having discussed this today with customers, is it should be configurable which privacy settings to over-ride. I might have an opportunity later this year to work on it