[D7] Social Share statistics

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

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.

Thanks for your contributions!

Automated Review

Lots of issues identified by the pareview.sh.Need to fix all of them. If you want to configure your sublimetext editor with drupal coding standards then read this blog post to fix this. http://www.qed42.com/blog/sublime-awesome-sauce-drupal

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

No: Follows the guidelines for master branch.

Licensing

May be: Please specify about the facebook icon image.Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Follows the guidelines for in-project documentation and the README Template.Update your project page as well.

Code long/complex enough for review

Yes/No: Follows the guidelines for project length and complexity.

Secure code

Yes. See how to write secure code, list security issues identified.

Coding style & Drupal API usage

1.   $items['node/%/twitter/success'] = array(
         'page callback' => 'social_share_statistics_update',
         'page arguments' => array('twitter'),
         'access arguments' => TRUE,
         'access callback' => TRUE,
         'type' => MENU_CALLBACK,
         
     );
   
     $items['node/%/fb/success'] = array(
         'page callback' => 'social_share_statistics_update',
         'page arguments' => array('fb'),
         'access arguments' => TRUE,
         'access callback' => TRUE,
         'type' => MENU_CALLBACK,
         
     );
   
     $items['node/%/gplus/success'] = array(
         'page callback' => 'social_share_statistics_update',
         'page arguments' => array('gplus'),
         'access arguments' => TRUE,
         'access callback' => TRUE,
         'type' => MENU_CALLBACK,
         
     );

   The permissions are very lossely coupled.This will leads to dos attack becuase the operation is too healthy at this callback.you should add the permissions with this callback.We should add some sort of permission with this callback.

2. social_share_statistics.info : Try to use the css and javascript on specific pages. By using this in .info will load these files at all pages.
   

   stylesheets[all][] = style.css
   scripts[] = js/social_share_statistics_callback.js

3. Its best practice to add hook_help in module.
4. social_share_statistics.module :
   

   function social_share_statistics_buttons_content() {  $fb_count=0;
     $twt_count=0;
     $gplus_count = 0;

   Initialze all these variables in single line.

5. social_share_statistics.module :
   

   function social_share_statistics_buttons_content()    $twitter_share = '<a class="twitter-share-button"
       href="https://twitter.com/share"
       data-url="' . $base_url . '/' . request_path() . '"
       data-via="' . $via . '"
       data-text="' . drupal_get_title() . '"
       data-count="none">
       Tweet
       </a>';

   Use l tag instead.

6. social_share_statistics.module : Move this function social_share_statistics_admin into seperate admin.inc file becauase the code in .module file execute at every request.
7. social_share_statistics.module : function social_share_statistics_admin : $show_shares = array(0 => t('No'), 1 => t('Yes')); Use drupal_map_assoc.Its good to use it.
8. social_share_statistics.module : function social_share_statistics_update : Use the drupal_write_record thats better and then you also get schema validation for free.
9. social_share_statistics.module : function social_share_statistics_buttons_content : As you are printing lot of html data. I suggest you should write the template and pass the variables to them. you will get the full flexibilty to manage the output.
10. (*) Remove all the used variables using variable_del in module .install file.
11. social_share_statistics.module : function social_share_statistics_preprocess_page : Use #attached instead of adding css,js using drupal_add_css and drupal_add_js respectively.

12. function social_share_statistics_ajax_callback($page_callback_result) {
      drupal_set_message(t('success share'));
    }

    I have not seen the usage of this function. If it is using somewhere then specify.

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.

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.

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

Updated title.

Hi Naveen,

Many thanks for the review.

We have fixed all the issues mentioned in the manual review except point 1.
For now, we have kept the url accessible for annonymous user as well so that
1) others can test the code without login,
2) the code works even if someone forgets to configure role in block config.

We could create an admin config setting for this to do it via backend as well.
Please suggest.

Thanks for your contribution.
I have downloaded the module from the given GIT path. In the .info file the below description is present.

stylesheets[all][] = style.css

When gone through the folders and files, looks, 'style.css' doesn't exist in the given version.

Hi Clintu,

Thanks for your review.

We have fixed this. 'style.css' doesn't exist in the module.

