Short Description

Forcing the user to choose one role after login, and led to other roles
does not work. User can switch role anytime just visit select_role/landing.

Description

This module is useful if your site have differences significantly related
layout and menu item on each role.

For example, you have a website of journal system, which has roles: author,
editor, and reviewer. Sometimes there is one user who has three roles at once.
If that user login, you want that user has to choose one role only. As soon as
the user login, the user must choose one role. After one role is selected,
then other roles will be removed for a while. So, if he/she selects a role as
an author, then the permission and access at the time he role as a reviewer
will not work.

This module uses hook_boot to remove unused roles from $GLOBALS['user']->roles.

Steps to test this module

  1. Login as user#1, then make roles: author, editor, reviewer, lecturer, student, administrator, and reader.
  2. Visit: admin/config/people/select_role, then check for author, editor, and reviewer, then save.
  3. Create one user called desi. give her role of author and reviewer. Save.
  4. Give access to the role author which can create article.
  5. Give access to the role reviewer which can create basic page.
  6. Logout from user#1, and login as desi. Now you will see the effect of this module. Visit node/add

Project Sandbox link

https://www.drupal.org/sandbox/m_roji28/2330439
http://git.drupal.org/sandbox/m_roji28/2330439.git

Git clone link

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/m_roji28/2330439.git select_role

Pareview.sh passed

http://pareview.sh/pareview/httpgitdrupalorgsandboxmroji282330439git

My Background

Drupal Developer in Universitas Indonesia. Build some website using Drupal.

My Review

Comments

PA robot’s picture

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.

ijortengab’s picture

Issue summary: View changes
rajesh.vishwakarma’s picture

Status: Needs review » Needs work

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: I'm not really sure about what exatly this module do, I think it's a new functionallty.
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. If "no", list security issues identified.
Coding style & Drupal API usage
  1. You should use db_select instead db_query.
  2. (+) hook_help is missing.

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.

ijortengab’s picture

Issue summary: View changes
ijortengab’s picture

Issue summary: View changes
ijortengab’s picture

thx rajesh.vishwakarma,
i've update README, then i hope clear about goal of this module.
also add hook_help

ijortengab’s picture

Status: Needs work » Needs review
pkamerakodi’s picture

Thanks for your contribution. Please find some review notes from my review.

1) delete the schema in unistall
2) updated the below code in select_role_boot
if (isset($_SESSION['select_role'])) {
if (!empty($_SESSION['select_role']['role_removed'])) {
global $user;
foreach ($_SESSION['select_role']['role_removed'] as $rid => $label) {
unset($user->roles[$rid]);
}
}
}

to

if (!empty($_SESSION['select_role']['role_removed'])) {
global $user;
foreach ($_SESSION['select_role']['role_removed'] as $rid => $label) {
unset($user->roles[$rid]);
}
}

3) I am really not sure if its a good practice to place html tags within t function, it actually takes out the purpose of translation function
Your current role now set to @role

4) In hook_permission as per drupal convention change the key to administer select role

5) For checking if user is anonymouse use
https://api.drupal.org/api/drupal/modules!user!user.module/function/user...

6) Call user_roles(TRUE) so it doesnot return you annon user role

Regards,
Prajwal

pkamerakodi’s picture

Status: Needs review » Needs work
ijortengab’s picture

Status: Needs work » Needs review

@pkamerakodi, thx for review
1) hook_schema don't need uninstall, it's automatically.
2) OK, changed
3) I think it's not problem
4) OK, changed
5) OK, changed
6) OK, changed

ijortengab’s picture

Issue summary: View changes
ijortengab’s picture

Issue summary: View changes
gaurav.pahuja’s picture

Hi Ijor,

It seems you are giving logged in user option to change role at anytime but there is no interface for that.
I gave two roles to a user i.e Administrator and editor. At time of login i was asked to choose a role.

I chose editor and then landed to homepage.
Now I directly went to 'select_role/landing' and selected as administrator and role is assigned to me.

Menu Callback:

$items['select_role/landing'] = array(
    'title' => 'Select Role',
    'description' => 'Select role that assign to you.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('select_role_landing'),
    'access callback' => 'select_role_access',
    'file' => 'select_role.pages.inc',
  );

Is this a expected functionality? If yes, there must be some way for a user to interchange roles.
My initial understanding was that user is not allowed to change role once logged in.

gaurav.pahuja’s picture

