Casengo Contact Widget

Hi thijs@casengo.com,

Thanks for submitting a project application for review. To help you track the status of your application you might want to enable email notifications for changes to your application:

http://drupal.org/project/issues/subscribe-mail/projectapplications

You might also want to participate in the review bonus program - this should help ensure your application is review quickly:

http://drupal.org/node/1410826

The PAReview tool identified issues which need addressing:

http://ventral.org/pareview/httpgitdrupalorgsandboxthijscasengocom184786...

• It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
   
• README.txt is missing, see the guidelines for in-project documentation.
   
• You need to do some tidying - generally your .module files should be in the root of your repository (unless you also have submodules). There's also a ZIP file that probably shouldn't be there.
   
• Remove all old CVS $Id tags, they are not needed anymore.
  

  casengo/casengo.module:2:// $Id:
  casengo/README.txt:1:$Id:
  casengo/plugins/casengo_context_reaction_add.inc:2:// $Id:
  

   

• Run coder to check your style, some issues were found (please check the Drupal coding standards). Please review the link above for further information.
   
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). Please review the link above for further information.
   

Please check/fix these issues before changing the status back to "needs review"

Hi,

here is my manual review to your code:-

1) Remove $Id$ , it is no longer required.
2) Your branch hai .zip file of your code. Please remove that.
3) Your module file and README.txt should also end with a new line.
4) Your info file contains ;Information added by drupal.org remove it, that will be added automatically.
5) Also please remove LICENSE.txt, it will be added automatically.
6) Please remove commented code from your module file.
7) In casengo_settings($form, &$form_state) i dont think parameters are required.

Also please remove master branch and make sure to fix your ventral issues here : http://ventral.org/pareview/httpgitdrupalorgsandboxthijscasengocom184786...

Hi,

First of all there are a lot of problems still identified by the online automated review tool:
http://ventral.org/pareview/httpgitdrupalorgsandboxthijscasengocom1847868git

One thing that is also very important is to change the master branch to a version specific branch, The link below tells you how to do this:

http://drupal.org/empty-git-master

Manual Review:

Remove the zip file and update the contents of the module with the contents of the zip file, and with Drupal you must only use the module root folder as cassengo_chat_widget and then all the files and functions use the name of the module instead of just casengo.
So it would look something like this:

-- casengo_contact_widget
---- plugins
------ casengo_contact_widget.context_reaction.inc
---- casengo_contact_widget.module
---- casengo_contact_widget.info
---- REAME.txt

With the contents of the Zip File:

In the plugin file you need to add doc blocks for the functions and their parameters.

Find a whole lot more info on creating Drupal modules here:

http://drupal.org/node/1074360

And could you state the version you're developing for in the issue, I assume it is Drupal 7?

Hello,

firstly the reason for this README.txt problem is because you have a nested folder, casengo_contact_widget in casengo_contact_widget, remove the nested folder and move the contents to the top level folder.

This is also most likely the reason why the PAReview tool gives no errors as well.

Secondly in the naming in the module must be consistent so all the files have the casengo_contact_widget and then all the hooks will have casengo_contact_widget_ as the hook. As well as this in custom variables you will need to use the consistent hook. Go here again to see what is required of Drupal modules:
http://drupal.org/node/361112

For all the hooks you use you must use Implements hook_page_alter() using page_alter as an example in the doc block as found below:
http://drupal.org/node/1354#hookimpl

You need a .install file which implements hook_uninstall() to remove the custom variables set using variable_del() such as casengo_code.

Although i say this you ought to avoid that casengo_code variable and use the theme layer through a theme function to generate this code and use a Javascript file which you include through drupal_add_js() to deal with all Javascript. You should also put this javascript in it's own js file.

In the info file you'll want to actually give a brief description of the widget in the description field as oppose to restating the name of the module.

In the README.txt there is something wrong with the encoding.

Hello,
Well yes but in your case you've created custom variables using variable_set so you have created fields in the table which you must remove on uninstall. The .install file use is because you need to use hook_uninstall() which can only be used in the install file.
The variables are:
cas_widget_domain
cas_widget_pos
cas_widget_label
cas_widget_theme
casengo_context
casengo_code

Example of using install file for hook_uninstall use:

<?php
/**
* @file
* File to uninstall custom variables.
*/

/**
* Implements hook_uninstall().
*/
function casengo_contact_widget_uninstall() {
  variable_del('cas_widget_domain');
  variable_del('cas_widget_pos');
  variable_del('cas_widget_label');
  variable_del('cas_widget_theme');
  // Add the rest of the variable_del() here.
}

As well as this I said it's worth avoiding creating a variable in the table called casengo_code and use the theme layer like below:
http://drupal.org/node/933976

Hello,

For the casengo_code variable it's best if you put the html through the theme layer using a theme function or template and the javascript in it's own javascript file.

As I said before you need a consistency in your naming conventions so instead of cas and casengo use casengo_contact_widget.

You need to remove all the variables declared by your module in the hook_uninstall, hence the comment Add the rest of the variables to delete at the bottom, my code was just a short example.

Hello,

1. You don't seem to have committed your latest changes.

2. For the casengo_code explaining more in depth, When producing markup from a module it is crucial for it to be overridden by a themer so I advise looking here:
http://drupal.org/node/933976

And then implementing something along these lines:

Add theme function to your module file like so:

/**
 * Implements hook_theme().
 */
function casengo_contact_widget_theme() {
  return array(
    'casengo_contact_widget_markup' => array(
      'domain' => NULL,
      'label' => NULL,
      'pos' => NULL,
      'theme' => NULL,
    )
  );
}

Then add the theme function to create the output (In the javascript you will want to replace s and po with self explanatory names):

/**
 * Theme function for casengo page output.
 */
function theme_casengo_contact_widget_markup($variables) {
  extract($variables);
  // This will be the file containing the Javascript.
  drupal_add_js('(function() {  var po = document.createElement(\'script\'); po.type = \'text/javascript\'; po.async = true;
    po.src = \'//' . $cas_domain . '.casengo.com/apis/vip-widget.js?r=\'+new Date().getTime();
    var s = document.getElementsByTagName(\'script\')[0]; s.parentNode.insertBefore(po, s);
    })();');
  $output = '<!--Place this code where you want VIP widget to be rendered -->
  <div class="casengo-vipbtn"><!-- subdomain="' . $domain . '" group="" label="' . $label . '" position="' . $pos . '" theme="' . $theme . '" --></div>
 <!--Place this code after the last Casengo VIP widget -->'
  return $output;
}

Finally for this to work you want to create an array containing all these variables in a new function or in the widget add function which is returned, you could work it out.

Then in page alter you want this ($markup_variables is the result of the last thing I mentioned for you to work out how to go about it):

$page['page_bottom']['tns']['#markup'] = theme('casengo_contact_widget', $markup_variables);

Seems module already pushed from sandbox to a project and issue should be closed.

Closing as it is now a full project as aliaric stated.

Adding project page/repo