Support for indicating and getting feedback for e-mail test messages

[lwcorp]

Description

1. Appended a test subject indicator to test messages (based on the existing translation of the word test)
2. Added a reply-to address to test messages with this logic:
   1. Left the current approach of using a manual reply-to address if one was supplied
   2. But if one wasn't supplied, take the currently logged-in admin's address
   3. If it's empty, take the general admin's address

Screenshots (if appropriate):

[bramley]

I think this is a reasonable idea but it needs to be controlled by configuration settings so that it is optional. Something like this will allow the prefix to be specified instead of being fixed

'test_email_prefix' => [
    'value'        => '',
    'description'  => s('Prefix for the subject of a test email'),
    'type'         => 'text',
    'allowempty'   => true,
    'category'     => 'campaign',
],
'test_email_reply_to' => [
    'value'        => false,
    'description'  => s("Use the admin's email address as Reply-To of a test email"),
    'type'         => 'boolean',
    'allowempty'   => true,
    'category'     => 'campaign',
],

The global variable $admin_auth should be used to get the email address and login name of the current admin because a plugin can provide this functionality instead of core phplist

$adminEmail = $admin_auth->adminEmail($_SESSION['logindetails']['id']);
$adminName = $admin_auth->adminName($_SESSION['logindetails']['id']);

but I am not sure that it should be making the admin's login id public in this way. Maybe not bother with a name.
The email address is a mandatory field when creating an admin, so there is no need to have a fall-back of getConfig('admin_address')

Also, I think that the test Reply-To should be used in preference to a Reply-To that has already been set for the campaign.

The variable $testReplyFrom is misnamed, it contains a Reply-To address.

[lwcorp]

'value' => '',
'value' => false,

What is the purpose of value (which is either blank or false) there? In any case, so many settings seem a bit like an overkill to me, but would you like to add them to this commit? Because I'm not sure where, maybe config.php?

but I am not sure that it should be making the admin's login id public in this way. Maybe not bother with a name.

I assume you meant "should be making the admin's name public" and not id. If so, I don't agree, as the more you use names, the less likely you are to get flagged as a spammer. Besides, it's only for internal testing, meaning it's the admin him/herself that interacts with the tester, so why hiding their name from the tester (who, let's face it, is probably the admin him/herself...)?

The global variable $admin_auth should be used to get the email address and login name of the current admin because a plugin can provide this functionality instead of core phplist

No problem, I've just switched to it, please review.

The email address is a mandatory field when creating an admin, so there is no need to have a fall-back of getConfig('admin_address')

I don't agree as in my case it's empty...

Also, I think that the test Reply-To should be used in preference to a Reply-To that has already been set for the campaign.

Do you mean it should override the campaign reply-to? I believe the purpose of testing should also be to play with different reply-to addresses just like it's possible to play with different From addresses.

The variable $testReplyFrom is misnamed, it contains a Reply-To address.

No problem, I've just renamed, please review.

[bramley]

I was suggesting adding fields to the Settings page, the code snippet is an example of adding two fields in this case to the Campaign group. That code is in the defaultconfig.php file, there is a long array of settings in the variable $default_config

The value field is the default value, so an empty prefix and false for using the admin's email address for Reply-to.

That way the new functionality is optional, e.g.

    $testPrefix = getConfig('test_email_prefix');
    if ($testPrefix != '') {
        // add the prefix to the campaign subject

The admin name is the logon name, my doubt was whether it is a good idea making that public. My logon name is deliberately not immediately guessable and would not want to make that public even to people who I generally trust. If we want to show a real name, "John Smith" say, then that could be provided by using an admin attribute.

You are right about the admin email address allowed to be empty, I had noticed that after my previous comment. But that is an error in amending an admin which allows the email address to be changed to be empty. Whereas it is mandatory when adding an admin. I will submit a pull request to make the validation consistent between adding and amending an admin.

The benefit I saw in having a reply-to of the admin's email address is when replies would usually go to a different address, either a Reply-to or the From address, to which the admin would not have access. That way reviewers could simply reply with their review comments instead of having to remember to use the admin's own email address. For this scenario to work, the admin address needs to override the campaign reply-to when sending a test message.

Probably best to wait for Michiel to comment before making any more changes as he may have different suggestions.

[michield]

I've reviewed it and ran it locally, and I think it's nice. This is a nice starting point for some kind of approval work-flow. For the time being slightly manually, but maybe at some point with an "approve/disapprove" link.

I'm not sure it needs to be configurable in the UI. We can add some optional settings in the config file and default to the current method. If you make "reply-to email" configurable, then what would you make it apart from the current admin? I guess it could go to a list or something, but that gets very messy. It makes sense to reply to the admin who sent the test email.

[michield]

I've marked it ready to merge into the next release. @bramley if you disagree, please let us know

[bramley]

m not sure it needs to be configurable in the UI. We can add some optional settings in the config file and default to the current method. If you make "reply-to email" configurable, then what would you make it apart from the current admin?

What did you mean by this? I don't see any further change to use a config file setting. I envisaged a setting for the prefix with an empty value meaning don't add a prefix, and a boolean setting for the reply-to address with true meaning change the reply-to address to be that of the current admin.

[michield]

m not sure it needs to be configurable in the UI. We can add some optional settings in the config file and default to the current method. If you make "reply-to email" configurable, then what would you make it apart from the current admin?

What did you mean by this? I don't see any further change to use a config file setting. I envisaged a setting for the prefix with an empty value meaning don't add a prefix, and a boolean setting for the reply-to address with true meaning change the reply-to address to be that of the current admin.

So, you mean you'd be able to avoid this new functionality by setting everything to "off"? I actually think it's quite useful to make this standard functionality, that when you check the "this is a test campaign" it adds the "(test)" to the subject. This can also be translated.

Then we can later expand on that with some approval workflow.

[phpListDockerBot]

This pull request has been mentioned on phpList Discuss. There might be relevant details there:

https://discuss.phplist.org/t/3-6-15-release-candidate-is-available-for-testing/9473/1

[phpListDockerBot]

This pull request has been mentioned on phpList Discuss. There might be relevant details there:

https://discuss.phplist.org/t/phplist-3-6-15-has-been-released/9495/1

[petersphilo]

Thank you all for your continued work on this wonderful product!
it would be nice, in the future, to be able disable this new feature (both the 'test' prepend and the email address switch) via either UI or a config setting..
Huge thanks for the ability to handle emojis in the subject!!

[lwcorp]

Thank you all for your continued work on this wonderful product!

Thanks!

it would be nice, in the future, to be able disable this new feature (both the 'test' prepend and the email address switch) via either UI or a config setting.

Since the official version was out days ago (after the RC was open for debate for weeks) plus it was already discussed here without a clear conclusion, may I suggest you open a new issue about it? You can mention #1031 (this PR) there.

[DDougster]

Just getting ready to send our first live and my admin had a heart-attack when test showed up in the subject. I assume this is only happening when you are sending the test message prior to it going out. Was there any warning put near the test buttons reminding people of this change?

Thanks,
Doug

[lwcorp]

It's called test, of course it's only for tests.
If you want it in the blue line, you can submit an issue and mention #1031 (this PR):