[D7] Country Specific Nodes

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.

Automated Review

Issues identified by pareview.sh seems to be clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication or fragmentation.

Master Branch

No: Does not follow 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 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

1. (+) Your git clone should be this git clone --branch 7.x-1.x http://git.drupal.org/sandbox/cmak/2290579.git country_specific_nodes instead of this git clone --branch 7.x-1.x cmak@git.drupal.org:sandbox/cmak/2290579.git country_specific_nodes
2. (*) This country_specific_nodes_content_type in country_specific_nodes.module file on line number 89 should be in country_specific_nodes.admin.inc file inside system_settings_form() also the function on line number 131 should be in country_specific_nodes.admin.inc file.

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.

Having the menu/form callbacks in the module file are surely not application blockers, anything else that you found or should this be RTBC instead? And anyone can fix the git clone command in the issue summary.

Thank you for your contribution!

Automated Review

PAReview doesn't complain about any issues on the module. Good job on taking care of that!

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

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. / No: List of security issues identified.

Coding style & Drupal API usage

1. (*) You are setting a variable (country_specific_nodes_def_cn) value using system_settings_form, but you aren't deleting them on module uninstall. Please make sure to delete the variable from the implementation of hook_uninstall().
2. (*) Comments for the function should be present before its definition.
   

   /**
    * Implements hook_node_load().
    */
   function country_specific_nodes_node_load($nodes, $types) {
     // This function is implemented to manage the display of nodes.
     // Based on $node->hidden value e.g. node.tpl.php.
   

   Move the comment above the function declaration, after the line Implements hook_node_load().

3. (*) Inside country_specific_nodes_node_load(), you check if the user has the id 1 to confirm if the user is the administrator. However, there might be many administrators for the site. If this is the intended behavior, add the appropriate behavior, or check for the role in the $user object.
4. (*) You are already creating a field while the module is installed. What is the need to create the field again in the country_specific_nodes_content_type_submit()? Please add a comment. I think only creating the instance of this field should be enough.
5. (*) It would be good if you could call field_cron from inside the if-else loop. Running the cron function even if a field is not created/deleted is an overkill.
6. Please consider to modify the code of this module at an entity level, rather than limiting it to content type level. Since, from Drupal 7 the entities are fieldable, this module would be helpful in various use cases like, for example, hiding some (commerce) products from a specific country. Just a suggestion
7. In country_specific_nodes_content_type_submit(), the array $country_instance is of no use when the user did not select any of the values because it will not go into the if condition where you check the value of $type_val. I would suggest creation of the variable inside the if condition. This will ensure that the code is clean.
8. Please note that field_delete_field(), only marks the field to be deleted and does not delete the field directly. Instead the field is deleted on the next cron run. This is okay, just wanted to let you know about this and let you decide how would you want to handle this. Please see this answer for more explanation.
9. You have placed the form function country_specific_nodes_default_country(), between another function which returns the form country_specific_nodes_content_type() and its submit function country_specific_nodes_content_type_submit(). It makes the code less readable. I would recommend you to move the function somewhere else, so that the form function and form submit function are together. I would think of moving all the configuration related forms to a separate file altogether, country_specific_nodes.admin.inc as suggested earlier as well.

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.

Also, after making the changes, I would suggest to participate in the Review bonus program and someone from the GIT admin team would look at your application right away!

Also, would you comment on why this issue is tagged under "PAReview: Single project promote"? I think this project has code which would qualify you to get the vetted user access as well. Please remove it if this wasn't intentional.

Almost there! Sorry I missed to mention one point earlier. You should consider adding a validate function to the form country_specific_nodes_default_country(), and check if the user enters a valid country code.
I was able to save the value asdf as the country code.
This could be checked by comparison of the value entered with the countries returned by country_get_list() for the list options.
Another option which you could opt for is this, instead of having the user to enter the value in a textfield, you could make a select list for the same with the values of the available countries.

I think this is the only issue which is remaining, which I could find, is a blocker. Other than this looks like RTBC to me.

Awesome!
One last thing, the help text for the select list needs to be changed too. Use something like

Please select the default country for the nodes.

However, this is not a blocker. So, RTBC from my side!
Time to get a review bonus and let a GIT admin look at this :-)

Cool, one more thing which needs change. You will need to change the screenshot of the configuration screen "Default country code page.png" on the project page. It shows the screenshot of the textfield.

Assigning to myself for next review, which will hopefully be tonight or tomorrow.

Automated Review

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

• 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

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

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

Not seeing any issues

Coding style & Drupal API usage

Avoid markup in strings passed to t(), eg in country_specific_nodes_content_type().

Explicitly running cron from country_specific_nodes_content_type_submit() is probably a bad idea. Just let the cleanup happen when cron runs next. Same for country_specific_nodes_uninstall().

(+) In country_specific_nodes_node_load(), you should check for a permission and not a role.

I think some of your is_object($node) calls may be unneeded, but look at the docs again for hook_node_access as the first parameter can be a string representing the node type to check.

$node->hidden may be too generic; you may want to think of a better name to namespace this to your module.

Views support would be nice.

Not sure if a field is most appropriate here. A custom table that maps to the property you define may be better in the long run.

(+) You probably want a permission to bypass the hook_node_access() check.

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.

Not seeing any show stoppers. Assigning to @er.pushpinderrana for a second look, if he has time.

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.

No commit after `1f050cb`, so same Automated Review.

Manual Review

I gone through the whole code, didn't see anything major.

Inline comments and functions docs looks good to me.

Project page also looks good.

I tested module functionality and it worked as intended.

My only real comment would be to take out HTML from t() function. Inclusion of Views and Entity would be a huge enhancement in this module.

Looking at the git history https://www.drupal.org/node/2290579/commits, some commit message are fine but few are too short, you should provide a meaningful short summary what you changed. See https://www.drupal.org/node/52287

Nothing major jumped out at me, so...

Thanks for your contribution, Makarand Chavan!

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.

Awesome!
Thanks MPD and Pushpinder!
Congrats Makarand!