[D7] VoiceCommander

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.

There is no functionality related to error reporting to end-user. For example if command is missing or don't allowed - user can't see any indications or errors on page.

Also there is no command for clearing cache.

Added handlers and set messages for errors, wrong commands or unexpected situations.

Hello Yuriy, Yaremiy, Alexandand, a nice contribution indeed, I hope a functionality like this can become quickly available for the Drupal community. At the moment I found many issues during the installation and usage process. My suggestions follow:

Automated Review
⊠ Git default branch is not set, see the documentation on setting a default branch.

Individual user account
☑ Follows the guidelines for individual user accounts.

No duplication
☑ Does not cause module duplication and/or fragmentation
Except the unmantained https://www.drupal.org/project/speech I see no similar solutions available for the community.

Master Branch
✎ Follows the guidelines for master branch. But a default branch is not set up, please check the pareview output.

README.txt/README.md
✎ The content itself is ok and follows the guidelines for in-project documentation and/or the README Template.
Just take care the underlining must everywhere be as long as the title.

Installation / Usage
⊠ I experienced issues during the installation with drush:
Fatal error: Call to undefined function drush_voicecommander_download() in /home/drupalpro/websites/merda.dev/sites/all/modules/custom/voicecommander/drush/voicecommander.drush.inc on line 113

After the installation the website warns you constantly:
VoiceCommander: plugin "Annyang" or file "annyang.min.js" is not exist in libraries folder.
Instead, once the module is installed I would send a notification to the user to warn him to install the libraries, providing links as support.

Also after having installed the warning remains until a cache clearing is performed.

While using it, it always appear the Allow Deny dialog box, which is annoying, and anyway it doesn’t catch what I say, even selecting all the menu. I am not sure which command list Is available, I would highlight it somewhere, because right now I couldn't perform any command. This was the result for me more or less: http://youtu.be/lu88J5JL8Hw

Project Description
⊠ The title of this issue is wrong, it should be [D7] VoiceCommander instead.

The git command is wrong,
git clone --branch 7.x-1.x yuriy.kostin@git.drupal.org:sandbox/yuriy.kostin/2385113.git voicecommander
should be instead
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/yuriy.kostin/2385113.git voicecommander

Also, you should provide the pareview link in the issue page to make the review process easier.
in this case: http://pareview.sh/pareview/httpgitdrupalorgsandboxyuriykostin2385113git

Good Luck with this amazing module!

Hello Andrea and thank you for your review of our first module. We have reviewed your comments and made next fixes and updates;

1. Automated Review: Default branch set. Now it is 7.x.-1.x
2. Master Branch: Master branch set. Now it is 7.x.-1.x
3. README.txt: Fixed underlines length.
4. Installation / Usage: Update user notification in case if library is not installed. Fixed drush post-install behavior: now we automatically clean cache after install. Soon we will provide highlighting of allowed commands functionality. Now we have next default commmands:
   • Cache clear: 'drupal cache'
   • Management menu: '[Administartor command prefix] [Management menu item]', where [Administartor command prefix] is an option that set at /admin/config/user-interface/voice-commander and [Management menu item] is an any menu link from management menu /admin/structure/menu/manage/management
5. Project Description: Updated project documentations.

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. (*) On voicecommander.module comment refer to Colorbox module
2. (+) Private function have missing docs.
3. (+) voicecommander.module, on line 75 there is drupal_set_message where the URL is defined above, and is replicate. User placeholder to replace it.
4. (+) voicecommander.module, on line 97 and 98 there is variable_get without default value.
5. Are you sure you need to expose the CC all through the AJAX callback?
6. On JS side: typeof (annyang) !== 'undefined' can be undefined !== annyang
7. (+) On JS side: config with hardcoded string is bad, please export it using drupal_add_js('settings', ..) and load this value drop Drupal.settings.voicecommander.....
8. (+) On JS side: limitate all DOM interaction to context param eg $('.my-selector', context).

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.

This review uses the Project Application Review Template.

PS: nice work!

This looks really cool!

I had a few teething problems though:

- Chrome asks for my permission to use the microphone every single time I held down CTRL+ALT. This makes it really hard to use.
- None of my commands were recognized at first. It's not clear from the readme what sort of things I can say. Some examples would be good, eg: 'Drupal Content Types', 'Drupal Modules', etc.

On to a code review:

There are a few mentions of another module, such as:

/**
 * The default path to the Colorbox directory.
 */

Repeating the name of the function doesn't count as documentation. You should state what the function does:

 * Custom function _voicecommander_base_menu_recursive().

You also need to document the @params and @return.

