[D7] Webform Feedback

Hi there,

first of all there are quite a few issues to sort out such as indentation, whitespace and correct comment formats.

You can find them all here:

http://ventral.org/pareview/httpgitdrupalorgsandboxprotitude1815672git

Look at the top of the page as well where it tells you about removing version number, removing old CVS $Id tags and more.

I have a few other suggestions from manual review:

• When you append the body of the form it could be better to use a render array because I know someone like me would want to go in to my theme template or a custom module and change classes and html tags. This is a lot easier when using render arrays with prefixes and suffixes.
  
  
• Couldn't you add the css and js files in the info file? (This is just my personal preference in keeping the module file clear, not essential)
  
  
• You want to create a version specific branch and remove the master branch.
  You'll find more instruction on that at the PAReview link.
  
  
• Code outside of functions is best avoided and it's probably best to load the variable in a specific hook or function in which you want to use it.
  
  
• Where you are creating and appending jquery strings couldn't you do that in a seperate function and return it to the block view alter hook like this:

  function webform_feedback_create_js() {
    $webform_addclass = "$('#block-webform-client-block-" . $webform_feedback_id . "').addClass('webform-feedback-block');";
  
    //set the position of the feedback button to the left or right side of the browser.
    $webform_position = variable_get('webform_feedback_position');
    if ($webform_position == 'right') {
      $webform_location = "$('#webform_feedback_popup .fby-screen').addClass('feedback-button-right')";
    }
    else {
     $webform_location = "$('#webform_feedback_popup .fby-screen').addClass('feedback-button-left')"; 
    }
  
    $webform_append = '$("body").append("<div id=\'webform_feedback_popup\'><div id=\'fby-screen\' class=\'fby-screen\'><div   id=\'fby-mask\' class=\'fby-mask\'></div><div id=\'fby-tab-1553\' class=\'fby-tab fby-tab-l\'><a href=\'#block-webform-client-block-' . $webform_feedback_id . '\'></a></div></div></div>");';
  
  $webform = '(function($) {$(document).ready(function() { ' . $webform_addclass . $webform_append . $webform_location . ' }); }(jQuery));', 'inline';
  
  return $webform;
  }
  

  And then in the hook you can simply have:

  $webform = webform_feedback_create_js();
  drupal_add_js($webform);
  

  Sorry about the code looking a bit poor but posting it on here it's hard to avoid text-wrapping with long lines of string appending (but don't forget about the render arrays.)
  
  

Manual Review :

1) There is no hook_uninstall() to remove your custom variables like: webform_feedback , webform_feedback_position.

2) Create a webform_feedback.install file and add hook_uninstall() in it.

1. Your project page is rather sparse. Take a look at Tips for a great project page.
2. in webform_feedback_block_view_alter, $block->bid is the numeric id of the block so you shouldn't be adding 'webform-client-block-' before the webform_feedback variable.
3. Perhaps you should add configure = module admin path to your .info file. That's what allows for the handy "configure links" on the module list.

Hello protitude,

Firstly have you already passed the one time review application stage as icon_links is a full project?

Going on to project review:

The comment in the file doc block for the module file is wrong as it says:
Installation file for the webform_feedback module.
which it is not.

For good practice and clarity you ought to put the webform_feedback_settings form in it's own webform_feedback_form.inc or webform_feedback.admin.inc file.

Change the $a variable in the settings form function to something more self explanatory.

Hello,

The JS file needs to follow the code indentation standards at:
http://drupal.org/node/172169

For JavaScript you should use Drupal behaviours instead of Document ready and looking further into it than before you can pass variables through settings so you can keep all the JavaScript/JQuery in the same file. See the page below for more info:
http://drupal.org/node/756722

For the settings form, It is better to use functions found here to get webforms with blocks turned on and getting their titles than the Database Abstraction Layer directly:
http://drupalcontrib.org/api/drupal/contributions!webform!webform.api.php/7

You could use webform_node_load and go from there.

There is a PHP error which means the module cannot be used where you are forming your processed array on line 24 of the admin inc, Your best bet is to put the array pop function in a seperate function outside of the array map and then add a returned variable version of that function to the array map.

What I mean is something like this:

<?php
function webform_feedback_array_level_pop($array_level) {
  return array_pop($array_level);
}

At the bottom of the page and then like this:

array_map(webform_feedback_array_level_pop($array_level), $field_query));

In the settings form function.

There doesn't seem to be an array level variable? so that makes this function useless.

When I enter <script>alert('XSS');</script> as the title of the webform node I get a nasty little JavaScript pop up, you want to avoid this by reading or re-reading here:
http://drupal.org/writing-secure-code

The root of this is somewhere in the block output.

Also a last thing would be that there is no AJAX popup created maybe you need to document something better or am I completely missing something?

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I'm only seeing minor things at this point.

You have two "core" declarations in your info file.

Could you add "package = Webform" to your info file? That way the "modules" page displays it alongside Webform and other related modules so I don't need to hunt for it. This is in line with the info file standards. "If your module comes with other modules or is meant to be used exclusively with other modules, enter the name of the package here"

Lastly, consider running your images through ImageOptim (Mac only) or something similar. I was able to remove 27.7% from ajax-loader.gif and 52.7% from feedback-tab.png without any noticeable reduction in quality. I know we're talking about less than 10K here, but every byte helps, especially on slow/weak mobile connections. I've attached my reduced versions in case you just want to use those.

Hi, Check
http://ventral.org/pareview/httpgitdrupalorgsandboxprotitude1815672git
and solve all errors/warnings.

You should mainly convert tabs into spaces at webform_feedback.js for code standards.

Except that, code is clear and can be approved.

Will review this today...

Looks pretty straightforward, you have a typo in your README - "bock" should be "block", but otherwise RTBC from me.

Did You consider using jQuery UI dialog for displaying popup?

Well it's been over a month and the code still looks decent.

Thanks for your contribution, protitude!

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.