[D6] [D7] LiveHelpNow Live Chat

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

I have reviewed and tested this module. Everything is functioning as expected and works great.

I'm afraid I got to push this back to needs work.

• You still have a master branch, please read about Moving from a master branch to a version branch. and delete to remote master branch!
• You are not using branch and tag-names correctly, please see the documentation about release naming conventions!
• I don't recommend to make use of tags until you can create releases (you should wait until this process is finished)
• package = "LiveHelpNow Help Desk -- Chat Button" Is too specific! Please use the package to describe the type of module. for example "Live chat", etc. I would suggest you have a look at other chat-modules and see what package they are using, so it won't fill up the modules page with tons of different package names for just one module.
• Please create a README.txt that follows the guidelines for in-project documentation.

You must use the same name for your project files as you use for the project shortname!
lhnchat.module should be livehelpnow.module and so on

An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.
After you renamed your files, please run all of your code through the coding-standards check at http://ventral.org/pareview and try to solve as many issues as possible.

And please don't register any accounts just to set your own application to RTBC.

"update" is a very bad commit message - commits are for changes, for sure you updated something. It would be much more useful if you explain *what* you changed in the commit. Also have a look at Commit messages - providing history and credit.

As a general best practice you keep data type correct and clean until you print it to the page.
If you have an integer in a variable use it as integer (0) not as string ('0').
If the javascript requires it to be a string - cast it. (string) 0;

I know it's very annoying but when you'r using a default value
variable_get('livehelpnow_buttonid', '35'),
in your form, you have use it everywhere else for this variable too!
variable_get('livehelpnow_buttonid', ''); <-- set 35 as default value

Rather than putting javascript directly/inline into a block you could make use of the inline option of drupal_add_js()

At the moment your printing out the javascript options completely unfiltered. Someone with the proper permissions could put anything into these options (like malicious javascript). You have to properly sanitize your output or validate the module setting values.

For example

  57     $form['livehelpnow_chatwindow'] = array(
  58     '#type' => 'textfield',
  59     '#title' => t('Chat Window ID'),
  60     '#default_value' => variable_get('livehelpnow_chatwindow', '0'),
  61     '#size' => 30,
  62     '#maxlength' => 255,
  63     '#description' => t('*Must be numeric'),
  64     '#required' => FALSE,
  65   );

If it must be numeric - enforce it!
add:

'#element_validate' => 'element_validate_integer_positive',

Anything else that can not be validated this way / or contains un-validated user input --> put it into check_plain() before printing it into the page / using it as inline javascript options!

  75    $form['livehelpnow_autochat'] = array(
  76     '#type' => 'checkbox',
  77     '#title' => t('Enable automatic chat invitation'),
  78     '#default_value' => variable_get('livehelpnow_autochat', '0'),
  79     '#required' => FALSE,
  80   );

It's a checkbox? use booleans!

(Also - avoid putting HTML into t()-functions, this makes the live of translators harder than it should)

In your 6.x-1.x branch you are not deleting all used variables on uninstallation.

Read about removing your existing tags here: http://nathanhoad.net/how-to-delete-a-remote-git-tag (Don't create tags until you can actually create releases)

I also recommend that you might have a look at https://drupal.org/writing-secure-code

Sorry, kicking back to needs work because of the security issues mentioned above.

The links Help Desk Software and Ticket System on your project page doesn't go anywhere.

The url admin/config/user-interface/livehelpnow should be admin/config/livehelpnow.

Wait,livehelpnow_clientid must be numeric say the #description, but in validate it accepts "lhn" and "-1" as valid? Can you make that clearer? Also, is_numeric() will happily accept any number, including floats like 3.14159. Maybe you want ctype_digit() or a regex instead?

drupal_add_js() was already mentioned for livehelpnow_block_view(). I guess it's fine how you have it, but I think you can still use drupal_add_js() with the 'inline' option and it should print in the same place directly in the page. The parameters look safe now that they're being validated on form submission.

Those seem like minor issues.

----
Top Shelf Modules - Enterprise modules from the community for the community.

The code seems fine. But,

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).
Please change your account information and enter your realname.

The application was created by user livehelpnow (who did include a real name), but that user only has a 1 line initial commit. The module looks like it was written by Eric Witchin. We can only grant Git Vetted User status to 1 person for this module. Signing posts as "LiveHelpNow" is not helping - that just makes it look like you're sharing a single account.

Also, should we be reviewing the 6.x or 7.x branch? I think I did 7.x last time, but now I'm not sure anymore.

Please post more information and set this issue back to "needs review".

----
Top Shelf Modules - Crafted, Curated, Contributed.

Sending to klausi, he may have time to help figure this out.

@livehelpnow: I'm confused, you are writing as Eric but your profile says Michael Kansky? So this is in fact a shared account? And that shared account did all the git commits.

Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

Proposed solution: rename the livehelpnow account to something individual like eric_xyz? And make the profile consistent please.

Thanks for clearing that up!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Account issues are resolved, and it's been a month without any problems, so...

Thanks for your contribution, eric_livehelpnow!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.