Simple Comment Rate

Link to sandbox is missing, please add it to the issue summary.

The response time for a review is now approaching 4 weeks. Get a review bonus and we will come back to your application sooner.

Hello. I spent manual review of your module.

• Please add repository link for not maintainers: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/ventzie/1437006.git simple_comment_rate
• Simple_Comment_Rate.install: @file block is missing.
• Implements hook_schema() - write a dot after end of comment. In other places also.
• Implementation of hook_init(). => Implements hook_init().
• You can see documentation about writing comments here http://drupal.org/node/1095546
• if(isset($_POST["SCR_ajax"]) || !user_access("view comment rates")) - use space between IF operator and conditions
• substr($comment->node_type,13); - use space between arguments.
• Please read http://drupal.org/coding-standards and check your code.

Added git clone --branch 7.x-1.x ventzie@git.drupal.org:sandbox/ventzie/1437006.git

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

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/simple_comment_rate.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
28 | ERROR | String concat is not required here; use a single string instead
--------------------------------------------------------------------------------

FILE: ...tes/all/modules/pareview_temp/test_candidate/simple_comment_rate.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
491 | ERROR | String concat is not required here; use a single string instead

That minor issues by automated review tools should not block a manual review.

MANUAL REVIEW
--------------
1. Is there a reason you need to use $_POST? If you are using AJAX, you should be able to tap into the drupal form functionality for that. I saw this used in several instances.

2. If you need to exit out of the application use drupal_exit() instead of exit(). I saw this in several spots.

3. .module Line 278: Each value in '#options' should be wrapped with t().

AUTO REVIEW
------------
1. Please run the module through coder using the minor mode and fix the returned errors.

2. PAReview online utility only returned two very minor errors: http://ventral.org/pareview/httpgitdrupalorgsandboxventzie1437006git

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

FILE: ...es/all/modules/pareview_temp/test_candidate/simple_comment_rate.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 65 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
 69 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------


FILE: ...w/sites/all/modules/pareview_temp/test_candidate/simple_comment_rate.js
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 28 | ERROR | String concat is not required here; use a single string instead
--------------------------------------------------------------------------------


FILE: ...tes/all/modules/pareview_temp/test_candidate/simple_comment_rate.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
 215 | ERROR | An operator statement must be followed by a single space
 215 | ERROR | There must be a single space before an operator statement
 216 | ERROR | An operator statement must be followed by a single space
 216 | ERROR | There must be a single space before an operator statement
 350 | ERROR | There should be no white space after an opening "("
 350 | ERROR | There must be a single space before an operator statement
 351 | ERROR | There should be no white space after an opening "("
 491 | ERROR | String concat is not required here; use a single string instead
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Manual review:
- the functions name need follow Naming conventions http://drupal.org/coding-standards#naming
simple_comment_rate_UpdateTable
simple_comment_rate_GetCommentRate
- Need space before and after operator "+"
jQuery('#rateupnumb_'+data.cid).text(data.votedup);
jQuery('#ratedownnumb_'+data.cid).text(data.voteddown);
data: "id="+this.id + "&SCR_ajax=1" + "&url=" + get_current_url()

"Adminitrators" spelling typo in README.txt

simple_comment_rate.module
Clean the variables:
@107 $id = $_POST["id"];
@196 $url = $_POST['url'];
@425 $url = $_GET['q'];

In the settings Page:
The message
"Users will be restricted to vote more than one for a certain comment"
should be
"Users will be restricted to vote more than once for a certain comment

The admin-only settings form is best if placed in a .inc file

And some small formatting changes, mostly extra spaces

Attached is a patch ( for the formatting issues only)

Edit: moving the patch to an attachment in next comment (too long)

simple_comment_rate.module
----------------------------------------
line 40: $type = substr($comment->node_type, 13);
why are you doing this substring ?

in function simple_comment_rate_menu()
the title and description should be enclosed in t()

