Add email test form

I've tested this extensively and it works really well. I hope it can be included.

Also, this is just a useful mechanism for testing if email functionality is working from a site at all!

Thanks for sharing!

Rgds,
John

Hi ceng,

Thanks a lot for your feedback and comments on this, it is certainly highly appreciated.

I have already taken a quick look before, at this issue, and patch, but didn't have enough time to write a proper answer.

Personally, I wouldn't be completely convinced this additional testing form would be absolutely required:

For testing, wouldn't the Core Contact module, the User module (create manually a fake/test user and notify of account creation), or a simple Rules (to send an email to a fixed email address when content is viewed or updated - should take no more than 2 minutes to put this rule together: on view content, send email to fixed address), already allow testing this module and more specifically email rerouting?

I would suggest to keep this ticket open a bit longer, since I would like to hear what others would have to say about this feature request, so I have changed the status back to needs review (I would like to get more reviews on this).
We will certainly remember your log feedback, but coding or functions is not really a problem here and looking at the patch, I would assume it should work well.

The problem is more about whether this would really be needed in this module or not.
Currently, I am personally leaning towards marking this as closed (won't fix), since I would think it would be superfluous and would not really add anything major/important/critical or that couldn't be done with other ways in less than 2 minutes.

In any case, if we were to really consider this feature request as acceptable and have it rolled in this module, it would at least have to be broken down into a different module, for example reroute_email_test, that would contain all the code for this form and depend on reroute_email.

Would anyone else be interested in seeing this feature added to the module?
Given the suggested testing methods for reroute_email, that I described above, could you please let us know why you would think this feature would be important to add to this module?

So we haven't completely said No to this feature request yet... but it's likely to happen if not enough interest is shown from the community.

Feel free to let me know if you would have any questions, comments, issues, concerns, suggestions, recommendations or objections on my comment or this feature request in general, I would be glad to provide more information or explain in further details.

Any further comments, feedback, testing, reviewing and reporting, would be highly appreciated.
Your opinion matters on this feature request.
Thanks again for the work on this patch, new feature, to all in advance for your support, comments, replies and feedback.
Cheers!

I did wonder about this not belonging in this module, but reckoned it was easier putting it here rather than elsewhere and it does no harm being here.

It almost needs to be in core somewhere or provided by it's own module - but then it is a development tool so wrapping it with a development module like reroute_email I think is fine.

As a developer, sending emails from a dev site to real people is a bad thing, hence the need for this module, but i really need to be convinced that this works and this is so convenient.

+1 for it going in.

In terms of separating into another module; that would seem pointless to me, the patch is 2.5KB and I can't see that having any performance impact, although I would probably be putting reroute_email_test() in a module_name.pages file with all other page callbacks myself.

Whilst I agree that this can be achieved via several other methods - all of those above are valid - that doesn't provide an argument for not adding this patch. Drupal is full of workflows that have many different approaches, all with their own merits, there is rarely a correct answer to the question 'how should I do this?'

I believe that this tiny addition to the module provides a simple tool to test that a) this module is correctly configured and b) that your Drupal site (and server) is correctly sending emails. This patch does that without the side effect of creating new users, or rules and without having to enable extra modules (e.g. rules, contact/webform).

As far as I'm concerned, this is a good, useful patch that will benefit the users of this module and it should go in.

Hi jweowu and ceng,

Thanks a lot guys for your kind and detailed feedback. It really seems you would be very favorable to seeing this rolled it.

I have nothing against any of the points you made, it seems reasonable and you've managed changing my opinion on that (Although, I wasn't completely against from the start).

As you kindly explained, we could either keep it in the same module and have it unobtrusively loaded through a pages.inc or another admin.inc path, or also considering this as a separate module in Reroute Email, something like reroute_email_test or something like that.
(To be further discussed, once we all agree this feature request should be added)

To other maintainers: @rfay, @kbahey
We would greatly appreciate if we could have some of your feedback on this guys?
Could you please let us know if you would have any reasons or comments this feature request shouldn't be added to the module?

We need more reviews and feedbacks on this in general, so we would greatly appreciate any more comments, questions, issues, feedback, objections, suggestions or recommendations on any aspects discussed in this ticket or this feature request in general.
Thanks in advance to all for your replies, comments and feedbacks.
Cheers!

Hi,

I've just been using this too, and it saved me a lot of time! Please can we commit it to the module.

Thanks,
Pete

+1 for this feature. It comes in handy to show the emails being redirected without any fuss.

Hi guys,

Thanks very much for everyone for your kind and clear comments.

Alright, I can see there seems to be a real interest for this feature coming from the community.

If we can't get any reply from @rfay or @kbahey two weeks after I requested their feedback (which I'm afraid is likely to happen, so basically around March 19, 2013), I would have to make the final call and it would be favorable to having this feature request rolled in.

I dropped the idea of breaking this into a different module, since it looks like this change would be very unobtrusive in terms of code, performance or maintenance (like @ceng said at #4: it does no harm being here).

So in the meantime, I went ahead and re-rolled the patch as an attempt to get reviews and feedbacks on the code, to prepare for a future commit.
Please find attached to this comment a patch against reroute_email-7.x-1.x at 4930477 which is just a re-roll of the initial patch submitted at #1 by @jweowu.
File attached as: reroute_email-add-email-test-form-1571502-10.patch

I've changed a few labels, added a few missing t calls and a few more coding standards, in particular, I took this opportunity to also modify the coding style in reroute_email menu for the $items array, to switch from a "Zend" formatting type, to a "Pear" formatting for the array to be aligned with the Core coding standards (Not that this caused any coding standards errors, but it's just a personal appreciation and I would prefer to keep things consistent with the way they are done in Core).
Most likely, I would be granting the authorship to @jweowu on this commit and I would also like to request some help from the community to back-port this feature to the 6.x-1.x branch (a bit later, of course, once this patch has been approved and rolled-in for the 7.x-1.x branch).

