[D7] cmood

Moving the issue to the right project applications issue queue.

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

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.

Hello polaki_viswanath,

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

1. (*) You don't need to use t() in hook_menu(), this is done automatically
2. (*) Admin path is wrong in info file
3. (*) Mood node fields source is hard-coded (title and body), and does not check if the body field exists nor does take into consideration the language or delta.
4. Maybe it's a good idea to have a setting for selecting what nodes should be checked for moods.
5. Please move all forms in the inc file and keep the main module clean
6. Why would you start with 7.x-2.x branch?
7. Please use confirm_form in your word delete page
8. The UI flow is not friendly. For example saving a word takes me back to admin/cmood, while I was in the word lisk.
9. Please use MENU_LOCAL_ACTION for actions like add links and MENU_LOCAL_TASK for Words/Nodes/weight. This will make it easier to navigate.
10. Use page arguments for edit/delete forms instead of arg(). For example: 'page arguments' => array('cmood_form', 2, 3), will provide the id and the operation as a param.

This review uses the Project Application Review Template.

Thanks,
Mihai

function cmood_word_delete($wid) {
  db_delete('word_with_weight')
    ->condition('wid', $wid)
    ->execute();
  drupal_set_message(t('Your word with weight has been deleted')
    . t('successfully!'), 'status');
  drupal_goto('admin/cmood');
}

function cmood_rank_word_delete($rwid) {
  db_delete('rank_word_with_weight')
    ->condition('rwid', $rwid)
    ->execute();
  drupal_set_message(t('Your rank word with weight has been deleted')
    . t('successfully!'), 'status');
  drupal_goto('admin/cmood/rank_word');
}

These are vulnerable to CSRF exploits. You either need to have a confirmation form or a security token in the URL to prevent DB write requests behind the user's back. See http://epiqo.com/en/all-your-pants-are-danger-csrf-explained

Added Security tag.I have seen that you have addressed the above security issue.

If added, 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 :-)

polaki_viswanath, just a few more things.

1) On cmood_form_submit the text for saving a new word needs to be corrected, now it's:
"Your word with weight has been savedsuccessfully!"
Your code:

 drupal_set_message(t('Your word with weight has been saved')
      . t('successfully!'), 'status');

If the message is shown anyway, you can add it at the end of the function, outside if/else. And there is no need to split the text.
Also 'status' is default for drupal_set_message, so you don't need to use it.

2) You'll need the cmood nodes list as a default local task to be able to navigate back. Just add something like this in hook_menu:

  $items['admin/cmood/nodes'] = array(
    'title' => 'Cmood Nodes',
    'type' => MENU_DEFAULT_LOCAL_TASK,
  );

3) You have repeated code in hook_node_presave and hook_node_insert (add it in a new function?).

Notes:
+ pareview report is clean.
+ tested the functionality with a few nodes that got mood tags fine.

drupal_goto should not be used in a form submit handler.The reason is that potentially other submit handlers might need to be called in order for things to work properly. If you call drupal_goto you would stop the execution flow and make it impossible for the other submit handlers to do their thing.
What you should do instead is to use $form_state['redirect'], that does the go-to for you when submission process is completed.

I'm not sure if this is best practice or a blocker so I won't change the status :)

Hi Polaki

I would like to suggest to reduce the line of code in "cmood.admin.inc". I was looking into the same file and got to know you have used multiple line of code for the options of "weight" at two different places ("cmood_rank_word_form" and "cmood_form"). Which can be done programatically in single method.

