Butterflive Live tracking and chat

Please read the guidelines (http://drupal.org/node/1011698) about submitting a project and fix your description.

Hi!

Sorry, but either you have not completely read http://drupal.org/node/1011698 or you have forgotten to save your description updates. I can't even see a project page link!

(status)

Modification of the description as required

OK, giving up and leaving this to someone more experienced here...

There seems to be some strange communication problems here. This is from the link we have now posted twice:

Fill out the issue form:
Component: 'module' or 'theme' (depending on the application)
Category: task
Status: needs review
Title: Your project name
Description:
A detailed description of what your project does, including how it is different from other, similar projects, if applicable.
For themes it's helpful to include a screenshot.
<strong>A link to your project page.
A direct link to your git repository (git clone ...).</strong>
Please specify if it's for Drupal 6 and/or 7.

Please upgrade your description according to ALL of these guidelines.

You should have a read of the instructions about creating sandbox projects.

description modifications

Master Branch

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

Code Review

Please run the Coder module on "minor" setting to help catch these. The coding standards have even more information in this area.

Automated Review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Automated Review using PAReview.sh

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  butterflive.module:305:	//Get request parameters
  butterflive.module:310:	//Perfrom request
  butterflive.module:315:	//Review data
  butterflive.module:321:	//Redirect with data
  butterflive.module:333:	//Get parameters
  butterflive.module:339:	//Display results
  butterflive.module:404:	//Perform request
  butterflive.module:409:	//Set variables
  butterflive.module:416:	//Redirect to the account seetings results page
  butterflive.module:427:	//Add css
  butterflive.module:466:	//Get the hashcode
  butterflive.module:469:	//Perform the request
  butterflive.module:474:	//Review data
  butterflive.module:605:	//mail 	The account owner's e-mail
  butterflive.module:611:	//password 	A password for the account
  butterflive.module:615:	//origin 	The identifier of the origin of the account creation request. For example if your request come from a Magento powered website, origin will be "magento".
  butterflive.module:618:	//websiteurl 	Your website URL. This is the website that will be authorized for this account. If set, website will be associated and activated at the account creation.Use this parameter if you know the site you will use with Butterflive to activate it now without using the Activate URL function
  butterflive.module:622:	//validateMailUrl( optional) 	The complete path to the mail validation page. The mail validation link will be pointing this URL.
  butterflive.module:634:	//Perform request
  butterflive.module:639:	//Work on data
  butterflive.module:644:	//Redirect
  

• ./butterflive.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

  	//origin 	The identifier of the origin of the account creation request. For example if your request come from a Magento powered website, origin will be "magento".
  	//websiteurl 	Your website URL. This is the website that will be authorized for this account. If set, website will be associated and activated at the account creation.Use this parameter if you know the site you will use with Butterflive to activate it now without using the Activate URL function
  	//validateMailUrl( optional) 	The complete path to the mail validation page. The mail validation link will be pointing this URL.
  

• ./butterflive.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  42- */
  54- */
  66- */
  79- */
  301- */
  367- */
  398- */
  653- */
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./butterflive.info ./butterflive.module ./butterflive_live_tracking_and_chat.info ./README.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Automated Review using the Coder module

Coder Settings:

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards, Coder Tough Love

Coder found 1 projects, 2 files, 1 critical warnings, 523 normal warnings, 0 warnings were flagged to be ignored

You should definately run coder and check the error messages that it outputs, far too many to repost here.

Description changed to suit the guidelines

Hi, I have now run drupalcs on your code. Find the results attached.

Before you open the file: This looks more at first sight than it is. I recommend you do not check by code line, but by issue. Most things look easily fixable with a good editor capable of "search/replace all" and regex. But, more important than to satisfy a review script is that you try to understand the complaints by reading into http://drupal.org/coding-standards - it is considered very important (although there are some other things we need to check once these simple formals are done) to have your code in a certain style as this eases collaborative future development a lot.

HTH,
dave

Hi, there are still some remains of drupalcs.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

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

  
  FILE: ...p709/sites/all/modules/pareview_temp/test_candidate/butterflive.install
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   23 | WARNING | A comma should follow the last multiline array item. Found: ''
   30 | WARNING | A comma should follow the last multiline array item. Found: ''
  --------------------------------------------------------------------------------
  
  
  FILE: ...dp709/sites/all/modules/pareview_temp/test_candidate/butterflive.module
  --------------------------------------------------------------------------------
  FOUND 24 ERROR(S) AFFECTING 23 LINE(S)
  --------------------------------------------------------------------------------
    37 | ERROR | Data type of return value is missing
    38 | ERROR | Return comment indentation must be 2 additional spaces
    51 | ERROR | Data type of return value is missing
    52 | ERROR | Return comment indentation must be 2 additional spaces
    65 | ERROR | Data type of return value is missing
    66 | ERROR | Return comment indentation must be 2 additional spaces
    79 | ERROR | Missing parameter type at position 1
    80 | ERROR | Parameter comment indentation must be 2 additional spaces at
       |       | position 1
    83 | ERROR | Parameter comment indentation must be 2 additional spaces at
       |       | position 2
    95 | ERROR | Data type of return value is missing
    96 | ERROR | Return comment indentation must be 2 additional spaces
   196 | ERROR | Data type of return value is missing
   197 | ERROR | Return comment indentation must be 2 additional spaces
   312 | ERROR | Missing parameter type at position 1
   313 | ERROR | Parameter comment indentation must be 2 additional spaces at
       |       | position 1
   315 | ERROR | Doc comment for var $form_state does not match actual variable
       |       | name &$form_state at position 2
   315 | ERROR | Missing parameter type at position 2
   316 | ERROR | Parameter comment indentation must be 2 additional spaces at
       |       | position 2
   429 | ERROR | Missing parameter type at position 1
   430 | ERROR | Parameter comment indentation must be 2 additional spaces at
       |       | position 1
   463 | ERROR | Missing parameter type at position 1
   464 | ERROR | Parameter comment indentation must be 2 additional spaces at
       |       | position 1
   753 | ERROR | Data type of return value is missing
   754 | ERROR | Return comment indentation must be 2 additional spaces
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:
* Did you check http://drupal.org/node/1166144#comment-5242028 thoroughly, regarding your branches?
* I really suggest two very important things.
** Install the review tools (pareview and drupalcs) on your own machine (this works even with windows and VMware or virtualpc). The tools evolve almost daily (also the standards do, but right now, the tools are just getting better in finding standard errors). It will save you lots of waiting time as way too few people help reviewing. This is really bad, I can't help it.
* Read into the coding standards. And yes, I mean, all (at least all that are relevant for your project). This is not for harassment, but to make code, well, standardized, so everyone does not need to bother getting used to your personal coding style, but can instead catch up as fast as possible with your essence of ideas and provide patches, add-ons etc. Just try to read into older, e.g. 6.x core files (or, at best, 5.x contributed projects) which do not match the current standards. This is really no fun.

hth, dave

I am in the middle or the review process as well. It was helpful when they showed me this link: http://ventral.org/pareview where you can run PAReview.

I just put in your git repository by putting in this url (folks can correct me if I've done it incorrectly) http://git.drupal.org/sandbox/pbouchequet/1165982.git and got a detailed report on what you need to do. I hope this assists you. Note: I'm watching my one year old and didn't have time to check if I ran this on your master branch or the correct branch which would be 7.x-1.x-dev. You can't use master on drupal.org I learned because there would be no way to know if you're on drupal 6 drupal 7 or drupal 8. Ok fatherhood calls.

Cheers,

Barnettech

I just put in the correct branch: http://git.drupal.org/sandbox/pbouchequet/1165982.git 7.x-1.x-dev into the tool above: http://ventral.org/pareview

These are the only errors that come back:

FILE: ...areview/sites/all/modules/pareview_temp/test_candidate/butterflive.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
4 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...view/sites/all/modules/pareview_temp/test_candidate/butterflive.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
60 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/butterflive.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
80 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
83 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 2
--------------------------------------------------------------------------------

I also see for example on line 101 they wont allow this to pass probably due to lack of indentation with your array:

100   $items['admin/config/system/butterflive'] = array(
101   'title' => 'Butterflive Live Tracking & Chat',
102   'description' => 'Configuration for the Butterflive module.',
103   'page callback' => 'drupal_get_form',
104   'page arguments' => array('butterflive_form'),
105   'access arguments' => array('administer users'),
106   'type' => MENU_NORMAL_ITEM,
107   );

http://drupal.org/coding-standards#array shows how drupal.org requires arrays to look.

I just ran it through pareview and there are only 2 minor errors for you to fix:
FILE: ...eview/sites/all/modules/pareview_temp/test_candidate/butterflive.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
80 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 1
83 | ERROR | Parameter comment indentation must be 2 additional spaces at
| | position 2

Fix these 2 minor issues, and then the next reviewer will at least know the automated review is done. Then a manual review, and I'm going to try to load up your module. I expect that is what the git admins for this area will want. They are encouraging us to review others while we wait btw, I am in the que as well with my "Droogle" module.

good luck. I'll try to load up your module now as well.

So I went to install your module and I use drush and I noticed that your base directory for your module is butterflive_live_tracking_and_chat but your module is actually called: butterflive.module so your base directory should really be "butterflive" and not "butterflive_live_tracking_and_chat". This broke drush actually and I was unable to install your module via drush.

Also I noticed that your functions are not properly namespaced. All functions should really start with your modules name. So function set_butterflive_param($param, $value) should really be function butterflive_set_param($param, $value) is my understanding. They dinged my module on this as well, and I've since fixed it. The idea is if you namespace, prefix every function with your module name, and the module name is unique on drupal.org, then there wont be conflicts with the naming of module functions.

Hi everybody!

From now on, I'll be working on this module in place of pbouchequet.
I have seen all remarks by barnettech and taken them into account.

Pareview is giving me 0 errors now on my computer (although I could not test the online version because it is in maintenance).

Also, I could not rename the project from "butterflive_live_tracking_and_chat" to "butterflive", so instead, I've prefixed all the functions with "butterflive_live_tracking_and_chat". It's a bit long, it can look a bit wierd, but it fits the coding standards.

Once more, thanks to anyone willing to review this module, and I'm sure we will have it validated very soon.

David.

Hi,

Ventral's Pareview is back online so I performed a last check. There was a problem with the branch name that I solved. Everything is fine now, and there are no more warning in the Ventral's Pareview:

http://ventral.org/pareview/httpgitdrupalorgsandboxpbouchequet1165982git...

Sorry for the long delay!

1. Please take a moment to make your project page follow tips for a great project page.
2. Do you REALLY want to use such a long module name ?! there's no problem of using just "butterflive"! seriously! ^^

3.  * Implements hook_init().
    * Displays The Butterflive tracking code if Butterflive is enabled.

   always put a blank comment line between short description and long description. ->

    * Implements hook_init().
    *
    * Displays The Butterflive tracking code if Butterflive is enabled.

4. what you're doing there with your own implementation of a settings/variable table makes no sense. please have a look at variable_get() and variable_set() and use these for your settings instead.
5. Don't add so much javascript code inline if there's only 1 variable value in there. use a js file and drupal.settings instead (read about drupal_add_js() and its "settings" option)
6. if you then really want to have the .js file inserted into every page, use "scripts[] = " in your .info (See https://drupal.org/node/542202)
7. array('administer users'), why administer users ( == permission to configure and administer user accounts, please choose a more accurate permission (like access administrative pages) or create you own one here
8. I don't think it makes sense to translate javascript ;-), don't encapsulate it with t()

I'll stop here as my impression is that here's unfortunately a huge lack of drupal-specific knowledge.

There are lots of good resources about drupal module development (books, screencasts, ...), please use them! they're totally worth it!

regards

It would be good if you'll tell us in some words what the "Butterflive services" actually are, and why people would integrate them on their sites.

I would also recommand choosing a smaller short_name for your project: "butterflive_live_tracking_and_chat_". This would make reading function-names much easiser, and less scrolling in file browsers. I see in your menu you are using "butterflive", I would recommand this as your shortname.

Javascript code should be in Javascript (.js) files:

function butterflive_live_tracking_and_chat_init() {
  if (butterflive_live_tracking_and_chat_get_status() && butterflive_live_tracking_and_chat_get_key() != '') {
    drupal_add_library('system', 'ui.dialog');
    drupal_add_js(
      'bflOptions =
      {
      key: "' . butterflive_live_tracking_and_chat_get_key() . '",
      src: "http://api.butterflive.com/butterflive-debug.js"
      };
      (function()
      {
      var btf = document.createElement(\'script\'); btf.type =
      \'text/javascript\'; btf.async = true;
      btf.src = bflOptions.src;
      var s = document.getElementsByTagName(\'script\')[0];
      s.parentNode.insertBefore(btf, s);
      })();
      ',
      'inline'
      );
  }
}

The same in butterflive_live_tracking_and_chat_admin().

I would also recommand identing lines which start with "->" as in-here:

function butterflive_live_tracking_and_chat_get_mail() {
  $query = db_select('butterflive', 'b');
  $query->condition('b.name', 'mail', '=')
  ->fields('b', array('value'))
  ->range(0, 1);
  return $query->execute()->fetchField(0);
}

/**
 * Returns Admin form.
 *
 * @return array
 *   The admin form.
 */
function butterflive_live_tracking_and_chat_admin() {
...
...
}

The t() function should not contain any HTML code as in butterflive_live_tracking_and_chat_activate_site_results():

 t("<div class=\"good\">You site is now registered, you can start using Butterflive now.<br/>
      <a href=\"$base_url?q=admin/config/system/butterflive/admin\">Go to admin page</a></div>"),

Also variables must not be used in t()! Please consult the t() function documentation.

This link will not work if short_url is activated in Drupal:
.... $base_url . "?q=admin/config/system/butterfli....
You should use the url() function for this. Please consult the documentation.

You should not use TAPs for indenting.

A big no-go for any web-application:
<div style="width:40%;margin-right: 5%;float:left;">
Styling information has to go into CSS-files, not as inline-HTML!
The same for: butterflive_live_tracking_and_chat_output_style()!!!

HTML should go into theme functions or even better template files:

    <div style=\"width:40%;margin-right: 2%;float:left;\">
    <iframe src=\"http://www.butterflive.com/frames/magento/index.php\" style=\"width:100%;height:400px;border: 0px solid #ffffff;\" scrolling=\"no\"></iframe>
    </div>
    <div style=\"clear:both\"></div>"),

You should also hook_form for creating forms!
See http://api.drupal.org/api/drupal/modules!node!node.api.php/function/hook...

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

Modification of the git repository address