Newsletter (D7)

Installed, tested and I like the module. Easy and clean workflow.

Review :

1. Licensing: No issues.
2. Module duplication: Possible duplication of effort with simplenews. I see that simplenews can do most the the listed things with addon modules but they have not been ported yet. Paris, can you please compare and log what is different here. See these modules :
   • sequenced_newsletter
   • simplenews_scheduler
   • simplenews_analytics
   • simplenews_statistics
   • simplenews_template
3. Code Review:
   • Break a line between functions declarations. eg.
     

     /**
      * Implements hook_taxonomy_term_delete().
      */
     function newsletter_taxonomy_term_delete($term) {
       db_delete('newsletter_index')
         ->condition('tid', $term->tid)
         ->execute();
     }
     /**
      * Implements hook_cron().
      */
     function newsletter_cron() {
       $status = array();
       $cron_lists = db_query('SELECT id, send_rate FROM {newsletter_lists} WHERE send_again = CURDATE() AND (send_rate = \'Daily\' OR send_rate = \'Weekly\' OR send_rate = \'Monthly\')')->fetchAll();
       if (is_array($cron_lists)) {
         foreach ($cron_lists as $cron_list) {
           $newsletter=new NewsletterMail($cron_list->id);
           $status[] = $newsletter->send();
           //@TODO update the last_sent and statistics
         }
       }
       NewsletterFunctions::feedback($status);
     }
     

     should be

     /**
      * Implements hook_taxonomy_term_delete().
      */
     function newsletter_taxonomy_term_delete($term) {
       db_delete('newsletter_index')
         ->condition('tid', $term->tid)
         ->execute();
     }
     
     /**
      * Implements hook_cron().
      */
     function newsletter_cron() {
       $status = array();
       $cron_lists = db_query('SELECT id, send_rate FROM {newsletter_lists} WHERE send_again = CURDATE() AND (send_rate = \'Daily\' OR send_rate = \'Weekly\' OR send_rate = \'Monthly\')')->fetchAll();
       if (is_array($cron_lists)) {
         foreach ($cron_lists as $cron_list) {
           $newsletter=new NewsletterMail($cron_list->id);
           $status[] = $newsletter->send();
           //@TODO update the last_sent and statistics
         }
       }
       NewsletterFunctions::feedback($status);
     }
     

   • Move hook_install and hook_unistall to a .install file.
   • Comment functions like these :
     

     function newsletter_lid_load($lid) {
       return db_query("SELECT * FROM {newsletter_lists} WHERE id = :id", array(':id' => $lid))->fetchObject();
     }
     function newsletter_sid_load($sid) {
       return db_query("SELECT * FROM {newsletter_subscribers} WHERE id = :id", array(':id' => $sid))->fetchObject();
     }
     function newsletter_tid_load($tid) {
       return db_query("SELECT * FROM {newsletter_templates} WHERE id = :id", array(':id' => $tid))->fetchObject();
     }
     

   • It might be better to place newsletter under "Structure" rather than first class citizens of the admin menu.
4. Some errors encountered when sending newsletter :
   

       Notice: Array to string conversion in drupal_attributes() (line 2261 of /Applications/XAMPP/xamppfiles/htdocs/lab7.2/includes/common.inc).
       Notice: Array to string conversion in drupal_attributes() (line 2261 of /Applications/XAMPP/xamppfiles/htdocs/lab7.2/includes/common.inc).
       Notice: Undefined variable: items in NewsletterMail->send() (line 226 of /Applications/XAMPP/xamppfiles/htdocs/lab7.2/sites/all/modules/newsletter/includes/mail.inc.php).
       Warning: implode() [function.implode]: Invalid arguments passed in NewsletterMail->send() (line 226 of /Applications/XAMPP/xamppfiles/htdocs/lab7.2/sites/all/modules/newsletter/includes/mail.inc.php).
       Notice: Undefined variable: items in NewsletterMail->send() (line 226 of /Applications/XAMPP/xamppfiles/htdocs/lab7.2/sites/all/modules/newsletter/includes/mail.inc.php).
       Warning: implode() [function.implode]: Invalid arguments passed in NewsletterMail->send() (line 226 of /Applications/XAMPP/xamppfiles/htdocs/lab7.2/sites/all/modules/newsletter/includes/mail.inc.php).
   