Code:
$weight = array(
-20 => t('-20'),
-19 => t('-19'),
-18 => t('-18'),
-17 => t('-17'),
-16 => t('-16'),
-15 => t('-15'),
-14 => t('-14'),
-13 => t('-13'),
-12 => t('-12'),
-11 => t('-11'),
-10 => t('-10'),
-9 => t('-9'),
-8 => t('-8'),
-7 => t('-7'),
-6 => t('-6'),
-5 => t('-5'),
-4 => t('-4'),
-3 => t('-3'),
-2 => t('-2'),
-1 => t('-1'),
0 => t('0'),
1 => t('1'),
2 => t('2'),
3 => t('3'),
4 => t('4'),
5 => t('5'),
6 => t('6'),
7 => t('7'),
8 => t('8'),
9 => t('9'),
10 => t('10'),
11 => t('11'),
12 => t('12'),
13 => t('13'),
14 => t('14'),
15 => t('15'),
16 => t('16'),
17 => t('17'),
18 => t('18'),
19 => t('19'),
20 => t('20'),
);

Hi Polaki
Just to minimize the code you can use the following script.

function getweight($number){
$weight = array();
for($i= -$number;$i<= $number;$i++){
$weight[$i]=t($i);
}
return $weight;
}
$number you can set as per your requirement.
Hope it make sence.

updated git clone url.

Awesome Project! Thanks for the Contribution.
Manual Review: Commit dfb1287

1. cmood.info : Remove this
   ; Information added by Drupal.org packaging script on 2015-1-12 . This will be added automatically by the drupal.org packaging script
2. cmood.install :
   

   /**
    * @file
    * Install, update and uninstall functions for the user module.
    */

   The doc comment is for cmood module.Please update.

3. cmood.module : The file doc comment needs improvement.Also the docs needs improvement at several places.
4. cmood.module : cmood_word_delete_confirm() : In the confirm_form() If the 'destination' query parameter is set in the URL when viewing a confirmation form, that value will be used instead of $path.So we can simply remove $_GET destination parameter.
5. cmood.module : cmood_word_delete_confirm_submit() : Remove the drupal_goto("admin/cmood/mood_word"); The form will be automatically redirected using the form redirect set in cmood_word_delete_confirm.Similarly for the function cmood_rank_word_delete_confirm_submit
6. (+)cmood.module : cmood_get_string_from_node() : I am pretty sure that this functions will throw warning when a node will not have the body field.
7. cmood.admin.inc : cmood_form_submit() . Use the drupal_write_record .This will do the schema validation free for you.Similarly for this function cmood_rank_word_form_submit

Rest seems fine to me.RTBC +1

Thanks! for the updates.
Love to set this to RTBC :)
Review of the 7.x-1.x branch (commit 519b9d4):
Eveything is take care of.Some little tweaks.
1)cmood.admin.inc :
Use of drupal_write_record to cleanup the insert and update logic.Something like that it will cleanup the code. http://cgit.drupalcode.org/block_noresults/commit/?id=a6899b8 This needs a little polish in the functions cmood_form_submit, cmood_rank_word_form_submit.Also remove the drupal_goto from these functions and use $form['redirect'] instead while creating the form.
2)cmood.admin.inc :
cmood_form :

 $form['wid'] = array(
    '#type' => 'hidden',
    '#title' => t('Word Id:'),
    '#default_value' => isset($w_edit['wid']) ? $w_edit['wid'] : '',
  );

Its better to use the #value if we don't need to do some front end operation on the field.Similarly for the function cmood_rank_word_form.Then in the cmood_form_submit we can also remove the is_numeric check from the wid submitted value.
3)
drupal_strlen is better than strlen and similarly drupal_substr
4)
function cmood_menu :

  $items['admin/cmood/nodes'] = array(
    'title' => 'Cmood node listing',
    'description' => 'Shows list of nodes with mood values.',
    'page callback' => 'cmood_node_mood_listing',
    'access arguments' => array('administer cmood'),
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'weight' => 0,
    'file' => 'cmood.admin.inc',
  );

