BACnet [D7]

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxdbt1022237027git

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.

Hi dbt102,

The module shows errors with coder and pareview. (http://pareview.sh/pareview/httpgitdrupalorgsandboxdbt1022237027git) You can easily fix most of them using the phpcbf command after installing the coder module.

Please change the git clone command to git clone --branch 7.x-1.x http://git.drupal.org/sandbox/dbt102/2237027.git bacnet. The one you have given is for the maintainer.

Automated Review

There are a lot of issues pointed out by paravew.sh, please correct http://pareview.sh/pareview/httpgitdrupalorgsandboxdbt1022237027git I opted not to include things in the manual review that appear in the automated review for berevity. You mention that the errors are insignificant, but it's important that your module follows the Drupal coding standards.

Manual Review

1. I would suggest changing the admin path to admin/config/services/bacnet and adding the appropriate configure directive to your .info file

2. in your hook_init function, you have to if statements that don't do anything.

if ( empty($library1['loaded']) ) {

}
if ( empty($library2['loaded'])) {
		
}

3. LIne 251, your missing a t() around "Offline". Same on 257

4. in bacnet_soap_get_value(), you are always setting cache_set and cache_get. I'd suggest doing cache_get first, and if it returns false, then run the query against the soap server to get the value, and caching that result. If cache_get returns false, use the request from the soap server, otherwise use the cached response.

5. in bacnet_soap_get_value() your retrieving data from a third party. That party shouldn't be trusted, and you should sanitize your data using check_plain for filter_xss as appropriate before saving that into the cache. The same goes for bacnet_soap_get_trend()

6. Throughout your code you have a lot of commented out items as well as unused variables. I'd suggest going through your module and cleaning up the unused variables as well as any commented out lines of code such as

//    drupal_set_message( 'End date:'.$end_date );

//    drupal_set_message( 'Start '.$start_date.'---- as a Timestamp  '.date('m/d/Y',strtotime($start_date)).' Test date: '.date('m/d/Y',strtotime( '-0 day')) );

Note for other reviewers: I did not review the views integration as that was one area I'm not comfortable reviewing.

hi dbt102 ,

I have gone through your code and found following issues;

• function bacnet_theme($existing, $type, $theme, $path) {
      $path = drupal_get_path('module', 'bacnet')."/views_plugins";
  
      $theme = array(
      'bacnet_meter' =>
        array(
          'template' => 'bacnet-meter',
          'arguments' => array(),
          'path' => $path,
        ),
  

  Here you are giving path of views_plugins, But there is no directory with this name in your module. and
  bacnet-meter template file also does not exist. So please correct it.

•  function bacnet_block_info() {
      global $user;
  

  Here you are not using $user variable anywhere, so what is purpose of writing it.

• function bacnet_init() {
      $library1 = libraries_load('flot');
      $library2 = libraries_load('transform');
      if (empty($library1['loaded'])) {
        echo "Flot library is loaded.";
      }
      if (empty($library2['loaded'])) {
        echo "Transform library is loaded.";
      }
  }
  

  You should drupal way of showing message to user, so use 'drupal_set_message' instead of echo.

• function bacnet_form_alter(&$form, &$form_state, $form_id) {
    switch ($form_id) {
      case 'bacnet_node_form';{
        $form['#validate'][] = 'bacnetsite_bacnet_node_form_validate';
        break;
      }
    }
  }
  

  Here please correct php syntax, use colon instead of semicolon case bacnet_node_form;

• There are lots of issues shown pareview. Please fix them, else this would blocker for this project to stable release.

Hi @dbt102

• Your code does not have correct indentation. Please follow guidelines given here drupal coding standards.
• Sorry, I forgot to mention it in previous comment. In form alter
  

  bacnet_form_alter

  case 'bacnet_node_form':{
           $form['#validate'][] = 'bacnetsite_bacnet_node_form_validate';
          break;
        }

  There is no need of curly braces here.

@dbt102, thankyou for your contribution!

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, http://pareview.sh/pareview/httpgitdrupalorgsandboxdbt1022237027git reported number of issues, there is possibility might be some issues can't be fixed but as I analyzed most of the issues can be easily fix related to Line indentation and other minor issues.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. As I googled not found any module similar to this.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

1. bacnet_block_view: this is also vulnerable to XSS. All variable comes form user provided input, so you need to sanitize it before passing into your js file. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again.

Coding style & Drupal API usage

1. (*) I tried to test this module functionality and found admin setting form most frustrated. You have not set default values for all your form elements that makes user confused. Following code make me so confused and even I was surprised every time I entered some different values for this field but every time it displays same value (2012). I was not sure what I was doing wrong.
   

   $form['operator']['login_name'] = array(
       '#type' => 'textfield',
       '#title' => t('B-AWS Login Name'),
       '#default_value' => variable_get('bacnet_login_name', ''),
       '#required' => TRUE,
       '#description' => 'Must have:<ul><li>Remote Data Access</li><li>Access Geographic Locations or Network Locations, as needed</li><li>Access Network items, as needed</li><li>Any privileges needed for the specific task at least Remote Data Access</li></ul>',
     );
   

   Issue, your form element name is login_name and you are fetching value for bacnet_login_name to set it default value.

   You are using following submit function but it never called.

   function bacnet_admin_settings_submit($form, &$form_state) {
     cache_clear_all('bacnet_', $table = 'cache', TRUE);
     variable_set('bacnet_login_name', $form_state['values']['login_name']);
   
     // If a password value was set. Avoids saving a blank password,
     // Since the password field defaults to empty, even if a password exists.
     if ($form_state['values']['password'] != '') {
       variable_set('bacnet_password', $form_state['values']['password']);
     }
   
     variable_set('bacnet_soap_server', $form_state['values']['soap_server']);
     variable_set('bacnet_test_point_gql_expr', $form_state['values']['test_gql_point_expression']);
     variable_set('bacnet_test_trend_gql_expr', $form_state['values']['test_gql_trend_expression']);
   
     drupal_set_message('Settings Saved.');
   }
   

   You are trying to set number of variables in this function that you are also using further because this form never called complete flow broken.

2. (+) You should set default values for every form element. Also keep this setting form in separate admin.inc file.
3. (+) You are adding bacnet.css and drupal.bacnet.js through .info file. Due to this, both will added on all the pages rather use #attached to add the JS/CSS file to the form render array or page render array where these actually required.
4. (+) Instead of using time(), should use REQUEST_TIME.
5. (+) Instead of using date or mktime, format_date.
6. You are using curly braces in bacnet-view-row-bargraph.tpl.php template file, better to use recommend approach mention here https://www.drupal.org/coding-standards.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks again for your hard work!

1 - on setting form validation is not proper working. There is no need to put submit function for this".
You must use variable_get to fetch the value on the form.

2 - There are a lots of files where i can see curly braces after "case:" like following
function bacnet_block_help($path, $arg) {
switch ($path) {
case 'admin/help#bacnet_block' :{
$ret_val = '

' . t('About') . '

';
$ret_val .= '

' . t('The BACnet Block module makes it easy to get data from a BACnet - Advanced Operator Workstation (B-AWS) into a Drupal block.') . '

';
return $ret_val;
}
}
}

3 - There are a lots of error in coding standard. i checked it on http://pareview.sh/. please check following url.
http://pareview.sh/pareview/httpgitdrupalorgsandboxdbt1022237027git

bacnet_block_view(): This looks vulnerable to XSS exploits. bacnet_soap_get_value() directly retrieves values from an untrusted third party source, so the return value must be considered user provided input. You need sanitize that in t() by using the correct placeholder with "@" or "%" instead of "!". Make sure to read https://www.drupal.org/node/28984 again.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None.

Manual Review

1. (+) Configure link on module page is incorrect that need to be fixed. In .info file there is admin/config/bacnet/manage and in .module file it is admin/config/services/bacnet
2. (*) As you are using system_settings_form(), it means bacnet_admin_settings_submit never called, need to remove it.
3. (+) bacnet_admin_settings_form(): All the variables defined in this form like bws1_server, bws1_login_name etc need to be delete using hook_uninstall() function after uninstallation of module.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If it is ready to review then please change the status of this issue to Need Review.

Removing review bonus tag, you have not done any manual review, you just copied the empty review template. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I conducted an automatic and manual review.

1. Contains a working repository link.
Non-duplicate
2. Repository has code with proper branch/tag naming

3. No license.txt file present.
No non-GPL code present.

4. Project page has a clear description of purpose.
README.txt present.
Module file has @file and per function docblocks.

5. No results returned from pareview

6. Reviewed code against coding standards - passed.
Correct use of t() function.

7. Output is being sanitized with t() and a filter_xss function is used for the cache statement.
Check_plain is in use where t() is not applicable.
No direct use of GET or POST. No outstanding security issues found.

Open4Energy is a community resource that exposes consumer energy scams, publishes directories of emerging technologies for energy monitoring, and promotes discussion on renewable energy sources. The site is sponsored by adTumbler, Inc - serving in excess of 2 million visitors since being released.

http://open4energy.com/

One of the biggest issues facing those of who who care about reducing our energy footprint is the capital cost of new equipment, retrofitting buildings, and implementing energy control equipment. We and others have proven time and time again, that NO energy is saved by merely monitoring the use of energy.

To actually save energy something has to change. Something has to be replaced, or switched off, or used less. We have toiled for 3 years in the consumer segment to raise awareness, find technologies that show the use of energy in a home, even to encourage families to "engage" in energy saving as part of educating their kids etc etc etc.

In the commercial and Government sector is has been even harder. There are many significant cultural and interests - let alone the complete lack of standards.

At the same time communities (schools, districts, government agencies etc) have proven that up to 35% of total energy used, can be saved when the community can be informed of their energy footprint, and updates as it changes. We are mostly good at saving when it affects our pocket book. We are notoriously poor at saving when it affects our community, or employer, or local government department - until we become aware and informed.

BACnet has emerged as a NIST standard that allows the energy footprint of a building, or a school, to be accessed. Drupal is the community standard that allows any organization to provide a secure web site for the reliable providing of information.

This project, although technically simple in it's functional capability of setting up a block, and presenting an energy value in the block, has immense potential for extending the role of Drupal as the open source standard for information sharing, to that of the open source standard for affecting change in the amount of energy we use.

adTumbler, Inc and Open4Energy have engaged with dbt102, to manually review this project for Pareview compliance, for functional excellence, and support of the BWS1 (SOAP) and BWS2 (REST) BACnet standards.

The current version of the module meets all our requirements for inclusion in the production site Open4Energy - it has been implemented on the site - from which we hope to encourage a community of innovators supporting Drupal and the Energy Dashboards BACnet and this module enable.

Git clone command for the sandbox is missing in the issue summary, please add it.

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

Open4Energy is a community resource that exposes consumer energy scams, publishes directories of emerging technologies for energy monitoring, and promotes discussion on renewable energy sources. The site is sponsored by adTumbler, Inc - serving in excess of 2 million visitors since being released.

http://open4energy.com/

The current version of the module meets all our requirements for inclusion in the production site Open4Energy - it has been implemented on the site - from which we hope to encourage a community of innovators supporting Drupal and the Energy Dashboards BACnet and this module enable.

Refer comment at #36 above for details

You seems to be almost there, looking forward to see a release soon. Good Job.

Review of the 7.x-1.x branch (commit 5482608):

• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: /home/klausi/pareview_temp/bacnet.module
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  --------------------------------------------------------------------------------
   216 | WARNING | Unused variable $version.
   283 | WARNING | Unused variable $version.
  --------------------------------------------------------------------------------
  

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. You have to get a review bonus to get a review from me.

manual review:

1. sorry about the wrong line number for the unused variables in the list() statements, this should now be fixed in DrupalPractice.
2. BACNET_MODE_BWS1: what does this constant and the others mean? Please add a comment. What is BWS1?
3. bacnet_soap_get_value(): why do you suppress PHP warnings here with "@"? Please add a comment.
4. bacnet_soap_get_value(): the filter_xss() is useless here since you are not using the return value? You should not apply filtering when writing to the database anyway, see https://www.drupal.org/node/28984
5. bacnet_schema(): empty function, so remove it.
6. bacnet_soap_get_value(): why do you execute a cache_get() immediately after the cache_set()? That does not make sense since you already have the $result at hand? Shouldn't you query the cache at the beginning of the function before invoking the SOAP request?
7. bacnet_soap_get_value(): the check_plain() is premature here, since you are not printing anything to HTML here. You should sanitize data while it is just passed around internally. And you use the "%" placeholder in bacnet_block_view() for printing the value, which will already run check_plain() for you, so you are doing double escaping right now. Make sure to read https://www.drupal.org/node/28984 again.
8. bacnet_block_view(): $usage will always be a TRUE-ish value, so the inline if when assigning to $block does not make sense? Also elsewhere, you often check $usage even if it just has been assigned right the line before.
9. bacnet_block_view(): why are the form_set_error() calls here? You are not in a form validation function here? I think you want to use drupal_set_message() instead.
10. bacnet_get(): this is vulnerable to XSS exploits. You are printing the result body of the request unsanitized to watchdog(). You need to apply check_plain() to the result of var_export() in the pre tags. This is a security blocker.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

1. bacnet_soap_get_value(): could you document @return here so that is clear what comes out of this function? See https://www.drupal.org/coding-standards/docs#return
2. bacnet_get(): this is vulnerable to XSS exploits. You are printing the result body of the request unsanitized to watchdog(). You need to apply check_plain() to the result of var_export() in the pre tags. This is a security blocker.

It's OK, because of "!" placeholder. Seems PAReview tool doesn't know about that.

Cloned module

Reviewed changes implemented per #56

Installed on http://open4energy.com - block reporting updated "now" energy value of 124kW - across all pages.

Yep, those issues were false positives from DrupalPractice sniffer, this is now fixed in the dev version. It clearly says that they could be false positives though, so better trust your own knowledge ;-)

manual review:

• bacnet_get_config(): why are $account and $license set to some obscure values? Please add comments.

But otherwise looks good to me now.

Thanks for your contribution, dbt102!

I updated your account so you can 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 stay 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

Congratulations @dbt102. Looking forward to work with this module soon.