I don't know what git branch should be reviewed, there is "master" and "dev"? Please create a 7.x-1.x main branch and delete the others to avoid confusion, see also http://pareview.sh/pareview/httpgitdrupalorgsandboxrinashah2229633git

Hi klausi,

Thanks a lot for the review.

We have updated the git branch as well the automated review link.

We are working on the issues listed on the automated review.

As discussed with @ mpdoandio Removing the security tag and updated the comment as well. We should add some sort of permission with this callback.

We have fixed all the issues found in pareview and also fixed the access permission mentioned by naveenvalecha. Please review.

Awesome!
Please address #11 addressed by klausi
There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732
Review of the 7.x-1.x branch (commit 031c6fa):

./README.txt: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.
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.
Security issue
There is the Cross site scripting XSS issue.See how to write secure code, list security issues identified.Screenshot attached to reproduce the issue.

Address aboves that stops me to set this application to RTBC.

These are minor nutipicks :

1. point 2 from #4. I don't think its needed now.Is it ?
2. Remove text " module" from name in social_stats_statistics.info
3. Wrong @doc comment
   

   /**
    * Implements system_settings_form (Social Share Configuration).
    */

   Use this one instead

   /**
    * Configuration form for the general settings for Social Share Statistics module.
    */

   Similarly wrong doc comment in social_share_statistics-custom-block.tpl.php

4. We does not need 'access callback' => TRUE, in these menu items : node/%/twitter/success , node/%/fb/success, node/%/gplus/success because we already have the access arguements.

Hi Naveen,

Thanks a lot for the review.

We have fixed the security issue as well as nutipicks.

Also we have sorted the branch issue addressed by Klausi.

Please review.

No more critical.Setting this to RTBC.
Some minors :

1. point 2 from #4. I don't think its needed now.Is it ?
2. function social_share_statistics_buttons_content : Use theme_image in place of $items['fb_share'] = '<img style="cursor:pointer;" alt="share this page" src="' . $base_url . '/' . drupal_get_path('module', 'social_share_statistics') . '/facebook-icon.png" onclick="fb_callout();">';
3. function social_share_statistics_update : Use drupal_write_record and merge all the arrays and set only the keys(fb,twitter,gplus) in switch. Awesome! use of drupal_write_record.
4. Its good to implement the hook_help in module.

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

Hi Naveen,

Thanks a lot for your review.

We will work on these minors now.

Assigning to myself for the review now, since it is similar to the module I maintain Social Stats.

Great work on the module! Some issues that needs to be addressed before getting this through.

Manual review

1. (*) I tried the module from the demo URL you've provided above. There seems to be a problem with sharing the node with Google Plus. The counter updates even when the node is not actually shared. And it doesn't deduct the number when I cancel the plus one. This is what I tried out:
   • Click on the Google Plus button. It opens a pop up with options to share the page (like sharing it with particular circles, etc).
   • Close this pop up window without sharing. This will mean that I have publicly plusone'd the node, and not actually shared. This will still update the counter of the total shares. Not sure if you are tracking plusone's too.
   • I can also cancel the plusone on Google Plus at any time by clicking the button again. However, when I do, the total share count and plusone count does not reduce by 1.

   I tried to cancel the plusone on the node thrice to make sure.

2. The instructions in the README file are easy to follow.
3. (+) In social_share_statistics_buttons_content(), it is never recommended to use SELECT * FROM ... in a db_query. Only select data which you need. This will be helpful when the module is used on a site with large number of nodes.
4. Naming convention of the array keys of the array $items could be made more meaningful. For e.g. $items['fb'] could become $items['fb_count'], and so on. Just a recommendation.
5. (*) Rather than passing the markup using the variable, it could be directly used in the template. It is recommended that this function only take care of the logic. The markup should be handled in the template.

   $items['fb_share'] = '<img style="cursor:pointer;" alt="share this page" src="' . $base_url . '/' . drupal_get_path('module', 'social_share_statistics') . '/facebook-icon.png" onclick="fb_callout();">';
   

   Same with the variable $items['g_share'].