Hey Paris, sorry for the late reply. Been super busy. I'll make sure I review this today. and update. Thanks for your help and understanding :)

The alpha version in combination with the Demosite really really helped. Without this, I would have never tested this module.

A link on your sandbox to this issue would also help.

"If you like this newsletter module please help to make this a full project by reviewing this module and posting your feedback here: Newsletter (D7) - Drupal.org Project applications"

My first impression after reviewing this a bit:

• This module is a promising approach to bring an easy out-of-the-box newsletter tool to Drupal.
• It provieds a straight forward setup without the complexity of simplenews. After all I found this module looking for an alternative for simplenews.

Sorry I used the wrong link in my suggestion. Now the sandbox page links itself :) it should link here:
http://drupal.org/node/1225708

Don't know if this is a way to help get it approved, but I have tried all sorts of newsletter, mail, subscription modules over the past year and Newsletter works right the first time and is intuitive. I think a module like this is necessary for community sites where people want to receive updates without being required to register.

Whoever has the power to assign something as a full project, please do it and help other people find it more easily. Thanks and thanks rootatwc!

As far as I've tested, it is indeed a very user friendly and all-in-one newsletter solution.
However, I got stuck on the mail template (in my case admin/newsletters/templates/edit/4?destination=admin/newsletters/templates) and the replacement patterns.

In the default template it shows
[repeat]
[item_title] -[item_date]
[item_teaser]
[/repeat]

and the explanation says:
[repeat] --- The section starting with this token will be replaced for all nodes that are to be sent with this list
[/repeat] --- End of the repeat section

But I don't understand how to make node(teasers) appear in the sent newsletter.
The email is successfully sent and received but the replacement patterns mentioned above are not replaced with the teasers.
So actually i have two questions: 1) how do i select nodes to be included in a newsletter and 2) Why don't the replacement patterns work.

The replacement pattern problem might have to do with the following:
After enabling your Newsletter module, and I create a new node (i.e. a blog post) I get an error:
Warning: Illegal offset type in isset or empty in pathauto_cleanstring() (line 180 of /home/primo1/public_html/sites/all/modules/pathauto/pathauto.inc).
Notice: Array to string conversion in pathauto_cleanstring() (line 185 of /home/primo1/public_html/sites/all/modules/pathauto/pathauto.inc).
Warning: Illegal offset type in pathauto_cleanstring() (line 223 of /home/primo1/public_html/sites/all/modules/pathauto/pathauto.inc).

(this set of error lines repeat 15 times on the same page)

As far as I see this error is similar to http://drupal.org/node/1246074 and indicates that the module is duplicating Tokens.
I have Token version 7.x-1.0-beta4 installed on my test site.

• Licensing: No problems found. Newsletter does not include any third party code. However it integrates with jqPlot which is dual licensed under the MIT and GPL version 2 licenses.
• Module duplication: There is a groups.drupal.org page with a Comparison of Newsletter / External E-Mail Campaign Integration Modules. There is need for consolidation in many areas of Drupal development. However, the main competitor simplenews is not "simple" and development is rather slow. IMO there is room and serious need for competition to see some more innovation to happen here.
• Overall understanding of Drupal API's: Drupal API's (hooks, new database abstraction layer, etc.) are used correctly. Maintainer plans to add and further improve views integration, use entities, etc.
• Security: No obvious security issues found. Coder module returns no problems.
• Drupal coding standards: The code looks very well structured – a lot better than many Drupal contrib modules (full projects). Coder module returns no coding style problems.
• Code documentation: The code is documented well using doxygen style. However comments could be slightly improved ;) , e.g.:
  

  All documentation and comments should form proper sentences and use proper grammar and punctuation.

  See: Doxygen and comment formatting conventions

