[D7] Select registration roles

Git clone command for the sandbox is missing in the issue summary, please add it.

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 are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsushilck2630130git

Fixed the git clone URL in the issue summary for non-maintainer users.

I'm a robot and this is an automated message from Project Applications Scraper.

@Smalution,

Module seems to be quite ok except in select_registration_roles.module file at line no. 102
$role_name = db_select('role', 'r')->fields('r', array('name'))->condition('rid', $select_roles)->execute()->fetchField();
Instead of select_roles it should be role variable.

@Smalution thanks for contribute.

'#title' => 'Roles', 
'#title' => 'Approval Roles',

1. Use t() for title.
2. Alternatively add hook_help.
3. Add dependency of user Module.

Have you considered using hook_form_FORM_ID_alter instead of hook_form_alter? This saves you a few lines and is easier to read for other developers. hook_form_FORM_ID_alter is called before hook_form_alter so that would clear the way for future improvements and new functionalities.
No application blockers though.

The module looks ok. I was just wondering whether the same functionality could be implemented with Drupal Rules but I am not sure.

Not a blocker but can make your life easier. Consider in function select_registration_roles_admin_settings_form and any other place where you have to extract roles info, to do so using user_roles(1) function, and just remove the unwanted entries (in your case, key 2). That turns unnecessary the next foreach.

Hi,
Please change MENU_NORMAL_ITEM to MENU_LOCAL_ACTION under hook_menu. It will show link with symbol. Right now link is not showing under admin/people.
Regards
Arvind

Automated Review
No issue http://pareview.sh/pareview/httpgitdrupalorgsandboxsushilck2630130git
Manual Review
README.txt/README.md
Yes: Follows Template
Coding style & Drupal API usage

• Just a recommendation It good if you implement hook_install() to delete variable_get() variables.

Project page template

https://www.drupal.org/node/2190239. Please refer this link, your project page doesn't follow proper template and doesn't have project summary and etc. It's easy to understand once you follow the template properly.

Thanks.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool or made comments about git commands. Make sure to read through the source code of the other projects, as requested on the https://www.drupal.org/node/1975228review bonus page.

Individual User Account?
No? It looks like this project has a company sponsor (in the README.txt). User name is the same as the company and the company is listed on the user account as well.
No duplication?
Looks similar to these modules:
https://www.drupal.org/project/autoassignrole
https://www.drupal.org/project/select_role
Master Branch?
Yes, default branch is set to 7.x-1.x
Licensing?
Licensing looks good.
3rd party assets/code?
I don’t see any 3rd party assets or code.
README.txt/README.md?
README.txt is present.
Code long/complex enough for review?
Yes, project is large enough and complex enough to review.

(+) Issue: I’m not going to mark as RBTC or ‘Needs Work’ because I’m not clear on the Individual User Account rules. On the review template the link to the standards lead to a page that was not clear on the issue. I believe the sponsor link in the README with a company name that matches your username is enough to seek clarification though.

I’ve sought out a second opinion here: https://groups.drupal.org/node/157669#comment-1141478

Manual review
Line 42:

$default_value = 0;

Throws the following notification:

Warning: Invalid argument supplied for foreach() in form_type_checkboxes_value() (line 2389 of C:\xampp\htdocs\Prive\Drupal7-dev\includes\form.inc).

Replace it with:

$default_value = array();

Line 65:
Replace the 2 by the constant DRUPAL_AUTHENTICATED_RID.

unset($roles['2']);

Line 78:
You might want to say which module here and maybe give a link to the configuration.

'#title' => t('You Must Configure the Module First'),

Line 88:
Wrap this in a t()

$role_name .= " <i> *needs administration approval</i>";

Line 118:
Use drupal_get_messages('status', TRUE); instead of clearing the session.

unset($_SESSION['messages']['status']);

Line 123:
Also check for type here (===) as array search may return 0 which will be evaluated as FALSE. More information here.

if ($key == FALSE) {

Line 124:
This can be removed.

$account->roles;

Line 134-136:
Do not send the mail to the user 1 account, send it to the sitewide email address or send it to a configurable email address. You're also using the preferred language of the registered user while you're sending the email to someone else. For more information, see how it's done here.

$admin = user_load(1);
$email = $admin->mail;
drupal_mail('select_registration_roles_with_approval', 'registration', $email, user_preferred_language($account), $params);

Line 147-149
Allow this to be translatable.

$output = "Name: " . $params['username'];
$output .= " had request a role that need your confirmation. ";
$output .= "Role(s) that need your approvals: ";

Line 150-159:
You might want to replace this by implode.

$i = 0;
foreach ($params['user_approval'] as $user_approval) {
  if ($i == 0) {
    $output .= $user_approval;
  }
  else {
    $output .= ", " . $user_approval;
    $i++;
  }
}

You should also add a .install file where you remove all your variables.

README file:

The README file follow's the recommended template, and is relatively clear.

Change Select Registration Roles module is for add role field in registration form.to The Select Registration Roles module adds a role field on the registration form.

Change This module work with core user module. to This module works with the core user module..

Info file:

The convention seems to be to use hyphens rather than underscores in urls. It's also recommended by Google (https://support.google.com/webmasters/answer/76329). So I recommend changing admin/people/select_registration_roles_setby_admin to admin/people/select-registration-roles-setby-admin.

Install file:

The install file looks good.

Module file:

Overall the module's code looks good.

In your menu hook update the configuration path to match what was suggested in my comment about the info file.

On line 78 there's a form field and the title is You must configure <a href = "@configure">roles need for registration.</a>. I'm not sure what you mean by that.

I wouldn't call it a blocker, but the grammar of various labels and messages to registering users and administrators could be improved. I'd be happy to help, but unfortunately I don't have the availability right now, but possibly next week.

Automated Review

No issue found.

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not causes] module duplication and/or fragmentation.

Similar module which provides the same functionality but they are different in functionality:

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Explain the functionality of module in detail

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

No issue found

Question:

If there is Admin approval required for all the new users, so why you use "Approval Roles" field ?

Thank you for your contribution!

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!

Thank you, 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 go also to the dedicated reviewer(s) as well.