for variable_get('scr_choose_how_to_vote', 0) == 0) ? "ips" : "uids"
I feel it would be a better idea to store either 'ips' or 'uids' in the variable instead of 0/1

line 215, 235 & 261: unused variables.

simple_comment_rate.install
--------------------------------------
line 64 & 72: $query = db_insert('role_permission')
$query is not required.

It appears that this module does the same thing as http://drupal.org/project/rate.
Can you please explain how your module differs from rate ?

Hi,

Sometimes people tend to do a wrong review, you can ignore such comments.

Some more review:
Please make your project application page better. Please check here to create a good project/module description page.

Please change the name of pics folder to images instead and the corresponding changes in the code too. Using name like pics is not the standard, use images which is the standard.

Prefer using "-" instead of "_" in the menu items, so the code line #72 to #100, would change to

/**
 * Implements hook_menu().
 */
function simple_comment_rate_menu() {
  $items['simple-comment-rate'] = array(
    'title' => 'Vote',
    'page callback' => 'simple_comment_rate_vote',
    'access callback' => 'user_access',
    'access arguments' => array('rate comment'),
    'type' => MENU_CALLBACK,
  );

  $items['admin/config/simple-comment-rate'] = array(
    'title' => 'Comment rate',
    'description' => 'Adjust the rating mechanism',
    'position' => 'right',
    'weight' => -5,
    'page callback' => 'system_admin_menu_block_page',
    'access arguments' => array('administer site configuration'),
  );

  $items['admin/config/simple-comment-rate/settings'] = array(
    'title' => 'Simple comment rate settings',
    'description' => 'Adjust the rating mechanism',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('simple_comment_rate_admin_settings'),
    'access arguments' => array('administer site configuration'),
    'type' => MENU_NORMAL_ITEM,
    'file' => 'simple_comment_rate.admin.inc',
  );

  return $items;
}

Would get back with a more in depth review.

Comments are entities in Drupal 7, so I believe that functionality of your module can be obtained with such modules as Fivestar, Rate and many more similar ones. Many of them requires Voting API.

We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org. If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module. (If the existing module is abandoned, please think about taking it over).

What's more - it is really important to inform users how your modules differ from existing ones.

.info file

You should not put such package into the .info file. Package means some functional group and it should be based on module functionality. As long as you provide only one module, not a bunch of modules you should not mention package at all or choose from existing ones.

.install file

Why do you get all roles automatically permission to vote and view comment rates? Even really complex module does not do that. In my opinion it is up to site administrator if he allows it to everyone and as long as permissions are additive they should be granted with users knowledge, not automatically.

It's the same with content types. Consider small site changed on production directly (many people do that, really). An administration can be too slow to disable some permission and content types before user will see and use them.

It is a bad practise.

You also should prefix your variables with full module name. Of source it can result in long variable names, but it is Drupal coding standard. I can imagine that scr module will exists someday because scr means a lot of things :)

.module file

hook_init()

I'm wondering why use these inline .js scripts.
You can pass values to .js file directly:
http://drupal.org/node/756722

simple_comment_rate_vote()

You should probably use drupal_exit() instead of simple exit(). It allows drupal to call hook_exit() for example, your exit() does not.

theme_simple_comment_rate()

You should probably move styles into separate .css file.
What's more with jQuery you are able to bind onmouseover and onmouseout rather than placing it into html element.

.js file

You should use Drupal.behaviors rather that document.ready. What's more I believe it might be easier for you to use $ instead of jQuery. You can look into core block module and block.js file to see how it is done.

Overall

After I vote there is no feedback that the vote was collected. It seems like nothing changes.
Console tells me: Uncaught TypeError: Cannot read property 'cid' of null
Is there something specific I should do to make it work?

Summary

I'm mainly interested why do we need another rating module. Of course your module can provide unique functionality among all others but I believe it should be mentioned in project page.

Hello ventzie,

I meant no offence, and I'm sorry if my post looked like one.

Duplication

