[D7] Albacross

Hi ALBACROSS,

1. Please check with automated review tool and correct it.
https://pareview.sh/pareview/https-git.drupal.org-project-albacross.git

2. Please remove unwanted comments from the .module file.

Hi albacross,

When i reviewed your module, i found "albacross" external script file path (serve.albacross.com/track.js) mentioned in your module's "hook_preprocess_page". Albacross script file is hardcoded in the hook.

There might be chance of changing the script file name or URL in future by albacross third party. It is better to have another configuration field just like "albacross_drupal_trackid" to update the "Albacross Script path". Can avoid hardcoded URLs. :)

Thanks,
Logeshwaran

To the reviewers: Please set the priority to Normal after reviewing the project.

Few suggestions:

1. You are using a variable “albacross_drupal_trackid” which will reside in the database. It would be great if you delete this variable when the module is uninstalled.
2. From the hook_help and comments in the .module file, I got that the script will be added in the footer. It would be great if you share this information in README.txt file and on the project page as well.
3. In the hook_form_validate, I don’t see the appropriate messages as per your logic. For example:
   

   if (!is_numeric($trackId)) {
       form_set_error('albacross_drupal_trackid', t('You must enter correct Track ID for your Albacross account'));
     }
   

   Here, probably “Please enter numeric tracking id from your Albacross account.” would be more clear to the user.

   Similarly,

   elseif ($trackId <= 0) {
       form_set_error('albacross_drupal_trackid', t('You should not use minus in front of your Track ID'));
     }
   

   This message would show in the case of zero(0) tracking id. So you can change it to like “Non-negative tracking id is not allowed."

Also, a form validation handler is not an implementation of hook_form_validate(), which is not a hook Drupal core uses.

/**
 * Implements hook_form_validate().
 */
function albacross_drupal_form_validate($form, &$form_state) {
  $trackId = $form_state['values']['albacross_drupal_trackid'];
  if (!is_numeric($trackId)) {
    form_set_error('albacross_drupal_trackid', t('You must enter correct Track ID for your Albacross account'));
  }
  elseif ($trackId <= 0) {
    form_set_error('albacross_drupal_trackid', t('You should not use minus in front of your Track ID'));
  }
}

If $trackID cannot contain a floating point number or an exponential number, the following code should be preferable.

/**
 * Form validation handler for albacross_drupal_form().
 */
function albacross_drupal_form_validate($form, &$form_state) {
  $trackId = $form_state['values']['albacross_drupal_trackid'];
  if (((string) (int) $trackId !=== (string) $trackId) || $trackId <= 0) {
    form_set_error('albacross_drupal_trackid', t('The track ID must be a positive integer'));
  }
}

Anyway, those aren't application stoppers, except the first one: A module needs to remove the persistent variables it uses.
It is better to delete the variables one by one with variable_del(), rather than using a database query that deletes all the persistent variables whose name starts with the module name.

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.