Problem/Motivation

I had a client who asked me to have an option of sorting a list of documents (view of content) with the help of their popularity on social media sites. For example, he wished to be able to sort some documents on basis of the number of facebook likes, comments, and shares; ascending or descending. I had to write a views plugin for the same. On searching I found no other module doing this. Hence, the contribution.

Description

This is a statistics module. It provides data from various social media sites. The data which is saved per node includes data from:

  1. Facebook : likes count, shares count, comments count & total count.
  2. Twitter : tweets count.
  3. LinkedIn : share count.
  4. Google Plus : plus one count
  5. Total Shares : Facebook total count + tweets count + LinkedIn share count + Google plus one count

The module has integration with Views and Panels (please refer the README.txt).

Project Page

https://drupal.org/sandbox/ajgit/2080003

Project issue queue

https://drupal.org/project/issues/2080003?status=All&categories=All

GIT Repository

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/ajgit/2080003.git

pareview.sh

http://pareview.sh/pareview/httpgitdrupalorgsandboxajgit2080003git (no issues reported).

Reviews of other projects

  1. https://drupal.org/node/2084273#comment-7844671
  2. https://drupal.org/node/2104683#comment-7940573
  3. https://drupal.org/node/2104683#comment-7940739
  4. https://drupal.org/node/2104683#comment-7941271
  5. https://drupal.org/node/2104683#comment-7943507
  6. https://drupal.org/node/2108495#comment-7950237
  7. More reviews

  8. https://drupal.org/node/2107725#comment-7959221
  9. https://drupal.org/node/2080721#comment-7959243
  10. https://drupal.org/node/2093037#comment-7959825
CommentFileSizeAuthor
#11 Social Statistics.png30.4 KBAjitS
#11 Share count.png16.79 KBAjitS
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PA robot’s picture

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.

littledynamo’s picture

Steps of review:

Cloned module into clean install of Drupal 7.23 and enabled.

Automated Tests

Ran module through pareview.sh - no issues found

Duplication

There is another module called Share Count Statistics which seems to be doing a similar thing - creating stats on how many times certain pieces of content have been shared etc. It has views integration and an API for adding new types.

Were you aware of this module? Can you explain how your module is different?

Security

No issues found.
No user input in this module, so no danger of XSS.
Database API used always so no danger of SQL Injection.

Manual Code Review

I noticed a DSM call on line 99 of social_stats.module:

dsm($facebook_data);

No other issues found - t() used consistently for all UI facing text, variables deleted in .install etc

littledynamo’s picture

Status: Needs review » Needs work
AjitS’s picture

Status: Needs work » Closed (duplicate)

@littledynamo : Thanks a lot for the review. I couldn't believe that I was not able to search the module that implemented that functionality. Nevertheless, it was a good learning experience going through the review process. I understand Having too many projects doing the same thing is confusing for developers and site builders. Hence, closing this issue myself as duplicate (feeling pity about myself). My module has some extra features, but I guess I'll talk about them being integrated to the existing module, and see if the maintainer accepts it. Will be back with a new module to get the complete GIT access soon :)

littledynamo’s picture

Its a pity that your module is a duplicate after all your hard work. All the best for your future Drupal efforts.

jcamposanok’s picture

Status: Closed (duplicate) » Needs review

@littledynamo
After reviewing both the Share Count Statistics source code and this one, I think this one is actually better implemented and has more features. Share Count Statistics has many unsolved issues, lacks documentation / install / uninstall support, and does not look like it's actively maintained. I also haven't found a way to actually add new services using the API the module says it provides.

Furthermore:
- This project offers Panels integration while the Share Count Statistics module does not
- Views filters are more fine-grained, allowing to obtain results per service and total
- The statistics are obtained during cron runs instead of every time the node is accessed (allowing the user to configure via cron how often the social services API are accesed/queried, and reducing performance impact on large sites)
- This project allows the user to obtain the count of Facebook likes, comments and shares. The Share Count module only provides the latter
- It also allows to configure the services per node type

I also think merging both projects it's the best way to go, or even start a new branch of the mentioned module using this sandbox project as a base. However, it's important to acknowledge this project does provide different functionalities on its own and offers a greater benefit for the users in general who are looking for this kind of solution.

So in my opinion it shouldn't be considered as a duplicate yet, since they offer similar but not the same functionality.

@AjitS
I recommend you to get in contact with the module maintainer and let him know of this proposal. I would also gladly be interested in cooperating with this project as a co-maintainer.

Please let me know any way I can help, since I'm very interested in the functionality this project provides.

AjitS’s picture

