[D7] Select Role

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.

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.

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

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.

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.

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!

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?

@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!

Changed status.

@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 :-)

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:

• 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.