The maintainer of this sandbox project is very responsive, and solved all issues fast and friendly :) examples:
#1281444: Fix coder errors related to confirm_form()
#1275610: Notice: Undefined index: email in newsletter_subscribe_advanced() (line 95 of .../newsletter/includes/user.inc).
#1272130: Configuration changes made under admin/newsletters/configure (General Settings) cant be saved

There was ONLY positive feedback to this project (see #1, #17, #18, #20).

IMO this sandbox is ready to be released as a full project.

Unfortunately, I did not have time to review the module after #1, but +1 from me for promoting this to a full project.

This module is really very close. Only a couple issues remaining:

Using the path "admin/newsletter/*" doesn't follow the Information architecture standards laid out by core. It should likely be "admin/config/newsletter/*".

The includes should be renamed, admin.inc should become newsletter.admin.inc.

The require_once's at the beginning of newsletter.module are not very Drupal-like. I picked a random function from functions.inc, newsletter_get_vids(), and saw it was only used in admin.inc. It could be moved there.

The $(document).ready(function () { in newsletter.js should be converted to Drupal.behaviors: http://drupal.org/node/171213#behaviors, and the function should be namespaced as well. The entire file can be wrapped in the one closure binding $ to jQuery.

In the .info, the description should end in a full stop, and doesn't need to be wrapped in quotes.

The CSS file should like be renamed newsletter.theme.css, in accordance with http://drupal.org/node/1285604.

Also, double check your trailing whitespace, there are some in the CSS, JS, and README.txt.

Well first of all, if you leave it, the comment could be

// Loads functions needed. Used separate files for organization purposes.

Spelling and spacing are important.

My point about the functions in includes/functions.inc and includes/user.inc is that certain functions seem to be only used by certain include files.

The following files are only ever called in admin.inc:

newsletter_send_draft()
newsletter_load_drafts()
newsletter_lid_load()
newsletter_sid_load()
newsletter_tid_load()
newsletter_nid_load()
newsletter_get_tree()
newsletter_template_checked_terms()

They could easily be moved. And those are just an example, I imagine things could be chopped up further.

One other thing that comes to mind: you should create an "empty" master branch in Git, that contains a README.txt explaining why it is empty. An example is here: http://drupalcode.org/project/examples.git/blob/refs/heads/master:/READM...

Otherwise those checking out your module with Git will get an error.

• Config moved to admin/config/newsletter
• includes renamed, example admin.inc > newsletter.admin.inc
• $(document).ready(function () { converted to Drupal.behaviors
• .info description fixed
• CSS file renamed newsletter.admin.css according to http://drupal.org/node/1089868
• trailing spaces fixed
• This got fixed too: #1286664: Fatal error: Call to undefined function newsletter_get_subscribe_form_lists()

I added a patch that adds a "." at the end of the comment as suggested in #25 ;-) Doxygen and comment formatting conventions. But that realy shouldn't block this project application...

Awesome, looks much better.

I've granted you full project creation rights. Use this ability carefully!

Thank you for your patience.

subscribing... is this still being developed?

This issue is still among the top search results for "Drupal newsletter D7".

The first link in this issue (http://drupal.org/sandbox/rootatwc/1212302) is dead and should be replaced with the full project URL...

updated statistics (they are usable now)

Updated issue summary.

I love the Newsletter module. I find it much simplier and more feature complete than simplenews. One issue, I am trying to figure out how to get a list of the newletters I have sent in the past. I can't seem to see them. Their must be nodes created right? I have tried to use views to grab them, but no luck.

Anyone have any thoughts?

@tbeckett this issue is only for project application matters and is already closed, please create a new support issue in the projects application queue for your request.

Updated issue summary.