function voicecommander_settings_form() {

Form builder functions have standard params which are missing here.

Providing a drush command is a very nice touch, though it does this:

$ drush vc
Command vc needs the following module(s) enabled to run:             [error]
voicecommander.
The drush command 'vc' could not be executed.

The README should have instructions on where to get the library from.

> Select menu for commands *(Management menu use default)

I don't understand what this UI text means at all.

The module description in the info is rather long -- some of that should go in a hook_help().

function _voicecommander_cc_all() {
  drupal_flush_all_caches();
}

Not sure what this is for...
If you want to provide a cache clear command, would the admin_menu menu item not do?

BTW, please don't tag issues with random keywords! Read the guidelines that are linked on the tag field.

> Also we have already described this behavior in readme file and module description

I eventually found that, but it's nested in the README several folders down. Would be better in the top-level README.

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.

This is breaking normal JS behaviour on my development site:

- I am working on https://www.drupal.org/project/reference_option_limit, and with VC enabled the JS on the field widget only works the first time.
- Module Filter doesn't work at all

Hello, Joachim and thank you for your time to view our project. We figured out the reason of error and have fixed it. For both cases described by you it was same root cause.

manual review:

1. "$t('@e', array('@e' => $error_type)),": that translatable string is not helpful, what would be a meaningful translation if it only consists of the placeholder? Just use check_plain() on $error_type here.
2. voicecommander_preprocess_page(): since you are not using $variables here you should use hook_page_build() instead to add your JS. And there you should use #attached on the $page render array instead of drupal_add_js(), see https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...
3. There is a security issue in this module, so I'm assigning this to naveenvalecha as part of our git admin security training program. If Naveen or Yuriy is not able to find it I will post the details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

JS: window.href uses / instead of Drupal.basePath or whatever it is called.

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

voicecommander.module : function _voicecommander_cc_all : drupal_flush_all_caches() is very heavy operation.As I have seen that you are calling from the voicecomamnder.js file.For long term, I would suggest please specify about it on the project page and also add the comments in the code.

  $items['cc_all'] = array(
    'title' => 'clear cache',
    'type' => MENU_NORMAL_ITEM,
    'page callback' => '_voicecommander_cc_all',
    'access arguments' => array('access voice commander settings'),
  );

Do we need the menu type "MENU_NORMAL_ITEM" . Please add a comment.
Also I suggest that menu path would be 'admin/config/....' instead of "cc_all" menu item path to . Its just suggestion.

2)
use Drupal.settings.basePath instead of / as @klausi addressed above.

3)
voicecommander.admin.inc function voicecommander_settings_form : Spelling issue, "administartor".
Security Issues :
1)
voicecommander.module function _voicecommander_base_menu_recursive: This function has an access by pass problem.So, users may see node titles and other menu entries that that may not have access to, which is a security problem. You need to call menu_link_load() on each item and then check access. This step will also translate the menu title.
Similarly in the _voicecommander_menus_recursive function.
2)
voicecommander.admin.inc function voicecommander_settings_form : The menu_get_menus function is displaying the Menus in the select list. This is displaying all the menus here.So this function will also display the custom menus to the user with "access voice commander settings" can see all the menus type even the user does not have "administer menu settings" permissions which seems a light security issue, so need some access check here as well.
Edit : Addressed by @Klausi to me on IRC. Thanks :)
3)

  $items['cc_all'] = array(
    'title' => 'clear cache',
    'type' => MENU_NORMAL_ITEM,
    'page callback' => '_voicecommander_cc_all',
    'access arguments' => array('access voice commander settings'),
  );

The path is also vulnerable to CSRF exploits.so either needs a confirmation form / security token in the url.See http://epiqo.com/en/all-your-pants-are-danger-csrf-explained for more information.

I don't think it is necessary to call menu_link_load() on every menu item, the API function should return the #access key for each item which indicates whether the user is allowed to see the item or not.

As I discussed with Naveen on IRC another security issue is the cc_all path. This is vulnerable to CSRF exploits because you are performing write operations without a confirmation form or a security token in the URL. See http://epiqo.com/en/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c...

Hello Naveenvalecha and Klausi,

About:

Security Issues :
1)
voicecommander.module function _voicecommander_base_menu_recursive: This function has an access by pass problem.So, users may see node titles and other menu entries that that may not have access to, which is a security problem. You need to call menu_link_load() on each item and then check access. This step will also translate the menu title.
Similarly in the _voicecommander_menus_recursive function.

Function menu_tree_all_data() is already have "security check". Look at include/menu.inc (line 1150 and line 1355).
I see no reason to double check for access.

Same as for

2)
voicecommander.admin.inc function voicecommander_settings_form : The menu_get_menus function is displaying the Menus in the select list. This is displaying all the menus here.So this function will also display the custom menus to the user with "access voice commander settings" can see all the menus type even the user does not have "administer menu settings" permissions which seems a light security issue, so need some access check here as well.

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

1. voicecommander_preprocess_page(): since you are not using $variables here you should use hook_page_build() instead to add your JS. And there you should use #attached on the $page render array instead of drupal_add_js(), see https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...
2. _voicecommander_cc_all_form(): The organization I worked with we usually add the prefix to functions which are the helper function and will be callled in module internally.
3. _voicecommander_cc_all_form(): $_GET['destination'].$_GET is fine but drupal_get_query_parameters function is preferable.
4. _voicecommander_cc_all_form_submit(): drupal_goto(); breaks the other processing because it do 30X so $form_state['redirect'] is prefferable.

5.  $items['admin/config/user-interface/voice-commander/cc_all'] = array(
       'title' => 'clear cache',
       'access arguments' => array('access voice commander settings'),
       'page callback' => 'drupal_get_form',
       'page arguments' => array('_voicecommander_cc_all_form', 1),
       'access callback' => TRUE,
       'type' => MENU_CALLBACK,
     );

   The second page argument is not using in the callback function ? If not then simply remove.

6. It looks _voicecommander_cc_all_form_submit will not be called using Js function clearCache because the confirm form will not be clicked automatically using js call at the menu path.Please add a comment ?

Waiting just for my last point.Otherwise RTBC :) Thanks for your help in reviewing other project applications and patience in the Project applications review process.

Repeating myself from the last review:

• "$t('@e', array('@e' => $error_type)),": that translatable string is not helpful, what would be a meaningful translation if it only consists of the placeholder? Just use check_plain() on $error_type here.

+ the things Naveen mentioned. But that are not application blockers, otherwise looks RTBC to me.

Assigning to Dave Reid as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, yuriy.kostin!

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.