@jcamposanok:
Thank you for the interest in my work.
Please see the issue created at : #2103551: Offering to maintain Share Count Statistics module

AjitS’s picture

Even if I become the maintainer of the module, does that mean I have to go through the review process again to get the full GIT access?
I have received some nice reviews on this module from my colleagues as well. I am hoping to get full GIT access with this. Can this be done? If yes, anything that I should do? I am ready to continue receiving the comments for improvements for the module; and I will happily work through them.
I have experience with maintaining the Soundslides module. Willing to contribute more :)

jcamposanok’s picture

Glad to know you want to keep improving the module and contributing to the community. I think at this moment you basically have two options:

1. Going through the entire review process detailed here in order to get your project promoted from sandbox to full module. You may also want to apply for a review bonus to speed things up
2. Wait for an answer to the possible duplication / fork / branch / integration scenarios with the author of Share Count Statistics (by the way, I already subscribed to that thread to see how things develop)

Even though I really want this module to be further improved and developed, I cannot grant you by myself the permission to promote this to a full module, since I may lose my "vetted user" git status because of the possible duplicity (see "Can I fork any project, even if I don't own or co-maintain it?" at the Drupal.org Git FAQ. The objective of the Drupal community is to encourage collaboration over competition, so I think going for option #2 may be the best way to go.

Meanwhile, you may get more people interested in this Project application and reach a general agreement that this is not a duplicate (as is my opinion). If the author of the other module does not provide you with an answer in about two weeks, we may look also for other options because that would mean the other project is either abandoned or unsupported.

Hope this helps.
Cheers!

dipen chaudhary’s picture

Disclaimer : I work with Ajit so I'm not sure how much my opinion will be weighed in seeing this through.

@jcamposanok has already mentioned detailed differences between the two modules in #6 above.

I just wish to add that I reviewed the initial version of this module and Ajit fixed all the issues in a day which says a lot about his enthusiasm for this module and his willingness to maintain it.

AjitS’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -PAreview: review bonus
FileSize
16.79 KB
30.4 KB

I'm glad to be a part of the community which always encourages to do more :-)
I think I will see this project review through, and see if I get the full GIT access. As of #7, I am planning to take over that module too (in case the module maintainer doesn't reply; marking the project as abandoned). I will do only bug fixes in that module, as I see there are many patches / suggestions which needs review in the module's issue queue.

Here are some more points (advantages) that makes social_stats different the other module (share_count):

My plans maintaining this:
- This project will be actively maintained.
- Will fix whatever bugs I can in share_count module (if I get access to do so). And mention on its project page that there will only be bug fixes made and users should use this(social_stats) module instead. This way will be much better than just taking over the module and creating a new branch with my code. If there is a new branch created in the existing module, the upgrade path would be very confusing for most of the users. Users will get notified that there is an update, and if they update the module the site will brake as the schema of the modules is different. Moreover, the code style is completely different, and users (developers) who read the code would find it confusing while comparing the two branches.
- If I don't get access to the module, then I will be happy to maintain this with full commitment.
- Also, I worked through some issues in D8. So, #2107321: [meta] Drupal 8 port of Social Stats

Enxebre’s picture

Hi AjitS,

Becouse you are using function "node_type_get_types()" in social_stats.admin.inc file you should add "node module" dependecy in .info file.

In line 47:
'#default_value' => $variable ? $variable : NULL.
In my opinion you can achieve the same result in a cleaner way with:
'#default_value' => $variable_get($types->type, NULL)

Just an opinion :)

As an improvement it would be interesteing that the module would work with entities, not just nodes.

Hope this helps!

Regards!

AjitS’s picture

@Enxebre : Thank you for the review.
node is a required module for Drupal. You can check the node.info file to check that it contains
required = TRUE
So, it is not necessary to add it separately as a dependency.
I've made change to line 47, to accommodate the change you've suggested.

I am aware that it will be helpful if this was entity based. However, I'm restricting the first version of the module to node_types. Maybe I'll support entity based statistics in future versions.

Thanks :)

AjitS’s picture

Issue summary: View changes

Updated issue summary.

Enxebre’s picture

Hi AjitS,
thanks for the information about the required attribute.

regards!

Enxebre’s picture

Issue summary: View changes

Updated issue summary.

AjitS’s picture

Issue summary: View changes

Updated issue summary.

AjitS’s picture

Issue tags: +PAreview: review bonus

Adding review bonus.

AjitS’s picture

Issue summary: View changes

Updated issue summary.

saheel.sikilkar’s picture

Status: Needs review » Reviewed & tested by the community

