[D7] Hierarchical Select Access

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

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.

Hello ,
I tried to clone this module on my local machine. It prompt me to enter your Drupal.org passowrd.
Issue:
siddharth@siddharth:~$ git clone --branch 7.x-1.x drupalove@git.drupal.org:sandbox/drupalove/2448299.git hierarchical_select_access
Cloning into 'hierarchical_select_access'...
drupalove@git.drupal.org's password:
Permission denied, please try again.

Please make sure you have given correct git repository link. Instructions:
You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".

Reference : https://www.drupal.org/node/1011698.

Hi
I have go through the module repository. Following are my insight:

hierarchical_select_access.install
Remove drupal_install_schema('hierarchical_select_access'); as in Drupal7 you do not need to explicitly call drupal_install_schema(). @see: https://www.drupal.org/update/modules/6/7

hierarchical_select_access.module

- It is recommended to declare global variable on top inside a function.
global $user; should be on first line in function hierarchical_select_access_form_node_form_alter(&$form, &$form_state) {

- You can use PHP empty() function instead of hierarchical_select_access_is_array_empty(array $arr) to reduce the extra loop iteration.

Automated Review

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

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 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 README Template.

1. The introduction should include a link to the project page and issue queue. If the project is a sandbox, these links should go to the sandbox until promotion.
2. Please link to the HS project page in the Requirements section
3. Configuration can be in a separate section
4. Installation instructions can be abbreviated to "Install as you would normally install a contributed Drupal module. See: https://drupal.org/documentation/install/modules-themes/modules-7 for further information."

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. In the info file: "Allows certain roles to access hierarchy select in pre-specified content types." is unclear, change to comething like "Limits access to Hierarchical Select Menu selector for specified roles and content types."
2. 'title' => 'Access', <-- Title could be more descriptive
3. $role_ids = variable_get('hierarchical_select_access_selected_role'); and $node_types = variable_get('hierarchical_select_access_selected_content'); <-- consider providing a default value such as array() for clarity. Consider describing the format of these variables as if they were in a docblock (ie. provide an example)
4. @todo This must be handled in a #process handler. <--- I don't see any proces handler yet :)
5. 'mlid',
   'module',
   'hidden',
   'has_children',
   'customized',
   'options',
   'expanded',
   'hidden',
   'parent_depth_limit', <--- don't hardcode these, other modules may add further link data.
6. Select the roles that should not see the hierarchical select menu <-- Select the roles that should not see the Hierarchical Select widget
7. Select the content types on which the hierarchical select menu will not be used. <-- Select the content types on which the Hierarchical Select widget will not appear in the node form.
8. The settings below will be used as site-wide defaults. <-- superfluous information, consider removing this
9. Configure site-wide settings for the Hierarchical Select Access. <-- spelling. " the Hierarchical Select Access module"
10. (*) This module is about the "Hierarchical Select Menu" (hs_menu) widget and not Hierarchical Select in general..
11. (*) As this is an access control module, consider disabling access by default and enabling only if certain conditions are met. Right now there are too many conditions under which the HS widget may accidently appear, as it appears by default and only hides if conditions are met.

12. // Shift the first value of the array to unset the submit callback
              // added by Hierarchical Select.
              array_shift($form['#submit']);

    <--- we don't know that the first value is the hierarchical select submit callback necessarily! Check for it specifically (unset any callbacks that start with "hs_").

13. (*) How do we ensure that your form_alter runs after HS's form_alter?

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.

Hi Yousif, I will try to look at this tomorrow. In the mean time, consider getting a PAReview bonus so one of the admins can promote this project once it's ready!

OK this looks ready to me, just a few small points:

1. The README was accidently reverted to the old version in one of the commits: http://cgit.drupalcode.org/sandbox-drupalove-2448299/commit/?id=90e3235
2. As to #12, that would be correct, but what if another module does array_unshift as well? Just to be sure, better to do:
   

   foreach ($form['#submit'] as $key => $value) {
       if ($value == 'hs_menu_node_form_submit') {
           unset($form['#submit'][$key]);
       }
   }
   

3. ('The settings below will be used as site-wide defaults.') is still there
4. Select the roles that should not see the Hierarchical Select widget. <-- has two spaces between "the" and "Hierachical Select"

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

manual review:

1. hierarchical_select_access_help(): the check_plain() and filter_xss_admin() is wrong here since no user provided text is involved. The README contents is statically provided by you, so no XSS threat.
2. hierarchical_select_access_admin_form(): doc block is wrong, this is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms
3. hierarchical_select_access_admin_form(): this could use system_settings_form(), then you don't need the submit handler.

But otherwise looks good to me, so ...

Thanks for your contribution, drupalove!

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.