[D7] Rapidmail

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxfatel1955388git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

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

Hi,

There are some issues I noted,
File : rapdimail.module

1) Remove test development codes, Line no 11
2)

if (!empty($rapidmail_load_library)) {
  module_load_include('php', 'rapidmail', 'includes/rapidmail.class');
} 
else {
  define('rapidmail_warning', 'Please download and include rapidmail.class.php from rapidmail.com and include it in "path/to/rapidmail/includes".');
}

You can include this section hook_init().

Hi fatel!

Automated Review

The code reviewer has picked up a number of code style issues. http://ventral.org/pareview/httpgitdrupalorgsandboxfatel1955388git-7x-10

Manual Review

• The branch for 7.x-1.x releases should be called 7.x-1.x rather than 7.x-1.0. When you come to make a release you can tag a particular commit as 7.x-1.0. (see http://drupal.org/node/1015226)
• You should delete the master branch.
• The .module .info and README.txt file should be in the root of the git repository (you don't need the rapidmail folder).
• The README.txt is really useful and detailed, however the lines shouldn't exceed 80 characters in length.
• The name in the .info file is human readable name so something like 'RapidMail Integration' might be better. The same goes for the description.
• The level of commenting looks good although some of it needs to be updated to fit with coding standards http://drupal.org/coding-standards/docs

It looks like a really useful module I'm looking forward to seeing it released :D

Hi fatel,

I've had a quick look through your module and found a couple of things that need fixing:

• In the rapidmail_form_submit() function you shouldn't be using $_POST to get the form values, instead you should use $form_state['values']. For example line 190 should read:
  

  if (isset($form_state['values']['rapidmail_firstname'])) {
  

• You also need a hook_uninstall function (must be in rapidmail.install file), to remove the variables you have set when you module is uninstalled:
  

  /**
   * Implements hook_uninstall().
   */
  function rapidmail_uninstall() {
    variable_del('rapidmail_af_fn');
    variable_del('rapidmail_af_fn_mand');
    // etc... 
  }
  

• Finally you check on the admin page to see if the rapidmail class has been loaded, but you don't check before you display the block. You probably don't want the block to be available if the class isn't included.

A couple of other minor points:

• At the moment you are including the rapidmail class on every page, do you need to do this or would you be better off just loading the class when you need it?
• If you get chance it might be worth looking at integrating with the libraries module, which can be used to manage external libraries within Drupal.

Good luck with your module, I hope the above is helpful to you.

Forgot to change the status to needs work with my above comment.

Manual Review:

1. Use t() function in line number 105, 120, 170.
2. After every function give one line gap.

Hi Fatel

Automated Review http://ventral.org/pareview/httpgitdrupalorgsandboxfatel1955388git

FILE: /var/www/drupal-7-pareview/pareview_temp/rapidmail.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
43 | WARNING | Code after RETURN statement cannot be executed
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/rapidmail.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
58 | WARNING | The administration menu callback should probably use
| | "administer site configuration" - which implies the user can
| | change something - rather than "access administration pages"
| | which is about viewing but not changing configurations.
--------------------------------------------------------------------------------

Manual Review
- In rapidmail.inc, do you think that we should run a single query to delete all needed variables instead of calling variable_del() mtuliple times? (Just my point of view)
- In rapidmail.module, you don't have to check for drupal_get_path(), because that function is in the core

 if (function_exists('drupal_get_path')) {
    $file = DRUPAL_ROOT . '/' . drupal_get_path('module', 'rapidmail') . "/includes/rapidmail.class.php";
    if (is_file($file)) {
      return $file;
    }
  }

You should use t() function in hook_init()

function rapidmail_init() {
  if (!rapidmail_library_available()) {
    define('RAPIDMAIL_WARNING', 'Please download and include 
          rapidmail.class.php from rapidmail.com and include it in 
          "path/to/rapidmail/includes".');
  }
}

in hook_help(), you should put the link in a variable, because if we change the link, we don't have to translate the text again

function rapidmail_help($path, $arg) {
  switch ($path) {
    case "admin/help#rapidmail":
      return '<p>' . t("You can get additional information <a href='http://drupal.org/sandbox/fatel/1955388' target='_blank'>here</a>.") . '</p>';

    break;
  }
}

change to

function rapidmail_help($path, $arg) {
  switch ($path) {
    case "admin/help#rapidmail":
      return '<p>' . t("You can get additional information <a href='!link' target='_blank'>here</a>.", array('!link' => 'http://drupal.org/sandbox/fatel/1955388')) . '</p>';

    break;
  }
}

That's all :-)

Couple of typos - "ist" and "menue" in the readme. This is a weird line in the .info file: ; NEW LINE.

You dont want to use hook_init() to check for your library - this will execute on every drupal page load, which is not necessary. Better to use hook_requirements() which will let you alert the administrator if there is a problem. Using if (defined('RAPIDMAIL_WARNING')) { is not the answer either, just do the check where it is necessary instead of creating a pseudo-global.

require_once rapidmail_library_available(); will produce a fatal error if the library isn't present, instead consider sending a nice message to the user and logging the event with watchdog() for the admin.

Those are not critical errors though, this is RTBC from me.

----
Top Shelf Modules - Enterprise modules from the community for the community.

thanks, looks good.

Sorry I didn't mention this earlier, but the Libraries API module is a recommended method for adding 3rd party dependencies.

In rapidmail_admin_settings() you can simplify by just using '#default_value' => variable_get('rapidmail_api_key') for each of the variables. system_settings_form() will save the values into the right variable names for you already. I don't think you are using RAPIDMAIL_WARNING anymore, and that can be removed.

Thanks for your contribution, fatel!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.