[D7] Commerce Shipping Country

Multiple Applications

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

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

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

Project 3: https://www.drupal.org/node/2398025

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.

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.

Module installation properly done without any error and code also look good.
But need to work on below points.

1. http://pareview.sh/pareview/httpgitdrupalorgsandboxmihaibrb2308211git
The variable naming is a blocker right now
2. Need to improve README.txt file in more detail.
Follows the guidelines for in-project
documentation and the README Template

I think would be better to use

define('COMMERCE_SHIPPING_COUNTRY_ADMIN', 'admin/commerce/config/shipping-country-limit');

rather

/**
 * Helper function to return the Payment Shipping Country admin path.
 */
function commerce_shipping_country_get_path() {
  return 'admin/commerce/config/shipping-country-limit';
}

because it's not best way for using strings values.

Added the project link

Automated Review

pareview.sh: variable naming, as mentioned before (https://www.drupal.org/node/2398043#comment-9466021)

Manual Review

No duplication

No: Does not cause module duplication and/or fragmentation. Although addressfield module has the same functionality, this module adds a possibility to change only Available countries based on user role.

Master Branch

No: Does not follow the guidelines for master branch. Master branch already exists.

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 the README Template. Configuration and usage how to would be very necessary.

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

1. (*) info file defines faulty module configuration interface: admin/config/commerce-shipping-country, but this url does not exists. The correct url is admin/commerce/config/shipping-country-limit, therefore the configuration interface is not accessible through the modules page.
2. I think using an own function to return administration interface path is not a drupal friendly and elegant solution. I prefer to see full path in hook_menu. I think these values do not change as frequently that it would be worthwhile to create a function for it.

This review uses the Project Application Review Template.

Comment and code don't agree:

 * Helper function to build a list of customer profile bundles.
 */
function _commerce_shipping_country_get_addressfields($bundle = NULL) {

Also, missing function docs (as well as a few other functions).

  $items['admin/commerce/config/shipping-country/edit'] = array(
  $items['admin/commerce/config/shipping-country/settings'] = array(

Seems an odd choice of paths, given both are forms that let you change settings!

  variable_set('commerce_shipping_country_customer_bundle', 'shipping');
  variable_set('commerce_shipping_country_customer_addressfield', 'commerce_customer_address');

It's not usual or necessary to set variables on install. Rely on the defaults you pass to variable_get() instead.

I'm also not sure where this module actually does its work to limit the countries -- all I see is settings forms.

I also have a suggestion regarding configuration storage: you could mark the field instance itself, add a checkbox in the field admin UI, and then detect which field to work with using FieldInfo API. That would simplify the UI for the user, and also make the setting exportable through Features as part of the field instance itself.

fixing tag

Looks good, thanks.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I removed the automated reviews from the issue summary.

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

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

  FILE: /home/klausi/pareview_temp/commerce_shipping_country.admin.inc
  ----------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ----------------------------------------------------------------------
   134 | ERROR | [x] Whitespace found at end of line
  ----------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ----------------------------------------------------------------------
  

• 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.

manual review:

• project page is a bit short, see https://www.drupal.org/node/997024 . What is the use case? Do I need the Commerce Shipping module as dependency? You should probably mention that this module does not do anything except providing an alternate UI to configuring the address field?

But otherwise looks good to me, so ...

Thanks for your contribution, mihai_brb!

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.