[D7] Block Country

I will review this and get back to you...

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.

As I review this module IP addresses are stored in SESSION variable. The drawback is if I am in another country for eg. originally I was in India and my SESSION is set for India and now I am relocated to US still I am able to see the Block which is set for India.

Removing review bonus tag, you have not done all manual reviews, you just repeated 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.

Automatic Review
This project has some issues in automatic review
ESLint has found some issues with your code (please check the JavaScript coding standards).
block_country.js: line 6, col 2, Error - Use the function form of "use strict". (strict)
block_country.js: line 20, col 4, Error - Missing semicolon. (semi)

Also add the automatic review link in this issue so that others can review it easily.
http://pareview.sh/pareview/httpgitdrupalorgsandboxashishdalvi2552025git
As it is not a blocker but is important for others to review easily.

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 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. Please add the section of "Similar projects and how they are different", if your project is similar to, or otherwise overlaps the functionality of some other project(s).
2. Provide help text in the Drupal UI: At both the end-user and coder/developer level, the Drupal help pages are the obvious place to access information about your module. All but the most trivial modules should implement hook_help().
3. Featured Request: Add functionality to show the block in country for certain duration. This is would help in showing blocks related to events, advertisements, news etc.

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.

Automated Review

[Best practice issues identified by pareview.sh / drupalcs / coder. Please don't copy/paste all of the results unless they are short. If there are a lot, then post a link to the automated review and mention that problems should be addressed.]

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

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.

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. (+) use drupal function module_load_include instead of include_once DRUPAL_ROOT . '/includes/locale.inc';.It is only required for country_get_list() function so it can be in its scope.
2. Just a recommendation
   • $form_state need to sanitized before using with db update.
   • uncheck "visible on all country" when selecting specific country in block configuration. Only a suggestion for enhancement.

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.

$form_state must not be sanitized before saving data with db_update(): "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://www.drupal.org/node/28984

The rest also does not look like application blockers, anything else that you found or should this be RTBC instead?

@klausi,
Rest is tested, I am changing the status to RTBC.

Assiging to myself for second review that may be tonight

Great module.
Review of the 7.x-1.x branch (commit b2a3642):

• 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.
There are few issues which I noticed while reviewing the module-
1. hook_form_alter has been implemented. But the function doc says - "Implements hook_form_FORMID_alter()."
2. Additionally, you will also have to implement hook_form_block_custom_block_delete_alter(hook_form_FORM_ID_alter) in order to delete your module specific configuration for this block,when this block will be deleted.

@neetu morwani: that sounds like useful improvements but are surely not application blockers - anything else that you found or should this stay RTBC?

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

• 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 :
Good points that @neetu.morwani addressed

1. The module implements hook_boot that will give performance issues for large scale websites so specify on the project page about the same.
2. Readme.txt is nice. you can also add maintainers as well as about performance implications in it see https://www.drupal.org/node/2181737
3. block_country_help : Use single quotes where ever possible as per coding standards see http://www.drupal.org/node/1354
4. (*)Implement a hook_form_FORM_ID_alter for block form delete and remove the block country specific configuration.or you can reuse the block_country_form_alter for it.
5. block_country_form_block_admin_configure_submit : you are adding block visiblity settings and removing the records. why not make the
   

       'unique keys' => array(
         'md' => array('module', 'delta'),
       ),

   as unique key in schema and instead of adding and removing the record load the visiblity settings by module and delta uniqueness in hook_form_alter to fetch the data.It will give performance hits. See block_noresults for more information.

6. Use drupal_write_record it will take care of update and insert for you in free of cost.

There are few extra lines in module so remove those.

Nothing major jumped at me, Assiging to klausi to give a final review if he has time.

@naveenvalecha: since this was RTBC already you can simply approve it, not need for a second git admin review.

Read (commit 154b623) :
Manual Review :

1. As per latest js standards we are using single quotes, so use here as well"use strict";

Nothing major jumped at me.Thanks for your contribution.

Thanks for your contribution, Ashish.Dalvi!

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.