[D7] Role expose

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

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 Perttu Helle and thank you for your work!

Pareview / Coder review: NO

http://pareview.sh/pareview/httpgitdrupalorgsandboxrpsu2294717git

FILE: /var/www/drupal-7-pareview/pareview_temp/role_expose.test
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
FILE: /var/www/drupal-7-pareview/pareview_temp/role_expose.test
FOUND 118 ERRORS AND 6 WARNINGS AFFECTING 78 LINES

README.txt/README.md

Follows the guidelines for in-project documentation and the README Template.

Manual review

Not found troubles

Secure code

Not found troubles

Code Style

Add configure = admin/config/people/role_expose in your .info file to make it directly accessible from module page.
You should create separate permission for admin/config/people/role_expose in your page instead of using default administer site configuration. Also you should keep this form in separate admin.inc file.
You have comment than more 80 characters in 1 line (at line 5 - role_expose.module)
You haven't "return" after "switch" into function role_expose_help($path, $arg)

Hi,

1) role_expose_form add a line break before return statement
2) we dont see need a switch statement in role_expose_help.
3) You can change if ($account == FALSE) { to if (!$account) {
4) $render_array['role_expose_user_role'] = array(
'#prefix' => '

',
'#markup' => "

" . t('Provided user (user id @user_id) does not exist.', array('@user_id' => $uid)) . "

",
);

Instead you can try using theme_html_tag
5) role_expose_viewable_roles_list add a line break before the return statement.
6) It is good practice to pass a default value when using variable_get for ex:
variable_get('role_expose_show_role_' . $role_id, FALSE);

Regards,
Prajwal

@rpsu, thank you for your contribution.

It's a simple yet useful module, well written, commented and easy to ready code.

It took me a while to figure out how to show roles in user tab but I found the proper configuration page.
I think it would be better to have a checkbox in admin/people/permissions/roles for each role then having a separate admin page. This is the only suggestion I can give you.
Great work !

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party code

Yes: Follows the guidelines for 3rd party 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. No security issues identified.

Coding style & Drupal API usage

Yes.

Multiple Applications

No.

Thank you again.

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.

Hi @rpsu!

The link is a great idea.

However I've written some code just to show you how I imagine the administration section :)

/**
 * Implements hook_form_FORM_ID_alter().
 */
function role_expose_form_user_admin_roles_alter(&$form, &$form_state) {
  $form['role_expose_field_set'] = array(
    '#type' => 'fieldset',
    '#collapsible' => TRUE,
    '#collapsed' => TRUE,
    '#title' => t('Expose roles in user <em>My roles</em> -tab'),
    '#description' => t('Check all roles you want users to see in their My Roles -tab. By default all roles are unchecked ie. not visible.'),
  );

  foreach ($form['roles'] as $rid => $data) {
    if (!is_numeric($rid) || $rid <= 1) {
      continue;
    }
    $exposed = variable_get('role_expose_show_role_' . $rid, FALSE);

    $form['role_expose_field_set']['role' . $rid] = array(
      '#type' => 'checkbox',
      '#default_value' => $exposed,
      '#title' => $form['roles'][$rid]['#role']->name,
      '#parents' => array('role_expose', $rid),
    );

  }

  $form['role_expose_field_set']['save_button'] = array(
    '#type' => 'submit',
    '#value' => t('Save exposed roles'),
    '#submit' => array('role_expose_user_admin_roles_submit'),
  );
}

/**
 * Submit handler.
 */
function role_expose_user_admin_roles_submit($form, &$form_state) {
  foreach ($form_state['values']['role_expose'] as $rid => $exposed) {
    variable_set('role_expose_show_role_' . $rid, (bool) $exposed);
  }
}

/**
 * Implements hook_form_FORM_ID_alter().
 */
function role_expose_form_user_admin_role_alter(&$form, &$form_state) {
  $rid = @$form['rid']['#value'];
  $form['role_expose'] = array(
    '#type' => 'checkbox',
    '#default_value' => variable_get('role_expose_show_role_' . $rid, FALSE),
    '#title' => t('Expose role in user "My Roles" -tab'),
  );

  $form['#submit'][] = 'role_expose_form_user_admin_role_submit';
}

/**
 * Submit handler.
 */
function role_expose_form_user_admin_role_submit($form, &$form_state) {
  $rid = $form_state['values']['rid'];
  variable_set('role_expose_show_role_' . $rid, (bool) $form_state['values']['role_expose']);
}

you can check by going to roles admin page
http://yoursite/admin/people/permissions/roles
and/or editing a role

^^

Automated Review

Yes.

http://pareview.sh/pareview/httpgitdrupalorgsandboxrpsu2294717git

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.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the

href="https://www.drupal.org/node/2181737">README Template

.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. Not much exposed forms. If "no", list security issues identified.

