[D7] Commerce Sendcloud

Hi Rob,

Thank you for your efforts, I did some manual review of the code however didn't install it yet. Please find my comments:

Automated Review

Review of the 7.x-1.x branch (commit b1f6510):

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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 / No: Does not follow] the licensing requirements.

3rd party assets/code

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

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) You can give the configure link to the module
2. (*) commerce_sendcloud_admin_form(): doc block is wrong, this is not a hook but a form building function. See https://www.drupal.org/coding-standards/docs#forms
3. (*) Use check_plain on line 153 & 190 of commerce_sendcloud.admi.inc and on line 183 of commerce_sendcloud.module
4. (*) Use db_select() instead of db_query() on line 258 of commerce_sendcloud.module
5. (*) Provide default value as you are checking for !emity()
6. (*) commerce_sendcloud_rules_action_info(): doc block is wrong.
7. (*) "variable_get('commerce_sendcloud_key', ','),": all variables defined by your module must be deleted in hook_uninstall().
8. (+) Check curl is enabled or disabled using function_exists() in hook_requirements().

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.

Great, you need to get the review bonus for further review of your module by community members,
see https://www.drupal.org/node/1975228

Hi Rob,

I downloaded and installed this module and got the below issues.

First Issue
======

1. Go to the url '/admin/commerce/config/shipping/services/sendcloud-shipping-method/add'.
2. I got one warning and one notice .See attached screenshot 'sendCloud_add_Service.png'.

Second Issue
========

1. Go to '/admin/commerce/config/shipping/sendcloud'.
2. I got one warning. See attached screenshot 'sendCloud_config.png'.

Solution: Check array count of 'commerce_sendcloud_shipping_methods' before executing loop in line 130 of 'commerce_sendcloud.admin.inc'. The variable $methods should be declared to handle null result.

Please fix these issues.

Congrats! Some minors that are ready to go .I would suggest you to take Review bonus to get the reviews from the Git administers.
Manual Review :
1)
commerce_sendcloud.install : commerce_sendcloud_uninstall : @typo "Delte"
2)

 $items['admin/commerce/config/shipping/sendcloud'] = array(
    'title' => 'SendCloud configuration',
    'description' => 'Manage SendCloud parameters',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('commerce_sendcloud_admin_form'),
    'access callback' => 'commerce_sendcloud_settings_permissions',
    'access arguments' => array('create'),
    'type' => MENU_LOCAL_TASK,
    'file' => 'commerce_sendcloud.admin.inc',
  );

you can pass the "administer sendcloud settings" directly as the "access argument" .

I have done the review of the visual review of the code.
I am not a git admin.I would encourage you to get "Review bonus" to get the review from the git administer.

I reviewed the module:

Line 348:

    foreach ($order->commerce_line_items[LANGUAGE_NONE] as $val) {

Line 423:

  $order->sendcloud_label[LANGUAGE_NONE][] = array(

Line 433:

  $order->sendcloud_tracking_number[LANGUAGE_NONE][] = array(

Have you considered using entity_metadata_wrapper? There is a method "set()->".

manual review:

1. project page is a bit short, see https://www.drupal.org/node/997024
2. There should be only one README file, either README.md or README.txt.
3. commerce_sendcloud_uninstall(): no need to drop your own table, Drupal core will do that for you.
4. commerce_sendcloud_service_permissions(): $op is unused here, should the be a @todo that this access callback will be expanded?
5. "module_invoke_all('commerce_sendcloud_shipping_service_insert', ...": Hooks that are provided by a module should be documented in MODULENAME.api.php, see http://drupal.org/node/161085#api_php
6. commerce_sendcloud_service_save(): so you call the insert hook twice? Shoudl the second module_invoke_all() call be "update" instead?
7. commerce_sendcloud_rules_action_info(): doc block is wrong, this is hook_rules_action_info(). And this hook should be in a *.rules.inc file.
8. "watchdog('SendCloud', 'Error when creating parcel: <pre>' . $e . '</pre>',...": do not concatenate variables into translatable watchdog() messages, use placeholders instead. See https://www.drupal.org/node/28984 . Same for: "watchdog('SendCloud Exception', $e, ...", the message should never be a dynamic variable.
9. commerce_sendcloud_service_edit_page(): if the page callback just returns a form then you can use drupal_get_form as callback in hook menu instead. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
10. commerce_sendcloud_admin_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms

So the wrong watchdog() calls are blockers right now, since you can get XSS security issues easily if you don't use placeholders. Please check all your watchdog() calls, the second parameter should always be a static string with placeholders. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Functional test

Note that my experience with Drupal Commerce is very limited. Some of these remarks might be due to how Commerce operates and are perhaps no fault of the module. I enabled the module and its dependencies on a clean Drupal instance with the standard install profile.

1. The configuration options are in strange locations. There is no entry on admin/config. Instead the configuration can be reached by a local task on the main admin/ landing page. On the main admin/ page there is also a local action "Add a sendcloud service" that leads to another configuration page. It seems very unconventional to use tabs and local actions for settings, and since this is really set-once-and-forget it seems strange to put these in such a prime location. Maybe this is how things are done in Commerce land?
2. When I click on "Add a sendcloud service" I get an error message "Library not loaded". I can find the download link in admin/reports so that's good. It would not be a bad idea to add this information to the installation instructions in the README since manual work is needed, cloning the git repo is not enough, I needed to create a folder 'sendcloud' in sites/all/libraries and move the SendCloudApi.php file in there. It looks like it was the intention to show this info in admin/reports, but a bug is preventing the path where the library should be installed from being displayed properly when the library is not yet installed. As soon as it is installed, the path shows up correctly but then it's too late :) It's fine to fix this later, this does not block acceptance of this project.
3. The Price field is in USD which seems bizarre since the service is European, and the prices on its site are all in euro. I'm assuming this is a Commerce default.
4. I'm not able to create a service. I'm getting "Error when fetching shipping methods" - presumably because I do not have an API key for the service.
5. The configuration options all seem to work as intended. There is no validation on the API key and secret, I could fill in random characters and this was accepted. This is not a problem in itself, wrong credentials will be rejected when API calls are made and can then be communicated to the user.
6. I was not able to figure out how to create a product and send it with sendcloud but that is Commerce's fault.

Code review

The code looks pretty good. Solid work! I have only some very minor remarks.

1. The service is spelled in three different ways throughout the code, also in user-facing strings. Is it "SendCloud", "Sendcloud" or "sendcloud"? It's better to be consistent.
2. Some typos: "Form constructior", "corect", "avaialble", "avialable".
3. As @klausi mentioned above, it is not needed to wrap forms into a page callback for commerce_sendcloud_service_delete_page() and commerce_sendcloud_service_edit_page(). You are correct, just use drupal_get_form() as the callback and pass the arguments in the page_arguments. That should do the trick.
4. The code is very well documented. Everything is easy to follow. Thanks for this!
5. Rules integration seems optional, you could technically remove it from the dependency list in the info file and check if the module is installed before using it. This can be done later.

Conclusion

The goal of the project review is to assess whether a developer is able to deliver good quality modules, following Drupal best practices and coding standards. From the code I can clearly see that this is written by an experienced Drupal developer. The code is well structured, well written and follows coding standards to the letter.

The issues I found are all minor and are fine to be fixed in subsequent beta releases. For me this is OK to be approved as a full project. Setting to RTBC.

Reverted weird changes pfrenssen did in the issue summary.

manual review:

1. hook_menu(): yes, you use drupal_get_form as page callback and array('commerce_sendcloud_service_form', 5) as arguments. You can search core for hook_menu() implementations that do that.
2. watchdog('commerce_sendcloud_rate', 'Deleted sendcloud service <em>%title</em>.' ...: The em tags are wrong here since the % placeholder will already use them. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/format...

Otherwise looks good to me.

Thanks for your contribution, Rob!

I updated your account so you can 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 stay 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.

Reverted weird changes pfrenssen did in the issue summary.

I had some assistance from the hilarious Cloud to Butt Plus Chrome extension. It is mainly intended for random giggles during the work day but(t) sometimes there are some unexpected casualties :D