6. (+) The standard way in which the templates are written in Drupal are, write markup and embed variables in it. For e.g.
   

   <div class="total_count"><?php print check_plain($items["count_text"])?><label><?php print $items["total_shares"]?></label></div>
   

   rather than what you follow in your module:

   echo '<div class="total_count">' . check_plain($items["count_text"]) . '<label>' . $items["total_shares"] . '</label></div>';
   

   For details please see any template from the core.

7. (*) The stats of a node stay in the database even after the node is deleted. Implement hook_node_delete() to remove them.
8. The module will be very helpful if you expose the data with views.
9. (*) You should mention on the project page that this module only tracks shares that are made using the share buttons that the module provides. If I share the nodes with some other mechanism (say directly copy paste the link to Facebook, Twitter, etc.), those actions / shares are not being counted.
10. Consider renaming the template file and removing the word custom from it.

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.

Once the issues mentioned are fixed, the application is ready to be RTBC'ed again. I would also recommend you to review other projects and get a Review bonus so that the GIT admins review your work right away. Good luck!

Note: I'm not using the Project Application Review Template since it was used before to cover other areas. I have however verified them.

Hi Ajit,

Thanks a lot for the review.

1. We have removed gplus shares count functionality for now as google prohibits any methods to recognize users' clicks on a Google+ button, the data of which is then disclosed, sold, or otherwise shared with other parties.
google + button policy

8. We will start working with the views very soon.

Rest we have fixed all other issues that you addressed.

Please review.

Thanks! No issues blocking as of now. Few issues that you should consider resolving before creating a new release.

1. Its a pity that you had to remove Google Plus altogether. I'm afraid that could reduce the practical usability since the module now only provides "Facebook shares", and "Tweets". Is it possible to add other services, like "LinkedIn" or "Pinterest"? This is however not a blocker, only a suggestion.
2. The module does a great job of tracking the users which shared the content. However, there is no way currently to use the data. It would be good if the views integration could be done before the stable release of the module. This is also a suggestion, and not a blocker.
3. In the function social_share_statistics_buttons_content(), you still use the code
   

   $items['fb-root'] = '<div id="fb-root"></div>';
   

   The markup is not required to be passed in the variable. It could be directly printed in the template.

4. The name of the template could use either underscores, or hyphens. Drupal core uses hyphens. The template name could be be changed to social-share-statistics-block.
5. You should mention on the project page that the module collects data only when the node is shared using the share buttons provided by it
6. Use two spaces in the JavaScript files as well. Refer to Drupal core on how the JS files are written.

Marking as RTBC. Please participate in the review bonus program, so that you get a review from GIT administrator.

Thank you for the contribution!

Thanks Ajit for the suggestions.. Really appreciate it.. :)

We will be integrating the module with views very soon and will start working on adding more social networks.

3. <div id="fb-root"></div> is necessary and so we are not printing it directly in the template.

We will be fixing the rest of the issues..

<div id="fb-root"></div> is necessary and so we are not printing it directly in the template.

Please help me understand what do you mean by this? If the div is necessary always, why not directly have it in template? All you have to do is replace

<?php print $items['fb-root'] ?>

with

<div id="fb-root"></div>

in the template.

Suppose the user customizes the template file to his needs and ends up removing the div.
In that case, i guess it would be safer to keep that in the module file. Pls suggest.

A user could also remove the variable from the template, no?
You should add the div to the template directly, and add a comment above it to mention that it should not be deleted.

P.S - Good job on getting the review bonus.

Hi Ajit,

Thanks :)

Yes that can also be the case..Will fix these minors asap.

manual review:

