slashrsm [slashrsm]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

1.

function sequenced_newsletter_menu() {

  $items = array();

  $items['admin/settings/sequenced_newsletter'] = array(
    'title' => t('Sequenced newsletter'),
    'description' => t('Configuration'),
    'page callback' => 'drupal_get_form',
    'page arguments' => array('sequenced_newsletter_admin'),
    'access arguments' => array('access administration pages'),
    'type' => MENU_NORMAL_ITEM,
  );

  $items['sequenced_newsletter/list'] = array(
    'title' => t('List of sent letters'),
    'description' => t('List of sent letters'),
    'page callback' => '_sequenced_newsletter_list_letters',
    'access arguments' => array('view sent letters list'),
    'type' => MENU_NORMAL_ITEM,
  );

  return $items;
}

Don't use t() on title and description.
Default type is MENU_NORMAL_ITEM so you don't actually need do write it.

2.

$lett_array = Array();

// should be

$lett_array = array();

Can be found at more places.

3. Please remove all the "debug" comments. eg. // print_r($subs_array);

Menu titles and descriptions are passed to t() from Drupal core code.
This is similar to what done from watchdog(); the second argument is not a translated string, but a string that watchdog() will pass to t().

Small details:

1. array with a small "a".

// sequenced_newsletter.module:41
$types = Array( 

2. The do it ;)

// sequenced_newsletter.module:11
$output = '<p>TODO: Help to be added.</p>'; 

// sequenced_newsletter.module:128
'description' => 'Sequenced module\'s list of sent emails (for debugging purpuses).',

You can use " " instead of ' ' here and skip the \ before 's

Avoid escaping quotation marks wherever possible.

Coder reports
sequenced_newsletter.module Line 13: There should be no trailing spaces.

Please use Coder and check your code before posting it here. It helps both me, you, and others that don't have to check and correct more times then needed.

I don't have no tips to get this done faster. For me, I don't have so much time to spend on this so it may take some times for me to review and respond. Hang in there and continue with the good job and hopefully you will be approved soon.

function _sequenced_newsletter_list_letters() {
  $out = "";
  $addresses = db_query("SELECT sid, title, date_spooled, mail FROM {sequenced_newsletter} as s, {node} as n, {simplenews_subscriptions} AS su WHERE s.nid=n.nid and s.sid=su.snid ORDER BY mail AND date_spooled DESC");
  $old_mail = "";
  while ($row = db_fetch_array($addresses)) {
    if ($old_mail != $row['mail']) {
      $out .= $old_mail == "" ? "<h2>" . $row['mail'] . "</h2><ul>" : "</ul>\n<h2>" . $row['mail'] . "</h2><ul>\n";
      $old_mail = $row['mail'];
    }

    $out .= "<li>" . $row['title'] . " (" . format_date($row['date_spooled']) . ")</li>\n";
  }

  $out .=  "</ul>\n";

  return $out;
}

$row['mail'] and $row['title'] is that user-submitted content? If yes (I think so), then it should be passed into check_plain() first.

Use check functions on output to prevent cross site scripting attacks

No piece of user-submitted content should ever be placed as-is into HTML.

* Use check_plain or theme('placeholder') for plain text.
* Use check_markup or filter_xss for markup containing text.
* Use the t() function with @ or % placeholders to construct safe, translatable strings.

See how to handle text in a secure fashion for more details.

from http://drupal.org/writing-secure-code

Do this alternatives do something?

      3 => t('1 minute - Just for debug'),
      4 => t('5 minutes - Just for debug'),

- - -

Code looks good otherwise.

Misspelling on "diabled" maybe?

'#description' => t("When enabled sequenced newsletters will be sent. When diabled nothing will happen."),

- - -

What is the big different between your module and this http://drupal.org/project/simplenews_scheduler ?

Okey! Sry I didn't get it when I read your first post, I was a bit tired.

For now I don't have any more to add. Maybe kiamlaluno can take a look at it and see if it should be approved.

The users who are applying for a CVS account cannot change the status of their own application to RTBC.

Did a overview again and found some.

1. Try not to use escape char \' in a t(), you can use " " around the text here instead.

      $output .= '<p>' . t('Module is configured on it\'s <a href="@settings">settings page</a>. Emails are sent through cron job; every time when cron runs module checks if it is already time to send emails. Emails are put in Simplenews spool, when necessary. Emails are then sent through Simplenews. Module also generates a, <a href="@list">list of all emails</a> sent to Simplenews spool.', array('@settings' => url('admin/settings/sequenced_newsletter'), '@list' => url('sequenced_newsletter/list'))) . '</p>';

2. _sequenced_newsletter_list_letters() you might want to reconstruct. Maybe use a theme function to put out the list, so it can be overrided. But after a while that list will be very long and you might want to add a pager option.

These two points are not errors, but proposals to improve. I have also tested the module and it work fine. So I will say this module is ready.

Will set this to rtbc and if kiamlaluno or someone else thinks that this two points should be changed before an approval, please make a note.

Escaping the string delimiter in a string passed to t() causes the string shown in l.d.o to contain \'; it should be fixed before the first official release of the module is released.
I trust Scyther for doing an accurate review, and I am going to approve this application.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.