[D7] passwordwidgets

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
No: Causes module duplication and/or fragmentation.
There are a few modules that handle toggling password visibility. Have you considered merging with them?
https://www.drupal.org/project/password_toggle
https://www.drupal.org/project/unmask_password

Master Branch
Yes: Follows the guidelines for master branch.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
No: Does not follow the guidelines for 3rd party assets/code.
There is a jquery.min.js included in the js folder. You should be able to remove this as you require the jquery_update module.

README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
Missing README.txt/README.md

Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.

Secure code
Yes: Meets the security requirements.

Coding style & Drupal API usage
There are errors reported by the automated review tools. Have you reviewed these?
http://pareview.sh/pareview/httpsgitdrupalorgsandboxamitdrupal2744573git

This review uses the Project Application Review Template.

Hi amit.drupal,

Some points are as follow

1: Please remove debugging code and commented code.
ex:

drupal_set_message($form_id);

   // $form['login_logout_notification'] = array(
   //    '#type' =>'checkbox', 
   //    '#title' => t('Enable Login Logout Notification Bar.'),
   //    '#description' => t('Enable login logout Notification in header section.'),
   //    '#default_value' => variable_get('login_logout_notification'),
   //  );

2: Please remove unused variable:
ex :
global $base_path;

3: I never find "loginBar.js" file in module.
drupal_add_js($path . '/loginBar.js');

4: Make sure remove all variables in hook_uninstall created by you.
variable_get('password_config')

5: Its better to use user_is_anonymous() instead of

if ($user->uid == 0)

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hi Amit.drupal,

I have installed this module and have done some manual code review and here is my findings:

• As your module has admin settings form to configure module settings, it is recommended to provide "configure" properties in .info file so that end user can understand from where to configure module settings.
• In "passwordwidgets.js" file, there is one commented code line i.e. "//jQuery('#edit-pass-pass2').hideShowPassword(jQuery(this).prop('checked'));", kindly remove this.
• Help page should be more descriptive.
• Project Page link should be there on Project Application Issue Page, it is missing, please provide.

Thanks
Sourdrup

@sourdrup: those are good tips but surely not application blockers. Anything else that you found or should this be RTBC instead?

Hi,

I can still see a lot of warning in pa review test
Automatic review : https://pareview.sh/node/374

Manual review :
Included the necessary css and js at two places passwordwidgets_add_js_and_css()
in hook_init and hook_form_alter, can remove one of them

Thanks
anup.singh

Hello Amit

Please find below reviews :

In file, passwordwidgets.module

1. There are ample of errors for indentations. e.g. Line No. 21, 22, 23, 24 etc

2. Concatenate operator should have surrounded by at least one space.

$output .= '<p>' . t("The Password toggle module adds a checkbox to password fields which toggles the masking of the password; that is, clicking the checkbox changes the '•' characters to the typed characters and back again. This allows users to verify they have typed the correct password, but also retain privacy if required.") .'</p>';

3. At Line 93, the formatting should be correct for following piece of code. Similarly for all the other such sections.

/**
 *
 *Implement hook_form();
 */

4. A new line is expected at the end of file which is missing.

In README.txt file,

Please follow the rule of maintaining no. of characters in each line.

Thanks

Removing review bonus tag, you have not listed 3 reviews in the issue summary? See https://www.drupal.org/node/1975228

You have a lot of warnings and errors from the pareview.sh project review tool. Almost all of these are formatting errors, (and I'm not qualified to comment on any JavaScript) but there are a couple warnings that I find troubling. You have declared 2 Drupal variables that are not prefixed with your module name. Not only is this against Drupal standards but this can cause unexpected errors if it collides with another modules variables.
* password_range
* password_type

Regarding having the admin form in the module file. I believe it's Drupal policy to have that in a MODULE_NAME.admin.inc file, (in hook_menu(), 'file' => 'passwordwidgets.admin.inc'). That way the admin code doesn't have to be loaded for every page, reducing the codebase for ever page and increasing performance.

I wouldn't bust you for the admin form but the variable names could really cause issues, (IMHO).

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hi

This is my manual review for your module which is given as follows.

1. For this method passwordwidgets_form_alter where you have implemented hook_form_alter(), you have added JS and CSS using drupal_add_js() and drupal_add_css().This is our best practise to include these when user_register, user_register_form visible for loaded.
And right now this have been included for every form.

So add this

  $path = drupal_get_path('module', 'passwordwidgets');
  drupal_add_js($path . '/js/hideShowPassword.js');
  drupal_add_css($path . '/css/passwordwidgets.css');
  $settings['password_config'] = variable_get('passwordwidgets_password_config');
  drupal_add_js(array('passwordwidgets' => $settings), 'setting');

under

  if (in_array($form_id, array('user_register', 'user_register_form'))) {

So that it can be loaded when user_register, user_register_form open.

Thanks
Ishwar

Code are looking good.
I think its ready for RTBC.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2883767

Project 2: https://www.drupal.org/node/2811601

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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