Mail Statistics

Hi ,

# coding standards are okay with online test (http://ventral.org/pareview/httpgitdrupalorgsandboxleksat1677600git)
# consider including this under package 'Mail' as its related to mail (in .info)
# there should be hook_uninstall to uninstall schema, variables (if any)

schemas don't need to be installed or uninstalled in drupal 7, this happens automatically

I think one of the things that could be changed is to have a real datetime field for the "date_string" and "send_date_string" fields in the schema, and not just a string in the format YY-MM-DD. I know that the reason is to not use special mysql functions when grouping, but for example this imposes also a restriction: you cannot group by month for example. So I think we can find a solution to use datetime fields, and the module to be able to use different sql functions (for mysql or pgsql) to group the results, depending if a mysql or a pgsql database is used.

Also, in the schema file, I saw you use "_mail_statistics_update_module_weight()" to change the weight of the module so that you are sure that the hook_mail_alter() is invoked the last time for your module. But, maybe a better solution is to use http://api.drupal.org/api/drupal/modules!system!system.api.php/function/... to alter only the mail_alter hook.

Create a detailed README.txt and project page, see: http://drupal.org/node/161085
Add configure = admin/config/system/mail_statistics to your mail_statistics.info

Hi Leksat,

A very interesting module indeed, but... I couldn't test all functionalities as there is a fatal error...

You call function _mail_statistics_format_time_string($timestamp) on mail_statistics_mail_process.inc at line 29, even if you define it at line 97 of mail_statistics.inc Drupal complains about it because the file is not included.
I've found this error when I tried to create a new user account or sending an email using contact module.
Fatal error: Call to undefined function _mail_statistics_format_time_string() in /var/www/d7/sites/all/modules/mail_statistics/includes/mail_statistics.mail_process.inc on line 29

changing status

Hi Leksat,
thank you for your module.

1) Manual code review. Since you have only one .js file, one .css file and one .png file may be it's better to place them inside the root module directory and name all of them "mail_statistics.EXT"? I think it will be more clear and transparent until you need to separate them into the several files.
2) Manual code review. includes/mail_statistics.mail_process.inc: lines 259-264 looks like you integrates with Simplenews. May be you should to add it to module dependencies or check on existance.

Hi Leksat,

This looks like a very useful module, and I wish I had this functionality on some of the web sites that I have built!

I performed a manual review of your code, and it looks well written and nicely organized. I think you have done a very nice job!

However, I did find some problems:

There were a few errors picked up by the automatic code reviewer at http://ventral.org/pareview.

Also, when I loaded your module and tested it, I was confused by the statistics report I received. I created emails using the Drupal Contact and Webform modules. The email created from the Webform module also contained a link for the recipient to open. I sent emails from these modules to two email addresses that I own, and opened the emails using both GMail and Microsoft Outlook. I also clicked on the link inside the Webform email. The report I received is shown below. It counted the emails that I sent, but it did not seem to know that I had opened the emails and clicked on the link.

Bob Humphrey

mail_statistics_schema() $schema not declared/initiated
mail_statistics_settings_form() $form not declared/initiated
_mail_statistics_register_click() and _mail_statistics_register_action() look to pass parameters as single array var

_mail_statistics_format_time_string perhaps you use a ternary operator consistent with other functions:

return (empty($cache[$timestamp])) ? $cache[$timestamp] = format_date($timestamp, 'custom', 'YmdHis') : $cache[$timestamp];

ditto with _mail_statistics_generate_uuid ()

You don't need to initialize array in PHP, so that is not a mistake.
"_mail_statistics_register_click() and _mail_statistics_register_action() look to pass parameters as single array var": what does that mean? Why should you pass only one ugly array parameter instead of a clear function signature?

That are surely no application blockers, anything else?

mail_statistics_report_form_submit() Form-generating functions commenting http://drupal.org/node/1354#functions

mail_statistics.detailed_report lines 101, 104, why are you reassigning $query to itself?

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Thank you leksat for wonderful module.

I reviewed your module manually and using automatic techniques, and it looks ready to be live now. Although there is margin to improve the standards but those are surely not show blocker.

- Sent email count increases even if there is problem in sending email (smtp is not configured or some other issue.)
- Some docblocks are missing at top of the files like js/charts.js, and some test files

I am not sure if i should mark it "reviewed and tested by community".

Rest seems perfect to me.

This is a very nice and useful module. I haven't done a review of it as such, but I've got a couple of comments:

#1. The 'mail-link' and 'mail-open' menu callbacks.

Let's say you install this module one a high-traffic site, and you send 1000 e-mails every minute. If all recipients open this e-mail, that would be 1000 page requests, and if every recipient clicks on two links in the e-mail that would be an additional 2000 page requests. Each of these page requests would be a full bootstrap of Drupal.

As a future performance improvemenent worth considering, how about moving these menu callbacks to,say, link.php and open.php which only does a partial bootstrap of Drupal up to the point where the Database (or variables) are initialised. This will not be as heavy on the web server, and the end user will most likely get a "smoother" experience as the clicked links will resolve faster.

Do a google search for something like "drupal bootstrap partial" and you'll get plenty of suggestions for how do to this.

#2 The documentation

The documentation says "Information about email opens collects only if your system is configured to send email in HTML format.". How does one configure the system to send email in HTML format? By default, Drupal sends e-mail in plain text. Does this module use an alter hook to determine the format of the e-mail? I can't see anything in mail_statistics_mail_alter which evaluates the format of the message?

Very nice module!

- In mail_statistics.info, remove the line 1 - it's a blank line from blob view: http://is.gd/14QrSX
- 'access callback' is left TRUE in some menu router items. Shouldn't we wrap the access ?
- In mail_statistics.admin.inc, use proper modifiers in t() function: !, %, @. You can't use <pre> tags in the t() function's first param because it will make the text not translatable. You can use replacement patterns.

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

http://ventral.org/pareview/httpgitdrupalorgsandboxleksat1677600git

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.
http://ventral.org/pareview/httpgitdrupalorgsandboxleksat1677600git

These items need to be fixed before RTBC, but the rest of the code looks good.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

I'm a robot and this is an automated message from Project Applications Scraper.

I'm sure that when somebody is interested in your work they will clone your sandbox, work on it and promote it themselves eventually. So don't worry: your work is not lost just because you don't need it anymore.