Commerce USPS

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

remotes/origin/7.x-1.x-dev

Review of the 7.x-1.x branch: Nice work on coding standards, just a minor warning. See http://ventral.org/pareview/httpgitdrupalorgsandboxandyg50001526238git.

Manual review :-

• commerce_usps.module :- Line 68, Your using hook_form_alter() but you could also use hook_form_FORM_ID_alter(), looks better ;)
• commerce_usps.module :- Line 81, 149, 163 ,182 , 195 Use proper comments, add param and return. Refer Doxygen and comment formatting conventions
• Add a .install file to your module to delete all those variables which you are setting in commerce_usps.inc. Refer hook_uninstall.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hi there,

Just had a look at your code. I haven't installed and tested it or run it through ventral yet.

I noticed in this function on line 196 in your main module file:

function commerce_usps_trim_service($service_name) {
return str_replace("^®", "", $service_name);
}

You use double quote instead of single quotes. Was there a reason for this? The double quote slows PHP down a little as it has to parse the strings in double quotes - unless you intended this? The str_replace function should work using single quotes.

In the commerce_usps.xml.inc file, with the values passed to $field_name and shipping_profile, are these values sanitized? (i.e. http://drupal.org/node/28984) Also are the values where you are performing the preg_replace and str_replace in the module file also sanitized? No clue as to what the USPS service code has in it that might do these things.

About to install it on a test instance and see what happens.

Cheers

No problem. Just trying to get the module working on a Drupal 7 instance. I haven't used the commerce module much before. It seemed to install OK. A lot of dependencies. I haven't experienced any errors as yet.

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: ...l-7/sites/all/modules/pareview_temp/test_candidate/commerce_usps.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   99 | ERROR | Line indented incorrectly; expected 8 spaces, found 9
  --------------------------------------------------------------------------------
  
  FILE: ...-7/sites/all/modules/pareview_temp/test_candidate/commerce_usps.xml.inc
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   37 | ERROR | Line indented incorrectly; expected 4 spaces, found 6
   98 | ERROR | Line indented incorrectly; expected 4 spaces, found 6
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. "'description' => t($description),": do not pass variables to t() where possible, as this cannout be detected by translation extraction tools.
2. "$description = 'USPS ' . $available[$service] . ' service.';": you should use placeholders for translatable strings.
3. "module_load_include('inc', 'commerce_usps', 'commerce_usps.xml');": No need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';
4. commerce_usps_service_list(): if that are user facing strings, they should run through t() for translation.
5. "variable_get('commerce_usps_markup')": there is no UI to set this variable? Also you should probably use an appropriate text filtering function, or is the sanitization done elsewhere? See http://drupal.org/node/28984
6. "@param object $order": The order is not a stdClass object, you should use the class name here. Also elsewhere. See http://drupal.org/node/1354#functions the data types section.

But that are just minor issues, otherwise this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Maybe you could also add the configuration page as "configure = " key to your info.

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.