Shortcut per Role

Looks like it could become a standard module for many developers - very simple and fills in a gap in Drupal's core functionality.

Some small things:

• A README.txt file should be included which explains the purpose behind the module, how it works and how to configure it.
• The shortcut_per_role.info file shouldn't have a space between "files" and the brackets, as evident in the first "files" line.
• Missing an initial comment in shortcut_per_role.install to indicate what it is used for.
• You shouldn't add hook_install() if it isn't needed.
• In shortcut_per_role_uninstall() you name the variable $role_names and then $role_name, but it is storing the role's 'rid' value, not the role name; you might want to change these variables to just "$roles" and "$role".
• The custom permission should be named "administer shortcuts per role" (plural "shortcuts").
• You shouldn't add hook_help() if it isn't being used.
• In shortcut_per_role_default_set() the return statement should not prefix "$default =" in front of the second argument, it should just be variable_get('shorcut_per_role_' . $role_highest_name . '_set', 'shortcut-set-1')
• Comments need a space after the two slashes, need to start with a capital letter, end with a period and wrap at 80 characters.
• In shortcut_per_role_user_admin_role_submit_alter() the code to identify the role being deleted has been commented out, so this will not work.
• I'm not overly certain that shortcut_per_role_default_set() is the correct approach for this, or at least you should include an explanation of this in the README.txt file.
• In shortcut_per_role_default_set() it accepts the variable "$account" as an argument but then you use the variable "$user" in the query.
• The query in shortcut_per_role_default_set() would work better as a db_select() with a condition to find the matching roles, or just don't query the list at all and just work directly with the $account->roles array.

So, well done on building something for this gap in Drupal core functionality and with a little more polish this will be ready for a full project.

Some further items I saw:

• shortcut_per_role.admin.inc needs a comment at the top to describe the file.
• The shortcut_per_role_admin_form() function needs a comment.
• In shortcut_per_role_admin_form(), the $sets query should list the fields you want and not use "*".
• In shortcut_per_role_admin_form() you could probably move the per-role title into the form element as a prefix rather than as a whole extra element.
• If you change shortcut_per_role_admin_form() to use the variable names as the array element you could then use system_settings_form() and not have to have the submit function at all, e.g.:
  

  function shortcut_per_role_admin_form() {
    $roles = db_query('SELECT rid, name FROM {role}')->fetchAllKeyed();
    $sets = db_query('SELECT set_name, title FROM {shortcut_set}')->fetchAllKeyed();
  
    foreach ($roles as $rid => $role) {
      $role =  str_replace(' ', '_', $role);
  
      $form['shorcut_per_role_' . $role . '_set'] = array(
        '#type' => 'select',
        '#default_value' => variable_get('shorcut_per_role_' . $role . '_set', $default = 'shortcut-set-1'),
        '#options' => $sets,
        '#prefix' => '<h3 class="title">' . t('Assign Shortcuts for ' . $role . ' Role') . '</h3>',
      );
    }
    $form['create_new_ss'] = array(
      '#markup' => '<h3 class="title">' . l(t('Add a shortcut set'), 'admin/config/user-interface/shortcut/add-set') . '</h3>',
    );
  
    return system_settings_form($form);
  }
  

• I now realize that the query in your hook_uninstall() should get the 'name' field and not the 'rid'.

Nearly there, a good improvement from your first code commit.

A few additional things:

• The query in shortcut_per_role_shortcut_default_set() really needs to be rewritten to use the db_select() syntax of Drupal 7, and pass the array_keys() of the $account->roles array as a condition rather than manually inserting it into the query. I would consider this to be the most important issue still remaining.
• You have some misspellings, e.g. "requirements", "configuration", "shortcut", maybe run the README.txt file through a spellchecker?
• Ensure that all comments and text files are wrapped at 80 characters.
• Make sure the tabs are correctly set to two spaces, it's a little erratic in some places where lines are incorrectly indented.
• There's a redundant line in shortcut_per_role_admin_form():
  

    $sets = array_keys(shortcut_sets());
    $sets = shortcut_sets();
  

  The first line can be removed.

Good work!

Make sure that your comments and the README.txt file wrap at 80 characters, e.g the README.txt file currently has:

* Go to admin/config/user-interface/shortcut-per-role and assign the shortcut set for roles

This should be:

* Go to admin/config/user-interface/shortcut-per-role and assign the shortcut
set for roles.

Also:

• The code in shortcut_per_role_user_role_delete() is indented by one extra space,
• The indentation is wrong in shortcut_per_role_uninstall(),
• In shortcut_per_role_admin_form() there should be extra spaces around the = sign in the following line:
  

  $sets=shortcut_sets();
  

• In shortcut_per_role_admin_form() the first foreach() block is indented incorrectly.

Another thing is that the wrapped lines on the $query in shortcut_per_role_shortcut_default_set() should be indented, and maybe simplified a little, i.e.:

  $query = db_select('role','r')
    ->condition('rid', implode(',', array_keys($account->roles)))
    ->fields('r', array('title'))
    ->orderBy('weight', 'DESC');

A few other small, quick things:

• In the .info file add a setting called "configure" with the value set to the module's settings page, e.g.:
  

  configure = admin/settings....
  

• All comments should have a period at the end of them, just line normal sentences.
• The trinary operator in shortcut_per_role_shortcut_default_set() would read more easily if it was replaced with a normal if() statement.
• There's an assignment in shortcut_per_role_user_role_delete() that's missing a space either side of the equals sign.
• You should probably move the variable_set() into the if() statement in shortcut_per_role_user_role_delete(), to avoid running any extra SQL queries if they aren't needed.
• The comment in shortcut_per_role_shortcut_default_set() needs to have a space after the two forward-slashs, the first letter should be a capital and you don't need the extra periods, one is fine.

Looks like ready to go then,

Please fix the open bug and set this back to "needs review."

This looks good to me.

babbar.ankit, please add more description to your project page. See: http://drupal.org/node/997024

5 files[] = shortcut_per_role.module
6 files[] = shortcut_per_role.install

These should be removed. It's only necessary to declare files[] if they declare a class or interface.

Thanks for your contribution, babbar.ankit! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.