[D7] cSupport Live Chat Plugin

Manual review of cSupport 7.x-1.x branch:

• All files should end in a newline, see http://drupal.org/coding-standards#indenting
• You have 2 .info files in your module, where i think the csupport_live_chat_plugin.info is obsolete
• The csupport.info contains a link, the standard of formatting can be found on http://drupal.org/node/542202#description
• Inline comments must end in full-stops, exclamation marks, or question marks

csupport.module:101

/**
 * Check if user can see the live chat or not
 */
function _csupport_role_can_see($user) {
  $roles_set = variable_get('csupport_visibility_roles', array());
  foreach ($user->roles as $id => $role) // Loop through the roles of the user
    if (isset($roles_set[$id]) && $roles_set[$id]==$id) return FALSE; // Exclude the live char for this role/user
  return TRUE; // User should see the live chat
}

Control structures should include the curly braces, http://drupal.org/coding-standards#controlstruct

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Go and review some other project applications, so we can get back to yours sooner.

Manual review of the 7.x-1.x branch:

Minor coding style issues:
csupport.module

• Line 10: Implements hook_foo(). should be Implements hook_help().

csupport.admin.inc

• Line 10: Implements hook_admin_settings_form(). does not exists, should be hook_form().
• Line 155: Implements hook_admin_settings_form_validate(). does not exists, should be hook_form_validate().

Other Issues:

I'll get the "The margin hast to be only numbers" although I filled in a number.
This is caused by the fact the value is a string (and not an integer).
So the check !is_int($form_state['values']['csupport_margin']) will fail, therefore you can use PHP intval to get the integer value of this variable.

The type ($form['general']['csupport_type']) doesn't update when I select the other option, the value of #default_value is hardcoded and not handled through the variable_get() function.

Other Question, is there a possibility to have a development mode where I can actually see the cSupport window, without having to register for a trial?

Coder and PAReview gives out a clean result

Manual review of the 7.x-1.x branch:

csupport.admin.inc

• I have some doubts of the way of implementing the colorpicker, someone else should review this.
  in csupport.admin.inc

    $form['layout']['csupport_bgcolor'] = array(
      '#type' => 'textfield',
      '#title' => t('Color'),
      '#default_value' => variable_get('csupport_bgcolor', ''),
      '#size' => 40,
      '#maxlength' => 7,
      '#required' => FALSE,
      '#description' => '<p>' . t('HEX format starting with #.') . '</p>',
      '#suffix' => '<div id="csupport_bgcolor_colorpicker"></div>' .
      '<script type="text/javascript">jQuery(document).ready(function() {' .
      '  var colorPicker = jQuery.farbtastic("#csupport_bgcolor_colorpicker",function(color){' .
      '    jQuery("#edit-csupport-bgcolor").val(color);' .
      '    jQuery("#edit-csupport-bgcolor").css({\'background-color\':color,\'color\':(this.hsl[2]>0.5?\'#000\':\'#fff\')});' .
      '  });' .
      '  if(/^(#[0-9a-fA-F]{6})$/.test(jQuery("#edit-csupport-bgcolor").val())) colorPicker.setColor( jQuery("#edit-csupport-bgcolor").val());' .
      '});</script>',
  

• A little note on csupport.admin.inc:121 and :147 you use " instead of a single quote in the t() function, not wrong but a bit inconsistent.
• Use // for inline comments (csupport.admin.inc 158, 163, 168

csupport.module

• On line 76 you have the check if ($margin > 0 && $margin < 101) and the input field is limited by 2 chars, why is this?
• You have an own implementation for role visibility, I think it is more Drupal like to move this to a seperate permission

Other consideration is to have the button in a block, but that is more a feature request.

I think you are almost there, besides the jQuery implementation and the role visibility issue, but someone else should give their opinion about it.

Hi csupport,

Everything seems to be validating OK through ventral.

I totally understand when you want to output javascript and it seems like there is no other way than to just to blatantly output it! I'm just wondering if you could possibly modify the form to use "attached" to import your javascript a little nicer. Perhaps you could write to the element using your javascript file rather than outputting it as you are. attached info is here: http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.html/7#attached

I think definitely move the role visibility implementation into where you have your hook_access/hook_permission implementation.

Just going off what's been posted here :) I had a look at the code and apart from the lengthy javascript insertion it looks alright to me.

Cheers.

hi csupport,

hey fair enough. you should try doing some reviews to try and bounce your project forward. check it out here: http://drupal.org/node/1410826

good luck!

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: ...drupal-7/sites/all/modules/pareview_temp/test_candidate/csupport.module
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   92 | ERROR | csupport_requirements() is an installation hook and must be
      |       | declared in an install file
  --------------------------------------------------------------------------------
  

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. csupport_install(): no need to set variables on installation as you can use default values in variable_get() anyway.
2. csupport_page_alter(): Your module is vulnerable to XSS exploits. If I enter "; //--><!]]> </script><script>alert('XSS'); // as domain name I get a nasty javascript popup. I would suggest that you pass your variables as javascript settings with drupal_add_js() and that you put your actual javascript into a dedicated *.js file. Please make sure to read http://drupal.org/writing-secure-code and all subpages.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

• If all the operators are offline, then the minimized chat button should say chat offline. This button should also turn red to indicate that the operator is offline.
• The user should be provided with a close 'X' option beside the minimize button to completely remove the chat from that page until its refreshed.
• The 'Contact Us Now!' title of the chat should be changed to 'Contact Us for Live Support!'.

Recommendations

• Make the chat button less buggy when you're in overlay mode editing the Drupal website. It freezes and intrudes with the main content.
• Provide the Drupal developer with the option to change the chat title and icon in the configuration settings.
• On the 'Message has been sent' screen, create a back button that is more clear.
• Give the option for the chat button to be fixed to the bottom of the screen or to be relative so that it can be placed in different regions.
• When logged into a Drupal account you should be able to quickly access the configure menu for the cSupport module from the top menu.
• Seeing as the customer can change their username that displays in the chat, the admin should be able to as well in the configuration page.

Just wanted to give you the review from an outside user. The module is secure but a bit buggy in overlay/development mode.

This application is not fixed? See http://drupal.org/node/532400

You have some unicode characters in your README.txt, which should only contain ASCII.

_csupport_role_can_see() shouldn't really exist. You should have a second permissions called (for ex) "see csupport chat" or something, and then the admin can assign it to all the roles necessary. Then your code can just check with user_access('see csupport chat') or whatever you named the permission.

Those are minor issues though, this looks RTBC from me.

cSupport does have a Drupal module listed at http://csupporthq.com/knowledgebase/plugins-for-csupport/ - but its just a zip file, and seems to refer to a defunct module - http://drupal.org/project/csupport_live_chat_plugin.

They should probably update that.

Drupal.org has the following policy

All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered.

Can you confirm that the csupport account is a single user? Filling out your profile at https://drupal.org/user/1765736 would help.

If you prefer, we can also promote this sandbox to a full project without granting you "git vetted user" access.

Thanks for the update, that looks better. Don't forget to update your git user name as well!

Thanks for your contribution, ds_dreamconception!

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 - Enterprise modules from the community for the community.