Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 28, 2024

Conversation

lwcorp
Copy link
Contributor

@lwcorp lwcorp commented Mar 5, 2024

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):

image

1. Appended a test subject indicator to test messages
1. Added a reply-to address to test messages that have no manual reply-to: using the logged in admin's address or at least the general admin's
@bramley
Copy link
Contributor

bramley commented Mar 8, 2024

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.

Rephrased variable name
Switched to using $admin_auth
@lwcorp
Copy link
Contributor Author

lwcorp commented Mar 8, 2024

'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...
image
image

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
Copy link
Contributor

bramley commented Mar 8, 2024

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
Copy link
Member

michield commented Mar 9, 2024

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
Copy link
Member

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

@bramley
Copy link
Contributor

bramley commented Mar 24, 2024

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
Copy link
Member

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.

@marianaballa marianaballa changed the base branch from main to release-3.6.15 March 27, 2024 15:37
@marianaballa marianaballa merged commit 61e952c into phpList:release-3.6.15 Mar 28, 2024
4 of 6 checks passed
@lwcorp lwcorp deleted the sending branch March 28, 2024 17:53
@phpListDockerBot
Copy link
Contributor

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
Copy link
Contributor

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

marianaballa added a commit that referenced this pull request Apr 26, 2024
* Translations for 3.6.15  (#1032)

* Translated using Weblate (English)

Currently translated at 91.4% (1950 of 2132 strings)

Translation: phpList/phpList3
Translate-URL: http://translate.phplist.org/projects/phplist/phplist3/en/

* Translated using Weblate (French)

Currently translated at 99.8% (2128 of 2132 strings)

Translation: phpList/phpList3
Translate-URL: http://translate.phplist.org/projects/phplist/phplist3/fr/

---------

Co-authored-by: Duncan Cameron <[email protected]>
Co-authored-by: Alain Rihs <[email protected]>

* Support for indicating and getting feedback for e-mail test messages (#1031)

* Update sendemaillib.php

1. Appended a test subject indicator to test messages
1. Added a reply-to address to test messages that have no manual reply-to: using the logged in admin's address or at least the general admin's

* Update sendemaillib.php

Rephrased variable name

* Update sendemaillib.php

Switched to using $admin_auth

* Allowing subscribers to be filtered by confirmed and/or blacklisted (#1030)

* Update users.php

Allowed to filter by confirmed and/or non blacklisted - and not just by unconfirmed and/or blacklisted

* Changed users to subscribers

* Bouncemgt - allowing processing only existing bounces + a related new rule action (#1028)

* Update bouncemgt.php

Added &justexisting=true

* Update processbounces.php

1. Added support for &justexisting=true
1. Added support for new bounce rule action

* Update lib.php

Added support for new bounce action

* Update bouncemgt.php

Added non default title (otherwise it takes the wrong one)

* Update processbounces.php

1. Replaced goto with if-else
1. Hardcoded "-1" instead of supplying it in a sprintf value

* Hardcoding defaults for older PHP versions

* Removed modern solution

* Update Common plugin and Segment plugin (#1024)

* Define timestamp columns explicitly (#1019)

* Define timestamp fields explicitly to avoid problem with the mysql setting explicit_defaults_for_timestamp

* Remove setting of timestamp fields that are automatically updated

* update CI to remove old PHP versions and add 8.3 (#1004)

* Escape single quote in error message (#1003)

* Allow ajax page links to have a title, defaulting to the link description (#1002)

Fixes #996

* Update CONTRIBUTING.md (#994)

Removed obsolete references

* update UUID class to the latest upstream (#990)

* update UUID class to the latest upstream

* clean up old files

* use the list order, even when grouping by category (#1025)

* restore ability to create other super users (#1014)

* restore ability to create other super users

* correctly initialise the privileges array

* Bounces' subscriber' status indicator + allowing to confirm right from bounces (#1029)

* Update listbounces.php

Added support for confirmed/blacklisted indicator

* Update bounces.php

Added confirmed/blacklisted indicator

* Update bounce.php

1. Added confirmed/blacklisted indicator
1. Added support for confirming user from a bounce

* Update bounce.php

1. Avoided ternary if because translation system doesn't support it
1. Used the newer s() function

* Update listbounces.php

Added curly brackets

* Used potential translation

* Php8fixes 202401 (#1026)

* remove deprecated ini_set call

* stop possible warning

* avoid warning

* avoid warning

* cast to int

* avoid warning on existing being null

* force template to be an integer

* suppress warnings

* check on valid var and cast to int

* give buttons an ID, so they can be targetted with testing

* avoid warning on empty array index

* add notification by email when an admin logs in from a new IP address. (#1027)

* add notification by email when an admin logs in from a new IP address.

* check IP per admin

* force columns to be not null

* prevent blocking login on an non-upgraded system and send login alert just to admin, or superuser

* keep newlines in translation as they are

* make shorter lines, so it renders a bit better

* Remove redundant upgrade steps (#1020)

* Remove steps that are unnecessary due to the 3.2.0 being the minimum upgrade version

* Keep silent when there are no subscriber UUIDs to generate

* Remove other unnecessary upgrade steps

---------

Co-authored-by: Michiel Dethmers <[email protected]>

* Use utf8mb4 for the connection etc (#1001)

* Use utf8mb4 for the connection etc

* Support utf8mb4 in campaign subject and content

---------

Co-authored-by: Michiel Dethmers <[email protected]>

* use PHP8.2 to build

* use latest phplint

* update docker build from bookworm

* set version

* avoid the admin being kicked out after upgrade (#1033)

* mark update translations as @wip

---------

Co-authored-by: Duncan Cameron <[email protected]>
Co-authored-by: Alain Rihs <[email protected]>
Co-authored-by: lwcorp <[email protected]>
Co-authored-by: Duncan Cameron <[email protected]>
Co-authored-by: Michiel Dethmers <[email protected]>
@petersphilo
Copy link

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
Copy link
Contributor Author

lwcorp commented Apr 29, 2024

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
Copy link

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
Copy link
Contributor Author

lwcorp commented Apr 30, 2024

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):
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants