[D7] Commerce NZPost

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.

Hello d-nz

My manual review:

.install

lines 11ss

function commerce_nzpost_uninstall() {
  $fields = array(
    'commerce_nzpost_api_key',
    'commerce_nzpost_services',
    'commerce_nzpost_default_package_size_length',
    'commerce_nzpost_default_package_size_width',
    'commerce_nzpost_default_package_size_height',
    'commerce_nzpost_rates_timeout',
  );

  foreach ($fields as $data) {
    variable_del($data);
  }
}

Extra code. You create an array and later a foreach to delete the variables. You can changed to

function commerce_nzpost_uninstall() {
  variable_del('commerce_nzpost_api_key');
  variable_del('commerce_nzpost_services');
  variable_del('commerce_nzpost_default_package_size_length');
  variable_del('commerce_nzpost_default_package_size_width');
  variable_del('commerce_nzpost_default_package_size_height');
  variable_del('commerce_nzpost_rates_timeout');
}

less codelines and less cpu use.

.module line 80

module_load_include('inc', 'commerce_nzpost', 'includes/commerce_nzpost.json');

Your are not include a file of another module so use require_once insted

require_once realpath(__DIR__) . '/includes/commerce_nzpost.json';

The ventral errors:

I see that you have the description in multiple lines, you can probe to create a variable with the text and concat the diferent lines with . or put all the description in one line.

Hello.

I have to correct one of the points of my review. Sorry. In the review process of a module of mine told me that instead utilizase module_load_include required, so I adopted that point as a correction to make.

My module has passed the review process, but the person who enabled the publication of the project (cweagans) commented as follows on the subject:

For this bit:

 389 /**
 390  * include the block functions file
 391  */
 392 require_once realpath(__DIR__) . '/pay_with_a_tweet.blocks.inc';
 393 
 394 /**
 395  * include the tokens functions file
 396  */
 397 require_once realpath(__DIR__) . '/pay_with_a_tweet.tokens.inc';

1) To load module include files, you should use module_load_include; and
2) If you're including this unconditionally, there's no reason to split them out into another file. It is, in fact, a bit slower, since you're adding a couple of disk reads/file open operations into the mix. You could probably just copy the contents of those files into your .module file and you'd be fine.

I understand that, in this case, it is better to follow the recommendation of a person who can enable publishing projects.

Hello Andy!

I've installed commerce_nzpost on standart Commerce Kickstart profile. I used '1234-5678-90ab-cdfe' as an API key.

After settings submission there is Notice:
Undefined index: commerce_nzpost_postal_code in commerce_nzpost_settings_form_submit() (line 154 of /.../sites/all/modules/commerce_nzpost/includes/commerce_nzpost.admin.inc)..

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

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

  
  FILE: /home/klausi/pareview_temp/includes/commerce_nzpost.admin.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   84 | ERROR | String concat is not required here; use a single string instead
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

manual review:

1. commerce_nzpost_settings_form(): $markup is user facing text and should run through t() for translation?
2. commerce_nzpost_api_request(): why do you use check_plain() here? The API key is never printed to HTML in this function? Same in commerce_nzpost_build_rate_request(), where the return value is also never printed to HTML? Make sure to read https://drupal.org/node/28984 again.

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

Assigning to MiSc as he might have time to take a final look at this.

You should remove the .gitignore file from the repo
There are a couple of minor errors reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxdnz2052157git

Otherwise looks pretty good, nice work!

Thanks for your contribution, d-nz!

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.