D7 Commerce Postmaster

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxkafmil2088841git

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.

I guess on your version control page you have to unselect maintainer, then copy the git instructions and paste it here.

You are not using the t() function as you are supposed to. For example the translation of your strings will be really difficult, since many of them are broken into substrings. Translator might see no context, and in different languages different word order might be required. I suggest you fix the following pareview warnings:

Pareview report: WARNING | Translatable strings must not begin or end with white spaces,
| | use placeholders with t() for variables

Took a manual look over the code and ran it through the coder module with some errors coming up. The recurring one is on the drupal_set_message not having the $message variable run through check_plain() or filter_xss() for cleaning up before display. This throws a security error which you'll likely want to fix and should be fairly simple to do.

Also, I saw that in the prepareShipment function (line 270) you're referencing a phone field which is not standard in commerce installs and which the user would have to create according to the same machine name you have in order for it to work. I would recommend having that field be added automatically or configured by the user in order to run into issues in the future.

Other than that the coder module just had some minor errors with formatting arrays etc. you may want to clean up and the readme has a few typos but that doesn't stop anything from being approved (just me being overly attentive I guess).

Functionally looks great though and looks like it is a very useful module!

@kafmil

I have been testing your module on pareview.sh and found some issues like :

• Does not follow Drupal naming conventions
• Some unused variables

For detailed view of the issues see http://pareview.sh/pareview/httpgitdrupalorgsandboxkafmil2088841git

Hi Kafmil,
I have reviewed this module, Please use get_t() instead of use t() in .install file.

Thanks,
Yash

Setting to needs review on request of OP

PAReview is still finding issue with the code, please have them addressed.

@kafmil, apologizes. Looks like you got offended with my previous comment. However its always good to follow coding standard. Thou its not an application blocker. There are very few people like Alex who don't have a day job & have dedicated their entire time to Drupal Community. Not just you, but more than 90% of the community people have a day job & they contribute to drupal out of their love for drupal. Since you have pointed me to the module contribution guide page, i would like to point out another item here. Application review bonus gets priority in this queue & administrators expect applications to perform a full manual review to be done at-least for 3 projects, which is missing this application. That might be a reason why this application is open for a long time.

1. Unused Variables : everyone (including you) will agree to the fact that unused variable declaration are good in any program. i believe now you have removed that
2. as you have organized most of you ADMIN form in to a separate inc file its good idea to move commerce_postmaster_admin_form() also to that inc. [This is a suggestion, but not a blocker]
3. Commented code can be removed [Again this is also a suggestion, not a blocker]

@kafmil, you need to wait for a git approver to take the RTBCed issue and grant you access.

What's the Postmaster service?
Are you aware of https://drupal.org/project/commerce_delivery?

You should definitely leave it at RTBC so a git approver sees it!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. These problems should be addressed.
http://pareview.sh/pareview/httpgitdrupalorgsandboxkafmil2088841git

Additional automated review:
See findings.txt for numerous spelling issues (~150) and other coding best practices.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

Place code review here using this format:

• Class CommercePostmasterAdmin
  All function php doc simply copies the function name for the short description. This is not useful.
• delete .gitignore file. Use a machine level .gitignore if you need to ignore .DS_Store.
• (*) shippingOptionsForm should use theme_table. This is repeated through the module for markup. Please use theme_table more often.
• Why can't countryGetList simply be a wrapper for the procedural call to country_get_list?
  

  public static function countryGetList() {
      include_once DRUPAL_ROOT . '/includes/iso.inc';
      $countries = _country_get_predefined_list();
      // Allow other modules to modify the country list.
      drupal_alter('countries', $countries);
      return $countries;
    }
  