We will be using this in a major media channel's website (10,000 daily authenticated traffic). Site is in testing mode. No known issues till now. Works like charm.

The most fascinating thing is the granular level of control provided by this module.

saheel.sikilkar’s picture

Issue summary: View changes

issue summary updated

AjitS’s picture

@saheel.sikilkar : Thank you for the review. All the best for your project :-)

AjitS’s picture

Issue summary: View changes

Updated issue summary.

AjitS’s picture

Issue summary: View changes

updated issue summary

klausi’s picture

Status: Needs review » Needs work

manual review:

  1. social_stats_install(): no need to set variables on installation as you make use of variable_get() with default values anyway.
  2. social_stats_uninstall(): why are you deleting arbitrary variables here and why are they not prefixed with your module name? This could easily collide with other modules and delete important variables?
  3. social_stats_schema(): primary and foreign keys to the node table are missing. And why do you need to duplicate the node type to the tables? I guess for better querying, but there is no index on the type? Please add a comment.
  4. "$fql_response = @drupal_http_request($request_url);": why do you need the "@" operator here? It is bad to surpress PHP warnings, so don't do that or at least add a comment why you do it.
  5. social_stats_cron(): this will not scale. If I have 1 million nodes in my database this cron job will run into a timeout. Please document on your project page that your module cannot be used on larger sites since it will kill the cron process. You should look into using the queue API to process more nodes. Also note that you are processing all the nodes on every cron run, which is every 5 or 10 minutes on many production sites!
  6. It would be cool to have an admin link on a node to trigger the update manually, just an idea. Or you could schedule an update whenever somebody views a node and no update is scheduled yet. Another use case for the queue API.
  7. social_stats_total_form_render(): all user facing text must run through t() for translation, in this case format_plural().
  8. social_stats_total_form_render(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075

So the variable names is a blocker right now, and also the hook_cron() scalability issue is a t least major. Otherwise this looks pretty good :-)

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

AjitS’s picture

Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Thank you for reviewing the module, and providing the insightful comments.

  1. Variables set in social_stats_install() because in social_stats_cron on line #234
    $start_date = strtotime(variable_get('social_stats_start_date', '-1 day'));
    

    I check the time from which to select the nodes to update the stats.

  2. All variable names are now prefixed with the module name social_stats_, and in social_stats_uninstall I only delete the variables that begin with the module name.
  3. I've defined nid as a primary key. Didn't needed to have a separate node_type column, so removed it.
  4. Removed the "@" operator that suppressed the errors. It was introduced from a patch which I received at #2105121: Project refactoring & suggestions : Change facebook API to newer version and have it as a separate function.
  5. I have added the following to the project page as an important note:
    This module shouldn't be used on large sites because it is cron based and can kill the cron process (timeout). See #2110237: Use Drupal 7 Queue API.

    And I had also mentioned Elysia Cron, as a recommended module on the project page, which would allow the user to change when the cron can be run, even on a production site.

  6. I too was thinking of that sort of functionality while developing this. But, for now I will take this as a feature request, and add this in later releases. The priority now is to get the GIT access as I have other projects which I want to contribute. The improvements will happen gradually. On that note, I like the configuration settings on the Notify module which is also cron based. Would implement something of that sort in this too, but not now.
  7. Used format_plural(), and t() wherever required for user facing text.
  8. Using db_query() now instead of db_select().

I had reviewed 6 projects. So, adding the review bonus back :-)

AjitS’s picture

Issue summary: View changes

Updated issue summary.

AjitS’s picture

Issue summary: View changes

Updated issue summary.

klausi’s picture

Assigned: Unassigned » jthorson
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Looks good to me now! You still don't need to set the variable in hook_install(), because variable_get() will just take the default value (the second parameter) if the variable does not exist in the DB.

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

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

klausi’s picture

Issue summary: View changes

Updated issue summary.

AjitS’s picture

Issue tags: +PAreview: review bonus

Thank you for the review :-)
I have removed the hook_install, and used the default value in variable_get.
Added reviews of three more projects in the issue summary.

klausi’s picture

Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, AjitS!

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.

klausi’s picture

Issue summary: View changes

Updated issue summary.

Status: Fixed » Closed (fixed)

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

kandrupaler’s picture

Hi, this module promises exactly what I want, so I installed it. However, I only see zeros for all shares, which I know is incorrect. I've run cron several times after installing, but no change. What am I doing wrong?

social_stats version: 7.x-1.7
drupal version: 7.38
social sharing buttons: Ridiculosusly Responsive Social Sharing Buttons (does it matter?)