As the page callbck, access arguments and file already defined in the 'admin/cmood' menu so its not needed here.Simply remove it.
5)
These functions are needed only for the admin operations. cmood_word_delete_confirm, cmood_word_delete_confirm_submit , cmood_rank_word_delete_confirm, cmood_rank_word_delete_confirm_submit So it needs to be move to cmood.admin.inc, it will also provide some performance gain, because .module files are called on every page request.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. project page is too short. What is the use case, could you give an example? See also https://www.drupal.org/node/997024
2. cmood_menu(): doc block is wrong. This is a hook implementation and should be documented as such. See https://www.drupal.org/coding-standards/docs#hookimpl
3. cmood_calculate(): Do not use db_select() for simple static queries, use db_query() instead. See https://www.drupal.org/node/310075
4. cmood_calculate(): inline code comments are missing. What does the preg_match_all() match?
5. cmood_get_string_from_node(): so this will only work on nodes with a body field? It might be nice to let the admin configure which text fields on which node type can be taken into account, just an idea.
6. "'#title' => 'Enter your word:',": all user facing text must run through t() for translation.
7. "t('<p>Enter a single word no spaces allowed.</p>')": HTML tags should be outside of t() where possible.
8. cmood_form(): why is the maxlength only 10 characters? Please add a comment.
9. cmood_form(): please don't use obscure variable names such as $arg3. This should be $operation and $word_id instead.

I found a XSS security issue in the code. As part of our git admin training I'm assigning this to naveenvalecha, so that he can take a second look. If he does not find it I'm going to reveal it next week :-). And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

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

Thanks Klausi! :-)
Manual Security Review of the 7.x-1.x branch (commit 519b9d4)::

1. (*) cmood_node_mood_listing : This function has the XSS security issue.Need to use check_plain on $row->title.
2. (*) cmood_node_mood_listing : Pretty sure that this function have an node access by pass problem.you are getting the node titles,so user with 'administer cmood' may see node titles that may not have access to, which is a security problem.you need to add node_access tag in your database query.In general, all
   db_select() against {node} should have this tag unless you have already established that the current user has access to the node. Even revealing the title of the
   node would be considered a security issue.
3. Currently module is saving the mood of every node on the node insert.This should be configurable at the admin configuration,which node types are allowed for the moods.its just an idea
4. Integration of the module with views will be an awesome feature.

The module has two security issues that needs to be addressed.
Steps to Reproduce of 1)
Create a node of any type and enter this in title <script>alert("XSS");</script> and when you open the admin/config page.you will see a nasty popup.Screenshot attached.

fixing tag

Hi polaki_viswanath,

Would like to bring your notice to following points
i. There is no provision to update the mood or the existing nodes. I believe using db_merge instead of db_update in hook_node_presave will solve the issue.
ii. mood are not getting deleted when deleting a particular node.
iii. When i go to add new 'mood word' or 'rank word', menus related to cmoods disappears.

Assigning to myself for the next review.It may be happened tonight.

Automated Review :

Manual Review :

1. (*)Function cmood_admin : This function is vulnerable to XSS exploits.Please do check_plain on $row->name before printing.
2. (*)Function cmood_rank_words : This function is vulnerable to XSS exploits.Please do check_plain on $row->name before printing.
3. Function cmood_word_delete : This function is deleting the word.So for long term also do some log entry like watchdog entry for this.Its a suggestion.Similarly in the function cmood_rank_word_delete, function cmood_node_delete
4. Please add description on these forms admin/cmood, admin/cmood/rank_word , admin/cmood/mood_word using hook_help it will be very useful from End user perspective.Its a suggestion.

Awesome Quick Reply!
Suggestions are always upto you :)
Review of the 7.x-1.x branch (commit f630480):
Considered My above blockers.Not found any more blocker.
But otherwise looks RTBC to me.
Assigning to klausi as he might have time to review.

manual review:

1. cmood_node_presave(): so this will fail for new nodes since there is no node ID yet?
2. cmood_get_weights(): why do you need to convert to string here? Please add a comment.
3. cmood_form_validate(): do not split up the t() call like that, one sentence should be in one t() call, otherwise this is really not working for translators.
4. cmood_node_mood_listing(): Use truncate_utf8() here to shorten the title, see https://api.drupal.org/api/drupal/includes!unicode.inc/function/truncate...

But otherwise looks good to me, so ...

Thanks for your contribution, polaki_viswanath!

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.