[D7] Email tester

What's the link to your sandbox?

Any reason that we can't improve the existing modules that intercept outgoing e-mails?
http://drupal.org/project/maillog
http://drupal.org/project/mail_logger

Jerome,

You've explained how your module differs from the links that Dave provided, but I suspect that wasn't the intent of his question.

Can you explain why we should approve a new module for this, compared to the option of adding a new 'modal popup' feature to an existing module?

In other words, would you instead consider creating a patch which adds the 'modal popup' feature as an enhancement to the pre-existing http://drupal.org/project/maillog module?

Closing due to inactivity, feel free to re-open if this was a mistake.

I think it's a fair point that logging and testing are two different things: you want a logger to send emails as normal while logging them; you want a tester to show you the emails without sending them.

So, logically, this module shouldn't do any logging of its own, and leave that to a logging module.

> Actually, my module doesn't log anything it just display outgoing emails in a modalframe.

That's perfect then!

Well, I think it's justified in being a project alongside the mail logging modules, as I think they cover different, complementary areas. Though you should ensure that they work well together.

Still needs a code review! :)

Hi jerome.megal,

I created some issues in your project issue queue. Please set this back to "needs review" when those are fixed.

This looks good to me.

Has the following code been tested to see if it is vulnerable to XSS at all? The outputting of variables directly in a theme function is kind of a big red flag.

 135 function theme_email_tester_format($emails_to_disp) {
 136   $output  = '<div id="email_popup" title="'. t('Emails sent:') .'">';
 137   $output .= '<ul id="email_accordion" style="width: 760px;">';
 138   foreach ($emails_to_disp as $key => $email) {
 139     $output .= '
 140       <li class="ui-accordion-group">
 141         <h3 class="ui-accordion-header"><a href=\'#\'>'. $key .'</a></h3>
 142         <div class="ui-accordion-data">'.
 143           '<p><strong>'.t('From').'</strong>:&nbsp;'.$email['from'].'</p>'.
 144           '<p><strong>'.t('To').'</strong>:&nbsp;'.$email['to'].'</p>'.
 145           '<p><strong>'.t('Language').'</strong>:&nbsp;'.$email['language'].'</p>'.
 146           '<p><strong>'.t('Subject').'</strong>:&nbsp;'.$email['subject'].'</p>'.
 147           '<p><strong>'.t('Body').'</strong>:&nbsp;'.$email['body'].'</p>'.
 148        '</div>
 149       </li>';
 150   }
 151   $output .= '</ul>';
 152   $output .= '</div>';
 153 
 154   return $output;

A few more issues:

• Module popup is definately susceptible to XSS attacks. I realize that this is supposed to be a admin/development tool, but all screen output must be properly sanitized by passing it through a filter such as check_plain, filter_xss, or filter_xss_admin. (Sending an email to <script>alert('xss!');</script> must not result in a pop-up.)
• When jquery_ui is not already installed, and a user has simply downloaded the jquery_ui module (and not the library itself), attempting to install Email_tester and have it auto-install jquery_ui (with library missing) resulted in a PHP fatal error:
  PHP Fatal error: Call to undefined function jquery_ui_add() in /var/www/d6/sites/all/modules/email_tester/email_tester.module on line 19
  This resulted in WSOD when visiting admin/build/modules. I had to manually disable the module in the database. To avoid this, you should leverage hook_requirements() to validate the proper installation of jquery_ui before allowing email_tester to be enabled.
• Email_tester.module is missing an EOL (end-of-line) on the last line of the file.
• The reroute email textfield on admin/settings/devel should be weighted so that it appears above the 'save settings' buttons.

In addition, Coder reports the following issues:

email_tester.module

Line 2: Commits to the Git repository do not require the CVS $Id$ keyword in each file. (Drupal Docs)

      // $Id$

Line 143: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms

                '<p><strong>'.t('From').'</strong>:&nbsp;'.$email['from'].'</p>'.

Line 144: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms

                '<p><strong>'.t('To').'</strong>:&nbsp;'.$email['to'].'</p>'.

Line 145: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms

                '<p><strong>'.t('Language').'</strong>:&nbsp;'.$email['language'].'</p>'.

Line 146: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms

                '<p><strong>'.t('Subject').'</strong>:&nbsp;'.$email['subject'].'</p>'.

Line 147: string concatenation should be formatted with a space separating the operators (dot .) and non-quote terms

                '<p><strong>'.t('Body').'</strong>:&nbsp;'.$email['body'].'</p>'.

email_tester.install

Line 2: Commits to the Git repository do not require the CVS $Id$ keyword in each file. (Drupal Docs)

      // $Id$

Thanks for the quick changes!

Generally, I would suggest using check_plain() over filter_xss() for the sanitization checks, unless that variable could legitimately hold HTML tags ... though both will achieve the desired result, check_plain() is probably lighter from a performance perspective.

With regards to the themeroller css files/images ... can you provide a link to some information which discusses the license terms regarding use of the themeroller-generated items? Any included code component that is not your own needs to be re-useable under a GPLv2 license.

And even if the output is useable under a GPLv2 license, I would recommend that the module only include the bare minimum css required to function - To illustrate, consider what would happen if a user installed two different modules on their site, both which have used themeroller to generate their css output. Since the themeroller files contain the same element names for both modules, you're going to run into css namespace conflicts ... or your module could be responsible for over-riding the jquery UI datepicker used in other areas of the site, forcing the site maintainer on a wild goose chase trying to find what broke things.

I would suggest minimalizing the css, and adding namespacing to the .css file definitions ... this is probably not a show-stopper for the application (though the license issue could be), but I'd at least give it a strong consideration.

Hmmm ... I remember typing this yesterday, but it appears it didn't submit ...

filter_xss() is probably the better choice for the email body, in case you're sending html emails. The check_plain() on the rest looks good.

And a big +1 for going with the default jquery UI theme. :)

Thanks for your contribution, jerome.megel! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.

I am giving credits to the users who participated in this issue.