This new patch has been tested and seems to work as expected, but I would greatly appreciate to have your feedback, questions, comments, reviews, suggestions, recommendations, improvements and testing of this new patch.

Feel free to let me know if you would have any further comments, issues, questions, objections, recommendations, suggestions, testing, reporting or concerns on the attached patch or any other aspects of this ticket in general, I would be glad to provide more information or explain in further details.

Thanks in advance to all for your feedback, reviews, testing and reporting.
Cheers!

Hi jweowu,

Thanks a lot for your prompt feedback and taking the time to review the new patch.

Agreed and I have re-rolled with just this change you suggested: the key is now test_email_form.
File attached as: reroute_email-add-email-test-form-1571502-12.patch.

Feel free to let me know if you would have any further comments, issues, questions, objections, recommendations, suggestions, testing, reporting or concerns on the attached patch or any other aspects of this ticket in general, I would be glad to provide more information or explain in further details.

As a quick side note @jweowu, I would also need help reviewing the patch from #1571508-3: More robust Cc / Bcc suppression and would greatly appreciate to have your feedback/reviews/testing on this as well.

Thanks in advance to all for your feedback, reviews, testing and reporting.
Cheers!

Hi guys,

Sorry again for stalling on this for almost a month, but since there hasn't been any feedback on the patch from #12, I wanted to wait a bit longer.

In any case, I have given the patch some more tests and it seems to work as expected.

Since there was no further objections or issues, I went ahead and committed the patch based on the work from @jweowu at:

• 7.x-1.x: f2e3878
• 6.x-1.x: 039c28b

Note that I also modified the README.txt file to add the Test Email Form to the module's description.
I also allowed myself to clean-up a bit module's description and add a Maintainers section.
(For more information, see README.txt-7.x-1.x-f2e3878-diff and README.txt-6.x-1.x-039c28b-diff)

When the next version is released, I should have Reroute Email's project page edited to add a section similar to the one added to the README.txt:

Test Email Form
---------------
Reroute Email also provides a convenient form for testing email sending or
rerouting. After enabling the module, a test email form is accessible under:
Admin -> Settings -> Reroute email -> Test email form

This form allows sending an email upon submission to the recipients entered in
the fields To, Cc and Bcc, which is very practical for testing if emails are
correctly rerouted to the configured addresses.

I allowed myself to mark this issue as fixed for now, but feel free to re-open it, or post a new ticket, at any time if you have any further objections with the solution suggested at #1 (we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of these commits or this ticket in general, I would be glad to provide more information or explain in more details.

Special thanks to @jweowu for the work on this patch.
Thanks again to everyone for your help, reviews, feedback and comments on this issue.
Cheers!

Hi guys,

I wanted to get back to this issue quickly:
After reviewing these changes again, I found we never actually discussed form validation for email fields.

I know this is a development module and this feature would be used for testing. However, I thought a user might submit the form with a wrong email for the Cc field, for example, and the system wouldn't let the user know.
In other words, I thought it might be good to prompt an error and let users know if an invalid email format was unintentionally submitted through the form.

Therefore, I went ahead and allowed myself to add Valid Email Address form validation for the fields To, Cc and Bcc.

The changes were committed against the:

• 7.x-1.x branch at 5a93293
• 6.x-1.x branch at 0aea89d

Feel free to re-open this issue, or post a new ticket, at any time if you have any further objections with this update/change or commits (5a93293, 0aea89d - we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of these commits or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks again to everyone for your help, reviews, feedback and comments on this issue.
Cheers!

One more thing I think we may have forgotten with this new feature was perhaps updating Reroute Email test cases.

So I added a new basic test case (RerouteEmailTestEmailTestCase) to the module to test:

1. Rerouting of test email sent through the test form.
2. Parameters from the test form generate the correct email (to, cc, bcc, subject and body are correctly captured).
3. Form validation: Email formats and required.

I went ahead and committed the changes against the 7.x-1.x branch at 4657633, followed by a minor commit at 39ff38b to fix coding standards errors introduced by the changes.

Feel free to re-open this issue, or post a new ticket, at any time if you have any further objections with this update/change or commits (4657633, 39ff38b - we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of these commits or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks again to everyone for your help, reviews, feedback and comments on this issue.
Cheers!

Hi @jweowu,

Thanks again very much for your kind feedback and follow-up with the latest changes on this issue.

I understand having an email validation for this form is probably arguable since module is more of a development tool than an actual site feature to be used by front-end users, such as the contact form, for example.

Additionally, I have tested a bit more without email rerouting, submitting the form with invalid email addresses and an error message is clearly displayed to the user:
Unable to send e-mail. Contact the site administrator if the problem persists.

The mail sending error would also seem to be properly caught by the watchdog in error reporting.
Error sending e-mail (from test@example.com to test_invalid).

Indeed, I agree with @jweowu, so I went ahead and removed email validation, but I still thought we might want to add a simple short description help text to the Email addresses fieldset to let users know about that:
Email addresses are not validated: any valid or invalid email address format could be submitted.

Tests were also updated accordingly.

Corresponding changes were committed against the:

• 7.x-1.x branch at c6db818
• 6.x-1.x branch at 12916cd

 
Feel free to re-open this issue, or post a new ticket, at any time if you have any further objections with this update/change or commits (c6db818, 12916cd - we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on any of these commits or this ticket in general, I would be glad to provide more information or explain in more details.

Thanks again to everyone for your help, reviews, feedback and comments on this issue.
Cheers!