Hi,

Ussage: This module helps site admins or adminstrators to calculate the mood of user who submitted a node.
For every node created, this module calculates a mood for the particular node and stores it or updates it.

Site admin must add words for which he wants the mood of content can be calculated. He must also add rank words for increasing the mode by rank number of times.

For example: If site admin adds a mood word "good" with a weigth of +2 and a rank word "very" with weight of +3, Then if a node title or body contains a word or phrase with "very good" then a mood is calculated which equals to 6 (3 * 2 = 6). If the title and body contains only word "good" then a mood will be calculated which equals to 2.

Sample: If a user creates a node which contains a text of "Hi, I am a very good boy. And I have a good smile.", then taking into the above word and rank weights the mood is calculated which equals to (3 * 2) + 2 = 8.

There are no modules available for evaluating the mood of content posted in the drupal site. By the help of this module site admins can know and analyse what kind of content exists or posted in the site.

Link of project: https://www.drupal.org/sandbox/polaki_viswanath/2111973

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/polaki_viswanath/2111973.git cmood
cd cmood

Pareview:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpolakiviswanath211197...

Reviews of other projects:
https://www.drupal.org/node/2325361#comment-9533709
https://www.drupal.org/node/2410637#comment-9533791
https://www.drupal.org/node/2378799#comment-9533947
https://www.drupal.org/node/2411415#comment-9538099
https://www.drupal.org/node/2378799#comment-9568167
https://www.drupal.org/node/2413889#comment-9568323

Comments

naveenvalecha’s picture

Project: Cmood » Drupal.org security advisory coverage applications
Component: Code » module

Moving the issue to the right project applications issue queue.

PA robot’s picture

Status: Needs review » Needs work

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.

mihai_brb’s picture

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

naveenvalecha’s picture

Issue tags: +PAReview: security
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

polaki_viswanath’s picture

Status: Needs work » Needs review
Issue tags: -PAReview: security

Hello All,

All the issues listed in the above comments have been fixed.

Changes made are as below:

1. Removed all the errors and spacing issues suggested by pareview.sh
2. Readme file changed.
3. Confirm forms added to delete functionality.
4. Few forms moved to inc files.
5. Menu and navigation fixed.
6. Arg functions removed.
7. Added 7.x-1.x branch and deleted 7.x-2.x branch and made 7.x-1.x branch default.
8. Suggested .info file changes made.

Thanks for your support

naveenvalecha’s picture

Issue tags: +PAReview: security

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 :-)

mihai_brb’s picture

Status: Needs review » Needs work

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.

polaki_viswanath’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus

Added "PAReview: review bonus"-tag, after manual reviewing three project applications.

Thanks

polaki_viswanath’s picture

Dear mihai_brb

As per your suggession I have updated my module. The changes which I made are as below

1. Adding menu default local task.
2. drupal set message fix. Removing the status argument.
3. Moving the redundant code in node presave and node insert into a new function.

Points which I have not done are as below

1. 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.
Reason is I wanted to show the admin status message even if he edits any mood or rank word and can't be possible if I use drupal_goto before drupal_set_message.

Thanks

polaki_viswanath’s picture

Status: Needs work » Needs review

Hi All
I have made all the changes as suggested.

Thanks

mihai_brb’s picture

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 :)

jaipal’s picture

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'),
);

jyotisankar’s picture

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.

polaki_viswanath’s picture

Dear Jyotisankar and Jaipal

I have modified my code base as you suggested, and thanks for the code snippet.

Thanks

naveenvalecha’s picture

Issue summary: View changes

updated git clone url.

naveenvalecha’s picture

Issue summary: View changes

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

polaki_viswanath’s picture

Dear Naveenvalecha

I have changed my module as per your suggessions. I have checked with the content types with no body and there are no errors and warnings.

Changes made by me as follows:

1. .info file changes removing "Information...." details.
2. Using destination and removing the $_GET and path from module file.
3. @file changes in the .module and .install file.
4. Using db_write_record in the form submissions.

Thanks

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

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.

klausi’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -PAReview: review bonus
FileSize
2.21 KB

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

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.

naveenvalecha’s picture

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.

polaki_viswanath’s picture

Status: Needs work » Needs review

Dear Naveenvalecha and Klausi

I had worked on most of the suggessions made by you. The changes which are made by me are as follows

1. Used drupal_write_record to cleanup the insert and update logic.
2. Changed cmood_form "#default_value" to "#value".
3. Used drupal_strlen and drupal_substr.
4. Removal of page callbck, access arguments and file in cmood_form
5. Moving confirm_forms and corrosponding submit functions to inc files.
6. Project page changes as required with more description and example.
7. doc block and inline comments added where suggested.
8. Adding t() function where nessary and removing html tags in t() function.
9. cmood_form max lenght changes and removal of obscure variable names.
10. Fixed xss issues while displaying node titles.
11. Added settings form for choosing content types.
12. Added node_access tag to the nodes with mood listing.
13. Integration with views module.
14. Code formatting and indentation fixing with pareview.sh

Use of db_query instead of db_select in cmood_calculate is not done due to increase in looping since I needed full array of words and ranks before my logic begins.

Thanks for your valuable reviews.

polaki_viswanath’s picture

Issue summary: View changes
Issue tags: ++PAReview: review bonus

Reviewed 3 more projects.

klausi’s picture

Issue tags: -+PAReview: review bonus +PAReview: review bonus

fixing tag

Chandan Chaudhary’s picture

Status: Needs review » Needs work

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.

polaki_viswanath’s picture

Status: Needs work » Needs review

Hi Chandan

Thanks for your review. I have used db_merge in place of db_update and implemented hook_node_delete for removing the mood when node is deleted.

And regarding the navigation you can use breadcrumb or back button since the edit and add callbacks are of type MENU_LOCAL_ACTION.

Thanks

naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha

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

naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned
Status: Needs review » Needs work

Automated Review :

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

  • 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.

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

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.
polaki_viswanath’s picture

Status: Needs work » Needs review

Dear Naveenvalecha

Thanks for your review. I have fixed the xss issues in cmood_admin and cmood_rank_words functions. I have updated hook_help for better user understanding.

I have not added any log entries in watchdog because according to me if there is any deletion then its under supervision of site admin or superadmin and these need no tracking.

Thanks

naveenvalecha’s picture

Assigned: Unassigned » klausi
Status: Needs review » Reviewed & tested by the community

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.

klausi’s picture

Assigned: klausi » Unassigned
Status: Reviewed & tested by the community » Fixed

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:

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.

polaki_viswanath’s picture

Thanks Klausi for your review.

I have made the changes which you have suggested.
I have checked the error in node_presave, removed typecasting, used one t function in descriptions and used truncate_utf8.

Thanks

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.