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

Feature - Send Test Mail #8643

Merged
merged 7 commits into from
Jan 20, 2016
Merged

Feature - Send Test Mail #8643

merged 7 commits into from
Jan 20, 2016

Conversation

Kubik-Rubik
Copy link
Member

This PR adds a new button ("Send Test Mail") in the Global Configuration with which the email settings can be tested with one click!

After the click on the button, the email data is transferred via an Ajax request to the server and a test mail is created and sent to the specified email address. If the sending process fails, a message with the error is displayed (see screenshots).

You don't have to save the configuration first, because the sending process will always be executed with the current entered values. Once the email settings are right, you should save them with a click on the "Save" button.

How to test

Install patch, go to the Global Configuration and click on the button! :-)
Change some settings (e.g. enter wrong values) and try it again.

Important: You should also test on an online server from which you really can send emails to receive the test mail. For instance, on local environments the "PHP Mail" option does not throw an error even though the email was not sent (if not configured properly)!

bildschirmfoto 2015-12-10 um 10 16 56

bildschirmfoto 2015-12-10 um 14 56 33

bildschirmfoto 2015-12-10 um 14 57 10

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Dec 10, 2015
@Kubik-Rubik Kubik-Rubik added this to the Joomla! 3.6.0 milestone Dec 10, 2015
COM_CONFIG_SENDMAIL_BODY="This is a test mail. If you receive it, then your email settings are correct!"
COM_CONFIG_SENDMAIL_ERROR="Test mail could not be sent."
COM_CONFIG_SENDMAIL_SUBJECT="Test mail from %s"
COM_CONFIG_SENDMAIL_SUCCESS="Mail sending process executed properly. Additionally, you should check whether you've received the test mail!"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be simplified into more basic english? Perhaps
"The email was sent successfully. You should check that you've received the test email."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've expected a correction from you! :-P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! :-)

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on bf8e351

Tested in localhost with phpmail, sendmail and smtp mail - all worked

I think its a little confusing (or not documented) that the test email is sent to the email address defined in the From email field


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@Kubik-Rubik
Copy link
Member Author

@brianteeman Thank you for testing it. Yes, this should be documented. Maybe directly in the success message? If yes, could you please send me the correct wording?

@brianteeman
Copy link
Contributor

how about
+COM_CONFIG_SENDMAIL_SUCCESS="The email was sent successfully to %email. You should check that you've received the test email."

@brianteeman
Copy link
Contributor

Or even better
+COM_CONFIG_SENDMAIL_SUCCESS="The email was sent successfully to %email using %method. You should check that you've received the test email."

@Kubik-Rubik
Copy link
Member Author

@brianteeman Good idea!

@brianteeman
Copy link
Contributor

Might be worth changing the email body message as well to show the settings
that were used to send the email

Perhaps
+COM_CONFIG_SENDMAIL_BODY="This is a test mail sent using %method. If you
receive it, then your email settings are correct!"

And even better if it could include all the options for that method

Otherwise when testing and receiving multiple emails you dont know which
method was successfully receieved

On 10 December 2015 at 14:37, Viktor Vogel notifications@github.com wrote:

@brianteeman https://github.com/brianteeman Good idea!


Reply to this email directly or view it on GitHub
#8643 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@Kubik-Rubik
Copy link
Member Author

@brianteeman PR updated with your suggestions. Could you please re-test? Thank you!

@brianteeman
Copy link
Contributor

Completely stopped working now
I have a button but thats all - pressing it doesnt give a success OR a fail
message

On 10 December 2015 at 16:15, Viktor Vogel notifications@github.com wrote:

@brianteeman https://github.com/brianteeman PR updated with your
suggestions. Could you please re-test? Thank you!


Reply to this email directly or view it on GitHub
#8643 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@brianteeman
Copy link
Contributor

Correction - I received the emails with the updated message so it was just in the Joomla admin that I was not getting any messages


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@Kubik-Rubik
Copy link
Member Author

@brianteeman I've changed the JavaScript file, so you have first to clear your browser cache.

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 43ba153

I just looked at the code and saw it was using some js - so my last problem was just a JS cache issue
All working well now.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@Kubik-Rubik
Copy link
Member Author

Updated screenshot:

bildschirmfoto 2015-12-10 um 17 29 23

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@pe7er
Copy link
Contributor

pe7er commented Dec 10, 2015

I have tested this item ✅ successfully on a4000f2

Works good!


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@alikon
Copy link
Contributor

alikon commented Dec 10, 2015

tested successfully


$model = new ConfigModelApplication;
$response = new JResponseJson($model->sendTestMail());
echo $response->__toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noooope. __toString is a magic PHP method. echo (string) $response is what you need

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't even need the (string) typecast. echo will automagically convert it. Take a look at https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_languages/controllers/strings.json.php and see how nice and simple it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, misunderstood it... :-) Thanks guys!

$conf->set('mailonline', $input->get('mailonline'));

// Prepare email and send try to send it
$mail_subject = JText::sprintf('COM_CONFIG_SENDMAIL_SUBJECT', $conf->get('sitename'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Camelcase variable as per our code style guidelines please

@RickR2H
Copy link
Member

RickR2H commented Dec 10, 2015

Switching only to SMTP and not filling any SMTP settings activates the phpmail fallback on clicking the button. The message states "The email was sent successfully to rick@r2h.nl using SMTP. You should check that you've received the test email." But it should give a message that not all SMTP settings are filled ans mail cannot be sent. Also filling all the fields with false date ativates the fallback to phpmail and ther is also no error message.

@brianteeman
Copy link
Contributor

But it should give a message that not all SMTP settings are filled ans mail cannot be sent. Also filling all the fields with false date ativates the fallback to phpmail and ther is also no error message.

When I put in invalid data I got a failed message

rnhf

@joomla-cms-bot
Copy link

This PR has received new commits.

CC: @brianteeman, @pe7er


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@Gerlof
Copy link

Gerlof commented Dec 11, 2015

I have tested this item ✅ successfully on bf5ba5e


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

1 similar comment
@dwarkesh0204
Copy link

I have tested this item ✅ successfully on bf5ba5e


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@Fedik
Copy link
Member

Fedik commented Dec 12, 2015

Beside that the thing is working, I have the note to 😄

In suggested implementation sendTestMail() totally ignore the form application.xml, this could leed to "false" result (after changes in the form in the future), example sendTestMail say OK but after saving the form validation say FAIL

So my suggestion is to use "common routine" (form filtering and validation) instead of fancy stuff with $app->input in sendTestMail()

Take save.php as example, in Sendtestmail controller, do something like:

$form = $model->getForm();
$validatedData = $model->validate($form, $data);
$result = $model->sendTestMail($validatedData);

To make the form validate only Email params, in the model loadForm need to use $xpath for fieldset[@name="mail"]. So maybe can be need one more method for getEmailForm here or something.

Just suggestion 😄

@brianteeman
Copy link
Contributor

Setting RTC- would be good to get this in 3.5 and not have to wait a year for 3.6 - there are no b/c issues


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8643.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 20, 2016
@wilsonge wilsonge modified the milestones: Joomla! 3.5.0, Joomla! 3.6.0 Jan 20, 2016
wilsonge added a commit that referenced this pull request Jan 20, 2016
@wilsonge wilsonge merged commit 5d4a0ae into joomla:staging Jan 20, 2016
@Kubik-Rubik Kubik-Rubik deleted the feature-sendtestmail branch May 10, 2016 09:04
@brianteeman brianteeman removed the RTC This Pull Request is Ready To Commit label May 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.