[D7] Commerce Purolator Shipping

Welcome,

please get a review bonus first. Then try to fix issues raised by automated review tools: http://ventral.org/pareview/httpgitdrupalorgsandboxcog-axis1800756git

Manual review:
1. Development still in master branch - Please move sources to 7.x-1.x branch (See http://drupal.org/node/1127732 for further details)
2. There is big punch of issue regarding line spaces, indentation and comments raised by automated review tool (See comment #1) - Please take them into account as well (and please configure your IDE to remove trailing spaces etc automatically)
3. README.txt missing - Please add
4. According to Commerce Development Standards, commerce_purolator_shipping.admin.inc should be located under /includes -folder (Please see http://www.drupalcommerce.org/developer-guide/development-standards)
5. There is number of variables used but no hook_uninstall defined to remove module specific variables (Please use variable_del() in hook_uninstall() to remove variables or use delete sql query).
6. commerce_purolator_shipping.module includes number of functions from Purolator Public Web Services Client Creation Code
- Have you checked that Purolator Public Web Services can be distributed part of module? Does GPL license apply for them as well?
- Perhaps you could move Purolator Public Web Services functions to a file of its own and add proper disclaimers and links to original content of functions?

7. Is inclusion of wsdl files needed? Aren't those available through web all times?

Finally, if you have reviewed up to three modules, please add PAReview: review bonus issue tag (after corrections done) along with PAReview: Commerce issue tag.

Hello,

I have installed the module and had a quick initial look. Other than the points raised above by Mitrpaka, I have a few suggestions below.

• Could points 1-4 of your install instructions (adding the weight field) be automated in an install hook?
• Could any of the additional API services be exploited by your module, e.g tracking?

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Updating git command mentioned in description.

I guess your application needs review? See the workflow: https://www.drupal.org/node/532400

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/1867612

Project 2: https://www.drupal.org/node/2365051

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Automated Review

n/a

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. There are weird question mark characters in the document that should get fixed.

�Shipping UI�

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: Does not meet the security requirements.

• (*) XSS and other badness comes when unfiltered text gets printed to the screen. Never trust text that you don't have complte control over. In .module file the response from calling the web service is printed to the screen unchecked: form_set_error('name', $response->ResponseInformation->Errors->Error->Description); Check if there is anything else like that in the code.

Coding style & Drupal API usage

Indentified issues in no particular order.

1. admin.inc: Duplicate array key:
   

     $form['packaging']['commerce_purolator_shipping_packaging'] = array(
       '#type' => 'radios',
       '#title' => t('Select Purolator Packaging Type'),
       '#default_value' => variable_get('commerce_purolator_shipping_packaging'),
       '#options' => array(
         'CustomerPackaging' => t('Customer Packaging'),
         'CustomerPackaging' => t('Customer Packaging'),
         'ExpressBox' => t('Express Box'),
         'ExpressEnvelope' => t('Express Envelope'),
         'ExpressPack' => t('Express Pack'),
       ),
     );

2. (+) variable_get variable name (province) is spelled wrong: commerce_purolator_shipping_povince
3. (+) variable_get variable name (registered) is spelled wrong: commerce_purolator_shipping_registerd_account
4. (*) Missing dependency on libraries api with several calls to libraries_get_path().
5. (+) .module file: Variable 'response' might have not been defined (at line 290)
6. (+) .module file: Variable 'weight' might have not been defined (at line 231)
7. .module file: Unused local variable $client (at line 253)
8. commerce_purolator_shipping_settings_form should use system_settings_form() with a validation handler to handle the password logic. No need for the custom submit handler.
9. Rather than storing a duplicate list of country codes, load iso.inc and call _country_get_predefined_list() from Drupal core. That list is regularly maintained. Or if Puralator only ships to select countries, unset the appropriate countries from that list.
10. select lists should use an empty option and empty value, rather than including a '--'. See https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...
11. hook_menu: Don't use capital letters for path names. I couldn't find anywhere it says not to, but no one does and since no one does, it will confuse people who manually type the path.
12. Spelling errors, Plese

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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