• (*) CommercePostmasterAdmin uses a lot of #markup without passing any of the data through t(). Please use t(), and concatenate the markup tags.
• (*) In commerce_postmaster.inc, don't through Exception, use something more specific or create a new exception type. Exception is very hard to catch in a try/catch by other modules except by catching Exception, which is very bad form.
  

  public static function getShipment() {
      if (!count(self::$shipment)) {
        throw new Exception('A shipping item has not been created, Commerce Postmaster is unable to proceed.');
      }

her

• There's a lot of repeated copy/paste of drupal_set_message(t('Please install the postmaster API library. Check the !readme for further information.', array('!readme' => l(t('read me'), 'admin/help/commerce_postmaster'))), 'error');. Try throwing that into a protected/private function so you can simply call $this->setError (or equivalent).
• (*) There's a hidden dependency on entity, commerce_customer, and addressfield. A module should list all direct dependencies.
• There's inconsistent use of entity_metadata_wrapper. Sometimes it is used and sometimes it isn't. Look for all occurrences of LANGUAGE_NONE to see examples where it isn't used and where it could be used.
• (*) Why is this module using its own method to calculate size and weight. Many commerce sites will already have standardized on using Physical. It's OK to use this module's implementation if there is a good reason, but the ideal is to not duplicate module functionality.
• Doing a function_exists check is not necessary since libraries is already a dependency of this module. Also, this should run as a runtime phase check, otherwise the module can't be enabled until the library is downloaded.
  

  function commerce_postmaster_requirements($phase) {
  ...
    if (function_exists('libraries_get_libraries')) {

• No need to explain what a hook_menu does. That's what 'Implements hook_foo' does. This is repeated through the .module file.
  

  /**
   * Implements hook_menu().
   *
   * Creates the menu items required by the module.
   */
  function commerce_postmaster_menu() {

• Its confusing the mechanism you are using to display the admin forms. Admin forms usually goes into module_name.admin.inc. However, with the use of a class for them (as you've done), they you (correctly) have them auto registered in the .info file. But then you don't need to list the file in the hook_menu. Or you could still use the file registration in the hook_menu, but then you'd need to consider that the file won't be autoloaded. Its confusing and non-Drupal like but I don't have a good suggestion for you to make it more Drupal like except to remove the nice classes. Which would be a pity.
• This should start with 'Implements hook_foo'.
  

  /**
   * Function commerce_postmaster_form_commerce_product_ui_product_form_alter.
   *
   * Implements hook_form_alter.
   * Alter the commerce products form to move the shipping options to a vertical
   * tab.
   *
   * @param array $form
   *   Drupal form array.
   * @param array $form_state
   *   Drupal form_state array.
   * @param string $form_id
   *   The id of the form.
   */
  function commerce_postmaster_form_commerce_product_ui_product_form_alter(&$form, &$form_state, $form_id) {

The starred items (*) are fairly big issues and warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

Missed adding findings.txt

Please don't RTBC your own issues, see the workflow: https://www.drupal.org/node/532400

@kafmil, I moved it to Needs work after reviewing in #2145921-30: D7 Commerce Postmaster. The next phase after that is for you to fix things, which it looks like you did. Then the isues gets moved to needs review and we wait for someone to review it and then they will mark as RTBC.

I'll see if I have some time here soon to give it another check. For now, leaving as needs review.

Hi kafmil,
You’ve written a nice module, and here are my comments:

Automated Review

The dsm() call will throw PHP fatal errors on sites on each cache clear, so this needs fixing.

re #40: What is fixed? From my review in #30, 2 of the first bullets are addressed. I'll leave it to you to re-check feedback. Marking needs work for now.

1. .inc: line 500 has two semi-colons in a row. ;;
2. admin.inc: line 1232 missing PHPDoc @param for $field_type
3. .inc: line 714 an object $formats shouldn't be an array key. Potentially related issue on line 218.
4. admin.inc: line 1350 the return value $field is never used.
5. admin.inc: line 1454 the $bundle parameter is never used.
6. .module: hook_permission the permission titles for create and check are identical, this should change.
7. (+) .module: line 129 the form array key should be indexed with the module name so the variable set/get is namespaced with the module name.
8. .install: line 43 isn't necessary since libraries is required by the module in the .info file.
9. .install: Deleting fields on uninstall, while logical, removes data that someone might want to retain even though the rest of the shipping functionality is gone. For example size and weight details. Perhaps this should not be done and a note provided in the README explaining these fields would need to be manually removed. Or check if there is any data in the fields on uninstall and set an error message if data exists.
10. .inc: line 75 consider using watchdog_exception() instead of watchdog. Same on line 156 & 195
11. .admin.inc: line 149 rather than calling theme_table, use #type => table and pass the values as a render array. The current approach makes it nearly impossible for someone else to alter the markup in a form_alter. Same on lines 186, 223, 260
12. .admin.inc: lines 521 and above logic should consider using confirm_form(). I didn't read the code thoroughly to know if this is a good choice. It might not be.
13. (+) .module: _commerce_postmaster_shipping_options_form_Validate & _commerce_postmaster_shipping_options_form_Submit should use lower case spelling. Otherwise the automagic form handlers won't call them.
14. (+).admin.inc: While not a git blocker, this is important! Don't pass #default_values through t(). If someone translated them, then they wouldn't match. Example on line 326 and compare to getLabelFormatArray().
15. (*).info: missing dependency on commerce_product. I installed the module and it is non-functional without that module.
16. (+) hook_help is broken for markdown implementation. If I enable markdown, then no help is available. Please refer to https://www.drupal.org/node/161085.
17. (+) I wasn't able to produce an XSS attack, but depending on how the data is displayed, it is possible.

Moving back to needs work on missing dependency.

Clarifying #43.17: This is in reference to the dimension input fields on the shipping options form.

re #45

4) I use phpstorm and its code quality tools.
11) https://api.drupal.org/api/drupal/includes%21theme.inc/function/theme_ta...

Marking as RTBC.

Assigning to mpdonadio to give this a second set of eyes.

(*) In commerce_postmaster_form_commerce_order_ui_order_form_alter() you should be using #access instead of visible states when you check permissions. The elements will still be there in the form, just invisible, so you are bypassing the access you have set up. If you fix these, I will approve the application.

Oh, and the branch should be '7.x-1.x'. '7.x-1.x-alpha1' would be a tag that you create later.

I will look at this tomorrow.

I am not seeing that change in the 7.x-1.x branch. There are still lots of places you are doing:

// Hide the item if the current user doesn't have access.
$form['commerce_pm_intl_service_level']['#states'] = array(
  'visible' => user_access('manage shipping options'),
);

Make sure you are working in the proper branch, delete the unneeded branch in your local and the remote, and update the clone command above.

Looks nice to me

@Valentine94, did you do a full review? We prefer people to use the Review Template.

Setting this back to Needs Review and assigning to myself as I have to verify #54.

Automated Review

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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.

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.

FILE: /home/matt/PAR/pareview_temp/commerce_postmaster.inc
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  10 | ERROR | [x] Missing class doc comment
 686 | ERROR | [ ] Parameter comment must end with a full stop
 768 | ERROR | [x] The closing brace for the class must have an empty
     |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /home/matt/PAR/pareview_temp/commerce_postmaster.admin.inc
----------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
   10 | ERROR | [x] Missing class doc comment
  544 | ERROR | [ ] Inline comments must end in full-stops,
      |       |     exclamation marks, or question marks
 1251 | ERROR | [x] Tag value indented incorrectly; expected 1 space
      |       |     but found 2
 1252 | ERROR | [ ] Parameter comment must end with a full stop
 1570 | ERROR | [x] The closing brace for the class must have an
      |       |     empty line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 5.24 secs; Memory: 26.5Mb

Manual Review

commerce_postmaster_check_tracking_wrapper() has some untranslated strings, and $message gets overwritten in the foreach. Do another pass for untranslated strings at some point.

In CommercePostmasterAdmin::shippingOptionsForm(), it is best use use a render array rather than generating themed output and stuffing into #markup.

With links in translated strings, it is best to pass in the URL only, not the rendered link. See https://www.drupal.org/node/322774

This is a big, dense module to review. My blocking issue from #52 has been addressed.

This was RTBC by an admin, and my blocker was addressed.

Thanks for your contribution, kafmil !

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.

You don't have to but I always recommend https://www.drupal.org/core-office-hours