1. social_share_statistics_block_view(): why do you add inline JS here instead of a dedicated JS file which is much easier to maintain. Please add a comment or move it to a JS file.
2. "$base_url . '/' . request_path()": do not build URLs your self, use url() instead.
3. social_share_statistics_buttons_content(): why do you have to foreach() loop here, you could just do a count query on the database?
4. social_share_statistics_get_nid(): use menu_get_object() instead of arg().
5. "'access arguments' => array('access administration settings'),": that permission does not exist in Drupal core and is also not defined by your module in hook_permission(). Did you mean "administer site configuration"?
6. social_share_statistics_update(): this is 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 also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained
7. "form_set_error("dbi bulk item", $pe->getMessage());": you should not print database errors to the user as it might reveal sensitive information about the database server or whatever. Use watchdog instead or just let the exception bubble up? And why do you use form_set_error()? There is not form here?
8. "echo "Something is wrong";": this is not helpful to the user, I would suggest to just ignore invalid cases? Never use a direct echo, this will destroy your page layout.
9. social_share_statistics-block.tpl.php: check_plain() should not happen in the template, this should be done in social_share_statistics_buttons_content() where you prepare the variables for printing.
10. social_share_statistics-block.tpl.php: do not build the share URL here, this should all be prepared in social_share_statistics_buttons_content() where you should do all the preparation. A template should just arrange stuff, not have any logic how data is assembled.

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

Thanks Klausi for the review.

3. Used foreach() loop as we are adding up the values of the share counts on fb,twitter for the respective nodes.

6. The administrator can configure securing the block to show the share buttons to only authenticated users.
We are setting up the url in js files here. Pls suggest.

We have tried to fix the rest of the issues mentioned.

For 3)
Use something like
SELECT count(fb),count(twitter) FROM {social_share_statistics} WHERE nid = $nid GROUP BY nid;
so you don't need to use foreach.

for 6)
Any user can hit
http://example.com/node/3/twitter/success
OR
http://example.com/node/3/fb/success
else use CSRF to raise share statistics for node id 3
So use security token or something else to avoid this.

Fixed CSRF issue and changed the query.

Please Review.

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

• Coder Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

manual review:

1. social_share_statistics_menu(): "node/%/twitter/success/%": if you use "%node" here instead and pass the argument to the menu callback then you don't need to do arg() or menu_get_object() magic. It is always better to pass dependencies explicitly into functions instead of getting them from global state. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
2. "drupal_get_token('checktoken')": usually the value is something specific to your module, i.e. "social_share_statistics" or similar.
3. "'data-url' => $base_url . '/' . request_path(),": do not build URLs your self, use url() instead.
4. fb.js: where does the token come from, it looks uninitialized here? Am I missing something? Does facebook share counting even work?
5. social_share_statistics_block_view(): all JS settings that you add should be grouped under your module name, not "data". See https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ad...
6. twitter.js: if you get the settings passed into the attach function shouldn't you use that instead of global Drupal.settings?

But that are not critical application blockers, otherwise looks RTBC to me.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

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

• 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

Coding style & Drupal API usage

List of identified issues

1. (+) In general, using #attached is preferred over drupal_add_css, drupal_add_js, and drupal_add_libray. You should also add your JS (social_share_statistics_callback.js) where it is actually required, instead of via the .info. Currently it is loading on every page.
2. You should also really have a hook_help() with some basic info about the module.
3. social-share-statistics-block.tpl.php: you have defined global $base_url but $base_url is not in use. remove it if not required.
4. social_share_statistics_get_nid(): arg() is evil, and should almost always be avoided. As klausi suggested earlier, use menu_get_object() instead of arg().
5. social_share_statistics_update(): drupal_set_message(t('success share')); Not a user friendly message.

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.

Looking at the git history at https://www.drupal.org/node/2229633/commits it looks like all of the recent commits are from Swarnendu-Dutta, which is a different user account? but it is hard to tell what that equates to in volume of code, only one can user can get the git vetted access, so please tell me who should get it.

Thanks a lot for your review.
We have fixed most of the issues addressed by klausi and pushpinder.

We are working on the rest.

The token has been previously initialized in twitter.js file so it dsnt require re-initialzing in fb.js. fb share count works along with tracking of the user who shared.

Please give the git vetted access to the project sandbox account.

Repeating the git commit message "Code Review" many times does not really help your git history. See https://www.drupal.org/node/52287 on how to write meaningful messages.

Me and swarnendu both are the authors of this module. How does it work in case of multiple authors?

In this case, you can just add him as maintainer. See https://www.drupal.org/node/1011698 for more detail.

Please give the git vetted access to the project sandbox account.

so...

Thanks for your contribution, rinashah!

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.