If you look into http://ventral.org/pachecklist you will see that second checkbox is about duplication. Reviewers should look for another modules with similar functionality and encourage applicants to join forces. Duplication is really big problem on drupal.org.

If your module is similar to another one it is essential to inform people how these modules differ, personally I made a module that has functionality similar to few others, but explained the differences and no one asked me why another module.

For now we have few modules that offer similar functionality, which I haven't mentioned in my previous post:
http://drupal.org/project/vote_up_down (6.x, 7.x)
http://drupal.org/project/updown (6.x)
http://drupal.org/project/rate (6.x, 7.x)
And in my opinion it is essential to provide information why users should choose your module instead of others and why you can't collaborate with others.

Drupal.org relies on volunteers which provide contrib modules and it is not my main idea to kill your application. But please, try to understand my concerns and if you can post differences it will be good. We can accept your module even if some of others provide similar functionality I would be really happy to set RTBC status for your application (as I did with some other project applications).
But I need to know why do we need more modules with similar functionality.

Drupal.behaviors

It is not an application blocker, so even if you choose to use document.ready your application can be set into RTBC status.

As far as I know Drupal.behaviors should be use instead of document.ready because document.ready does not initialize after dom rebuild with AJAX (D7) or AHAH (D6).
Please, read resources I've based on:
http://drupal.org/node/571888
http://drupal.org/node/1372104
http://drupal.org/node/1458084

In my opinion you never know how comments are loaded, they can be loaded by AJAX and I believe you should provide preferred way to attach js code. Of course, .bind() can do similar thing (or .once) but Drupal.behaviors is the Drupal way in my opinion.

If you think different I will be happy to learn some more if you can provide me with explanation.

And as I said before - it is not an application blocker :)

exit()

I admit that I know a little about Drupal and there are some things that I missed or simply do not know. However I do not understand why you are sarcastic. I know that waiting for review and explaining how things are done in this way instead of other way may be exhaused - my first module was not published and I've done some explanation by myself. But as I said - it is not my intention to block your project application, so I will be grateful if you explained using exit() rather than being sarcastic.

If I can be honest - I've read some serious amount of books and some of them had errors inside (serious errors, even fatal errors while you tried to check the code). Although I'm not saying it's an error or mistake. I have not read this book.

But please, read some posts that encourage users to change from exit() to drupal_exit():
http://drupal.org/node/1420472
http://drupal.org/node/1402948

Even on documentation page there is clear statement that functions should rarely end with exit().
http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_exit/7

As this is official documentation and I do not see a reason why your function should end with exit() I stay with my statement you should use drupal_exit().

I tend to believe more official documentation (even if it is made by people) than books in that case. If you think otherwise it will be really good if you tell us why you think so :)

But this is not an application blocker just like Drupal.behaviors.

Summary

As I said on the beginning, I'm really sorry if my post was discouraging. Sometimes I had similar thoughts when my previous application was put back to needs work because of comment standards (sic!). I've abandoned that module application.
But I've learned on that case and when I made another module I knew what to expect, so the process was easier for me.
It is not essential though to abandon first application ^^

Project application reviews are done by volunteers, there is no such thing as hired reviewers. So probably we will be making a mistakes and asking questions which you think are stupid.
It is also a good exercise before maintaing your module and more weird questions and feature request that will appear in issue queue.

I hope you will not resign sharing your work with the community. It is a great thing and I'm really grateful that there are people like you, because it makes Drupal excellent choice.

Reason to needs work status

The only reason I put your application back to needs work was this part:

After I vote there is no feedback that the vote was collected. It seems like nothing changes.
Console tells me: Uncaught TypeError: Cannot read property 'cid' of null
Is there something specific I should do to make it work?

Maybe I was not specific about it and it made you angry, but I was unable to use your module because of javascript error. I hope you will go back to improving your module (or tell me what I'm doing wrong to get such error) and explaining the differences beetwen you module and already existing ones. Then I will be happy to put your application in RTBC status.

Regards,
Milena

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

folder name added