Couple of more feedback points:

Unused $user variable on line # 103

function select_role_access() {
  global $user;
  if (user_is_anonymous()) {
    return FALSE;
  }
  $return = TRUE;
  if (isset($_SESSION['select_role'])) {
    $select_role_options = select_role_options();
    // User that no option to select role, is don't need to select role.
    $count = count($select_role_options);
    if (empty($count)) {
      $return = FALSE;
    }
  }
  return $return;
}

Also I was not able to understand how you are using function select_role_form_user_profile_form_alter

Can you please brief me more on usage of this function? I may be missing something.

gaurav.pahuja’s picture

Status: Needs review » Needs work
ijortengab’s picture

Issue summary: View changes

thx for reviewing it.
1. sorry for incomplete information, i expect user can switch to other role,
prevent troublesome users. i think it's a waste of time if the user must relogin to
switch role. I've update issue summary.
2. remove unused $user. thx.
3. about select_role_form_user_profile_form_alter().
3.1. i've add extra information above: Provide extra fields to give the select roles to certain users.
3.2. i've add extra #description: Select roles that will be selected by this user. This configuration effected for this user only. Leave blank to use !url.

ijortengab’s picture

Status: Needs work » Needs review
naveenvalecha’s picture

Status: Needs review » Needs work

Hi @ijortengab,
Thanks for your contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. are passed.

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. If "no", list security issues identified.
Coding style & Drupal API usage
  1. Use the drupal session functions for setting and retereving value in session https://api.drupal.org/api/drupal/includes!session.inc/7 instead of global php $_SESSION object.
  2. Rename the function select_role_admin_form to select_role_admin_settings_form to look it more genric name and use return system_settings_form($form); instead return $form; so that the fields value(like select role field values will be stored in the variable select_role_global) automatically get stored in the variables and remove the submit handler easily.
  3. Remove this submit handler as the values of the variables will be automatically set.So there is no need to write the submit handler.if you need to do some extra work on submit in future then you can use the $form["#submit"] and assign your extra function into it and process the results into that function.
  4. function select_role_options($user = NULL) : There is not any call to this function in the module with argument supplied.So its also needed in the function defination.Also Use global $user; <code> in place of <code>$user = $GLOBALS['user'];

This review uses the Project Application Review Template.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

klausi’s picture

Status: Needs work » Needs review

It is perfectly fine to use $_SESSION (that is the "API") and $GLOBALS can also be used. The other stuff mentioned in your review are minor improvements, any other application blockers that you found?

naveenvalecha’s picture

@klausi,

It is perfectly fine to use $_SESSION (that is the "API") and $GLOBALS can also be used. The other stuff mentioned in your review are minor improvements, any other application blockers that you found?

Thanks, I have not found any application blocker to set this to RTBC.Setting this to RTBC.
@ijortengab,
Please do the following minor coding style changes(#2,#3) as mentioned in #18. Regarding #1 and #4 we can directly use them as @kalusi mentioned in #19

please review 2 other project applications to get a review bonus as you have already reviewed 1 other project application. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Changed status.

naveenvalecha’s picture

@ijortengab,
As I did not find any release blocker in the module last time.To make this module publish just address the #18 and get a review bonus as you have already reviewed 1 other project application. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

ijortengab’s picture

thx @naveenvalecha and @klausi, for your review.
sory for delayed update..
About of review other project, I'll do it soon to .

ijortengab’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus
klausi’s picture

Status: Reviewed & tested by the community » Fixed

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

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    FILE: /home/klausi/pareview_temp/select_role.install
    --------------------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    --------------------------------------------------------------------------------
     32 | ERROR | Expected one space after the comma, 0 found
    --------------------------------------------------------------------------------
    
  • 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:

  1. select_role_help(): HTML tags should be outside of t() where possible.
  2. "'#title' => 'Global Select Role',": all user facing text must run through t() for translation. Same for "'#markup' => isset($_SESSION['select_role']) ? 'Select your role to change.' : 'Select your role first before continue.',". Please check all your strings.
  3. README: Drupal 7 already requires PHP 5.2, so you can remove that section.

But otherwise looks good to me, so ...

Thanks for your contribution, ijortengab!

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:

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.

ijortengab’s picture

I've updated code to add t().
Thx @klausi, @naveenvalecha, and all reviewers for reviewing this module.

Kind regards,
@ijortengab

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.