dev/core/#133 Make Conditions Stricter When Checking If Reply-To Field Is Empty

[vinuvarshith]

Overview

reply-to field is NULL in the following case

• Add a new mailing (or reuse one)
• Select a 'reply to' email (I had to enable "Enable Custom Reply-To" in 'civicrm/admin/mail')
• Fill in other fields as usual
• Under 'Responses' tab, check "Track Replies" checkbox. This brings up a warning message on the top right saying 'reply-to' has been cleared out (but still allows user to save mailing)
• Proceed to next step and save mailing

Technical Details

In CRM_Mailing_BAO_Mailing.php when adding a new mailing (add() method), there is a condition to check if 'reply-to' field is set and if it is not set, then 'from-email' is used.
here instead if using
if(!isset($params['replyto_email']))
we should use
if(empty($params['replyto_email']))
so that empty string like '' will also be considered and reply-to could not be NULL

[civicrm-builder]

Can one of the admins verify this patch?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

I think this is pretty compelling. My feeling is we should probably also only set it if id is empty though as we don't want to overwrite the reply_to if the subject is being updated by the api. I was going to merge on the basis this would be 'no worse' on that front but I'm not sure actually - it might expose more pathways to that 'bug'

[seamuslee001]

@eileenmcnaughton well I think this probably safe to merge on its own, we also need to take into account the situation say where you have updated the from email address so now the reply to also needs to be updated as well.

[eileenmcnaughton]

@seamuslee001 how do we differentiate that though?

It seems like setting the reply address from the from address would be always correct when there is no id & sometimes correct when there is an id and it's hard to know when, unless the calling function handles it. This change increases the cases in which the BAO would make a guess about what the calling function really intended.

Perhaps we need to handle this defaulting at the form level not the BAO ?

[seamuslee001]

@eileenmcnaughton well its hard to tell i tend to think that empty is better than isset here in general, Interms of setting the reply to base on the from that isn't always the case either sometimes people will specifically set a separate reply to address compared to the from, I think form level maybe more sensible of course form level here = angular not Quickform / smarty

[eileenmcnaughton]

@seamuslee001 I agree empty is better but changing at the BAO level has a risk of unintended consequences (although I think that risk is only on updates).

[vinuvarshith]

@eileenmcnaughton
I thought empty was better than isset in this case. But what do you suggest then to keep users from saving a mailing with empty value for 'reply to' field? Pls let me know and I will try to address that.
Thanks

[eileenmcnaughton]

I would be ok with

if ((!$id && empty($params['replyto_email'])) || !isset($params['replyto_email'])

which would retain the existing behaviour on update but would be more aggressive on new.

If we want to be more aggressive on update I think we should do that at the form layer

[vinuvarshith]

@eileenmcnaughton
I have updated the condition as you have suggested. So existing behaviour is retained on update.

[eileenmcnaughton]

great - merging. For updates we should fix at the form level