[D7] Comment Deletion

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

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.

Automated Review

Looks good

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Add "configure = admin/config/system/comment_delete" to your .info file
2. in comment_delete_settings_form() can't it be
   '#options' => node_type_get_names(),
   rather callinng function and setting variable ?

Correcting Title and setting priority to Normal as you cannot set priority to critical.

Some notice for you:
In hook_batch_finish(), line 107:

$count = $count + count($cid);

Can be changed to:

$count += count($cid);

Looks good to me - RTBC.

Also would be good if you will fix an indent near a 103-106 lines(Drupal Coding standards).

Regards, Valentine.

Automated Review

Looks fine

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

[Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) For menu item admin/comment_delete_batch change menu type from MENU_DEFAULT_LOCAL_TASK to MENU_CALLBACK, Due to this there is an unwanted link is created on administrator page. Changing status as Need work

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 added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Views Bulk operations also provides the same functionality to list and delete the comments of specific node types.Please add the difference on the project page how its functionality is different from the VBO

Reviewing this....

Automated Review

No. Coder is still giving indentation error.

Please add 2 more space on line 103 to 106.I tried on local and it's working fine after that.

if ($node->cid) {
          $cid = db_select('comment', 'c')
            ->fields('c', array('cid'))
            ->condition('nid', $node->nid, '=')
            ->execute()
            ->fetchCol();

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. One Suggestion is that, on configuration page only show the content type for which, comment is exist. There is so significance for running the batch process for content type for which no comment available.
2. Otherwise, Overall module functionality working fine...Nice 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 added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Now it looks fine. Making it RTBC..

Sorry, but this has quite a few problems, including some obvious code formatting issues!

/**
 * Implements hook_batch().
 */
/**
 * Implements hook_batch_submit().
 */
/**
 * Implements hook_batch_finish().
 */

No such hooks. Please review the documentation guidelines for hook implementations and functions!

    drupal_set_message($count . ' Comments has deleted for the selected content type');

This should be translated.

    drupal_set_message(t('An error @operation with arguments : @args',
    array(
      '@operation' => $error_operation[0],
      '@args' => print_r($error_operation[0],
    TRUE))));

If this is an error, then pass 'error' to drupal_set_message. Also, the message is not a proper sentence.

      $nodes = node_load_multiple(array(), array('type' => $type));

You're using Batch API, which is great, but you're then not actually making use of it properly. Your batch operation will only ever run once. You're loading ALL the nodes of one type in one go, which won't scale: on larger sites, this will simply crash.

  $items['admin/comment_delete_batch'] = array(

It's not good practice to add your own items directly below the 'admin/' path.

Changes implemented according to comments.

Removing review bonus tag, you have not done any manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

Hi Chetan,
Thanks for this!

I went through your code, and found a few inefficient/problematic uses of the t() function. I maintain quite a lot of sites with localized languages, and number 1 issue we face when it comes to translation is that the wrong uses of t() function makes it difficult for other translators to translate the string literals. Read the api doc first if you have not taken a thorough look.

comment_delete.module:93: Use t() function, and do not include HTML inside the t() function string. You can put them outside the t() call, and concatenate strings.

'<pre>' . t('<strong>Note:</strong> Execute Batch for delete Comments for the selected content type.') . '</pre>'

comment_delete.module:105: Why does it store the function name in a variable, and then call it? Can't it call it directly?

node_type_count: Consider renaming this function to a module name-prefixed one. comment_delete_node_type_count()?

There are many other code styling, documentation and best practices issues reported in pareview.

Also, I would reconsider how the module works in terms of usability.

When you delete comments of a specific node type, that is a one-time operation in almost all cases. So, storing the content type names in a variable doesn't make sense. A form submit handler can perfectly execute a batch (call batch_set and batch_process() in sequent). I think it would make more sense if the module offered a simple admin UI, and upon form submission, it immediately executes the batch with the just-submitted data.

Also, a batch process can usually handle many nodes. If there are 20,000 nodes in a site, a better approach would be to add an operation for each node, so the progress counters indicate how many nodes have been processed. You can call comment_delete_multiple on all comments found in a single node.
There's even a function that you can use directly! See comment_node_delete().
A per-node comment delete approach would also make sense if you plan to expose it as an admin option in admin/content page and/or

Hello Chetan Sharma!

Manual Review:

Individual user account
Yes, follows the guidelines for individual user accounts.

No duplication
Yes, doesn't cause module duplication and/or fragmentation.

Master Branch
Yes, follows the guidelines for master branch.

Licensing
Yes, follows the licensing requirements.

3rd party assets/code
Yes, follows the guidelines for 3rd party assets/code.

README.txt/README.md
Yes, follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review
Yes, follows the guidelines for project length and complexity.

Coding style & Drupal API usage
1. (*) Pareview.sh detected some problems in your code.
http://pareview.sh/pareview/httpgitdrupalorgsandboxchetansharma2408403git

2. (*) I've found one problem with funcionality of your module. If there are
more than 5 comments on a nodes of a given content type, only 5 of them will
be deleted. Other comments will remain there.
Limit is set to 5 at your code, see line 142 in a .module file.

3. (*) Function comment_delete_settings_form() (in .admin.inc file) is not hook.
Add comments for function node_type_count() and rename it (function shold be
prefix 'comment_delete').

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.

Works fine for me.

Automated Review

All good here. Pareview.sh is clean but you might want to add some test cases though (SimpleTest).

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Batch API seems to be implemented incorrectly. Not sure how much it has changed since the last reviews were done, but I couldn't get this to work. Looking at the code it seems you are calling batch_set() incorrectly. The batch you return in comment_delete_batch_process() should instead be created in comment_delete_batch_submit() and passed into batch_set(). Here's a good example/explanation of Batch API.
2. comment_delete.module: line 135, comments should be inline with rest of the code.

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 added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Individual User Account?
Yes: Chetan Sharma has an individual User Account.
No duplication?
Views Bulk Operations can do something similar, but the differences were addressed by the author in comment #18.
Master Branch?
Yes: Default branch is set to 7.x-1.x
Licensing?
Yes: Project is following the licensing rules.
3rd party assets/code?
No 3rd party assets or code present.
README.txt/README.md?
README.txt present.
Is the secure code?
Code meets security requirements.
Code long/complex enough for review?
Yes: The code present in this module is enough for review.

Recommendation: In comment_delete.module line #94 ‘Note: Execute Batch for delete Comments for the selected content type.’ should be run through the translation function t().

The module worked for me, looks good, does as advertised.

Marking as RTBC.

Removed one automated review.

manual review:

1. '#suffix' => '<pre><strong>Note:</strong> Execute Batch for delete Comments for the selected content type.</pre>',: all user facing text must run through t() for translation.
2. comment_delete_batch_operation(): why do you unset() $cid here, should not be necessary? Please add a comment or remove it.

Thanks for your contribution, Chetan Sharma!

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.