Coding style & Drupal API usage

Module seems to be Ready for RTBC.

/**
 * Implements hook_form().
 *
 * Role Expose module configuration form.
 */
function role_expose_form($form, &$form_state) {

This is definitely not hook_form()!

    $form['role_expose_show_role_' . $role_id] = array(
      '#type' => 'checkbox',
      '#title' => t('Expose role %role', array("%role" => $role_name)),
      '#default_value' => variable_get('role_expose_show_role_' . $role_id, FALSE),
    );

It would be more efficient -- for this code, and probably for variable memory -- to make this checkboxes and a single array variable.

Role Expose -module

Typo.

  $form['info_text'] = array(
    '#type' => 'item',
    '#markup' => "<p>" . t('Check all roles you want users to see in their <em>My Roles</em> -tab. By deafault all roles are unchecked ie. not visible.') . "</p>",
  );

This is maybe better in hook_help().

      '#tag' => 'div',
      '#value' => "<p>" . t('Provided user (user id @user_id) does not exist.', array('@user_id' => $uid)) . "</p>",

This is a lot of work to handle a user mistake. If a user enters 'user/UID/role' where UID is not a user, a simpler and more correct response is to return a 404. Drupal's menu system has a helper function for that.

function role_expose_user_access($account_id) {

Missing param docs.

      $render_array['role_expose_user_role'] = array(
        '#theme' => 'item_list',
        '#items' => $list,
        '#title' => t('Your site roles'),
      );

Needs a theme function maybe.

BTW, did you consider exposing this on the user profile instead?

Hey the code looks really clear, but here's my two cents:
1) package = "User management" - it's not necessary to wrap package name to double quotes http://cgit.drupalcode.org/sandbox-rpsu-2294717/tree/role_expose.info#n5
2) You have to use drupal variable_del() function instead direct db query in hook_uninstall() http://cgit.drupalcode.org/sandbox-rpsu-2294717/tree/role_expose.install....

In readme there is:

* Customize exposed roles Administration » Configuration » People » Role Expose

but there is no such page.

The actual settings are under Administration » People » Permissions » Roles at the very bottom which was hard to find.

In line 163 of role_expose.module there is:

  if (user_access('view roles of all users') && user_access('access user profiles')) {
    return TRUE;
  }

I think there should be:

  if (user_access('view roles of all users') && (user_access('access user profiles') || user_access('administer users'))) {
    return TRUE;
  }

Because administering users also allows accessing profiles. After changing this +1 for RTBC from me.

Added:
Or rather:

  if ((user_access('view roles of all users') && user_access('access user profiles')) || user_access('administer users')) {
    return TRUE;
  }

Because administering users should give all relevant privileges automatically :) .

You changed path in README to Administration » Configuration » People » Roles , however this is still misleading. The proper path is Administration » People » Roles ( Administration » Configuration » People and Administration » People are two working paths that are leading to something different ) .

Automated Review

Yes.

http://pareview.sh/pareview/httpgitdrupalorgsandboxrpsu2294717git

Best practice issues identified by pareview.sh .

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party 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. Not much exposed forms. If "no", list security issues identified.

Coding style & Drupal API usage

Looks good to me but i found a small glitch that needs to be addressed.
Here is what i did.
1. Enabled the module. Set the permissions.
2. Exposed couple of roles in admin/people/permissions/roles but when i de-select all the roles it throws an error message Notice: Undefined variable: rids in role_expose_user_admin_roles_submit() (line 107 of /opt/lampp/htdocs/testbox/sites/all/modules/role_expose/role_expose.module).

@Swarnendu-Dutta Is there something else that you could find that is blocking the review? Should this be in RTBC?

Only that the notice message comes up mentioned earlier..Dsnt affect with the functionality though..
But needs to be fixed before a stable release.
Looks good to me..
Setting this to RTBC.

Please participate in the review bonus program, so that you get a review from GIT administrator.

Thanks for your contribution.. :)

fixing tag.

manual review:

1. role_expose_user_admin_roles_submit(): Instead of the foreach() loop a simple $rids = array_filter($form_state['values']['role_expose']); should do it.
2. role_expose_user_view_alter(): this is vulnerable to XSS exploits. If I enter a role with the name <script>alert('XSS role');</script> then this will trigger a nasty javascript popup. You need to sanitize all user provided text before printing, make sure to read https://www.drupal.org/node/28984 again. This is not a security issue, since the permission to enter such malicious roles is "administer permissions" which has the restrict access => TRUE flag set. You should still fix this attack vector.
3. role_expose_exposed_list(): I would probably just use array_intersect() with array_keys($account->roles) and the role_expose_show_roles variable.

Although you should definitely fix those issues they are not application blockers, so this looks good to me.

Thanks for your contribution, rpsu!

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.